fix: Read minimum version requirements from the project runner pin#1506
Conversation
MINIMUM_REQUIRED_DISPATCHER_VERSION duplicated the pin's minimumDispatcherVersion, and MINIMUM_REQUIRED_PROJECT_RUNNER_VERSION had no runtime reader left after the dispatcher regex fallback was removed — only CI guards still parsed both constants out of C# source with regexes. Each duplicate declaration was one more value to keep in sync by hand, so the pin JSON is now the single source and the four C# constants (two values plus derived release tags) are gone. - Add CliPinReader so the package reads its own shipped pin; setup, installation detection, and the settings window take the dispatcher floor from it and fail fast when the pin cannot be loaded - Repoint the protocol minimum guard at the pin's projectRunnerVersion while preserving its intent: a protocol bump still requires a published project runner release advertising the new protocol - Drop the CliConstants regex from the dispatcher minimum guard; it now validates the two pins against each other and the published release - Keep the Go/C# protocol equality test; REQUIRED_CLI_PROTOCOL_VERSION is the one remaining C# version constant
📝 WalkthroughWalkthroughThis PR replaces hard-coded minimum dispatcher/project-runner version constants in Unity's CliConstants with values read from the shipped Unity package pin JSON via a new CliPinReader. Corresponding Go release-automation and CLI-contract guards are updated to parse minimum versions from pin files instead of regex-extracted constants, with tests updated accordingly. ChangesUnity Editor pin-derived version reading
Go release-automation and CLI-contract guard updates
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
2 issues found across 17 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cli/release-automation/internal/automation/protocol_minimum_version_guard.go">
<violation number="1" location="cli/release-automation/internal/automation/protocol_minimum_version_guard.go:161">
P2: The protocol minimum guard now hard-fails every release verification error, including the known missing-contract bootstrap path, so the first renamed-tag migration can be blocked even when no protocol mismatch is readable. Consider restoring a narrow exemption for only the release-contract-missing bootstrap case while still surfacing real protocol/content mismatches.
(Based on your team's feedback about scoped bootstrap exemption for missing release contracts.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if !protocolMinimumVersionBootstrapAllowsUnpublishedProjectRunner(ctx, repoRoot, config.HeadRef, result, err) { | ||
| result.MinimumCliReleaseProtocolError = err.Error() | ||
| } | ||
| result.MinimumCliReleaseProtocolError = err.Error() |
There was a problem hiding this comment.
P2: The protocol minimum guard now hard-fails every release verification error, including the known missing-contract bootstrap path, so the first renamed-tag migration can be blocked even when no protocol mismatch is readable. Consider restoring a narrow exemption for only the release-contract-missing bootstrap case while still surfacing real protocol/content mismatches.
(Based on your team's feedback about scoped bootstrap exemption for missing release contracts.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cli/release-automation/internal/automation/protocol_minimum_version_guard.go, line 161:
<comment>The protocol minimum guard now hard-fails every release verification error, including the known missing-contract bootstrap path, so the first renamed-tag migration can be blocked even when no protocol mismatch is readable. Consider restoring a narrow exemption for only the release-contract-missing bootstrap case while still surfacing real protocol/content mismatches.
(Based on your team's feedback about scoped bootstrap exemption for missing release contracts.) </comment>
<file context>
@@ -154,9 +158,7 @@ func AnalyzeProtocolMinimumVersionGuardForRefs(
- if !protocolMinimumVersionBootstrapAllowsUnpublishedProjectRunner(ctx, repoRoot, config.HeadRef, result, err) {
- result.MinimumCliReleaseProtocolError = err.Error()
- }
+ result.MinimumCliReleaseProtocolError = err.Error()
}
}
</file context>
There was a problem hiding this comment.
Not changing this one, deliberately. The removed exemption was not a general release-contract-missing escape hatch: it was gated on the base ref still using the deleted MINIMUM_REQUIRED_PROJECT_RUNNER_VERSION constant (UsesPreRenameMinimumVersion), a precondition that can never be true again after this PR. All live uloop-project-runner-v3.0.0-beta.43..46 tags — including the current pin target — resolve their contract through the existing 3-generation runnerContractPathChain fallback and advertise protocolVersion, so there is no reachable legitimate missing-contract case today. If a future layout rename adds a fourth generation, the right fix is appending the new path to runnerContractPathChain in that same PR; silently exempting missing contracts would mask exactly that omission.
There was a problem hiding this comment.
The parent comment was wrong here: that exemption was only reachable through the deleted UsesPreRenameMinimumVersion precondition, and the live tags already resolve through runnerContractPathChain. I agree the right fix for a future fourth-generation layout would be to add the new path in that PR, not exempt missing contracts.
This comment was influenced by this learning. Open the link to edit it, or reply here to edit or delete it.
Setup and installation detection each carried their own load-or-throw copy of the same pin read, which invites drift in error handling. Both now call CliPinReader.LoadMinimumDispatcherVersionOrThrow().
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Assets/Tests/Editor/CliInstallationDetectorTests.cs (1)
222-237: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the dispatcher-version comparison branch.
The updated call only exercises the
!detection.IsDispatcherearly-return path; the newminimumDispatcherVersioncomparison branch (CliVersionComparer.IsVersionGreaterThanOrEqual(detection.Version, minimumDispatcherVersion)) added toIsShellDetectionUsableForPathSetupisn't covered by any test here.✅ Suggested additional test
[Test] public void IsShellDetectionUsableForPathSetup_WhenDispatcherVersionBelowMinimum_ReturnsFalse() { CliInstallationDetection detection = new( "2.9.9", "/Users/ExampleUser/.npm-global/bin/uloop", isDispatcher: true); bool result = CliInstallationDetector.IsShellDetectionUsableForPathSetup( detection, UnityEngine.RuntimePlatform.OSXEditor, (path, platform) => false, "3.0.0"); Assert.That(result, Is.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 `@Assets/Tests/Editor/CliInstallationDetectorTests.cs` around lines 222 - 237, Add a test that covers the dispatcher-version gate in CliInstallationDetector.IsShellDetectionUsableForPathSetup, since the current test only verifies the !detection.IsDispatcher early return. Create a CliInstallationDetection with isDispatcher: true and a version lower than the minimumDispatcherVersion argument, then assert the method returns false. Use the existing test class and the IsShellDetectionUsableForPathSetup path to confirm the CliVersionComparer.IsVersionGreaterThanOrEqual branch is exercised.
🤖 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 `@Packages/src/Editor/Application/CliPinReader.cs`:
- Around line 85-108: Wrap the JObject.Parse call in CliPinReader.LoadCliPin (or
the surrounding parsing path used by LoadPackagePin and
LoadMinimumDispatcherVersionOrThrow) so malformed pin JSON returns
CliPinLoadResult.FromFailure instead of throwing a raw JsonException. Keep the
existing failure pattern consistent with the missing-file, empty-content, and
missing-key checks by catching parse exceptions and including the pinPath plus a
descriptive parse error in the failure message.
---
Nitpick comments:
In `@Assets/Tests/Editor/CliInstallationDetectorTests.cs`:
- Around line 222-237: Add a test that covers the dispatcher-version gate in
CliInstallationDetector.IsShellDetectionUsableForPathSetup, since the current
test only verifies the !detection.IsDispatcher early return. Create a
CliInstallationDetection with isDispatcher: true and a version lower than the
minimumDispatcherVersion argument, then assert the method returns false. Use the
existing test class and the IsShellDetectionUsableForPathSetup path to confirm
the CliVersionComparer.IsVersionGreaterThanOrEqual branch is exercised.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 545996d6-3282-4ee6-8fb7-a091a703b61b
⛔ Files ignored due to path filters (1)
Packages/src/Editor/Application/CliPinReader.cs.metais excluded by none and included by none
📒 Files selected for processing (16)
Assets/Tests/Editor/CliInstallationDetectorTests.csAssets/Tests/Editor/CliSetupApplicationServiceTests.csAssets/Tests/Editor/CliSetupCompatibilityTests.csPackages/src/Editor/Application/CliPinReader.csPackages/src/Editor/Application/CliSetupApplicationService.csPackages/src/Editor/Domain/CliConstants.csPackages/src/Editor/Infrastructure/CLI/CliInstallationDetector.csPackages/src/Editor/Presentation/UnityCliLoopSettingsWindow.cscli/common/clicontract/protocol_version_consistency_test.gocli/dispatcher/internal/dispatcher/dispatcher_test.gocli/release-automation/internal/automation/contract_file_at_ref_test.gocli/release-automation/internal/automation/dispatcher_minimum_version_guard.gocli/release-automation/internal/automation/dispatcher_minimum_version_guard_test.gocli/release-automation/internal/automation/protocol_minimum_version_guard.gocli/release-automation/internal/automation/protocol_minimum_version_guard_test.gocli/release-automation/internal/automation/protocol_minimum_version_parse.go
💤 Files with no reviewable changes (1)
- Packages/src/Editor/Domain/CliConstants.cs
| string content = File.ReadAllText(pinPath); | ||
| if (string.IsNullOrWhiteSpace(content)) | ||
| { | ||
| return CliPinLoadResult.FromFailure( | ||
| $"Unity CLI Loop pin file at {pinPath} is empty."); | ||
| } | ||
|
|
||
| JObject parsed = JObject.Parse(content); | ||
| string projectRunnerVersion = parsed[PIN_JSON_PROJECT_RUNNER_VERSION_KEY]?.ToString(); | ||
| string minimumDispatcherVersion = parsed[PIN_JSON_MINIMUM_DISPATCHER_VERSION_KEY]?.ToString(); | ||
| if (string.IsNullOrWhiteSpace(projectRunnerVersion)) | ||
| { | ||
| return CliPinLoadResult.FromFailure( | ||
| $"Unity CLI Loop pin file at {pinPath} is missing {PIN_JSON_PROJECT_RUNNER_VERSION_KEY}."); | ||
| } | ||
| if (string.IsNullOrWhiteSpace(minimumDispatcherVersion)) | ||
| { | ||
| return CliPinLoadResult.FromFailure( | ||
| $"Unity CLI Loop pin file at {pinPath} is missing {PIN_JSON_MINIMUM_DISPATCHER_VERSION_KEY}."); | ||
| } | ||
|
|
||
| return CliPinLoadResult.FromSuccess( | ||
| new CliPin(projectRunnerVersion, minimumDispatcherVersion)); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unwrap JObject.Parse failures into CliPinLoadResult.FromFailure.
Every other failure mode here (missing file, empty content, missing keys) is surfaced via CliPinLoadResult.FromFailure with a descriptive message. A malformed pin JSON, however, will let JObject.Parse throw a raw JsonException straight through LoadPackagePin() / LoadMinimumDispatcherVersionOrThrow(), bypassing the friendly error wrapping and surfacing an opaque stack trace during setup instead of "Unity CLI Loop cannot resolve minimum dispatcher version from the package pin: ...". CliInstallationDetector.ParseCliContractOutput already catches JsonException for similarly untrusted content, so this would keep the pattern consistent.
🛡️ Proposed fix
- JObject parsed = JObject.Parse(content);
- string projectRunnerVersion = parsed[PIN_JSON_PROJECT_RUNNER_VERSION_KEY]?.ToString();
- string minimumDispatcherVersion = parsed[PIN_JSON_MINIMUM_DISPATCHER_VERSION_KEY]?.ToString();
+ JObject parsed;
+ try
+ {
+ parsed = JObject.Parse(content);
+ }
+ catch (Newtonsoft.Json.JsonException ex)
+ {
+ return CliPinLoadResult.FromFailure(
+ $"Unity CLI Loop pin file at {pinPath} contains invalid JSON: {ex.Message}");
+ }
+ string projectRunnerVersion = parsed[PIN_JSON_PROJECT_RUNNER_VERSION_KEY]?.ToString();
+ string minimumDispatcherVersion = parsed[PIN_JSON_MINIMUM_DISPATCHER_VERSION_KEY]?.ToString();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| string content = File.ReadAllText(pinPath); | |
| if (string.IsNullOrWhiteSpace(content)) | |
| { | |
| return CliPinLoadResult.FromFailure( | |
| $"Unity CLI Loop pin file at {pinPath} is empty."); | |
| } | |
| JObject parsed = JObject.Parse(content); | |
| string projectRunnerVersion = parsed[PIN_JSON_PROJECT_RUNNER_VERSION_KEY]?.ToString(); | |
| string minimumDispatcherVersion = parsed[PIN_JSON_MINIMUM_DISPATCHER_VERSION_KEY]?.ToString(); | |
| if (string.IsNullOrWhiteSpace(projectRunnerVersion)) | |
| { | |
| return CliPinLoadResult.FromFailure( | |
| $"Unity CLI Loop pin file at {pinPath} is missing {PIN_JSON_PROJECT_RUNNER_VERSION_KEY}."); | |
| } | |
| if (string.IsNullOrWhiteSpace(minimumDispatcherVersion)) | |
| { | |
| return CliPinLoadResult.FromFailure( | |
| $"Unity CLI Loop pin file at {pinPath} is missing {PIN_JSON_MINIMUM_DISPATCHER_VERSION_KEY}."); | |
| } | |
| return CliPinLoadResult.FromSuccess( | |
| new CliPin(projectRunnerVersion, minimumDispatcherVersion)); | |
| } | |
| string content = File.ReadAllText(pinPath); | |
| if (string.IsNullOrWhiteSpace(content)) | |
| { | |
| return CliPinLoadResult.FromFailure( | |
| $"Unity CLI Loop pin file at {pinPath} is empty."); | |
| } | |
| JObject parsed; | |
| try | |
| { | |
| parsed = JObject.Parse(content); | |
| } | |
| catch (Newtonsoft.Json.JsonException ex) | |
| { | |
| return CliPinLoadResult.FromFailure( | |
| $"Unity CLI Loop pin file at {pinPath} contains invalid JSON: {ex.Message}"); | |
| } | |
| string projectRunnerVersion = parsed[PIN_JSON_PROJECT_RUNNER_VERSION_KEY]?.ToString(); | |
| string minimumDispatcherVersion = parsed[PIN_JSON_MINIMUM_DISPATCHER_VERSION_KEY]?.ToString(); | |
| if (string.IsNullOrWhiteSpace(projectRunnerVersion)) | |
| { | |
| return CliPinLoadResult.FromFailure( | |
| $"Unity CLI Loop pin file at {pinPath} is missing {PIN_JSON_PROJECT_RUNNER_VERSION_KEY}."); | |
| } | |
| if (string.IsNullOrWhiteSpace(minimumDispatcherVersion)) | |
| { | |
| return CliPinLoadResult.FromFailure( | |
| $"Unity CLI Loop pin file at {pinPath} is missing {PIN_JSON_MINIMUM_DISPATCHER_VERSION_KEY}."); | |
| } | |
| return CliPinLoadResult.FromSuccess( | |
| new CliPin(projectRunnerVersion, minimumDispatcherVersion)); | |
| } |
🤖 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/Application/CliPinReader.cs` around lines 85 - 108, Wrap
the JObject.Parse call in CliPinReader.LoadCliPin (or the surrounding parsing
path used by LoadPackagePin and LoadMinimumDispatcherVersionOrThrow) so
malformed pin JSON returns CliPinLoadResult.FromFailure instead of throwing a
raw JsonException. Keep the existing failure pattern consistent with the
missing-file, empty-content, and missing-key checks by catching parse exceptions
and including the pinPath plus a descriptive parse error in the failure message.
Source: Learnings
Summary
Part 5 of the version-gate consolidation (G4), following #1501/#1503/#1504/#1505.
MINIMUM_REQUIRED_DISPATCHER_VERSIONduplicated the pin'sminimumDispatcherVersion, andMINIMUM_REQUIRED_PROJECT_RUNNER_VERSIONhad no runtime reader left after #1501 — only CI guards still parsed both constants out of C# source with regexes. The pin JSON is now the single source and the four C# constants (two values plus derived release tags) are gone.After this PR the manually maintained version values in the whole system are down to three: the
protocolVersionpair (Go/C# wire contract) and the pin'sminimumDispatcherVersion.CliPinReaderso the package reads its own shipped pin; setup, installation detection, and the settings window take the dispatcher floor from it and fail fast when the pin cannot be loadedprojectRunnerVersionwhile preserving its intent: a protocol bump still requires a published project runner release advertising the new protocolREQUIRED_CLI_PROTOCOL_VERSIONis the one remaining C# version constantValidation
scripts/check-go-cli.sh,scripts/test-release-please-config.sh,scripts/test-stamp-release-inputs.sh,scripts/test-sync-release-please-package-releases.shcheck-release-triggers: passeduloop compile: 0 errors / 0 warnings