From 04a0ceea9ae0508ec8eed81b550b75c121cb956d Mon Sep 17 00:00:00 2001 From: garrytan Date: Sat, 31 Oct 2026 13:05:31 +0000 Subject: [PATCH] fix: a claimed clean review row with any unresolved non-gate CRITICAL finding reads UNVERIFIED (pass 2 red-team) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- changelog.d/gst-430-notes-only-pass.md | 4 ++-- lib/review-evidence.ts | 3 ++- test/review-notes-only-pass.test.ts | 3 +++ 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/changelog.d/gst-430-notes-only-pass.md b/changelog.d/gst-430-notes-only-pass.md index d088a157b..32fb83dff 100644 --- a/changelog.d/gst-430-notes-only-pass.md +++ b/changelog.d/gst-430-notes-only-pass.md @@ -9,11 +9,11 @@ - **`gstack-tree-receipt`** fails closed (`EVIDENCE_IDENTITY_UNKNOWN`) when git cannot produce the diff between the gated and current heads, and names git's reason. Before, it read a failed diff as no changes. - **`gstack-review-log`** records a completed `review` or `review-fix-check` row claimed as `issues_found` as `clean` with zero counts when every finding's logger-stamped action is `fixed` or `auto-fixed`, or `deferred` or `noted` on a finding marked `blocking: false` that is not CRITICAL, and the findings account for the claimed issue and critical counts. A skipped finding, a gate finding left open, a CRITICAL or blocking note, a finding with no action, or an incomplete pass keeps the claimed status. - **Findings files** (`--findings`) count `auto-fixed` as resolved, the same as `fixed`, and `deferred` or `noted` only below the floor. Their gate rows are stamped like inline ones, so an unattended run's gate stays open whatever its status or disposition says. -- **`review_freshness`** reads a claimed clean row as UNVERIFIED when a finding is a gate left `open` or a CRITICAL or blocking deferral or note. +- **`review_freshness`** reads a claimed clean row as UNVERIFIED when a finding is a gate left `open`, a CRITICAL or blocking deferral or note, or any other unresolved CRITICAL finding. - **`review_freshness`** for a pass whose tree moved now reads `content changed during review: the tree moved from to while ran …`, followed by the repair: keep the checkout frozen until every reviewer returns, then rerun on the frozen tree (docs/troubleshooting.md#review-content-changed). ### Tests - `test/ship-rereview-triggers.test.ts`: preflight on a founderstar-style adjacent edit prints the scoped-review `NEXT: carry route`, and its dry run writes no packet. `test/ship-control-flow.test.ts`: Step 1.4 and Step 16 stages 2 and 3 name the routes. - `test/ship-receipt-carry-large-diff.test.ts`: a PR that adds 6,000 receipts under a long directory name writes a receipt with its product patch-id and carries across a base merge. Negative control: one rescored receipt still refuses with `RECEIPT_CARRY_PRODUCT_CHANGED`. -- `test/review-notes-only-pass.test.ts`: a pass with three notes is recorded clean and reads CURRENT. Negative controls: a skipped finding, an open gate finding (also under a caller's resolved status), a CRITICAL or blocking note, a note with no `blocking` field, counts the findings do not cover, a finding with no action, and an incomplete pass keep `issues_found`. A claimed clean row with an open gate or a CRITICAL deferral is not CURRENT, while kept and restored gates, skipped advisories and an INFORMATIONAL deferral stay CURRENT. A `--findings` gate row stays open in an unattended run. Also checks the reason a pass gets when its tree moved. +- `test/review-notes-only-pass.test.ts`: a pass with three notes is recorded clean and reads CURRENT. Negative controls: a skipped finding, an open gate finding (also under a caller's resolved status), a CRITICAL or blocking note, a note with no `blocking` field, counts the findings do not cover, a finding with no action, and an incomplete pass keep `issues_found`. A claimed clean row with an open gate, a CRITICAL deferral, or a skipped or action-less CRITICAL finding is not CURRENT, while kept and restored gates, skipped advisories and an INFORMATIONAL deferral stay CURRENT. A `--findings` gate row stays open in an unattended run. Also checks the reason a pass gets when its tree moved. - `test/ship-receipt-carry-large-diff.test.ts` also: the patch-id file lives under the git directory (an unwritable TMPDIR still carries; an unwritable spot refuses by name), and a stale directory is swept. `test/train-gate.test.ts`: a diff git cannot produce refuses the tree receipt with git's reason. `test/ship-rereview-triggers.test.ts`: a dry run the receipt would refuse anyway names that refusal. diff --git a/lib/review-evidence.ts b/lib/review-evidence.ts index cfbb8f730..60dc4e669 100644 --- a/lib/review-evidence.ts +++ b/lib/review-evidence.ts @@ -355,7 +355,8 @@ export function reviewFreshness(rec: Record, currentWtree: string): if (rec.status !== 'clean' || open > 0 || rec.critical > 0 || (rec.skill === 'codex-review' && rec.findings > (rec.findings_fixed ?? 0)) || (Array.isArray(rec.findings) && rec.findings.some(f => record(f) && (f.action === 'open' || - ((f.action === 'deferred' || f.action === 'noted') && (f.blocking === true || isCriticalFinding(f))))))) { + ((f.action === 'deferred' || f.action === 'noted') && (f.blocking === true || isCriticalFinding(f))) || + (!f.gate && isCriticalFinding(f) && isOpenByAction(f)))))) { return { status: 'UNVERIFIED', reason: 'review has unresolved findings or did not finish clean' }; } return { status: 'CURRENT', reason: 'completed clean pass on unchanged content' }; diff --git a/test/review-notes-only-pass.test.ts b/test/review-notes-only-pass.test.ts index 383d5cf9c..fb58821e6 100644 --- a/test/review-notes-only-pass.test.ts +++ b/test/review-notes-only-pass.test.ts @@ -91,6 +91,9 @@ describe('a notes-only /ship review pass (GST-430)', () => { expect(reviewFreshness(row([note(1, 'deferred', { severity: 'CRITICAL', issue: null })]), w)?.status).toBe('UNVERIFIED'); expect(reviewFreshness(row([note(1, 'deferred', { blocking: true })]), w)?.status).toBe('UNVERIFIED'); expect(reviewFreshness(row([note(1, 'open', { gate: 'RH1' })]), w)?.status).toBe('UNVERIFIED'); + expect([note(1, 'skipped', { severity: 'CRITICAL' }), { fingerprint: 'lib/a.ts:2:x', severity: 'CRITICAL' }, note(1, 'mystery', { severity: 'CRITICAL' })] + .map(f => reviewFreshness(row([f]), w)?.status)).toEqual(['UNVERIFIED', 'UNVERIFIED', 'UNVERIFIED']); + expect(reviewFreshness(row([note(1, 'kept', { gate: 'RH1', reason: 'r', actor: 'interactive', severity: 'CRITICAL' })]), w)?.status).toBe('CURRENT'); const gate = (action: string) => note(1, action, { gate: 'RH1', reason: 'r', actor: 'interactive' }); expect([gate('kept'), gate('restored'), note(1, 'skipped'), note(1, 'deferred', { blocking: undefined }), note(1, 'noted')] .map(f => reviewFreshness(row([f]), w)?.status)).toEqual(['CURRENT', 'CURRENT', 'CURRENT', 'CURRENT', 'CURRENT']);