test(U9): give the FN-7720 "no fabricated verdict" invariant a real assertion (#2541)

**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) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-07-29 09:35:54 -07:00
committed by GitHub
parent 69790dc3e7
commit 82baaa0b67

View File

@@ -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" });