Enforce cognitive-complexity limit everywhere; narrow scripts biome override - #1729
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request refactors scripts, mutation execution, test utilities, and code-quality detectors into focused helpers, tightens Biome lint enforcement, and replaces selected assignment expressions with explicit operations while preserving existing behavior. ChangesScript and source refactoring
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f01ab5c26a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@scripts/mutation.ts`:
- Around line 81-95: Update applyArg so recognized VALUE_FLAGS entries without a
following token immediately raise a clear CLI usage error instead of being added
to positional. Preserve consuming and returning true when a value is present,
and ensure missing values for flags such as --timeout, --jobs, --source, and
--test fail fast consistently.
In `@scripts/safe-upgrade.ts`:
- Around line 270-287: Document the invariant at the non-null assertion in
printUpgrades: add a concise comment explaining that checkUpgrade sets
newVersion and newPublishedAt together and upgrades is filtered by non-null
newVersion, so newPublishedAt is guaranteed present. Keep the existing non-null
assertion and avoid adding a defensive branch.
In `@test/lib/checkout-pricing-consistency.test.ts`:
- Line 158: In the modifier-generation loop, sample rng.randInt(0, 4) once
before the loop and store it in a local count variable, then iterate against
that variable instead of calling rng.randInt repeatedly in the loop condition.
- Around line 113-114: Update the Rng.pick signature and the corresponding
makeRng implementation to accept readonly T[] instead of mutable T[], allowing
as const tuples to type-check while preserving existing behavior.
In `@test/lib/code-quality/detectors.ts`:
- Around line 426-439: The template substitution scanner in
skipTemplateSubstitution must ignore comment contents while tracking brace
depth. Extend its scan to skip line and block comments, then add a direct
regression test for a substitution containing `}` in a comment followed by a
nested template such as `${/* } */ `x,y`}`, verifying parseArgList does not
misinterpret the comma.
- Around line 779-789: Update skipTypeParams to use the existing angleDepthDelta
helper when adjusting angle-bracket depth, so the > in an arrow token (=>) is
not treated as the closing type-parameter delimiter; preserve returning
immediately after depth reaches zero and verify parseTypeAliasBody recognizes
aliases such as type A<T = () => void>.
In `@test/lib/fold-tree.test.ts`:
- Around line 65-77: The fold outcome validation must verify that rejected folds
preserve the quantity from before the fold, not merely differ from the attempted
running total. Update the fold test flow around foldChild and foldOutcomeValid
to capture the pre-fold recorded quantity, pass it into the validator, and
assert exact equality for rejected outcomes while retaining the accepted-case
checks.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 73bac0bc-b53b-4729-af06-500db9171533
📒 Files selected for processing (16)
biome.jsonscripts/compact-test-reporter.tsscripts/mutation.tsscripts/mutation/isolation.tsscripts/mutation/runner.tsscripts/safe-upgrade.tsscripts/stripe-mock.tstest/lib/checkout-pricing-consistency.test.tstest/lib/code-quality.test.tstest/lib/code-quality/detectors.tstest/lib/db/migration-restore/helpers.tstest/lib/fold-tree.test.tstest/lib/i18n-coverage.test.tstest/lib/square/client.test.tstest/shared/bunny-db.test.tstest/test-utils/fake-dom.ts
💤 Files with no reviewable changes (1)
- biome.json
Replace the blanket scripts/**/*.ts exclusion that disabled four rules with a narrower override that only turns off noConsole and noExcessiveCognitiveComplexity, so noAssignInExpressions and noImplicitAnyLet are now enforced in scripts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RJV4Bsf2owkANcSfn5sZgm
Remove the noExcessiveCognitiveComplexity relaxations for scripts (was off) and tests (was raised to max 30), so every function is held to the base max of 15. Refactor the 20 functions that exceeded it across scripts/ and test/ by extracting well-named helpers and flattening nested conditionals, preserving exact behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RJV4Bsf2owkANcSfn5sZgm
Turn both rules on at the top level and rewrite the six sites that tripped them (assignment-in-expression via ??= and an implicit-any let), preserving behavior. Also restructure skipTypeParams so its fall-through return stays on the covered path, keeping detectors.ts at 100% coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RJV4Bsf2owkANcSfn5sZgm
…iant Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RJV4Bsf2owkANcSfn5sZgm
dd6c1af to
2c0e70f
Compare
|
Thanks for the review. Disposition of the 7 comments (this PR's refactors were required to preserve existing behavior, which frames several of these): Applied:
Skipped (behavior-preserving — same as pre-PR
Skipped (would regress CI):
Skipped (pre-existing edge case, out of scope):
Happy to open a follow-up for the fail-fast / comment-aware-tokenizer improvements if you'd like them tracked separately. Generated by Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@scripts/compact-test-reporter.ts`:
- Around line 140-163: Refactor assignAtField to use a typed Record-based
key-to-handler dispatch, matching the VALUE_FLAGS pattern, with handlers for
file, line, and column that perform the existing assignments and numeric
conversions. Type the key or dispatch access so TypeScript enforces all
supported fields, while preserving current behavior for unknown keys.
In `@scripts/stripe-mock.ts`:
- Around line 307-325: Preserve the first meaningful startup error in
startStripeMock by updating lastStartupError only when result.error is
non-empty, so later retry results with empty errors cannot overwrite accumulated
stderr.
In `@test/lib/checkout-pricing-consistency.test.ts`:
- Around line 119-125: Update the `rand` helper so its returned value is always
strictly less than 1, including when `seed` equals `0x7fffffff`; adjust the
normalization or clamp the result before `pick` and `randInt` consume it. Verify
`pick` cannot select beyond the array and `randInt` never exceeds `hi`.
In `@test/lib/code-quality.test.ts`:
- Around line 401-414: Add a concise comment immediately before
contents.get(file)! in collectLineViolations explaining that ensureLoaded()
populates contents using the same files list, so every requested file is
guaranteed to be present; retain the non-null assertion.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c054351e-20cf-49e0-ab21-b71e53e23ea7
📒 Files selected for processing (22)
biome.jsonscripts/compact-test-reporter.tsscripts/mutation.tsscripts/mutation/isolation.tsscripts/mutation/runner.tsscripts/safe-upgrade.tsscripts/stripe-mock.tssrc/features/admin/index.tssrc/features/api/payment-processing/index.tssrc/features/public/ticket-form.tssrc/ui/client/admin/markdown-preview.tssrc/ui/client/order.tstest/lib/checkout-pricing-consistency.test.tstest/lib/code-quality.test.tstest/lib/code-quality/detectors.tstest/lib/db/migration-restore/helpers.tstest/lib/fold-tree.test.tstest/lib/i18n-coverage.test.tstest/lib/square/client.test.tstest/shared/bunny-db.test.tstest/test-utils/fake-dom.tstest/test-utils/test-browser.ts
… invariant - compact-test-reporter: replace assignAtField if-chain with a typed Record dispatch, matching the VALUE_FLAGS pattern per coding guidelines - stripe-mock: only overwrite lastStartupError with non-empty errors so a later empty retry can't drop the first meaningful stderr - code-quality.test: document why contents.get(file)! is safe Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RJV4Bsf2owkANcSfn5sZgm
|
Applied 3 of 4 in
Skipped: the Generated by Claude Code |
Note three review items deferred as out of scope for the complexity refactor: arrow-aware skipTypeParams, comment-skipping in skipTemplateSubstitution, and a stronger foldOutcomeValid rejection assertion. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RJV4Bsf2owkANcSfn5sZgm
Add the two other deferred review items: mutation.ts CLI value-flag fail-fast, and the seeded PRNG rand() can-return-1 normalization. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RJV4Bsf2owkANcSfn5sZgm
Both branches appended a deferred-follow-ups section to TODO.md at the same spot; the resolution keeps this branch's PR #1736 notes and main's PR #1729 notes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnAupwn2rCio7DENdnWh2w
`deno task mutation --source` (or `--test`/`--jobs`/`--timeout`) with no following value used to quietly treat the flag itself as a positional glob. That surfaced much later as a confusing "No source files matched" or "Unexpected positional argument" error, hiding the real mistake. It now reports a clear "Missing value for --source." straight away. The pure argument parsing moves out of the CLI script into a new scripts/mutation/args.ts so it can be unit-tested on its own, leaving scripts/mutation.ts as a thin shell that expands globs and runs. Adds tests for parseArgs with full line/branch coverage and a 100% mutation kill rate, including the missing-value regression. Origin: TODO.md "Code-quality detector & test-strengthening follow-ups (from PR #1729)", item 3. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0191NXZLeokw3pYtJCwJK4VN
Summary
Removes biome overrides that let parts of the codebase off the hook, and refactors the code that then failed to lint.
Changes
biome.jsonscripts/**/*.tsoverride (which disablednoExcessiveCognitiveComplexity,noAssignInExpressions,noConsole, andnoImplicitAnyLet) with one that only keepsnoConsoleoff — console output is intentional in CLI/build/test scripts. (noAssignInExpressionsandnoImplicitAnyLetareoffat the top level, so they remain off everywhere; this override no longer redundantly re-disables them.)test/**override that raisednoExcessiveCognitiveComplexityto max 30. Tests now use the base max of 15.Net effect:
noExcessiveCognitiveComplexity(max 15) is now enforced acrosssrc,test, andscriptswith no exceptions.Refactors — 20 functions that exceeded complexity 15 were refactored by extracting well-named helpers and flattening nested conditionals, preserving exact behavior:
scripts/:compact-test-reporter.ts,mutation.ts,mutation/isolation.ts,mutation/runner.ts,safe-upgrade.ts(×2),stripe-mock.tstest/:code-quality/detectors.ts(×5),checkout-pricing-consistency.test.ts,code-quality.test.ts,db/migration-restore/helpers.ts,fold-tree.test.ts,i18n-coverage.test.ts,square/client.test.ts,shared/bunny-db.test.ts,test-utils/fake-dom.tsVerification
deno task lint:ci— 1919 files, no errors/warnings.deno checkover thesrc/test/cligate — passes.🤖 Generated with Claude Code
Summary by CodeRabbit