fix(cli): keep a named rule's config-file options under --rules - #377
Merged
Merged
Conversation
The committed "does not mutate its inputs" test only exercised the bare 'off' string case. The nested-object branch (severity: 'off' alongside options) is the one that could plausibly regress into a mutation, since the obvious way to write it is to delete `severity` off the caller's object rather than building a new one.
--rules synthesized a fresh { ruleId: 'off' } map for every rule it
didn't name, discarding the config file's own rules map wholesale. An
L3 rule (inert until its convention is declared) then ran with no
declaration at all and reported nothing; the aggregated "this
declaration does not check what it says" diagnostic vanished with it,
so a dead glob and a compliant tree produced identical output.
resolve-args now emits --rules/--ignore as id lists (allowRules/
ignoreRules) instead of a synthesized rules map, and analyzeProject's
resolveRuleSelection (already wired in) composes the final map from
the config file's rules, keeping severity/options for every named rule
while still force-enabling it over a file 'off' and disabling every
rule not named.
'still narrows to the rules --rules names' and the pre-existing '--rules still disables every rule not named' asserted the same thing against the same fixture and rule id, differing only in Set.toEqual vs. array-spread comparison. Keep the pre-existing one.
--rules still narrows a run to the ids it names and still overrides a config-file 'off' for those ids, but no longer discards their severity and options -- it inherits them from the config file. Corrects the Precedence section in both docs-site pages and the CLI-embedded config topic, extends the config-file design doc's existing correction note, and adds the changeset for the behaviour Tasks 1-2 shipped.
run() built the same nine-field option list three times: the initial analysis, the retry after monorepo app selection, and applyScope's baseline re-analysis. A field present in one list and missing from another fails silently — a baseline analyzed under different rules reports every pre-existing finding as new. Extract the shared subset into one helper so a new option cannot reach one path and miss another, and cover the main path with a run()-level test on --rules that only passes if both the narrowing and the inherited options arrived. Also documents the public `rules` option on RunOptions and AnalyzeOptions, matching RuleSelectionInput.rules's wording.
The unreleased --ignore changeset closed by asserting --rules still replaces the config file's `rules` whole, which would have shipped as a CHANGELOG bullet contradicting the adjacent one — a user auditing it before keeping a --rules CI invocation would read that their config file is still discarded. Same claim, same release. The other four are the same statement in other places: two test comments (one declaring option inheritance under --rules out of scope, one giving whole-field replacement as the reason force-enable holds), buildRulesConfig's docstring contradicting the correction four lines below it, and a spike bullet in the 2026-07-05 design doc that the correction note above it does not reach. The configuration guide's "overrides a config-file 'off'" clause invited reading `overrides` entries as included; scoped to the top-level `rules` field in both languages, since a path-scoped 'off' is a recorded gap.
The shared option subset stops a field from reaching one analyzeProject path and missing another, but no test exercised --baseline together with a rule-selection option, so dropping the whole analyzeOpts argument at the applyScope call left the suite green. That is the severe shape: --rules X against a config setting X to 'off' force-enables X on the head side only, so every pre-existing X finding is reported as new — silent, wrong, in CI. config-file-project sets seo/title-presence to 'off', which makes the two sides disagree unless the selection reaches both.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe CLI now separates rule selection from complete rule-map replacement. ChangesRule selection behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant resolveArgs
participant analyzeProject
participant resolveRuleSelection
participant Analysis
CLI->>resolveArgs: Parse --rules and --ignore
resolveArgs->>analyzeProject: Pass allowRules and ignoreRules
analyzeProject->>resolveRuleSelection: Resolve configured and selected rules
resolveRuleSelection-->>analyzeProject: Return final rule settings
analyzeProject->>Analysis: Execute selected rules
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/cli/test/analyze-project.test.ts (1)
251-251: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName the test after the observed behavior.
restores the self-diagnostic a discarded declaration silencedincludes the regression cause. Use a behavior-focused name such asreports an unmatched placement declaration under --rules.As per coding guidelines, test names must describe the behavior they verify, not the reasoning behind the test.
Suggested rename
-it('restores the self-diagnostic a discarded declaration silenced', async () => { +it('reports an unmatched placement declaration under --rules', async () => {🤖 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/cli/test/analyze-project.test.ts` at line 251, Rename the test case currently titled “restores the self-diagnostic a discarded declaration silenced” to a behavior-focused name describing that an unmatched placement declaration is reported under --rules, without mentioning the regression cause.Source: Coding guidelines
packages/cli/test/resolve-args.test.ts (1)
130-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the CLI never emits
rulesfor every selection shape.The contract reserves
rulesfor whole-map callers. These tests verify the ID lists but notoptions?.rulesfor--ignore-only or combined flags. Add that assertion so a synthesized map regression cannot pass this file.Suggested assertions
it('carries --ignore as an id list, independent of --rules', () => { const options = resolve('--ignore', 'seo/canonical-url').options; expect(options?.ignoreRules).toEqual(['seo/canonical-url']); expect(options?.allowRules).toBeUndefined(); + expect(options?.rules).toBeUndefined(); }); it('carries both id lists when both flags are passed', () => { const options = resolve('--rules', 'seo/title-presence', '--ignore', 'seo/canonical-url').options; expect(options?.allowRules).toEqual(['seo/title-presence']); expect(options?.ignoreRules).toEqual(['seo/canonical-url']); + expect(options?.rules).toBeUndefined(); });🤖 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/cli/test/resolve-args.test.ts` around lines 130 - 140, Extend both tests around resolve() to assert that options?.rules is undefined for --ignore-only and combined --rules/--ignore invocations. Keep the existing allowRules and ignoreRules assertions unchanged, ensuring every tested selection shape verifies the CLI does not emit a synthesized rules map.
🤖 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 `@docs/superpowers/plans/2026-08-06-rule-selection.md`:
- Line 487: Add a language tag, such as text, to the opening fence of the JSDoc
example in the rule-selection plan, leaving the example content unchanged.
In `@packages/cli/docs/config.md`:
- Around line 97-101: Clarify the combined --rules/--ignore semantics in
packages/cli/docs/config.md (lines 97-101): describe preservation relative to
the setting resolved before --ignore, and state that rules not listed in --rules
remain disabled. Apply the same clarification in
docs/src/content/docs/guides/(setup)/configuration.mdx (line 228) and the
equivalent Japanese wording in
docs/src/content/docs/ja/guides/(setup)/configuration.mdx (line 200). Regenerate
packages/cli/src/docs/generated.ts (line 27) from the Markdown source; do not
edit the generated file manually.
In `@packages/cli/src/resolve-args.ts`:
- Around line 229-234: Correct the comment above allowRules in resolveArgs so it
no longer claims empty parsed lists remain distinguishable from omitted flags;
state that both become undefined here, and mention the distinction only if
needed for resolveRuleSelection, which accepts [] separately.
---
Nitpick comments:
In `@packages/cli/test/analyze-project.test.ts`:
- Line 251: Rename the test case currently titled “restores the self-diagnostic
a discarded declaration silenced” to a behavior-focused name describing that an
unmatched placement declaration is reported under --rules, without mentioning
the regression cause.
In `@packages/cli/test/resolve-args.test.ts`:
- Around line 130-140: Extend both tests around resolve() to assert that
options?.rules is undefined for --ignore-only and combined --rules/--ignore
invocations. Keep the existing allowRules and ignoreRules assertions unchanged,
ensuring every tested selection shape verifies the CLI does not emit a
synthesized rules map.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: 8cf43ddf-c3b4-40cc-b6d8-e8cd46708fd5
📒 Files selected for processing (23)
.changeset/ignore-flag-config-options.md.changeset/rules-flag-keeps-options.mddocs/src/content/docs/guides/(setup)/configuration.mdxdocs/src/content/docs/ja/guides/(setup)/configuration.mdxdocs/superpowers/plans/2026-08-06-rule-selection.mddocs/superpowers/specs/2026-07-05-config-file-design.mddocs/superpowers/specs/2026-08-06-rule-selection-design.mdpackages/cli/docs/config.mdpackages/cli/src/docs/generated.tspackages/cli/src/index.tspackages/cli/src/resolve-args.tspackages/cli/src/rule-selection.tspackages/cli/src/rules-config.tspackages/cli/test/analyze-project.test.tspackages/cli/test/fixtures/dead-declaration-project/package.jsonpackages/cli/test/fixtures/dead-declaration-project/src/app.htmlpackages/cli/test/fixtures/dead-declaration-project/src/lib/Widget.sveltepackages/cli/test/fixtures/dead-declaration-project/src/routes/+page.sveltepackages/cli/test/fixtures/dead-declaration-project/svelte-vitals.config.mjspackages/cli/test/resolve-args.test.tspackages/cli/test/rule-selection.test.tspackages/cli/test/run-baseline.test.tspackages/cli/test/run.test.ts
…ment The --rules + --ignore precedence sentence claimed a rule named by neither flag keeps its file-configured severity and options; under --rules, resolveRuleSelection forces it to 'off' first, so it does not. Also tag a bare JSDoc fence in the design plan, and correct a resolve-args.ts comment that claimed empty and omitted id-list flags stay distinguishable, when they both collapse to undefined.
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.
Why
--rules Xdiscarded X's own configuration, so an option-configured rule could not be run alone.A field report on 0.41.0: following an instruction sheet that used
--rules architecture/reserved-name-placement, a real project got zero findings from a rule whose options it had declared — and read that as "the tree complies".For a rule that is inert until its convention is declared, "built-in defaults" means no convention at all. So narrowing a CI run to a few rule ids silently swapped the project's configuration for none.
And it was doubly silent, which the field established and the original design did not record. A discarded options map leaves no declaration, so the rule's own aggregated "this declaration does not check what it says" finding disappeared alongside its findings. A dead glob and a fully compliant tree produced identical output at exit 0 — the exact reading the charter's inverse-precision gate exists to prevent, and why this half of the defect was the one worth fixing first.
--ignore's half shipped separately as a patch. This is the other half.The root cause: one field carrying two meanings
AnalyzeOptions.ruleshad two consumers that meant different things by it.--rules'off'entries, one per rule not namedrulesobject, written as a plugin optionThe CLI's value was partial by construction and expressed selection through the absence of an entry. The plugin's was complete and expressed configuration through presence.
2026-07-05-config-file-design.md§3 chose whole-field replacement and named the constraint in a parenthetical: "which works by generatingoffentries for everything unlisted". That reasoning was correct given the encoding — selection expressed as absence cannot survive a merge, because absence is not a value you can layer. The encoding is what had to change.An earlier attempt tried a plain
{ ...file.rules, ...opts.rules }merge and broke the documented behaviour that passage protects:--rules Xforce-enabling X over a config-file'off', measured at 2 findings before and 0 after. It was discarded.What
Flags select; the config file configures.
'off'is the only setting that is purely selection, so it is the only one a flag overrides. A severity or an options map is configuration and survives.Three properties now hold together — each rejected scheme achieved exactly two, and it was the same pair that failed both times. Whole-field replacement held narrowing and force-enable, losing options. A plain merge held narrowing and options, losing force-enable. Properties 2 and 3 cannot co-hold while selection is encoded as absence, because one slot has to say both "no entry, so enabled" and "an entry, so configured".
--rules Xruns only X.--rules Xforce-enables X over a config-file'off'.--rules Xkeeps X's severity and options from the config file.rulesregains one honest meaning — the whole map, replaced, which is what the Vite plugin and programmatic callers already pass. The CLI's flags travel as id lists,allowRulesandignoreRules, andignoreRulesis applied last so deny still beats allow.The composition is now a pure function in its own module,
packages/cli/src/rule-selection.ts. That is not tidying: the--ignoredefect reached a release because the composition sat insideanalyzeProject, which loads a config file and detects a project before it gets there, so the only way to exercise it was a full analyze run over a fixture — and no such test existed for the flag combinations. Every row of the design's case table is now a unit test.What is deliberately not solved
config.overridesstill suppresses per-path, and no flag reaches into it. A rule scoped off undersrc/legacy/**stays off there under--rules. Recorded rather than fixed: making a selection flag reach into path-scoped configuration is a larger question. The guides now scope their claim to the top-levelrulesfield so they do not imply otherwise.--rules X --category <a category X is not in>runs nothing at exit 0.categoriesfilters after selection. Pre-existing and untouched here.buildRulesConfigstays exported with its tests, though the CLI no longer calls it. It is public API; removing it is a separate breaking decision, not one to make inside a behaviour fix. Its docstring now says so.Verification
cli836,core1266,vite206 pass;tsc --noEmitclean in all three; lint and format clean across 957 files; docs gates 27;floor-smoke8/8.The whole-branch review ran 23 engine-level assertions putting every case-table row through
selectRules/settingSeverity/settingOptions/resolveRuleOptionson a realdefineConfig, rather than checking the returned map's shape — a rewrite producing a setting the engine treats as off would otherwise pass a map-shaped test while defeating the design.{ severity: 'off', options }→{ options }givessettingSeverity === undefined,resolveRuleOptions().max === 3rather than the built-in 200, and wakes the L3 mention path. It also tried to construct an input where two of the three properties hold and the third fails, across seven setting shapes, and could not.End to end against the built
dist:--rules architecture/component-sizereports "Component is 7 lines (over 3)" — the file's threshold, not the built-in — and the dead-glob fixture emits "The declaration 'placements.e2e → src/nowhere/**' does not check what it says: matched no directory." Both halves of the silence are gone.The interesting part: three of four forwarding sites were unguarded, and the fix is structural
Deleting
allowRules: opts.allowRulesat any of the threerun()-level call sites left all 834 tests passing. Only the one insideanalyzeProjectwas covered, because every test entered throughanalyzeProjectand none reachedrun().The measured failure for the
--diff/--baselinesite:--rules X --baseline origin/mainagainst a config setting X to'off'force-enables X on the head side and not on the baseline side, so every pre-existing X finding is reported as new — silent, wrong, in CI.Fixed by extraction rather than by three more tests. The three sites passed an identical field list; it is now built once, so a future option cannot reach one path and miss another. Deleting a field from the helper now fails a test. One site genuinely differs —
applyScopesupplies its owncwdfor the baseline checkout — so the helper omitscwdand it stays explicit where it is needed.A second gap survived that: removing the whole
analyzeOptsline was still silent, since no test paired--baselinewith a rule-selection option. That is closed too, and the mutation now prints the exact CI failure above.Five statements about this mechanism were false before merge, and a false statement in prose is this defect in another medium. The one that mattered most: the unreleased
--ignorechangeset still said "--rules's own whole-field replacement is unchanged". Both changesets ship in the same release, so the CHANGELOG would have carried two adjacent bullets contradicting each other about--rules— read by exactly the user auditing whether to keep a--rulesCI invocation. A test comment also declared this branch's own fix out of scope, in the file holding its end-to-end guards.Design:
docs/superpowers/specs/2026-08-06-rule-selection-design.md. Plan:docs/superpowers/plans/2026-08-06-rule-selection.md, corrected in place each time execution proved a step wrong — including one suggested fixture that would have shipped a hollow test, becauseanalyzeProjectloads the config from the checkout's owncwdand both sides would have produced the finding regardless.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--rules.--ignorereliably disables selected rules and takes precedence when both options target the same rule.Documentation