From 82baaa0b67850896831adfb847d158bb4080965a Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 29 Jul 2026 09:35:54 -0700 Subject: [PATCH] test(U9): give the FN-7720 "no fabricated verdict" invariant a real assertion (#2541) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **U9, PR6.** Test-only, one file, no production change. Found while characterizing the reviewer lane (U9 is "review *and* merge"; PRs 1–5 covered merge). ## A test named for an invariant it does not assert `store-bypass-review.test.ts` has a case called *"rewrites the failed step to skipped with bypass audit metadata **and no fabricated verdict**"*, containing `expect(result?.verdict).toBeUndefined()`. Its fixture sets `verdict: undefined`. **The assertion is vacuous.** Deleting `delete bypassed.verdict;` from `store.ts` leaves the whole suite green. Measured: `NEW-failures=0` across `store-bypass-review`, `task-merge-bypass`, `task-merge`, `legacy-adoption`. I explicitly confirmed the suite **runs rather than skips** — 9 tests via `pgDescribe` against the shared PG harness. A skipped suite produces exactly the same misleading zero, and that is the failure mode I hit earlier in this unit with a regex that matched nothing. ## Why it matters FN-7720 is explicit that a bypass writes status `skipped` and **never fabricates a reviewer verdict**. The invariant only has teeth when the failed step *carries* a verdict — which is the actual risk case: a reviewer says `REVISE`, an operator bypasses, and the verdict rides forward onto a `skipped` step. Every downstream reader then sees a reviewer verdict attached to a step no reviewer passed. The production code is **correct**. It was simply unasserted. ## The added case is two-sided With `verdict: "REVISE"` seeded, it asserts: - the bypassed step has **no** verdict (not carried forward), and - `bypassedFromVerdict` preserves `"REVISE"` (not silently lost from the audit trail) so it fails if the clear is removed *and* if the audit field is dropped. A one-sided version would pass against a bypass that simply discards all verdict history. | Mutation | NEW failures | |---|---| | remove `delete bypassed.verdict` | **1** — this test, and only it | | drop `bypassedFromVerdict` | **1** — this test, and only it | ## Reviewer-lane characterization so far By-name coverage search done **first** this time, per the lesson from #2520: | Invariant | Verdict | |---|---| | FN-8492 orphaned pending results REWRITTEN to failed, never deleted | **covered** — `legacy-adoption.test.ts`, NEW=2; one case is literally named "NEVER deletes an orphaned entry" | | FN-7720 bypass writes status `skipped` | **covered** — NEW=1 | | FN-7720 bypass never fabricates a verdict | **was vacuous** — fixed here | Still to characterize, and stated rather than implied: review verdicts routing as graph outcomes, and provider-outage hold-in-place (no fabricated verdict on outage). Those are the next PR. ## Note on this shared checkout Earlier in this unit I used `git stash` to isolate a measurement and, because my tree was already committed-clean, the `pop` targeted the operator's stash entry. It failed safely on an untracked-file conflict and both entries are intact — but that was luck. I no longer use stash here; isolation is done by editing and restoring files directly, with `git status` asserted clean afterwards. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) --- .../src/__tests__/store-bypass-review.test.ts | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/packages/core/src/__tests__/store-bypass-review.test.ts b/packages/core/src/__tests__/store-bypass-review.test.ts index a49c4d70de..522712025d 100644 --- a/packages/core/src/__tests__/store-bypass-review.test.ts +++ b/packages/core/src/__tests__/store-bypass-review.test.ts @@ -81,6 +81,39 @@ pgDescribe("TaskStore.bypassFailedPreMergeReviewStep", () => { expect(logged).toBe(true); }); + /* + FNXC:ReviewBypass 2026-07-29-09:30 (U9): + The case above asserts `verdict` is undefined against a fixture whose verdict is + ALREADY undefined, so the assertion is vacuous: deleting `delete bypassed.verdict` + from store.ts leaves it green. Measured by mutation — NEW-failures=0 across + store-bypass-review, task-merge-bypass, task-merge and legacy-adoption. + + The invariant only has teeth when the failed step CARRIES a verdict. That is the + real risk: a reviewer said REVISE, an operator bypasses, and the verdict rides + forward onto a `skipped` step — so every downstream reader sees a reviewer verdict + attached to a step no reviewer passed. FN-7720 requires the bypass to clear it and + preserve the original only in the audit field. + */ + it("clears a real verdict off the bypassed step and keeps it only as audit history", async () => { + await seedInReviewTask("FN-BYP-VERDICT", { + workflowStepResults: [failedStep({ verdict: "REVISE", output: "reviewer asked for changes" })], + }); + + const updated = await store().bypassFailedPreMergeReviewStep("FN-BYP-VERDICT", { + reason: "operator override after reviewer outage", + actor: "operator-2", + }); + + const result = updated.workflowStepResults?.[0]; + expect(result?.status).toBe("skipped"); + // The bypass must NOT carry the reviewer's verdict onto the skipped step. + expect(result?.verdict).toBeUndefined(); + // ...but it must not lose it either: the audit field preserves what was bypassed. + expect(result?.bypassedFromVerdict).toBe("REVISE"); + expect(result?.bypassedFromStatus).toBe("failed"); + expect(result?.bypassedBy).toBe("operator-2"); + }); + it("records a run-audit event for the bypass", async () => { await seedInReviewTask("FN-BYP-002", { workflowStepResults: [failedStep()] }); await store().bypassFailedPreMergeReviewStep("FN-BYP-002", { reason: "infra failure", actor: "operator-2" });