chore: Keep Infrastructure editor sources under 500 lines#1785
Merged
hatayama merged 2 commits intoJul 14, 2026
Merged
Conversation
…ehavior Add characterization coverage before Extract Class so scan-state transitions and reference aggregation stay observable without restructuring production code in the same commit. Co-authored-by: Cursor <cursoragent@cursor.com>
Extract Class: move asmdef reference-requirement recording out of ThirdPartyToolMigrationAssemblyUsageScanState and move login-shell CLI probe command/parsing out of CliInstallationDetector. No behavior change; characterization and existing CliInstallationDetector tests stay green. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
📝 WalkthroughWalkthroughThe change extracts CLI shell probing from ChangesCLI shell probing
Migration reference recording
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Infrastructure/CLI/CliShellInstallationProbe.cs`:
- Around line 221-233: The dispatcher detection logic in
CliShellInstallationProbe must validate the JSON DispatcherContractVersion
against CliConstants.REQUIRED_DISPATCHER_CONTRACT_VERSION before returning a
valid CliInstallationDetection, rejecting missing or mismatched contract
generations and removing the semver-only compatibility comment. Update the
mocked JSON payload around
Assets/Tests/Editor/CliInstallationDetectorTests.cs:213 to include the required
contract version field; no other test behavior should change.
🪄 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: 507f81a3-6a02-4d84-812a-52547cc10c3a
⛔ Files ignored due to path filters (3)
Assets/Tests/Editor/ThirdPartyToolMigrationAssemblyUsageScanStateTests.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/CLI/CliShellInstallationProbe.cs.metais excluded by none and included by nonePackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationAssemblyUsageReferenceRequirementRecorder.cs.metais excluded by none and included by none
📒 Files selected for processing (6)
Assets/Tests/Editor/CliInstallationDetectorTests.csAssets/Tests/Editor/ThirdPartyToolMigrationAssemblyUsageScanStateTests.csPackages/src/Editor/Infrastructure/CLI/CliInstallationDetector.csPackages/src/Editor/Infrastructure/CLI/CliShellInstallationProbe.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationAssemblyUsageReferenceRequirementRecorder.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationAssemblyUsageScanState.cs
Comment on lines
+221
to
+233
| // Why: dispatcher compatibility is enforced by the pin's minimumDispatcherVersion (semver floor), | ||
| // so identifying the dispatcher only needs its release version. | ||
| string dispatcherVersion = parsed[VERSION_JSON_DISPATCHER_VERSION_PROPERTY]?.ToString(); | ||
| if (string.IsNullOrEmpty(dispatcherVersion)) | ||
| { | ||
| return new CliInstallationDetection(null, executablePath); | ||
| } | ||
|
|
||
| return new CliInstallationDetection( | ||
| dispatcherVersion, | ||
| executablePath, | ||
| true); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Enforce exact dispatcher contract generation compatibility.
The implementation explicitly bypasses exact dispatcher contract matching, relying only on minimumDispatcherVersion. Based on learnings, the setup/compatibility parsing logic must first validate an exact match to CliConstants.REQUIRED_DISPATCHER_CONTRACT_VERSION and reject mismatched dispatcher contract generations before reporting setup compatibility.
Packages/src/Editor/Infrastructure/CLI/CliShellInstallationProbe.cs#L221-L233: Enforce an exact match againstCliConstants.REQUIRED_DISPATCHER_CONTRACT_VERSION(e.g., checking"DispatcherContractVersion"in the JSON) before returning a valid dispatcher detection, and remove the comment suggesting semver alone is sufficient.Assets/Tests/Editor/CliInstallationDetectorTests.cs#L213-L213: Update the test's mocked JSON payload (around line 200) to include the required contract version field so the test continues to pass once the exact contract match check is enforced.
📍 Affects 2 files
Packages/src/Editor/Infrastructure/CLI/CliShellInstallationProbe.cs#L221-L233(this comment)Assets/Tests/Editor/CliInstallationDetectorTests.cs#L213-L213
🤖 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/CliShellInstallationProbe.cs` around
lines 221 - 233, The dispatcher detection logic in CliShellInstallationProbe
must validate the JSON DispatcherContractVersion against
CliConstants.REQUIRED_DISPATCHER_CONTRACT_VERSION before returning a valid
CliInstallationDetection, rejecting missing or mismatched contract generations
and removing the semver-only compatibility comment. Update the mocked JSON
payload around Assets/Tests/Editor/CliInstallationDetectorTests.cs:213 to
include the required contract version field; no other test behavior should
change.
Source: Learnings
hatayama
merged commit Jul 14, 2026
0c43b33
into
feature/god-class-split-integration
2 checks passed
6 tasks
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
Changes
ThirdPartyToolMigrationAssemblyUsageScanState(committed before production restructuring).ThirdPartyToolMigrationAssemblyUsageReferenceRequirementRecorder.CliShellInstallationProbe(existing detector tests retargeted to the probe).Verification
dist/darwin-arm64/uloop compile→ 0 errors / 0 warningsuloop run-testsEditMode filterThirdPartyToolMigrationAssemblyUsageScanState|CliInstallationDetector→ 30 passeddotnet test tests/UnityCliLoop.CodeComplexity.Tests→ 12 passedDiscipline note (from PR-4 Fable review)
Made with Cursor