diff --git a/.changeset/test-show-fixture-findings.md b/.changeset/test-show-fixture-findings.md new file mode 100644 index 00000000..7e94cbe3 --- /dev/null +++ b/.changeset/test-show-fixture-findings.md @@ -0,0 +1,17 @@ +--- +"@taskless/cli": patch +--- + +`test --json` now reports the findings a rule's fixtures produced. + +Each rule result carries a `findings` array: the same finding shape `check --json` prints — `source`, `ruleId`, `severity`, `message`, `file`, `range`, `matchedText`, and the optional `note` and `fix` — plus a `bucket` of `"pass"` or `"fail"` naming the fixture that produced it. The rendered `message` is the point. A rule whose message interpolates its captures can have the slots in the wrong order, fire on every `fail/` fixture, stay quiet on every `pass/` one, and be reported as a rule that passed; the rendered message is the only evidence otherwise, and until now the only way to see it was a second `check` run against a fixture path you had to construct yourself. + +The array is **always present and empty rather than absent** — for a rule that produced nothing, one whose verification failed before its fixtures ran, a refused runtime rule, `verify` (which runs no fixtures at all), and ast-grep rules, whose findings are not surfaced yet. Fail-bucket findings are reported on a passing run too, since that is the only run that produces them. + +On the human path a passing rule still prints one line. A **failing** rule now prints the findings that bear on the failure beneath it, labelled by bucket and rendered the way `check` renders a finding. That includes pass-bucket findings: `pass fixture wrongly fired: ` said _that_ it happened and never what matched. + +Vale and runtime rules only. ast-grep follows separately: the vendored binary's `sg test` has no `--json` and no output-format flag, and its fixtures are inline YAML scalars rather than files. + +No flag was added, and nothing new is executed: the findings were already in hand and were being discarded. + +**If you consume `@taskless/cli/schemas`:** `zod`'s `.parse()` strips keys the schema does not declare, so a consumer still on the previously published `verifyTestOutputSchema` will silently drop `findings` from the payload it returns until the dependency is upgraded. Nothing breaks — but the field simply not being there, on a CLI that is emitting it, is the kind of thing that generates a bug report. diff --git a/openspec/changes/archive/2026-09-23-test-show-fixture-findings/proposal.md b/openspec/changes/archive/2026-09-23-test-show-fixture-findings/proposal.md new file mode 100644 index 00000000..5318f1e7 --- /dev/null +++ b/openspec/changes/archive/2026-09-23-test-show-fixture-findings/proposal.md @@ -0,0 +1,66 @@ +## Why + +`test --json` reports a boolean per rule and nothing else: + +```json +{ + "ok": true, + "rules": [ + { + "engine": "vale", + "ruleId": "no-hedging", + "ok": true, + "errors": [], + "violations": [], + "ran": true + } + ] +} +``` + +`check --json` over the same rule's `fail/` fixture returns three findings carrying a rendered `message`, `file`, `range`, `matchedText` and `severity`. The rendered message is what a verdict cannot express: a `substitution` rule whose message interpolates its captures can have the `%s` slots in the wrong order, fire on every `fail/` fixture, stay quiet on every `pass/` one, and be reported as a rule that passed. An agent authoring a rule has no way to see the message it wrote except by running a second command against a path it has to construct. + +The findings already exist in-process and are discarded at a seam: + +- **Vale** — `packages/cli/src/rules/vale/verify.ts` reduces `outcome.results`, a `CheckResult[]`, to `firedIn`, a `Set` of file paths, and derives `missingFailures`/`unexpectedFindings` from the set. Message, range, matched text and severity die at that line. +- **Runtime** — `packages/cli/src/rules/runtime/run-fixtures.ts` reads only `execution.findings.length`. `findings` is a `CheckResult[]`. + +So this is plumbing rather than a new engine invocation: no extra subprocess, no second scan, nothing re-run. + +## What Changes + +- **`test --json` carries a `findings` array on every rule result**, each entry the `CheckResult` shape `check --json` already prints plus a `bucket` of `"pass"` or `"fail"`. `CheckResult` is reused verbatim rather than narrowed: it is already the published shape for a finding, and a second shape is a second thing to keep in sync. +- **The array is always present, never omitted.** Empty for an ast-grep rule, for a rule whose `verify` failed before fixtures ran, for a refused runtime rule, and for a rule that simply produced nothing. A key that is sometimes absent is one a consumer learns to treat as optional, and the reading that follows is that absence means zero — which is exactly the inference this change exists to make unnecessary. +- **Fail-bucket findings are reported on a green run.** They are the `%s`-order evidence, and a payload that carries the evidence only once the rule is failing carries it at the one moment it is no longer needed. +- **The human path keeps one line per passing rule.** Under a failing rule the offending findings print underneath, labelled by bucket, through `util/format.ts`'s existing `formatText` — the same renderer `check` uses, over the same `CheckResult[]` it takes as its parameter. A pass-bucket finding prints there: today `unexpectedFindings` names the file that wrongly fired and stops, which says _that_ it happened and not _what_ matched. Fail-bucket findings are not dumped on a green human run; that is what `--json` is for. +- **No new flag.** No `--verbose`: it has no precedent anywhere in `packages/cli/src` or `openspec/specs`, and the house pattern is a rich `--json` beside a terse human render. No `--include-fixtures` on `check` either: an explicit fixture path already works, because the `.taskless/**` exclusion in `rules/vale/run.ts` is gated on `wholeProject`. A flag an agent has to know to pass is the ergonomics problem being filed, not its fix. +- **Vale's `unexpectedFindings` and `missingFailures` are left alone.** They are internal to `ValeRuleVerification`, and whether the findings array subsumes them is a separate decision from whether the findings are reported at all. +- **ast-grep reports an empty array in this PR.** `runTests` spawns `sg test` and regex-parses `test result: ok. N passed; N failed;`. The vendored 0.45.3 binary has no `--json` and no output-format flag, and its fixtures are inline YAML scalars rather than files, so there is no directory to attribute a finding to. Surfacing them needs a different mechanism, which is PR 2. + +## Delivery shape + +**Single PR, targeting `main`.** The spec, the implementation and the archive land together. It is not a stack: nothing is stacked above this branch and nothing needs to be, because the two engines that already hold a `CheckResult[]` are the whole of what this change plumbs. + +ast-grep is deliberately **out of scope rather than a later slice of this change**, and gets its own proposal when it is written. Surfacing its findings is not more of the same work: `sg test` has no `--json` and no output-format flag, and its fixtures are inline YAML scalars rather than files, so it needs a way to get structured output out of a binary that offers none. Nothing in this change's spec delta requires it — the requirement states that the array is empty for an engine that does not yet surface its fixture findings, which is exactly what an ast-grep rule reports here and is true rather than misleading. + +## Capabilities + +### New Capabilities + +None. + +### Modified Capabilities + +- `cli-rule-validation`: "Test runs a rule's fixtures and runs verify first" restated to require the per-rule `findings` array, its always-present-and-possibly-empty contract, fail-bucket reporting on a green `--json` run, and the human rendering under a failing rule. + +Deliberately untouched: "A rejection names the constraint it violated", which governs `violations[]`. Those are CONSTRAINT violations — what a rule broke about its own shape — and a fixture finding is what a rule reported about a document. Conflating the two is the confusion this change is correcting. + +## Impact + +- `packages/cli/src/schemas/verify-test.ts`: `findings` on `ruleResultSchema`, the `CheckResult` shape plus `bucket`, defaulted to `[]` so the field is present in output for `verify` as well as `test`. +- `packages/cli/src/rules/vale/verify.ts`: keep the `CheckResult[]` beside `firedIn` and attribute each finding to a bucket by its file path. +- `packages/cli/src/rules/runtime/run-fixtures.ts`: keep `execution.findings`, tagged with the case's bucket. +- `packages/cli/src/rules/inspect.ts`: `findings` on `RuleTestResult`, populated per engine and empty on every early return. +- `packages/cli/src/commands/verify.ts`: the human render under a failing rule. +- Tests: `packages/cli/test/test-fixture-findings.test.ts`. +- `.changeset/test-show-fixture-findings.md`. diff --git a/openspec/changes/archive/2026-09-23-test-show-fixture-findings/specs/cli-rule-validation/spec.md b/openspec/changes/archive/2026-09-23-test-show-fixture-findings/specs/cli-rule-validation/spec.md new file mode 100644 index 00000000..de9789d3 --- /dev/null +++ b/openspec/changes/archive/2026-09-23-test-show-fixture-findings/specs/cli-rule-validation/spec.md @@ -0,0 +1,103 @@ +## MODIFIED Requirements + +### Requirement: Test runs a rule's fixtures and runs verify first + +`test` SHALL execute a rule against its test material — ast-grep test cases, Vale `pass`/`fail` fixture buckets, or the runtime harness — and SHALL run `verify` first, stopping on a verify failure without running the fixtures. + +Ordering is the point. When a rule is both malformed and under-fixtured, the fixture complaint is the less useful of the two errors and is the one that surfaces first if the checks run in the other order — so the author is told their fixtures are incomplete while the reason the rule could never have run goes unmentioned. + +A rule that populates only one bucket has proved only half of what a rule claims, whatever its engine. An engine SHALL NOT be trusted to report this itself: `ast-grep test` reports an empty `invalid:` bucket as `1 passed; 0 failed` and exits zero, so a rule that has never matched anything is indistinguishable from one that passed. + +A runtime rule's fixtures execute code, so they SHALL run only when `--dangerously-run-scripts` is passed, and under no other mechanism. `test` SHALL NOT consult the rule service to decide this, and SHALL make no network request in the course of running fixtures. + +That is deliberately stricter than the policy `check` applies, not softer. `check` executes rules as a side effect of scanning a repository, so a blessed signature admits code the user never asked to run and a reconcile stands between the request and the execution. `test` runs fixtures because the user asked for them, so the verb is the consent and the flag is the confirmation; a rule that `check` would run unflagged on a blessed signature still requires the flag here. Nothing executes under `test` that would not have executed under the shared policy. + +A run refused for want of the flag SHALL be reported as not run, and SHALL be reported as neither a pass nor a failure: the rule is not defective, and no action available to its holder would make a failure green. The refusal SHALL name the flag, and SHALL NOT direct the reader to authenticate — authenticating cannot bless a rule that never left the working tree, so naming it would offer a fix that is not one. + +`test` SHALL report, per rule, the findings its fixtures produced, as a `findings` array carrying the same finding shape `check --json` prints — `source`, `ruleId`, `severity`, `message`, `file`, `range`, `matchedText` and the optional `note` and `fix` — plus the `bucket` the fixture that produced it belongs to. + +The RENDERED message is the point, and it is what a verdict cannot carry. A rule whose message interpolates its captures can have its slots in the wrong order and still fire on every `fail/` fixture and stay quiet on every `pass/` one, so a boolean verdict reports it as a rule that passed. The only evidence that the message says what its author meant is the message as the engine rendered it, against material the author wrote, which `test` already has in hand and discards. + +The array SHALL be present on every rule result and SHALL be empty rather than absent when there is nothing to report — including for an engine that does not yet surface its fixture findings, a rule whose verification failed before fixtures ran, and a run the execution policy refused. A consumer SHALL NOT have to distinguish "this rule produced no findings" from "this command does not report findings", because a key that is sometimes absent is one a reader learns to treat as optional, and the reading that follows is that its absence means zero. + +Fail-bucket findings SHALL be reported under `--json` on a passing run. They are the evidence, and a payload that carries the evidence only once the rule is already failing carries it at the one moment it is no longer needed. + +The human rendering SHALL stay a single line per passing rule. Under a FAILING rule it SHALL print the findings that bear on the failure, labelled by bucket, using the same renderer `check` prints findings with, so one finding does not read two ways depending on which command surfaced it. A pass-bucket finding SHALL be printed there: it is a fixture that wrongly fired, and naming the file says only that it happened, while the finding says what matched. + +#### Scenario: A malformed rule reports the malformation, not the fixtures + +- **WHEN** `test` runs against a rule that is both invalid and missing a fixture bucket +- **THEN** it SHALL report the validation error +- **AND** it SHALL NOT report the fixture coverage as the failure + +#### Scenario: Vale fixtures are tested per bucket + +- **WHEN** `test` runs against a Vale rule +- **THEN** every `fail/` document SHALL produce at least one finding for that rule +- **AND** every `pass/` document SHALL produce none +- **AND** a rule populating only one bucket SHALL be reported as unverified rather than passing + +#### Scenario: ast-grep fixtures are counted per bucket + +- **WHEN** `test` runs against an ast-grep rule +- **THEN** the `valid:` and `invalid:` entries SHALL be counted across every `-test.yml` file the rule owns +- **AND** a rule populating only one bucket SHALL be reported as unverified rather than passing +- **AND** a rule whose buckets are all empty or absent SHALL be reported as unverified rather than passing +- **AND** a green `ast-grep test` run SHALL NOT on its own be sufficient to report the rule as passing + +#### Scenario: Runtime fixtures are run per case + +- **WHEN** `test` runs against a runtime rule with `--dangerously-run-scripts` +- **THEN** each directory under `.tests/fail/` SHALL be passed to the check as its `root` and SHALL produce at least one finding +- **AND** each directory under `.tests/pass/` SHALL be passed as its `root` and SHALL produce none +- **AND** a rule populating only one bucket SHALL be reported as unverified rather than passing +- **AND** a rule holding no fixture cases at all SHALL be reported as unverified rather than passing + +#### Scenario: A runtime rule without the flag is reported as not run + +- **WHEN** `test` runs against a runtime rule without `--dangerously-run-scripts` +- **THEN** the rule SHALL NOT be reported as passing +- **AND** the output SHALL say the fixtures did not run and why +- **AND** the output SHALL name `--dangerously-run-scripts` as what would run them +- **AND** the output SHALL NOT direct the reader to authenticate +- **AND** the rule SHALL NOT be counted among the rules tested +- **AND** the refusal alone SHALL NOT fail the command + +#### Scenario: Testing a runtime rule reaches no network + +- **WHEN** `test` runs against a runtime rule, with or without `--dangerously-run-scripts` +- **THEN** the CLI SHALL NOT request a token, resolve an organization, or reconcile +- **AND** the outcome SHALL NOT depend on authentication state, a git remote, or the availability of the rule service + +#### Scenario: A case that never reaches the check is reported as a fixture defect + +- **WHEN** a fixture case produces no narrow matches, so the check is never invoked +- **THEN** the CLI SHALL report that case as a fixture defect naming the case +- **AND** SHALL say the check did not run +- **AND** SHALL do so whether the case is in `pass/` or `fail/`, since a case that never invokes the check is evidence about the fixture rather than about the rule + +#### Scenario: A check that throws is distinguished from one that finds nothing + +- **WHEN** a runtime rule's check raises while running a fixture case +- **THEN** that SHALL be reported as the check failing, not as the case producing no findings + +#### Scenario: Test reports the findings its fixtures produced + +- **WHEN** `test --json` runs against a rule whose fixtures produced findings +- **THEN** each rule result SHALL carry a `findings` array +- **AND** each entry SHALL carry the rendered `message`, `file`, `range`, `matchedText` and `severity` that `check --json` reports for the same finding +- **AND** each entry SHALL name the bucket of the fixture that produced it +- **AND** the fail-bucket findings SHALL be reported even when the rule passed + +#### Scenario: The findings array is present and empty rather than absent + +- **WHEN** `test --json` reports a rule that produced no findings, whose verification failed before its fixtures ran, whose run was refused, or whose engine does not surface fixture findings +- **THEN** the rule result SHALL still carry a `findings` array +- **AND** that array SHALL be empty + +#### Scenario: A failing rule prints the findings that bear on the failure + +- **WHEN** `test` runs without `--json` and a rule fails because a `pass/` fixture fired +- **THEN** the offending findings SHALL be printed under that rule, labelled by bucket +- **AND** they SHALL be rendered the way `check` renders a finding +- **AND** a rule that passed SHALL still print one line diff --git a/openspec/changes/archive/2026-09-23-test-show-fixture-findings/tasks.md b/openspec/changes/archive/2026-09-23-test-show-fixture-findings/tasks.md new file mode 100644 index 00000000..ffa2c8b0 --- /dev/null +++ b/openspec/changes/archive/2026-09-23-test-show-fixture-findings/tasks.md @@ -0,0 +1,29 @@ +## 1. Spec + +- [x] 1.1 Restate "Test runs a rule's fixtures and runs verify first" in full — all 8 standing scenarios, byte-identical titles, and the full normative prose — with the findings requirement added. Verify by archiving into a throwaway commit and counting scenarios before and after. + +## 2. Schema + +- [x] 2.1 `schemas/verify-test.ts`: a `findings` array on `ruleResultSchema`, carrying the `CheckResult` fields plus `bucket: "pass" | "fail"`, defaulted to `[]` so every rule result in both commands' output carries it. + +## 3. Engines + +- [x] 3.1 `rules/vale/verify.ts`: carry the filtered `CheckResult[]` on `ValeRuleVerification` alongside `firedIn`, each finding bucketed by whether its file is a `pass/` or `fail/` fixture. Leave `missingFailures`/`unexpectedFindings` as they are. +- [x] 3.2 `rules/runtime/run-fixtures.ts`: collect `execution.findings` per case onto the report, tagged with the case's bucket. +- [x] 3.3 `rules/inspect.ts`: `findings` on `RuleTestResult`, populated from both engines, `[]` for ast-grep and on every path that returns before fixtures run. + +## 4. Human rendering + +- [x] 4.1 `commands/verify.ts`: under a failing rule print the bearing findings, labelled by bucket, through `util/format.ts`'s `formatText`. One line per passing rule, unchanged. + +## 5. Tests + +- [x] 5.1 Vale and runtime, both buckets, `--json` and human paths. +- [x] 5.2 A message-order regression: a `substitution` rule whose rendered message interpolates a token, asserting the rendered text, failing if the slots are swapped. +- [x] 5.3 A pass-bucket finding appears on the human path when the rule failed. +- [x] 5.4 `findings` present and empty for an ast-grep rule. + +## 6. Release + +- [x] 6.1 `.changeset/test-show-fixture-findings.md`, `patch`, noting that `zod`'s `parse()` strips unknown keys so a consumer on the old published schema drops the new field until they upgrade. +- [x] 6.2 `pnpm build`, `pnpm typecheck`, `pnpm lint`, `pnpm test`. diff --git a/openspec/specs/cli-rule-validation/spec.md b/openspec/specs/cli-rule-validation/spec.md index d6ed8c7a..c19b07ee 100644 --- a/openspec/specs/cli-rule-validation/spec.md +++ b/openspec/specs/cli-rule-validation/spec.md @@ -126,6 +126,16 @@ That is deliberately stricter than the policy `check` applies, not softer. `chec A run refused for want of the flag SHALL be reported as not run, and SHALL be reported as neither a pass nor a failure: the rule is not defective, and no action available to its holder would make a failure green. The refusal SHALL name the flag, and SHALL NOT direct the reader to authenticate — authenticating cannot bless a rule that never left the working tree, so naming it would offer a fix that is not one. +`test` SHALL report, per rule, the findings its fixtures produced, as a `findings` array carrying the same finding shape `check --json` prints — `source`, `ruleId`, `severity`, `message`, `file`, `range`, `matchedText` and the optional `note` and `fix` — plus the `bucket` the fixture that produced it belongs to. + +The RENDERED message is the point, and it is what a verdict cannot carry. A rule whose message interpolates its captures can have its slots in the wrong order and still fire on every `fail/` fixture and stay quiet on every `pass/` one, so a boolean verdict reports it as a rule that passed. The only evidence that the message says what its author meant is the message as the engine rendered it, against material the author wrote, which `test` already has in hand and discards. + +The array SHALL be present on every rule result and SHALL be empty rather than absent when there is nothing to report — including for an engine that does not yet surface its fixture findings, a rule whose verification failed before fixtures ran, and a run the execution policy refused. A consumer SHALL NOT have to distinguish "this rule produced no findings" from "this command does not report findings", because a key that is sometimes absent is one a reader learns to treat as optional, and the reading that follows is that its absence means zero. + +Fail-bucket findings SHALL be reported under `--json` on a passing run. They are the evidence, and a payload that carries the evidence only once the rule is already failing carries it at the one moment it is no longer needed. + +The human rendering SHALL stay a single line per passing rule. Under a FAILING rule it SHALL print the findings that bear on the failure, labelled by bucket, using the same renderer `check` prints findings with, so one finding does not read two ways depending on which command surfaced it. A pass-bucket finding SHALL be printed there: it is a fixture that wrongly fired, and naming the file says only that it happened, while the finding says what matched. + #### Scenario: A malformed rule reports the malformation, not the fixtures - **WHEN** `test` runs against a rule that is both invalid and missing a fixture bucket @@ -183,6 +193,27 @@ A run refused for want of the flag SHALL be reported as not run, and SHALL be re - **WHEN** a runtime rule's check raises while running a fixture case - **THEN** that SHALL be reported as the check failing, not as the case producing no findings +#### Scenario: Test reports the findings its fixtures produced + +- **WHEN** `test --json` runs against a rule whose fixtures produced findings +- **THEN** each rule result SHALL carry a `findings` array +- **AND** each entry SHALL carry the rendered `message`, `file`, `range`, `matchedText` and `severity` that `check --json` reports for the same finding +- **AND** each entry SHALL name the bucket of the fixture that produced it +- **AND** the fail-bucket findings SHALL be reported even when the rule passed + +#### Scenario: The findings array is present and empty rather than absent + +- **WHEN** `test --json` reports a rule that produced no findings, whose verification failed before its fixtures ran, whose run was refused, or whose engine does not surface fixture findings +- **THEN** the rule result SHALL still carry a `findings` array +- **AND** that array SHALL be empty + +#### Scenario: A failing rule prints the findings that bear on the failure + +- **WHEN** `test` runs without `--json` and a rule fails because a `pass/` fixture fired +- **THEN** the offending findings SHALL be printed under that rule, labelled by bucket +- **AND** they SHALL be rendered the way `check` renders a finding +- **AND** a rule that passed SHALL still print one line + ### Requirement: The generation loop runs verify and test The rule generation loop SHALL run `verify` and then `test` against a newly authored or newly delivered rule, and SHALL treat a failure of either as a rule that is not ready to report as complete. diff --git a/packages/cli/src/commands/verify.ts b/packages/cli/src/commands/verify.ts index 5e6b074c..1b2d7b96 100644 --- a/packages/cli/src/commands/verify.ts +++ b/packages/cli/src/commands/verify.ts @@ -5,6 +5,7 @@ import { defineCommand } from "citty"; import { ensureTasklessDirectory } from "../filesystem/directory"; import { requireCurrentSchema } from "../filesystem/migrate"; +import type { FixtureFinding } from "../rules/fixtures"; import { testOneRule, verifyOneRule, @@ -19,8 +20,51 @@ import { import { outputSchema as verifyTestOutputSchema } from "../schemas/verify-test"; import { makeErrorEnvelope, writeJsonError } from "../types/errors"; import { CLIError } from "../util/cli-error"; +import { formatText } from "../util/format"; import { markNotice } from "../util/notices"; +/** + * The findings a result carries, or none. + * + * `verify` never runs fixtures, so its results have no `findings` at all, while + * `test`'s always do — a narrowing rather than an optional field, because the + * two commands share this renderer and only one of them has anything to render. + */ +function findingsOf( + result: RuleVerification | RuleTestResult +): FixtureFinding[] { + return "findings" in result ? result.findings : []; +} + +/** + * The findings under a FAILING rule, labelled by bucket. + * + * Printed only on a failure, and only on the human path. A green run says one + * line per rule, which is what makes a wall of ticks readable; the evidence a + * passing run produced goes to `--json`, where something is reading it on + * purpose. + * + * `pass` first, because a `pass/` fixture that fired is usually the reason the + * rule failed, and because it is the case the existing output serves worst: + * `pass fixture wrongly fired: ` says THAT it happened and never what + * matched, which is the one thing the author has to know to fix it. + * + * Rendered through `check`'s own `formatText` rather than a second renderer. + * These are `CheckResult`s, which is its parameter type, and a finding that + * read two ways depending on which command surfaced it would be a worse + * outcome than any formatting this costs. + */ +function printFindings(findings: FixtureFinding[]): void { + for (const bucket of ["pass", "fail"] as const) { + const inBucket = findings.filter((finding) => finding.bucket === bucket); + if (inBucket.length === 0) continue; + console.log(` ${bucket} fixture findings:`); + for (const line of formatText(inBucket).split("\n")) { + console.log(line === "" ? "" : ` ${line}`); + } + } +} + /** * The shared body of `verify` and `test`. * @@ -171,6 +215,12 @@ async function runOverPath(options: { console.log(line); } } + // Under the errors, because the errors say which fixture is wrong and + // these say what the rule reported about it. A refused run has nothing + // to show: nothing ran. + if (!result.ok && !isRefused(result)) { + printFindings(findingsOf(result)); + } } // A rule that did not run is not among the rules tested. Counting it there // is the summary half of the same defect as the tick: "1 rule(s) tested" diff --git a/packages/cli/src/rules/fixtures.ts b/packages/cli/src/rules/fixtures.ts index cd569de1..b2adc22e 100644 --- a/packages/cli/src/rules/fixtures.ts +++ b/packages/cli/src/rules/fixtures.ts @@ -2,6 +2,7 @@ import type { Dirent } from "node:fs"; import { readdir } from "node:fs/promises"; import { isMissingDirectory } from "./errno"; +import type { CheckResult } from "../types/check"; /** * The two questions every engine's fixture reader asks, answered once. @@ -98,3 +99,30 @@ export function describeCoverageShortfall( ? `${ruleId} has no fixtures, so nothing shows it fires or stays quiet.` : `${ruleId} has only ${coverage.replace("-only", "")}${suffix} fixtures — half a claim.`; } + +/** + * The two buckets a fixture lives in, over the two engines whose buckets are + * directories. + * + * Declared here rather than beside either engine because a bucket is now part + * of what `test` REPORTS, not only of how each engine reads its own tests: a + * consumer reading `findings[].bucket` is reading one vocabulary, and it should + * not be one engine's copy of it that they happen to be reading. ast-grep's + * buckets are the `valid:`/`invalid:` keys of its own test YAML and keep their + * own spelling, for {@link FixtureCoverage}'s reason. + */ +export type FixtureBucket = "pass" | "fail"; + +/** + * One finding a fixture produced, with the bucket that produced it. + * + * {@link CheckResult} verbatim, which is deliberate: it is already the shape + * `check --json` prints, so a finding means the same thing whichever command + * surfaced it, and there is one shape to keep in step with the engines rather + * than two. `bucket` is the only addition, and it is the half a `CheckResult` + * cannot carry — a finding does not know whether the document it came from was + * supposed to be clean. + */ +export interface FixtureFinding extends CheckResult { + bucket: FixtureBucket; +} diff --git a/packages/cli/src/rules/inspect.ts b/packages/cli/src/rules/inspect.ts index 372d719a..cdd9e4cd 100644 --- a/packages/cli/src/rules/inspect.ts +++ b/packages/cli/src/rules/inspect.ts @@ -8,7 +8,7 @@ import { ruleDirectory, ruleFilePath, } from "./engines"; -import { describeCoverageShortfall } from "./fixtures"; +import { describeCoverageShortfall, type FixtureFinding } from "./fixtures"; import { type EngineName } from "./layout"; import { assessCaptureDirectory, @@ -116,6 +116,21 @@ export interface RuleTestResult { * on a pass, since a pass is the case it exists for. */ notices: string[]; + /** + * Every finding this rule's fixtures produced, tagged with its bucket. + * + * ALWAYS PRESENT, empty rather than absent. A rule that produced nothing, a + * rule whose `verify` failed before its fixtures ran, a refused runtime rule + * and an ast-grep rule all report `[]`, because a consumer must never have to + * tell "this rule produced no findings" from "this command does not report + * findings" — a key that is sometimes absent is one a reader learns to treat + * as optional, and then reads absence as zero. + * + * Both buckets, including on a rule that passed. The `fail` bucket is the + * evidence that the rendered messages say what their author meant, and it is + * only ever produced by the runs that passed. + */ + findings: FixtureFinding[]; } /** What `test` needs beyond a rule, all of it about the runtime engine. */ @@ -425,7 +440,7 @@ export async function testOneRule( // call, which would spawn `sg test` twice for one answer. const verification = await withIdCollision(cwd, ruleId, verdict); if (!verification.ok) { - return { ...verification, ran: false }; + return { ...verification, ran: false, findings: [] }; } const errors = [...result.tests.errors]; const violations: RuleViolation[] = []; @@ -483,12 +498,20 @@ export async function testOneRule( violations, ran: true, notices: verification.notices, + // Empty, and true. `sg test` reports `test result: ok. N passed; N + // failed;` and nothing else — the vendored binary has no `--json` and no + // output-format flag — and its fixtures are inline YAML scalars rather + // than files, so there is no document to attribute a finding to. + // Surfacing them needs a different mechanism than reading what the + // engine already handed us, which is the whole of what this does for the + // other two engines. + findings: [], }; } const verification = await verifyOneRule(cwd, rule); if (!verification.ok) { - return { ...verification, ran: false }; + return { ...verification, ran: false, findings: [] }; } if (engine === "vale") { @@ -502,6 +525,7 @@ export async function testOneRule( violations: [], ran: false, notices: [], + findings: [], }; } const errors: string[] = []; @@ -523,6 +547,7 @@ export async function testOneRule( violations: [], ran: true, notices: result.notices, + findings: result.findings, }; } @@ -561,6 +586,7 @@ export async function testOneRule( errors: [reason], violations: [], ran: false, + findings: [], refused: reason, notices: [], }; @@ -585,6 +611,7 @@ export async function testOneRule( errors: [reason], violations: [], ran: false, + findings: [], refused: reason, notices: [], }; @@ -606,6 +633,7 @@ export async function testOneRule( violations: [], ran: false, notices: [], + findings: [], }; } @@ -622,5 +650,6 @@ export async function testOneRule( violations: [], ran: true, notices: [], + findings: report.findings, }; } diff --git a/packages/cli/src/rules/runtime/fixtures.ts b/packages/cli/src/rules/runtime/fixtures.ts index 099750d8..38c81def 100644 --- a/packages/cli/src/rules/runtime/fixtures.ts +++ b/packages/cli/src/rules/runtime/fixtures.ts @@ -3,13 +3,11 @@ import { join } from "node:path"; import { bucketEntries, classifyCoverage, + type FixtureBucket, type FixtureCoverage, } from "../fixtures"; import { ruleTestsDirectory } from "../engines"; -/** The two buckets a fixture case can live in. */ -export type FixtureBucket = "pass" | "fail"; - /** * One fixture case: a DIRECTORY, whose path is the `root` the harness hands * the check. diff --git a/packages/cli/src/rules/runtime/run-fixtures.ts b/packages/cli/src/rules/runtime/run-fixtures.ts index 88edb97f..39c0ec56 100644 --- a/packages/cli/src/rules/runtime/run-fixtures.ts +++ b/packages/cli/src/rules/runtime/run-fixtures.ts @@ -1,4 +1,4 @@ -import { describeCoverageShortfall } from "../fixtures"; +import { describeCoverageShortfall, type FixtureFinding } from "../fixtures"; import type { RuntimeRule } from "./discover"; import { executeRuntimeRuleDetailed } from "./harness"; import type { RuntimeRunOptions } from "./harness"; @@ -43,6 +43,20 @@ export interface RuntimeFixtureReport { neverInvoked: string[]; /** Cases where the narrow or the check itself failed, rather than found nothing. */ checkFailures: FixtureCheckFailure[]; + /** + * Every finding the check produced, each tagged with its case's bucket. + * + * The lists above are counts of cases that broke an expectation; this is what + * the check actually SAID, which is the half a verdict cannot carry. A rule + * whose message interpolates its captures can have the slots reversed and + * still fire in every `fail/` case and stay quiet in every `pass/` one. + * + * A case counted in `checkFailures` contributes nothing here. A harness + * failure arrives as a single synthesized error-severity finding, and + * reporting that as something the rule found would attribute the harness's + * crash to the rule — the exact confusion `checkFailures` exists to prevent. + */ + findings: FixtureFinding[]; } /** `fail/case-1` — the bucket is half the identity of a case. */ @@ -71,6 +85,7 @@ export async function runRuntimeFixtures( const unexpectedFindings: string[] = []; const neverInvoked: string[] = []; const checkFailures: FixtureCheckFailure[] = []; + const findings: FixtureFinding[] = []; for (const fixtureCase of fixtures.cases) { const execution = await executeRuntimeRuleDetailed( @@ -95,6 +110,14 @@ export async function runRuntimeFixtures( continue; } + // Collected before the bucket expectations are judged, so a case that met + // its expectation still reports what it found. That is the whole point on + // the `fail/` side: those findings are the evidence the messages render + // correctly, and they exist only on the runs that passed. + for (const finding of execution.findings) { + findings.push({ ...finding, bucket: fixtureCase.bucket }); + } + if (fixtureCase.bucket === "fail") { if (execution.findings.length === 0) missingFailures.push(label(fixtureCase)); @@ -118,6 +141,7 @@ export async function runRuntimeFixtures( unexpectedFindings, neverInvoked, checkFailures, + findings, }; } diff --git a/packages/cli/src/rules/vale/verify.ts b/packages/cli/src/rules/vale/verify.ts index 2abb74cd..40049024 100644 --- a/packages/cli/src/rules/vale/verify.ts +++ b/packages/cli/src/rules/vale/verify.ts @@ -8,6 +8,7 @@ import { bucketEntries, classifyCoverage, type FixtureCoverage, + type FixtureFinding, } from "../fixtures"; import { runVale, type ValeRunOutcome } from "./run"; @@ -146,6 +147,21 @@ export interface ValeRuleVerification { missingFailures: string[]; /** Fixtures that should have been clean and were not. */ unexpectedFindings: string[]; + /** + * Every finding the fixtures produced, each tagged with its bucket. + * + * Carried ALONGSIDE the two lists above rather than instead of them. They + * answer "which fixture is wrong", which is what decides `passed`; this + * answers "what did the rule actually say", which is what an author needs to + * see and what no verdict can express — a rule whose message interpolates + * its captures can have the slots reversed, fire on every `fail/` fixture, + * stay quiet on every `pass/` one, and pass. + * + * Empty when Vale was never run, which a one-sided fixture set short-circuits + * into, for `findings`' reason on the reported envelope: absent and empty must + * not be two different answers to the same question. + */ + findings: FixtureFinding[]; /** * Which buckets held documents. Only `"both"` can be `passed: true`: a * `fail/` fixture proves the rule fires, a `pass/` fixture proves it does not @@ -247,6 +263,7 @@ export async function verifyValeRule( passed: false, missingFailures: [], unexpectedFindings: [], + findings: [], fixtures, notices: [], }; @@ -269,11 +286,27 @@ export async function verifyValeRule( // Only findings for the rule under test count. The config isolates it, so // this should be every finding — filtering anyway means a leak shows up as // a verification that still measures the right thing. - const firedIn = new Set( - outcome.results - .filter((result) => result.ruleId === ruleId) - .map((result) => result.file) + const ruleResults = outcome.results.filter( + (result) => result.ruleId === ruleId + ); + const firedIn = new Set(ruleResults.map((result) => result.file)); + + // The findings themselves, kept rather than reduced away. `firedIn` above + // is the same data with everything but the file path thrown out, and what + // it throws out — the rendered message, the range, the matched text — is + // the only evidence an author has that the rule says what they wrote. + // + // Bucketed by membership in `pass/`, with `fail/` as the remainder rather + // than as a second lookup. Vale ran over the rule's tests directory and + // nothing else, and `fixtureFiles` refuses a nested directory inside a + // bucket, so every file here is a fixture in one bucket or the other. + const passPaths = new Set( + passFixtures.map((file) => toRelativePosix(cwd, file)) ); + const findings: FixtureFinding[] = ruleResults.map((result) => ({ + ...result, + bucket: passPaths.has(result.file) ? "pass" : "fail", + })); const missingFailures = failFixtures .map((file) => toRelativePosix(cwd, file)) @@ -287,6 +320,7 @@ export async function verifyValeRule( passed: missingFailures.length === 0 && unexpectedFindings.length === 0, missingFailures, unexpectedFindings, + findings, fixtures, notices: outcome.notices, }; diff --git a/packages/cli/src/schemas/verify-test.ts b/packages/cli/src/schemas/verify-test.ts index 41dacdab..eee38134 100644 --- a/packages/cli/src/schemas/verify-test.ts +++ b/packages/cli/src/schemas/verify-test.ts @@ -7,6 +7,50 @@ import { z } from "zod"; * so they share one envelope. `ran` is `test`-only: `verify` never reaches a * test run, and `test` reports `false` when verification failed first. */ +/** + * A position, zero-based in both axes, exactly as `check --json` reports one. + */ +const positionSchema = z.object({ + line: z.number().int(), + column: z.number().int(), +}); + +/** + * One finding a rule's fixtures produced. + * + * The `CheckResult` shape `check --json` already prints, plus the `bucket` the + * fixture that produced it lives in. Reused verbatim rather than narrowed to + * what `test` "needs": a finding means the same thing whichever command + * surfaced it, and a second, smaller shape would be a second thing to keep in + * step with the engines. + * + * `message` is the field this exists for. It is the RENDERED message, with the + * rule's captures already interpolated, and it is the only evidence that a + * rule whose message interpolates its captures put the slots in the right + * order — such a rule fires on every `fail/` fixture, stays quiet on every + * `pass/` one, and a boolean verdict reports it as a rule that passed. + */ +const fixtureFindingSchema = z.object({ + source: z.string().describe("The engine that produced it"), + ruleId: z.string(), + severity: z.enum(["error", "warning", "info", "hint"]), + message: z + .string() + .describe( + "The message as the engine rendered it, with the rule's captures already interpolated" + ), + note: z.string().optional(), + file: z.string().describe("The fixture the finding was reported against"), + range: z.object({ start: positionSchema, end: positionSchema }), + matchedText: z.string(), + fix: z.string().optional(), + bucket: z + .enum(["pass", "fail"]) + .describe( + "The fixture bucket that produced it. A `pass` finding is a rule that fired where it should not have; a `fail` finding is the rule doing its job" + ), +}); + const ruleResultSchema = z.object({ engine: z.enum(["sg", "vale", "runtime"]), ruleId: z.string(), @@ -47,6 +91,12 @@ const ruleResultSchema = z.object({ .describe( "Things true about the rule that do not make it a failure, reported even on a pass. One notice per element, so a consumer can render each on its own; empty when there is nothing to say, never absent" ), + findings: z + .array(fixtureFindingSchema) + .default([]) + .describe( + "The findings this rule's fixtures produced, each tagged with its bucket. ALWAYS PRESENT, empty rather than absent — for a rule that produced nothing, for one whose verification failed before its fixtures ran, for a refused run, for `verify`, which runs no fixtures at all, and for an engine whose fixture findings are not surfaced yet. A key that is sometimes absent is one a reader learns to treat as optional, and reads absence as zero. The `fail` bucket is reported on a passing run too: it is the evidence that the rendered messages say what their author meant, and a payload carrying that only once the rule is already failing carries it at the one moment it is no longer needed" + ), }); /** Output schema for `taskless verify --json` and `taskless test --json`. */ diff --git a/packages/cli/test/test-fixture-findings.test.ts b/packages/cli/test/test-fixture-findings.test.ts new file mode 100644 index 00000000..c1f8d384 --- /dev/null +++ b/packages/cli/test/test-fixture-findings.test.ts @@ -0,0 +1,460 @@ +import { execFile } from "node:child_process"; +import { 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 { findValeBinary } from "../src/rules/vale/binary"; +import { cliRejectionToResult } from "./support/spawn-cli"; + +/** + * The findings `test` reports, per rule and per fixture bucket. + * + * The defect this file exists for: `test --json` reported a boolean per rule + * and nothing else, while the findings that produced the boolean were in hand + * and thrown away — Vale's reduced to a `Set` of file paths, runtime's to + * `findings.length`. A rule whose message interpolates its captures can have + * the slots in the wrong order, fire on every `fail/` fixture, stay quiet on + * every `pass/` one, and be reported as a rule that passed. The RENDERED + * message is the only evidence otherwise, so the assertions on it below are the + * point of the whole file and must not be relaxed to "some message". + * + * These spawn the built CLI rather than calling `testOneRule`, because the + * always-present `findings` key and the human rendering are both properties of + * what the command prints. + */ + +const execFileAsync = promisify(execFile); +const binPath = resolve(import.meta.dirname, "../dist/index.js"); + +const withVale = findValeBinary().path === undefined ? describe.skip : describe; + +let cwd: string; + +async function runCli(args: string[]) { + try { + const { stdout, stderr } = await execFileAsync("node", [binPath, ...args]); + return { stdout, stderr, exitCode: 0 }; + } catch (error) { + return cliRejectionToResult(error, [binPath, ...args]); + } +} + +interface Finding { + source: string; + ruleId: string; + severity: string; + message: string; + file: string; + range: { + start: { line: number; column: number }; + end: { line: number; column: number }; + }; + matchedText: string; + bucket: "pass" | "fail"; +} + +interface Report { + ok: boolean; + rules: { + engine: string; + ruleId: string; + ok: boolean; + errors: string[]; + ran?: boolean; + refused?: string; + findings: Finding[]; + }[]; +} + +function findingsOf(report: Report, bucket?: "pass" | "fail"): Finding[] { + const findings = report.rules.flatMap((rule) => rule.findings); + return bucket === undefined + ? findings + : findings.filter((finding) => finding.bucket === bucket); +} + +beforeEach(async () => { + cwd = await mkdtemp(join(tmpdir(), "tskl-findings-")); + await runCli(["init", "-d", cwd]); +}); + +afterEach(async () => { + await rm(cwd, { recursive: true, force: true }); +}); + +/* -------------------------------------------------------------------------- */ +/* Vale */ +/* -------------------------------------------------------------------------- */ + +const VALE_RULE = "use-email"; + +/** + * A `substitution` rule, chosen deliberately over `existence`. + * + * `substitution` is the check whose message takes TWO `%s` slots — the + * replacement and the match — so it is the one that can be wrong in the way a + * verdict cannot see. Vale fills them in that order, which is the opposite of + * the order they appear in the swap, and an author who assumes otherwise ships + * a rule that tells every reader to replace `email` with `e-mail`. + */ +const SUBSTITUTION_STYLE = [ + "extends: substitution", + `message: "Use '%s' instead of '%s'"`, + "level: warning", + "ignorecase: true", + "swap:", + " e-mail: email", + "", +].join("\n"); + +const VALE_CONFIG = [ + "[*.md]", + `tskl) rule = ${VALE_RULE}`, + `${VALE_RULE}.${VALE_RULE} = YES`, + "", +].join("\n"); + +/** The message Vale renders, with the slots in the order Vale fills them. */ +const RENDERED = "Use 'email' instead of 'e-mail'"; + +async function writeValeRule(options: { passFires?: boolean } = {}) { + const directory = join(cwd, ".taskless", "rules", "vale", VALE_RULE); + await mkdir(join(directory, ".tests", "pass"), { recursive: true }); + await mkdir(join(directory, ".tests", "fail"), { recursive: true }); + await writeFile(join(directory, `${VALE_RULE}.yml`), SUBSTITUTION_STYLE); + await writeFile(join(directory, ".vale.ini"), VALE_CONFIG); + await writeFile( + join(directory, ".tests", "fail", "bad.md"), + "Send an e-mail today.\n" + ); + await writeFile( + join(directory, ".tests", "pass", "ok.md"), + (options.passFires ?? false) + ? "Send an e-mail now.\n" + : "Send an email today.\n" + ); +} + +async function testVale(...extra: string[]) { + return runCli([ + "test", + `.taskless/rules/vale/${VALE_RULE}`, + "-d", + cwd, + ...extra, + ]); +} + +withVale("a Vale rule's fixture findings", () => { + it("reports the rendered message, so a swapped %s pair cannot pass", async () => { + // THE test. `substitution` renders replacement-then-match; a rule written + // the other way round renders "Use 'e-mail' instead of 'email'" and fires + // and stays quiet in exactly the same places, so nothing but this string + // distinguishes the two. + await writeValeRule(); + + const { stdout, exitCode } = await testVale("--json"); + const report = JSON.parse(stdout) as Report; + + expect(exitCode).toBe(0); + expect(report.ok).toBe(true); + const messages = findingsOf(report).map((finding) => finding.message); + expect(messages).toEqual([RENDERED]); + expect(messages).not.toContain("Use 'e-mail' instead of 'email'"); + }); + + it("carries the whole check finding, not a summary of one", async () => { + await writeValeRule(); + + const { stdout } = await testVale("--json"); + const report = JSON.parse(stdout) as Report; + const finding = findingsOf(report)[0]; + + expect(finding).toBeDefined(); + expect(finding?.source).toBe("vale"); + expect(finding?.ruleId).toBe(VALE_RULE); + expect(finding?.severity).toBe("warning"); + expect(finding?.matchedText).toBe("e-mail"); + expect(finding?.file).toContain("fail/bad.md"); + expect(finding?.range.start.line).toBe(0); + expect(finding?.range.start.column).toBe(8); + }); + + it("reports the fail bucket on a run that passed", async () => { + // The evidence is only ever produced by a green run. A payload that + // carried it once the rule was already failing would carry it at the one + // moment nobody needs it. + await writeValeRule(); + + const { stdout } = await testVale("--json"); + const report = JSON.parse(stdout) as Report; + + expect(report.rules[0]?.ok).toBe(true); + expect(findingsOf(report, "fail")).toHaveLength(1); + expect(findingsOf(report, "pass")).toHaveLength(0); + }); + + it("tags a wrongly-fired pass fixture as the pass bucket", async () => { + await writeValeRule({ passFires: true }); + + const { stdout, exitCode } = await testVale("--json"); + const report = JSON.parse(stdout) as Report; + + expect(exitCode).toBe(1); + expect(report.ok).toBe(false); + const passFindings = findingsOf(report, "pass"); + expect(passFindings).toHaveLength(1); + expect(passFindings[0]?.file).toContain("pass/ok.md"); + expect(passFindings[0]?.message).toBe(RENDERED); + }); + + it("prints one line and no findings when the rule passed", async () => { + // The human path stays scannable. A wall of ticks is the format's whole + // value, and the evidence goes to `--json`, where something reads it on + // purpose. + await writeValeRule(); + + const { stdout } = await testVale(); + + expect(stdout).toContain(`✓ vale/${VALE_RULE}`); + expect(stdout).not.toContain(RENDERED); + expect(stdout).not.toContain("fixture findings"); + }); + + it("prints what matched under a failing rule, not only which file", async () => { + // `pass fixture wrongly fired: ` says THAT it happened. The author + // still has to open the file to learn what matched, which is the one thing + // they need to fix it. + await writeValeRule({ passFires: true }); + + const { stdout } = await testVale(); + + expect(stdout).toContain(`✗ vale/${VALE_RULE}`); + expect(stdout).toContain("pass fixture wrongly fired:"); + expect(stdout).toContain("pass fixture findings:"); + expect(stdout).toContain(RENDERED); + // `check`'s own renderer, so a finding does not read two ways depending on + // which command surfaced it. + expect(stdout).toContain(`warning[${VALE_RULE}] ${RENDERED}`); + expect(stdout).toContain("> e-mail"); + }); +}); + +/* -------------------------------------------------------------------------- */ +/* Runtime */ +/* -------------------------------------------------------------------------- */ + +const RUNTIME_RULE = "no-eval"; + +const EVAL_CAPTURE = [ + "id: no-eval-abc12345", + "language: typescript", + "rule:", + " pattern: eval($ARG)", + "metadata:", + " taskless:", + " version: 1", + " kind: runtime", + " name: no-eval", + " check: check.ts", + " match: anchor", + "", +].join("\n"); + +const RUNTIME_MESSAGE = "eval on a non-literal is not allowed"; + +/** Reports only where the argument is not a literal, so a pass case can run. */ +const FLAGS_DYNAMIC_EVAL = String.raw`import { readFileSync } from "node:fs"; +import { join } from "node:path"; + +export default async function (root, matches) { + return matches + .filter((m) => !readFileSync(join(root, m.file), "utf8").includes('eval("')) + .map((m) => ({ + file: m.file, + line: m.line, + column: m.column, + message: "${RUNTIME_MESSAGE}", + severity: "warning", + })); +} +`; + +/** Reports every match, so the `pass/` case wrongly fires. */ +const FLAGS_EVERY_MATCH = `export default async function (root, matches) { + return matches.map((m) => ({ + file: m.file, + line: m.line, + column: m.column, + message: "${RUNTIME_MESSAGE}", + severity: "warning", + })); +} +`; + +async function writeRuntimeRule(check: string = FLAGS_DYNAMIC_EVAL) { + const directory = join(cwd, ".taskless", "rules", "runtime", RUNTIME_RULE); + await mkdir(join(directory, "captures"), { recursive: true }); + await writeFile(join(directory, "captures", "eval.yml"), EVAL_CAPTURE); + await writeFile(join(directory, "check.ts"), check); + + for (const [bucket, source] of [ + ["fail", "const input = globalThis.userInput;\neval(input);\n"], + ["pass", 'eval("1 + 1");\n'], + ] as const) { + const caseDirectory = join(directory, ".tests", bucket, `${bucket}-case`); + await mkdir(caseDirectory, { recursive: true }); + await writeFile(join(caseDirectory, "sample.ts"), source); + } +} + +async function testRuntime(...extra: string[]) { + return runCli([ + "test", + `.taskless/rules/runtime/${RUNTIME_RULE}`, + "-d", + cwd, + "--dangerously-run-scripts", + ...extra, + ]); +} + +describe("a runtime rule's fixture findings", () => { + it("reports what the check said, bucketed by case, on a passing run", async () => { + await writeRuntimeRule(); + + const { stdout, exitCode } = await testRuntime("--json"); + const report = JSON.parse(stdout) as Report; + + expect(exitCode).toBe(0); + expect(report.rules[0]?.ok).toBe(true); + const failFindings = findingsOf(report, "fail"); + expect(failFindings).toHaveLength(1); + expect(failFindings[0]?.message).toBe(RUNTIME_MESSAGE); + expect(failFindings[0]?.severity).toBe("warning"); + expect(failFindings[0]?.file).toContain("sample.ts"); + expect(findingsOf(report, "pass")).toHaveLength(0); + }); + + it("tags a wrongly-fired pass case as the pass bucket", async () => { + await writeRuntimeRule(FLAGS_EVERY_MATCH); + + const { stdout, exitCode } = await testRuntime("--json"); + const report = JSON.parse(stdout) as Report; + + expect(exitCode).toBe(1); + expect(findingsOf(report, "pass")).toHaveLength(1); + expect(findingsOf(report, "fail")).toHaveLength(1); + }); + + it("prints the pass-bucket finding under a failing rule", async () => { + await writeRuntimeRule(FLAGS_EVERY_MATCH); + + const { stdout } = await testRuntime(); + + expect(stdout).toContain(`✗ runtime/${RUNTIME_RULE}`); + expect(stdout).toContain("pass fixture findings:"); + expect(stdout).toContain(RUNTIME_MESSAGE); + }); + + it("prints one line and no findings when the rule passed", async () => { + await writeRuntimeRule(); + + const { stdout } = await testRuntime(); + + expect(stdout).toContain(`✓ runtime/${RUNTIME_RULE}`); + expect(stdout).not.toContain("fixture findings"); + }); + + it("carries an empty array, not an absent key, on a refused run", async () => { + // Without the flag nothing executes, so there is nothing to report — and + // that is the case a consumer must not have to tell apart from "this + // command does not report findings". + await writeRuntimeRule(); + + const { stdout } = await runCli([ + "test", + `.taskless/rules/runtime/${RUNTIME_RULE}`, + "-d", + cwd, + "--json", + ]); + const report = JSON.parse(stdout) as Report; + + expect(report.rules[0]?.refused).toBeDefined(); + expect(report.rules[0]?.findings).toEqual([]); + }); +}); + +/* -------------------------------------------------------------------------- */ +/* Always present */ +/* -------------------------------------------------------------------------- */ + +async function writeSgRule() { + const directory = join(cwd, ".taskless", "rules", "sg", "no-eval-sg"); + await mkdir(join(directory, ".tests"), { recursive: true }); + await writeFile( + join(directory, "no-eval-sg.yml"), + "id: no-eval-sg\nlanguage: TypeScript\nseverity: error\n" + + "message: no eval\nrule:\n pattern: eval($ARG)\n" + ); + await writeFile( + join(directory, ".tests", "no-eval-sg-test.yml"), + "id: no-eval-sg\nvalid:\n - const a = 1;\ninvalid:\n - eval(x);\n" + ); +} + +describe("the findings array is present on every rule result", () => { + it("is empty rather than absent for an ast-grep rule", async () => { + // `sg test` reports a count and nothing else, and its fixtures are inline + // YAML scalars rather than files. Empty is the true answer here, and it has + // to be stated rather than left to an absent key. + await writeSgRule(); + + const { stdout, exitCode } = await runCli([ + "test", + ".taskless/rules/sg/no-eval-sg", + "-d", + cwd, + "--json", + ]); + const report = JSON.parse(stdout) as Report; + + expect(exitCode).toBe(0); + expect(report.rules[0]?.ok).toBe(true); + expect(report.rules[0]?.findings).toEqual([]); + }); + + it("is empty rather than absent for verify, which runs no fixtures", async () => { + await writeSgRule(); + + const { stdout } = await runCli([ + "verify", + ".taskless/rules/sg/no-eval-sg", + "-d", + cwd, + "--json", + ]); + const report = JSON.parse(stdout) as Report; + + expect(report.rules[0]?.findings).toEqual([]); + }); + + it("is empty rather than absent when verification failed before fixtures ran", async () => { + // No `.vale.ini`, so `verify` fails and `test` never reaches the fixtures. + const directory = join(cwd, ".taskless", "rules", "vale", VALE_RULE); + await mkdir(directory, { recursive: true }); + await writeFile(join(directory, `${VALE_RULE}.yml`), SUBSTITUTION_STYLE); + + const { stdout, exitCode } = await testVale("--json"); + const report = JSON.parse(stdout) as Report; + + expect(exitCode).toBe(1); + expect(report.rules[0]?.ok).toBe(false); + expect(report.rules[0]?.ran).toBe(false); + expect(report.rules[0]?.findings).toEqual([]); + }); +}); diff --git a/packages/cli/test/vale-verify.test.ts b/packages/cli/test/vale-verify.test.ts index ec3c2f95..48cdff2f 100644 --- a/packages/cli/test/vale-verify.test.ts +++ b/packages/cli/test/vale-verify.test.ts @@ -247,6 +247,11 @@ withVale("verifyValeRule", () => { } ); const result = await verifyValeRule(cwd, "no-simply"); + // Exhaustive on purpose: a field added to the verification shows up here + // rather than sliding in unasserted. `findings` is spelled out per finding + // for the same reason — it is carried rather than reduced to the file paths + // in `missingFailures`/`unexpectedFindings`, and the rendered `message` is + // the field the array exists to carry. expect(result).toEqual({ ruleId: "no-simply", passed: true, @@ -254,6 +259,34 @@ withVale("verifyValeRule", () => { unexpectedFindings: [], fixtures: "both", notices: [], + findings: [ + { + source: "vale", + ruleId: "no-simply", + severity: "warning", + message: "Avoid 'simply'", + file: ".taskless/rules/vale/no-simply/.tests/fail/a.md", + range: { + start: { line: 0, column: 5 }, + end: { line: 0, column: 10 }, + }, + matchedText: "simply", + bucket: "fail", + }, + { + source: "vale", + ruleId: "no-simply", + severity: "warning", + message: "Avoid 'simply'", + file: ".taskless/rules/vale/no-simply/.tests/fail/b.md", + range: { + start: { line: 0, column: 8 }, + end: { line: 0, column: 13 }, + }, + matchedText: "simply", + bucket: "fail", + }, + ], }); });