feat(manage_editor): add wait_for_compilation action#978
Open
smuhlaci wants to merge 8 commits intoCoplayDev:betafrom
Open
feat(manage_editor): add wait_for_compilation action#978smuhlaci wants to merge 8 commits intoCoplayDev:betafrom
smuhlaci wants to merge 8 commits intoCoplayDev:betafrom
Conversation
Poll editor_state until compilation and domain reload finish so the AI can wait for script changes to compile instead of using fixed sleep. - manage_editor(action="wait_for_compilation", timeout=30) on server - CLI: unity-mcp editor wait-compile [--timeout N] - Reuses wait_for_editor_ready from refresh_unity; no C# changes Co-authored-by: Cursor <cursoragent@cursor.com>
… references and guides
…wait-compile
- Remove unused _WAIT_FOR_COMPILATION_ACTIONS from manage_editor.py
- Use result.get('message', ...) in editor wait-compile so non-timeout
errors (e.g. Python/connectivity) are reported accurately
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
… not cut off Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Reviewer's GuideAdds a new read-only manage_editor action wait_for_compilation, exposes it via a unity-mcp CLI subcommand, documents the new workflow, and adds integration tests covering ready, polling, timeout, and domain reload scenarios. Sequence diagram for manage_editor wait_for_compilation via CLIsequenceDiagram
actor Developer
participant CLI as unity_mcp_CLI
participant ManageEditor as manage_editor_tool
participant WaitHelper as _wait_for_compilation
participant ReadyPoller as wait_for_editor_ready
participant Unity as Unity_Editor
Developer->>CLI: unity-mcp editor wait-compile --timeout T
CLI->>CLI: get_config()
CLI->>ManageEditor: run_command manage_editor
activate ManageEditor
ManageEditor->>ManageEditor: manage_editor(action=wait_for_compilation, timeout=T)
ManageEditor->>WaitHelper: _wait_for_compilation(ctx, timeout=T)
activate WaitHelper
WaitHelper->>WaitHelper: timeout_s = clamp(T or 30, 1, 120)
WaitHelper->>ReadyPoller: wait_for_editor_ready(ctx, timeout_s)
activate ReadyPoller
loop until ready or timeout
ReadyPoller->>Unity: poll editor_state
Unity-->>ReadyPoller: compilation_state, domain_reload_state
end
ReadyPoller-->>WaitHelper: ready, elapsed
deactivate ReadyPoller
alt editor ready
WaitHelper-->>ManageEditor: success=True, data.waited_seconds=elapsed
else timeout
WaitHelper-->>ManageEditor: success=False, data.timeout_seconds=timeout_s
end
deactivate WaitHelper
ManageEditor-->>CLI: result dict
deactivate ManageEditor
CLI->>CLI: format_output(result)
alt success
CLI->>Developer: print_success Compilation complete
else failure
CLI->>Developer: print_error Timed out waiting for compilation
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded a new manage_editor action Changes
Sequence DiagramsequenceDiagram
participant Agent as Agent
participant CLI as CLI\r\n(`editor.py`)
participant Service as manage_editor\r\nService
participant RefreshUnity as refresh_unity\r\nwait_for_editor_ready
participant EditorState as editor_state\r\nget_editor_state
Agent->>CLI: unity-mcp editor wait-compile --timeout 30
CLI->>CLI: compute transport_timeout = ceil(timeout) + 10
CLI->>Service: run_command(manage_editor, action=wait_for_compilation, timeout=30)
Service->>RefreshUnity: wait_for_editor_ready(timeout_s=30)
loop Poll until ready or timeout
RefreshUnity->>EditorState: get_editor_state()
EditorState-->>RefreshUnity: {ready_for_tools, compilation.is_compiling, ...}
alt ready_for_tools == true
RefreshUnity-->>Service: {waited_seconds, success:true}
else timeout exceeded
RefreshUnity-->>Service: {timeout_seconds, success:false}
else keep polling
RefreshUnity->>RefreshUnity: sleep & retry
end
end
Service-->>CLI: formatted result + message
CLI-->>Agent: success or timeout message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="Server/src/services/tools/manage_editor.py" line_range="84-85" />
<code_context>
+ """Poll editor_state until compilation and domain reload finish."""
+ from services.tools.refresh_unity import wait_for_editor_ready
+
+ timeout_s = float(timeout) if timeout is not None else 30.0
+ timeout_s = max(1.0, min(timeout_s, 120.0))
+ ready, elapsed = await wait_for_editor_ready(ctx, timeout_s=timeout_s)
+
</code_context>
<issue_to_address>
**suggestion:** Consider surfacing or documenting the 120s upper bound on wait_for_compilation.
Since values are clamped to `[1.0, 120.0]`, callers can pass much larger timeouts yet only see the clamped value in the timeout message. To avoid confusion, either reject values >120 with a clear error, or explicitly document the 120s max (e.g., in the tool description / CLI help) so users know larger values won’t be honored.
Suggested implementation:
```python
async def _wait_for_compilation(ctx: Context, timeout: int | float | None) -> dict[str, Any]:
"""Poll editor_state until compilation and domain reload finish.
The timeout is clamped to the inclusive range [1.0, 120.0] seconds to
avoid excessively long waits in the Unity editor.
"""
from services.tools.refresh_unity import wait_for_editor_ready
timeout_s = float(timeout) if timeout is not None else 30.0
# Clamp to [1.0, 120.0] seconds to keep waits reasonable and bounded.
timeout_s = max(1.0, min(timeout_s, 120.0))
ready, elapsed = await wait_for_editor_ready(ctx, timeout_s=timeout_s)
```
To fully surface this behavior to end users (e.g., CLI users or tool consumers), you should also:
1. Update the tool description and/or CLI help text for the command that uses `_wait_for_compilation` to mention that the timeout is clamped to a maximum of 120 seconds.
2. If there is a JSON schema or OpenAPI spec for this tool, document the valid timeout range there as well (e.g., `minimum: 1.0`, `maximum: 120.0`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Server/src/cli/commands/editor.py (1)
42-42: Consider usingmath.ceilfor the transport timeout calculation.
int(timeout)truncates, soint(59.9) + 10 = 69while the actual tool wait is 59.9s. The 10-second buffer handles this adequately, but usingmath.ceil(timeout) + 10would be slightly more precise for edge cases.🔧 Optional: Use ceiling for transport timeout
+import math + `@editor.command`("wait-compile") ... # Ensure the transport timeout outlasts the compilation wait (add a small buffer). - transport_timeout = int(timeout) + 10 + transport_timeout = int(math.ceil(timeout)) + 10🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/src/cli/commands/editor.py` at line 42, Replace the truncating conversion when computing the transport timeout by using math.ceil on the timeout value so edge cases are handled precisely: change the assignment to compute transport_timeout = math.ceil(timeout) + 10 (ensure math is imported and that timeout is a numeric value or converted to float first); update the code around the transport_timeout and timeout variables accordingly to avoid truncation from int().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Server/src/cli/commands/editor.py`:
- Line 42: Replace the truncating conversion when computing the transport
timeout by using math.ceil on the timeout value so edge cases are handled
precisely: change the assignment to compute transport_timeout =
math.ceil(timeout) + 10 (ensure math is imported and that timeout is a numeric
value or converted to float first); update the code around the transport_timeout
and timeout variables accordingly to avoid truncation from int().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49f8730b-48f0-41ca-bc7d-db5bb4c47035
📒 Files selected for processing (8)
.claude/skills/unity-mcp-skill/references/tools-reference.mdServer/src/cli/CLI_USAGE_GUIDE.mdServer/src/cli/commands/editor.pyServer/src/services/tools/manage_editor.pyServer/tests/integration/test_manage_editor_wait.pydocs/guides/CLI_EXAMPLE.mddocs/guides/CLI_USAGE.mdunity-mcp-skill/references/tools-reference.md
Document the 1-120 second wait_for_compilation timeout clamp in the tool, CLI, and references so callers know larger values are not honored. Round the CLI transport timeout up before adding its buffer so fractional waits are covered precisely. Made-with: Cursor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Server/src/cli/commands/editor.py`:
- Around line 44-46: The code uses the raw timeout option when calling
run_command and computing transport_timeout; clamp the timeout to the
server-allowed range (1..120) first and then use that clamped value for both the
payload and transport timeout calculation (i.e., replace uses of timeout in the
call to run_command("manage_editor", {"action":"wait_for_compilation","timeout":
timeout}, ...) and in transport_timeout = math.ceil(timeout) + 10 with the
clamped_timeout variable), ensuring negative or huge inputs cannot produce
invalid or excessive transport timeouts.
- Around line 48-52: The CLI currently prints an error when the compilation wait
returns {"success": False} but still exits with code 0; update the failure
branch in the editor wait-compile handling (where `result` is checked and
`print_success`/`print_error` are called) to terminate with a non-zero exit code
(e.g., call `sys.exit(1)` or raise `SystemExit(1)`) after calling `print_error`
so callers see a failing exit status when `result.get("success")` is false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5569b198-997a-4785-9ec1-3c0d69744787
📒 Files selected for processing (7)
.claude/skills/unity-mcp-skill/references/tools-reference.mdServer/src/cli/CLI_USAGE_GUIDE.mdServer/src/cli/commands/editor.pyServer/src/services/tools/manage_editor.pydocs/guides/CLI_EXAMPLE.mddocs/guides/CLI_USAGE.mdunity-mcp-skill/references/tools-reference.md
✅ Files skipped from review due to trivial changes (5)
- docs/guides/CLI_EXAMPLE.md
- Server/src/cli/CLI_USAGE_GUIDE.md
- unity-mcp-skill/references/tools-reference.md
- .claude/skills/unity-mcp-skill/references/tools-reference.md
- docs/guides/CLI_USAGE.md
Clamp the wait-compile timeout on the CLI before deriving the transport timeout so invalid or excessive values never reach the request layer. Exit with a non-zero status when compilation waiting fails so shell pipelines stop on timeout. Made-with: Cursor
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Server/tests/test_cli.py (1)
1102-1114: Also assert lower-bound timeout clamping in this failure-path test.Since this case passes
--timeout -5, add an assertion on the outgoing request payload timeout so negative values are proven to be sanitized before request dispatch.Proposed test tightening
- with patch("cli.commands.editor.get_config", return_value=mock_config): - with patch("cli.commands.editor.run_command", return_value=wait_response): + with patch("cli.commands.editor.get_config", return_value=mock_config): + with patch("cli.commands.editor.run_command", return_value=wait_response) as mock_run: result = runner.invoke(cli, ["editor", "wait-compile", "--timeout", "-5"]) assert result.exit_code == 1 assert "Timed out after 120s waiting for compilation to finish." in result.output + mock_run.assert_called_once() + args = mock_run.call_args + assert args[0][1]["timeout"] == 1.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Server/tests/test_cli.py` around lines 1102 - 1114, The test test_editor_wait_compile_returns_nonzero_on_failure should also verify that negative timeouts are clamped before dispatch: patch cli.commands.editor.run_command as mocked_run_command, call runner.invoke as you already do with "--timeout -5", then assert mocked_run_command was called and inspect its call args/kwargs (the timeout kwarg passed to run_command) to ensure it was sanitized (e.g., timeout >= 0 or equals the expected lower-bound value used by the implementation) before sending the request. This uses the existing patches for get_config and run_command and the run_command symbol to locate where to assert the outgoing timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Server/tests/test_cli.py`:
- Around line 1102-1114: The test
test_editor_wait_compile_returns_nonzero_on_failure should also verify that
negative timeouts are clamped before dispatch: patch
cli.commands.editor.run_command as mocked_run_command, call runner.invoke as you
already do with "--timeout -5", then assert mocked_run_command was called and
inspect its call args/kwargs (the timeout kwarg passed to run_command) to ensure
it was sanitized (e.g., timeout >= 0 or equals the expected lower-bound value
used by the implementation) before sending the request. This uses the existing
patches for get_config and run_command and the run_command symbol to locate
where to assert the outgoing timeout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e87e59f-2c25-49a8-a7a0-3ca1a720c2e8
📒 Files selected for processing (2)
Server/src/cli/commands/editor.pyServer/tests/test_cli.py
🚧 Files skipped from review as they are similar to previous changes (1)
- Server/src/cli/commands/editor.py
Assert that negative wait-compile timeouts are clamped before dispatch so the CLI test verifies the outgoing request is sanitized as well as returning a non-zero exit code. Made-with: Cursor
Author
|
@Scriptwonder @dsarno what do you think about this PR? |
Collaborator
|
I think it could be useful, but my only concern is that LLMs might not call it. During my heavy usage, they do not go sleep, just polling and checking. (Opus 4.6) Is it possible we can do it as a post-hook for manage_script or manage_shader (any changes that will trigger domain reload)? Just a thought. |
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
2 participants
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.
Summary
manage_editor(action=\"wait_for_compilation\")so agents can wait for compile plus domain reload instead of sleeping after script editsunity-mcp editor wait-compile --timeout ...and documentation updates for the new workflowTest plan
cd Server && uv run pytest tests/integration/test_manage_editor_wait.py -vNotes
smuhlaci:betaonto a dedicated branch.Made with Cursor
Summary by Sourcery
Add a Unity editor management action and CLI command to wait for script compilation to complete before proceeding, with integration tests and documentation updates.
New Features:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Documentation
Tests