feat(check): restrict a run to named rules with --rule - #385
Conversation
|
@claude /review |
|
Claude finished @thecodedrift's task in 5m 49s —— View job Review completeReview mode: incremental — read 0 prior review thread(s) before reviewing.
Findings1. 2. 3. 4. What looked solid
|
| let mutableSelection: RuleSelection | undefined; | ||
| if (requestedRules.length > 0) { | ||
| try { | ||
| mutableSelection = await resolveRuleSelection(cwd, requestedRules); |
There was a problem hiding this comment.
[New] Efficiency (minor): resolveRuleSelection here already reads every engine's rule directory via listRuleIds(cwd, "sg" | "vale" | "runtime") internally. A few lines below, astGrepRuleIds = await listRuleIds(cwd, "sg") (line 265) and later valeRuleIds = await listRuleIds(cwd, "vale") (line 380) each re-read the same directories from scratch, discarding the lists resolveRuleSelection already fetched moments earlier. Not expensive (a couple of extra readdirs), but RuleSelection — or a small helper alongside it — could expose the unfiltered per-engine lists so check doesn't walk the same rule directories twice per run.
e32c484 to
790f4d7
Compare
All three medium findings verified against the code and fixed in b1e0d52: Vale configs are now validated for every rule (only the written blocks are narrowed), — AI Coding Agent |
`taskless check --rule <id>` (repeatable) measures one rule over the whole project, instead of running every rule and filtering the JSON afterwards. Each engine narrows by its own mechanism, so a filtered run reports exactly what an unfiltered run reports for that rule: ast-grep gets an anchored `--filter`, leaving the config and the walk byte-identical; Vale's config is assembled from only the selected rules, each rule's own matchers verbatim, so its scope is unchanged; runtime rules are filtered before planning, so the signature gate still applies. An id no rule directory has is refused with RULE_NOT_FOUND naming it, because a typo that measured nothing reports the same "0 findings" a clean rule does.
… scan `assembleValeConfig` narrowed `ruleIds` before the validation loop, so an unselected rule's config was never read and could never contribute to `refusals`. In a project with a schema-rejected `bad-rule`, an unfiltered `check` refused the whole Vale engine and exited non-zero, while `check --rule good-rule` assembled and ran clean — the filtered run reporting findings the unfiltered run never produces, which is the one equality the flag rests on. Narrowing now applies to which validated blocks are written, never to which rules are validated. `resolveRuleSelection` recomputed "which engines hold this id" from a locally built per-engine map; it now calls `findRuleEngines`, the one implementation that already answers it, which also stops `check` walking every rule directory twice. `extractRuleFilters` scanned argv itself and so did not know that `--timeout` consumes the token after it: for `check --timeout --rule no-eval` it claimed `no-eval` as a rule id while `splitRawArguments` had already made it a path. `splitRawArguments` now reports each value-taking flag's value, in both spellings, and `extractRuleFilters` reads that one pass.
b1e0d52 to
9f398b0
Compare
`check --rule <id>` selects every engine holding the id, and #385's test proved it by seeding `vale/no-eval` beside the fixture's `sg/no-eval`. Migration 9 now renames exactly that state, and `runCli` migrates on every invocation through `migrateFixture`, so the collision was renamed to `no-eval-sg`/`no-eval-vale` before `check` ever saw it: `--rule no-eval` exited `RULE_NOT_FOUND` and the test died reading `.map` of an undefined `results`. The migration invalidated the setup, not the behaviour. An id held by two engines still selects both, and a project can still reach that state — by hand, or by a merge landing a same-id rule under another engine — which is the case the new per-rule check in `verify` exists to catch. So the fixture is migrated first and the second engine's copy seeded after, with a comment naming migration 9 so the setup is not "simplified" back. Also names the consequence in the changeset: an id passed to `--rule` yesterday may not exist today, and that failure is `RULE_NOT_FOUND` rather than a quiet zero findings.
`check --rule <id>` selects every engine holding the id, and #385's test proved it by seeding `vale/no-eval` beside the fixture's `sg/no-eval`. Migration 9 now renames exactly that state, and `runCli` migrates on every invocation through `migrateFixture`, so the collision was renamed to `no-eval-sg`/`no-eval-vale` before `check` ever saw it: `--rule no-eval` exited `RULE_NOT_FOUND` and the test died reading `.map` of an undefined `results`. The migration invalidated the setup, not the behaviour. An id held by two engines still selects both, and a project can still reach that state — by hand, or by a merge landing a same-id rule under another engine — which is the case the new per-rule check in `verify` exists to catch. So the fixture is migrated first and the second engine's copy seeded after, with a comment naming migration 9 so the setup is not "simplified" back. Also names the consequence in the changeset: an id passed to `--rule` yesterday may not exist today, and that failure is `RULE_NOT_FOUND` rather than a quiet zero findings.
`check --rule <id>` selects every engine holding the id, and #385's test proved it by seeding `vale/no-eval` beside the fixture's `sg/no-eval`. Migration 9 now renames exactly that state, and `runCli` migrates on every invocation through `migrateFixture`, so the collision was renamed to `no-eval-sg`/`no-eval-vale` before `check` ever saw it: `--rule no-eval` exited `RULE_NOT_FOUND` and the test died reading `.map` of an undefined `results`. The migration invalidated the setup, not the behaviour. An id held by two engines still selects both, and a project can still reach that state — by hand, or by a merge landing a same-id rule under another engine — which is the case the new per-rule check in `verify` exists to catch. So the fixture is migrated first and the second engine's copy seeded after, with a comment naming migration 9 so the setup is not "simplified" back. Also names the consequence in the changeset: an id passed to `--rule` yesterday may not exist today, and that failure is `RULE_NOT_FOUND` rather than a quiet zero findings.
Why
An author iterating on a new rule wants one number: how often does this rule fire across the repository.
checkhad no way to ask it, so the documented workaround ran every engine and every rule and filtered afterwards:On the dogfood repository (~1,100 markdown files, eleven voice rules) that is the slow path on every iteration of a branch.
test <path>isolates one rule but runs only its fixtures, never the project.What
taskless check --rule <id>, repeatable.--rule a --rule bmeasures both.The load-bearing claim is an equality: what
--rule <id>reports has to be exactly what an unfilteredcheckreports for that id, because the author records that number against a branch and compares it to CI. Each engine therefore narrows by the mechanism that leaves everything else alone:sg scan --filter '^(?:a|b)$'--no-ignore hiddenand the--globsexclusions are byte-identical to an unfiltered run. Anchored, or--rule no-evalwould also reportno-eval-in-tests..vale.iniis built from only the selected rules--rulenarrows what may run and never widens it: a runtime rule named here still faces the signature gate.An id that names no rule directory under any engine is refused (
RULE_NOT_FOUND, exit 1, the id in the message). A typo that silently measured nothing reports "0 findings", which is also what a clean rule reports, and those are the two answers the author is choosing between. An id held by two engines selects both, unlikerules delete, which refuses an ambiguous id because deleting the wrong rule is irreversible.The issue's suggested mechanism was not the right one
#379 proposes reusing
buildIsolatingConfig, the configtestassembles for one rule, "pointed at the project walk instead of the fixture tree". That config scopes the rule under[*], which is right for a fixture tree and wrong for a project: a rule scoped[docs/**.md]by its own config would be measured over every file Vale can read, code included, and the count would come out larger thancheckreports in a way that looks like a noisy rule rather than a broken harness. Narrowing assembly instead keeps the rule's real scope.design.mdin the change records the reasoning, including why removing the other rules' blocks cannot alter the surviving one (the config schema rejects a rule assigning another rule's key).Tests
packages/cli/test/check-rule-filter.test.ts, spawning the built CLI over the mixed-engine fixture: a single--rulenarrows an ast-grep run and a Vale run; repeated--ruleunions across engines; an unknown id errors naming it; the git-ignore exclusions still hold under a filter; the filtered result equals the unfiltered run's findings for that id, for both engines; two engines holding one id run both. Plus unit coverage for the repeatable argv parsing and the anchored filter argv.pnpm typecheck,pnpm lintandpnpm --filter @taskless/cli test(1658 tests) pass.Notes
patch: pre-1.0, an added flag does not earn aminor, and with--ruleabsent every path is the one that shipped.check-rule-filteris archived on this PR (single PR, declared in the proposal). Its delta is ADDED-only; the archive dry-run confirmed thecli-checkspec went from 46 scenarios to 54 with none dropped.create-vale-rulecorpus-count passage) is left as a follow-up: that recipe is being edited on another branch right now.Fixes #379
Refs #362
@claude /review