diff --git a/.crew/peaches.yaml b/.crew/peaches.yaml index 6d372a3d..48463189 100644 --- a/.crew/peaches.yaml +++ b/.crew/peaches.yaml @@ -62,3 +62,42 @@ reviewer: intent before CI runs. applies_to: - "lib/public/**/*.js" + + # Full decision record (why this option over a CLAUDE.md reminder, a + # lint-checked annotation, or mutation testing; how the nondeterministic + # case is handled): docs/guides/TESTING_CONVENTIONS.md. + # + # This is a review-judgment rule, not a regex — no `pattern` field. + # PEACHES's per-chunk model reasoning applies it directly against the + # diff text, same as any other rulebook item without a mechanical + # pattern (see AGENT.md's Rulebook section: project overrides are cited + # by id, not required to be regex-detectable). + - id: clagentic-console.demonstrated-test-failure + severity: blocking + description: >- + Any new or modified test asserting a bug fix (a "regression test") + must be traceable to a genuine failure against the pre-fix code, not + merely accepted on the author's say-so. Do not pass a regression + test on the strength of a PR/commit claim alone ("verified via git + stash", "confirmed failing before the fix") — independently trace + it: identify the parent/pre-fix commit for the changed test and + confirm, from the diff and commit history available in this review, + that the assertion the test makes is actually exercised by the code + path the fix touches (the PR #394 failure mode was a test that + touched an unrelated directory a readdirSync listing never surfaced + — plausible-looking, but not connected to the bug at all). You do + not need to re-run the suite to do this; read the pre-fix version of + the test/source pair in the diff and confirm the assertion would + have failed against it. + Exception — nondeterministic/flaky failures (e.g. lr-b5d62f, a + load-dependent race that reproduces in only some full-suite runs): + a test for this class cannot be demonstrated to fail on demand by + construction. Do not block on demonstrated-failure for these; instead + require the PR to say explicitly, in the body or a commit message, + that the failure is load-dependent/nondeterministic and name the + mechanism (the race/ordering/timing condition), not just assert + "flaky, trust me". A claim of nondeterminism with no named mechanism + is itself a finding — it is exactly as unverifiable as an unproven + determinism claim and must not be used to route around this rule. + applies_to: + - "test/**/*.js" diff --git a/docs/guides/TESTING_CONVENTIONS.md b/docs/guides/TESTING_CONVENTIONS.md new file mode 100644 index 00000000..7168899c --- /dev/null +++ b/docs/guides/TESTING_CONVENTIONS.md @@ -0,0 +1,107 @@ +# Testing Conventions + +## Regression tests must be demonstrated failing pre-fix (lr-4e1242) + +### The problem + +Three PRs shipped a regression test that passed both with and without the +bug it claimed to cover: + +1. **lr-9bcd7b** — a test passed with and without the bug. +2. **lr-255e** — same. +3. **PR #394**, first attempt (lr-3ccc78) — a test that claimed to cover a + symlink-escape containment fix never called `fs.symlinkSync`. It created + an unrelated directory that a `readdirSync` listing never surfaced, so it + passed whether or not containment worked. + +The correction each time is the same and already known: verify, via `git +stash` or an equivalent pre-fix checkout, that the test genuinely **fails** +against unmodified code before the fix exists. Applied, it works — AMoS +confirmed 5 of 8 tests genuinely failed pre-fix on lr-4a13c3, and 3 of 5 on +PR #393. + +The PR #394 case is the one worth reading closely, because it shows the +convention failing even when everyone involved already knew it. The +dispatching prompt named lr-9bcd7b and lr-255e explicitly. The author +apparently believed (or claimed) the test was verified. PEACHES reviewed the +same test and its narrative described it as verifying "symlinks don't +escape base dir" — a plausible-sounding description of a test that never +exercised a symlink at all. The test was only caught because BOBBIE, on a +second pass, independently traced the pre-fix commit rather than trusting +the author's or PEACHES's read of it. + +### Why a reminder isn't a mechanism + +The convention was, at that point, transmitted entirely by a coordinator +remembering to paste it into a dispatch prompt. It held exactly as long as +someone remembered to say it, and it failed on the one round where it was +said explicitly and a human/agent reviewer still passed the vacuous test on +a plausible-sounding description rather than tracing it. Restating the +convention more prominently (a project CLAUDE.md rule, for example) does not +change that failure mode: CLAUDE.md is read by the *author*, not +independently checked by anyone downstream, and the PR #394 failure was a +**reviewer** failure, not an author-forgot-the-rule failure — the author's +claim was simply not verified. + +### The mechanism actually adopted + +**`.crew/peaches.yaml` rule `clagentic-console.demonstrated-test-failure`** +makes BOBBIE's PR #394 behavior — independently tracing the pre-fix commit +rather than accepting a claim — the standard PEACHES review step for any new +or modified file under `test/**/*.js`, not something that happens only when +a particular reviewer happens to think of it. This was chosen over three +other candidates considered for this task: + +- **A CLAUDE.md convention alone.** Rejected: this is the mechanism that + already existed and already failed at PR #394, including in the specific + case where the reminder was stated explicitly in the dispatch prompt. It + binds the author, not the reviewer, and the demonstrated failure mode was + a reviewer failure. +- **A required annotation/comment in the test file, checked by a lint + rule.** Rejected: a lint rule can only check that an annotation exists, + not that its claim is true. A fabricated "observed failing pre-fix" + comment costs the same one line as a fabricated stash claim already does + today — the rule would add a decorative artifact without adding + verification. It also conflicts with this codebase's standing convention + against decorative/provenance comments in source (lore-ID discipline): + once the claim is unverifiable by the lint itself, the comment is exactly + that — decoration, not enforcement. +- **Mutation testing on changed files.** Rejected as disproportionate for + this codebase. It would catch a broader class than the one in front of + us (regression tests that don't actually exercise the changed logic) but + at real cost: a new dependency and toolchain (`allow_new_deps` is not + granted for this), added runtime on a suite that already has a documented + flake budget (lr-b5d62f), and per-file tuning to avoid false positives on + the daemon's heavily integration-style tests (WebSocket relay, IPC, + filesystem). The narrower problem — "this specific new test would have + passed against the bug it claims to fix" — is fully covered by the + reviewer trace above at a fraction of the cost. If the class of bug this + task exists for widens (e.g. correctness bugs with no accompanying test + at all slipping through), mutation testing is worth revisiting on its own + merits, but adopting it now to solve a narrower, already-solved problem + would be disproportionate. + +PEACHES applies the rule as a review-judgment item (no regex `pattern` +field — the reviewer's model reasoning applies it directly against the +diff), consistent with how PEACHES already applies non-mechanical rulebook +entries. + +### The nondeterministic-failure case + +Not every regression test can be demonstrated failing on demand. lr-b5d62f +is a load-dependent flake in `test/daemon-bootstrap-guard.test.js` that +reproduces in only about 3 of 8 full-suite runs — by construction, a +"prove it fails before the fix" checkout cannot reliably reproduce it either. +A rule that required demonstrated failure unconditionally would either block +this legitimate class of work or, worse, teach people to write a false +"verified via stash" attestation to get past it — which is the exact +failure mode this rule exists to close, just moved one level up. + +The rule's answer: for a nondeterministic/load-dependent failure, PEACHES +does not require demonstrated failure. It requires the PR body or a commit +message to say explicitly that the failure is load-dependent/nondeterministic +**and name the actual mechanism** — the specific race, ordering, or timing +condition — not merely assert "flaky, trust me". An unnamed-mechanism +flakiness claim is treated as a finding in its own right: it is exactly as +unverifiable as an unproven determinism claim, and accepting it without +challenge would recreate the same gap this rule closes.