Merge remote-tracking branch 'origin/main' into gsta-96-preamble-hoist

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

# Conflicts:
#	CHANGELOG.md
#	test/fixtures/skill-md-size-allowlist.json

test/fixtures/skill-md-size-allowlist.json

@@ -7,7 +7,7 @@
     "agy/gstack-plan-design-review": 120811,
     "agy/gstack-plan-devex-review": 116557,
     "agy/gstack-plan-eng-review": 144399,
-    "agy/gstack-review": 103605,
+    "agy/gstack-review": 103569,
     "claude/design-review": 114874,
     "codex/gstack-autoplan": 108636,
     "codex/gstack-design-review": 123041,
@@ -15,26 +15,26 @@
     "codex/gstack-plan-design-review": 121883,
     "codex/gstack-plan-devex-review": 118065,
     "codex/gstack-plan-eng-review": 145564,
-    "codex/gstack-review": 103128,
+    "codex/gstack-review": 103092,
     "copilot/gstack-autoplan": 109336,
     "copilot/gstack-design-review": 122537,
     "copilot/gstack-plan-design-review": 121122,
     "copilot/gstack-plan-devex-review": 116019,
     "copilot/gstack-plan-eng-review": 144564,
-    "copilot/gstack-review": 103186,
+    "copilot/gstack-review": 103150,
     "cursor/gstack-autoplan": 108568,
     "cursor/gstack-design-review": 121802,
     "cursor/gstack-plan-design-review": 119667,
     "cursor/gstack-plan-devex-review": 115289,
     "cursor/gstack-plan-eng-review": 143154,
-    "cursor/gstack-review": 120038,
+    "cursor/gstack-review": 120002,
     "factory/gstack-autoplan": 110846,
     "factory/gstack-design-review": 123586,
     "factory/gstack-land-and-deploy": 100108,
     "factory/gstack-plan-design-review": 122442,
     "factory/gstack-plan-devex-review": 118652,
     "factory/gstack-plan-eng-review": 146262,
-    "factory/gstack-review": 123811,
+    "factory/gstack-review": 123775,
     "gbrain/gstack-autoplan": 108975,
     "gbrain/gstack-design-review": 112007,
     "gbrain/gstack-plan-design-review": 108424,
@@ -48,7 +48,7 @@
     "kiro/gstack-plan-design-review": 119519,
     "kiro/gstack-plan-devex-review": 115141,
     "kiro/gstack-plan-eng-review": 143016,
-    "kiro/gstack-review": 119862,
+    "kiro/gstack-review": 119826,
     "openclaw/gstack-autoplan": 108798,
     "openclaw/gstack-design-review": 110930,
     "openclaw/gstack-plan-design-review": 107393,
@@ -58,12 +58,12 @@
     "opencode/gstack-plan-design-review": 120954,
     "opencode/gstack-plan-devex-review": 115897,
     "opencode/gstack-plan-eng-review": 144423,
-    "opencode/gstack-review": 120660,
+    "opencode/gstack-review": 120624,
     "slate/gstack-autoplan": 108455,
     "slate/gstack-design-review": 121715,
     "slate/gstack-plan-design-review": 119593,
     "slate/gstack-plan-devex-review": 115215,
     "slate/gstack-plan-eng-review": 143085,
-    "slate/gstack-review": 119950
+    "slate/gstack-review": 119914
   }
 }

test/review-fix-range.test.ts

