feat(test): report the findings a rule's fixtures produced - #396
Conversation
|
@claude /review |
|
Claude finished @thecodedrift's task in 4m 57s —— View job Review complete
Review mode: incremental — read 0 prior review thread(s) before reviewing. What I checkedI read the full diff ( Specifically verified, given this repo's own track record of subtle failures in exactly these spots:
I did not find a correctness bug, a removed/unre-established invariant, or a CLAUDE.md violation in the diff. No inline comments posted. |
Read in full, and no code change was made, because the review found nothing to change: its closing line is "I did not find a correctness bug, a removed/unre-established invariant, or a CLAUDE.md violation in the diff. No inline comments posted." It surfaced as Two invariants it verified are load-bearing and deliberate, recorded here so a later pass does not "tidy" them:
CI is green (Validate, Changeset, Demo rules, OpenSpec Label — 5 passed, 0 failed), and the gates were re-run on the pushed tree: — AI Coding Agent |
Restates "Test runs a rule's fixtures and runs verify first" in full: a MODIFIED block replaces the standing requirement, so all 8 existing scenarios and the full normative prose are carried verbatim or archive deletes them silently. Five of the eight are unrelated to this change and are present only to survive. Adds the findings requirement: a per-rule array carrying check's own finding shape plus the fixture bucket, always present and empty rather than absent, with the fail bucket reported on a green --json run, and the human render printing the bearing findings under a failing rule.
test --json now carries a findings array on every rule result: check's own CheckResult shape plus the bucket of the fixture that produced it. The rendered message is the point — a rule whose message interpolates its captures can have the slots reversed, fire on every fail/ fixture, stay quiet on every pass/ one, and be reported as a rule that passed. The findings were already in hand and were being discarded: Vale reduced its CheckResult[] to a Set of file paths, and the runtime runner read only findings.length. Nothing new is executed and no flag was added. The array is always present and empty rather than absent — a key that is sometimes absent is one a reader learns to treat as optional, and then reads absence as zero. Fail-bucket findings are reported on a passing run, since that is the only run that produces them. On the human path a passing rule still prints one line; a failing rule prints the bearing findings beneath it, labelled by bucket and rendered through util/format.ts's formatText, the same renderer check uses. Pass-bucket findings print there: "pass fixture wrongly fired: <file>" said that it happened and never what matched. ast-grep reports an empty array for now: sg test has no --json and no output-format flag, and its fixtures are inline YAML scalars rather than files.
This PR targets main, so it is the tip and the tip archives. The delivery shape in the proposal said "stacked, merging forward, bottom of two", which was never true of a branch based on main; corrected to a single PR with ast-grep as a separate future change rather than a later slice of this one. Checked before archiving that no part of the delta describes behaviour only the ast-grep half would deliver. Every ast-grep mention in the MODIFIED block is standing text already on main, and the new prose makes an empty array explicitly conformant for an engine that does not yet surface its fixture findings. Verified by title set rather than by count: cli-rule-validation 37 -> 40 scenarios, 7 -> 7 requirements, zero lost, and the result is identical to the pre-flight dry run.
240bfaa to
c87633d
Compare
test --jsonreported a boolean per rule and nothing else:{"ok":true,"rules":[{"engine":"vale","ruleId":"no-hedging","ok":true,"errors":[],"violations":[],"ran":true}]}Every rule result now carries a
findingsarray: theCheckResultshapecheck --jsonalready prints —source,ruleId,severity,message,file,range,matchedText, and the optionalnoteandfix— plus abucketof"pass"or"fail"naming the fixture that produced it.The rendered message is the point. A
substitutionrule whose message interpolates its captures can have the%sslots in the wrong order, fire on everyfail/fixture, stay quiet on everypass/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 secondcheckrun against a fixture path you had to construct yourself.What changed
schemas/verify-test.ts—findingsonruleResultSchema, defaulted to[]so the key is present in both commands' output.rules/vale/verify.ts— theCheckResult[]is kept besidefiredIninstead of being reduced to aSet<string>of file paths. Findings are bucketed by membership inpass/.rules/runtime/run-fixtures.ts—execution.findingsis collected per case rather than only its.length. A case counted incheckFailurescontributes nothing: a harness crash arrives as one synthesized error-severity finding, and reporting that as something the rule found is the confusioncheckFailuresexists to prevent.rules/inspect.ts—findingsonRuleTestResult, populated per engine and[]on every path that returns before fixtures run.commands/verify.ts— the human render.rules/fixtures.ts—FixtureFindingandFixtureBucket, shared.runtime/fixtures.ts's duplicateFixtureBucketis removed in favour of it;reference.ts's is left alone, being the corpus's own vocabulary.Nothing new is executed, no subprocess is added, and no flag was added. The findings were already in hand and were being discarded at a seam.
Decisions worth flagging to a reviewer
verify(which runs no fixtures at all), and ast-grep. A key that is sometimes absent is one a reader learns to treat as optional, and the reading that follows is that absence means zero. There is a test per case.--jsonrun. They are the evidence, and they are only ever produced by a run that passed.util/format.ts's existingformatText—check's own renderer, over theCheckResult[]that is already its parameter type. Pass-bucket findings print there:pass fixture wrongly fired: <file>said that it happened and never what matched.--verboseand no--include-fixtures.--verbosehas zero precedent acrosspackages/cli/srcandopenspec/specs; the house pattern is a rich--jsonbeside a terse human render. An explicit fixture path already works oncheck, because the.taskless/**exclusion inrules/vale/run.tsis gated onwholeProject. A flag an agent has to know to pass is the ergonomics problem being filed, not its fix.missingFailures/unexpectedFindingsare untouched. They are internal toValeRuleVerificationand decidepassed; whetherfindingssubsumes them is a separate call.violations[]is untouched, deliberately. Those are constraint violations — what a rule broke about its own shape. A fixture finding is what a rule reported about a document. Conflating the two is the confusion this change is correcting, so### Requirement: A rejection names the constraint it violatedis not modified.ast-grep is out of scope, as its own future change
runTestsspawnssg testand regex-parsestest result: ok. N passed; N failed;. The vendored 0.45.3 binary has no--jsonand 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. An ast-grep rule reportsfindings: []here, which is true rather than misleading — and the spec delta makes that explicitly conformant, requiring an empty array "for an engine that does not yet surface its fixture findings". So nothing archived by this PR describes behaviour the shipped code does not meet.This PR targets
mainand is not part of a stack. An earlier revision of this description called it the bottom of a two-PR stack; that was wrong. Spec, implementation and archive land together here, and ast-grep gets its own proposal when that work is written rather than being a later slice of this change.OpenSpec
### Requirement: Test runs a rule's fixtures and runs verify firstis restated in full under## MODIFIED Requirements. A MODIFIED block replaces the standing requirement, so anything left out is deleted from the spec on archive with nothing reporting it andvalidate --strictstill passing.Five of the eight restated scenarios have nothing to do with this change — malformed-rule ordering, ast-grep per bucket, runtime-without-flag, runtime-reaches-no-network, and check-that-throws. They are carried verbatim purely so they survive the archive. Please do not tidy them out.
Verified the way
CLAUDE.mdprescribes rather than trusted: first as a dry run against a throwaway SHA, then for real.cli-rule-validationwent 37 → 40 scenarios and 7 → 7 requirements, with zero prior scenarios lost and exactly the three new ones added — compared by scenario-title set rather than by count, so an addition cannot mask a drop. The real archive is identical to the dry run.The change is archived in this PR, since it targets
mainand the tip archives.Consumers of
@taskless/cli/schemaszod's.parse()strips keys the schema does not declare, so a consumer still on the previously publishedverifyTestOutputSchemawill silently dropfindingsfrom the object it hands back, on a CLI that is emitting it. Not a break — the package is pre-1.0 and this is additive, so the changeset ispatch— but it is the kind of thing that generates a bug report, and the changeset says so.Verification
pnpm build,pnpm typecheck,pnpm lint— all clean.pnpm test— 103 files, 1712 tests, all passing."Replace '%s' with '%s'"(the plausible wrong order) turns three assertions red, including the human-path one. It fails when the slots are wrong.vale-verify.test.tswas an exhaustivetoEqualon the whole verification object and needed the new field. It was extended, not relaxed: both findings are spelled out with their rendered messages and ranges, so the object stays exhaustively asserted.Refs #386