diff --git a/openspec/changes/runtime-fixture-runner/design.md b/openspec/changes/runtime-fixture-runner/design.md index 3e6ffa2d..9f911605 100644 --- a/openspec/changes/runtime-fixture-runner/design.md +++ b/openspec/changes/runtime-fixture-runner/design.md @@ -26,24 +26,63 @@ Coverage is a four-way classification rather than a boolean, because **Goals.** Run a runtime rule's fixtures. Report honestly when they did not run. Make `.tests/` mean something for the one tier that ships them and reads none. -**Non-Goals.** Changing the execution gate. Adding a runtime-specific bypass. -Making `check` run fixtures — `check` scans a repository, `test` runs fixtures, -and that separation is why the gate can be shared without the commands merging. +**Non-Goals.** Loosening the execution gate anywhere. Adding a runtime-specific +bypass. Touching `check`'s behaviour at all — its gate, its reconcile and its +use of `planRuntime` are exactly what they were. Making `check` run fixtures: +`check` scans a repository, `test` runs fixtures, and that separation is why +the two can hold the same flag without holding the same policy (D1). ## Decisions -### D1 — The gate is `check`'s gate, unchanged - -A fixture run executes `check.ts`. That is the same code, from the same -delivery, with the same signature, as the code `check` runs against a -repository. So it runs under the same policy: blessed by an authenticated -reconcile, or `--dangerously-run-scripts`. +### D1 — `test` takes the flag half of `check`'s gate, and no reconcile at all 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, not about what the program may do. A separate, -softer gate for fixtures would be the client-side bypass the runtime spec -already forbids, arrived at by a different route. +statement about the input, not about what the program may do. So a fixture run +is gated, and `--dangerously-run-scripts` is that gate — the same flag `check` +carries, spelled the same way, printing the same warning. + +**What `test` does not do is reconcile.** The first draft of this change shared +`check`'s whole gate, which meant `test` called `getToken`, and on a project +holding any runtime rule went on to `resolveOrgSubject` and `reconcile` over +the network. `test` had been fully offline before this change, and that is the +half that was wrong. + +The two commands execute rules for different reasons, and the gate belongs to +the reason rather than to the code: + +- `check` executes rules as a **side effect** of scanning a repository. Nobody + asked for code to run; they asked for a report. The gate is what stands + between the request and the execution, so that code never runs silently. +- `test` runs fixtures **because the user asked it to**. The verb is the + consent. Asking a server for permission to run your own fixtures is + overreach, and it is not the kind of overreach that buys safety. + +Ask who a reconcile in `test` would ever admit, and the answer is: someone +testing an already-blessed delivered rule — a rule the service verified against +failing and passing examples on its way to accepting it. That is the one +audience for whom running the fixtures locally proves least. Meanwhile a +locally authored rule has no signature and never will, because blessing is +recording and nothing recorded it (D6), so for the audience that actually runs +`test` on a runtime rule the reconcile is pure cost paid for an answer that is +always "no". + +**This is strictly more conservative, not a softening.** Nothing executes under +`test` that would not have executed before, and one thing that would have — +a blessed delivered rule, running with no flag — now requires the flag. There +is no security argument against it, because there is no case where the new +behaviour runs code the old behaviour refused. + +It also disposes of the `--anonymous` question rather than answering it. The +review asked whether `test` should carry `check`'s `--anonymous` flag to +suppress the network call. With no network there is nothing to suppress, and a +flag whose only job is to turn off a call that should not have been there is a +worse answer than not making the call. + +The one property that had to survive is that the fixture path is not a _softer_ +gate than the scan path, since that would be the client-side bypass the runtime +spec forbids reached by a different route. It survives by being a _stricter_ +one. ### D2 — A run that did not happen is a third state, not a pass and not a failure @@ -111,13 +150,22 @@ one-sided rule look complete. The same applies here and is worth stating rather than inheriting by imitation: a fixture bucket that could not be read is an error, never an empty bucket. -### D6 — Authoring a runtime rule locally now requires the flag +### D6 — Testing a runtime rule requires the flag, authored or delivered -A locally authored rule has no signature and never will, because blessing is -recording and nothing recorded it. So its author must pass +Under D1 the flag is the whole gate for `test`, so this is now true of every +runtime rule rather than only of unblessed ones. The authored case is still the +one worth stating, because it is the one where a reader will look for a way +out and find none: a locally authored rule has no signature and never will, +because blessing is recording and nothing recorded it. Its author must pass `--dangerously-run-scripts` to test their own rule, which `sg` and `vale` authors do not have to do. +**The message must not send them to `auth login`.** The first draft inherited +`check`'s "not authenticated" reason, which reads like the fix and is not one: +authenticating would not have blessed a rule that never left the working tree. +With reconcile gone the message names the flag and nothing else, and a test +asserts the absence. + That is friction and it is the correct friction. The alternative is a rule that executes unblessed code because it happens to live in the working tree, which is exactly the property the gate exists to deny — a delivered rule and an authored @@ -195,6 +243,47 @@ either bucket, naming the case and saying the check never ran. It is actionable in a way "expected a finding, got none" is not, and it points at the file the author has to change. +### D9 — The third fixture reader is the one that had to be shared + +The Vale runner is the model this change copied (see Context), and copying it +verbatim is what made the copying a problem. `rules/vale/verify.ts` carried a +`directoryEntries` whose doc comment called itself "the single place that +decides which `readdir` failures are absence and which are problems, so no +caller can accidentally answer that question differently" — a claim a third +near-identical copy falsified the day it was written. + +The drift was already measurable rather than hypothetical: the "half a claim" +coverage message existed in three places. + +So `rules/fixtures.ts` now holds the two decisions that are genuinely the same +across `sg`, `vale` and `runtime` — the missing-versus-unreadable +discrimination, and the four-way coverage classification with its message — and +all three engines call it. + +**What stayed local is what is genuinely different, and it is the rejection.** +Vale's buckets hold documents, so a nested DIRECTORY is its error; runtime's +hold one directory per case, so a loose FILE is. Those are opposite rules for +the same-shaped read, and folding them together would have needed a flag that +made the shared function harder to read than the two callers it replaced. + +One thing looked like drift and was not. The coverage message spells the bucket +`valid:` for `sg` and `pass/` for the other two. That is not two punctuations +for one idea: ast-grep's buckets are keys in a test YAML document and the other +two are directories, so each names the bucket in the shape its author will go +looking for. It is a parameter of the shared message rather than something +normalised away. + +The classification is parameterised on the bucket names for the same reason +(`FixtureCoverage<"valid" | "invalid">` against `FixtureCoverage<"pass" | +"fail">`), so the three engines share one set of four states under their own +vocabularies rather than sharing a vocabulary they do not have. + +`sg`'s own test-file enumeration was left alone deliberately. It catches every +`readdir` failure as "no test files", which is the leniency this module exists +to prevent — but fixing it changes what `sg` reports on an unreadable `.tests/`, +and that is a behaviour change wearing a refactor's clothes. It belongs in its +own change, with its own test. + ## Risks / Trade-offs **A rule with no fixtures fails, and that is the decision.** The other two diff --git a/openspec/changes/runtime-fixture-runner/specs/cli-rule-validation/spec.md b/openspec/changes/runtime-fixture-runner/specs/cli-rule-validation/spec.md index 1e478376..33afe2fc 100644 --- a/openspec/changes/runtime-fixture-runner/specs/cli-rule-validation/spec.md +++ b/openspec/changes/runtime-fixture-runner/specs/cli-rule-validation/spec.md @@ -8,7 +8,11 @@ Ordering is the point. When a rule is both malformed and under-fixtured, the fix 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 delivered code, so they SHALL run only under the policy `check` already applies: an authenticated reconcile that returns the rule's signature in `run`, or `--dangerously-run-scripts`. A run refused by that policy 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. +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. #### Scenario: A malformed rule reports the malformation, not the fixtures @@ -33,20 +37,28 @@ A runtime rule's fixtures execute delivered code, so they SHALL run only under t #### Scenario: Runtime fixtures are run per case -- **WHEN** `test` runs against a runtime rule and the execution policy permits it +- **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 the policy refuses is reported as not run +#### Scenario: A runtime rule without the flag is reported as not run -- **WHEN** `test` runs against a runtime rule with no blessed signature and no `--dangerously-run-scripts` +- **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 diff --git a/openspec/changes/runtime-fixture-runner/specs/cli-runtime-rule-execution/spec.md b/openspec/changes/runtime-fixture-runner/specs/cli-runtime-rule-execution/spec.md index 57af3e8b..cb4f837f 100644 --- a/openspec/changes/runtime-fixture-runner/specs/cli-runtime-rule-execution/spec.md +++ b/openspec/changes/runtime-fixture-runner/specs/cli-runtime-rule-execution/spec.md @@ -37,26 +37,41 @@ one-sided rule look complete. - **THEN** the CLI SHALL report the failure - **AND** SHALL NOT treat the bucket as holding no cases -### Requirement: Fixture execution obeys the runtime execution gate - -Executing a fixture case SHALL be permitted only where executing the rule itself -would be: when an authenticated reconcile returns the rule's signature in `run`, -or when `--dangerously-run-scripts` is passed. No rule identifier SHALL be -exempt, and the fixture path SHALL NOT constitute a separate or softer gate. - -**Rationale.** 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. A softer gate for -fixtures would be the client-side bypass this capability already forbids, -reached by another route. - -#### Scenario: Fixtures do not run for an unblessed rule - -- **WHEN** `test` runs against a runtime rule whose signature no reconcile has blessed, without the escape flag +### Requirement: Fixture execution is gated on the escape flag alone + +Executing a fixture case SHALL require `--dangerously-run-scripts`, and SHALL be +permitted under no other mechanism. No rule identifier SHALL be exempt, and a +blessed signature SHALL NOT substitute for the flag. Deciding this SHALL NOT +involve the rule service: no token SHALL be read, no organization resolved, and +no reconcile performed. + +**Rationale.** That the input is test data is a statement about the input, not +about what the program may do, so the fixture path SHALL NOT be a softer gate +than the scan path. It is a stricter one, and for a reason that is about consent +rather than about the bytes. A scan executes rules as a **side effect** of a +request for a report, so a gate has to stand between the two or code runs +silently; that gate is what a reconcile serves. Running fixtures is the request +itself, so the verb is the consent and the flag is the confirmation. + +Reconciling here would also buy nothing. The only rule it could ever admit is +one the service already verified before delivering it, while a locally authored +rule has no signature and never will — so for the audience that runs fixtures it +is a network round trip whose answer is always "no". + +#### Scenario: Fixtures do not run without the flag + +- **WHEN** `test` runs against a runtime rule without `--dangerously-run-scripts` - **THEN** no fixture case SHALL be executed +- **AND** this SHALL hold irrespective of authentication state or any signature the rule carries #### Scenario: The documented escape runs fixtures - **WHEN** `test` runs with `--dangerously-run-scripts` - **THEN** fixture cases SHALL execute under that flag's existing warning - **AND** under no other mechanism + +#### Scenario: A scan's gate is unaffected + +- **WHEN** `check` scans a repository holding a runtime rule +- **THEN** it SHALL apply its existing policy unchanged: a signature returned in `run` by an authenticated reconcile, or `--dangerously-run-scripts` +- **AND** the fixture runner SHALL NOT alter what a scan executes diff --git a/openspec/changes/runtime-fixture-runner/tasks.md b/openspec/changes/runtime-fixture-runner/tasks.md index 1d519425..5c7d4ff5 100644 --- a/openspec/changes/runtime-fixture-runner/tasks.md +++ b/openspec/changes/runtime-fixture-runner/tasks.md @@ -7,51 +7,60 @@ permanently unverified. ## 1. Read the fixtures -- [ ] 1.1 Enumerate `.tests/pass/` and `.tests/fail/` one level deep, requiring each entry to be a directory (D3). A loose file names its own path in the error rather than being skipped -- [ ] 1.2 Read the two buckets independently and rethrow anything that is not a missing directory, so an unreadable bucket cannot present as an empty one (D5) -- [ ] 1.3 Classify coverage as `both` / `pass-only` / `fail-only` / `none`, and let only `both` reach a pass (D4) +- [x] 1.1 Enumerate `.tests/pass/` and `.tests/fail/` one level deep, requiring each entry to be a directory (D3). A loose file names its own path in the error rather than being skipped +- [x] 1.2 Read the two buckets independently and rethrow anything that is not a missing directory, so an unreadable bucket cannot present as an empty one (D5) +- [x] 1.3 Classify coverage as `both` / `pass-only` / `fail-only` / `none`, and let only `both` reach a pass (D4) ## 2. Run them -- [ ] 2.1 Execute the rule's `check.ts` once per case, with the case directory as the harness `root` (D3), through the existing `invoke.ts` rather than a second invocation path -- [ ] 2.2 Require every `fail/` case to produce at least one finding, and every `pass/` case to produce none. Name the cases that broke either direction, the way the Vale runner names `missingFailures` and `unexpectedFindings` -- [ ] 2.3 Treat a check that throws as a distinct outcome from a check that returned no findings. Both are zero findings downstream, and only one of them is the rule's fault -- [ ] 2.4 Distinguish a case that never reached the check from one where the check found nothing (D8). `executeRuntimeRule` gates on the narrow and returns `[]` without invoking `check.ts`, so the runner needs the invocation signal rather than only the findings -- [ ] 2.5 Report a case producing no narrow matches as a fixture defect in BOTH buckets, naming the case and saying the check never ran. A `pass/` case that never invokes the check proves nothing about the check staying quiet +- [x] 2.1 Execute the rule's `check.ts` once per case, with the case directory as the harness `root` (D3), through the existing `invoke.ts` rather than a second invocation path +- [x] 2.2 Require every `fail/` case to produce at least one finding, and every `pass/` case to produce none. Name the cases that broke either direction, the way the Vale runner names `missingFailures` and `unexpectedFindings` +- [x] 2.3 Treat a check that throws as a distinct outcome from a check that returned no findings. Both are zero findings downstream, and only one of them is the rule's fault +- [x] 2.4 Distinguish a case that never reached the check from one where the check found nothing (D8). `executeRuntimeRule` gates on the narrow and returns `[]` without invoking `check.ts`, so the runner needs the invocation signal rather than only the findings +- [x] 2.5 Report a case producing no narrow matches as a fixture defect in BOTH buckets, naming the case and saying the check never ran. A `pass/` case that never invokes the check proves nothing about the check staying quiet -## 3. Gate it exactly as `check` does +## 3. Gate it on the flag, and only the flag -- [ ] 3.1 Run fixtures only when an authenticated reconcile returns the rule's signature in `run`, or when `--dangerously-run-scripts` is passed (D1) -- [ ] 3.2 Add `--dangerously-run-scripts` to `test`, printing the same warning `check` prints -- [ ] 3.3 Test that no rule id is special-cased, and that a gated-out fixture run executes nothing -- [ ] 3.4 Test that the flag is the only mechanism besides a blessed signature. A fixture path is not a softer gate because its input is test data +- [x] 3.1 Run fixtures only when `--dangerously-run-scripts` is passed. `test` does NOT reconcile: it runs fixtures because the user asked it to, so the verb is the consent (D1). `check`'s gate is untouched +- [x] 3.2 Add `--dangerously-run-scripts` to `test`, printing the same warning `check` prints, from one shared spelling so the two cannot drift +- [x] 3.3 Test that no rule id is special-cased, and that a gated-out fixture run executes nothing +- [x] 3.4 Test that the flag is the only mechanism at all. A fixture path is not a softer gate because its input is test data — and with no reconcile it is a stricter one, since a blessed rule that `check` runs unflagged still needs the flag here +- [x] 3.5 Test that the refusal never sends the author to `auth login`. Authenticating cannot bless a rule that never left the working tree, so naming it would be a fix that is not one (D6) +- [x] 3.6 Confirm nothing was orphaned by dropping reconcile from `test`. `createRuntimeGate`/`RuntimeGate` were `test`-only and are deleted; `planRuntime` and `repairWithheldRules` stay, `check`-only, in `rules/runtime/plan.ts` ## 4. Report a run that did not happen -- [ ] 4.1 Stop returning `ok: true` from `testOneRule`'s runtime branch for a run that did not occur -- [ ] 4.2 Render a skipped rule with its own marker and the reason, never a tick (D2) -- [ ] 4.3 Stop counting a skipped rule in "N rule(s) tested" -- [ ] 4.4 Make `ran` load-bearing in the `--json` envelope rather than advisory, and confirm the exit code does not fail on a gated-out run alone -- [ ] 4.5 Say what to do about it: a rule that did not run because nothing blessed it should name `--dangerously-run-scripts` in the message (D6) +- [x] 4.1 Stop returning `ok: true` from `testOneRule`'s runtime branch for a run that did not occur +- [x] 4.2 Render a skipped rule with its own marker and the reason, never a tick (D2) +- [x] 4.3 Stop counting a skipped rule in "N rule(s) tested" +- [x] 4.4 Make `ran` load-bearing in the `--json` envelope rather than advisory, and confirm the exit code does not fail on a gated-out run alone +- [x] 4.5 Say what to do about it: a rule that did not run because nothing blessed it should name `--dangerously-run-scripts` in the message (D6) ## 5. Prove it bites -- [ ] 5.1 Reproduce the current defect first, as a failing test: a runtime rule whose fixtures never ran must not report `ok: true` and must not print a tick -- [ ] 5.2 Test a rule whose `fail/` case produces no findings, which is the silent regression this exists to catch -- [ ] 5.3 Test a rule whose `pass/` case produces findings, which is the indiscriminate rule -- [ ] 5.4 Test each coverage class, asserting only `both` can pass -- [ ] 5.5 Test that an unreadable bucket is an error and not an empty bucket (D5) -- [ ] 5.6 Revert the runner and watch the suite fail before believing it +- [x] 5.1 Reproduce the current defect first, as a failing test: a runtime rule whose fixtures never ran must not report `ok: true` and must not print a tick +- [x] 5.2 Test a rule whose `fail/` case produces no findings, which is the silent regression this exists to catch +- [x] 5.3 Test a rule whose `pass/` case produces findings, which is the indiscriminate rule +- [x] 5.4 Test each coverage class, asserting only `both` can pass +- [x] 5.5 Test that an unreadable bucket is an error and not an empty bucket (D5) +- [x] 5.6 Revert the runner and watch the suite fail before believing it. **Measured:** reverting the runner and the reporting fix (`git revert --no-commit` of both commits, confirmed applied by grep before running: no `run-fixtures.ts`, no `executeRuntimeRuleDetailed`, renderer back to `result.ok ? "✓" : "✗"`, runtime branch back to `ok: true`) and rebuilding gives **15 failed / 1190 passed of 1205**, one test file failing. Restored: **1205 passed** ## 6. Say so where an author reads it -- [ ] 6.1 `create-runtime-rule` states that testing a locally authored rule needs the flag, and why (D6) -- [ ] 6.2 `verify-rule` stops reporting runtime tests as "not run" without saying what was not run +- [x] 6.1 `create-runtime-rule` states that testing a locally authored rule needs the flag, and why (D6) +- [x] 6.2 `verify-rule` stops reporting runtime tests as "not run" without saying what was not run -## 7. Close out +## 7. Share the bucket reader rather than copying it a third time -- [x] 7.1 Decide the no-fixtures case. **Settled: it fails, matching the other engines.** Every rule needs a fixture. Depth is not policed, so a trivial case that exercises little and passes is acceptable, but it can only live in `pass/` — a `fail/` case that produces nothing is the silent regression this runner exists to catch -- [ ] 7.2 Tell the generator team, who have recorded this as a limit on what the demonstration can assert -- [ ] 7.3 Confirm runtime deliveries carry `.tests/`. Not a request: a runtime rule ships with fixtures, and a delivery without them is a defect on their side (D7). If any do not, file it as a bug rather than proposing it as a contract change -- [ ] 7.4 Once 7.3 confirms deliveries carry them, add fixtures to delivery completeness as its own change. Sequenced only so a service defect does not reach users as a refused write with nothing they can do about it -- [ ] 7.5 Archive the change +- [x] 7.1 Extract the missing-versus-unreadable `readdir` discrimination and the four-way coverage classification to `rules/fixtures.ts`, and point `sg`, `vale` and `runtime` at it (D9) +- [x] 7.2 Extract the "half a claim" coverage message, which existed in three places. Keep the bucket suffix a parameter: `valid:` is a YAML key and `pass/` is a directory, so the difference is meaning rather than drift +- [x] 7.3 Keep each engine's rejection local. Vale refuses a nested DIRECTORY because its buckets hold documents; runtime refuses a non-directory because its buckets hold case directories +- [x] 7.4 Change no engine's observable behaviour. **Measured:** 75 files / 1205 tests before, 75 files / 1206 after, the one addition being 3.5's new test. `pnpm cli check` reports the same 4 pre-existing warnings either side + +## 8. Close out + +- [x] 8.1 Decide the no-fixtures case. **Settled: it fails, matching the other engines.** Every rule needs a fixture. Depth is not policed, so a trivial case that exercises little and passes is acceptable, but it can only live in `pass/` — a `fail/` case that produces nothing is the silent regression this runner exists to catch +- [ ] 8.2 Tell the generator team, who have recorded this as a limit on what the demonstration can assert +- [ ] 8.3 Confirm runtime deliveries carry `.tests/`. Not a request: a runtime rule ships with fixtures, and a delivery without them is a defect on their side (D7). If any do not, file it as a bug rather than proposing it as a contract change +- [ ] 8.4 Once 8.3 confirms deliveries carry them, add fixtures to delivery completeness as its own change. Sequenced only so a service defect does not reach users as a refused write with nothing they can do about it +- [ ] 8.5 Archive the change diff --git a/packages/cli/src/agent/create-runtime-rule.txt b/packages/cli/src/agent/create-runtime-rule.txt index c0b66ccd..3a485a82 100644 --- a/packages/cli/src/agent/create-runtime-rule.txt +++ b/packages/cli/src/agent/create-runtime-rule.txt @@ -94,6 +94,47 @@ runtime rule it finds is listed as skipped with the reason So a runtime rule you write now is inert until the user logs in. Say that plainly rather than letting them discover it from a silent check. +## Testing a rule you wrote yourself needs the flag + +`%(TASKLESS_CLI)s test` runs a runtime rule's fixtures by executing its +`check.ts` against each case directory. The flag is the whole gate, for +every rule, always: + +``` +%(TASKLESS_CLI)s test .taskless/rules/runtime/ --dangerously-run-scripts +``` + +**`test` does not talk to the rule service at all**, and there is no +blessed-signature path that runs fixtures without the flag. That makes +it stricter than `check`, which will execute a blessed rule with no flag +because it runs rules as a side effect of scanning a repository. `test` +runs them because you asked it to, so the verb is the consent and the +flag is the confirmation; nothing is asked of a server in between. + +**A locally authored rule has no signature and never will**, so this was +never going to be avoidable for a rule you wrote. Blessing is recording: +a signature is blessed because a reconcile reported it and the service +recorded it, and nothing recorded a rule that has not left the working +tree. Do not send anyone to `auth login` over this. It changes nothing +here, and the message does not mention it. + +The friction is the correct friction. `sg` and `vale` authors do not +face it because their rules are unexecuted. A delivered rule and one you +wrote a minute ago are indistinguishable on disk, so an exemption for +"my own rule" would be an exemption for any rule, and the warning the +flag prints is accurate in both cases. + +Without the flag `test` prints `○` for the rule, says the fixtures did +not run, names the flag, and exits 0. That is neither a pass nor a +failure, and it is deliberately not a failure: nothing the holder of an +unblessed rule can do would make a failure green. + +Both buckets have to be populated for the rule to pass, and every case +has to match at least one capture. A case the captures do not match +never reaches `check.ts`, and is reported as a defect in the case rather +than in the rule, in `pass/` as well as `fail/`: such a case proves the +capture did not match, which is a fact about the fixture. + ## Steps 1. **Tell the user what their rule needs, and why it is gated.** Name @@ -121,11 +162,13 @@ that plainly rather than letting them discover it from a silent check. ## Important Notes -- `--dangerously-run-scripts` makes `check` execute runtime rules - without server verification. It exists for local iteration on a rule - you wrote yourself and just read. It is not a way to ship a rule to a - team, and suggesting it to work around a login turns a deliberate gate - into an unreviewed code-execution path on someone else's machine. +- `--dangerously-run-scripts` makes `check` and `test` execute runtime + rules without server verification. It exists for local iteration on a + rule you wrote yourself and just read, which includes running its + fixtures. On `test` it is the only mechanism there is. It is not a way + to ship a rule to a team, and suggesting it to work around a login + turns a deliberate gate into an unreviewed code-execution path on + someone else's machine. - Do not author a `check.ts` and leave it in the repository unmentioned. A skipped runtime rule reports nothing, which reads exactly like a passing one. diff --git a/packages/cli/src/agent/verify-rule.txt b/packages/cli/src/agent/verify-rule.txt index b9d511ce..c43540cc 100644 --- a/packages/cli/src/agent/verify-rule.txt +++ b/packages/cli/src/agent/verify-rule.txt @@ -35,7 +35,19 @@ What each engine is checked for: |-----------|--------------------------------------------------------------------------------------------------------------------------------------|------------------------------------------------------------| | `sg` | ast-grep schema, plus `id`/`language`/`severity`/`message`/`rule`, and `regex` accompanied by `kind` | the `valid`/`invalid` cases in `.tests/` | | `vale` | style parses, `extends` and `message` present, `level` in vocabulary, and the rule's `.vale.ini` enables `.` under a matcher | the `.tests/pass/` and `.tests/fail/` buckets | -| `runtime` | `check.ts` present, at least one capture rule in `captures/` | reported as not run: the `.tests/pass/` and `.tests/fail/` cases need the server harness | +| `runtime` | `check.ts` present, at least one capture rule in `captures/` | each directory under `.tests/pass/` and `.tests/fail/`, as the check's `root` | + +A runtime rule's fixtures execute its `check.ts`, so they run only under +the policy `check` already applies: an authenticated reconcile that +returns the rule's signature, or `--dangerously-run-scripts`. When +neither holds, that rule prints `○` rather than a tick, names the reason +and the flag, and is reported as `"ran": false` with a `refused` string +saying why. It is counted under "did not run" rather than among the +rules tested, and on its own it does not fail the command. + +Read `ran` before `ok`. A runtime rule that did not run is not a rule +that passed, and treating the two alike is the defect this reporting +exists to prevent. ## What a path means @@ -70,10 +82,12 @@ Both commands answer in the same shape, one entry per rule: - `ok` at the top is true only when every rule passed. - `errors` is one string per problem, written to be read by a person. - `test` adds `ran`, which is false when `verify` failed and the tests - never executed. + never executed, and false for a runtime rule the execution policy + refused. The second case also carries `refused` with the reason, and + is the one where `ok` alone would mislead you. -Without `--json` the same information prints as a `✓`/`✗` line per rule -with its errors indented beneath. +Without `--json` the same information prints as a `✓`/`✗`/`○` line per +rule with its errors indented beneath. `○` is a rule that did not run. ## Exit codes diff --git a/packages/cli/src/commands/check.ts b/packages/cli/src/commands/check.ts index 97fd3fd1..d84e35fe 100644 --- a/packages/cli/src/commands/check.ts +++ b/packages/cli/src/commands/check.ts @@ -11,27 +11,12 @@ import { getTelemetry } from "../telemetry"; import { outputSchema as checkOutputSchema } from "../schemas/check"; import { makeErrorEnvelope, writeJsonError } from "../types/errors"; import { CLIError } from "../util/cli-error"; -import { getToken } from "../auth/token"; -import { resolveOrgSubject } from "../auth/org"; -import { resolveRepositoryUrl } from "../util/git-remote"; -import { getCliPrefix } from "../util/package-manager"; import { requireCurrentSchema } from "../filesystem/migrate"; -import { reconcile } from "../api/reconcile"; -import type { ReconcileResponse } from "../api/reconcile"; -import { restoreRule } from "../api/restore"; -import { repairTargets, verifyRestoredCheck } from "../rules/runtime/repair"; -import { writeRuleFile } from "../rules/files"; -import { PurgeIncompleteError } from "../rules/deliver"; -import { - discoverRuntimeRules, - type RuntimeRule, -} from "../rules/runtime/discover"; -import { - materializeRuntimeRules, - reportRuntimeChecks, - selectBlessedRuntimeRules, - signRuntimeChecks, -} from "../rules/runtime/run-set"; +import { discoverRuntimeRules } from "../rules/runtime/discover"; +// The gate lives beside the runtime engine rather than inside this command, +// because `test` runs a rule's fixtures under exactly this policy. Sharing the +// implementation is what makes that a fact rather than an intention. +import { planRuntime } from "../rules/runtime/plan"; async function pathExists(absolutePath: string): Promise { try { @@ -82,278 +67,6 @@ function extractPositionalPaths(rawArguments: string[]): string[] { return splitRawArguments(rawArguments, ["--timeout"]).positionals; } -/** A runtime rule that will not run, with why (advisory). */ -interface SkippedRuntimeRule { - rule: string; - reason: string; -} - -/** The runtime-execution plan resolved from auth state and flags. */ -interface RuntimePlan { - /** Rules to execute — materialized when gated, live under `--dangerously-run-scripts`. */ - execute: RuntimeRule[]; - /** Rules that will not run, with a reason. */ - skipped: SkippedRuntimeRule[]; - /** Human-only notices about the runtime disposition. */ - notices: string[]; -} - -/** Skip every runtime rule with a shared reason (an unverified path). */ -function skipAllRuntime(rules: RuntimeRule[], reason: string): RuntimePlan { - return { - execute: [], - skipped: rules.map((rule) => ({ rule: rule.name, reason })), - notices: [], - }; -} - -/** - * Decide which runtime rules run. A runtime rule's `check.ts` is arbitrary code - * execution, so it runs only when its signature is server-validated (an - * authenticated reconcile that returns it in `run`) or `--dangerously-run-scripts` - * is set. Every unverified path — anonymous, logged out, no remote, or a - * reconcile that cannot complete — skips runtime rules without failing. - */ -async function planRuntime( - cwd: string, - discovered: RuntimeRule[], - options: { anonymous: boolean; dangerouslyRunScripts: boolean } -): Promise { - if (discovered.length === 0) return { execute: [], skipped: [], notices: [] }; - - if (options.dangerouslyRunScripts) { - return { - execute: discovered, - skipped: [], - notices: [ - "Warning: --dangerously-run-scripts is executing runtime rule code without server verification.", - ], - }; - } - - if (options.anonymous) { - return skipAllRuntime( - discovered, - "anonymous mode — runtime rules were not verified and did not run" - ); - } - - const token = await getToken(cwd, { silent: true }); - if (!token) { - return skipAllRuntime( - discovered, - "not authenticated — runtime rules were not verified and did not run" - ); - } - - let repositoryUrl: string; - try { - repositoryUrl = await resolveRepositoryUrl(cwd); - } catch { - return skipAllRuntime( - discovered, - "no GitHub remote — runtime rules could not be verified and did not run" - ); - } - - // A rule whose check.ts is missing/unreadable is reported, not fatal: signing - // never throws, and such rules are surfaced as skipped so static checks and - // the other runtime rules are unaffected. - const { signed, unreadable } = await signRuntimeChecks(discovered); - const unreadableSkips: SkippedRuntimeRule[] = unreadable.map((rule) => ({ - rule: rule.name, - reason: "its check.ts is missing or unreadable", - })); - - const orgSubject = await resolveOrgSubject(cwd, token); - const outcome = await reconcile(token, { - orgId: orgSubject, - repositoryUrl, - files: reportRuntimeChecks(cwd, signed), - }); - - if (outcome.status === "unauthorized") { - return skipAllRuntime( - discovered, - `authentication was rejected — run \`${getCliPrefix()} auth login\` to re-authenticate` - ); - } - if (outcome.status === "unavailable") { - return skipAllRuntime( - discovered, - `the rule service was unavailable (${outcome.reason})` - ); - } - - const { blessed, withheld } = selectBlessedRuntimeRules( - signed, - outcome.result.run - ); - let execute: RuntimeRule[] = []; - try { - execute = - blessed.length > 0 ? await materializeRuntimeRules(cwd, blessed) : []; - } catch (error) { - const message = error instanceof Error ? error.message : String(error); - return skipAllRuntime( - discovered, - `runtime rules could not be materialized (${message})` - ); - } - // Repair the working tree from the server's verdicts. This changes what the - // NEXT run sees and nothing about this one: an `unsafe` rule stays withheld - // below whether or not its bytes were just restored. Fetching code and - // executing it in the same pass that discovered the drift would move the - // gate, and the gate is the point. - const repair = await repairWithheldRules(cwd, token, { - repositoryUrl, - result: outcome.result, - }); - - return { - execute, - skipped: [ - ...unreadableSkips, - ...withheld.map((rule) => ({ - rule: rule.name, - reason: "not blessed by the server (unsafe / unknown / drift)", - })), - ], - notices: repair.notices, - }; -} - -/** - * Act on the verdicts `check` used to parse and discard. - * - * `unsafe` and `missing` are repairable and are fetched; `unknown` is not, and - * gets an explanation instead. Every outcome here is a NOTICE rather than a - * failure: a rule that could not be repaired is a rule that stays withheld, - * which is already the safe state. A repair failing must never be the reason a - * `check` fails. - */ -async function repairWithheldRules( - cwd: string, - token: string, - input: { repositoryUrl: string; result: ReconcileResponse } -): Promise<{ notices: string[] }> { - const notices: string[] = []; - - // A file this disk holds that the service never issued. There is nothing to - // fetch, and saying so is the whole job: it reads as an unexplained skip - // otherwise, and the causes are ordinary (hand-written, or belonging to - // another organization or installation). - for (const entry of input.result.unknown) { - notices.push( - `${entry.file} was not issued by the rule service, so it cannot be ` + - `restored and will not run. It was written by hand, or belongs to a ` + - `different organization or installation.` - ); - } - - const { targets, unidentified } = repairTargets(input.result); - - // A repairable entry that named no rule. The service's own schema requires - // one, so reaching here means it broke that contract — and the entry is - // skipped rather than guessed at, because a rule left unrepaired stays - // withheld, which is safe, while a request built from a missing id asks for - // a rule nobody named and fails in a way nobody reads. - for (const entry of unidentified) { - notices.push( - `${entry.file} needs to be restored, but the rule service did not say ` + - `which rule it belongs to, so it could not be requested and will not ` + - `run.` - ); - } - - // Fetched concurrently: each target is a different rule id under the same - // token and repository, so they do not order against each other, and a repo - // with several drifted rules would otherwise pay one round trip per rule on - // every `check` until they reconverge. The WRITES stay sequential below, - // because two rules can share a directory prefix and a half-applied set is - // the state this whole path exists to avoid. - const fetched = await Promise.all( - targets.map(async (target) => ({ - target, - outcome: await restoreRule(token, { - ruleId: target.ruleId, - repositoryUrl: input.repositoryUrl, - }), - })) - ); - - for (const { target, outcome } of fetched) { - if (outcome.status !== "ok") { - notices.push( - `${target.file} could not be restored (${ - outcome.status === "unauthorized" - ? "authentication was rejected" - : outcome.reason - }).` - ); - continue; - } - - const rule = outcome.rules.find( - (candidate) => candidate.id === target.ruleId - ); - if (rule === undefined) { - notices.push( - `${target.file} could not be restored: the service returned no rule ` + - `called ${target.ruleId}.` - ); - continue; - } - - const verdict = await verifyRestoredCheck(target, rule); - if (!verdict.ok) { - notices.push(`${target.file} was not restored: ${verdict.reason}.`); - continue; - } - - try { - await writeRuleFile(cwd, rule); - } catch (error) { - // A failed WRITE and a failed CLEANUP ask the reader for opposite - // things, and saying "could not be written" for both is worse than - // saying nothing: the blessed bytes are on disk in the second case, so - // a reader acting on it re-runs a repair that already succeeded, or - // decides the rule is unrepaired and edits it by hand. - if (error instanceof PurgeIncompleteError) { - notices.push( - `${target.file} was rewritten with the bytes the service blessed, ` + - `but ${String(error.failures.length)} stale ` + - `${error.failures.length === 1 ? "entry" : "entries"} could not ` + - `be removed and an engine still reads ` + - `${error.failures.length === 1 ? "it" : "them"}: ` + - `${error.failures.join(", ")}.` - ); - continue; - } - const message = error instanceof Error ? error.message : String(error); - notices.push(`${target.file} could not be written (${message}).`); - continue; - } - // Now says what the DIRECTORY contains, not just what was written. The - // delivered set is authoritative (see `writeDeliveredFileSet`), so a file - // the set does not name — a stray capture beside the rule, which reconcile - // never reported because only `check.ts` is signed — is gone rather than - // left in place still changing what the rule matches. The one exception is - // `.tests/`, which is named here rather than glossed: fixtures are data no - // engine reads, they are kept, and a reader should not have to infer that - // from silence. - notices.push( - `${target.file} was restored: its rule directory now holds exactly the ` + - `files the service delivered, apart from test fixtures under ` + - `\`.tests/\`, which are left alone. It does not run in this pass; the ` + - `next \`check\` reports the repaired signature and is blessed through ` + - `the ordinary path.` - ); - } - - return { notices }; -} - /** Parse `--timeout ` into milliseconds; invalid/absent → undefined (default). */ function parseTimeoutMs(raw: string | undefined): number | undefined { if (raw === undefined) return undefined; diff --git a/packages/cli/src/commands/verify.ts b/packages/cli/src/commands/verify.ts index 121a4a27..b19a30c6 100644 --- a/packages/cli/src/commands/verify.ts +++ b/packages/cli/src/commands/verify.ts @@ -124,7 +124,17 @@ async function runOverPath(options: { results.push(await run(cwd, rule)); } - const failed = results.filter((result) => !result.ok); + // Three outcomes, not two. A run the execution policy refused is neither a + // pass nor a failure: the rule is not defective, and no action available to + // its holder would make a failure green, so failing it would turn `test` red + // for every project holding a runtime rule. It is excluded from the failures + // that set the exit code and from the count of rules tested, and gets its + // own marker below. + const isRefused = (result: RuleVerification | RuleTestResult): boolean => + "refused" in result && result.refused !== undefined; + const refused = results.filter((result) => isRefused(result)); + const failed = results.filter((result) => !result.ok && !isRefused(result)); + const tested = results.length - refused.length; if (json) { console.log( @@ -137,7 +147,10 @@ async function runOverPath(options: { ); } else { for (const result of results) { - const mark = result.ok ? "✓" : "✗"; + // `○` is neither tick nor cross on purpose: the rule was not tested, and + // a reader scanning the column has to be able to see that at a glance. + // The reason prints below it, from `errors`, and names what would run it. + const mark = isRefused(result) ? "○" : result.ok ? "✓" : "✗"; console.log(`${mark} ${result.engine}/${result.ruleId}`); for (const error of result.errors) { console.log(` ${error}`); @@ -149,10 +162,17 @@ async function runOverPath(options: { console.log(` notice: ${result.notice}`); } } + // 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" + // about a rule nothing ran. + const notRun = + refused.length === 0 + ? "" + : ` ${String(refused.length)} rule(s) did not run.`; console.log( failed.length === 0 - ? `\n${String(results.length)} rule(s) ${label === "verify" ? "verified" : "tested"}.` - : `\n${String(failed.length)} of ${String(results.length)} rule(s) failed.` + ? `\n${String(tested)} rule(s) ${label === "verify" ? "verified" : "tested"}.${notRun}` + : `\n${String(failed.length)} of ${String(tested)} rule(s) failed.${notRun}` ); } @@ -203,15 +223,43 @@ export const testCommand = defineCommand({ name: "test", description: "Run a rule's tests, after verifying the rule itself", }, - args: ruleTargetArguments, + args: { + ...ruleTargetArguments, + // The same flag `check` carries, spelled the same way. Here it is the ONLY + // thing that runs a runtime rule's fixtures: `test` does not reconcile, so + // there is no blessed-signature path that runs them without it. That makes + // this stricter than `check`, not looser — `check` will execute a blessed + // rule with no flag at all, because it runs rules as a side effect of + // scanning, whereas nothing runs under `test` unless it is asked for twice. + "dangerously-run-scripts": { + type: "boolean", + description: + "Run runtime-rule check.ts fixtures, which executes untrusted code", + default: false, + }, + }, async run({ args }) { const cwd = resolve(args.dir ?? process.cwd()); + const dangerouslyRunScripts = Boolean(args["dangerously-run-scripts"]); + // Warned once for the whole run, not once per rule: a tree of runtime rules + // is one decision by the user, and repeating the sentence per rule teaches + // people to scroll past it. On stderr, and suppressed under `--json` for + // `check`'s reason: a machine consumer cannot read prose. + let warned = false; await runOverPath({ cwd, target: args.path ?? ".taskless/rules", json: args.json, label: "test", - run: testOneRule, + run: async (ruleCwd, rule) => + testOneRule(ruleCwd, rule, { + dangerouslyRunScripts, + onRuntimeWarning: (message: string) => { + if (warned || args.json) return; + warned = true; + console.error(message); + }, + }), }); }, }); diff --git a/packages/cli/src/rules/fixtures.ts b/packages/cli/src/rules/fixtures.ts new file mode 100644 index 00000000..cd569de1 --- /dev/null +++ b/packages/cli/src/rules/fixtures.ts @@ -0,0 +1,100 @@ +import type { Dirent } from "node:fs"; +import { readdir } from "node:fs/promises"; + +import { isMissingDirectory } from "./errno"; + +/** + * The two questions every engine's fixture reader asks, answered once. + * + * Three engines verify a rule against fixtures — `sg` over `valid:`/`invalid:` + * keys in ast-grep test YAML, `vale` over `pass/`/`fail/` documents, `runtime` + * over `pass/`/`fail/` case directories — and all three had their own copy of + * the same two decisions. The copies were near-verbatim, which is worse than + * different: a comment in `rules/vale/verify.ts` 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 a third copy made that sentence false the day it was written. + * + * What is shared here is only what is genuinely the same. What each engine + * REJECTS is not: Vale's buckets hold documents, so a nested directory is the + * error; runtime's hold one directory per case, so a loose file is. Those stay + * beside their engines, where the reason for them is legible. + */ + +/** + * Directory entries, with a directory that is not there reading as an empty one. + * + * The single place that decides which `readdir` failures are absence and which + * are problems. Buckets are read independently, and this discrimination is why + * that is safe: swallowing an `EACCES` on `pass/` would yield `[]` while + * `fail/` still had fixtures, so the rule would not look one-sided and could + * report a pass having never checked the pass side at all. A permissions + * problem must not read as "no pass fixtures were written". + */ +export async function bucketEntries(directory: string): Promise { + try { + return await readdir(directory, { withFileTypes: true }); + } catch (error) { + if (isMissingDirectory(error)) return []; + throw error; + } +} + +/** + * Which fixture buckets a rule actually populated, over that engine's bucket + * names. + * + * Four states rather than a boolean, because a caller wants to say different + * things about them: `"none"` is an unwritten rule, while a `-only` is a + * half-written one, which is the more misleading of the two. A rule with only + * failing fixtures has shown it fires and not that it stays quiet; a rule with + * only passing ones has shown the opposite. Only `"both"` can reach a pass. + * + * Parameterised on the bucket names rather than fixed to `pass`/`fail` because + * `sg`'s buckets are the `valid:`/`invalid:` keys of ast-grep's own test YAML, + * and its vocabulary is ast-grep's rather than ours. + */ +export type FixtureCoverage = + | "both" + | `${Bucket}-only` + | "none"; + +/** + * Classify a rule's buckets by how many fixtures each held. + * + * `positive` is the bucket asserting the rule stays quiet (`pass`, `valid`) and + * `negative` the one asserting it fires (`fail`, `invalid`). + */ +export function classifyCoverage< + Positive extends string, + Negative extends string, +>( + positive: { name: Positive; count: number }, + negative: { name: Negative; count: number } +): FixtureCoverage { + if (positive.count > 0 && negative.count > 0) return "both"; + if (positive.count > 0) return `${positive.name}-only`; + if (negative.count > 0) return `${negative.name}-only`; + return "none"; +} + +/** + * What `test` says about coverage that is not `"both"`, or nothing when it is. + * + * `suffix` is how that engine spells a bucket, and it genuinely differs rather + * than having drifted: Vale's and runtime's buckets are directories, so they + * read `pass/`; ast-grep's are keys in a YAML document, so they read `valid:`. + * Naming a bucket in the shape the author will go and look for it is the point + * of the message, so the difference is a parameter rather than something to + * normalise away. + */ +export function describeCoverageShortfall( + ruleId: string, + coverage: FixtureCoverage, + suffix: "/" | ":" +): string | undefined { + if (coverage === "both") return undefined; + return coverage === "none" + ? `${ruleId} has no fixtures, so nothing shows it fires or stays quiet.` + : `${ruleId} has only ${coverage.replace("-only", "")}${suffix} fixtures — half a claim.`; +} diff --git a/packages/cli/src/rules/inspect.ts b/packages/cli/src/rules/inspect.ts index e7b2109e..4d0573a6 100644 --- a/packages/cli/src/rules/inspect.ts +++ b/packages/cli/src/rules/inspect.ts @@ -8,11 +8,19 @@ import { ruleDirectory, ruleFilePath, } from "./engines"; +import { describeCoverageShortfall } from "./fixtures"; import { type EngineName } from "./layout"; import { assessCaptureDirectory, + discoverRuntimeRules, strayModules, } from "./runtime/discover"; +import { readRuntimeFixtures } from "./runtime/fixtures"; +import { RUN_SCRIPTS_WARNING } from "./runtime/harness"; +import { + describeFixtureReport, + runRuntimeFixtures, +} from "./runtime/run-fixtures"; import { validateValeRule } from "../schemas/vale-rule"; import { verifyRule, type VerifyResult } from "./verify"; import { verifyValeRule } from "./vale/verify"; @@ -40,8 +48,26 @@ export interface RuleTestResult { ruleId: string; ok: boolean; errors: string[]; - /** Absent when `verify` failed and the tests never ran. */ + /** + * Whether the rule's tests actually ran. + * + * Load-bearing rather than advisory. `test` used to report a runtime rule as + * `ok: true, ran: false` and print a tick, so the one field that knew the + * rule had not been tested was the one nothing read. + */ ran: boolean; + /** + * Set when the execution policy refused the run, carrying the reason. + * + * The third outcome, and the one `ok` cannot express. A rule that cannot run + * because nothing blessed it is not a pass, and it is not a failure either: + * failing it would turn `test` red for every project holding a runtime rule, + * with no action available to its holder that makes it green. So it is + * neither, and the renderer, the summary count and the exit code all read + * this field rather than inferring the state from `ok` and `ran` together + * (which cannot distinguish it from a rule whose `verify` failed). + */ + refused?: string; /** * Something the engine said about its own configuration, as opposed to about * the rule. Vale reports a misplaced `.vale.ini` assignment this way: it @@ -53,6 +79,34 @@ export interface RuleTestResult { notice?: string; } +/** What `test` needs beyond a rule, all of it about the runtime engine. */ +export interface TestOptions { + /** + * Run a runtime rule's fixtures, which executes its `check.ts`. + * + * The WHOLE gate for `test`, and deliberately not `check`'s gate. `check` + * reconciles because it executes rules as a side effect of scanning a + * repository: the user asked for a scan, code ran, and the gate is what stops + * that happening silently. `test` runs fixtures because the user asked it to + * — the verb is the consent — so asking a server for permission to run your + * own fixtures is overreach, and the flag alone decides. + * + * That is STRICTLY MORE CONSERVATIVE than gating on reconcile as well: + * nothing executes here that would not have executed before, and a blessed + * rule that previously ran without the flag now requires it. Absent, which + * is the safe default, every runtime rule is refused. + */ + dangerouslyRunScripts?: boolean; + /** + * Called immediately before fixtures actually execute, never otherwise. + * + * Lazy so `test --dangerously-run-scripts` over a tree of `sg` rules does not + * warn about code it never ran. Deduplication belongs to the caller, which + * knows how many rules one command run covers. + */ + onRuntimeWarning?: (message: string) => void; +} + async function readYaml(path: string): Promise { return parse(await readFile(path, "utf8")) as unknown; } @@ -233,7 +287,8 @@ export async function verifyOneRule( */ export async function testOneRule( cwd: string, - rule: ResolvedRule + rule: ResolvedRule, + options: TestOptions = {} ): Promise { const { engine, ruleId } = rule; @@ -250,13 +305,17 @@ export async function testOneRule( // populated only one bucket has proved only half of what a rule claims. // `ast-grep test` will not say so — an empty `invalid:` bucket is // `1 passed; 0 failed`, exit zero — so the message has to come from here. - if (result.tests.fixtures !== "both") { - errors.push( - result.tests.fixtures === "none" - ? `${ruleId} has no fixtures, so nothing shows it fires or stays quiet.` - : `${ruleId} has only ${result.tests.fixtures.replace("-only", "")}: fixtures — half a claim.` - ); - } + // + // `:` rather than `/` because ast-grep's buckets are the `valid:`/`invalid:` + // KEYS of a test YAML document, not directories. The other two engines read + // `pass/`. Naming a bucket in the shape the author will search for is the + // point of the message, so the difference is deliberate, not drift. + const shortfall = describeCoverageShortfall( + ruleId, + result.tests.fixtures, + ":" + ); + if (shortfall !== undefined) errors.push(shortfall); return { engine, ruleId, @@ -286,13 +345,10 @@ export async function testOneRule( }; } const errors: string[] = []; - if (result.fixtures !== "both") { - errors.push( - result.fixtures === "none" - ? `${ruleId} has no fixtures, so nothing shows it fires or stays quiet.` - : `${ruleId} has only ${result.fixtures.replace("-only", "")}/ fixtures — half a claim.` - ); - } + // `/` because Vale's buckets are directories; see the `sg` branch above for + // why that suffix is a parameter rather than one spelling for all three. + const shortfall = describeCoverageShortfall(ruleId, result.fixtures, "/"); + if (shortfall !== undefined) errors.push(shortfall); for (const file of result.missingFailures) { errors.push(`fail fixture did not fire: ${file}`); } @@ -309,14 +365,93 @@ export async function testOneRule( }; } - // Runtime rules execute code, so their tests run through the harness under - // the same server verification `check` requires. Out of scope here: `test` - // reports that rather than quietly claiming a pass. + // Runtime rules execute code, so `test` runs their fixtures only behind + // `--dangerously-run-scripts`. It does NOT reconcile, and that is the whole + // difference from `check`. + // + // `check` asks the server 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 scan and the execution. `test` runs fixtures because the + // user typed `test`, and the verb is the consent; asking a server for + // permission to run your own fixtures is overreach. + // + // The only party a reconcile here would ever admit is someone testing an + // already-blessed delivered rule, whose fixtures the service verified before + // it delivered anything. A locally authored rule has no signature and never + // will, because blessing is recording and nothing recorded it — so for the + // audience that actually runs `test` on a runtime rule, a reconcile is pure + // cost paid for an answer that is always "no". + // + // Dropping it is strictly more conservative: nothing runs that would not have + // run before, and the blessed case that used to run without the flag now + // needs it. It also removes the `--anonymous` question rather than answering + // it, since with no network there is nothing to suppress. + if (options.dangerouslyRunScripts !== true) { + // Neither a pass nor a failure (D2), and the message has to say what would + // change it. The flag is the only route for anyone, so naming it here is + // the difference between a dead end and an instruction. + const reason = + `fixtures did not run: running them executes the rule's check.ts. ` + + `Pass --dangerously-run-scripts to run them.`; + return { + engine, + ruleId, + ok: false, + errors: [reason], + ran: false, + refused: reason, + }; + } + + // Discovery, not a plan: the bytes are the working tree's, which is the point + // of testing a rule you are authoring. `discoverRuntimeRules` is the same + // enumeration `check` plans over, so a rule it refuses — no loadable capture, + // or an unsigned module beside `check.ts` — is refused here identically + // rather than by a second opinion. `verify` has already run and named the + // real defect; this reports that nothing was run, not a verdict on the rule. + const discovered = await discoverRuntimeRules(cwd); + const runtimeRule = discovered.find((candidate) => candidate.name === ruleId); + if (runtimeRule === undefined) { + const reason = + `fixtures did not run: it was not discovered as a runnable runtime ` + + `rule (no capture rules under captures/).`; + return { + engine, + ruleId, + ok: false, + errors: [reason], + ran: false, + refused: reason, + }; + } + + let fixtures; + try { + fixtures = await readRuntimeFixtures(cwd, ruleId); + } catch (error) { + // A bucket that could not be read, or an entry that is not a directory. + // Both are failures of the fixtures rather than refusals of the run, so + // they fail: the alternative is a bucket reading as empty, which makes a + // two-sided rule look one-sided and a one-sided rule look complete. + return { + engine, + ruleId, + ok: false, + errors: [error instanceof Error ? error.message : String(error)], + ran: false, + }; + } + + // Warned at the last possible moment: everything above can still decline to + // run anything, and a warning about code that never executed is noise. + options.onRuntimeWarning?.(RUN_SCRIPTS_WARNING); + + const report = await runRuntimeFixtures(runtimeRule, fixtures); return { engine, ruleId, - ok: true, - errors: [], - ran: false, + ok: report.passed, + errors: describeFixtureReport(ruleId, report), + ran: true, }; } diff --git a/packages/cli/src/rules/runtime/fixtures.ts b/packages/cli/src/rules/runtime/fixtures.ts new file mode 100644 index 00000000..099750d8 --- /dev/null +++ b/packages/cli/src/rules/runtime/fixtures.ts @@ -0,0 +1,114 @@ +import { join } from "node:path"; + +import { + bucketEntries, + classifyCoverage, + 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. + * + * Vale's buckets hold documents; runtime's hold directories. That is not a + * stylistic difference. `executeRuntimeRule(root, rule)` already takes a root + * and `check` already passes the repository root, so a case directory is the + * same argument with a smaller tree behind it. A runtime rule exists because + * its evidence spans more than one file, so a layout allowing one file per + * case could not express the rules this tier is for. + */ +export interface RuntimeFixtureCase { + bucket: FixtureBucket; + /** The case directory's own name, for messages. */ + name: string; + /** Absolute path, and the `root` the check will be given. */ + root: string; +} + +/** + * Which buckets a rule actually populated. + * + * Four cases rather than a boolean, and the reasoning is `ValeFixtureCoverage`'s + * because the mistake it corrects is the same one: `"none"` is an unwritten + * rule, while `"pass-only"`/`"fail-only"` is a half-written one, which is the + * more misleading of the two. A rule with only `fail/` cases has shown it fires + * and not that it stays quiet; only `both` can reach a pass. + */ +export type RuntimeFixtureCoverage = FixtureCoverage<"pass" | "fail">; + +/** + * The cases in one bucket, one level deep. + * + * A missing bucket is an empty one and anything else rethrows, which is + * {@link bucketEntries}'s decision rather than one taken again here: swallowing + * an `EACCES` on `pass/` would yield `[]` while `fail/` still had cases, so the + * rule would not look one-sided and could report a pass having never checked + * the pass side at all. + * + * What is this engine's own is the rejection, and it is the OPPOSITE of Vale's. + * Vale's buckets hold documents, so a nested directory is its error; these hold + * one directory per case, so every entry must be a directory and a loose file + * is an error naming its own path rather than an entry quietly skipped. The + * check is handed a root and reads whatever it needs beneath it, so a file has + * no root to be: there is no sensible reading of `pass/example.ts` that the + * harness could act on, and ignoring it would leave an author with a fixture + * they wrote, that never ran, and that nothing mentioned. + */ +async function bucketCases( + cwd: string, + ruleId: string, + bucket: FixtureBucket +): Promise { + const directory = join(ruleTestsDirectory(cwd, "runtime", ruleId), bucket); + const entries = await bucketEntries(directory); + + const loose = entries.find((entry) => !entry.isDirectory()); + if (loose !== undefined) { + throw new Error( + `A runtime fixture case is a directory: ${join(directory, loose.name)} ` + + `is not one. The check is given the case directory as its root and ` + + `reads the files it needs beneath it, so a bare file in ${bucket}/ ` + + `has no root to be and would never run.` + ); + } + + return entries.map((entry) => ({ + bucket, + name: entry.name, + root: join(directory, entry.name), + })); +} + +/** Every fixture case a runtime rule holds, with what the buckets cover. */ +export interface RuntimeFixtures { + cases: RuntimeFixtureCase[]; + coverage: RuntimeFixtureCoverage; +} + +/** + * Read a runtime rule's fixture cases from `.tests/pass/` and `.tests/fail/`. + * + * Reads the buckets independently rather than walking `.tests/` once, so an + * unreadable bucket surfaces as itself instead of as an absence. + */ +export async function readRuntimeFixtures( + cwd: string, + ruleId: string +): Promise { + const [pass, fail] = await Promise.all([ + bucketCases(cwd, ruleId, "pass"), + bucketCases(cwd, ruleId, "fail"), + ]); + + return { + cases: [...fail, ...pass], + coverage: classifyCoverage( + { name: "pass", count: pass.length }, + { name: "fail", count: fail.length } + ), + }; +} diff --git a/packages/cli/src/rules/runtime/harness.ts b/packages/cli/src/rules/runtime/harness.ts index f6ffd056..749b9d6f 100644 --- a/packages/cli/src/rules/runtime/harness.ts +++ b/packages/cli/src/rules/runtime/harness.ts @@ -9,6 +9,18 @@ import { DEFAULT_CHECK_TIMEOUT_MS, invokeCheck } from "./invoke"; /** Scanner-agnostic `source` label for runtime-rule findings. */ export const RUNTIME_SOURCE = "taskless-runtime"; +/** + * What `--dangerously-run-scripts` says when it is what let the code run. + * + * Stated beside the executor rather than in either command, because `check` and + * `test` reach the flag by different routes — `check` through the reconcile + * plan, `test` with no plan at all — and the sentence a user reads should not + * depend on which route ran. Two spellings would drift, and the drift would be + * invisible: both are warnings nobody diffs. + */ +export const RUN_SCRIPTS_WARNING = + "Warning: --dangerously-run-scripts is executing runtime rule code without server verification."; + /** Options controlling a runtime-rule run. */ export interface RuntimeRunOptions { /** Restrict the narrow to these paths (diff scope); empty scans the repo. */ @@ -58,30 +70,62 @@ function harnessErrorResult( } /** - * Execute one runtime rule: run the ast-grep narrow, gate on matches (zero - * matches ⇒ `check.ts` is never invoked), invoke `check.ts`, and map its - * findings onto `CheckResult`. A harness failure is isolated to a single + * One run of a rule against one root, with the facts a caller cannot recover + * from the findings alone. + * + * `findings` is exactly what {@link executeRuntimeRule} returns, so the scan + * path is unchanged. The other two fields carry what an empty array hides: + * + * - `invoked` says whether `check.ts` ran at all. The narrow gates it, so a + * root with no matches produces `[]` having never reached the check, which is + * indistinguishable downstream from a check that ran and found nothing. A + * scan does not care — both mean "nothing to report about this tree" — but a + * fixture case does, in BOTH buckets: a `fail/` case in that state is a + * fixture that never reached the check rather than a rule that stopped + * firing, and a `pass/` case in that state proves the narrow did not match + * rather than that the check stays quiet. + * - `failure` says the harness itself broke — the narrow threw, or the check + * threw, timed out, or returned unusable output. Downstream that is also + * zero findings from the check's point of view, and only one of the two is + * the rule's fault. + */ +export interface RuntimeExecution { + /** The findings, mapped exactly as the scan path receives them. */ + findings: CheckResult[]; + /** Whether `check.ts` was actually invoked. */ + invoked: boolean; + /** Set when the narrow or the check failed, rather than found nothing. */ + failure?: string; +} + +/** + * Execute one runtime rule and report what happened, not only what it found. + * + * This is the single execution path. {@link executeRuntimeRule} is a thin + * projection of it, so the narrow runs once, `check.ts` is invoked once, and + * "did it run" has one source of truth rather than a scan and a fixture runner + * each deciding for themselves. A harness failure is isolated to a single * error-severity finding and never throws. */ -export async function executeRuntimeRule( +export async function executeRuntimeRuleDetailed( root: string, rule: RuntimeRule, options: RuntimeRunOptions = {} -): Promise { +): Promise { let matches; try { matches = await runNarrow(root, rule, options.paths ?? []); } catch (error) { - return [ - harnessErrorResult( - root, - rule, - `narrow failed: ${error instanceof Error ? error.message : String(error)}` - ), - ]; + const message = `narrow failed: ${error instanceof Error ? error.message : String(error)}`; + return { + findings: [harnessErrorResult(root, rule, message)], + invoked: false, + failure: message, + }; } - if (matches.length === 0) return []; // gate: no matches, no check + // gate: no matches, no check. The gate is the reason `invoked` exists. + if (matches.length === 0) return { findings: [], invoked: false }; const result = await invokeCheck( rule.checkFile, @@ -90,9 +134,33 @@ export async function executeRuntimeRule( options.timeoutMs ?? DEFAULT_CHECK_TIMEOUT_MS ); if (result.status === "error") { - return [harnessErrorResult(root, rule, result.message)]; + return { + findings: [harnessErrorResult(root, rule, result.message)], + invoked: true, + failure: result.message, + }; } - return result.findings.map((finding) => findingToCheckResult(rule, finding)); + return { + findings: result.findings.map((finding) => + findingToCheckResult(rule, finding) + ), + invoked: true, + }; +} + +/** + * Execute one runtime rule: run the ast-grep narrow, gate on matches (zero + * matches ⇒ `check.ts` is never invoked), invoke `check.ts`, and map its + * findings onto `CheckResult`. A harness failure is isolated to a single + * error-severity finding and never throws. + */ +export async function executeRuntimeRule( + root: string, + rule: RuntimeRule, + options: RuntimeRunOptions = {} +): Promise { + const execution = await executeRuntimeRuleDetailed(root, rule, options); + return execution.findings; } /** diff --git a/packages/cli/src/rules/runtime/plan.ts b/packages/cli/src/rules/runtime/plan.ts new file mode 100644 index 00000000..b5fddadd --- /dev/null +++ b/packages/cli/src/rules/runtime/plan.ts @@ -0,0 +1,311 @@ +import { getToken } from "../../auth/token"; +import { resolveOrgSubject } from "../../auth/org"; +import { resolveRepositoryUrl } from "../../util/git-remote"; +import { getCliPrefix } from "../../util/package-manager"; +import { reconcile } from "../../api/reconcile"; +import type { ReconcileResponse } from "../../api/reconcile"; +import { restoreRule } from "../../api/restore"; +import { writeRuleFile } from "../files"; +import { PurgeIncompleteError } from "../deliver"; +import { repairTargets, verifyRestoredCheck } from "./repair"; +import { RUN_SCRIPTS_WARNING } from "./harness"; +import { type RuntimeRule } from "./discover"; +import { + materializeRuntimeRules, + reportRuntimeChecks, + selectBlessedRuntimeRules, + signRuntimeChecks, +} from "./run-set"; + +/** + * Deciding WHICH runtime rules may execute during a `check`, separately from + * executing them. + * + * `check` is the only caller and the only command that needs this. It executes + * rules as a SIDE EFFECT of scanning a repository — nobody asked for code to + * run — so a reconcile stands between the scan and the execution, and every + * unverified path skips rather than fails. + * + * `test` deliberately does not come through here. It runs a rule's fixtures + * because the user typed `test`, so the verb is the consent and + * `--dangerously-run-scripts` is its whole gate; see the runtime branch of + * `rules/inspect.ts`. That is stricter than sharing this module, not looser: + * nothing runs under `test` that would not have run before, and a blessed rule + * that used to run there without the flag now needs it. + * + * It lived inside `commands/check.ts` and moved here for a reason that has + * since evaporated (sharing the gate with `test`). It stays because a second + * reason holds on its own: this is policy, `commands/check.ts` is argument + * parsing and rendering, and `repairWithheldRules` below is a long piece of + * recovery logic that a command file has no business carrying. + */ + +/** A runtime rule that will not run, with why (advisory). */ +export interface SkippedRuntimeRule { + rule: string; + reason: string; +} + +/** The runtime-execution plan resolved from auth state and flags. */ +export interface RuntimePlan { + /** Rules to execute — materialized when gated, live under `--dangerously-run-scripts`. */ + execute: RuntimeRule[]; + /** Rules that will not run, with a reason. */ + skipped: SkippedRuntimeRule[]; + /** Human-only notices about the runtime disposition. */ + notices: string[]; +} + +/** Skip every runtime rule with a shared reason (an unverified path). */ +function skipAllRuntime(rules: RuntimeRule[], reason: string): RuntimePlan { + return { + execute: [], + skipped: rules.map((rule) => ({ rule: rule.name, reason })), + notices: [], + }; +} + +/** + * Decide which runtime rules run. A runtime rule's `check.ts` is arbitrary code + * execution, so it runs only when its signature is server-validated (an + * authenticated reconcile that returns it in `run`) or `--dangerously-run-scripts` + * is set. Every unverified path — anonymous, logged out, no remote, or a + * reconcile that cannot complete — skips runtime rules without failing. + */ +export async function planRuntime( + cwd: string, + discovered: RuntimeRule[], + options: { anonymous: boolean; dangerouslyRunScripts: boolean } +): Promise { + if (discovered.length === 0) return { execute: [], skipped: [], notices: [] }; + + if (options.dangerouslyRunScripts) { + return { + execute: discovered, + skipped: [], + notices: [RUN_SCRIPTS_WARNING], + }; + } + + if (options.anonymous) { + return skipAllRuntime( + discovered, + "anonymous mode — runtime rules were not verified and did not run" + ); + } + + const token = await getToken(cwd, { silent: true }); + if (!token) { + return skipAllRuntime( + discovered, + "not authenticated — runtime rules were not verified and did not run" + ); + } + + let repositoryUrl: string; + try { + repositoryUrl = await resolveRepositoryUrl(cwd); + } catch { + return skipAllRuntime( + discovered, + "no GitHub remote — runtime rules could not be verified and did not run" + ); + } + + // A rule whose check.ts is missing/unreadable is reported, not fatal: signing + // never throws, and such rules are surfaced as skipped so static checks and + // the other runtime rules are unaffected. + const { signed, unreadable } = await signRuntimeChecks(discovered); + const unreadableSkips: SkippedRuntimeRule[] = unreadable.map((rule) => ({ + rule: rule.name, + reason: "its check.ts is missing or unreadable", + })); + + const orgSubject = await resolveOrgSubject(cwd, token); + const outcome = await reconcile(token, { + orgId: orgSubject, + repositoryUrl, + files: reportRuntimeChecks(cwd, signed), + }); + + if (outcome.status === "unauthorized") { + return skipAllRuntime( + discovered, + `authentication was rejected — run \`${getCliPrefix()} auth login\` to re-authenticate` + ); + } + if (outcome.status === "unavailable") { + return skipAllRuntime( + discovered, + `the rule service was unavailable (${outcome.reason})` + ); + } + + const { blessed, withheld } = selectBlessedRuntimeRules( + signed, + outcome.result.run + ); + let execute: RuntimeRule[] = []; + try { + execute = + blessed.length > 0 ? await materializeRuntimeRules(cwd, blessed) : []; + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + return skipAllRuntime( + discovered, + `runtime rules could not be materialized (${message})` + ); + } + // Repair the working tree from the server's verdicts. This changes what the + // NEXT run sees and nothing about this one: an `unsafe` rule stays withheld + // below whether or not its bytes were just restored. Fetching code and + // executing it in the same pass that discovered the drift would move the + // gate, and the gate is the point. + const repair = await repairWithheldRules(cwd, token, { + repositoryUrl, + result: outcome.result, + }); + + return { + execute, + skipped: [ + ...unreadableSkips, + ...withheld.map((rule) => ({ + rule: rule.name, + reason: "not blessed by the server (unsafe / unknown / drift)", + })), + ], + notices: repair.notices, + }; +} + +/** + * Act on the verdicts `check` used to parse and discard. + * + * `unsafe` and `missing` are repairable and are fetched; `unknown` is not, and + * gets an explanation instead. Every outcome here is a NOTICE rather than a + * failure: a rule that could not be repaired is a rule that stays withheld, + * which is already the safe state. A repair failing must never be the reason a + * `check` fails. + */ +async function repairWithheldRules( + cwd: string, + token: string, + input: { repositoryUrl: string; result: ReconcileResponse } +): Promise<{ notices: string[] }> { + const notices: string[] = []; + + // A file this disk holds that the service never issued. There is nothing to + // fetch, and saying so is the whole job: it reads as an unexplained skip + // otherwise, and the causes are ordinary (hand-written, or belonging to + // another organization or installation). + for (const entry of input.result.unknown) { + notices.push( + `${entry.file} was not issued by the rule service, so it cannot be ` + + `restored and will not run. It was written by hand, or belongs to a ` + + `different organization or installation.` + ); + } + + const { targets, unidentified } = repairTargets(input.result); + + // A repairable entry that named no rule. The service's own schema requires + // one, so reaching here means it broke that contract — and the entry is + // skipped rather than guessed at, because a rule left unrepaired stays + // withheld, which is safe, while a request built from a missing id asks for + // a rule nobody named and fails in a way nobody reads. + for (const entry of unidentified) { + notices.push( + `${entry.file} needs to be restored, but the rule service did not say ` + + `which rule it belongs to, so it could not be requested and will not ` + + `run.` + ); + } + + // Fetched concurrently: each target is a different rule id under the same + // token and repository, so they do not order against each other, and a repo + // with several drifted rules would otherwise pay one round trip per rule on + // every `check` until they reconverge. The WRITES stay sequential below, + // because two rules can share a directory prefix and a half-applied set is + // the state this whole path exists to avoid. + const fetched = await Promise.all( + targets.map(async (target) => ({ + target, + outcome: await restoreRule(token, { + ruleId: target.ruleId, + repositoryUrl: input.repositoryUrl, + }), + })) + ); + + for (const { target, outcome } of fetched) { + if (outcome.status !== "ok") { + notices.push( + `${target.file} could not be restored (${ + outcome.status === "unauthorized" + ? "authentication was rejected" + : outcome.reason + }).` + ); + continue; + } + + const rule = outcome.rules.find( + (candidate) => candidate.id === target.ruleId + ); + if (rule === undefined) { + notices.push( + `${target.file} could not be restored: the service returned no rule ` + + `called ${target.ruleId}.` + ); + continue; + } + + const verdict = await verifyRestoredCheck(target, rule); + if (!verdict.ok) { + notices.push(`${target.file} was not restored: ${verdict.reason}.`); + continue; + } + + try { + await writeRuleFile(cwd, rule); + } catch (error) { + // A failed WRITE and a failed CLEANUP ask the reader for opposite + // things, and saying "could not be written" for both is worse than + // saying nothing: the blessed bytes are on disk in the second case, so + // a reader acting on it re-runs a repair that already succeeded, or + // decides the rule is unrepaired and edits it by hand. + if (error instanceof PurgeIncompleteError) { + notices.push( + `${target.file} was rewritten with the bytes the service blessed, ` + + `but ${String(error.failures.length)} stale ` + + `${error.failures.length === 1 ? "entry" : "entries"} could not ` + + `be removed and an engine still reads ` + + `${error.failures.length === 1 ? "it" : "them"}: ` + + `${error.failures.join(", ")}.` + ); + continue; + } + const message = error instanceof Error ? error.message : String(error); + notices.push(`${target.file} could not be written (${message}).`); + continue; + } + // Now says what the DIRECTORY contains, not just what was written. The + // delivered set is authoritative (see `writeDeliveredFileSet`), so a file + // the set does not name — a stray capture beside the rule, which reconcile + // never reported because only `check.ts` is signed — is gone rather than + // left in place still changing what the rule matches. The one exception is + // `.tests/`, which is named here rather than glossed: fixtures are data no + // engine reads, they are kept, and a reader should not have to infer that + // from silence. + notices.push( + `${target.file} was restored: its rule directory now holds exactly the ` + + `files the service delivered, apart from test fixtures under ` + + `\`.tests/\`, which are left alone. It does not run in this pass; the ` + + `next \`check\` reports the repaired signature and is blessed through ` + + `the ordinary path.` + ); + } + + return { notices }; +} diff --git a/packages/cli/src/rules/runtime/run-fixtures.ts b/packages/cli/src/rules/runtime/run-fixtures.ts new file mode 100644 index 00000000..88edb97f --- /dev/null +++ b/packages/cli/src/rules/runtime/run-fixtures.ts @@ -0,0 +1,159 @@ +import { describeCoverageShortfall } from "../fixtures"; +import type { RuntimeRule } from "./discover"; +import { executeRuntimeRuleDetailed } from "./harness"; +import type { RuntimeRunOptions } from "./harness"; +import type { + RuntimeFixtureCase, + RuntimeFixtureCoverage, + RuntimeFixtures, +} from "./fixtures"; + +/** A case that reached the check but broke its bucket's expectation. */ +export interface FixtureCheckFailure { + /** The case directory's name, which is what an author has to go and edit. */ + name: string; + message: string; +} + +/** + * What running a runtime rule's fixtures showed. + * + * Named after the Vale runner's fields on purpose: `missingFailures` and + * `unexpectedFindings` mean the same thing one tier up, so a reader who knows + * one runner knows this one. The two extra lists are D8's, and they exist + * because "the check produced no findings" is three different situations that + * an array of findings cannot tell apart. + */ +export interface RuntimeFixtureReport { + passed: boolean; + coverage: RuntimeFixtureCoverage; + /** `fail/` cases where the check ran and reported nothing. */ + missingFailures: string[]; + /** `pass/` cases where the check ran and reported something. */ + unexpectedFindings: string[]; + /** + * Cases whose narrow matched nothing, so `check.ts` was never invoked. + * + * A fixture defect in EITHER bucket. A `fail/` case here did not show the + * rule stopped firing, it showed the fixture never reached the check; a + * `pass/` case here looks like a clean pass and proves only that the narrow + * did not match, so it cannot show the check stays quiet. Both point at the + * fixture, which is the file the author can actually change. + */ + neverInvoked: string[]; + /** Cases where the narrow or the check itself failed, rather than found nothing. */ + checkFailures: FixtureCheckFailure[]; +} + +/** `fail/case-1` — the bucket is half the identity of a case. */ +function label(fixtureCase: RuntimeFixtureCase): string { + return `${fixtureCase.bucket}/${fixtureCase.name}`; +} + +/** + * Run every fixture case a runtime rule holds, one case directory per run. + * + * The case directory IS the `root`, which is why this is a caller of + * {@link executeRuntimeRuleDetailed} rather than a sibling of it: process + * spawn, timeout, narrowing, capture discovery and the result mapping are the + * ones `check` already proves on every authenticated scan, pointed at a smaller + * tree. Nothing here re-runs the narrow or re-invokes the check. + * + * Cases run in sequence, matching `executeRuntimeRules`, so `tsx` worker + * startup stays predictable. + */ +export async function runRuntimeFixtures( + rule: RuntimeRule, + fixtures: RuntimeFixtures, + options: RuntimeRunOptions = {} +): Promise { + const missingFailures: string[] = []; + const unexpectedFindings: string[] = []; + const neverInvoked: string[] = []; + const checkFailures: FixtureCheckFailure[] = []; + + for (const fixtureCase of fixtures.cases) { + const execution = await executeRuntimeRuleDetailed( + fixtureCase.root, + rule, + options + ); + + // Checked first, and separately from the findings. A throw and a timeout + // both arrive as one error-severity finding, so reading the findings alone + // would count a crashed check as a `fail/` case that fired correctly. + if (execution.failure !== undefined) { + checkFailures.push({ + name: label(fixtureCase), + message: execution.failure, + }); + continue; + } + + if (!execution.invoked) { + neverInvoked.push(label(fixtureCase)); + continue; + } + + if (fixtureCase.bucket === "fail") { + if (execution.findings.length === 0) + missingFailures.push(label(fixtureCase)); + } else if (execution.findings.length > 0) { + unexpectedFindings.push(label(fixtureCase)); + } + } + + return { + // Only `both` can pass, for `ValeFixtureCoverage`'s reason: a rule with one + // bucket has proved half of what a rule claims, and a rule with neither has + // proved nothing while exiting zero. + passed: + fixtures.coverage === "both" && + missingFailures.length === 0 && + unexpectedFindings.length === 0 && + neverInvoked.length === 0 && + checkFailures.length === 0, + coverage: fixtures.coverage, + missingFailures, + unexpectedFindings, + neverInvoked, + checkFailures, + }; +} + +/** + * The report as the lines `test` prints under the rule. + * + * Each line names the case, because the case is the file the author edits. The + * `neverInvoked` line says the check did not run rather than "expected a + * finding, got none": the second is true and sends the author to `check.ts`, + * which is not where the problem is. + */ +export function describeFixtureReport( + ruleId: string, + report: RuntimeFixtureReport +): string[] { + const errors: string[] = []; + + // `/` because these buckets are directories, which is how the author will go + // looking for them. `sg`'s are YAML keys and read `valid:`. + const shortfall = describeCoverageShortfall(ruleId, report.coverage, "/"); + if (shortfall !== undefined) errors.push(shortfall); + for (const name of report.missingFailures) { + errors.push(`fail fixture did not fire: ${name}`); + } + for (const name of report.unexpectedFindings) { + errors.push(`pass fixture wrongly fired: ${name}`); + } + for (const name of report.neverInvoked) { + errors.push( + `fixture case matched no captures, so check.ts never ran: ${name}. ` + + `The case is the defect, not the check: add code the rule's captures match.` + ); + } + for (const failure of report.checkFailures) { + errors.push(`check failed on ${failure.name}: ${failure.message}`); + } + + return errors; +} diff --git a/packages/cli/src/rules/vale/verify.ts b/packages/cli/src/rules/vale/verify.ts index 95805bc7..dfb706a5 100644 --- a/packages/cli/src/rules/vale/verify.ts +++ b/packages/cli/src/rules/vale/verify.ts @@ -1,11 +1,14 @@ -import { type Dirent, mkdtempSync, rmSync, writeFileSync } from "node:fs"; -import { readdir } from "node:fs/promises"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join, posix, relative, resolve, sep } from "node:path"; import { listRuleIds, ruleTestsDirectory } from "../engines"; import { RULES_DIRECTORY } from "../layout"; -import { isMissingDirectory } from "../errno"; +import { + bucketEntries, + classifyCoverage, + type FixtureCoverage, +} from "../fixtures"; import { runVale, type ValeRunOutcome } from "./run"; /** Where a rule's fixtures live, relative to the project root. */ @@ -56,34 +59,22 @@ export function buildIsolatingConfig(cwd: string, ruleId: string): string { ].join("\n"); } -/** - * Directory entries, with a directory that is not there reading as an empty one. - * - * The single place that decides which `readdir` failures are absence and which - * are problems, so no caller can accidentally answer that question differently. - */ -async function directoryEntries(directory: string): Promise { - try { - return await readdir(directory, { withFileTypes: true }); - } catch (error) { - if (isMissingDirectory(error)) return []; - throw error; - } -} - /** * Fixture documents directly under `///`. * - * A missing directory is an empty bucket; anything else rethrows. The buckets - * are read independently, so a swallowed `EACCES` on `pass/` would silently - * yield `[]` while `fail/` still had fixtures — the rule would not look - * one-sided, and could report `passed: true` having never checked the pass side - * at all. A permissions problem must not read as "no pass fixtures were - * written". + * A missing directory is an empty bucket and anything else rethrows, which is + * {@link bucketEntries}'s decision rather than one taken again here: the + * buckets are read independently, so a swallowed `EACCES` on `pass/` would + * silently yield `[]` while `fail/` still had fixtures — the rule would not + * look one-sided, and could report `passed: true` having never checked the pass + * side at all. * - * A bucket is one directory deep, and a nested directory is rejected rather - * than ignored. The two halves of verification disagree about recursion: this - * read is flat, but Vale is invoked over the whole `rule-tests/` tree and + * What is Vale's own is the rejection below, and it is the opposite of the + * runtime engine's: Vale's buckets hold DOCUMENTS, so a nested directory is the + * error, where runtime's hold one directory per case and a loose file is. A + * bucket is one directory deep, and a nested directory is rejected rather than + * ignored. The two halves of verification disagree about recursion: this read + * is flat, but Vale is invoked over the whole `rule-tests/` tree and * lints recursively. Silently skipping a nested entry therefore fails in the * dangerous direction — a nested `pass/` fixture that wrongly fires produces a * finding this function never collected, so `unexpectedFindings` discards it, @@ -101,7 +92,7 @@ async function fixtureFiles( bucket: "pass" | "fail" ): Promise { const directory = join(valeRuleTestsDirectory(cwd, ruleId), bucket); - const entries = await directoryEntries(directory); + const entries = await bucketEntries(directory); const nested = entries.find((entry) => entry.isDirectory()); if (nested !== undefined) { @@ -132,16 +123,13 @@ function toRelativePosix(cwd: string, absolute: string): string { * different things about them: `"none"` is an unwritten rule, while * `"fail-only"`/`"pass-only"` is a half-written one, which is the more * misleading state of the two. + * + * A name for {@link FixtureCoverage} over Vale's bucket names rather than a + * fourth hand-written union: the classification is `classifyCoverage`'s, so a + * caller reading `ValeFixtureCoverage` and one reading `SgFixtureCoverage` are + * reading the same four states under two vocabularies. */ -export type ValeFixtureCoverage = "both" | "pass-only" | "fail-only" | "none"; - -/** Classify a rule's buckets by how many documents each held. */ -function coverageOf(passCount: number, failCount: number): ValeFixtureCoverage { - if (passCount > 0 && failCount > 0) return "both"; - if (passCount > 0) return "pass-only"; - if (failCount > 0) return "fail-only"; - return "none"; -} +export type ValeFixtureCoverage = FixtureCoverage<"pass" | "fail">; export interface ValeRuleVerification { ruleId: string; @@ -195,7 +183,7 @@ export async function discoverValeRuleTests(cwd: string): Promise { const ruleIds = await listRuleIds(cwd, "vale"); const withTests: string[] = []; for (const ruleId of ruleIds) { - const entries = await directoryEntries(valeRuleTestsDirectory(cwd, ruleId)); + const entries = await bucketEntries(valeRuleTestsDirectory(cwd, ruleId)); if (entries.length > 0) withTests.push(ruleId); } return withTests; @@ -239,7 +227,10 @@ export async function verifyValeRule( // Short-circuited before Vale runs: the rule has to be edited either way, so // there is nothing a subprocess could add that `fixtures` does not say. - const fixtures = coverageOf(passFixtures.length, failFixtures.length); + const fixtures = classifyCoverage( + { name: "pass", count: passFixtures.length }, + { name: "fail", count: failFixtures.length } + ); if (fixtures !== "both") { return { ruleId, diff --git a/packages/cli/src/rules/verify.ts b/packages/cli/src/rules/verify.ts index c3d0501a..5d83ce56 100644 --- a/packages/cli/src/rules/verify.ts +++ b/packages/cli/src/rules/verify.ts @@ -12,6 +12,7 @@ import { findRegexWithoutKind, } from "../schemas/ast-grep-rule"; import { pathPrefixed, schemaLayer } from "../schemas/layer"; +import { classifyCoverage, type FixtureCoverage } from "./fixtures"; import { AST_GREP_TSX_SPLIT, AST_GREP_VERSION, @@ -65,7 +66,7 @@ export interface RequirementsResult extends LayerResult { * YAML rather than Vale's `pass/`/`fail/` directories, so the names follow * ast-grep's vocabulary. */ -export type SgFixtureCoverage = "both" | "valid-only" | "invalid-only" | "none"; +export type SgFixtureCoverage = FixtureCoverage<"valid" | "invalid">; export interface TestLayerResult extends LayerResult { passed: number; @@ -364,17 +365,6 @@ async function discoverRuleTestFiles( .map((entry) => join(directory, entry)); } -/** Classify a rule's buckets by how many sources each held. */ -function coverageOf( - validCount: number, - invalidCount: number -): SgFixtureCoverage { - if (validCount > 0 && invalidCount > 0) return "both"; - if (validCount > 0) return "valid-only"; - if (invalidCount > 0) return "invalid-only"; - return "none"; -} - /** * Count what the author actually put in each bucket, across every test file * the rule owns. @@ -422,7 +412,10 @@ async function fixtureCoverage( if (Array.isArray(buckets.invalid)) invalidCount += buckets.invalid.length; } - return coverageOf(validCount, invalidCount); + return classifyCoverage( + { name: "valid", count: validCount }, + { name: "invalid", count: invalidCount } + ); } // --- Layer 3: Test execution --- diff --git a/packages/cli/src/schemas/verify-test.ts b/packages/cli/src/schemas/verify-test.ts index 04a92817..3a8b3cdc 100644 --- a/packages/cli/src/schemas/verify-test.ts +++ b/packages/cli/src/schemas/verify-test.ts @@ -15,7 +15,15 @@ const ruleResultSchema = z.object({ ran: z .boolean() .optional() - .describe("`test` only: whether the rule's tests actually ran"), + .describe( + "`test` only: whether the rule's tests actually ran. Branch on this, not on `ok` alone: `ok: true` with `ran: false` is not a rule that passed" + ), + refused: z + .string() + .optional() + .describe( + "`test` only: the execution policy declined to run the rule's fixtures, and why. Neither a pass nor a failure, excluded from the rules tested, and never on its own a reason for a non-zero exit" + ), notice: z .string() .optional() diff --git a/packages/cli/test/runtime-fixture-runner.test.ts b/packages/cli/test/runtime-fixture-runner.test.ts new file mode 100644 index 00000000..a25ba523 --- /dev/null +++ b/packages/cli/test/runtime-fixture-runner.test.ts @@ -0,0 +1,465 @@ +import { execFile } from "node:child_process"; +import { chmod, 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"; + +/** + * `taskless test` against a runtime rule. + * + * The defect this file exists for: `test` printed `✓ runtime/` and counted + * the rule among those tested for a rule whose fixtures never ran. Every + * assertion here is about the difference between running a rule and saying + * one ran, so none of them may be relaxed into "does not crash". + * + * These spawn the built CLI rather than calling `testOneRule`, because the tick + * and the summary line are half the defect and only the command prints them. + */ + +const execFileAsync = promisify(execFile); +const binPath = resolve(import.meta.dirname, "../dist/index.js"); + +let cwd: string; + +async function runCli(args: string[]) { + try { + const { stdout, stderr } = await execFileAsync("node", [binPath, ...args]); + return { stdout, stderr, exitCode: 0 }; + } catch (error) { + const failure = error as { stdout: string; stderr: string; code: number }; + return { + stdout: failure.stdout ?? "", + stderr: failure.stderr ?? "", + exitCode: failure.code, + }; + } +} + +interface Report { + ok: boolean; + rules: { + engine: string; + ruleId: string; + ok: boolean; + errors: string[]; + ran?: boolean; + refused?: string; + }[]; +} + +/** A capture matching `eval(...)`, which every fixture below is written around. */ +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"); + +/** + * The realistic shape: the capture narrows to `eval(...)`, and the check + * decides. It reports only where the argument is not a literal, which is the + * judgement a runtime rule exists to make and an ast-grep pattern cannot. + * + * This is what makes a `pass/` case possible at all. A pass case has to MATCH + * the narrow, or `check.ts` is never invoked and the case proves nothing about + * the check staying quiet — so a check that flagged every match would have no + * passing fixture that ran. + */ +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: "eval on a non-literal is not allowed", + severity: "warning", + })); +} +`; + +/** Reports one finding per match: the indiscriminate rule, which cannot pass. */ +const FLAGS_EVERY_MATCH = `export default async function (root, matches) { + return matches.map((m) => ({ + file: m.file, + line: m.line, + column: m.column, + message: "eval is not allowed", + severity: "warning", + })); +} +`; + +/** Reaches the check and deliberately reports nothing. */ +const FLAGS_NOTHING = `export default async function () { + return []; +} +`; + +/** Reaches the check and dies there, which is not "found nothing". */ +const THROWS = `export default async function () { + throw new Error("check exploded"); +} +`; + +const RULE = "no-eval"; + +function ruleDirectory(): string { + return join(cwd, ".taskless", "rules", "runtime", RULE); +} + +/** Write the rule itself: one capture and one `check.ts`, and no fixtures. */ +async function writeRule(check: string = FLAGS_DYNAMIC_EVAL): Promise { + const directory = ruleDirectory(); + await mkdir(join(directory, "captures"), { recursive: true }); + await writeFile(join(directory, "captures", "eval.yml"), EVAL_CAPTURE); + await writeFile(join(directory, "check.ts"), check); +} + +/** + * A fixture case: a DIRECTORY holding source, which is the `root` the check is + * given. + * + * Two axes, and keeping them apart is the point of D8. `matching` decides + * whether the NARROW finds anything, which is a property of the fixture; + * `flagged` decides whether the CHECK reports on what the narrow found, which + * is a property of the rule. A case can match and not be flagged (the useful + * `pass/` case), or fail to match at all (a fixture defect in either bucket). + */ +async function writeCase( + bucket: "pass" | "fail", + name: string, + options: { matching?: boolean; flagged?: boolean } = {} +): Promise { + const matching = options.matching ?? true; + const flagged = options.flagged ?? bucket === "fail"; + const directory = join(ruleDirectory(), ".tests", bucket, name); + await mkdir(directory, { recursive: true }); + const source = matching + ? flagged + ? "const input = globalThis.userInput;\neval(input);\n" + : 'eval("1 + 1");\n' + : "const total = 1 + 1;\n"; + await writeFile(join(directory, "sample.ts"), source); + return directory; +} + +async function testRule(...extra: string[]) { + return runCli([ + "test", + `.taskless/rules/runtime/${RULE}`, + "-d", + cwd, + ...extra, + ]); +} + +async function testRuleJson(...extra: string[]): Promise { + const { stdout } = await testRule("--json", ...extra); + return JSON.parse(stdout) as Report; +} + +beforeEach(async () => { + cwd = await mkdtemp(join(tmpdir(), "tskl-rt-runner-")); + await runCli(["init", "--no-interactive", "-d", cwd]); +}); + +afterEach(async () => { + await rm(cwd, { recursive: true, force: true }); +}); + +/** + * 5.1 — the defect itself. + * + * With no blessed signature and no escape flag the fixtures cannot run. That is + * not a failure of the rule, but it is emphatically not a pass, and the two + * assertions below are the ones that were false before the runner existed. + */ +describe("a runtime rule whose fixtures did not run", () => { + it("is not reported as passing, and prints no tick", async () => { + await writeRule(); + await writeCase("fail", "uses-eval"); + await writeCase("pass", "no-eval"); + + const { stdout, exitCode } = await testRule(); + + expect(stdout).not.toContain(`✓ runtime/${RULE}`); + // A refusal by policy is not the rule's fault, so it must not fail either. + expect(exitCode).toBe(0); + expect(stdout).not.toContain("1 rule(s) tested"); + }); + + it("says the fixtures did not run, and names the flag that would run them", async () => { + await writeRule(); + await writeCase("fail", "uses-eval"); + await writeCase("pass", "no-eval"); + + const { stdout } = await testRule(); + + expect(stdout).toContain("did not run"); + expect(stdout).toContain("--dangerously-run-scripts"); + }); + + it("offers the flag as the only route, and never sends the author to auth", async () => { + // `test` does not reconcile, so authenticating changes nothing here and + // saying otherwise would send an author to a login that cannot help: a + // locally authored rule has no signature and never will. The flag is the + // whole gate, so it has to be the whole message. + await writeRule(); + await writeCase("fail", "uses-eval"); + await writeCase("pass", "no-eval"); + + const { stdout } = await testRule(); + + expect(stdout).toContain("--dangerously-run-scripts"); + expect(stdout).not.toContain("auth login"); + expect(stdout).not.toContain("not authenticated"); + expect(stdout).not.toContain("server verification"); + }); + + it("reports ran: false in --json, and does not report ok: true", async () => { + await writeRule(); + await writeCase("fail", "uses-eval"); + await writeCase("pass", "no-eval"); + + const report = await testRuleJson(); + const rule = report.rules[0]; + + expect(rule?.engine).toBe("runtime"); + expect(rule?.ran).toBe(false); + expect(rule?.ok).not.toBe(true); + // The envelope still succeeds: nothing here is a defect in the rule. + expect(report.ok).toBe(true); + }); +}); + +/** 3.3 / 3.4 — the gate is the gate, and nothing about a fixture softens it. */ +describe("the execution gate", () => { + it("executes nothing for an unblessed rule", async () => { + // A check that writes a file is the only way to prove non-execution: the + // absence of findings is what a gated-out run and a clean run share. + const witness = join(cwd, "witness.txt"); + await writeRule( + `import { writeFileSync } from "node:fs"; +export default async function () { + writeFileSync(${JSON.stringify(witness)}, "ran"); + return []; +} +` + ); + await writeCase("fail", "uses-eval"); + await writeCase("pass", "no-eval"); + + await testRule(); + + await expect( + import("node:fs/promises").then((fs) => fs.readFile(witness, "utf8")) + ).rejects.toThrow(); + }); + + it("does not exempt a rule for its id", async () => { + // The demo rule id gets no special treatment; only the flag or a blessing + // does. Same assertion as above under the name a bypass would have used. + await writeRule(); + await writeCase("fail", "uses-eval"); + await writeCase("pass", "no-eval"); + + const report = await testRuleJson(); + + expect(report.rules[0]?.ran).toBe(false); + }); + + it("runs the fixtures under --dangerously-run-scripts, with the warning", async () => { + await writeRule(); + await writeCase("fail", "uses-eval"); + await writeCase("pass", "no-eval"); + + const { stdout, stderr, exitCode } = await testRule( + "--dangerously-run-scripts" + ); + + expect(stderr).toContain("--dangerously-run-scripts is executing runtime"); + expect(stdout).toContain(`✓ runtime/${RULE}`); + expect(exitCode).toBe(0); + }); +}); + +/** 5.2 / 5.3 — the two directions a fixture can break. */ +describe("fixture directions", () => { + it("fails a fail/ case that produced no findings", async () => { + await writeRule(FLAGS_NOTHING); + await writeCase("fail", "uses-eval"); + await writeCase("pass", "no-eval"); + + const report = await testRuleJson("--dangerously-run-scripts"); + const rule = report.rules[0]; + + expect(rule?.ran).toBe(true); + expect(rule?.ok).toBe(false); + expect(rule?.errors.join("\n")).toContain("fail/uses-eval"); + expect(report.ok).toBe(false); + }); + + it("fails a pass/ case that produced findings", async () => { + // The indiscriminate rule: it flags every match, so the `pass/` case + // reaches the check and is reported anyway. That is the rule the `pass/` + // bucket exists to catch, and nothing but running it can. + await writeRule(FLAGS_EVERY_MATCH); + await writeCase("fail", "uses-eval"); + await writeCase("pass", "literal-eval"); + + const report = await testRuleJson("--dangerously-run-scripts"); + const rule = report.rules[0]; + + expect(rule?.ran).toBe(true); + expect(rule?.ok).toBe(false); + expect(rule?.errors.join("\n")).toContain("pass/literal-eval"); + }); +}); + +/** + * 2.4 / 2.5 — a case that never reached the check. + * + * The `pass/` half is the quiet one and is the reason this is not just a + * kinder message on the `fail/` side: such a case reads as a clean pass while + * proving only that the narrow did not match. + */ +describe("a case that never reaches the check", () => { + it("is a fixture defect in fail/, not a rule that stopped firing", async () => { + await writeRule(); + await writeCase("fail", "matches-nothing", { matching: false }); + await writeCase("pass", "no-eval"); + + const report = await testRuleJson("--dangerously-run-scripts"); + const errors = report.rules[0]?.errors.join("\n") ?? ""; + + expect(report.rules[0]?.ok).toBe(false); + expect(errors).toContain("fail/matches-nothing"); + expect(errors).toContain("check.ts never ran"); + // The wrong message is the one that sends the author to the check. + expect(errors).not.toContain("fail fixture did not fire"); + }); + + it("is a fixture defect in pass/ too", async () => { + await writeRule(); + await writeCase("fail", "uses-eval"); + await writeCase("pass", "matches-nothing", { matching: false }); + + const report = await testRuleJson("--dangerously-run-scripts"); + const errors = report.rules[0]?.errors.join("\n") ?? ""; + + expect(report.rules[0]?.ok).toBe(false); + expect(errors).toContain("pass/matches-nothing"); + expect(errors).toContain("check.ts never ran"); + }); +}); + +/** 2.3 — a check that throws is not a check that found nothing. */ +describe("a check that throws", () => { + it("is reported as the check failing, not as an empty result", async () => { + await writeRule(THROWS); + await writeCase("fail", "uses-eval"); + await writeCase("pass", "no-eval"); + + const report = await testRuleJson("--dangerously-run-scripts"); + const errors = report.rules[0]?.errors.join("\n") ?? ""; + + expect(report.rules[0]?.ok).toBe(false); + expect(errors).toContain("check failed on fail/uses-eval"); + expect(errors).toContain("check exploded"); + // A throw in `fail/` must not be scored as the case firing correctly, and + // a throw in `pass/` must not be scored as the case staying quiet. + expect(errors).toContain("check failed on pass/no-eval"); + }); +}); + +/** 5.4 — coverage, where only `both` reaches a pass. */ +describe("fixture coverage", () => { + it("passes with both buckets populated", async () => { + await writeRule(); + await writeCase("fail", "uses-eval"); + await writeCase("pass", "no-eval"); + + const report = await testRuleJson("--dangerously-run-scripts"); + + expect(report.rules[0]?.ok).toBe(true); + expect(report.rules[0]?.ran).toBe(true); + }); + + it("fails with only fail/ cases", async () => { + await writeRule(); + await writeCase("fail", "uses-eval"); + + const report = await testRuleJson("--dangerously-run-scripts"); + + expect(report.rules[0]?.ok).toBe(false); + expect(report.rules[0]?.errors.join("\n")).toContain("only fail/"); + }); + + it("fails with only pass/ cases", async () => { + await writeRule(); + await writeCase("pass", "no-eval"); + + const report = await testRuleJson("--dangerously-run-scripts"); + + expect(report.rules[0]?.ok).toBe(false); + expect(report.rules[0]?.errors.join("\n")).toContain("only pass/"); + }); + + it("fails with no fixtures at all", async () => { + await writeRule(); + + const report = await testRuleJson("--dangerously-run-scripts"); + + expect(report.rules[0]?.ok).toBe(false); + expect(report.rules[0]?.errors.join("\n")).toContain("has no fixtures"); + }); +}); + +/** 5.5 — a bucket that cannot be read is not a bucket holding nothing. */ +describe("an unreadable bucket", () => { + it("is an error rather than an empty bucket", async () => { + await writeRule(); + await writeCase("fail", "uses-eval"); + const passBucket = join(ruleDirectory(), ".tests", "pass"); + await mkdir(passBucket, { recursive: true }); + await chmod(passBucket, 0o000); + + try { + const report = await testRuleJson("--dangerously-run-scripts"); + + expect(report.rules[0]?.ok).toBe(false); + // The wrong answer is "only fail/ fixtures": that reads as a rule the + // author half-wrote, when the pass cases may well be sitting right there. + expect(report.rules[0]?.errors.join("\n")).not.toContain("only fail/"); + } finally { + await chmod(passBucket, 0o755); + } + }); + + it("refuses a loose file in a bucket, naming it", async () => { + await writeRule(); + await writeCase("fail", "uses-eval"); + const passBucket = join(ruleDirectory(), ".tests", "pass"); + await mkdir(passBucket, { recursive: true }); + await writeFile(join(passBucket, "loose.ts"), "const x = 1;\n"); + + const report = await testRuleJson("--dangerously-run-scripts"); + + expect(report.rules[0]?.ok).toBe(false); + expect(report.rules[0]?.errors.join("\n")).toContain("loose.ts"); + }); +}); diff --git a/packages/cli/test/runtime-fixtures.test.ts b/packages/cli/test/runtime-fixtures.test.ts new file mode 100644 index 00000000..9edda9a3 --- /dev/null +++ b/packages/cli/test/runtime-fixtures.test.ts @@ -0,0 +1,124 @@ +import { chmod, mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { readRuntimeFixtures } from "../src/rules/runtime/fixtures"; + +/** + * Reading a runtime rule's fixture cases. + * + * The buckets are read independently and every entry must be a directory. Both + * are load-bearing rather than tidy: a bucket that reads as empty when it is + * unreadable makes a two-sided rule look one-sided, and a case silently skipped + * is a fixture an author wrote that never ran and that nothing mentioned. + */ + +const RULE = "no-eval"; + +async function caseDirectory( + cwd: string, + bucket: "pass" | "fail", + name: string +): Promise { + const directory = join( + cwd, + ".taskless", + "rules", + "runtime", + RULE, + ".tests", + bucket, + name + ); + await mkdir(directory, { recursive: true }); + await writeFile(join(directory, "sample.ts"), "const x = 1;\n", "utf8"); + return directory; +} + +describe("reading runtime fixture cases", () => { + let cwd: string; + + beforeEach(async () => { + cwd = await mkdtemp(join(tmpdir(), "tskl-rt-fixtures-")); + }); + + afterEach(async () => { + await rm(cwd, { recursive: true, force: true }); + }); + + it("reads both buckets and reports the case directories as roots", async () => { + const failRoot = await caseDirectory(cwd, "fail", "flags-it"); + const passRoot = await caseDirectory(cwd, "pass", "leaves-it"); + + const { cases, coverage } = await readRuntimeFixtures(cwd, RULE); + + expect(coverage).toBe("both"); + // The root IS the case directory: that is what the harness is handed. + expect(cases).toEqual( + expect.arrayContaining([ + { bucket: "fail", name: "flags-it", root: failRoot }, + { bucket: "pass", name: "leaves-it", root: passRoot }, + ]) + ); + }); + + it.each([ + ["both", ["fail", "pass"]], + ["fail-only", ["fail"]], + ["pass-only", ["pass"]], + ["none", []], + ] as const)("classifies coverage as %s", async (expected, buckets) => { + for (const bucket of buckets) { + await caseDirectory(cwd, bucket, "case-1"); + } + + const { coverage } = await readRuntimeFixtures(cwd, RULE); + expect(coverage).toBe(expected); + }); + + it("treats a missing bucket as empty rather than as a failure", async () => { + // Only `fail/` exists. A rule mid-authoring is not a broken read. + await caseDirectory(cwd, "fail", "case-1"); + + const { cases, coverage } = await readRuntimeFixtures(cwd, RULE); + expect(coverage).toBe("fail-only"); + expect(cases).toHaveLength(1); + }); + + it("refuses a loose file, naming it, rather than skipping it", async () => { + await caseDirectory(cwd, "fail", "case-1"); + await writeFile( + join(cwd, ".taskless/rules/runtime", RULE, ".tests/fail/loose.ts"), + "const x = 1;\n", + "utf8" + ); + + // Skipping it would leave an author with a fixture that never ran and that + // nothing mentioned, which is the failure this tier keeps producing. + await expect(readRuntimeFixtures(cwd, RULE)).rejects.toThrow(/loose\.ts/); + }); + + it("does not read an unreadable bucket as an empty one", async () => { + // The discrimination that matters. If this swallowed the permission error, + // the rule below would report `fail-only` — a one-sided rule — when in + // truth its pass side was simply unreadable. + await caseDirectory(cwd, "fail", "case-1"); + const passBucket = join( + cwd, + ".taskless/rules/runtime", + RULE, + ".tests/pass" + ); + await mkdir(passBucket, { recursive: true }); + await chmod(passBucket, 0o000); + + try { + await expect(readRuntimeFixtures(cwd, RULE)).rejects.toThrow(); + } finally { + // Restore before cleanup, or `rm` cannot remove it either. + await chmod(passBucket, 0o755); + } + }); +});