fix: pre-landing review fixes (GST-1084)

- unadmitted() and the specialist coverage use Object.hasOwn, so an inherited name such as toString is not an admitted replay
- MUST_PIN names every non-specialist reviewer source; deleting one of those pins now fails
- writeManifest writes the pins before the manifest head; pin parse errors name the file
- mergePinEdits removes its repository when setup throws, and the merge tests clean up every merge they made

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

scripts/bump-harness-version.ts

@@ -41,7 +41,11 @@ function readPins(root: string): Record<string, ManifestEntry> {
   for (const rel of (fs.readdirSync(dir, { recursive: true }) as string[]).map(f => f.split(path.sep).join('/')).sort()) {
     if (fs.statSync(path.join(dir, rel)).isDirectory()) continue;
     if (!rel.endsWith('.json')) throw new Error(`${PINS_DIR}/${rel}: every harness pin is <pinned path>.json`);
-    files[rel.slice(0, -'.json'.length)] = JSON.parse(fs.readFileSync(path.join(dir, rel), 'utf8'));
+    try {
+      files[rel.slice(0, -'.json'.length)] = JSON.parse(fs.readFileSync(path.join(dir, rel), 'utf8'));
+    } catch (e) {
+      throw new Error(`${PINS_DIR}/${rel}: ${(e as Error).message}`);
+    }
   }
   return files;
 }
@@ -101,9 +105,8 @@ export function decide(manifest: HarnessManifest, files: TreeEntry[], decision:
     : manifest.files[entry.file]!])) };
 }
 
