feat(runtime): run a rule's fixtures, and stop reporting a pass when they did not - #252
Conversation
…s/fail Group 1 of the fixture runner: the reader, with nothing executing yet. A case is a DIRECTORY, and its path is the root the harness will be handed. That follows from `executeRuntimeRule(root, rule)` already taking a root rather than from a preference: a case directory is the same argument with a smaller tree behind it. Two guards carry the weight, both taken from the Vale reader because the mistakes they prevent are the same ones. A bucket that cannot be read is an error, never an empty bucket. Swallowing an `EACCES` on `pass/` would yield nothing while `fail/` still had cases, so a two-sided rule would look one-sided and could report a pass having never checked its pass side. A loose file is refused by name rather than skipped. The check is given a root and reads beneath it, so a bare file has no root to be; ignoring it would leave an author with a fixture they wrote, that never ran, and that nothing mentioned. Both proved rather than asserted: reverting each guard fails its own test and leaves the other six green.
A fixture case is a case DIRECTORY handed to `executeRuntimeRule` as its `root`, which is the same argument `check` already passes with the repository root. So the runner is a caller of the executor rather than a sibling of it: the loop and the pass/fail assertion are new, the execution is not. D8 is the part that needed a change in the harness. `executeRuntimeRule` gates on the narrow and returns `[]` without invoking `check.ts`, so a case whose narrow matches nothing is indistinguishable downstream from a check that ran and found nothing. Under the pass/fail rules that would fail a `fail/` case and blame the check for a fixture that never reached it, and would pass a `pass/` case that proves only that the narrow did not match. `executeRuntimeRuleDetailed` is therefore the single execution path, adding `invoked` and `failure` beside the findings, and `executeRuntimeRule` is a thin projection of it. One narrow, one invocation, one source of truth for "did it run", and the scan's call site is untouched. A case producing no narrow matches is reported as a fixture defect in BOTH buckets, naming the case and saying the check never ran. A check that throws is reported as the check failing, since both arrive as zero findings and only one is the rule's fault.
…ates `test` has to run a runtime rule's fixtures under the policy `check` already applies: an authenticated reconcile that returns the rule's signature in `run`, or `--dangerously-run-scripts`. Nothing about a fixture makes the code safer. The bytes do not know what directory they are pointed at, and "it is only running against test data" is a statement about the input rather than about what the program may do. `planRuntime` and its repair pass therefore move out of `commands/check.ts` and into `rules/runtime/plan.ts`, unchanged. The alternative was a second implementation of the gate in `test`, which is a bypass waiting to be discovered: faithful on the day it is written and drifting from then on. `createRuntimeGate` wraps it for a command that reports rule by rule. The plan is memoized, so a `test` over a tree of runtime rules asks the service once rather than once per rule, and the whole discovered set is reported to reconcile exactly as `check` reports it. It is lazy because most projects hold no runtime rules, and a `test` over `sg` rules must not reach the network. No rule id is exempt, and no path through the gate is softer than another.
…ilure
`testOneRule` returned `{ ok: true, errors: [], ran: false }` for every runtime
rule, and the renderer read only `ok`, so `test` printed `✓ runtime/<id>` and
"1 rule(s) tested" about a rule it had not tested. The one field that knew was
the one nothing read.
The runtime branch now takes the rule through the shared gate and, when
admitted, runs its fixtures. When the gate refuses, the result carries
`refused` with the reason, and three call sites read it: the renderer prints
`○` rather than a tick, the summary counts the rule under "did not run" rather
than among the rules tested, and the exit code ignores it.
`ok: false` alone would have been the wrong correction. A rule that cannot run
because nothing blessed it is not defective, and failing it turns `test` red
for every project holding a runtime rule with no action available that makes it
green. `ok` also cannot express the state on its own: a rule whose `verify`
failed is likewise `ok: false, ran: false` and must still fail.
`ran` becomes load-bearing in the `--json` envelope, `refused` carries the
reason beside it, and both are documented as the fields a caller branches on.
The refusal message names `--dangerously-run-scripts`, because for a locally
authored rule that is the author's only route: blessing is recording, and
nothing recorded a rule that has never left the working tree. The flag is added
to `test` with `check`'s description and `check`'s warning, on stderr, and one
gate is built per command run so a tree of runtime rules is planned once.
Written against the unfixed CLI first, where 15 of the 17 assertions failed and
the first one failed with exactly the output the proposal recorded:
expected '✓ runtime/no-eval\n\n1 rule(s) tested…' not to contain
'✓ runtime/no-eval'
The two that passed are the gate's, and they passed for the wrong reason: with
no runner at all, "no fixture case was executed" was trivially true.
The suite covers the defect (no tick, no count, `ran: false`, exit 0), the gate
(nothing executes unblessed, no id is exempt, the flag is the only other way
in), both fixture directions, both never-invoked cases, a check that throws,
every coverage class, and an unreadable or malformed bucket.
Two details in the fixtures are load-bearing rather than incidental. The rule
flags `eval` only on a non-literal argument, because a check that flagged every
match would have no `pass/` case that both matched the narrow and stayed quiet,
and a `pass/` case that does not match the narrow proves nothing. And `matching`
and `flagged` are separate knobs on a case, because the narrow and the check are
what D8 exists to keep apart.
Reverting the runner and the reporting fix, and rebuilding, fails 15 of 1205 tests in one file. Restoring returns 1205 passed. The revert was verified applied before the suite was run, by grepping for the absence of `run-fixtures.ts` and `executeRuntimeRuleDetailed` and for the return of the old renderer and the old `ok: true`. A revert that does not revert makes passing tests look like proof, which has happened here before.
`create-runtime-rule` now states that testing a rule you just wrote needs `--dangerously-run-scripts`, and why the obvious reading of the message is wrong: a locally authored rule has no signature and never will, because blessing is recording and nothing recorded a rule that has not left the working tree. The message says "not authenticated", which reads like logging in would fix it, so the recipe says plainly that it would not. It also states what a passing runtime rule needs: both buckets populated, and every case matching at least one capture. A case the captures do not match never reaches `check.ts` and is a defect in the case, in `pass/` as much as in `fail/`. `verify-rule` stopped describing runtime tests as "reported as not run" with nothing about what would run them. Its engine table now says what `test` runs for a runtime rule, and the reporting section says how a refused run appears: `○` rather than a tick, `ran: false` with `refused` beside it, counted under "did not run", and never on its own a reason for a non-zero exit.
|
@claude /review |
|
Claude finished @thecodedrift's task in 13m 4s —— View job ReviewReview mode: incremental — read 0 prior review thread(s) before reviewing. I read the full diff ( Overall the core defect fix is solid: [New] [New] Runtime fixture-bucket logic (coverage classification + directory reading) is a third parallel implementation of Vale's [New] Duplicate refusal predicate in [New] Fixture cases run strictly sequentially; the stated justification doesn't hold [New] Three-outcome result ( No blocking issues — the network-call and duplication findings above are the ones I'd want addressed or consciously accepted before merge; the rest are lower-severity cleanup/design notes. |
`createRuntimeGate` hardcoded `anonymous: false`, so `taskless test` called `getToken` and, for any project holding a runtime rule, went on to `resolveOrgSubject` and `reconcile` over the network. `test` was fully offline before this change. The fix is not `--anonymous`. It is to take reconcile out of `test` entirely. `check` reconciles because it executes rules as a SIDE EFFECT of scanning a repository: nobody asked for code to run, so a gate has to stand between the request and the execution. `test` runs fixtures because the user asked it to, and the verb is the consent — asking a server for permission to run your own fixtures is overreach. The only party a reconcile here could ever admit is someone testing an already-blessed delivered rule, which the service verified before delivering it; a locally authored rule has no signature and never will, so for the audience that actually runs `test` on a runtime rule it is a round trip whose answer is always "no". This is strictly more conservative. Nothing executes that would not have executed before, and a blessed rule that `check` runs unflagged now needs `--dangerously-run-scripts` here. It also disposes of the `--anonymous` question rather than answering it: with no network there is nothing to suppress. `check`'s gate, its reconcile, and its use of `planRuntime` are untouched. `createRuntimeGate`/`RuntimeGate` were `test`-only and are deleted. `planRuntime` and `repairWithheldRules` stay in `rules/runtime/plan.ts`, `check`-only: the move's original justification is gone, but policy still does not belong in a command file. The refusal message no longer inherits `check`'s "not authenticated", which read like a fix and was not one. Also extracts the fixture reading that had been copied a third time. Vale's `directoryEntries` claimed to be "the single place that decides which `readdir` failures are absence and which are problems, so no caller can accidentally answer that question differently", and the runtime copy made that false. The missing-vs-unreadable discrimination and the four-way coverage classification with its message now live in `rules/fixtures.ts`, and all three engines call it. Each engine's rejection stays local, because they are opposites: Vale refuses a nested DIRECTORY, runtime refuses a non-directory. The coverage message's `valid:` versus `pass/` looked like drift and is not — ast-grep's buckets are YAML keys and the other two are directories — so the suffix is a parameter rather than something normalised away. Behaviour is unchanged: 1205 tests before, 1206 after, the one addition being the new test that the refusal never says `auth login`.
|
Ref: review comment 5515932991 → 7c7bb8a Three of the five addressed; the other two consciously deferred, with reasons. 1. You were right about the defect and I went further than the suggested fix: reconcile is out of The reasoning, which is now in D1 rather than only here: Ask who a reconcile in This is strictly more conservative. Nothing executes that would not have executed before, and one thing that would have — a blessed rule running unflagged — now requires the flag. Orphan check: One thing your finding did not mention that fell out of it: the refusal message inherited 2. Third parallel bucket/coverage implementation — MEDIUM. Extracted.
Each engine's rejection stayed local, because they are opposites rather than variants: Vale refuses a nested directory (its buckets hold documents), runtime refuses a non-directory (its buckets hold one directory per case). Folding those together needed a flag that would have made the shared function harder to read than the two callers it replaced. One correction to the finding. The Left alone deliberately: Behaviour proof, since the extraction had to be inert: 1205 tests / 75 files before, 1206 / 75 after, the single addition being the new 3. Duplicate refusal predicate — LOW. Folded.
4. Sequential fixture cases — not taken. The analysis is right that nothing is shared between cases, so the stated justification does not hold as written. Not taken here because it is a performance change to a path with no measurement behind it, and it would land in the same diff as a security-gate change, where the two want different review attention. Worth its own change with a fixture count that shows the cost. 5. Three-outcome result as two booleans — not taken. Agreed on the shape, and agreed it is the argument D8 makes one level down. Not taken because — AI Coding Agent |
1 participant
Implements
runtime-fixture-runnertask groups 1 through 6. Group 7 (close-out, outbound to the generator team) is untouched.The defect
taskless testprinted✓ runtime/<id>and "1 rule(s) tested" at exit 0 for a rule whose fixtures never ran.testOneRulereturned{ ok: true, errors: [], ran: false }, and the renderer read onlyok. The comment above that return said "testreports that rather than quietly claiming a pass." The human path did exactly the quiet claim.Runtime was the only tier without a fixture runner:
sgrunsast-grep test,valeruns both buckets and asserts the asymmetry,runtimereturned a tick.What landed
readRuntimeFixtures— buckets read independently, entries must be directories, four-way coveragerun-fixtures.ts— cases through the existing executor, four defect classesplanRuntimemoved beside the engine it gates;testgains--dangerously-run-scripts○, excluded from "N tested", ignored by the exit codecreate-runtime-ruleandverify-rulesay what runs and what stops itD8: one execution path, not two
executeRuntimeRulegates on the narrow (if (matches.length === 0) return []), so a case whose narrow matches nothing was indistinguishable from a check that ran and found nothing — which would have failed afail/case and blamed the check for a fixture that never reached it.A literal added field was impossible, since the function returns an array with nowhere to put one. Instead
executeRuntimeRuleDetailedis now the single execution path returning{ findings, invoked, failure? }, andexecuteRuntimeRuleis a two-line projection of it.check's call site indispatch.tsis untouched and compiles unchanged. One narrow, one invocation, one source of truth.runNarrowis not duplicated.failurealso settles task 2.3: a narrow that throws isinvoked: false, failure; a check that throws isinvoked: true, failure. The runner checksfailurebeforeinvokedbefore the bucket assertion, so a crashed check infail/is never scored as the case firing correctly.Where implementation found the design wrong
ranalone cannot carry the state (D2). A rule whoseverifyfailed is alsook: false, ran: false, and that one must fail the command — while a refused run must not. Task 4.1 forbidsok: trueand 4.4 forbids failing, so the two states are indistinguishable fromokandrantogether. An explicitrefusedfield carries the reason and is the discriminator the renderer, the summary and the exit code all read. This is a--jsonenvelope addition the design did not anticipate and deserves a reviewer's eye.A
pass/case is harder to author than the design implies. D8 requires every case to match the narrow, so apass/case must match the capture and leave the check quiet — meaning a check that flags every match has no validpass/fixture at all. That went intocreate-runtime-rule.txtas guidance rather than staying a comment.Coverage is assessed only on the admitted path. A refused rule means the CLI never looked at its fixtures, rather than reporting "refused and also under-fixtured". Defensible; the design left it open.
Proving it bites (5.6)
Reverted
5a85ce8and4054f4e, and verified the revert applied before running anything —run-fixtures.tsabsent, zero occurrences ofexecuteRuntimeRuleDetailed, renderer back toresult.ok ? "✓" : "✗", runtime branch back took: true. A revert that does not revert has made passing tests look like proof in this repo before.The first failure reproduced the proposal's output exactly:
expected '✓ runtime/no-eval\n\n1 rule(s) tested…' not to contain '✓ runtime/no-eval'.Two of the 17 passed under the revert, and passed for the wrong reason: with no runner at all, the gate's "nothing was executed" is trivially true.
Checks
pnpm build,pnpm typecheck,pnpm lint, full suite (1205 passed),pnpm cli checkwith exactly the 4 pre-existingno-hedgingwarnings.