Patch: headless http server launch, Kilo Code (#1120), e2e bridge harness#1201
Merged
Scriptwonder merged 12 commits intoJun 15, 2026
Merged
Conversation
…uilder EditMode test assumptions UnityTypeResolver.FindCandidates treated a short name as ambiguous when an internal type shared it (richer assembly sets), so unqualified generics like List<T>/Dictionary<TKey,TValue> failed to resolve while fully-qualified names worked. Add DisambiguateByIdentity: de-dupe by FullName, then prefer a single public type, then a single core/BCL type, narrowing only to a unique survivor so genuine clashes still report ambiguous. Fix EditMode tests that encoded built-in-pipeline/contract assumptions and only failed under URP + ProBuilder (the package test project uses the built-in pipeline and lacks ProBuilder, so these were never exercised): - ManageGraphics negative tests read result["error"] (ErrorResponse has no "message" field); feature_add uses the handler's "type" param, not "feature_type" - ManageMaterialProperties uses the pipeline-appropriate color property (_BaseColor on URP/HDRP) - MCPToolParameterTests asserts pipeline-appropriate shader name and reads _Smoothness on URP/HDRP - ManageProBuilder get_mesh_info test requests include:"faces" Verified in TestbedMCP (URP 17.2 + ProBuilder 6.0.9): full EditMode suite 887 passed / 0 failed / 46 skipped.
… liveness polling
Convert the local MCP HTTP server launch from a visible-terminal model to a
headless background launch and make startup diagnosable and robust.
- Launch windowless via TerminalLauncher.CreateHeadlessProcessStartInfo;
combined stdout/stderr redirected to a per-port log at
Library/MCPForUnity/Logs/server-launch-{port}.log (truncated each launch)
- Replace the per-launch confirmation dialog with a one-time confirm gated on
the new EditorPrefs key HttpServerLaunchConfirmed; the quiet auto-start path
skips it and does not set the flag
- Prepend platform uv/uvx PATH entries so bare uvx/uv resolves under
GUI-launched Unity's minimal non-login PATH (notably macOS)
- Replace the fixed ~30-attempt reachability waits with open-ended polling
tied to the launched process's liveness (5-minute hard cap), emitting a
tail-of-log failure report when the server dies — in both
HttpAutoStartHandler and McpConnectionSection
- Simplify quit-time cleanup to a handshake-scoped StopManagedLocalHttpServer
so headless servers are not left as invisible orphans
- Transport-aware UI button labels (Connect/Disconnect vs Start/End Session),
a transient "Starting…" state, and clearer lifecycle logging
Tests: TerminalLauncherTests covers the headless start-info contract;
ServerManagementServiceCharacterizationTests covers the one-time-confirm gate,
quiet-path bypass, and per-port log redirection.
…ayDev#1120) Kilo Code v7.0.33+ moved MCP config out of the VS Code extension's globalStorage/mcp_settings.json to a CLI-style kilo.jsonc under ~/.config/kilo, with a new schema (https://app.kilo.ai/config.json): an "mcp" container, type:"remote" for HTTP servers (type:"local" for stdio), and an "enabled" flag. Writing the legacy mcpServers / type:"http" / disabled config left the server showing as stdio + disabled. - KiloCodeConfigurator targets ~/.config/kilo/kilo.jsonc on every OS and declares the new format (mcp container, type:remote/local, enabled:true, $schema) - Generalize the per-client HTTP "type" override: replace the UsesStreamableHttpType bool with McpClient.HttpTypeValue (Cline/Roo => "streamableHttp", Kilo => "remote", default "http"); add StdioTypeValue, ServerContainerKey and SchemaUrl fields - ConfigJsonBuilder honors ServerContainerKey ("mcp") and writes a root $schema; JsonFileMcpConfigurator.CheckStatus/Unregister read/remove from the configured container so status detection and teardown work for Kilo - Replace StreamableHttpTypeTests with ClientConfigFormatTests covering Kilo's remote/mcp/enabled format, Cline's streamableHttp, and generic http Verified: 5 new tests + 13 existing config tests pass (EditMode, Unity 2021.3).
A one-command, no-API-key end-to-end gate for the Python<->Unity bridge, runnable locally and in CI. - tools/local_harness.py: boots a headless Hub-licensed Editor (or attaches with --reuse) and runs smoke + EditMode + PlayMode legs over the bridge, aggregating JUnit; exit codes 0-5 (pass / regression / unreachable / no-compile / no-license / no-editor) - Server/tests/e2e/bridge_smoke.py: deterministic no-LLM contract driver over the real wire path; the no-LLM counterpart to claude-nl-suite.yml - .github/workflows/e2e-bridge.yml: PR gate booting headless Unity in CI; self-skips (warns) when Unity license secrets are absent - .github/workflows/python-tests.yml: run the hermetic harness unit tests and add tools/** to the path triggers - tools/tests/test_local_harness.py: 69 hermetic unit tests for the harness's Unity-free decision logic (discovery, version resolution, exit-code mapping) - Document the harness in CLAUDE.md and the contributor docs
- Server/tests/test_tool_test_symmetry.py discovers every module exposing an @mcp_for_unity_tool and fails CI if it is not referenced by any test - A KNOWN_UNTESTED quarantine lets pre-existing gaps (execute_menu_item, manage_shader, manage_tools) skip rather than fail; a second test fails if a quarantine entry is stale, so the list can only shrink Operationalizes the CLAUDE.md rule that every new tool ships with a test.
pyproject.toml is already at 9.7.1; the lockfile lagged at 9.6.6.
Contributor
📝 WalkthroughWalkthroughAdds a 1,610-line Python headless E2E test harness ( ChangesHeadless harness, HTTP server launch, client config, and CI
Sequence Diagram(s)sequenceDiagram
participant GHA as GitHub Actions (e2e-bridge.yml)
participant Harness as tools/local_harness.py
participant DockerLauncher as DockerLauncher
participant Unity as Unity Editor (headless)
participant Smoke as bridge_smoke.py
participant UTF as MCP run_tests/get_test_job
GHA->>GHA: Stage Unity license (ULF or EBL)
GHA->>GHA: Warm up project (Library import)
GHA->>Harness: python3 tools/local_harness.py --ci --legs smoke,editmode,playmode
Harness->>DockerLauncher: docker run unity-mcp container
DockerLauncher->>Unity: McpCiBoot.StartStdioForCi batch
Unity-->>Harness: status file written (port, instanceID)
Harness->>Harness: TCP probe → ReadyInfo
Harness->>Harness: compile_probe via read_console
Harness->>Smoke: subprocess bridge_smoke.py --junit
Smoke->>Unity: send_command_with_retry (bridge commands)
Unity-->>Smoke: responses
Smoke-->>Harness: LegOutcome (exit 0/1/2)
Harness->>UTF: run_tests (EditMode)
UTF->>Unity: MCP run_tests command
Unity-->>UTF: job_id
UTF->>UTF: poll get_test_job
UTF-->>Harness: LegOutcome
Harness->>UTF: run_tests (PlayMode, retry once)
UTF-->>Harness: LegOutcome
Harness->>Harness: merge_junit + write_reports
Harness->>GHA: exit code 0–5
GHA->>GHA: upload reports/junit-*.xml
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…roject runtime artifacts The 8 EditMode/Tools test scripts were tracked without their .meta files, so every contributor's Unity regenerated them with random GUIDs (perpetual untracked churn). Commit the metas so Unity reuses a stable GUID. Also ignore the MCP bridge / UIToolkit runtime artifacts (Assets/UnityMCP, Assets/UI) the test project emits when run.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR delivers a set of “quick-win” reliability improvements across the Unity ↔ Python bridge, including headless local HTTP server lifecycle management, updated Kilo Code MCP config generation (new kilo.jsonc schema), reflection-type ambiguity reductions, and a deterministic CI-gated E2E harness that boots headless Unity and runs smoke/EditMode/PlayMode legs.
Changes:
- Launch the local HTTP server headless with per-port launch logs, first-run confirmation gating, and process-liveness-based startup polling.
- Update MCP client config generation to support Kilo Code’s new
kilo.jsoncformat and generalize per-client HTTPtypeoverrides. - Add a deterministic E2E bridge smoke driver + a reusable Python harness (
tools/local_harness.py) and wire it into CI.
Reviewed changes
Copilot reviewed 38 out of 43 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| website/docs/contributing/testing.md | Documents the local headless harness usage, flags, and exit codes. |
| website/docs/contributing/dev-setup.md | Links dev setup docs to the harness entrypoint and usage section. |
| tools/tests/test_local_harness.py | Adds hermetic unit tests for the harness’ pure helpers and wire-shape handling. |
| tools/local_harness.py | Introduces the headless Unity boot + bridge wait + smoke/EditMode/PlayMode runner with JUnit output. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.cs | Makes shader/property assertions pipeline-aware (Built-in vs URP/HDRP). |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSceneValidationTests.cs.meta | Adds Unity meta for new/updated test asset. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSceneTemplateTests.cs.meta | Adds Unity meta for new/updated test asset. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSceneMultiSceneTests.cs.meta | Adds Unity meta for new/updated test asset. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageProBuilderTests.cs | Updates mesh info request to include face data explicitly. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePrefabsStageTests.cs.meta | Adds Unity meta for new/updated test asset. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePhysicsTests.cs.meta | Adds Unity meta for new/updated test asset. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialPropertiesTests.cs | Makes material color property checks pipeline-aware (_Color vs _BaseColor). |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGraphicsTests.cs.meta | Adds Unity meta for new/updated test asset. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGraphicsTests.cs | Aligns tests with error payload shape and updated parameter names. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageEditorUndoTests.cs.meta | Adds Unity meta for new/updated test asset. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ExecuteCodeTests.cs.meta | Adds Unity meta for new/updated test asset. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/Server/TerminalLauncherTests.cs | Adds coverage for new headless process start info creation + quoting/redirect behavior. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/Characterization/ServerManagementServiceCharacterizationTests.cs | Adds characterization coverage for first-run confirmation gating and per-port launch logs. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/ClientConfigFormatTests.cs.meta | Adds Unity meta for new test file. |
| TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/ClientConfigFormatTests.cs | Adds tests pinning Kilo Code’s new config format and per-client HTTP type rules. |
| TestProjects/UnityMCPTests/.gitignore | Ignores runtime artifacts generated by bridge/UI Toolkit in the Unity test project. |
| Server/uv.lock | Bumps the editable server package version metadata. |
| Server/tests/test_tool_test_symmetry.py | Adds a guard ensuring every tool module has some test reference (with quarantine list). |
| Server/tests/e2e/README.md | Documents deterministic bridge E2E driver usage and CI behavior. |
| Server/tests/e2e/bridge_smoke.py | Adds deterministic no-LLM E2E smoke script driving the live Unity bridge. |
| MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs | Improves HTTP local server start UX (“Starting…”), process-aware polling, and remote connect/disconnect labeling. |
| MCPForUnity/Editor/Services/ServerManagementService.cs | Implements headless server launch, per-port launch logs, PATH fixups, and launch-process liveness reporting. |
| MCPForUnity/Editor/Services/Server/TerminalLauncher.cs | Adds headless process start info builder with stdout/stderr redirection to a log file. |
| MCPForUnity/Editor/Services/Server/ITerminalLauncher.cs | Extends terminal launcher interface with headless launch API. |
| MCPForUnity/Editor/Services/McpEditorShutdownCleanup.cs | Ensures Unity-launched headless local servers are stopped on editor quit via managed handshake. |
| MCPForUnity/Editor/Services/IServerManagementService.cs | Extends server management interface with launch-log and liveness/failure-report APIs. |
| MCPForUnity/Editor/Services/HttpAutoStartHandler.cs | Aligns auto-start polling with process-liveness + hard-cap semantics and improves logging. |
| MCPForUnity/Editor/Models/McpClient.cs | Adds client-specific config schema knobs (HttpTypeValue, container key, $schema, etc.). |
| MCPForUnity/Editor/Helpers/UnityTypeResolver.cs | Adds identity-based disambiguation for common generic/BCL type names. |
| MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs | Implements per-client container/type overrides + optional root $schema. |
| MCPForUnity/Editor/Constants/EditorPrefKeys.cs | Adds first-run HTTP server launch confirmation preference key. |
| MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs | Makes config read/write/removal honor per-client server container keys. |
| MCPForUnity/Editor/Clients/Configurators/KiloCodeConfigurator.cs | Updates Kilo Code to new kilo.jsonc schema and paths. |
| MCPForUnity/Editor/Clients/Configurators/ClineConfigurator.cs | Switches to explicit HttpTypeValue=streamableHttp override. |
| CLAUDE.md | Documents the harness entrypoint, flags, and exit-code contract for contributors. |
| .gitignore | Ignores .mcp.json. |
| .github/workflows/python-tests.yml | Runs harness unit tests (tools/tests) in CI when Server/tools change. |
| .github/workflows/e2e-bridge.yml | Adds CI-gated deterministic E2E bridge workflow using the harness. |
Files not reviewed (3)
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/ClientConfigFormatTests.cs.meta: Generated file
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ExecuteCodeTests.cs.meta: Generated file
- TestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageEditorUndoTests.cs.meta: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+45
to
+52
| def _is_referenced(stem: str) -> bool: | ||
| pattern = re.compile(rf"\b{re.escape(stem)}\b") | ||
| for test_file in TESTS_DIR.rglob("test_*.py"): | ||
| if test_file.resolve() == Path(__file__).resolve(): | ||
| continue | ||
| if pattern.search(test_file.read_text(encoding="utf-8")): | ||
| return True | ||
| return False |
| if sec: | ||
| roots.append(sec) | ||
|
|
||
| best: tuple[tuple[int, int, str], str, str] | None = None # ((patch, suffix-as-key), binary, dir_version) |
Comment on lines
+899
to
+900
| image = os.environ.get("UNITY_IMAGE", "") | ||
| workspace = Path(os.environ.get("GITHUB_WORKSPACE", str(REPO_ROOT))) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/e2e-bridge.yml:
- Around line 51-59: Replace version tag references with full commit SHAs for
all third-party actions (actions/checkout, astral-sh/setup-uv, and
actions/upload-artifact) to pin them to immutable commits instead of mutable
version tags. Additionally, add `persist-credentials: false` to the checkout
action's `with` section to disable credential persistence for improved security.
Apply these changes consistently across all three action invocations in the
workflow file.
In `@tools/local_harness.py`:
- Around line 1351-1353: Add a fail-fast guard immediately after the parse_legs
function call that validates the legs list is not empty. After assigning the
result of parse_legs(args.legs) to the legs variable, check if legs is empty and
exit with an error message before proceeding to the args.ci condition. This
prevents the script from continuing execution with an invalid or misspelled leg
value that results in an empty legs list, which would otherwise run no legs and
exit successfully.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15ddbb33-9e84-49a0-844b-40b4b743eacb
⛔ Files ignored due to path filters (1)
Server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
.github/workflows/e2e-bridge.yml.github/workflows/python-tests.yml.gitignoreCLAUDE.mdMCPForUnity/Editor/Clients/Configurators/ClineConfigurator.csMCPForUnity/Editor/Clients/Configurators/KiloCodeConfigurator.csMCPForUnity/Editor/Clients/McpClientConfiguratorBase.csMCPForUnity/Editor/Constants/EditorPrefKeys.csMCPForUnity/Editor/Helpers/ConfigJsonBuilder.csMCPForUnity/Editor/Helpers/UnityTypeResolver.csMCPForUnity/Editor/Models/McpClient.csMCPForUnity/Editor/Services/HttpAutoStartHandler.csMCPForUnity/Editor/Services/IServerManagementService.csMCPForUnity/Editor/Services/McpEditorShutdownCleanup.csMCPForUnity/Editor/Services/Server/ITerminalLauncher.csMCPForUnity/Editor/Services/Server/TerminalLauncher.csMCPForUnity/Editor/Services/ServerManagementService.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csServer/tests/e2e/README.mdServer/tests/e2e/bridge_smoke.pyServer/tests/test_tool_test_symmetry.pyTestProjects/UnityMCPTests/.gitignoreTestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/ClientConfigFormatTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/ClientConfigFormatTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/Characterization/ServerManagementServiceCharacterizationTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Services/Server/TerminalLauncherTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ExecuteCodeTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/MCPToolParameterTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageEditorUndoTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGraphicsTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageGraphicsTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageMaterialPropertiesTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePhysicsTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManagePrefabsStageTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageProBuilderTests.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSceneMultiSceneTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSceneTemplateTests.cs.metaTestProjects/UnityMCPTests/Assets/Tests/EditMode/Tools/ManageSceneValidationTests.cs.metatools/local_harness.pytools/tests/test_local_harness.pywebsite/docs/contributing/dev-setup.mdwebsite/docs/contributing/testing.md
Comment on lines
+51
to
+59
| - uses: actions/checkout@v4 | ||
| if: steps.detect.outputs.unity_ok == 'true' | ||
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - uses: astral-sh/setup-uv@v4 | ||
| if: steps.detect.outputs.unity_ok == 'true' | ||
| with: | ||
| python-version: "3.11" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wf=".github/workflows/e2e-bridge.yml"
echo "Action references in ${wf}:"
rg -n '^\s*uses:\s*' "$wf"
echo
echo "Refs that are NOT pinned to a 40-char commit SHA:"
python - <<'PY'
import pathlib, re
p = pathlib.Path(".github/workflows/e2e-bridge.yml")
for i, line in enumerate(p.read_text(encoding="utf-8").splitlines(), 1):
m = re.match(r'\s*uses:\s*([^\s]+)', line)
if not m:
continue
ref = m.group(1)
tag = ref.split('@', 1)[1] if '@' in ref else ''
if not re.fullmatch(r'[0-9a-fA-F]{40}', tag):
print(f"Line {i}: {ref}")
PYRepository: CoplayDev/unity-mcp
Length of output: 251
🏁 Script executed:
#!/bin/bash
# Read the workflow file to see actual content around the referenced lines
echo "=== Lines 45-65 (around lines 51-59) ==="
sed -n '45,65p' .github/workflows/e2e-bridge.yml
echo
echo "=== Lines 185-195 (around line 190) ==="
sed -n '185,195p' .github/workflows/e2e-bridge.yml
echo
echo "=== All 'uses:' lines in the file ==="
grep -n 'uses:' .github/workflows/e2e-bridge.ymlRepository: CoplayDev/unity-mcp
Length of output: 1404
Pin third-party actions to immutable SHAs and disable checkout credential persistence.
At lines 51, 56, and 190, action refs are tag-pinned (@v4) instead of SHA-pinned, and the checkout action at line 51 retains default credential persistence. Replace version tags with full commit SHAs and add persist-credentials: false to checkout:
Example fix for checkout (lines 51–54)
- uses: actions/checkout@<40-char-sha>
if: steps.detect.outputs.unity_ok == 'true'
with:
fetch-depth: 0
persist-credentials: falseApply the same SHA pinning pattern to astral-sh/setup-uv@v4 (line 56) and actions/upload-artifact@v4 (line 190).
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 51-54: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 51-51: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 56-56: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/e2e-bridge.yml around lines 51 - 59, Replace version tag
references with full commit SHAs for all third-party actions (actions/checkout,
astral-sh/setup-uv, and actions/upload-artifact) to pin them to immutable
commits instead of mutable version tags. Additionally, add `persist-credentials:
false` to the checkout action's `with` section to disable credential persistence
for improved security. Apply these changes consistently across all three action
invocations in the workflow file.
Source: Linters/SAST tools
- local_harness: fail fast (exit 2) on empty --legs and on --ci without UNITY_IMAGE, instead of silently exiting green / crashing with an unhandled CalledProcessError - local_harness: correct the nearest-patch `best` type annotation (key is (patch:int, suffix:str), not a 3-tuple) - test_tool_test_symmetry: fix the module docstring to match behavior (only test_*.py files are scanned; non-test_*.py scripts like tests/e2e/bridge_smoke.py do not count toward coverage) Deferred CodeRabbit's SHA-pin suggestion for e2e-bridge.yml: the repo pins actions by tag (@v4/@v6), not SHA, so pinning one workflow would be inconsistent — left as a repo-wide policy decision.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
Server/tests/test_tool_test_symmetry.py (1)
46-53:⚠️ Potential issue | 🟠 Major | ⚡ Quick winText search can falsely mark a tool as “covered.”
_is_referencedtreats any word match as coverage, including mentions in comments/docstrings/strings (Line 47–Line 52). That allows untested tools to pass the guard by accidental text mention, which weakens the policy this file is meant to enforce.Use AST-based matching for real symbol usage/imports instead of raw regex content search.
Suggested direction (AST-based check)
+import ast import re from pathlib import Path @@ def _is_referenced(stem: str) -> bool: - pattern = re.compile(rf"\b{re.escape(stem)}\b") for test_file in TESTS_DIR.rglob("test_*.py"): if test_file.resolve() == Path(__file__).resolve(): continue - if pattern.search(test_file.read_text(encoding="utf-8")): - return True + src = test_file.read_text(encoding="utf-8") + tree = ast.parse(src, filename=str(test_file)) + for node in ast.walk(tree): + if isinstance(node, ast.Import): + if any(alias.name.split(".")[-1] == stem for alias in node.names): + return True + elif isinstance(node, ast.ImportFrom): + if node.module and node.module.split(".")[-1] == stem: + return True + if any(alias.name == stem for alias in node.names): + return True return False🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Server/tests/test_tool_test_symmetry.py` around lines 46 - 53, The _is_referenced function currently uses regex pattern matching on raw file text content to detect tool references, which causes false positives from mentions in comments, docstrings, and string literals. Replace the regex-based search with AST-based matching to properly detect actual imports and symbol usage. Parse each test file using the ast module, then traverse the AST to look for genuine import statements and name references that actually use the tool, rather than searching for text matches in the file content. This ensures only real code usage counts as coverage and prevents untested tools from falsely passing the guard.tools/local_harness.py (4)
1529-1532:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake compile failures blocking even for non-strict PlayMode.
--strict-playmodeshould only control PlayMode test failures. A compile-probe failure is a project setup failure, so--legs playmodeshould still exit 3 instead of green.Suggested fix
if "playmode" in legs: if not compile_ok: - outcomes.append(LegOutcome("playmode", "fail", blocking=bool(args.strict_playmode), + outcomes.append(LegOutcome("playmode", "fail", blocking=True, detail="project does not compile", exit_code=3))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/local_harness.py` around lines 1529 - 1532, The compile failure in PlayMode is only marked as blocking when args.strict_playmode is true, but compile failures should always be blocking regardless of the strict mode flag. In the LegOutcome instantiation where detail is set to "project does not compile", change the blocking parameter from bool(args.strict_playmode) to True. This ensures that compile failures always cause exit code 3, since these are project setup failures rather than PlayMode test failures that should be controlled by the --strict-playmode flag.
1384-1386:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the status directory when
--keep-alivepreserves the editor.With default local settings,
--keep-aliveskips editor teardown but still deletes the temp status directory infinally. That leaves a live bridge without its discovery file.Suggested fix
else: status_dir = Path(tempfile.mkdtemp(prefix="unity-mcp-harness-")) - owns_status_dir = True + owns_status_dir = not args.keep_alive + if args.keep_alive: + print(f"== keep-alive: preserving status dir {status_dir} ==")Also applies to: 1393-1404
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/local_harness.py` around lines 1384 - 1386, The status directory created in the else block at lines 1384-1386 should be preserved when --keep-alive mode is active, since this flag also preserves the editor. Modify the cleanup logic in the finally block at lines 1393-1404 to conditionally skip deletion of the status directory when --keep-alive is enabled. Check the --keep-alive flag before deleting the status directory so that the discovery file remains available for the live bridge.
678-678:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove or wire
--native-editmodebefore exposing it.
args.native_editmodeis parsed but never read, so the flag is a silent no-op and can produce a green run without the requested native EditMode leg.Suggested fix option: fail fast until implemented
args = build_arg_parser().parse_args(argv) + if args.native_editmode: + print("::error:: --native-editmode is not implemented by local_harness.py yet") + return 2 + legs = parse_legs(args.legs)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/local_harness.py` at line 678, The `--native-editmode` argument is added to the argument parser but `args.native_editmode` is never actually read or used anywhere in the code, making the flag a silent no-op. Remove the `add_argument` call for the `--native-editmode` flag entirely, or if the feature is planned for future implementation, add a validation check immediately after argument parsing that raises an error with a clear message stating the feature is not yet implemented if a user attempts to use the `--native-editmode` flag (when `args.native_editmode` is True). Choose the approach based on whether the feature is planned or should be removed.
1198-1228:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse failed result rows when deciding the UTF leg outcome.
This builds failed JUnit cases from row state, but the final outcome only checks
summary.failed. If the summary is missing or inconsistent, failed rows can still return a passing leg.Suggested fix
rows = result.get("results") if isinstance(result, dict) else None + row_failed = 0 if isinstance(rows, list) and rows: for r in rows: if not isinstance(r, dict): continue rname = str(r.get("fullName") or r.get("name") or f"{mode}.test") rtime = float(r.get("durationSeconds", 0.0) or 0.0) state = str(r.get("state") or "") if state.lower() in ("failed", "error"): + row_failed += 1 fmsg = str(r.get("message") or "") + "\n" + str(r.get("stackTrace") or "") suite.cases.append(JUnitCase(name=rname, time_s=rtime, failure=fmsg.strip())) @@ - if failed > 0: + effective_failed = failed or row_failed + if effective_failed > 0: return LegOutcome(name, "fail", blocking=blocking, - detail=f"{failed}/{total} {mode} tests failed", exit_code=1, + detail=f"{effective_failed}/{total or len(suite.cases)} {mode} tests failed", exit_code=1, junit_suite=suite)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/local_harness.py` around lines 1198 - 1228, The leg outcome decision at the end of the code only checks the summary.failed count, but failed test rows are being added to suite.cases independently of the summary. If the summary is missing or inconsistent, failed rows can still result in a passing leg. Modify the final outcome check before returning LegOutcome to verify not just if failed > 0, but also whether any cases in suite.cases have failures set. The leg should return a "fail" outcome if either the summary indicates failures or if any individual test case in suite.cases has a failure message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/local_harness.py`:
- Around line 1351-1354: The current validation in the parse_legs guard only
checks for completely empty input, but silently drops invalid values in mixed
input like "--legs smoke,edimode". Enhance the validation after calling
parse_legs to detect partially invalid inputs by comparing the number of parsed
legs against the number of input legs provided. If parse_legs returns fewer legs
than were input, reject the entire command with an error message indicating that
invalid values were detected. Consider having parse_legs also return information
about which specific values were invalid so you can report them back to the user
for clarity.
---
Outside diff comments:
In `@Server/tests/test_tool_test_symmetry.py`:
- Around line 46-53: The _is_referenced function currently uses regex pattern
matching on raw file text content to detect tool references, which causes false
positives from mentions in comments, docstrings, and string literals. Replace
the regex-based search with AST-based matching to properly detect actual imports
and symbol usage. Parse each test file using the ast module, then traverse the
AST to look for genuine import statements and name references that actually use
the tool, rather than searching for text matches in the file content. This
ensures only real code usage counts as coverage and prevents untested tools from
falsely passing the guard.
In `@tools/local_harness.py`:
- Around line 1529-1532: The compile failure in PlayMode is only marked as
blocking when args.strict_playmode is true, but compile failures should always
be blocking regardless of the strict mode flag. In the LegOutcome instantiation
where detail is set to "project does not compile", change the blocking parameter
from bool(args.strict_playmode) to True. This ensures that compile failures
always cause exit code 3, since these are project setup failures rather than
PlayMode test failures that should be controlled by the --strict-playmode flag.
- Around line 1384-1386: The status directory created in the else block at lines
1384-1386 should be preserved when --keep-alive mode is active, since this flag
also preserves the editor. Modify the cleanup logic in the finally block at
lines 1393-1404 to conditionally skip deletion of the status directory when
--keep-alive is enabled. Check the --keep-alive flag before deleting the status
directory so that the discovery file remains available for the live bridge.
- Line 678: The `--native-editmode` argument is added to the argument parser but
`args.native_editmode` is never actually read or used anywhere in the code,
making the flag a silent no-op. Remove the `add_argument` call for the
`--native-editmode` flag entirely, or if the feature is planned for future
implementation, add a validation check immediately after argument parsing that
raises an error with a clear message stating the feature is not yet implemented
if a user attempts to use the `--native-editmode` flag (when
`args.native_editmode` is True). Choose the approach based on whether the
feature is planned or should be removed.
- Around line 1198-1228: The leg outcome decision at the end of the code only
checks the summary.failed count, but failed test rows are being added to
suite.cases independently of the summary. If the summary is missing or
inconsistent, failed rows can still result in a passing leg. Modify the final
outcome check before returning LegOutcome to verify not just if failed > 0, but
also whether any cases in suite.cases have failures set. The leg should return a
"fail" outcome if either the summary indicates failures or if any individual
test case in suite.cases has a failure message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6ffa6f9-f91e-4ac8-a437-99d15f8e290b
📒 Files selected for processing (2)
Server/tests/test_tool_test_symmetry.pytools/local_harness.py
parse_legs stays a lenient normalizer (test_drops_unknown_legs), but main() now validates the raw --legs input against the shared ALLOWED_LEGS and exits 2 on any unknown value -- so a typo like `smoke,edimode` fails loudly instead of silently dropping the bad leg and running a subset.
Scriptwonder
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Triage quick-wins plus a deterministic end-to-end test harness for the Python↔Unity bridge. Everything is a bug fix, test infrastructure, or docs — nothing gated behind flags.
Type of Change
Changes Made
kilo.jsoncMCP format (~/.config/kilo,mcpcontainer,type:"remote",enabled) instead of the legacymcp_settings.json. Generalizes the per-client HTTPtypeoverride.List<T>,Dictionary<…>); fix URP/ProBuilder EditMode test assumptions.tools/local_harness.py(boots headless Unity, runs smoke/EditMode/PlayMode legs over the bridge) +Server/tests/e2e/bridge_smoke.py(deterministic no-LLM driver) +.github/workflows/e2e-bridge.ymlCI gate.Compatibility / Package Source
file:(local dev checkout)Packages/packages-lock.json: N/ATesting/Screenshots/Recordings
cd Server && uv run pytest tests/ -v) — 1262 passedDocumentation Updates
tools/UPDATE_DOCS_PROMPT.md(recommended)Related Issues
Fixes #1120
Fixes #1188
Relates to #56
Additional Notes
Library/MCPForUnity/Logs/server-launch-{port}.log. The issue requested a Windows toggle; this removes the window unconditionally instead — happy to add an opt-in "show terminal" toggle if maintainers prefer.unity_reflectchange resolves spurious ambiguity for unqualified BCL generics (List<T>,Dictionary<…>); genuine cross-namespace clashes (e.g.UI.ButtonvsUIElements.Button) remain reported as ambiguous by design, so this only partially relates.CLAUDE.mdand the website contributing docs).Summary by CodeRabbit