docs(openspec): propose the runtime fixture runner - #249
Conversation
|
@claude /review |
|
Claude finished @thecodedrift's task in 6m 23s —— View job Review mode: incremental — read 0 prior review thread(s) before reviewing
ScopeThis PR is proposal-only: Delta integrity — checked, passes. The Factual claims — checked, accurate.
Findings[New] Design gap: the fixture runner's interaction with the narrow-match gate is unspecified, posted inline on I did not find other correctness problems: no missing scenario, no internal contradiction between Not done
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The narrow-gate finding was bucketed low and is not: it would have shipped a spec mandating the exact ambiguity this change exists to remove. Fixed as D8 with tasks 2.4/2.5 and a scenario, not deferred. Two things worth naming about the review itself. Checking the And tracing the claims into — AI Coding Agent |
`taskless test` reports a runtime rule as passing when its fixtures never ran. `testOneRule` returns `ok: true, errors: [], ran: false`, and the renderer reads only `ok`, so the command prints a tick and "1 rule(s) tested" about a rule it did not test. This is an unimplemented requirement rather than a missing feature. `cli-rule-validation` already says `test` executes a rule against "ast-grep test cases, Vale pass/fail fixture buckets, or the runtime harness". Two of the three are implemented and have scenarios; the third has neither, and its absence reports as success. The decision the change turns on is what to do with a run that did not happen. Returning `ok: false` is the obvious correction and is also wrong: a rule that cannot run because nothing blessed it is not defective, and failing it would turn `test` red for every project holding a runtime rule with nothing available to make it green. So there is a third outcome, reported as itself. The gate is unchanged. A fixture run executes the same check.ts under the same signature as a scan, and that the input is test data is a statement about the input rather than about what the program may do. The Vale runner is the model, including its strictness: buckets read independently so an unreadable one cannot present as empty, coverage classified four ways so only `both` can pass, and a non-directory entry refused by name rather than skipped. Runtime differs in one way that matters, since a case is a directory and that directory is the harness root.
…t a sibling Settles the open question in 7.1: a runtime rule with no fixture cases fails, matching ast-grep and Vale. Making the tier that executes arbitrary code the one whose rules need not prove anything would be the wrong exception. Depth is not policed. A trivial case that exercises little and passes is fine, but the latitude reaches one bucket only: a `fail/` case producing no findings is the silent regression this runner exists to catch, so a case that tests nothing can only be a `pass/` case. The `fail/` case is the one that has to do real work, and the one worth having. Also corrects the change's own framing, which understated what already exists. Runtime rules execute today: `check` reaches `executeRuntimeRule` through `dispatch.ts:205` with the repository root, gated by signature planning. The gap is fixtures, and only in `test`. That makes D3 an observation rather than a decision. `executeRuntimeRule` already takes a root, so a case directory is a different root handed to the same function, and the runner is a loop plus an assertion over machinery `check` already proves on every authenticated run.
…not taken A rule with no fixtures proves nothing, which makes it incomplete by the same standard as a rule with no captures. Delivery already answers that question and answers it before writing, so a refused set leaves no directory behind. Catching it in `test` instead means the rule is already on disk and the failure reads as the holder's fault for a file the service sent. That is the styleguide's rule about build output applied to a payload: an invariant enforced where the artifact is produced cannot be violated, while one enforced afterwards can only be detected. It is not taken here, because it is a contract change. A runtime delivery with no `.tests/` is valid today, and the published spec names `ENGINE_LAYOUTS` as the completeness authority, so requiring fixtures refuses payloads that are correct under the agreed contract. If the service is not already sending them, every runtime delivery breaks with nothing written. So the halves land in order. `test` failing an authored rule is ours alone and ships with this change. Delivery completeness follows once the generator team has confirmed fixtures ride in `files[]` as a matter of course, as its own change.
`executeRuntimeRule` gates on the narrow and returns an empty array without invoking `check.ts` when nothing matches. So a fixture case whose narrow finds nothing in its small directory is indistinguishable downstream from a check that ran and found nothing, and the scenarios as drafted would fail a `fail/` case for it while blaming the check for a fixture that never reached it. That is this change's own failure mode reintroduced inside the fix, which is worth fixing in a proposal rather than after implementation. The runner therefore needs the invocation signal, not only the findings, and reports three outcomes per case. A case producing no narrow matches is a fixture defect in BOTH buckets: the `pass/` side is the quieter half, because such a case reads as a clean pass while proving only that the narrow did not match. It cannot show the check stays quiet, so counting it as evidence is the same empty-scan-reports-success shape one level down. Found by review.
8f7ff48 to
ff3551f
Compare
… proposal D7 framed requiring `.tests/` as a contract change needing the generator team's agreement. That is wrong. A runtime rule ships with a fixtures directory, and a delivery without one is a defect on their side rather than a payload shape we are asking them to adopt. They already hold the material, since their verification gate executes the generated check against failing and passing examples before accepting the rule, and they have already established that fixtures are files in the collection rather than a field beside it. What is sequenced is enforcement, and for a reason unrelated to agreement. Being right about whose bug it is does not stop `describeIncompleteSet` from turning that bug into a refused write for every user, with nothing written and nothing they can do. So: confirm deliveries carry them, file anything that does not as the defect it is, and enforce once the fix has shipped.
1 participant
Proposal only. No source changes; every path is under
openspec/.The defect
taskless testreports a runtime rule as passing when its fixtures never ran. Measured against a scaffolded project holding one runtime rule:testOneRulereturnsok: true, errors: [], ran: falsefor runtime, and the renderer atcommands/verify.tsismark = result.ok ? "✓" : "✗"— it never readsran. The comment directly above that return says "testreports that rather than quietly claiming a pass." The human path does exactly the quiet claim.This is an unimplemented requirement, not a missing feature.
cli-rule-validationalready saystestexecutes a rule against "ast-grep test cases, Valepass/failfixture buckets, or the runtime harness". Two of the three are implemented and have scenarios. The third has neither, and its absence reports as success.Why now
.tests/paths,PRESERVED_SUBTREESkeeps them through a purge,strayModulesexempts them at any depth. Every piece of support exists except the runner.testas a limit on what their side can assert.ast-grep testcalls an emptyinvalid:bucket a pass. The runtime tier does it one level up.The decision this turns on (D2)
ok: falseis the obvious correction and it is also wrong. A rule that cannot run because nothing blessed it is not defective, and failing it would turntestred for every project holding a runtime rule, with no action available to make it green. That trades a silent wrong answer for a loud useless one.So there are three outcomes: it ran and behaved, it ran and did not, or it did not run and here is why. The third gets its own marker, does not count toward "N rule(s) tested", and does not fail the command.
ranstops being advisory metadata and becomes what a caller branches on.The
sgprecedent (verify.ts:581, "skips are errors, never a pass") points the other way and that is consistent, not contradictory: there a skip means something went wrong, because the runner exists. Here the run is refused by a security policy working as designed. Same word, opposite correct handling — the distinction this codebase already got wrong once withunsafeversusmissing.The gate does not move (D1)
A fixture run executes the same
check.ts, from the same delivery, under the same signature as a scan. That the input is test data is a statement about the input, not about what the program may do. Blessed-by-reconcile or--dangerously-run-scripts, no id exempt.Consequence, stated rather than discovered (D6): a locally authored rule has no signature and never will, so its author needs the flag to test their own rule. That is the correct friction — a delivered rule and an authored one are indistinguishable on disk.
Modelled on the Vale runner
rules/vale/verify.tsalready carries the lessons this tier would otherwise relearn, and the design cites them rather than re-deriving: buckets read independently so a swallowedEACCEScannot present as "no fixtures were written"; coverage classified four ways becausefail-onlyis more misleading thannone; a non-conforming entry refused by name rather than skipped.Runtime differs in one way that matters (D3), and it is an observation rather than a choice:
executeRuntimeRule(root, rule, options)already takes a root, andcheckalready passes the repository root throughdispatch.ts:205. A fixture case directory is simply a different root handed to the same function.What already exists, corrected
An earlier draft of this description understated the reuse. Runtime rules execute today —
checkruns them against your repository through the harness, gated by signature planning, and the process spawn, timeout, narrowing, capture discovery and result shape are all proven there on every authenticated run.The gap is fixtures, and only in
test.testOneRulenever calls any of it. So the runner is a caller of the existing executor, not a sibling of it: a loop over case directories plus the pass/fail assertion.A rule needs a fixture (was 7.1, now settled)
A runtime rule holding no fixture cases fails, matching ast-grep and Vale. Making the tier that executes arbitrary code the one whose rules need not prove anything would be the wrong exception.
Depth is not policed: a trivial case that exercises little and passes is acceptable. But that latitude reaches one bucket only. A
fail/case producing no findings is the silent regression this runner exists to catch, so a case that tests nothing can only be apass/case, and thefail/case has to do real work. That is also the one worth having, since it proves the rule fires at all.The cost is named rather than hidden: runtime rules have not been carrying fixtures, because nothing ran them, so this fails rules that were correct under the old behaviour. Same trade this CLI already made when
check,verifyandteststopped migrating silently.Checks
openspec validate runtime-fixture-runner --strictpasses. TheMODIFIEDdelta was built from the live requirement programmatically rather than transcribed, and verified by archiving on a scratch commit: all three existing scenarios survive alongside the three added.