fix(review): gate decided stops on a composed re-rule verdict

packages/cli/src/commands/review/capture-local.incremental.test.ts

@@ -12,6 +12,7 @@
 
 import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
 import { execFileSync } from 'node:child_process';
+import { createHash } from 'node:crypto';
 import {
   mkdtempSync,
   rmSync,
@@ -477,6 +478,177 @@ describe('capture-local — round-2 regressions from the stop work', () => {
     ) as Record<string, unknown>;
     expect(sidecar['runId']).toBe('run-abc');
   });
+
+  it('stamps the fence’s binding fields — null hash when no cache was seen', () => {
+    // A first clean-tree stop saw no cache: null is the stampable value,
+    // and the compose fence fails closed on a cache file appearing since.
+    seedDirtyTree();
+    git('add', '-A');
+    git('commit', '-q', '--no-verify', '-m', 'all committed');
+    const plan = capture();
+    expect(plan['nothingToReview']).toEqual({ reason: 'clean-tree' });
+    const sidecar = JSON.parse(
+      readFileSync(join(repo, '.qwen/tmp/qwen-review-local-stop.json'), 'utf8'),
+    ) as Record<string, unknown>;
+    expect(sidecar['cachePath']).toBe(plan['cachePath']);
+    expect(sidecar['findingsHash']).toBeNull();
+  });
+
+  it('stamps the cache a cached stop saw — the ledger’s content hash', () => {
+    // The compose grant re-hashes the cache the plan names and refuses on
+    // any departure, so a ledger edited between capture and compose fails
+    // closed like a foreign stamp.
+    seedDirtyTree();
+    const cachePath = promoteCandidate(
+      capture({ model: 'model-a' }),
+      'model-a',
+    );
+    recordOpenCritical(cachePath);
+    const second = capture({ cache: cachePath, model: 'model-a' });
+    expect(second['nothingToReview']).toEqual({
+      reason: 'unchanged-since-last-round',
+    });
+    const sidecar = JSON.parse(
+      readFileSync(join(repo, '.qwen/tmp/qwen-review-local-stop.json'), 'utf8'),
+    ) as Record<string, unknown>;
+    expect(sidecar['cachePath']).toBe(second['cachePath']);
+    expect(sidecar['findingsHash']).toBe(
+      createHash('sha256').update(readFileSync(cachePath)).digest('hex'),
+    );
+  });
+
+  it('binds the ledger the stop DECIDED from — a file-form --cache outside the canonical dir', () => {
+    // The stop decision reads the `--cache`-resolved ledger; the stamp and
+    // the plan's published `cachePath` must name that same file. Stamping
+    // the canonical `.qwen/review-cache/…` path while the decision
+    // consulted a caller-named file had the fence verify a baseline the
+    // stop never saw — an ENOENT null hash over a nonexistent canonical
+    // file, an empty grant baseline, and an exit 0 over the open Critical
+    // the stop had just consumed.
+    seedDirtyTree();
+    const canonical = promoteCandidate(
+      capture({ model: 'model-a' }),
+      'model-a',
+    );
+    // Outside the repo entirely, so the hand-named copy is not a new
+    // untracked file that would itself defeat the unchanged stop.
+    const outside = join(repo, '..', `hand-named-ledger-${Date.now()}.json`);
+    writeFileSync(outside, readFileSync(canonical));
+    rmSync(canonical);
+    recordOpenCritical(outside);
+    const second = capture({ cache: outside, model: 'model-a' });
+    expect(second['nothingToReview']).toEqual({
+      reason: 'unchanged-since-last-round',
+    });
+    // ONE resolved value for every consumer: plan, sidecar, and hash.
+    expect(second['cachePath']).toBe(outside);
+    const sidecar = JSON.parse(
+      readFileSync(join(repo, '.qwen/tmp/qwen-review-local-stop.json'), 'utf8'),
+    ) as Record<string, unknown>;
+    expect(sidecar['cachePath']).toBe(outside);
+    expect(sidecar['findingsHash']).toBe(
+      createHash('sha256').update(readFileSync(outside)).digest('hex'),
+    );
+  });
+
+  it('hashes the DECISION-time ledger bytes, not a second read at stamp time', async () => {
+    // A ledger edit landing in the decision→stamp window (a concurrent
+    // round's Step-8 rewrite of the shared file) must not be baked into
+    // the stamp: the stamp and the decision are projections of ONE read.
+    // The spy makes every cache read AFTER the first return bytes with the
+    // blocker dropped — with the fix the stamp still hashes the
+    // decision-time bytes; without it the stamp followed the second read.
+    const { readFileSync: realRead } =
+      await vi.importActual<typeof import('node:fs')>('node:fs');
+    seedDirtyTree();
+    const cachePath = promoteCandidate(
+      capture({ model: 'model-a' }),
+      'model-a',
+    );
+    recordOpenCritical(cachePath);
+    const original = realRead(cachePath) as Buffer;
+    const expected = createHash('sha256').update(original).digest('hex');
+    const mutatedCache = JSON.parse(original.toString('utf8')) as Record<
+      string,
+      unknown
+    >;
+    mutatedCache['findings'] = [];
+    const mutated = Buffer.from(JSON.stringify(mutatedCache));
+    let cacheReads = 0;
+    vi.mocked(readFileSync).mockImplementation(((
+      path: unknown,
+      opts: unknown,
+    ) => {
+      if (path === cachePath) {
+        cacheReads++;
+        if (cacheReads > 1) {
+          return typeof opts === 'string' ? mutated.toString('utf8') : mutated;
+        }
+      }
+      return realRead(
+        path as Parameters<typeof realRead>[0],
+        opts as Parameters<typeof realRead>[1],
+      );
+    }) as typeof readFileSync);
+    try {
+      const second = capture({ cache: cachePath, model: 'model-a' });
+      expect(second['nothingToReview']).toEqual({
+        reason: 'unchanged-since-last-round',
+      });
+    } finally {
+      vi.mocked(readFileSync).mockRestore();
+    }
+    const sidecar = JSON.parse(
+      readFileSync(join(repo, '.qwen/tmp/qwen-review-local-stop.json'), 'utf8'),
+    ) as Record<string, unknown>;
+    expect(sidecar['findingsHash']).toBe(expected);
+  });
+
+  it('stamps the scope-emptied split into the sidecar beside the hash', () => {
+    // The `superseded` deduction reads membership off `supersededPaths`,
+    // and the plan copy is model-editable after this write — only the
+    // capture-stamped copy certifies the split, and the compose fence
+    // refuses a plan whose split departs from it.
+    seedDirtyTree();
+    const cachePath = promoteCandidate(
+      capture({ model: 'model-a' }),
+      'model-a',
+    );
+    recordOpenCritical(cachePath);
+    git('checkout', '--', '.');
+    const plan = capture({ cache: cachePath, model: 'model-a' });
+    expect(plan['nothingToReview']).toEqual({ reason: 'scope-emptied' });
+    const scope = (
+      plan['incremental'] as { scope: { supersededPaths?: string[] } }
+    ).scope;
+    const sidecar = JSON.parse(
+      readFileSync(join(repo, '.qwen/tmp/qwen-review-local-stop.json'), 'utf8'),
+    ) as Record<string, unknown>;
+    expect(sidecar['supersededPaths']).toEqual(scope.supersededPaths);
+    expect((sidecar['supersededPaths'] as string[]).length).toBeGreaterThan(0);
+  });
+
+  it('unlinks a stale stop sidecar when a later capture proves the tree moved', () => {
+    // An earlier round's sidecar at this stable name stays fence-valid
+    // (same reason, same cache, same hash) after the tree moves on — a
+    // hand-written stop plan could ride it. A capture that decides NO stop
+    // removes it: absent is the truthful state.
+    seedDirtyTree();
+    git('add', '-A');
+    git('commit', '-q', '--no-verify', '-m', 'all committed');
+    const stopped = capture();
+    expect(stopped['nothingToReview']).toEqual({ reason: 'clean-tree' });
+    expect(
+      existsSync(join(repo, '.qwen/tmp/qwen-review-local-stop.json')),
+    ).toBe(true);
+    // The tree moves; the next capture decides a real round.
+    writeFileSync(join(repo, CHANGED), 'export const moved = 1;\n');
+    const moved = capture();
+    expect(moved['nothingToReview']).toBeUndefined();
+    expect(
+      existsSync(join(repo, '.qwen/tmp/qwen-review-local-stop.json')),
+    ).toBe(false);
+  });
 });
 
 describe('capture-local — a narrower round cannot certify a wider one', () => {

packages/cli/src/commands/review/capture-local.ts

@@ -61,7 +61,8 @@ import {
   movedSince,
   hashWorktreeFiles,
   isPathProvablyAbsent,
-  readLocalCache,
+  type readLocalCache,
+  readLocalCacheFromBytes,
   revisionIdentities,
   stateIdOf,
   UNHASHABLE,
@@ -736,8 +737,24 @@ function runCaptureLocal(args: CaptureLocalArgs): void {
     args.cache !== undefined
       ? resolveCachePath(args.cache, target, sourcePath)
       : null;
+  // ONE read of the ledger's bytes: the stop DECISION below parses this
+  // buffer and the stop stamp hashes the SAME buffer — a second disk read
+  // at stamp time let a concurrent round's ledger rewrite land in the
+  // decision→stamp window and be baked into the stamp, invisible to the
+  // compose fence (which then verified a baseline the decision never
+  // consulted). Raw bytes are kept beside the parse because the stamp is
+  // sha256 of the FILE's bytes, malformed JSON included — the parse
+  // fail-quiets, the hash must not.
+  let cacheEarlyBytes: Buffer | null = null;
+  if (cachePathEarly !== null) {
+    try {
+      cacheEarlyBytes = readFileSync(cachePathEarly);
+    } catch {
+      // No cache file — the decision sees no anchor and the stamp is null.
+    }
+  }
   const cacheEarly =
-    cachePathEarly === null ? null : readLocalCache(cachePathEarly);
+    cacheEarlyBytes === null ? null : readLocalCacheFromBytes(cacheEarlyBytes);
   const vanishedPresent: readonly string[] =
     cacheEarly === null
       ? []
@@ -1146,7 +1163,43 @@ function runCaptureLocal(args: CaptureLocalArgs): void {
   // so a parent polling `qwen-review-<target>-plan.json` found nothing for
   // every file review and reported "Review did not complete" over a decided
   // round. This name is derived from the same `target` the parent derives.
+  // ONE resolved value for every consumer: the stop DECISION above read the
+  // `--cache`-resolved ledger (`cachePathEarly` — a file-form `--cache` is
+  // returned unchanged, directory form resolves the canonical basename), so
+  // the stamp below and the plan's published `cachePath` must name that
+  // same file. Stamping the canonical `.qwen/review-cache/…` path while the
+  // decision consulted a caller-named file had the fence faithfully verify
+  // a baseline the stop never saw — an ENOENT hash over a nonexistent
+  // canonical file, an empty grant baseline, and an exit 0 over the open
+  // Critical the stop had just consumed.
+  const cachePath = cachePathEarly ?? cachePathFor(target, sourcePath);
   if (nothingToReview) {
+    // The baseline's content bound into the stamp: the compose grant
+    // re-hashes the cache the plan names and refuses on any departure, so
+    // a ledger edited between capture and compose fails closed like a
+    // foreign stamp. Null is a stampable value — no cache existed at this
+    // stop, so no findings were seen, and the fence fails closed on a file
+    // appearing since. The hash is of the DECISION-time bytes when a
+    // `--cache` scoped this round — stamp and decision are projections of
+    // the one read above, so an edit landing in the decision→stamp window
+    // cannot be baked into the stamp. Only the no-`--cache` canonical
+    // path still reads the disk here: that decision consulted no ledger,
+    // so there is no decision-time buffer to prefer.
+    let findingsHash: string | null = null;
+    if (cachePathEarly !== null) {
+      findingsHash =
+        cacheEarlyBytes === null
+          ? null
+          : createHash('sha256').update(cacheEarlyBytes).digest('hex');
+    } else {
+      try {
+        findingsHash = createHash('sha256')
+          .update(readFileSync(cachePath))
+          .digest('hex');
+      } catch {
+        // No cache file at this stop.
+      }
+    }
     writeFileSync(
       tmpFile(target, 'stop.json'),
       `${JSON.stringify(
@@ -1161,12 +1214,37 @@ function runCaptureLocal(args: CaptureLocalArgs): void {
           ...(process.env['QWEN_REVIEW_RUN_ID']
             ? { runId: process.env['QWEN_REVIEW_RUN_ID'] }
             : {}),
+          // The compose fence's binding fields: the cache the grant must
+          // read, and the hash its content must still carry.
+          cachePath,
+          findingsHash,
+          // The scope-emptied split, capture-certified: the `superseded`
+          // deduction's input must be THIS list, and the plan it also
+          // rides in is model-editable after this write — a split edited
+          // between capture and compose could blanket-supersede a live
+          // blocker past a fence that binds only reason/cache/hash.
+          // Stamped in the interactive (no-run-id) shape too.
+          ...(nothingToReview.reason === 'scope-emptied'
+            ? { supersededPaths: incremental?.scope?.supersededPaths ?? [] }
+            : {}),
         },
         null,
         2,
       )}\n`,
       'utf8',
     );
+  } else {
+    // This capture proves the tree MOVED past whatever an earlier stop
+    // certified, so an earlier round's sidecar at this stable name is now
+    // a stale stamp: left in place, it stays fence-valid (same reason,
+    // same cache path, same hash if the ledger did not change) and a
+    // later hand-written stop plan could ride it. Absent IS the truthful
+    // state — this round decided no stop.
+    try {
+      unlinkSync(tmpFile(target, 'stop.json'));
+    } catch {
+      // nothing to remove
+    }
   }
 
   const diffPath = tmpFile(target, 'diff.txt');
@@ -1202,7 +1280,7 @@ function runCaptureLocal(args: CaptureLocalArgs): void {
     // diverges from any hand recipe). A round-2 medium review of
     // `srclink/foo.ts` predicted `srclink_foo.ts.json`, found nothing, and
     // ruled on zero ledger entries over a Critical that still stood.
-    cachePath: cachePathFor(target, sourcePath),
+    cachePath,
     cacheCandidatePath,
     ...(candidateWritten ? { cacheCandidateStateId: candidate.stateId } : {}),
     ...planEffortField(args.effort),