@@ -1,5 +1,5 @@
 /**
- * #3339: /ship Step 9 may re-review only the fix range, but a fix-range pass
+ * #3339: /ship Step 9 and /review may re-review only the fix range, but a fix-range pass
  * can never be the last review. The fix base comes from the review log (never
  * the working tree or the caller) and is the commit whose tree the fixing pass
  * started from, a narrowed row is never CURRENT and carries no
@@ -214,3 +214,75 @@ describe('ship Step 9 instructions', () => {
     expect(army).toContain('continue to Step 10, and only from a full pass: after a fix-range pass, run one full Step 9 first.');
   });
 });
+
+describe('review Step 3 and Step 5.8 instructions', () => {
+  const skill = fs.readFileSync(path.join(ROOT, 'review/SKILL.md'), 'utf8');
+  const step3 = skill.slice(skill.indexOf('## Step 3: Get the diff'), skill.indexOf('## Step 3.4:'));
+  const step58 = skill.slice(skill.indexOf('## Step 5.8: Persist Eng Review result'), skill.indexOf('### Report the final review'));
+  const flat3 = step3.replace(/\s+/g, ' '), flat58 = step58.replace(/\s+/g, ' ');
+
+  test('the first pass starts full and only the pass after an edited pass narrows its DIFF_BASE, never Step 4.8 or other base uses', () => {
+    const block = step3.slice(step3.indexOf('```bash'), step3.indexOf('```', step3.indexOf('```bash') + 7));
+    expect(block).toContain('gstack-review-log --start review\ngit diff "$DIFF_BASE"');
+    expect(block).not.toContain('--fix-range');
+    expect(flat3).toContain('After an edited pass, start the next with `--start review --fix-range`: it prints REVIEW_START, then `FIX_BASE=<sha>`. Wherever this pass sets DIFF_BASE outside Step 4.8, use FIX_BASE instead; Step 4.8 and every other `<base>` use stay full. On refusal, start a full pass.');
+    expect(skill.match(/--fix-range/g)).toHaveLength(1);
+  });
+
+  test('an edited pass is logged before the repeat, kept out of Step 5.0, and a fix-range pass is never the final review', () => {
+    const edited = flat58.indexOf("Edited: increment CYCLES once, then fill and save the record below with `converged:false`. Step 5.0 never treats this invocation's records as prior reviews.");
+    const repeat = flat58.indexOf('Below 3, repeat Steps 3–5 with a new REVIEW_START.');
+    const none = flat58.indexOf('No edits: after a full pass, fill the record below. A fix-range pass is never final: repeat Steps 3–5 with a full start.');
+    expect(edited).toBeGreaterThan(-1);
+    expect(repeat).toBeGreaterThan(edited);
+    expect(none).toBeGreaterThan(repeat);
+    expect(flat58).toContain("finish only this pass's own token");
+  });
+});
+
+describe('review Step 3/5.8 order against the real logger', () => {
+  // /review's order on every pass: core start, Step 4.8 attempt start and finish, fixes in the working tree, core finish; never a commit.
+  const reviewRow = (fields: Record<string, unknown>) => JSON.stringify({ skill: 'review', status: 'clean', issues_found: 0, critical: 0, informational: 0, ...fields });
+  function reviewPass(fields: Record<string, unknown>, start: string[], fixes?: string) {
+    const started = bin('gstack-review-log', start);
+    expect(started.code).toBe(0);
+    const [token, base] = started.out.split('\n');
+    // Step 4.8: each pass's adversarial attempt saves its row before any fix and before the core row.
+    const attempt = bin('gstack-review-log', ['--start', 'adversarial-review']);
+    expect(attempt.code).toBe(0);
+    expect(bin('gstack-review-log', [JSON.stringify({ skill: 'adversarial-review', status: 'clean', source: 'claude', ...clean }), '--finish', attempt.out]).code).toBe(0);
+    if (fixes) edit(fixes);
+    expect(bin('gstack-review-log', [reviewRow(fields), '--finish', token!]).code).toBe(0);
+    return base;
+  }
+
+  test('a committed first pass narrows the next repeat to the uncommitted fixes, then the chain returns to a full pass', () => {
+    const reviewed = head();
+    reviewPass(fixing, ['--start', 'review'], 'export const a = 2;\n');
+    expect(reviewPass(fixing, ['--start', 'review', '--fix-range'], 'export const a = 3;\n')).toBe(`FIX_BASE=${reviewed}`);
+    expect(gitIn(repo, `diff --name-only ${reviewed}`).trim()).toBe('a.ts');
+    const third = narrowStart();
+    expect(third.code).toBe(1);
+    expect(third.err).toContain('started from uncommitted content');
+    reviewPass(clean, ['--start', 'review']);
+    const last = rows().at(-1)!;
+    expect(last.scope).toBeUndefined();
+    expect(reviewFreshness(last, last.wtree)?.status).toBe('CURRENT');
+  }, 30_000);
+
+  test('a zero-edit fix-range pass is never CURRENT even if it were logged, so the final record needs a full pass', () => {
+    reviewPass(fixing, ['--start', 'review'], 'export const a = 2;\n');
+    reviewPass(clean, ['--start', 'review', '--fix-range']);
+    const narrowed = rows().at(-1)!;
+    expect(narrowed.scope).toBe('fix-range');
+    expect(reviewFreshness(narrowed, narrowed.review_binding.range_end_wtree)?.status).toBe('UNVERIFIED');
+  }, 30_000);
+
+  test('a first pass over uncommitted work cannot be narrowed', () => {
+    edit('export const a = 1; // uncommitted\n');
+    reviewPass(fixing, ['--start', 'review'], 'export const a = 2;\n');
+    const r = narrowStart();
+    expect(r.code).toBe(1);
+    expect(r.err).toContain('started from uncommitted content');
+  }, 30_000);
+});

test/review-workflow-clarity.test.ts

@@ -229,7 +229,8 @@ test('review finalization ownership: initialize invocation state and capture the
   expect(flat).toMatch(/REVIEW_START for this core candidate before reading its diff/i);
   expectMentions(flat, [['never', 'reading', 'before']], 'flat');
   expect(flat).toMatch(/separate PASS_START tokens, not REVIEW_START/);
-  expectMentions(flat, [['only', 'finishes', 'final']], 'flat');
+  expect(flat).toContain('After an edited pass, start the next with `--start review --fix-range`');
+  expect(flat).toContain('On refusal, start a full pass.');
 });
 
 test('review finalization ownership: late findings use Fix-First before the bounded parent transition', () => {
@@ -240,7 +241,7 @@ test('review finalization ownership: late findings use Fix-First before the boun
     'Structured approval does not waive advisory/test_stub ASK gates',
     '## Step 5.8: Persist Eng Review result',
     'Edited: increment CYCLES once',
-    'No edits: fill the record below',
+    'No edits: after a full pass, fill the record below',
     '### 2. Fill the record',
     '--finish REVIEW_START',
   ];
@@ -248,7 +249,7 @@ test('review finalization ownership: late findings use Fix-First before the boun
   expect(positions.every(position => position >= 0)).toBe(true);
   expect(positions).toEqual([...positions].sort((a, b) => a - b));
   expect(flat).toContain('at most 3 fix cycles');
-  expect(flat).toContain('At 3, persist `converged:false`');
+  expect(flat).toContain('Edited: increment CYCLES once, then fill and save the record below with `converged:false`.');
   const limit = flat.slice(flat.indexOf('At 3,'), flat.indexOf('No edits:'));
   expect(limit).toMatch(/then stop this invocation/i);
   expectMentions(limit, [['without', 'summary', 'fourth']], 'limit');
@@ -276,14 +277,14 @@ test('review finalization ownership: required native completion and optional out
   expect(flat).toContain('`CONVERGED`: true only for a completed zero-edit pass');
 });
 
-test('review finalization ownership: finish only the final core token without log-time capture', () => {
+test('review finalization ownership: each logged pass finishes its own core token without log-time capture', () => {
   const step = skill.slice(skill.indexOf('## Step 5.8: Persist Eng Review result'));
   const flat = step.replace(/\s+/g, ' ');
   expect(step.match(/--finish REVIEW_START/g)).toHaveLength(1);
   expect(step).not.toContain('--start review');
   expect(step).not.toContain('--finish PASS_START');
   expect(flat).toMatch(/never invent a binding or replace REVIEW_START at log time/i);
-  expectMentions(flat, [['only', 'finish', 'final']], 'flat');
+  expect(flat).toContain("finish only this pass's own token");
   expect(step).toContain('"completed":COMPLETED,"converged":CONVERGED,"cycles":CYCLES');
 });