-/** Write the manifest head and one pin file per harness file, rewriting only pins whose bytes change and deleting dropped ones. */
+/** Write one pin file per harness file (rewriting only pins whose bytes change, deleting dropped ones), then the manifest head, so a failed pin write never moves the version. */
 export function writeManifest(root: string, manifest: HarnessManifest): void {
-  fs.writeFileSync(path.join(root, MANIFEST), `${JSON.stringify({ version: manifest.version, patterns: manifest.patterns }, null, 2)}\n`);
   for (const file of Object.keys(readPins(root))) if (!manifest.files[file]) fs.rmSync(pinPath(root, file));
   for (const [file, entry] of Object.entries(manifest.files)) {
     const target = pinPath(root, file), text = `${JSON.stringify(entry)}\n`;
@@ -111,6 +114,7 @@ export function writeManifest(root: string, manifest: HarnessManifest): void {
     fs.mkdirSync(path.dirname(target), { recursive: true });
     fs.writeFileSync(target, text);
   }
+  fs.writeFileSync(path.join(root, MANIFEST), `${JSON.stringify({ version: manifest.version, patterns: manifest.patterns }, null, 2)}\n`);
 }
 
 if (import.meta.main) {

test/harness-version.test.ts

@@ -18,7 +18,7 @@ import * as fs from 'node:fs';
 import * as os from 'node:os';
 import { LIVE_REGISTRY, type Registry } from '../scripts/eval-flake-rank';
 import { matchGlob } from './helpers/test-selection';
-import { mergePinEdits } from './helpers/pin-merge-fixture';
+import { mergePinEdits, type PinMerge } from './helpers/pin-merge-fixture';
 
 const ROOT = path.resolve(import.meta.dir, '..');
 
@@ -75,6 +75,9 @@ describe('HARNESS_VERSION guard', () => {
       fs.writeFileSync(path.join(dir, PINS_DIR, 'z.ts.txt'), 'stray\n');
       expect(() => readManifest(dir)).toThrow('every harness pin is <pinned path>.json');
       fs.rmSync(path.join(dir, PINS_DIR, 'z.ts.txt'));
+      fs.writeFileSync(path.join(dir, PINS_DIR, 'z.ts.json'), '<<<<<<< ours\n');
+      expect(() => readManifest(dir)).toThrow(`${PINS_DIR}/z.ts.json: `);
+      fs.writeFileSync(path.join(dir, PINS_DIR, 'z.ts.json'), '{"blob":"z","since":1}\n');
       fs.writeFileSync(path.join(dir, MANIFEST), JSON.stringify({ version: 3, patterns: [], files: {} }));
       expect(() => readManifest(dir)).toThrow('holds only version and patterns');
     } finally {
@@ -113,9 +116,12 @@ describe('HARNESS_VERSION guard', () => {
       writeManifest(root, decide(manifest, files, 'non-behavioral', reason));
     };
     const floor = 'test/helpers/pty/runners/floor.ts', observation = 'test/helpers/pty/runners/observation.ts';
-    const clean = mergePinEdits(copyManifest, record(floor, 'a'.repeat(40), 'floor runner logging only'), record(observation, 'b'.repeat(40), 'observation runner comment only'));
-    const conflict = mergePinEdits(copyManifest, record(floor, 'a'.repeat(40), 'floor runner logging only'), record(floor, 'c'.repeat(40), 'floor runner rename only'));
+    const merges: PinMerge[] = [];
     try {
+      const clean = mergePinEdits(copyManifest, record(floor, 'a'.repeat(40), 'floor runner logging only'), record(observation, 'b'.repeat(40), 'observation runner comment only'));
+      merges.push(clean);
+      const conflict = mergePinEdits(copyManifest, record(floor, 'a'.repeat(40), 'floor runner logging only'), record(floor, 'c'.repeat(40), 'floor runner rename only'));
+      merges.push(conflict);
       expect(clean.output).not.toContain('CONFLICT');
       expect(clean.clean).toBe(true);
       expect(JSON.parse(clean.read(`${PINS_DIR}/${floor}.json`))).toEqual({ blob: 'a'.repeat(40), since: live.version, nonBehavioral: 'floor runner logging only' });
@@ -124,7 +130,7 @@ describe('HARNESS_VERSION guard', () => {
       expect(conflict.clean).toBe(false);
       expect(conflict.output).toContain(`CONFLICT (content): Merge conflict in ${PINS_DIR}/${floor}.json`);
     } finally {
-      for (const merge of [clean, conflict]) fs.rmSync(merge.root, { recursive: true, force: true });
+      for (const merge of merges) fs.rmSync(merge.root, { recursive: true, force: true });
     }
   });
 });

test/helpers/pin-merge-fixture.ts

@@ -21,15 +21,21 @@ export function mergePinEdits(seed: (root: string) => void, a: (root: string) =>
     if (result.status !== 0) throw new Error(`git ${args.join(' ')} failed (${result.status}): ${result.stderr}`);
     return result.stdout;
   };
-  must('init', '-q', '-b', 'base');
   const commit = (edit: (root: string) => void, message: string) => { edit(root); must('add', '-A'); must('commit', '-q', '--no-verify', '-m', message); };
-  commit(seed, 'seed');
-  for (const [branch, edit] of [['a', a], ['b', b]] as const) {
-    must('checkout', '-q', '-b', branch, 'base');
-    commit(edit, branch);
+  let merged: ReturnType<typeof git>;
+  try {
+    must('init', '-q', '-b', 'base');
+    commit(seed, 'seed');
+    for (const [branch, edit] of [['a', a], ['b', b]] as const) {
+      must('checkout', '-q', '-b', branch, 'base');
+      commit(edit, branch);
+    }
+    merged = git('merge-tree', '--write-tree', 'a', 'b');
+    if (merged.status !== 0 && merged.status !== 1) throw new Error(`git merge-tree failed (${merged.status}): ${merged.stderr}`);
+  } catch (e) {
+    fs.rmSync(root, { recursive: true, force: true });
+    throw e;
   }
-  const merged = git('merge-tree', '--write-tree', 'a', 'b');
-  if (merged.status !== 0 && merged.status !== 1) throw new Error(`git merge-tree failed (${merged.status}): ${merged.stderr}`);
   const tree = merged.stdout.split('\n')[0]!;
   return { root, clean: merged.status === 0, output: merged.stdout, read: rel => must('show', `${tree}:${rel}`) };
 }

test/review-packets-removed.test.ts

@@ -11,7 +11,7 @@ import * as fs from 'fs';
 import * as os from 'os';
 import * as path from 'path';
 import { sectionTexts } from '../lib/review-brief';
-import { mergePinEdits } from './helpers/pin-merge-fixture';
+import { mergePinEdits, type PinMerge } from './helpers/pin-merge-fixture';
 
 const ROOT = path.join(import.meta.dir, '..');
 const REMOVED = [
@@ -69,7 +69,11 @@ function readPinDir(dir: string): Record<string, unknown> {
   for (const rel of (fs.readdirSync(dir, { recursive: true }) as string[]).map(f => f.split(path.sep).join('/')).sort()) {
     if (fs.statSync(path.join(dir, rel)).isDirectory()) continue;
     if (!rel.endsWith('.json')) throw new Error(`${dir}/${rel}: every pin file is <name>.json`);
-    out[rel.slice(0, -'.json'.length)] = JSON.parse(fs.readFileSync(path.join(dir, rel), 'utf8'));
+    try {
+      out[rel.slice(0, -'.json'.length)] = JSON.parse(fs.readFileSync(path.join(dir, rel), 'utf8'));
+    } catch (e) {
+      throw new Error(`${dir}/${rel}: ${(e as Error).message}`);
+    }
   }
   return out;
 }
@@ -97,6 +101,24 @@ function readPins(pins: string) {
 }
 type Pins = ReturnType<typeof readPins>;
 
+/** Every source a reviewer brief, checklist or depth rule comes from: deleting one of these pins fails, never drops the check. */
+const MUST_PIN = [
+  'lib/review-depth.ts',
+  'lib/review-risk.ts',
+  'review/checklist.md',
+  'review/design-checklist.md',
+  'review/gate-integrity.md',
+  'review/sections/adversarial.md',
+  'review/sections/review-army.md',
+  'scripts/resolvers/fix-check.ts',
+  'ship/sections/adversarial.md',
+  'ship/sections/review-army.md',
+];
+
+/** MUST_PIN entries and specialist checklists under root with no pin. */
+const unpinned = (pins: Pins, root: string) => [...MUST_PIN, ...fs.readdirSync(path.join(root, 'review/specialists')).filter(f => f.endsWith('.md')).map(f => `review/specialists/${f}`)]
+  .filter(f => !Object.hasOwn(pins.files, f));
+
 /** Pinned files whose LF-normalized bytes under root no longer hash to their pin. */
 const stalePins = (pins: Pins, root: string) => Object.entries(pins.files).filter(([file, pin]) => {
   const text = fs.readFileSync(path.join(root, file), 'utf8').replace(/\r\n/g, '\n');
@@ -105,7 +127,7 @@ const stalePins = (pins: Pins, root: string) => Object.entries(pins.files).filte
 
 /** Pins naming no replay in the store, and replays whose justification names no no-loss replay or re-score. */
 const unadmitted = (pins: Pins) => [
-  ...Object.entries(pins.files).filter(([, pin]) => !(pin.replay in pins.replays)).map(([file, pin]) => `${file} names unknown replay ${pin.replay}`),
+  ...Object.entries(pins.files).filter(([, pin]) => !Object.hasOwn(pins.replays, pin.replay)).map(([file, pin]) => `${file} names unknown replay ${pin.replay}`),
   ...Object.entries(pins.replays).filter(([, what]) => !/no-loss|re-score/.test(what)).map(([replay]) => `replay ${replay} is not a no-loss replay or re-score`),
 ];
 
@@ -140,7 +162,11 @@ describe('reviewer briefs stay at their no-loss-tested bytes (GSTA-167)', () =>
 
   test('the pins cover every specialist checklist', () => {
     const specialists = fs.readdirSync(path.join(ROOT, 'review/specialists')).filter(f => f.endsWith('.md')).map(f => `review/specialists/${f}`);
-    expect(specialists.filter(f => !(f in PINNED.files))).toEqual([]);
+    expect(specialists.filter(f => !Object.hasOwn(PINNED.files, f))).toEqual([]);
+  });
+
+  test('every source a reviewer brief, checklist or depth rule comes from has a pin (GST-1084)', () => {
+    expect(unpinned(PINNED, ROOT)).toEqual([]);
   });
 
   // Value: protects=the one-file-per-pin layout keeps every check the in-file table had; fails_when=a stale sha, an unknown or
@@ -160,13 +186,23 @@ describe('reviewer briefs stay at their no-loss-tested bytes (GSTA-167)', () =>
 
       const pin = (file: string, value: unknown) => fs.writeFileSync(path.join(pins, 'files', `${file}.json`), `${JSON.stringify(value)}\n`);
       pin('review/design-checklist.md', { sha: PINNED.files['review/design-checklist.md']!.sha, replay: 'GST-0000' });
+      pin('lib/review-risk.ts', { sha: PINNED.files['lib/review-risk.ts']!.sha, replay: 'toString' });
       fs.writeFileSync(path.join(pins, 'replays', 'GST-0001.json'), `${JSON.stringify({ replay: 'GST-0001', justification: 'reviewed by hand' })}\n`);
-      expect(unadmitted(readPins(pins))).toEqual(['review/design-checklist.md names unknown replay GST-0000', 'replay GST-0001 is not a no-loss replay or re-score']);
+      expect(unadmitted(readPins(pins))).toEqual(['lib/review-risk.ts names unknown replay toString', 'review/design-checklist.md names unknown replay GST-0000',
+        'replay GST-0001 is not a no-loss replay or re-score']);
+
+      expect(unpinned(readPins(pins), ROOT)).toEqual([]);
+      fs.rmSync(path.join(pins, 'files', 'lib/review-risk.ts.json'));
+      fs.rmSync(path.join(pins, 'files', 'review/specialists/testing.md.json'));
+      expect(unpinned(readPins(pins), ROOT)).toEqual(['lib/review-risk.ts', 'review/specialists/testing.md']);
 
       pin('review/gate-integrity.md', { ...PINNED.files['review/gate-integrity.md'], note: 'extra' });
       fs.writeFileSync(path.join(pins, 'replays', 'GST-0002.json'), `${JSON.stringify({ replay: 'GST-0003', justification: 'a no-loss replay' })}\n`);
       expect(readPins(pins).malformed).toEqual(['replays/GST-0002.json (replay GST-0003 belongs in replays/GST-0003.json)',
         'files/review/gate-integrity.md.json (expected exactly sha, replay)']);
+      fs.writeFileSync(path.join(pins, 'files', 'review/gate-integrity.md.json'), '<<<<<<< a\n{}\n');
+      expect(() => readPins(pins)).toThrow('files/review/gate-integrity.md.json: ');
+      fs.rmSync(path.join(pins, 'files', 'review/gate-integrity.md.json'));
       fs.writeFileSync(path.join(pins, 'brief-bodies', 'specialist.txt'), 'stray\n');
       expect(() => readPins(pins)).toThrow('every pin file is <name>.json');
     } finally {
@@ -184,9 +220,12 @@ describe('reviewer briefs stay at their no-loss-tested bytes (GSTA-167)', () =>
     };
     const seed = (root: string) => fs.cpSync(PINS, root, { recursive: true });
     const army = 'ship/sections/review-army.md', adversarial = 'ship/sections/adversarial.md';
-    const clean = mergePinEdits(seed, repin(army, 'a'.repeat(64), 'GST-9001+GST-967'), repin(adversarial, 'b'.repeat(64), 'GST-9002'));
-    const conflict = mergePinEdits(seed, repin(army, 'a'.repeat(64), 'GST-9001'), repin(army, 'c'.repeat(64), 'GST-9003'));
+    const merges: PinMerge[] = [];
     try {
+      const clean = mergePinEdits(seed, repin(army, 'a'.repeat(64), 'GST-9001+GST-967'), repin(adversarial, 'b'.repeat(64), 'GST-9002'));
+      merges.push(clean);
+      const conflict = mergePinEdits(seed, repin(army, 'a'.repeat(64), 'GST-9001'), repin(army, 'c'.repeat(64), 'GST-9003'));
+      merges.push(conflict);
       expect(clean.output).not.toContain('CONFLICT');
       expect(clean.clean).toBe(true);
       expect(JSON.parse(clean.read(`files/${army}.json`))).toEqual({ sha: 'a'.repeat(64), replay: 'GST-9001+GST-967' });
@@ -196,7 +235,7 @@ describe('reviewer briefs stay at their no-loss-tested bytes (GSTA-167)', () =>
       expect(conflict.clean).toBe(false);
       expect(conflict.output).toContain(`CONFLICT (content): Merge conflict in files/${army}.json`);
     } finally {
-      for (const merge of [clean, conflict]) fs.rmSync(merge.root, { recursive: true, force: true });
+      for (const merge of merges) fs.rmSync(merge.root, { recursive: true, force: true });
     }
   });
 });