feat: Code execution waits for reload recovery by default#1142
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (26)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (10)
📝 WalkthroughWalkthroughAdds parameter-driven domain-reload wait behavior for the execute-dynamic-code CLI command, bumps minimum CLI release to v3.0.0-beta.9, introduces WaitForDomainReload (default true) across schema/docs, implements wait and disconnect handling, integrates into execution/readiness flows, and updates tests/completion. ChangesDomain-Reload Wait Feature
Sequence Diagram(s)sequenceDiagram
participant CLI
participant UnityIPCClient
participant Editor
participant ReadinessProbe
CLI->>UnityIPCClient: send execute-dynamic-code request (params include WaitForDomainReload / CompileOnly)
UnityIPCClient->>CLI: return outcome (possibly contains DomainReloadWaitRequired flag)
CLI->>Editor: if DomainReloadWaitRequired or dispatch-disconnect -> wait for domain reload readiness
Editor->>ReadinessProbe: readiness checks (probe uses domainReloadWaitParam:false for warmup)
ReadinessProbe->>Editor: readiness result
Editor->>CLI: readiness signal (complete/failure)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Packages/src/Cli~/internal/cli/completion.go (1)
249-254: ⚡ Quick winKeep cache fallback if embedded execute-dynamic-code lookup misses.
The unconditional
returnsuppresses the existing cache fallback path when embedded defaults don’t contain the tool. Return only on embedded-hit, otherwise fall through to the normal cache lookup.Proposed patch
if command == executeDynamicCodeCommandName { if tool, ok := findTool(loadDefaultTools(), command); ok { printOptionsForTool(tool, stdout) + return } - return }🤖 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 `@Packages/src/Cli`~/internal/cli/completion.go around lines 249 - 254, The current block around executeDynamicCodeCommandName unconditionally returns, which prevents the cache fallback when findTool(loadDefaultTools(), command) misses; change the control flow so that you only return when the embedded lookup succeeds (i.e., when findTool(...) yields ok and you call printOptionsForTool), but when it does not find the tool do not return and let the function continue to the normal cache lookup path; adjust the if/ok handling in the executeDynamicCodeCommandName branch to only exit on embedded-hit and otherwise fall through.Assets/Tests/Editor/CliSetupApplicationServiceTests.cs (1)
41-42: ⚡ Quick winAvoid hardcoded version/tag literals in assertions.
Use
CliConstantshere too, so future version bumps only touch one source of truth.♻️ Suggested update
- Assert.That(service.GetMinimumRequiredCliVersion(), Is.EqualTo("3.0.0-beta.9")); + Assert.That( + service.GetMinimumRequiredCliVersion(), + Is.EqualTo(CliConstants.MINIMUM_REQUIRED_CLI_VERSION)); @@ - Assert.That(service.GetMinimumRequiredCliReleaseTag(), Is.EqualTo("cli-v3.0.0-beta.9")); + Assert.That( + service.GetMinimumRequiredCliReleaseTag(), + Is.EqualTo(CliConstants.MINIMUM_REQUIRED_CLI_RELEASE_TAG));Also applies to: 52-53
🤖 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 `@Assets/Tests/Editor/CliSetupApplicationServiceTests.cs` around lines 41 - 42, The test is asserting a hardcoded CLI version string; update the assertions in CliSetupApplicationServiceTests to use the centralized constant from CliConstants instead of the literal "3.0.0-beta.9" (and the other literal at the second assertion location), e.g. call CliConstants.MinimumRequiredCliVersion (or the exact constant name defined for the required CLI version) when asserting service.GetMinimumRequiredCliVersion() to ensure a single source of truth for version bumps.
🤖 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.
Nitpick comments:
In `@Assets/Tests/Editor/CliSetupApplicationServiceTests.cs`:
- Around line 41-42: The test is asserting a hardcoded CLI version string;
update the assertions in CliSetupApplicationServiceTests to use the centralized
constant from CliConstants instead of the literal "3.0.0-beta.9" (and the other
literal at the second assertion location), e.g. call
CliConstants.MinimumRequiredCliVersion (or the exact constant name defined for
the required CLI version) when asserting service.GetMinimumRequiredCliVersion()
to ensure a single source of truth for version bumps.
In `@Packages/src/Cli`~/internal/cli/completion.go:
- Around line 249-254: The current block around executeDynamicCodeCommandName
unconditionally returns, which prevents the cache fallback when
findTool(loadDefaultTools(), command) misses; change the control flow so that
you only return when the embedded lookup succeeds (i.e., when findTool(...)
yields ok and you call printOptionsForTool), but when it does not find the tool
do not return and let the function continue to the normal cache lookup path;
adjust the if/ok handling in the executeDynamicCodeCommandName branch to only
exit on embedded-hit and otherwise fall through.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3de39f77-8224-4628-8552-761721cf4fc4
⛔ Files ignored due to path filters (3)
Packages/src/Cli~/dist/darwin-amd64/uloopis excluded by!**/dist/**and included by nonePackages/src/Cli~/dist/darwin-arm64/uloopis excluded by!**/dist/**and included by nonePackages/src/Cli~/dist/windows-amd64/uloop.exeis excluded by!**/dist/**,!**/*.exeand included by none
📒 Files selected for processing (17)
Assets/Tests/Editor/CliSetupApplicationServiceTests.csAssets/Tests/Editor/DynamicCodeToolTests/FirstPartyToolSchemaMetadataTests.csPackages/src/Cli~/contract.jsonPackages/src/Cli~/internal/cli/compile_wait.goPackages/src/Cli~/internal/cli/compile_wait_test.goPackages/src/Cli~/internal/cli/completion.goPackages/src/Cli~/internal/cli/completion_test.goPackages/src/Cli~/internal/cli/dynamic_code_wait.goPackages/src/Cli~/internal/cli/launch_test.goPackages/src/Cli~/internal/cli/run.goPackages/src/Cli~/internal/cli/tool_readiness.goPackages/src/Cli~/internal/cli/tools_test.goPackages/src/Cli~/internal/tools/default-tools.jsonPackages/src/Editor/Application/CliSetupApplicationService.csPackages/src/Editor/Domain/CliConstants.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/ExecuteDynamicCodeSchema.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Skill/SKILL.md
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
execute-dynamic-code can mutate editor or asset state in ways that start a domain reload after the tool response is produced. Defaulting to a post-reload checkpoint keeps follow-up CLI calls from racing the recovering editor, while --no-wait-for-domain-reload preserves the explicit fast path for callers that opt out. Use a Unity-side compile/reload signal in the dynamic-code response so the native CLI waits only after Unity has observed reload work, and wait through dispatched transport disconnects when a reload drops IPC before the response is serialized. Raise the native CLI contract and Unity package minimum CLI version together so older beta CLIs cannot satisfy the new package-side expectation. Keep the GitHub Release tag as a separate cli-v-prefixed constant so installer code reads like the release page.
Keep the compile-only execution path available for internal diagnostics, but stop advertising it through CLI option listings, execute-dynamic-code skill docs, and public examples so normal users see only the supported workflow surface.
hatayama
force-pushed
the
feature/hatayama/update-domain-reload-default
branch
from
fbed937 to
7365c7d
Compare
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
1 participant
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
User Impact
--no-wait-for-domain-reloadis documented as the explicit fast path, while compile-only diagnostics are no longer advertised in help, skills, or examples.Changes
execute-dynamic-code, including a Unity-side reload signal and recovery wait after dispatched transport disconnects.3.0.0-beta.9, with a separatecli-vrelease tag constant for installer flows.Verification
scripts/check-go-cli.shgo test ./...Packages/src/Cli~/dist/darwin-arm64/uloop compile --project-path /Users/a12115/ghq/hatayama/unity-cli-loopPackages/src/Cli~/dist/darwin-arm64/uloop --list-options execute-dynamic-codePackages/src/Cli~/dist/darwin-arm64/uloop execute-dynamic-code --compile-only --code 'return 1;' --project-path /Users/a12115/ghq/hatayama/unity-cli-loopOverview
This PR makes the Unity CLI Loop's execute-dynamic-code command wait for Editor domain-reload recovery by default after executing dynamic code. This reduces race conditions where follow-up CLI commands run before the Editor is fully ready. Opting out is possible via a negated flag.
Key Changes
Version Bumping
Schema & Tool Definition
CLI Implementation (Go)
CLI tooling & Types
Editor (C#) Behavior
Tests & Verification
C# tests:
Go tests:
CI / verification commands referenced: go test ./internal/cli and go test ./..., scripts/check-go-cli-source.sh and scripts/check-go-cli.sh, Build/CLI smoke runs (darwin-arm64), and focused EditMode tests.
User-Facing Changes
Architecture Diagram (class-style flow)