From a6844ffddb1112f6c1dbc7eee9af1298fec2e06d Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 22 Sep 2026 10:20:59 -0700 Subject: [PATCH 1/2] feat(check): restrict a run to named rules with --rule `taskless check --rule ` (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. --- .changeset/check-rule-filter.md | 5 + .../2026-09-22-check-rule-filter/design.md | 49 +++ .../2026-09-22-check-rule-filter/proposal.md | 50 ++++ .../specs/cli-check/spec.md | 55 ++++ .../2026-09-22-check-rule-filter/tasks.md | 27 ++ openspec/specs/cli-check/spec.md | 54 ++++ packages/cli/src/commands/check.ts | 108 ++++++- packages/cli/src/rules/assemble.ts | 44 ++- packages/cli/src/rules/dispatch.ts | 13 + packages/cli/src/rules/rule-filter.ts | 86 ++++++ packages/cli/src/rules/scan.ts | 32 ++ packages/cli/test/check-rule-filter.test.ts | 282 ++++++++++++++++++ 12 files changed, 796 insertions(+), 9 deletions(-) create mode 100644 .changeset/check-rule-filter.md create mode 100644 openspec/changes/archive/2026-09-22-check-rule-filter/design.md create mode 100644 openspec/changes/archive/2026-09-22-check-rule-filter/proposal.md create mode 100644 openspec/changes/archive/2026-09-22-check-rule-filter/specs/cli-check/spec.md create mode 100644 openspec/changes/archive/2026-09-22-check-rule-filter/tasks.md create mode 100644 packages/cli/src/rules/rule-filter.ts create mode 100644 packages/cli/test/check-rule-filter.test.ts diff --git a/.changeset/check-rule-filter.md b/.changeset/check-rule-filter.md new file mode 100644 index 00000000..5dd79af5 --- /dev/null +++ b/.changeset/check-rule-filter.md @@ -0,0 +1,5 @@ +--- +"@taskless/cli": patch +--- + +`taskless check --rule ` (repeatable) restricts a run to the named rules, so a rule can be measured over the whole project without running every other rule and filtering the JSON afterwards. The filter applies to both static engines and to runtime rules, keeps every exclusion a whole-project run applies, and refuses an id no rule directory has. diff --git a/openspec/changes/archive/2026-09-22-check-rule-filter/design.md b/openspec/changes/archive/2026-09-22-check-rule-filter/design.md new file mode 100644 index 00000000..91602aa9 --- /dev/null +++ b/openspec/changes/archive/2026-09-22-check-rule-filter/design.md @@ -0,0 +1,49 @@ +## Context + +`check --rule ` has to report, for the named rule, exactly what an unfiltered `check` reports for that rule. Not approximately: the author writes the number into a branch's record and compares it against the number `check` produces in CI. Any divergence between "the filtered path" and "the unfiltered path" is a wrong number that nothing detects. + +That constraint decides the mechanism, engine by engine. + +## Decisions + +### The issue's suggestion (reuse `buildIsolatingConfig`) is the wrong mechanism for Vale + +taskless/cli#379 proposes reusing "the isolating config `test` already assembles for one rule, pointed at the project walk instead of the fixture tree". Read, that config (`rules/vale/verify.ts`) is: + +``` +StylesPath = +MinAlertLevel = suggestion + +[*] +. = YES +``` + +`[*]` is the problem. It is correct for a fixture tree, where every document exists to exercise the rule, and wrong for a project walk: a rule scoped `[docs/**.md]` by its own config would, under this config, be measured over every file Vale can read, code included. The count would be larger than `check` reports, and larger in a way that looks like the rule being noisy rather than like the harness being wrong. The config schema calls the same shape out in a rule's own config (`matcher [*] enables for every file Vale can read, code included`). + +**Instead, assembly is narrowed.** `assembleValeConfig` takes the selected ids and emits only those rules' blocks, each rule's own matchers verbatim. The rule's scope is then byte-identical to what it is in a full run. + +Removing the other rules' blocks cannot change the surviving rule's effective setting, and that is a fact about the config schema rather than an assumption: a rule's config may only assign its own `.` key — the schema rejects an assignment that "names another rule" — so no removed block could have been turning the selected rule on or off. Vale's positional precedence (last matcher wins; since 3.21.0 last assignment within a matcher wins) has nothing to act on across rules. + +### ast-grep narrows with `--filter`, not by narrowing `ruleDirs` + +ast-grep 0.45.3 has `scan --filter `: "Scan the codebase with rules with ids matching REGEX." It changes exactly one thing — which loaded rules may report — leaving the config, the walk, `--no-ignore hidden` and the two `--globs` exclusions untouched. The alternative, writing an assembled config whose `ruleDirs` names only the selected rule directories, would have been a second config-generation path to keep in step with the first. + +The regex is anchored (`^(?:a|b)$`). Unanchored, `--rule no-eval` would also report `no-eval-in-tests`. + +### An unknown id is a refusal, not an empty run + +A `--rule` naming nothing runs nothing and reports "0 findings" — which is also what a rule that fires nowhere reports, and that is precisely the answer the author is trying to obtain. The two must not look alike, so an unresolvable id exits 1 with `RULE_NOT_FOUND` and the id in the message. + +Resolution happens **before** the "No rules configured" gate, so the message is about the id in every project rather than about the project in some of them. + +### An ambiguous id selects both rules + +`rules delete` refuses an id held by two engines (`RULE_ID_AMBIGUOUS`) because deleting the wrong one is irreversible. Measuring is neither irreversible nor destructive, and an unfiltered `check` would have run both, so `--rule` runs both and each finding carries its engine in `source`. + +### `--rule` is read from raw argv + +The flag is repeatable, and a parser that collapses a repeat to one value turns `--rule a --rule b` into a measurement of one rule while the author reads the number as covering two. Values are scanned out of `rawArgs` (both `--rule a` and `--rule=a`, stopping at `--`), and `--rule` is added to the value-taking flags the shared positional scanner knows — without that, `check --rule no-eval` scans `no-eval` as a path, finds no such file, and takes the "every supplied path was filtered out" branch: a clean exit 0 with no findings, indistinguishable from the measurement the author wanted. + +### An engine with nothing selected is skipped, not filtered to nothing + +Handing ast-grep a filter that matches no rule still spawns it, loads every rule and walks the project to report none. When the selection contains no `sg` rule the engine is skipped outright; Vale assembly returns `undefined` for the same case, which dispatch already reads as "nothing to run". diff --git a/openspec/changes/archive/2026-09-22-check-rule-filter/proposal.md b/openspec/changes/archive/2026-09-22-check-rule-filter/proposal.md new file mode 100644 index 00000000..1991eb59 --- /dev/null +++ b/openspec/changes/archive/2026-09-22-check-rule-filter/proposal.md @@ -0,0 +1,50 @@ +## Why + +An author iterating on a new rule wants one number: how many times does this rule fire across the repository, before it ships at `warning`. `check` has no way to ask that. It runs every rule in `.taskless/rules/`, so the documented workaround is to run everything and filter afterwards: + +``` +taskless check --json | jq '[.results[] | select(.ruleId == "")] | length' +``` + +That runs every engine and every rule to answer a question about one. On the dogfood repository (~1,100 markdown files, eleven voice rules) it is the slow path on every iteration of a branch. `test ` does isolate one rule, but it runs only that rule's fixtures and never the project, so it cannot answer the question at all. + +## What Changes + +- `taskless check --rule `, repeatable, restricting the run to the named rules. `--rule a --rule b` measures both. +- The narrowing is per engine, because the engines narrow by different mechanisms: + - **ast-grep**: `sg scan --filter '^(?:a|b)$'`, ast-grep's own flag for scanning with a subset of the rules a config loads. Everything else about the invocation — the config, the walk, `--no-ignore hidden`, the `--globs` exclusions — is byte-identical to an unfiltered run. + - **Vale**: the assembled `.vale.ini` is built from only the selected rules, each rule's own matchers kept verbatim. Vale has no rule-selection flag, so the config is the only place to express it. + - **runtime**: the discovered rule list is filtered before planning, so `--rule` narrows 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 a **refusal** (`RULE_NOT_FOUND`, exit 1, the id named). A typo that silently measured nothing would report "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. `check` with no filter would have run both, and `--rule` narrows a run rather than redefining it. + +Nothing here is **BREAKING**. Pre-1.0, an added flag is a `patch`; nothing that exists today changes behavior when `--rule` is absent. + +## Non-goals + +- `--rule` does not take a path. `test` takes a path, `check --rule` takes an id, which is what a finding carries in `ruleId` and what the author reads out of the JSON. +- `--rule` does not override the runtime signature gate, and does not re-enable a rule the project has removed. + +## Capabilities + +### New Capabilities + +None. + +### Modified Capabilities + +- `cli-check`: one new requirement, "Check restricts the run to named rules with --rule". Nothing existing is modified — `--rule` narrows a run the way positional paths already do, and the auth, dispatch and exit-code requirements are unchanged. + +## Impact + +- `packages/cli/src/commands/check.ts`: the `--rule` flag, its repeatable argv parsing, and the resolution/refusal. +- `packages/cli/src/rules/rule-filter.ts` (new): resolve requested ids into a per-engine selection, or refuse. +- `packages/cli/src/rules/scan.ts`: `--filter` argv for ast-grep. +- `packages/cli/src/rules/assemble.ts`: Vale assembly accepts a rule-id narrowing. +- `packages/cli/src/rules/dispatch.ts`: carries the ast-grep selection through to the scan. +- `packages/cli/test/check-rule-filter.test.ts` (new). +- `packages/cli/src/agent/create-vale-rule.md`: the corpus-count passage should name the flag. **Deliberately not touched here** — that file is being edited on another branch, and the recipe change is a follow-up (taskless/cli#379, last bullet). + +## Delivery shape + +**Single PR.** One flag, its per-engine plumbing, its tests, the spec delta and the archive fit one reviewable diff, and the change is safe in production on its own: with `--rule` absent every code path is the one that shipped. diff --git a/openspec/changes/archive/2026-09-22-check-rule-filter/specs/cli-check/spec.md b/openspec/changes/archive/2026-09-22-check-rule-filter/specs/cli-check/spec.md new file mode 100644 index 00000000..cf1f3720 --- /dev/null +++ b/openspec/changes/archive/2026-09-22-check-rule-filter/specs/cli-check/spec.md @@ -0,0 +1,55 @@ +## ADDED Requirements + +### Requirement: Check restricts the run to named rules with --rule + +The `check` subcommand SHALL accept a `--rule ` flag, repeatable, naming the rules the run is restricted to. When `--rule` is absent the run SHALL be unchanged. When one or more are given, the CLI SHALL report findings only for the named rules, and for each named rule SHALL report exactly what an unfiltered `check` over the same paths would have reported for it: the same walk, the same exclusions (`.taskless/`, `.git/`, git-ignored paths, converter-dependent formats), and the same per-rule scope. + +A rule id is the name of a rule's directory under `.taskless/rules//`. The filter SHALL apply to every engine. An id whose rule directory exists under more than one engine SHALL select the rule under each of them. An id that names no rule directory under any engine SHALL be refused: the CLI SHALL exit with code 1 and report the error code `RULE_NOT_FOUND` naming the unresolved id, and SHALL NOT run any engine. + +`--rule` SHALL NOT widen what may run. A runtime rule named by `--rule` SHALL remain subject to the signature-validated path, and SHALL be reported as skipped on an unverified path exactly as it would be in an unfiltered run. + +#### Scenario: A single --rule narrows the run to that rule + +- **WHEN** a user runs `taskless check --rule ` in a project with rules beyond `` +- **THEN** the results SHALL contain findings for `` only +- **AND** they SHALL be the findings an unfiltered `taskless check` reports for `` + +#### Scenario: Repeated --rule unions the named rules + +- **WHEN** a user runs `taskless check --rule a --rule b` +- **THEN** the results SHALL contain the findings for `a` and the findings for `b`, and no others + +#### Scenario: The filter applies to both static engines + +- **WHEN** a user names an ast-grep rule with `--rule`, and separately names a Vale rule +- **THEN** each run SHALL report that rule's findings over the whole project +- **AND** a Vale rule SHALL be measured under its own config's matchers, not over every file Vale can read + +#### Scenario: Whole-project exclusions still apply under --rule + +- **WHEN** a user runs `taskless check --rule ` with no positional paths in a project with git-ignored directories +- **THEN** the CLI SHALL NOT report findings from `.taskless/`, `.git/`, or git-ignored paths + +#### Scenario: An unknown rule id is refused + +- **WHEN** a user runs `taskless check --rule ` and no engine directory holds a rule directory named `` +- **THEN** the CLI SHALL exit with code 1 +- **AND** under `--json` stdout SHALL carry the standardized error envelope with code `RULE_NOT_FOUND` and a message naming `` +- **AND** the CLI SHALL NOT run any engine + +#### Scenario: An id held by two engines selects both rules + +- **WHEN** a user runs `taskless check --rule ` and `` names a rule directory under two engines +- **THEN** the CLI SHALL run both rules and SHALL report the findings of each, distinguished by the `source` field + +#### Scenario: --rule does not bypass the runtime signature gate + +- **WHEN** a user runs `taskless check --rule ` where `` is a runtime rule and the run is on an unverified path +- **THEN** the CLI SHALL NOT execute that rule's `check.ts` +- **AND** SHALL report it as skipped exactly as an unfiltered `check` would + +#### Scenario: --rule leaves positional path arguments intact + +- **WHEN** a user runs `taskless check --rule src/foo.ts` +- **THEN** the CLI SHALL treat `src/foo.ts` as the path to scan and `` as the rule filter +- **AND** SHALL NOT treat `` as a path diff --git a/openspec/changes/archive/2026-09-22-check-rule-filter/tasks.md b/openspec/changes/archive/2026-09-22-check-rule-filter/tasks.md new file mode 100644 index 00000000..e85281de --- /dev/null +++ b/openspec/changes/archive/2026-09-22-check-rule-filter/tasks.md @@ -0,0 +1,27 @@ +## 1. Design + +- [x] 1.1 Read `buildIsolatingConfig` and establish whether it is the right mechanism for a project walk (it is not: its `[*]` matcher discards the rule's own scope). +- [x] 1.2 Confirm the vendored ast-grep exposes a rule-id filter for `scan` (`--filter `, 0.45.3) and that the config schema forbids a rule assigning another rule's key. + +## 2. Implementation + +- [x] 2.1 `packages/cli/src/rules/rule-filter.ts`: resolve requested ids into a per-engine selection; refuse an unknown id with `RULE_NOT_FOUND`. +- [x] 2.2 `packages/cli/src/rules/scan.ts`: `sgFilterArgv` emitting an anchored `--filter`, threaded through `runAstGrepScan`. +- [x] 2.3 `packages/cli/src/rules/assemble.ts`: `AssembleOptions.ruleIds` narrowing the Vale assembly. +- [x] 2.4 `packages/cli/src/rules/dispatch.ts`: carry the ast-grep selection to the scan. +- [x] 2.5 `packages/cli/src/commands/check.ts`: the `--rule` flag, repeatable argv parsing, `--rule` as a value-taking flag for the positional scanner, selection applied to all three engines. +- [x] 2.6 `.changeset/check-rule-filter.md` (`patch`). + +## 3. Tests + +- [x] 3.1 `packages/cli/test/check-rule-filter.test.ts`: a single `--rule` narrows an ast-grep run and a Vale run; repeated `--rule` unions across engines; an unknown id errors naming it; the gitignore exclusions still hold under a filter; the filtered result equals the unfiltered run's findings for that id, for both engines. +- [x] 3.2 Unit coverage for the repeatable argv parsing and the anchored filter argv. + +## 4. Spec + +- [x] 4.1 Add the `cli-check` requirement "Check restricts the run to named rules with --rule" as an ADDED block; nothing existing needs modifying. +- [x] 4.2 `pnpm openspec validate check-rule-filter --strict`, then the dry-run archive check from CLAUDE.md (scenario count before vs after), then archive for real. + +## 5. Verification + +- [x] 5.1 `pnpm build`, `pnpm typecheck`, `pnpm lint`, `pnpm --filter @taskless/cli test` all pass. diff --git a/openspec/specs/cli-check/spec.md b/openspec/specs/cli-check/spec.md index d2ff6019..0072efc8 100644 --- a/openspec/specs/cli-check/spec.md +++ b/openspec/specs/cli-check/spec.md @@ -395,3 +395,57 @@ SHALL be the only way to execute runtime rules on an unverified path. - **WHEN** the `vale` binary is unavailable but `.taskless/sg/` has rules - **THEN** the CLI reports the Vale engine as unavailable and still returns ast-grep results + +### Requirement: Check restricts the run to named rules with --rule + +The `check` subcommand SHALL accept a `--rule ` flag, repeatable, naming the rules the run is restricted to. When `--rule` is absent the run SHALL be unchanged. When one or more are given, the CLI SHALL report findings only for the named rules, and for each named rule SHALL report exactly what an unfiltered `check` over the same paths would have reported for it: the same walk, the same exclusions (`.taskless/`, `.git/`, git-ignored paths, converter-dependent formats), and the same per-rule scope. + +A rule id is the name of a rule's directory under `.taskless/rules//`. The filter SHALL apply to every engine. An id whose rule directory exists under more than one engine SHALL select the rule under each of them. An id that names no rule directory under any engine SHALL be refused: the CLI SHALL exit with code 1 and report the error code `RULE_NOT_FOUND` naming the unresolved id, and SHALL NOT run any engine. + +`--rule` SHALL NOT widen what may run. A runtime rule named by `--rule` SHALL remain subject to the signature-validated path, and SHALL be reported as skipped on an unverified path exactly as it would be in an unfiltered run. + +#### Scenario: A single --rule narrows the run to that rule + +- **WHEN** a user runs `taskless check --rule ` in a project with rules beyond `` +- **THEN** the results SHALL contain findings for `` only +- **AND** they SHALL be the findings an unfiltered `taskless check` reports for `` + +#### Scenario: Repeated --rule unions the named rules + +- **WHEN** a user runs `taskless check --rule a --rule b` +- **THEN** the results SHALL contain the findings for `a` and the findings for `b`, and no others + +#### Scenario: The filter applies to both static engines + +- **WHEN** a user names an ast-grep rule with `--rule`, and separately names a Vale rule +- **THEN** each run SHALL report that rule's findings over the whole project +- **AND** a Vale rule SHALL be measured under its own config's matchers, not over every file Vale can read + +#### Scenario: Whole-project exclusions still apply under --rule + +- **WHEN** a user runs `taskless check --rule ` with no positional paths in a project with git-ignored directories +- **THEN** the CLI SHALL NOT report findings from `.taskless/`, `.git/`, or git-ignored paths + +#### Scenario: An unknown rule id is refused + +- **WHEN** a user runs `taskless check --rule ` and no engine directory holds a rule directory named `` +- **THEN** the CLI SHALL exit with code 1 +- **AND** under `--json` stdout SHALL carry the standardized error envelope with code `RULE_NOT_FOUND` and a message naming `` +- **AND** the CLI SHALL NOT run any engine + +#### Scenario: An id held by two engines selects both rules + +- **WHEN** a user runs `taskless check --rule ` and `` names a rule directory under two engines +- **THEN** the CLI SHALL run both rules and SHALL report the findings of each, distinguished by the `source` field + +#### Scenario: --rule does not bypass the runtime signature gate + +- **WHEN** a user runs `taskless check --rule ` where `` is a runtime rule and the run is on an unverified path +- **THEN** the CLI SHALL NOT execute that rule's `check.ts` +- **AND** SHALL report it as skipped exactly as an unfiltered `check` would + +#### Scenario: --rule leaves positional path arguments intact + +- **WHEN** a user runs `taskless check --rule src/foo.ts` +- **THEN** the CLI SHALL treat `src/foo.ts` as the path to scan and `` as the rule filter +- **AND** SHALL NOT treat `` as a path diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index 14cb984b..358208a0 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -14,6 +14,7 @@ import { makeErrorEnvelope, writeJsonError } from "../types/errors"; import { CLIError } from "../util/cli-error"; import { requireCurrentSchema } from "../filesystem/migrate"; import { discoverRuntimeRules } from "../rules/runtime/discover"; +import { resolveRuleSelection, type RuleSelection } from "../rules/rule-filter"; // The gate lives beside the runtime engine rather than inside this command, // because `test` runs a rule's fixtures under exactly this policy. Sharing the // implementation is what makes that a fact rather than an intention. @@ -65,7 +66,46 @@ async function filterExistingPaths( * value-taking flag, so it is named here rather than in the shared set. */ function extractPositionalPaths(rawArguments: string[]): string[] { - return splitRawArguments(rawArguments, ["--timeout"]).positionals; + return splitRawArguments(rawArguments, VALUE_FLAGS).positionals; +} + +/** + * `check`'s own value-taking flags, for the shared argv scanner. + * + * `--rule` has to be here or its value is scanned as a positional path: + * `check --rule no-eval` would look for a file called `no-eval`, find none, and + * take the "every supplied path was filtered out" branch — a clean exit 0 with + * no findings, which is the same output a rule that fires nowhere produces. + */ +const VALUE_FLAGS = ["--timeout", "--rule"] as const; + +/** + * Every `--rule` value in argv, in the order given. + * + * Read from raw argv rather than from citty's parsed `args` because the flag is + * REPEATABLE, and a parser that collapses a repeat to a single value turns + * `--rule a --rule b` into a measurement of one rule while the author reads the + * number as covering two. Both spellings are accepted (`--rule a` and + * `--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[] { + const ids: string[] = []; + for (let index = 0; index < rawArguments.length; index++) { + const argument = rawArguments[index]!; + if (argument === "--") break; + if (argument === "--rule") { + const value = rawArguments[index + 1]; + if (value !== undefined && value !== "--") { + ids.push(value); + index++; + } + continue; + } + if (argument.startsWith("--rule=")) + ids.push(argument.slice("--rule=".length)); + } + return ids.filter((id) => id !== ""); } /** Parse `--timeout ` into milliseconds; invalid/absent → undefined (default). */ @@ -108,6 +148,10 @@ export const checkCommand = defineCommand({ type: "string", description: "Per-runtime-check timeout in seconds (default 10)", }, + rule: { + type: "string", + description: "Run only the named rule; repeatable (--rule a --rule b)", + }, }, async run({ args, rawArgs }) { const cwd = resolve(args.dir ?? process.cwd()); @@ -182,6 +226,34 @@ export const checkCommand = defineCommand({ } throw error; } + // Resolved BEFORE the "no rules configured" gate, so a mistyped id is + // reported as a mistyped id in every project rather than as "no rules + // configured" in some of them. The refusal is handled here for the same + // reason the scaffold refusal above is: it asks the caller to fix the + // command line, not to read a failed scan. + const requestedRules = extractRuleFilters(rawArgs); + let mutableSelection: RuleSelection | undefined; + if (requestedRules.length > 0) { + try { + mutableSelection = await resolveRuleSelection(cwd, requestedRules); + } catch (error) { + if (error instanceof CLIError) { + if (args.json) { + writeJsonError(error.code ?? "INVALID_INPUT", error.message); + } else { + console.error(`Error: ${error.message}`); + } + process.exitCode = 1; + return; + } + throw error; + } + } + + // Rebound as a const so narrowing survives into the callbacks below: a + // `let` is re-widened inside a closure, and the filter is read from one. + const selection = mutableSelection; + const dispatch = await planEngineDispatch(cwd); // Static rules (trusted ast-grep YAML) always run; runtime rules @@ -200,9 +272,19 @@ export const checkCommand = defineCommand({ const runtimeEnabled = runtimeDispatch?.present === true && runtimeDispatch.executor === "runtime-harness"; - const runtimeRules = runtimeEnabled + const discoveredRuntimeRules = runtimeEnabled ? await discoverRuntimeRules(cwd) : []; + // `--rule` narrows WHAT runs; it does not widen what may run. A runtime + // rule named here is still subject to the signature gate, so an + // unauthenticated `check --rule ` reports the same skip it + // would have reported inside a whole-project run. + const runtimeRules = + selection === undefined + ? discoveredRuntimeRules + : discoveredRuntimeRules.filter((rule) => + selection.runtime.includes(rule.name) + ); // "No rules configured" has to mean *no engine* has any, not just these // two: a project whose only rules live in `.taskless/rules/vale/` would @@ -253,11 +335,22 @@ export const checkCommand = defineCommand({ // Assemble both engine configs from the per-rule tree. Each returns // `undefined` when its engine has no rules, which dispatch reads as // "nothing to run" rather than running an empty config. - const assembled = await assembleEngineConfigs(cwd); + const assembled = await assembleEngineConfigs( + cwd, + selection === undefined ? {} : { ruleIds: selection.vale } + ); const dispatched = await runEngines({ cwd, paths: existingPaths, - astGrepConfigPath: assembled.sg, + // An `sg` selection that is empty means no ast-grep rule was named, + // so the engine has nothing to do and is skipped rather than being + // handed a filter that matches nothing — which would still spawn + // ast-grep, load every rule, and walk the project to report none. + astGrepConfigPath: + selection !== undefined && selection.sg.length === 0 + ? undefined + : assembled.sg, + ...(selection === undefined ? {} : { astGrepRuleIds: selection.sg }), vale: assembled.vale, runtimeRules: plan.execute, runtimeTimeoutMs: parseTimeoutMs(args.timeout), @@ -290,7 +383,12 @@ export const checkCommand = defineCommand({ warningCount, findings: results.length, ruleCount: - astGrepRuleIds.length + valeRuleIds.length + runtimeRules.length, + astGrepRuleIds.length + + valeRuleIds.length + + // Discovered, not the `--rule` subset: the question this count + // answers is how many rules the workspace has configured, and the + // other two terms are unfiltered for the same reason. + discoveredRuntimeRules.length, }; // Computed by `runEngines`, not here: the exit code is a fact about a diff --git a/packages/cli/src/rules/assemble.ts b/packages/cli/src/rules/assemble.ts index adf40c7c..fd61d0e6 100644 --- a/packages/cli/src/rules/assemble.ts +++ b/packages/cli/src/rules/assemble.ts @@ -163,6 +163,37 @@ export interface RefusedValeConfig { export type ValeAssembly = AssembledValeConfig | RefusedValeConfig; +/** Narrowing shared by the engine assemblers. */ +export interface AssembleOptions { + /** + * Restrict assembly to these rule ids — `check --rule`. `undefined` is the + * ordinary whole-project run and means every rule; an empty array means no + * rule was selected for this engine, so it assembles nothing and the engine + * is skipped. + * + * Narrowing Vale by REMOVING the other rules' blocks, rather than by writing + * a config that enables one rule under `[*]`, is what makes a filtered run + * report what an unfiltered run would have reported for that rule. Each rule + * keeps its own matchers verbatim, so its scope is unchanged, and the schema + * refuses a config that assigns another rule's key (the "names another rule" + * rejection in `schemas/vale-config.ts`), so no removed block could have been + * setting the selected rule's `.` value. Vale's positional precedence + * therefore has nothing left to act on across rules, and the surviving block + * resolves exactly as it did among the others. + */ + ruleIds?: readonly string[]; +} + +/** `available` narrowed to `selected`, or all of it when nothing was selected. */ +function selectRuleIds( + available: string[], + selected: readonly string[] | undefined +): string[] { + if (selected === undefined) return available; + const wanted = new Set(selected); + return available.filter((ruleId) => wanted.has(ruleId)); +} + /** * Assemble `.taskless/.vale.ini` from every Vale rule's own config. * @@ -177,9 +208,13 @@ export type ValeAssembly = AssembledValeConfig | RefusedValeConfig; * against no rules and report a clean pass. */ export async function assembleValeConfig( - cwd: string + cwd: string, + options: AssembleOptions = {} ): Promise { - const ruleIds = await listRuleIds(cwd, "vale"); + const ruleIds = selectRuleIds( + await listRuleIds(cwd, "vale"), + options.ruleIds + ); const blocks: string[] = []; const sections = new Set(); const advisories: string[] = []; @@ -288,10 +323,11 @@ export interface AssembledConfigs { } export async function assembleEngineConfigs( - cwd: string + cwd: string, + options: AssembleOptions = {} ): Promise { const [vale, sg] = await Promise.all([ - assembleValeConfig(cwd), + assembleValeConfig(cwd, options), assembleSgConfig(cwd), ]); return { vale, sg }; diff --git a/packages/cli/src/rules/dispatch.ts b/packages/cli/src/rules/dispatch.ts index f175fe8e..2b9f8071 100644 --- a/packages/cli/src/rules/dispatch.ts +++ b/packages/cli/src/rules/dispatch.ts @@ -81,6 +81,16 @@ export interface DispatchOptions { * runs it and does not know how it was built. */ astGrepConfigPath: string | undefined; + /** + * Rule ids the ast-grep engine is restricted to — `check --rule`. Omitted is + * the ordinary run and means every rule in the config. + * + * Vale needs no equivalent here: its narrowing happened at assembly, so what + * arrives on `vale` is already the filtered config. Two engines, two + * mechanisms, because ast-grep can be told which loaded rules may report and + * Vale cannot. + */ + astGrepRuleIds?: readonly string[]; /** * What Vale assembly produced: the `--config` path with its advisories, a * refusal because a rule's config broke the schema, or `undefined` when @@ -147,6 +157,9 @@ async function runAstGrepEngine( } const scan = await runAstGrepScan(options.cwd, options.paths, { configPath: options.astGrepConfigPath, + ...(options.astGrepRuleIds === undefined + ? {} + : { ruleIds: options.astGrepRuleIds }), }); return { engine: "sg", results: scan.results }; } diff --git a/packages/cli/src/rules/rule-filter.ts b/packages/cli/src/rules/rule-filter.ts new file mode 100644 index 00000000..cfb9d68b --- /dev/null +++ b/packages/cli/src/rules/rule-filter.ts @@ -0,0 +1,86 @@ +import { CLIError } from "../util/cli-error"; +import { listRuleIds } from "./engines"; +import { ENGINES, RULES_DIRECTORY, type EngineName } from "./layout"; + +/** + * Which rules a run is restricted to, split by the engine that owns each one. + * + * Split rather than kept as one list because the two static engines narrow by + * different mechanisms — ast-grep by `--filter` over the ids it loaded, Vale by + * assembling a config that contains only the selected rules — and each has to + * be handed only the ids it can act on. An engine whose list is empty has no + * work in this run and is skipped outright, which is not the same as being + * handed a filter that matches nothing: the second one still spawns. + */ +export interface RuleSelection { + sg: string[]; + vale: string[]; + runtime: string[]; +} + +/** Whether `id` names a rule directory under any engine, engine by engine. */ +function enginesHolding( + id: string, + byEngine: Record +): EngineName[] { + return ENGINES.filter((engine) => byEngine[engine].includes(id)); +} + +/** + * Resolve `--rule` ids into the per-engine selection a run is restricted to. + * + * Every id has to name a rule directory on disk. An id that names none is a + * refusal rather than an empty run: the whole point of the flag is to measure + * one rule over the project, and a typo that silently measures nothing reports + * "0 findings", which is also what a clean rule reports. The two are the answers + * an author is choosing between, so they must never look alike. + * + * An id held by more than one engine selects **both**. That is deliberately + * unlike `rules delete`, which refuses an ambiguous id (`RULE_ID_AMBIGUOUS`): + * deleting is destructive and irreversible, so guessing which rule the caller + * meant is unacceptable, while measuring is neither. `check` with no filter + * would have run both of them, and `--rule` narrows a run rather than + * redefining it, so both still run and the findings carry the engine in their + * `source` field. + * + * Reads directory names, not the `id:` inside a rule file. The directory name + * IS the rule id here (`rules/constraints.ts`, `rules/verify.ts` both state and + * enforce it), so this is the same identity `test` and `rules delete` address a + * rule by, and a rule whose file disagrees with its directory is already a + * `verify` failure rather than something for this to guess at. + */ +export async function resolveRuleSelection( + cwd: string, + requested: readonly string[] +): Promise { + const byEngine = {} as Record; + await Promise.all( + ENGINES.map(async (engine) => { + byEngine[engine] = await listRuleIds(cwd, engine); + }) + ); + + // Deduplicated, because `--rule a --rule a` is one rule, and an id repeated + // into the ast-grep filter alternation or the Vale assembly would otherwise + // be a rule listed twice. Insertion order is kept so the unknown-id message + // reads back in the order the ids were typed. + const unique = [...new Set(requested)]; + const unknown = unique.filter( + (id) => enginesHolding(id, byEngine).length === 0 + ); + if (unknown.length > 0) { + throw new CLIError( + `No rule named ${unknown.map((id) => `"${id}"`).join(", ")} under ` + + `.taskless/${RULES_DIRECTORY}/. A rule id is the name of its directory ` + + `under .taskless/${RULES_DIRECTORY}//.`, + "RULE_NOT_FOUND" + ); + } + + const selection: RuleSelection = { sg: [], vale: [], runtime: [] }; + for (const id of unique) { + for (const engine of enginesHolding(id, byEngine)) + selection[engine].push(id); + } + return selection; +} diff --git a/packages/cli/src/rules/scan.ts b/packages/cli/src/rules/scan.ts index 8e4ed5f5..6f746f42 100644 --- a/packages/cli/src/rules/scan.ts +++ b/packages/cli/src/rules/scan.ts @@ -132,6 +132,37 @@ export interface ScanOptions { * layout pass the ephemeral config written for it instead. */ configPath?: string; + /** + * Restrict reporting to these rule ids — `check --rule`. Omitted (or empty) + * runs every rule the config loads. + * + * Expressed as ast-grep's own `--filter ` rather than by narrowing + * `ruleDirs` in the assembled config, because the filter changes exactly one + * thing: which loaded rules may report. The config, the walk, the + * `.gitignore` handling and the `--globs` exclusions are byte-identical to an + * unfiltered run, so a filtered count is the unfiltered count for that rule + * by construction rather than by two code paths agreeing. + */ + ruleIds?: readonly string[]; +} + +/** + * `--filter` argv restricting the scan to `ruleIds`, or nothing when there is + * no restriction. + * + * Anchored, and the ids escaped, because `--filter` takes a REGEX matched + * against every loaded rule id: unanchored, `--rule no-eval` would also report + * `no-eval-in-tests`, which is a different rule the author did not ask to + * measure. Rule ids are `[a-z0-9-]`, so nothing in one is a regex metacharacter + * today; the escape is here so that a later widening of `isValidRuleId` cannot + * turn an id into a pattern silently. + */ +export function sgFilterArgv(ruleIds: readonly string[] | undefined): string[] { + if (ruleIds === undefined || ruleIds.length === 0) return []; + const alternation = ruleIds + .map((ruleId) => ruleId.replaceAll(/[$()*+.?[\\\]^{|}]/g, String.raw`\$&`)) + .join("|"); + return ["--filter", `^(?:${alternation})$`]; } /** @@ -246,6 +277,7 @@ export async function runAstGrepScan( "--config", options.configPath ?? ASSEMBLED_SG_CONFIG, "--json=stream", + ...sgFilterArgv(options.ruleIds), ...sgWalkArgv(paths), ...(paths.length > 0 ? ["--", ...paths] : []), ]; diff --git a/packages/cli/test/check-rule-filter.test.ts b/packages/cli/test/check-rule-filter.test.ts new file mode 100644 index 00000000..88ed6e9b --- /dev/null +++ b/packages/cli/test/check-rule-filter.test.ts @@ -0,0 +1,282 @@ +import { execFile } from "node:child_process"; +import { cp, mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { promisify } from "node:util"; + +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { extractRuleFilters } from "../src/commands/check"; +import { sgFilterArgv } from "../src/rules/scan"; +import { findValeBinary } from "../src/rules/vale/binary"; +import { migrateFixture } from "./support/current-project"; + +const execFileAsync = promisify(execFile); +const binPath = resolve(import.meta.dirname, "../dist/index.js"); +const fixturesDirectory = resolve( + import.meta.dirname, + "fixtures/mixed-engines-project" +); + +/** + * `check --rule ` over a project with rules in both static engines. + * + * The flag exists so an author can measure ONE rule over the whole repository + * while iterating on it, instead of running every rule and filtering the JSON + * afterwards (taskless/cli#379). That makes the load-bearing claim a claim + * about equality: what `--rule ` reports has to be exactly what an + * unfiltered `check` reports for that id — the same walk, the same exclusions, + * the same findings — or the number the author records against a branch is not + * the number `check` would have produced. + * + * Spawns the built CLI over a real project, because the two engines narrow by + * different mechanisms (ast-grep's `--filter`, a Vale config assembled from + * only the selected rules) and the question is whether those two mechanisms + * agree with the unfiltered run. A mock of either would be asserting the mock. + */ + +/** Run the built CLI, tolerating a non-zero exit. */ +async function runCli( + arguments_: string[] +): Promise<{ stdout: string; stderr: string; exitCode: number }> { + await migrateFixture(arguments_); + + try { + const { stdout, stderr } = await execFileAsync("node", [ + binPath, + ...arguments_, + ]); + return { stdout, stderr, exitCode: 0 }; + } catch (error) { + const failure = error as { stdout: string; stderr: string; code: number }; + return { + stdout: failure.stdout ?? "", + stderr: failure.stderr ?? "", + exitCode: failure.code, + }; + } +} + +interface CheckFinding { + source: string; + ruleId: string; + file: string; +} + +interface CheckOutput { + success: boolean; + results: CheckFinding[]; +} + +/** `(source, ruleId, file)` triples, sorted, for order-free comparison. */ +function triples(results: CheckFinding[]): string[] { + return results + .map((finding) => `${finding.source} ${finding.ruleId} ${finding.file}`) + .toSorted(); +} + +/** Vale ships per-platform; an unsupported host has none. */ +const valeAvailable = findValeBinary().path !== undefined; + +describe("extractRuleFilters", () => { + it("collects every --rule value, in both spellings", () => { + expect( + extractRuleFilters(["check", "--rule", "a", "--json", "--rule=b"]) + ).toEqual(["a", "b"]); + }); + + it("stops at the end-of-options marker", () => { + // After `--` every token is a path, including one spelled like this flag. + expect(extractRuleFilters(["check", "--", "--rule", "a"])).toEqual([]); + }); +}); + +describe("sgFilterArgv", () => { + it("anchors the alternation so an id is not a prefix match", () => { + // Unanchored, `no-eval` would also report `no-eval-in-tests`. + expect(sgFilterArgv(["no-eval", "no-console-warn"])).toEqual([ + "--filter", + "^(?:no-eval|no-console-warn)$", + ]); + }); + + it("passes no filter when nothing is selected", () => { + const unselected: string[] | undefined = undefined; + expect(sgFilterArgv(unselected)).toEqual([]); + expect(sgFilterArgv([])).toEqual([]); + }); +}); + +describe("check --rule", () => { + let project: string; + + async function check(...arguments_: string[]): Promise { + const { stdout } = await runCli([ + "check", + "-d", + project, + "--json", + ...arguments_, + ]); + return JSON.parse(stdout.trim()) as CheckOutput; + } + + beforeEach(async () => { + project = await mkdtemp(join(tmpdir(), "taskless-rule-filter-")); + await cp(fixturesDirectory, project, { recursive: true }); + }); + + afterEach(async () => { + await rm(project, { recursive: true, force: true }); + }); + + it("narrows an ast-grep run to the named rule", async () => { + const filtered = await check("--rule", "no-eval"); + + expect(triples(filtered.results)).toEqual(["ast-grep no-eval sample.js"]); + // The flag's value is not a path: were it scanned as one it would not + // exist, and `check` would take its "every supplied path was filtered out" + // branch — an empty result set that looks exactly like a rule that never + // fires. + expect(filtered.results.length).toBeGreaterThan(0); + }); + + it("errors naming an id no rule directory has", async () => { + const { stdout, exitCode } = await runCli([ + "check", + "-d", + project, + "--json", + "--rule", + "no-such-rule", + ]); + const envelope = JSON.parse(stdout.trim()) as { + ok: boolean; + code: string; + message: string; + }; + + expect(envelope.ok).toBe(false); + expect(envelope.code).toBe("RULE_NOT_FOUND"); + // Naming the id is the whole point: a typo that measured nothing would + // report "0 findings", which is also what a clean rule reports. + expect(envelope.message).toContain("no-such-rule"); + expect(exitCode).toBe(1); + }); + + it("reports exactly what an unfiltered run reports for that rule", async () => { + const everything = await check(); + const filtered = await check("--rule", "no-console-warn"); + + expect(triples(filtered.results)).toEqual( + triples( + everything.results.filter( + (finding) => finding.ruleId === "no-console-warn" + ) + ) + ); + // Only warnings survive the filter, so the run that failed on `no-eval` + // now succeeds — the filter reaches the exit code, not just the output. + expect(everything.success).toBe(false); + expect(filtered.success).toBe(true); + }); + + describe("with a gitignored copy of the project's files", () => { + beforeEach(async () => { + await mkdir(join(project, "ignored"), { recursive: true }); + await cp(join(project, "README.md"), join(project, "ignored/README.md")); + await cp(join(project, "sample.js"), join(project, "ignored/sample.js")); + await writeFile(join(project, ".gitignore"), "ignored/\n"); + await execFileAsync("git", ["init", "--quiet"], { cwd: project }); + }); + + it("keeps the exclusions a whole-project run applies", async () => { + const filtered = await check("--rule", "no-eval"); + + // Not vacuous: the tracked copy of the same file still fires. + expect(triples(filtered.results)).toEqual(["ast-grep no-eval sample.js"]); + }); + }); + + const withVale = valeAvailable ? describe : describe.skip; + + withVale("with the Vale binary available", () => { + it("narrows a Vale run to the named rule", async () => { + const filtered = await check("--rule", "no-obviously"); + + expect(triples(filtered.results)).toEqual([ + "vale no-obviously README.md", + ]); + }); + + it("unions repeated --rule across both engines", async () => { + const filtered = await check("--rule", "no-eval", "--rule", "no-simply"); + + expect(triples(filtered.results)).toEqual([ + "ast-grep no-eval sample.js", + "vale no-simply README.md", + ]); + }); + + it("matches the unfiltered run for a Vale rule, scope included", async () => { + const everything = await check(); + const filtered = await check("--rule", "no-obviously"); + + // The equality that matters for Vale: each rule's own matchers are kept + // verbatim in the filtered config, so removing the other rules cannot + // change this one's scope. + expect(triples(filtered.results)).toEqual( + triples( + everything.results.filter( + (finding) => finding.ruleId === "no-obviously" + ) + ) + ); + }); + + it("selects both rules when two engines hold the id", async () => { + // `rules delete` refuses an ambiguous id because deleting the wrong rule + // is irreversible. Measuring is not, and an unfiltered `check` would have + // run both, so both run and `source` tells them apart. + const valeRule = join(project, ".taskless/rules/vale/no-eval"); + await mkdir(valeRule, { recursive: true }); + await writeFile( + join(valeRule, "no-eval.yml"), + [ + "extends: existence", + "message: \"Avoid 'objectionable'\"", + "level: warning", + "tokens:", + " - objectionable", + "", + ].join("\n") + ); + await writeFile( + join(valeRule, ".vale.ini"), + ["[*.md]", "tskl) rule = no-eval", "no-eval.no-eval = YES", ""].join( + "\n" + ) + ); + + const filtered = await check("--rule", "no-eval"); + + expect(triples(filtered.results)).toEqual([ + "ast-grep no-eval sample.js", + "vale no-eval README.md", + ]); + }); + + it("keeps a gitignored copy out of a filtered Vale run", async () => { + await mkdir(join(project, "ignored"), { recursive: true }); + await cp(join(project, "README.md"), join(project, "ignored/README.md")); + await writeFile(join(project, ".gitignore"), "ignored/\n"); + await execFileAsync("git", ["init", "--quiet"], { cwd: project }); + + const filtered = await check("--rule", "no-obviously"); + + expect(triples(filtered.results)).toEqual([ + "vale no-obviously README.md", + ]); + }); + }); +}); From 9f398b0a773b91e3ca6aeb96df523c6bd86a6e29 Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 22 Sep 2026 13:44:14 -0700 Subject: [PATCH 2/2] fix(check): validate every Vale rule under --rule, and share the argv scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- .../2026-09-22-check-rule-filter/design.md | 2 + .../specs/cli-check/spec.md | 6 ++ openspec/specs/cli-check/spec.md | 6 ++ packages/cli/src/commands/check.ts | 28 +++---- packages/cli/src/rules/assemble.ts | 32 ++++---- packages/cli/src/rules/rule-filter.ts | 37 ++++----- packages/cli/src/util/argv.ts | 31 ++++++- packages/cli/test/check-rule-filter.test.ts | 81 +++++++++++++++++++ packages/cli/test/help-flag.test.ts | 22 +++++ 9 files changed, 193 insertions(+), 52 deletions(-) diff --git a/openspec/changes/archive/2026-09-22-check-rule-filter/design.md b/openspec/changes/archive/2026-09-22-check-rule-filter/design.md index 91602aa9..836d798b 100644 --- a/openspec/changes/archive/2026-09-22-check-rule-filter/design.md +++ b/openspec/changes/archive/2026-09-22-check-rule-filter/design.md @@ -22,6 +22,8 @@ MinAlertLevel = suggestion **Instead, assembly is narrowed.** `assembleValeConfig` takes the selected ids and emits only those rules' blocks, each rule's own matchers verbatim. The rule's scope is then byte-identical to what it is in a full run. +**Narrowing applies to what is written, not to what is validated.** Every Vale rule's config is still read and put through the config schema, and any rejection still refuses the whole assembly; only the surviving blocks are filtered. Validating just the selected rules would let `check --rule good` exit clean in a project where `bad`'s config is rejected, while an unfiltered `check` there refuses the Vale engine and reports nothing for `good` — the filtered run would then report findings the unfiltered run never produced, which is the exact equality this flag rests on. + Removing the other rules' blocks cannot change the surviving rule's effective setting, and that is a fact about the config schema rather than an assumption: a rule's config may only assign its own `.` key — the schema rejects an assignment that "names another rule" — so no removed block could have been turning the selected rule on or off. Vale's positional precedence (last matcher wins; since 3.21.0 last assignment within a matcher wins) has nothing to act on across rules. ### ast-grep narrows with `--filter`, not by narrowing `ruleDirs` diff --git a/openspec/changes/archive/2026-09-22-check-rule-filter/specs/cli-check/spec.md b/openspec/changes/archive/2026-09-22-check-rule-filter/specs/cli-check/spec.md index cf1f3720..65fbd5e7 100644 --- a/openspec/changes/archive/2026-09-22-check-rule-filter/specs/cli-check/spec.md +++ b/openspec/changes/archive/2026-09-22-check-rule-filter/specs/cli-check/spec.md @@ -30,6 +30,12 @@ A rule id is the name of a rule's directory under `.taskless/rules//`. T - **WHEN** a user runs `taskless check --rule ` with no positional paths in a project with git-ignored directories - **THEN** the CLI SHALL NOT report findings from `.taskless/`, `.git/`, or git-ignored paths +#### Scenario: A refused sibling config refuses a filtered run too + +- **WHEN** a user runs `taskless check --rule ` in a project where a DIFFERENT Vale rule's config is rejected by the config schema +- **THEN** the CLI SHALL refuse the Vale engine and exit non-zero, exactly as an unfiltered `taskless check` does +- **AND** the refusal SHALL name the rejected rule even though `--rule` did not select it + #### Scenario: An unknown rule id is refused - **WHEN** a user runs `taskless check --rule ` and no engine directory holds a rule directory named `` diff --git a/openspec/specs/cli-check/spec.md b/openspec/specs/cli-check/spec.md index 0072efc8..38b713f9 100644 --- a/openspec/specs/cli-check/spec.md +++ b/openspec/specs/cli-check/spec.md @@ -426,6 +426,12 @@ A rule id is the name of a rule's directory under `.taskless/rules//`. T - **WHEN** a user runs `taskless check --rule ` with no positional paths in a project with git-ignored directories - **THEN** the CLI SHALL NOT report findings from `.taskless/`, `.git/`, or git-ignored paths +#### Scenario: A refused sibling config refuses a filtered run too + +- **WHEN** a user runs `taskless check --rule ` in a project where a DIFFERENT Vale rule's config is rejected by the config schema +- **THEN** the CLI SHALL refuse the Vale engine and exit non-zero, exactly as an unfiltered `taskless check` does +- **AND** the refusal SHALL name the rejected rule even though `--rule` did not select it + #### Scenario: An unknown rule id is refused - **WHEN** a user runs `taskless check --rule ` and no engine directory holds a rule directory named `` diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index 358208a0..fab1a094 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -88,24 +88,20 @@ const VALUE_FLAGS = ["--timeout", "--rule"] as const; * number as covering two. Both spellings are accepted (`--rule a` and * `--rule=a`), and scanning stops at `--` so a path literally named `--rule` * after the end-of-options marker is a path. + * + * Read through `splitRawArguments`, the same scanner {@link extractPositionalPaths} + * uses, rather than a second scan of its own. A private scan would not know + * which OTHER flags consume a token. For the malformed `check --timeout --rule + * no-eval`, the shared scanner hands `--rule` to `--timeout` as its value and + * `no-eval` to `positionals`; a `--rule`-only scan would instead read `--rule` + * as a flag and claim `no-eval` as a rule id, so the same tokens would be both + * a path and a rule id in one run. One pass cannot disagree with itself. */ export function extractRuleFilters(rawArguments: string[]): string[] { - const ids: string[] = []; - for (let index = 0; index < rawArguments.length; index++) { - const argument = rawArguments[index]!; - if (argument === "--") break; - if (argument === "--rule") { - const value = rawArguments[index + 1]; - if (value !== undefined && value !== "--") { - ids.push(value); - index++; - } - continue; - } - if (argument.startsWith("--rule=")) - ids.push(argument.slice("--rule=".length)); - } - return ids.filter((id) => id !== ""); + return splitRawArguments(rawArguments, VALUE_FLAGS) + .values.filter((entry) => entry.flag === "--rule") + .map((entry) => entry.value) + .filter((id) => id !== ""); } /** Parse `--timeout ` into milliseconds; invalid/absent → undefined (default). */ diff --git a/packages/cli/src/rules/assemble.ts b/packages/cli/src/rules/assemble.ts index fd61d0e6..f22ef826 100644 --- a/packages/cli/src/rules/assemble.ts +++ b/packages/cli/src/rules/assemble.ts @@ -180,20 +180,19 @@ export interface AssembleOptions { * setting the selected rule's `.` value. Vale's positional precedence * therefore has nothing left to act on across rules, and the surviving block * resolves exactly as it did among the others. + * + * **This narrows what is WRITTEN, never what is VALIDATED.** Every Vale rule + * in the project is still read and put through the config schema, and any + * rejection still refuses the whole assembly. Skipping an unselected rule's + * validation would make `--rule good` exit clean in a project where `bad`'s + * config is rejected, while an unfiltered `check` in that same project + * refuses the Vale engine outright — so the filtered run would report + * findings the unfiltered run never produced, which is the one equality this + * flag exists to preserve. */ ruleIds?: readonly string[]; } -/** `available` narrowed to `selected`, or all of it when nothing was selected. */ -function selectRuleIds( - available: string[], - selected: readonly string[] | undefined -): string[] { - if (selected === undefined) return available; - const wanted = new Set(selected); - return available.filter((ruleId) => wanted.has(ruleId)); -} - /** * Assemble `.taskless/.vale.ini` from every Vale rule's own config. * @@ -211,10 +210,12 @@ export async function assembleValeConfig( cwd: string, options: AssembleOptions = {} ): Promise { - const ruleIds = selectRuleIds( - await listRuleIds(cwd, "vale"), - options.ruleIds - ); + const ruleIds = await listRuleIds(cwd, "vale"); + // A filter selects which validated blocks are written, and is deliberately + // NOT applied to `ruleIds`: the loop below has to reach every rule so a + // rejected sibling still lands in `refusals`. + const selected = + options.ruleIds === undefined ? undefined : new Set(options.ruleIds); const blocks: string[] = []; const sections = new Set(); const advisories: string[] = []; @@ -237,6 +238,9 @@ export async function assembleValeConfig( refusals.push({ ruleId, rejections: verdict.rejections }); continue; } + // Validated above, filtered here. Its sections and advisories are dropped + // with its block, because they describe a rule this run does not run. + if (selected !== undefined && !selected.has(ruleId)) continue; for (const pattern of verdict.sections) sections.add(pattern); advisories.push(...verdict.advisories); blocks.push(valeRuleBlock(ruleId, source)); diff --git a/packages/cli/src/rules/rule-filter.ts b/packages/cli/src/rules/rule-filter.ts index cfb9d68b..d82e59bf 100644 --- a/packages/cli/src/rules/rule-filter.ts +++ b/packages/cli/src/rules/rule-filter.ts @@ -1,6 +1,6 @@ import { CLIError } from "../util/cli-error"; -import { listRuleIds } from "./engines"; -import { ENGINES, RULES_DIRECTORY, type EngineName } from "./layout"; +import { findRuleEngines } from "./engines"; +import { RULES_DIRECTORY, type EngineName } from "./layout"; /** * Which rules a run is restricted to, split by the engine that owns each one. @@ -18,14 +18,6 @@ export interface RuleSelection { runtime: string[]; } -/** Whether `id` names a rule directory under any engine, engine by engine. */ -function enginesHolding( - id: string, - byEngine: Record -): EngineName[] { - return ENGINES.filter((engine) => byEngine[engine].includes(id)); -} - /** * Resolve `--rule` ids into the per-engine selection a run is restricted to. * @@ -53,21 +45,25 @@ export async function resolveRuleSelection( cwd: string, requested: readonly string[] ): Promise { - const byEngine = {} as Record; - await Promise.all( - ENGINES.map(async (engine) => { - byEngine[engine] = await listRuleIds(cwd, engine); - }) - ); - // Deduplicated, because `--rule a --rule a` is one rule, and an id repeated // into the ast-grep filter alternation or the Vale assembly would otherwise // be a rule listed twice. Insertion order is kept so the unknown-id message // reads back in the order the ids were typed. const unique = [...new Set(requested)]; - const unknown = unique.filter( - (id) => enginesHolding(id, byEngine).length === 0 + + // `findRuleEngines` is the one implementation of "which engines hold this + // id", shared with `rules delete` and `test`. A second copy here — listing + // every engine's directory and matching names — would be a second definition + // of rule identity, and a later change to one (case-insensitive ids, a new + // engine) would silently diverge from the other. + const holders = new Map(); + await Promise.all( + unique.map(async (id) => { + holders.set(id, await findRuleEngines(cwd, id)); + }) ); + + const unknown = unique.filter((id) => holders.get(id)!.length === 0); if (unknown.length > 0) { throw new CLIError( `No rule named ${unknown.map((id) => `"${id}"`).join(", ")} under ` + @@ -79,8 +75,7 @@ export async function resolveRuleSelection( const selection: RuleSelection = { sg: [], vale: [], runtime: [] }; for (const id of unique) { - for (const engine of enginesHolding(id, byEngine)) - selection[engine].push(id); + for (const engine of holders.get(id)!) selection[engine].push(id); } return selection; } diff --git a/packages/cli/src/util/argv.ts b/packages/cli/src/util/argv.ts index 9f429dc9..05ed3f95 100644 --- a/packages/cli/src/util/argv.ts +++ b/packages/cli/src/util/argv.ts @@ -28,6 +28,26 @@ export interface SplitArguments { * by a value-taking flag are not included. */ flags: string[]; + /** + * Every value a value-taking flag carried, in order, in both spellings + * (`--flag value` and `--flag=value`). + * + * Here rather than in each caller because a second scanner is a second + * opinion about what the same tokens mean. `check` needs every `--rule` + * value, and a hand-rolled scan of its own would not know that `--timeout` + * consumes the token after it: `check --timeout --rule no-eval` would give + * `no-eval` to `--rule` while this scanner had already handed it to + * `positionals`. Reading both answers off one pass makes that disagreement + * impossible rather than unlikely. + */ + values: FlagValue[]; +} + +/** One occurrence of a value-taking flag, with the value it consumed. */ +export interface FlagValue { + /** The flag as written, without any `=value` suffix. */ + flag: string; + value: string; } /** @@ -43,6 +63,7 @@ export function splitRawArguments( valueFlags.length > 0 ? new Set([...DIR_FLAGS, ...valueFlags]) : DIR_FLAGS; const positionals: string[] = []; const flags: string[] = []; + const values: FlagValue[] = []; for (let index = 0; index < rawArguments.length; index++) { const argument = rawArguments[index]!; if (argument === END_OF_OPTIONS) { @@ -54,18 +75,26 @@ export function splitRawArguments( flags.push(argument); // `--dir=` carries its own value; `-d ` eats the next token — // unless that token is `--`, which ends the options rather than being one. + const equals = argument.indexOf("="); + if (equals > 0) { + const flag = argument.slice(0, equals); + if (consumesValue.has(flag)) + values.push({ flag, value: argument.slice(equals + 1) }); + continue; + } if ( consumesValue.has(argument) && rawArguments[index + 1] !== undefined && rawArguments[index + 1] !== END_OF_OPTIONS ) { + values.push({ flag: argument, value: rawArguments[index + 1]! }); index++; } continue; } positionals.push(argument); } - return { positionals, flags }; + return { positionals, flags, values }; } /** diff --git a/packages/cli/test/check-rule-filter.test.ts b/packages/cli/test/check-rule-filter.test.ts index 88ed6e9b..875be200 100644 --- a/packages/cli/test/check-rule-filter.test.ts +++ b/packages/cli/test/check-rule-filter.test.ts @@ -7,6 +7,7 @@ import { promisify } from "node:util"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; import { extractRuleFilters } from "../src/commands/check"; +import { splitRawArguments } from "../src/util/argv"; import { sgFilterArgv } from "../src/rules/scan"; import { findValeBinary } from "../src/rules/vale/binary"; import { migrateFixture } from "./support/current-project"; @@ -66,6 +67,7 @@ interface CheckFinding { interface CheckOutput { success: boolean; results: CheckFinding[]; + failures?: string[]; } /** `(source, ruleId, file)` triples, sorted, for order-free comparison. */ @@ -89,6 +91,17 @@ describe("extractRuleFilters", () => { // After `--` every token is a path, including one spelled like this flag. expect(extractRuleFilters(["check", "--", "--rule", "a"])).toEqual([]); }); + + it("agrees with the positional scan about a value-taking flag", () => { + // Malformed: `--timeout` was given no value, so it eats `--rule` and + // `no-eval` is a path. Both readings come off one scan, so they cannot + // disagree about whether `no-eval` is a path or a rule id. + const argv = ["check", "--timeout", "--rule", "no-eval"]; + expect(extractRuleFilters(argv)).toEqual([]); + expect( + splitRawArguments(argv, ["--timeout", "--rule"]).positionals + ).toEqual(["check", "no-eval"]); + }); }); describe("sgFilterArgv", () => { @@ -198,6 +211,74 @@ describe("check --rule", () => { }); }); + describe("with a sibling Vale rule the config schema rejects", () => { + // No Vale binary needed: a rejected config refuses the engine during + // assembly, before anything is spawned. + beforeEach(async () => { + const broken = join(project, ".taskless/rules/vale/bad-rule"); + await mkdir(broken, { recursive: true }); + await writeFile( + join(broken, "bad-rule.yml"), + [ + "extends: existence", + "message: \"Avoid 'bad'\"", + "level: warning", + "tokens:", + " - bad", + "", + ].join("\n") + ); + // Assigns ANOTHER rule's key, which `schemas/vale-config.ts` rejects as + // a cross-rule override. + await writeFile( + join(broken, ".vale.ini"), + [ + "[*.md]", + "tskl) rule = bad-rule", + "no-simply.no-simply = NO", + "", + ].join("\n") + ); + }); + + it("refuses a filtered run exactly as the unfiltered run does", async () => { + const everything = await check(); + const filtered = await check("--rule", "no-obviously"); + + // The unfiltered run is the reference: one rejected config refuses the + // whole Vale engine, fail-closed, and nothing Vale would have found is + // reported. + expect(everything.success).toBe(false); + expect(everything.failures?.join("\n")).toContain("bad-rule"); + + // `--rule` narrows what runs; it must not narrow what is VALIDATED. + // Were the unselected `bad-rule` skipped before validation, this run + // would assemble `no-obviously` alone, report its findings and exit + // clean — a number the unfiltered run never produces for that id. + expect(filtered.success).toBe(false); + expect(filtered.failures?.join("\n")).toContain("bad-rule"); + expect( + filtered.results.filter((finding) => finding.source === "vale") + ).toEqual([]); + expect(triples(filtered.results)).toEqual( + triples( + everything.results.filter( + (finding) => finding.ruleId === "no-obviously" + ) + ) + ); + }); + + it("refuses even when the filter names no Vale rule at all", async () => { + // The ast-grep selection is the only non-empty one here, so this is the + // path where Vale assembly could most plausibly be skipped outright. + const filtered = await check("--rule", "no-eval"); + + expect(filtered.success).toBe(false); + expect(filtered.failures?.join("\n")).toContain("bad-rule"); + }); + }); + const withVale = valeAvailable ? describe : describe.skip; withVale("with the Vale binary available", () => { diff --git a/packages/cli/test/help-flag.test.ts b/packages/cli/test/help-flag.test.ts index 53bb64ec..27957d59 100644 --- a/packages/cli/test/help-flag.test.ts +++ b/packages/cli/test/help-flag.test.ts @@ -105,6 +105,7 @@ describe("splitRawArguments", () => { expect(splitRawArguments(["-d", "/tmp", "check"])).toEqual({ positionals: ["check"], flags: ["-d"], + values: [{ flag: "-d", value: "/tmp" }], }); }); @@ -112,6 +113,7 @@ describe("splitRawArguments", () => { expect(splitRawArguments(["check", "--", "-h", "--json"])).toEqual({ positionals: ["check", "-h", "--json"], flags: ["--"], + values: [], }); }); @@ -119,6 +121,7 @@ describe("splitRawArguments", () => { expect(splitRawArguments(["-d", "--", "src"])).toEqual({ positionals: ["src"], flags: ["-d", "--"], + values: [], }); }); @@ -128,6 +131,25 @@ describe("splitRawArguments", () => { .positionals ).toEqual(["check", "src"]); }); + + it("reports a value-taking flag's value in both spellings", () => { + // One pass answers both "which tokens are paths" and "what did this flag + // get", so a caller that needs the second never writes a second scanner. + expect( + splitRawArguments( + ["check", "--rule", "a", "--rule=b", "--dir=/tmp"], + ["--rule"] + ).values + ).toEqual([ + { flag: "--rule", value: "a" }, + { flag: "--rule", value: "b" }, + { flag: "--dir", value: "/tmp" }, + ]); + }); + + it("does not report a value for a flag that takes none", () => { + expect(splitRawArguments(["check", "--json=yes"]).values).toEqual([]); + }); }); describe("hasHelpFlag", () => {