fix: Keep Go and C# complexity checks under fifteen#1403
Merged
Conversation
Split large dispatch, launch, retry, and IPC response flows into focused helpers so the advisory cyclomatic complexity check passes without changing command behavior.
Split large Unity-side migration and input simulation flows into focused helpers so the CA1502 complexity gate passes while preserving existing behavior.
Split CLI completion, error classification, skills dispatch, value conversion, and request metadata test helpers so the stricter cyclomatic complexity threshold can pass.
Refactor remaining Unity complexity hotspots into smaller parsing, planning, input, and response-building helpers so the stricter CA1502 limit can pass. Update the Go, C#, script, and CI complexity defaults to enforce the new threshold consistently.
Contributor
📝 WalkthroughWalkthroughThe PR lowers the configured code-complexity limit to 15 and splits CLI, third-party migration, dynamic-code, and input/control flows into helper methods, state objects, and smaller dispatch paths. It also updates replay, simulation, play-mode, keyboard-symbol, and serialized-property helpers. ChangesCode-complexity threshold update
CLI command routing and Unity connection plumbing
Third-party tool migration scanning and planning
Dynamic code and source scanning helpers
Input replay, simulation, and control helpers
Serialized property value dispatch
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 |
There was a problem hiding this comment.
2 issues found across 43 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/internal/cli/launch_options.go">
<violation number="1" location="cli/internal/cli/launch_options.go:101">
P3: Launch option value parsing duplicates existing CLI flag parsing logic instead of reusing the shared path. This increases maintenance risk and can cause inconsistent argument handling across commands.</violation>
</file>
Note: This PR contains a large number of files. cubic only reviews up to 40 files per PR, so some files may not have been reviewed. cubic prioritizes the most important files to review.
On a pro plan you can use ultrareview for larger PRs.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
Packages/src/Editor/FirstPartyTools/FindGameObjects/GameObjectFinder/ComponentPropertySerializer.cs (1)
119-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Quaternion-as-default is acceptable but couples the predicate and switch.
GetUnityStructSerializedPropertyValueis only reachable whenIsUnityStructSerializedPropertyTypereturns true, so the assertedQuaterniondefault is safe today. Note the latent coupling: adding a new struct type to the predicate without a matchingcasehere would silently returnquaternionValuein release builds (the assert is compiled out). Keeping these two members in sync is required. TheDebug.Assertprecondition use itself is appropriate.🤖 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/FirstPartyTools/FindGameObjects/GameObjectFinder/ComponentPropertySerializer.cs` around lines 119 - 141, `GetUnityStructSerializedPropertyValue` currently relies on a `Quaternion` default branch that is only safe because `IsUnityStructSerializedPropertyType` filters the inputs, creating a hidden coupling between the predicate and the switch. Keep these two members in sync by updating `GetUnityStructSerializedPropertyValue` whenever `IsUnityStructSerializedPropertyType` changes, and prefer making the default branch explicitly match the supported `SerializedPropertyType.Quaternion` case so any future struct type additions require an intentional new case rather than silently falling through to `quaternionValue` in release builds. The existing `Debug.Assert` precondition is fine; the fix is to ensure the switch remains exhaustive for the predicate’s accepted types.
🤖 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 `@cli/internal/cli/launch_options.go`:
- Around line 68-78: The applyLaunchMaxDepthOption parser currently accepts any
negative integer for --max-depth, but only -1 should be allowed as the unlimited
sentinel. Update the validation in applyLaunchMaxDepthOption to reject values
below -1 before assigning options.maxDepth, returning invalidValueArgumentError
for out-of-range input while preserving the existing handling for valid integers
and the nextLaunchOptionIndex flow.
In `@cli/internal/cli/tools.go`:
- Around line 307-312: The convertObjectValue helper currently accepts JSON null
because json.Unmarshal into the parsed map returns a nil map without error, so
object-typed options can slip through as non-objects. Update convertObjectValue
to explicitly reject null before returning, and only accept values that
unmarshal to a non-nil map; keep the invalidValueArgumentError path for option
and value validation so callers still get a consistent error for
convertObjectValue in tools.go.
In
`@Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SourceShaper.cs`:
- Around line 166-207: The attributed type detection in
TryAnalyzeAttributedTypeDeclaration is missing support for access modifiers
between the attribute block and the type keyword, so cases like attributed
public classes are misclassified. After SkipAttributeBlock and SkipWhitespace,
add the same access-modifier skipping logic used by
TryAnalyzeModifiedTypeDeclaration (for example via SkipAccessModifiers and then
checking IsTypeDeclarationKeyword on the returned position) before setting
result.HasTypeDeclaration and calling SkipTopLevelBlock.
In
`@Packages/src/Editor/FirstPartyTools/FindGameObjects/GameObjectFinder/ComponentPropertySerializer.cs`:
- Around line 112-116: The `ComponentPropertySerializer` code uses an
unqualified `Debug.Assert` in the primitive string branch, while the rest of the
class already uses `UnityEngine.Debug.Assert` consistently. Update the assertion
in the `SerializeProperty` flow to use the fully qualified
`UnityEngine.Debug.Assert` reference so the style matches the other assertions
in this file.
In
`@Packages/src/Editor/FirstPartyTools/SimulateMouseUi/SimulateMouseUiUseCase.cs`:
- Around line 485-495: The click simulation in SimulateMouseUiUseCase should
always send pointerUp for click-only targets, even when PressTarget is null.
Update the pointer handling around the resolvedTargets block so the pointerUp
dispatch uses the stored click/press target from pointerData (set by
CreateResolvedPressablePointerTargets) rather than being gated only on
PressTarget; keep the existing pointerDown behavior tied to
PressTarget/RawTarget, but ensure IPointerUpHandler and IPointerClickHandler
targets still receive pointerUp.
In
`@Packages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationArgumentRules.cs`:
- Around line 103-133: The comma handling in ConsumeCharacter currently ignores
comments, so top-level splitting can break on commas inside block or single-line
comments. Update ThirdPartyToolMigrationArgumentRules to recognize and skip
comment regions before treating a comma as an argument separator, either by
adding comment scan modes alongside the existing literal/nesting handling or by
reusing the same code-text masking used elsewhere. Make sure the fix keeps
TryMigrateLegacyToolAttributeList receiving intact argument segments from
ThirdPartyToolMigrationAttributeRules.
In
`@Packages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationFastAssemblyRequirementCollector.cs`:
- Around line 530-583: The assembly-scoped legacy screenshot/capture/timeout
checks in ThirdPartyToolMigrationFastAssemblyRequirementCollector are ignoring
legacy aliases because they pass Array.Empty<string>() into the
ThirdPartyToolMigrationRules methods. Update
CollectFastFirstPartyScreenshotRequirementsAsync to thread
scanState.AssemblyScopedLegacyAliasesByDirectory through and use
GetAssemblyScopedNames(..., assemblyDirectory) when calling
HasLegacyEditorWindowCaptureUtilitySourceTarget,
HasLegacyScreenshotSourceTarget, and
HasLegacyEditorWindowCaptureUtilityTimeoutMigration so alias-qualified usages
are detected consistently with the scan-state paths.
- Around line 518-527: The FirstPartyScreenshotRequirementScan result currently
treats timeout migrations as affecting source requirements but not the target
flag, so a timeout-only case can be missed. Update the return in
ThirdPartyToolMigrationFastAssemblyRequirementCollector to include
hasTimeoutMigration in the HasMigrationTarget calculation alongside the existing
source-target checks, using the FirstPartyScreenshotRequirementScan construction
as the place to fix it.
---
Nitpick comments:
In
`@Packages/src/Editor/FirstPartyTools/FindGameObjects/GameObjectFinder/ComponentPropertySerializer.cs`:
- Around line 119-141: `GetUnityStructSerializedPropertyValue` currently relies
on a `Quaternion` default branch that is only safe because
`IsUnityStructSerializedPropertyType` filters the inputs, creating a hidden
coupling between the predicate and the switch. Keep these two members in sync by
updating `GetUnityStructSerializedPropertyValue` whenever
`IsUnityStructSerializedPropertyType` changes, and prefer making the default
branch explicitly match the supported `SerializedPropertyType.Quaternion` case
so any future struct type additions require an intentional new case rather than
silently falling through to `quaternionValue` in release builds. The existing
`Debug.Assert` precondition is fine; the fix is to ensure the switch remains
exhaustive for the predicate’s accepted types.
🪄 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: e8134537-6940-4cdd-bc17-8884af28f6bf
⛔ Files ignored due to path filters (2)
Packages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationAssemblyUsageScanState.cs.metais excluded by none and included by nonetools/UnityCliLoop.CodeComplexity/CodeMetricsConfig.txtis excluded by none and included by none
📒 Files selected for processing (41)
.github/workflows/code-complexity.ymlPackages/src/Editor/FirstPartyTools/Common/InputRecording/InputReplayer.csPackages/src/Editor/FirstPartyTools/ControlPlayMode/ControlPlayModeUseCase.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/DynamicCodeLiteralHoister.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/PreUsingResolver.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SourceShaper.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/ExecuteDynamicCodeUseCase.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/Execution/AwaitableHelper.csPackages/src/Editor/FirstPartyTools/FindGameObjects/GameObjectFinder/ComponentPropertySerializer.csPackages/src/Editor/FirstPartyTools/SimulateMouseUi/SimulateMouseUiUseCase.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationArgumentRules.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationAssemblyUsageAnalyzer.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationAssemblyUsageAsyncAnalyzer.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationAssemblyUsageScanState.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationCSharpRules.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationDetectionRules.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationFastAssemblyRequirementCollector.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationModels.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationPlanBuilder.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationTargetScanner.csPackages/src/Editor/Infrastructure/ThirdPartyToolMigration/ThirdPartyToolMigrationTimingInvocationRules.csPackages/src/Runtime/SimulateKeyboard/KeySymbolMap.cscli/.golangci-complexity.ymlcli/internal/cli/completion.gocli/internal/cli/connection_retry.gocli/internal/cli/connection_retry_flow.gocli/internal/cli/error_envelope.gocli/internal/cli/error_envelope_classification.gocli/internal/cli/launch.gocli/internal/cli/launch_options.gocli/internal/cli/run.gocli/internal/cli/run_dispatch.gocli/internal/cli/skills.gocli/internal/cli/skills_dispatch.gocli/internal/cli/tools.gocli/internal/unityipc/client.gocli/internal/unityipc/client_test.goscripts/check-code-complexity.shtools/UnityCliLoop.CodeComplexity/CodeComplexityAnalyzerRunner.cstools/UnityCliLoop.CodeComplexity/CodeComplexityModels.cstools/UnityCliLoop.CodeComplexity/CommandLineOptions.cs
💤 Files with no reviewable changes (1)
- cli/internal/cli/error_envelope.go
Tighten CLI argument validation and cover review-reported parser edge cases. Preserve C# migration and UI dispatch behavior for attributed types, comments, aliases, and click-only targets.
Merged
This was referenced Jul 6, 2026
This was referenced Jul 21, 2026
Merged
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
Verification
CODE_COMPLEXITY_FAIL_ON_EXCEEDED=true scripts/check-code-complexity.shscripts/check-go-cli.shdotnet test tests/UnityCliLoop.CodeComplexity.Tests/UnityCliLoop.CodeComplexity.Tests.csproj --configuration Releasecli/dist/darwin-arm64/uloop compile --project-path "$(git rev-parse --show-toplevel)"cli/dist/darwin-arm64/uloop run-tests --project-path "$(git rev-parse --show-toplevel)" --test-mode EditMode --filter-type regex --filter-value ".*(SourceShaperTests|PreUsingResolver.*Tests|DynamicCodeLiteralHoisterTests|ExecuteDynamicCodeUseCaseTests|ControlPlayModeUseCaseTests|ThirdPartyToolMigrationRulesTests|ThirdPartyToolMigrationFileServiceTests).*"cli/dist/darwin-arm64/uloop run-tests --project-path "$(git rev-parse --show-toplevel)" --test-mode PlayMode --filter-type regex --filter-value ".*SimulateMouseUiTests.*"/Users/a12115/dotfiles/.claude/skills/autoreview/scripts/autoreview --mode branch --base origin/v3-beta