Repository navigation
feat: Gate CLI/Unity compatibility on an IPC protocol version instead of release numbers - #1329
Merged
Merged
Conversation
The semver-based compatibility floor forced PR authors to predict the next release number, which diverged from release-please numbering and broke a release. Compatibility is really about the C# IPC contract, so the contract now carries an integer protocolVersion that only moves when the Unity package and the CLI stop interoperating. The CLI sends it in the uloop request metadata so the Unity side can gate on it.
The Unity package rejected old CLIs by comparing release numbers against MINIMUM_REQUIRED_CLI_VERSION, which forced PR authors to predict the next release number and broke when CLI changes accumulated across releases. The gate now reads uloop.protocolVersion from the request metadata and rejects clients below REQUIRED_CLI_PROTOCOL_VERSION, an integer that moves only on a breaking IPC contract change. A missing or non-integer protocolVersion fails the gate, so CLIs released before the handshake are treated as outdated. The cli_update_required error now reports current/required protocol versions instead of a target release tag. MINIMUM_REQUIRED_CLI_VERSION stays as the installer pin for setup/update flows.
The minimum-version warning fired whenever Go CLI files changed without bumping MINIMUM_REQUIRED_CLI_VERSION. That constant is now only the installer pin, so the warning enforced the wrong thing, depended on a git diff, and tangled with release-please numbering. Replace the whole apparatus (comment binary, scripts, pull_request_target workflow, and the fail-on-warning build step) with one deterministic Go test: cli/contract.json protocolVersion must equal the Unity package's REQUIRED_CLI_PROTOCOL_VERSION. It runs in the existing test suite, needs no diff, PR number, or GitHub API, and has no false positives. Whether a given change is breaking enough to bump the protocol is a code-review and docs concern, not a path heuristic.
Replace the commit-time MINIMUM_REQUIRED_CLI_VERSION rule, which no longer reflects how the gate works, with guidance for the protocol version: keep cli/contract.json protocolVersion and CliConstants.REQUIRED_CLI_PROTOCOL_VERSION equal and bump both only on a breaking IPC change. Clarify that release-please owns the version stamps and that MINIMUM_REQUIRED_CLI_VERSION is the installer pin, neither of which belongs in a feature PR.
Contributor
|
Warning Review limit reached
More reviews will be available in 13 minutes and 52 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR transitions the CLI/Unity package IPC compatibility model from minimum version string comparison to exact-match integer protocol generation, introducing a ChangesCLI IPC Protocol Version Negotiation
Application Services and CLI Inspection
UI and Settings Window Updates
CLI Version Output and Metadata
CLI Error Handling and Messaging
IPC Protocol Reminder and Coordination
Removal of Legacy Minimum-Version Automation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 unit tests (beta)
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 23 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Use the IPC protocol generation, rather than release semver, for runtime compatibility and setup UI decisions. - Require exact protocol matches at IPC handshake time with distinct guidance for older and newer CLI protocols. - Expose `uloop --version --json` so the Unity package can detect CLI protocol metadata. - Make setup update and replacement actions protocol-aware while keeping release tags limited to installation.
Ask CodeRabbit to flag IPC-facing changes for protocol-version review without treating ordinary CLI or release changes as automatic bump triggers.
Tighten the AGENTS wording for protocol-version bump guidance so the contract-generation rule reads cleanly.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Assets/Tests/Editor/SetupWizardWindowTests.cs (1)
488-509:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the required behavior comment to this test.
This changed test method is missing the short comment that explains what behavior it verifies.
✏️ Suggested comment
public void GetCliButtonTextForSetupWizard_ReturnsExpectedLabel( bool cliInstalled, bool isInstallingCli, bool isChecking, bool needsUpdate, bool needsDowngrade, bool needsCliPathSetup, string cliVersion, string requiredCliVersion, string expectedLabel) { + // Verifies that the setup wizard button text reflects install, update, downgrade, PATH-repair, and checking states. string label = SetupWizardWindow.GetCliButtonTextForSetupWizard( cliInstalled, isInstallingCli, isChecking,As per coding guidelines,
**/*[Tt]est?(s).cs: Every test method must have a short comment that states what behavior the test verifies.🤖 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/SetupWizardWindowTests.cs` around lines 488 - 509, Add a short one-line comment above the test method GetCliButtonTextForSetupWizard_ReturnsExpectedLabel describing the behavior it verifies (e.g., "Verifies GetCliButtonTextForSetupWizard returns the correct label for given CLI/install states"), ensuring the comment succinctly states the expected behavior of SetupWizardWindow.GetCliButtonTextForSetupWizard for the supplied parameter combinations.Source: Coding guidelines
Packages/src/Editor/Infrastructure/CLI/CliInstallationDetector.cs (1)
482-551:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWrap process in
usingstatement to prevent resource leak.The process is manually disposed at lines 529, 538, and 544, but if an exception occurs between process creation (line 497) and the try block (line 522), the process will leak. Wrap the process lifecycle in a
usingstatement for guaranteed cleanup.🔒 Recommended fix to add using statement
Process process = ProcessStartHelper.TryStart(startInfo); if (process == null) { return null; } - StringBuilder outputBuilder = new(); - - process.OutputDataReceived += (sender, e) => - { - if (e.Data != null) - { - outputBuilder.Append(e.Data); - } - }; - process.ErrorDataReceived += (sender, e) => { }; - - process.BeginOutputReadLine(); - process.BeginErrorReadLine(); - - using CancellationTokenRegistration registration = ct.Register(() => - { - KillProcessIfRunning(process); - }); - - try + using (process) { - bool exited = process.WaitForExit(PROCESS_TIMEOUT_MS); - - if (!exited) + StringBuilder outputBuilder = new(); + + process.OutputDataReceived += (sender, e) => { - KillProcessIfRunning(process); - process.Dispose(); - return null; - } - - // Parameterless WaitForExit flushes async output buffers - process.WaitForExit(); - - string output = outputBuilder.ToString().Trim(); - bool failed = process.ExitCode != 0 || string.IsNullOrEmpty(output); - process.Dispose(); - - return failed ? null : output; - } - catch (Exception ex) - { - process.Dispose(); - if (!ct.IsCancellationRequested) + if (e.Data != null) + { + outputBuilder.Append(e.Data); + } + }; + process.ErrorDataReceived += (sender, e) => { }; + + process.BeginOutputReadLine(); + process.BeginErrorReadLine(); + + using CancellationTokenRegistration registration = ct.Register(() => { - UnityEngine.Debug.LogWarning($"[UnityCliLoop] Failed to detect CLI version: {ex.Message}"); + KillProcessIfRunning(process); + }); + + try + { + bool exited = process.WaitForExit(PROCESS_TIMEOUT_MS); + + if (!exited) + { + KillProcessIfRunning(process); + return null; + } + + // Parameterless WaitForExit flushes async output buffers + process.WaitForExit(); + + string output = outputBuilder.ToString().Trim(); + bool failed = process.ExitCode != 0 || string.IsNullOrEmpty(output); + + return failed ? null : output; + } + catch (Exception ex) + { + if (!ct.IsCancellationRequested) + { + UnityEngine.Debug.LogWarning($"[UnityCliLoop] Failed to detect CLI version: {ex.Message}"); + } + return null; } - return null; }🤖 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/Editor/Infrastructure/CLI/CliInstallationDetector.cs` around lines 482 - 551, The Process returned by ProcessStartHelper.TryStart in ExecuteCliVersionCommand can leak if an exception occurs before the manual Dispose calls; wrap the Process in a using statement (e.g., using var process = ProcessStartHelper.TryStart(startInfo);) immediately after creation and return early if it's null, then remove the explicit process.Dispose() calls inside the method (including the ones after KillProcessIfRunning and in the catch), keeping the existing CancellationTokenRegistration and calls to KillProcessIfRunning and process.WaitForExit(PROCESS_TIMEOUT_MS) as-is so the using ensures deterministic cleanup.Assets/Tests/Editor/UnityCliLoopFirstPartyServerLifecycleBindingTests.cs (1)
11-14:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the test comment to reflect protocol version verification.
The comment states the test verifies "the internal readiness probe does not depend on user-toggleable tools," but the updated assertion now checks
protocolVersionin the request metadata. Update the comment to clarify that the test also verifies the protocol version is included in the readiness request.🤖 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/UnityCliLoopFirstPartyServerLifecycleBindingTests.cs` around lines 11 - 14, Update the failing test comment for CreateGetVersionReadinessRequestJson_UsesInternalHealthCheckWithCliMetadata to reflect the new assertion: replace or extend the current description ("Tests that the internal readiness probe does not depend on user-toggleable tools.") with a brief note that the test also verifies the protocol version is included in the readiness request metadata (i.e., that protocolVersion is present and correct in the generated request JSON).Source: Coding guidelines
🧹 Nitpick comments (1)
cli/internal/cli/error_envelope_test.go (1)
215-231: ⚡ Quick winAlso lock the retry contract in the newer-protocol test.
This branch currently only verifies the first guidance string. Adding the same
ErrorCode/Retryable/SafeToRetryassertions as the older-protocol test will keep the newer-protocol classification from regressing silently.🧪 Suggested assertion block
cliErr := classifyError(err, errorContext{projectRoot: "/tmp/MyProject", command: "compile"}) if cliErr.ErrorCode != errorCodeCLIUpdateRequired { t.Fatalf("error code mismatch: %#v", cliErr) } + if !cliErr.Retryable || !cliErr.SafeToRetry { + t.Fatalf("retry flags mismatch: %#v", cliErr) + } if len(cliErr.NextActions) == 0 || cliErr.NextActions[0] != "Update the Unity package to a version that supports this CLI protocol, or install the CLI from the same release as the package." { t.Fatalf("next actions mismatch: %#v", cliErr.NextActions) }🤖 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 `@cli/internal/cli/error_envelope_test.go` around lines 215 - 231, The newer-protocol test TestClassifyCliUpdateRequiredRPCErrorForNewerProtocol only asserts the NextActions string; add the same assertions used in the older-protocol test to lock the retry contract: after calling classifyError(err, ...) assert cliErr.ErrorCode == errorCodeCLIUpdateRequired, cliErr.Retryable == true (or the expected boolean as in the older test), and cliErr.SafeToRetry == false (or the expected boolean from the older test) so ErrorCode, Retryable and SafeToRetry cannot regress; reference the classifyError result variable cliErr and the constant errorCodeCLIUpdateRequired when adding these checks.
🤖 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.
Outside diff comments:
In `@Assets/Tests/Editor/SetupWizardWindowTests.cs`:
- Around line 488-509: Add a short one-line comment above the test method
GetCliButtonTextForSetupWizard_ReturnsExpectedLabel describing the behavior it
verifies (e.g., "Verifies GetCliButtonTextForSetupWizard returns the correct
label for given CLI/install states"), ensuring the comment succinctly states the
expected behavior of SetupWizardWindow.GetCliButtonTextForSetupWizard for the
supplied parameter combinations.
In `@Assets/Tests/Editor/UnityCliLoopFirstPartyServerLifecycleBindingTests.cs`:
- Around line 11-14: Update the failing test comment for
CreateGetVersionReadinessRequestJson_UsesInternalHealthCheckWithCliMetadata to
reflect the new assertion: replace or extend the current description ("Tests
that the internal readiness probe does not depend on user-toggleable tools.")
with a brief note that the test also verifies the protocol version is included
in the readiness request metadata (i.e., that protocolVersion is present and
correct in the generated request JSON).
In `@Packages/src/Editor/Infrastructure/CLI/CliInstallationDetector.cs`:
- Around line 482-551: The Process returned by ProcessStartHelper.TryStart in
ExecuteCliVersionCommand can leak if an exception occurs before the manual
Dispose calls; wrap the Process in a using statement (e.g., using var process =
ProcessStartHelper.TryStart(startInfo);) immediately after creation and return
early if it's null, then remove the explicit process.Dispose() calls inside the
method (including the ones after KillProcessIfRunning and in the catch), keeping
the existing CancellationTokenRegistration and calls to KillProcessIfRunning and
process.WaitForExit(PROCESS_TIMEOUT_MS) as-is so the using ensures deterministic
cleanup.
---
Nitpick comments:
In `@cli/internal/cli/error_envelope_test.go`:
- Around line 215-231: The newer-protocol test
TestClassifyCliUpdateRequiredRPCErrorForNewerProtocol only asserts the
NextActions string; add the same assertions used in the older-protocol test to
lock the retry contract: after calling classifyError(err, ...) assert
cliErr.ErrorCode == errorCodeCLIUpdateRequired, cliErr.Retryable == true (or the
expected boolean as in the older test), and cliErr.SafeToRetry == false (or the
expected boolean from the older test) so ErrorCode, Retryable and SafeToRetry
cannot regress; reference the classifyError result variable cliErr and the
constant errorCodeCLIUpdateRequired when adding these checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 03dbf5b0-8ad4-4bd2-9637-64f9b965e95d
📒 Files selected for processing (42)
.coderabbit.yaml.github/workflows/build-and-test.yml.github/workflows/cli-minimum-version-warning.ymlAGENTS.mdAssets/Tests/Editor/CliInstallationDetectorTests.csAssets/Tests/Editor/CliPathSetupFlowTests.csAssets/Tests/Editor/CliSetupApplicationServiceTests.csAssets/Tests/Editor/CliVersionComparerTests.csAssets/Tests/Editor/JsonRpcProcessorCliVersionGateTests.csAssets/Tests/Editor/ProjectIpcWarmupClientTests.csAssets/Tests/Editor/SetupWizardWindowTests.csAssets/Tests/Editor/UnityCliLoopFirstPartyServerLifecycleBindingTests.csAssets/Tests/Editor/UnityCliLoopSettingsWindowCliActionTests.csPackages/src/Editor/Application/CliSetupApplicationService.csPackages/src/Editor/CompositionRoot/UnityCliLoopFirstPartyServerLifecycleBinding.csPackages/src/Editor/Domain/CliConstants.csPackages/src/Editor/Domain/CliVersionComparer.csPackages/src/Editor/Infrastructure/Api/JsonRpcProcessor.csPackages/src/Editor/Infrastructure/Api/JsonRpcRequest.csPackages/src/Editor/Infrastructure/CLI/CliInstallationDetector.csPackages/src/Editor/Presentation/Setup/SetupWizardWindow.csPackages/src/Editor/Presentation/UnityCliLoopSettingsWindow.cscli/cmd/comment-cli-minimum-version-warning/main.gocli/contract.gocli/contract.jsoncli/contract_test.gocli/internal/architecture/comment_cli_minimum_version_warning_test.gocli/internal/automation/minimum_version_warning.gocli/internal/automation/minimum_version_warning_test.gocli/internal/cli/error_envelope.gocli/internal/cli/error_envelope_test.gocli/internal/cli/help_test.gocli/internal/cli/run.gocli/internal/cli/run_help.gocli/internal/cli/tools.gocli/internal/unityipc/client.gocli/internal/unityipc/client_test.gocli/protocol_version_consistency_test.goscripts/check-cli-minimum-version-warning.shscripts/comment-cli-minimum-version-warning.shscripts/test-cli-minimum-version-warning-workflow.shscripts/test-comment-cli-minimum-version-warning.sh
💤 Files with no reviewable changes (10)
- scripts/test-comment-cli-minimum-version-warning.sh
- scripts/test-cli-minimum-version-warning-workflow.sh
- cli/cmd/comment-cli-minimum-version-warning/main.go
- .github/workflows/cli-minimum-version-warning.yml
- .github/workflows/build-and-test.yml
- scripts/comment-cli-minimum-version-warning.sh
- scripts/check-cli-minimum-version-warning.sh
- cli/internal/automation/minimum_version_warning.go
- cli/internal/automation/minimum_version_warning_test.go
- cli/internal/architecture/comment_cli_minimum_version_warning_test.go
Add contract-shape tests for protocol and heartbeat metadata, plus a non-blocking PR reminder that highlights IPC-facing diffs when protocol declarations were not changed.
Resolve protocol gate conflicts by keeping protocolVersion 1 while adopting the latest published CLI release version from origin/v3-beta.
Treat protocolVersion integers outside the Int32 range as missing metadata so compatibility checks return deterministic update guidance instead of parser overflow errors.
Merged
3 tasks done
RyanXie123
pushed a commit
to RyanXie123/unity-cli-loop
that referenced
this pull request
Sep 22, 2026
… of release numbers (hatayama#1329)
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
uloop updatebefore any tool runs — the error now reports protocol generations instead of a guessed release tag.Why
Compatibility is really about the C#↔Go IPC contract, but it was expressed as a semver floor (
MINIMUM_REQUIRED_CLI_VERSION). That coupled a compatibility decision to release-please's numbering: every CLI change nudged the floor, and the prediction broke when several CLI PRs landed between releases. Protocol versions decouple the two: the integer moves only on a breaking IPC change, never per release.Changes
cli/contract.jsongainsprotocolVersion; the CLI sends it in theulooprequest metadata (cli/contract.go,cli/internal/unityipc/client.go).JsonRpcProcessorreadsuloop.protocolVersionand rejects clients belowCliConstants.REQUIRED_CLI_PROTOCOL_VERSION. A missing or non-integer value fails the gate, so pre-handshake CLIs are treated as outdated. Thecli_update_requirederror now carries current/required protocol versions.pull_request_targetworkflow, fail-on-warning build step) and replaced it with one deterministic Go test,TestProtocolVersionMatchesUnityPackage, that fails the build if the Go and C# protocol declarations diverge. No git diff, PR number, or GitHub API involved.MINIMUM_REQUIRED_CLI_VERSIONstays only as the installer pin for setup/update; release-please still owns thecliVersion/default-tools.jsonstamps.Verification
scripts/check-go-cli.sh: green (fmt, vet, lint, full Go suite incl. the new invariant test).contract.jsonprotocolVersion to 2 makesTestProtocolVersionMatchesUnityPackagefail as expected; reverted.JsonRpcProcessorCliVersionGateTests+UnityCliLoopFirstPartyServerLifecycleBindingTestspass (16/16); broader gate/setup/comparer classes pass (30/30). 3 unrelated pre-existing failures (prefab/architecture/recovery-flake) reproduce identically on the base branch.