Skip to content

fix: Dispatcher requires the project runner pin instead of parsing CliConstants#1501

Merged
hatayama merged 2 commits into
v3-betafrom
refactor/require-project-runner-pin
Jul 5, 2026
Merged

fix: Dispatcher requires the project runner pin instead of parsing CliConstants#1501
hatayama merged 2 commits into
v3-betafrom
refactor/require-project-runner-pin

Conversation

@hatayama

@hatayama hatayama commented Jul 4, 2026

Copy link
Copy Markdown
Owner

Summary

Part 1 of the version-gate consolidation (G4). The dispatcher had a regex-based runtime fallback that parsed MINIMUM_REQUIRED_PROJECT_RUNNER_VERSION out of C# CliConstants.cs when the project-runner pin JSON was missing. The fallback substituted a semantically wrong value (the setup minimum instead of the pinned latest release) and hid pin synchronization failures. The pin JSON is now the only runtime source for resolving the project runner version.

  • Delete readDispatcherPinFromCliConstants, its CliConstants candidate paths, and the regex constants; a missing pin now fails fast with the existing dispatcherPinResolutionError guidance
  • Replace the fallback tests with missing-pin error coverage
  • CliPinSynchronizer logs a warning when the package source pin is missing instead of returning false silently
  • Editor bootstrap keeps running when pin sync fails so one bad pin cannot take down the rest of initialization

Upgrade path

If .uloop/project-runner-pin.json is missing, the dispatcher searches the package-shipped pin copies; when none is found it exits with NextActions guidance, and opening the Unity project regenerates the pin via CliPinSynchronizer.

Validation

  • scripts/check-go-cli.sh (fmt, vet, lint, tests, binary rebuild)
  • uloop compile: 0 errors / 0 warnings
  • EditMode CliPinSynchronizerTests: 6/6 passed
  • Full EditMode suite: only the 10 machine-dependent failures that also fail on clean v3-beta (verified via stash)

Review in cubic

… runner pin

The fallback parsed MINIMUM_REQUIRED_PROJECT_RUNNER_VERSION out of C# source
with a regex and substituted a semantically wrong value (the setup minimum
instead of the pinned latest release), while hiding pin synchronization
failures. The pin JSON is now the only runtime source for resolving the
project runner version, so a missing pin fails fast with the existing
resolution guidance instead of silently running a stale runner.

- Delete readDispatcherPinFromCliConstants, its candidate paths, and the
  CliConstants regex constants; replace fallback tests with missing-pin
  error coverage
- Log a warning in CliPinSynchronizer when the package source pin is
  missing instead of returning false silently
- Keep editor bootstrap running when pin sync fails so one bad pin cannot
  take down the rest of initialization
@coderabbitai

coderabbitai Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 449805a1-9dcf-4599-a23d-889b3eb1f000

📥 Commits

Reviewing files that changed from the base of the PR and between a530acd and a6c8bf1.

📒 Files selected for processing (1)
  • Assets/Tests/Editor/CliPinSynchronizerTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
  • Assets/Tests/Editor/CliPinSynchronizerTests.cs

📝 Walkthrough

Walkthrough

Cli pin synchronization now reports missing source pins with a warning and a false return, while dispatcher pin loading no longer falls back to CliConstants.cs and tests now expect missing-pin failure paths.

Changes

Unity CLI Pin Synchronizer

Layer / File(s) Summary
Pin sync return value and warning logging
Packages/src/Editor/Infrastructure/CLI/CliPinSynchronizer.cs, Packages/src/Editor/CompositionRoot/UnityCliLoopEditorBootstrapper.cs, Assets/Tests/Editor/CliPinSynchronizerTests.cs
SyncCurrentProjectPin returns a bool, missing source pins log a warning and return false, startup explicitly ignores the return value, and a test covers the warning and skipped project pin creation.

Dispatcher Pin Resolution Simplification

Layer / File(s) Summary
Remove CliConstants.cs fallback from pin loading
cli/dispatcher/internal/dispatcher/dispatcher_pin.go
loadDispatcherPin now resolves pins only from JSON pin file candidates; the CliConstants.cs fallback helpers and parsing regexes are removed.
Tests for missing pin file behavior
cli/dispatcher/internal/dispatcher/dispatcher_test.go
The fallback test is replaced with a missing-pin failure test, and an integration test checks non-zero exit plus stderr guidance when compile runs without a pin file.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant UnityCliLoopEditorBootstrapper
  participant CliPinSynchronizer
  participant FileSystem

  UnityCliLoopEditorBootstrapper->>CliPinSynchronizer: SyncCurrentProjectPin()
  CliPinSynchronizer->>FileSystem: check source pin file
  alt source pin missing
    CliPinSynchronizer->>CliPinSynchronizer: Debug.LogWarning(pinFileName, sourcePath)
    CliPinSynchronizer-->>UnityCliLoopEditorBootstrapper: false
    UnityCliLoopEditorBootstrapper->>UnityCliLoopEditorBootstrapper: continue startup
  else source pin present
    CliPinSynchronizer-->>UnityCliLoopEditorBootstrapper: true
  end
Loading
sequenceDiagram
  participant compile
  participant runDispatcherWithDeps
  participant loadDispatcherPin
  participant FileSystem

  compile->>runDispatcherWithDeps: start compile
  runDispatcherWithDeps->>loadDispatcherPin: loadDispatcherPin()
  loadDispatcherPin->>FileSystem: check project-runner-pin.json candidates
  FileSystem-->>loadDispatcherPin: not found
  loadDispatcherPin-->>runDispatcherWithDeps: error
  runDispatcherWithDeps-->>compile: non-zero exit, stderr guidance
Loading

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: dispatcher pin resolution now requires the project runner pin instead of parsing CliConstants.
Description check ✅ Passed The description matches the changeset and objectives, covering the fallback removal, warning behavior, tests, and bootstrap changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/require-project-runner-pin

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 5 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread Assets/Tests/Editor/CliPinSynchronizerTests.cs Outdated
The prefix-only regex would keep passing if the warning dropped the
missing source path, which is the part that makes the diagnostic
actionable. Exact-match the whole message instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant