feat(check): restrict a run to named rules with --rule - #385
thecodedrift wants to merge 1 commit into
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
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
|
|
||
| /** Whether `id` names a rule directory under any engine, engine by engine. */ | ||
| function enginesHolding( |
There was a problem hiding this comment.
[New] Reuse: packages/cli/src/rules/engines.ts already exports findRuleEngines(cwd, id): Promise<EngineName[]>, documented as "Every engine whose directory holds id... Empty when no engine does" — the exact fact enginesHolding recomputes here from a locally-built byEngine map. Two independent implementations of "which engine(s) hold this rule id" now exist (one stat-based per id, one list-based batched over all ids); a future change to rule-identity resolution (e.g. case-insensitive ids, or a new engine) only needs to update one of them to silently diverge from the other. Not a functional bug today — listRuleIds/subdirectories and findRuleEngines's per-engine stat should agree — but worth calling out as the kind of duplication STYLEGUIDE-CODE.md's "Reuse" concerns target.
| * `--rule=a`), and scanning stops at `--` so a path literally named `--rule` | ||
| * after the end-of-options marker is a path. | ||
| */ | ||
| export function extractRuleFilters(rawArguments: string[]): string[] { |
There was a problem hiding this comment.
[New] extractRuleFilters hand-rolls its own argv scan instead of extending the shared splitRawArguments (util/argv.ts), which already tracks which flags consume the next token (-d/--dir, and now --timeout/--rule via VALUE_FLAGS). Because this function only recognizes --rule/--rule=/--, it has no awareness that --timeout (or -d/--dir) also consumes the following token.
Concretely: for check --timeout --rule no-eval (a malformed invocation — --timeout given no value), extractPositionalPaths (via splitRawArguments, which does know --timeout is value-consuming) treats --rule as --timeout's value and no-eval as a positional path. extractRuleFilters, scanning the same rawArgs independently, has no such awareness, sees --rule as a real flag, and consumes no-eval as the rule id. The two scanners disagree about what the same tokens mean. Low-severity since it only triggers on a malformed --timeout/-d invocation immediately followed by --rule, but it's a real inconsistency introduced by not sharing the scanner.
| options: AssembleOptions = {} | ||
| ): Promise<ValeAssembly | undefined> { | ||
| const ruleIds = await listRuleIds(cwd, "vale"); | ||
| const ruleIds = selectRuleIds( |
There was a problem hiding this comment.
[New] This filtering bypasses the fail-closed Vale refusal for rules the filter excludes, which breaks the PR's own equality invariant.
ruleIds is narrowed to the selected ids here, before the validation loop starting a few lines below (for (const ruleId of ruleIds) { ... validateValeRuleConfig ... }). That means an unselected sibling rule's config is never read, never validated, and can never contribute to refusals.
Concretely: in an unfiltered check, if any Vale rule's config is schema-rejected (e.g. it assigns another rule's <id>.<id> key — a real rejection per schemas/vale-config.ts), refusals.length > 0 and the whole Vale engine is refused (dispatch.ts's runValeEngine treats status: "refused" as a failure — no Vale rule runs, exit code 1). That's deliberate fail-closed behavior per RefusedValeConfig's docstring.
But check --rule good-rule in the same project, where a different rule bad-rule is broken and good-rule is not, never even looks at bad-rule's config (it's filtered out of ruleIds before the loop runs), so refusals stays empty and good-rule assembles and runs normally, producing real findings and (possibly) a clean exit — while the unfiltered run reports 0 findings for good-rule (Vale never ran at all) plus a failure.
That directly contradicts design.md's stated constraint: "what --rule <id> reports has to be exactly what an unfiltered check reports for that id". None of the new tests in check-rule-filter.test.ts cover a broken sibling rule under --rule, so this isn't caught.
A fix consistent with the design's own reasoning would be to still validate every rule (for refusal purposes) and only narrow which validated blocks are written into the assembled config, rather than narrowing which rules get validated at all.
| 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.
`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.
e32c484 to
790f4d7
Compare
This branch has not been deployed
1 participant
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