fix(review-log): record a notes-only /ship pass clean; name the cause and repair when a pass's tree moved (GST-430)

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

changelog.d/gst-430-notes-only-pass.md

@@ -0,0 +1,9 @@
+**A /ship review pass that only left notes now binds at Step 11.5.** A pass whose findings were all fixed, deferred or noted has no open defect, but a driver could still log it as `issues_found`. Step 11.5 then refused it, and the only way out was an extra rebind pass (CAPY-2, qm#2837). When the tree changes during a pass, the refusal now names the start and end trees and the cheapest repair (GST-430).
+
+### Fixed
+- **`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 action is `fixed`, `auto-fixed`, `deferred` or `noted`. A skipped finding, a gate finding left open, a finding with no action, or an incomplete pass keeps the claimed status.
+- **Findings files** (`--findings`) count `auto-fixed`, `deferred` and `noted` actions as resolved, the same as `fixed`.
+- **`review_freshness`** for a pass whose tree moved now reads `content changed during review: the tree moved from <start> to <end> while <skill> 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/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, a finding with no action, and an incomplete pass keep `issues_found`. Also checks the reason a pass gets when its tree moved.

docs/troubleshooting.md

@@ -546,6 +546,25 @@ local `origin/<base>` at `<rev>`.
 **Fix.** Run `git fetch origin <base>` outside the sandbox and rerun /review if
 the base has moved.
 
+<a id="review-content-changed"></a>
+### `review_freshness: STALE content changed during review: the tree moved from <start> to <end> while <skill> ran ...` / **Review records missing or mismatched**
+
+**Meaning.** The working tree changed between the pass's `gstack-review-log --start` and
+its logged row: an edit, commit or merge landed while reviewers were still reading. The
+reviewers may not have seen that change, so the row's `review_binding.state` is `changed`.
+Step 11.5 cannot bind it and inserts `9 → 10 → 11 → 11.5`. Logging the same results again
+cannot repair it.
+
+**Fix.** Keep the checkout frozen (no edits, commits, merges or pushes) from a pass's start
+until every reviewer in it has returned and its row is logged. If the mid-pass change was
+your own fix to that pass's findings, log the pass with `"converged":false` and the fix
+as `"action":"fixed"`. `gstack-review-plan` then picks the repeat: one fix-check reading
+the touched hunks when every fix is below the blocking floor, and 11.5 accepts that
+fix-check record as Step 9's. Rerun Step 11's native pass on the same frozen tree.
+
+**Expected result.** `gstack-review-read` shows both records at one tree with
+`review_freshness` `CURRENT`, and Step 11.5 continues to Step 12.
+
 <a id="review-fingerprint-tmpdir"></a>
 ### `gstack-wtree: cannot create a temp index under <dir>; set TMPDIR to a writable directory` / `gstack-review-log: cannot create a private object directory under <dir>; no working-tree fingerprint. Set TMPDIR to a writable directory.`
 

lib/headless-artifacts.ts

@@ -548,7 +548,7 @@ export interface DerivedReviewStatus {
 export function isOpenFinding(row: Record<string, unknown>): boolean {
   if (typeof row.disposition === 'string') return !RESOLVED.includes(row.disposition);
   if (typeof row.status === 'string') return !['resolved', 'fixed', 'closed'].includes(row.status);
-  return row.action !== 'fixed';
+  return !['fixed', 'auto-fixed', 'deferred', 'noted'].includes(row.action as string);
 }
 export function isCriticalFinding(row: Record<string, unknown>): boolean {
   return typeof row.severity === 'string' && row.severity.toLowerCase() === 'critical';

lib/review-evidence.ts

@@ -2,7 +2,7 @@ import { createHash } from 'node:crypto';
 import { spawnSync } from 'node:child_process';
 import { closeSync, constants, fstatSync, lstatSync, mkdirSync, openSync, readFileSync, unlinkSync, writeFileSync } from 'node:fs';
 import { join } from 'node:path';
-import { deriveReviewStatus, readJsonl } from './headless-artifacts';
+import { deriveReviewStatus, isOpenFinding, readJsonl } from './headless-artifacts';
 import { resultLine, type ResultCodeName } from './result-codes';
 
 const DIFF_REVIEWS = new Set(['review', 'review-fix-check', 'adversarial-review', 'codex-review', 'design-review-lite', 'ship']);
@@ -271,6 +271,12 @@ export function bindReview(rec: Record<string, any>, token: string, env = proces
   if (env.GSTACK_STAMP_TREE) rec.tree = env.GSTACK_STAMP_TREE;
   if (env.GSTACK_STAMP_DIRTY) rec.dirty = env.GSTACK_STAMP_DIRTY === 'true';
   stampGateFindings(rec.findings, env.GSTACK_STAMP_SESSION_KIND);
+  // A /ship pass whose findings were all fixed, deferred or noted left no open defect (review-army item 6), so a
+  // claimed issues_found is recorded clean with zero counts and Step 11.5 can bind it (GST-430).
+  if (!findingsPath && (rec.skill === 'review' || rec.skill === 'review-fix-check') && rec.status === 'issues_found' && rec.completed === true &&
+      Array.isArray(rec.findings) && rec.findings.length > 0 && rec.findings.every(f => record(f) && typeof f.action === 'string' && !isOpenFinding(f))) {
+    Object.assign(rec, { status: 'clean', issues_found: 0, critical: 0, informational: 0 });
+  }
   if (!DIFF_REVIEWS.has(rec.skill)) {
     if (env.GSTACK_STAMP_WTREE) rec.wtree = env.GSTACK_STAMP_WTREE;
     return rec;
@@ -326,7 +332,10 @@ export function reviewFreshness(rec: Record<string, any>, currentWtree: string):
   if (!DIFF_REVIEWS.has(rec.skill)) return;
   if (rec.skill === 'ship') return { status: 'UNVERIFIED', reason: 'ship telemetry is not a review pass' };
   const binding = rec.review_binding;
-  if (binding?.state === 'changed') return { status: 'STALE', reason: 'content changed during review' };
+  if (binding?.state === 'changed') {
+    const short = (w: unknown) => typeof w === 'string' ? w.slice(0, 12) : 'unknown';
+    return { status: 'STALE', reason: `content changed during review: the tree moved from ${short(binding.start_wtree)} to ${short(binding.end_wtree)} while ${rec.skill} ran (an edit, commit or merge landed before its row was logged), so the record cannot bind. Repair: freeze the checkout until every reviewer returns, then rerun the steps 11.5 names on the frozen tree; when the edit was your own fix, log it as a fix so gstack-review-plan picks the repeat depth (docs/troubleshooting.md#review-content-changed)` };
+  }
   if (rec.scope === 'fix-range') return { status: 'UNVERIFIED', reason: 'fix-range pass; only a later full pass can clear review' };
   if (binding?.state !== 'verified' || rec.completed !== true || rec.converged !== true ||
       !rec.wtree || binding.start_wtree !== rec.wtree || binding.end_wtree !== rec.wtree) {

test/review-notes-only-pass.test.ts

@@ -0,0 +1,84 @@
+/**
+ * GST-430 (CAPY-2, qm#2837): a /ship review pass whose only findings were notes or deferrals has no
+ * open defect, so the logger records it clean with issues_found 0 and Step 11.5 can bind it. A finding
+ * still open (skipped, gate open, no action) keeps the claimed status. A row whose tree moved during
+ * the pass names that cause and the cheapest repair.
+ */
+import { describe, test, expect, beforeEach, afterEach } from 'bun:test';
+import { spawnSync } from 'node:child_process';
+import * as fs from 'node:fs';
+import * as os from 'node:os';
+import * as path from 'node:path';
+import { isOpenFinding } from '../lib/headless-artifacts';
+import { reviewFreshness } from '../lib/review-evidence';
+
+const ROOT = path.resolve(import.meta.dir, '..');
+const BIN = path.join(ROOT, 'bin', 'gstack-review-log');
+
+let home: string;
+beforeEach(() => { home = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-notes-only-')); });
+afterEach(() => { fs.rmSync(home, { recursive: true, force: true }); });
+
+function run(args: string[]) {
+  const r = spawnSync(BIN, args, { cwd: ROOT, encoding: 'utf8', timeout: 60_000, env: { ...process.env, GSTACK_HOME: home, GSTACK_SESSION_KIND: 'unattended' } });
+  return { code: r.status, err: r.stderr, out: r.stdout };
+}
+function lastRow(): any {
+  const projects = path.join(home, 'projects');
+  const rows = fs.readdirSync(projects).flatMap(slug => fs.readdirSync(path.join(projects, slug)).filter(f => f.endsWith('-reviews.jsonl'))
+    .flatMap(f => fs.readFileSync(path.join(projects, slug, f), 'utf8').trim().split('\n').map(l => JSON.parse(l))));
+  return rows.at(-1);
+}
+const note = (i: number, action: string, extra: Record<string, unknown> = {}) =>
+  ({ fingerprint: `lib/a.ts:${i}:testing`, severity: 'INFORMATIONAL', action, locus: 'diff', kind: 'test-gap', blocking: false, raised_by: 'specialist-batched', ...extra });
+const pass = (findings: unknown[], extra: Record<string, unknown> = {}) => JSON.stringify({
+  skill: 'review', via: 'ship', timestamp: '2026-10-31T00:00:00Z', status: 'issues_found', issues_found: findings.length, critical: 0,
+  informational: findings.length, quality_score: 9, specialists: {}, findings, completed: true, converged: true, cycles: 0, pass: 2,
+  review_scope: 'frozen', range_base: null, ...extra,
+});
+
+describe('a notes-only /ship review pass (GST-430)', () => {
+  // Value: protects=Step 11.5 binding a pass whose 3 notes are not defects; catches=a notes-only pass logged as
+  // issues_found and left UNVERIFIED, which cost CAPY-2 an extra rebind pass.
+  test('three noted findings are recorded clean with 0 issues and read CURRENT', () => {
+    const token = run(['--start', 'review']).out.trim();
+    const r = run([pass([note(1, 'noted'), note(2, 'noted'), note(3, 'deferred', { issue: null })]), '--finish', token]);
+    expect(r.code).toBe(0);
+    const row = lastRow();
+    expect([row.status, row.issues_found, row.critical, row.informational, row.review_binding.state]).toEqual(['clean', 0, 0, 0, 'verified']);
+    expect(reviewFreshness(row, row.wtree)).toEqual({ status: 'CURRENT', reason: 'completed clean pass on unchanged content' });
+  });
+
+  // Value: protects=open defects still blocking 11.5; catches=a normalization that clears a skipped finding, a
+  // gate finding the unattended logger forced open, a finding with no action, or an incomplete pass.
+  test('negative controls keep issues_found', () => {
+    const cases: Array<[string, string]> = [
+      ['skipped', pass([note(1, 'noted'), note(2, 'skipped')])],
+      ['gate open', pass([note(1, 'noted'), note(2, 'kept', { gate: 'RH1', gate_id: 'g', reason: 'r' })])],
+      ['no action', pass([note(1, 'noted'), { fingerprint: 'lib/a.ts:9:testing', severity: 'CRITICAL' }])],
+      ['incomplete', pass([note(1, 'noted')], { completed: false })],
+    ];
+    for (const [name, row] of cases) {
+      expect(run([row]).code).toBe(0);
+      expect([name, lastRow().status]).toEqual([name, 'issues_found']);
+    }
+  });
+
+  test('isOpenFinding: noted, deferred and auto-fixed actions are not open; skipped and missing actions are', () => {
+    expect(['fixed', 'auto-fixed', 'deferred', 'noted', 'skipped', undefined].map(action => isOpenFinding({ action })))
+      .toEqual([false, false, false, false, true, true]);
+  });
+});
+
+describe('a tree that changed during the pass (GST-430)', () => {
+  // Value: protects=a refusal that says why the record cannot bind and what the cheapest repair is; catches=the
+  // bare 'content changed during review' that sent CAPY-2 through a ~$20 full pass without saying why.
+  test('the STALE reason names both trees, the cause and the repair', () => {
+    const s = 'a'.repeat(64), e = 'b'.repeat(64);
+    const fresh = reviewFreshness({ skill: 'adversarial-review', review_binding: { state: 'changed', start_wtree: s, end_wtree: e } }, e);
+    expect(fresh?.status).toBe('STALE');
+    expect(fresh?.reason).toContain(`content changed during review: the tree moved from ${s.slice(0, 12)} to ${e.slice(0, 12)} while adversarial-review ran`);
+    expect(fresh?.reason).toContain('freeze the checkout until every reviewer returns');
+    expect(fresh?.reason).toContain('docs/troubleshooting.md#review-content-changed');
+  });
+});