chore(critique,reconcile): share hardened validators and the manifest lock — sc-1414 clusters 3+5

cli/__tests__/atomic-write-lock.test.mts

@@ -0,0 +1,117 @@
+/**
+ * withLock ownership safety. The mutex must never hand two read-modify-write callers the manifest
+ * at once, which means a stale lock may only be reaped when its holder is PROVABLY gone — age alone
+ * would evict a live-but-paused writer — and a release may only remove the caller's OWN acquisition.
+ * Every case below drives the real filesystem: the lock dir, its holder stamp, and its mtime.
+ */
+import { spawnSync } from 'node:child_process';
+import {
+  existsSync,
+  mkdirSync,
+  mkdtempSync,
+  readFileSync,
+  rmSync,
+  utimesSync,
+  writeFileSync,
+} from 'node:fs';
+import { tmpdir } from 'node:os';
+import { join } from 'node:path';
+import { afterEach, describe, expect, it } from 'vitest';
+import { withLock } from '../lib/atomic-write.mts';
+
+const roots: string[] = [];
+const mkTmp = () => {
+  const d = mkdtempSync(join(tmpdir(), 'devkit-lock-'));
+  roots.push(d);
+  return d;
+};
+
+afterEach(() => {
+  for (const d of roots.splice(0)) rmSync(d, { recursive: true, force: true });
+});
+
+/** Plant a held lock: dir + `<pid>:<uuid>` stamp, aged `ageMs` into the past (mtime set LAST). */
+const plantLock = (
+  lockDir: string,
+  { pid, ageMs, stamped = true }: { pid: number; ageMs: number; stamped?: boolean },
+) => {
+  mkdirSync(lockDir);
+  if (stamped) writeFileSync(join(lockDir, 'holder'), `${pid}:planted-uuid`, 'utf8');
+  const when = new Date(Date.now() - ageMs);
+  utimesSync(lockDir, when, when);
+};
+
+/** A pid that is definitely not running: spawn a trivial process and reuse its pid after it exits. */
+const deadPid = () => {
+  const p = spawnSync(process.execPath, ['-e', ''], { stdio: 'ignore' });
+  if (typeof p.pid !== 'number') throw new Error('could not obtain a pid');
+  return p.pid;
+};
+
+const STALE_MS = 90_000; // > the 60s LOCK_STALE_MS
+const FRESH_MS = 1_000;
+
+describe('withLock', () => {
+  it('runs the callback under the lock and releases it afterwards', () => {
+    const lockDir = join(mkTmp(), 'manifest.json.lock');
+    const seen = withLock(lockDir, () => {
+      expect(existsSync(lockDir)).toBe(true);
+      return readFileSync(join(lockDir, 'holder'), 'utf8');
+    });
+    expect(seen.startsWith(`${process.pid}:`)).toBe(true);
+    expect(existsSync(lockDir)).toBe(false);
+  });
+
+  it('reaps a stale lock whose holder is gone', () => {
+    const lockDir = join(mkTmp(), 'manifest.json.lock');
+    plantLock(lockDir, { pid: deadPid(), ageMs: STALE_MS });
+    expect(withLock(lockDir, () => 'acquired')).toBe('acquired');
+    expect(existsSync(lockDir)).toBe(false);
+  });
+
+  it('does NOT reap a stale lock whose holder is still alive', () => {
+    // The reviewer's case: a live writer paused past the stale window still owns its lock. Our own
+    // pid stands in for it — reaping here would run a second read-modify-write concurrently.
+    const lockDir = join(mkTmp(), 'manifest.json.lock');
+    plantLock(lockDir, { pid: process.pid, ageMs: STALE_MS });
+    expect(() => withLock(lockDir, () => 'acquired')).toThrow(/timed out acquiring manifest lock/);
+    expect(existsSync(lockDir)).toBe(true);
+    expect(readFileSync(join(lockDir, 'holder'), 'utf8')).toBe(`${process.pid}:planted-uuid`);
+  });
+
+  it('does NOT reap a fresh lock even when its holder is gone', () => {
+    // A young lock is presumed live: the holder may be mid-acquire, and the caller can afford to wait.
+    const lockDir = join(mkTmp(), 'manifest.json.lock');
+    plantLock(lockDir, { pid: deadPid(), ageMs: FRESH_MS });
+    expect(() => withLock(lockDir, () => 'acquired')).toThrow(/timed out acquiring manifest lock/);
+    expect(existsSync(lockDir)).toBe(true);
+  });
+
+  it('reaps a stale UNSTAMPED lock (acquirer died between its mkdir and its stamp write)', () => {
+    const lockDir = join(mkTmp(), 'manifest.json.lock');
+    plantLock(lockDir, { pid: 0, ageMs: STALE_MS, stamped: false });
+    expect(withLock(lockDir, () => 'acquired')).toBe('acquired');
+    expect(existsSync(lockDir)).toBe(false);
+  });
+
+  it('does not release a lock that is no longer ours', () => {
+    // Simulates being wrongly reaped mid-section: another holder now owns lockDir. An unconditional
+    // rmSync in the finally would strip THEIR lock and admit a third writer.
+    const lockDir = join(mkTmp(), 'manifest.json.lock');
+    withLock(lockDir, () => {
+      writeFileSync(join(lockDir, 'holder'), '999999:someone-elses-uuid', 'utf8');
+    });
+    expect(existsSync(lockDir)).toBe(true);
+    expect(readFileSync(join(lockDir, 'holder'), 'utf8')).toBe('999999:someone-elses-uuid');
+  });
+
+  it('releases the lock when the callback throws', () => {
+    const lockDir = join(mkTmp(), 'manifest.json.lock');
+    expect(() =>
+      withLock(lockDir, () => {
+        throw new Error('boom');
+      }),
+    ).toThrow('boom');
+    expect(existsSync(lockDir)).toBe(false);
+  });
+});

cli/lib/atomic-write.mts

@@ -5,17 +5,124 @@
  * UNIQUE (pid + timestamp) so two callers writing the same target never collide on the temp name.
  *
  * Shared by the ship manifest writer (cli/lib/ship/reconcile-manifest-write.mjs) and reconcile's
- * pruneBranch (cli/lib/reconcile.mjs) — both mutate .devkit/reconcile-manifest.json. The lost-update
- * race (two read-modify-write callers) is guarded SEPARATELY by each caller's mkdir-mutex; this
- * function only guarantees a single write is never torn.
+ * pruneBranch (cli/lib/reconcile.mjs) — both mutate .devkit/reconcile-manifest.json. writeFileAtomic
+ * only guarantees a single write is never torn; the lost-update race (two read-modify-write callers)
+ * is guarded by `withLock` below, which both callers wrap their read→modify→write in.
  *
  * Distinct from the two gate-engine/<engine>/atomic-write.mjs copies on purpose: a gate-engine ships
  * its own copy to stay independently vendorable (no cross-engine import), whereas cli/ has one home.
  */
-import { renameSync, writeFileSync } from 'node:fs';
+import { randomUUID } from 'node:crypto';
+import { lstatSync, mkdirSync, readFileSync, renameSync, rmSync, writeFileSync } from 'node:fs';
+import { join } from 'node:path';
 
 export function writeFileAtomic(path: string, contents: string): void {
   const tmp = `${path}.${process.pid}.${Date.now()}.tmp`;
   writeFileSync(tmp, contents, 'utf8');
   renameSync(tmp, path);
 }
+
+const LOCK_STALE_MS = 60_000; // age below which a lock is presumed live regardless of its holder
+const LOCK_WAIT_MS = 5_000; // total time to retry a contended lock before throwing (never write unlocked)
+const LOCK_RETRY_MS = 25; // pause between mkdir attempts, so a contended wait does not spin the CPU
+
+/** The acquisition stamp lives INSIDE the lock dir, so it is created and removed with the lock. */
+const holderFile = (lockDir: string) => join(lockDir, 'holder');
+
+/** Block the calling thread — the critical section is synchronous, so there is no loop to yield to. */
+const sleepSync = (ms: number) => {
+  Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms);
+};
+
+/**
+ * Is `pid` still running? EPERM means the process exists but belongs to another uid — still alive.
+ * Only ESRCH (no such process) proves death, so every ambiguous answer errs toward "keep waiting".
+ */
+function pidAlive(pid: number): boolean {
+  try {
+    process.kill(pid, 0);
+    return true;
+  } catch (e: unknown) {
+    return e instanceof Error && 'code' in e && e.code === 'EPERM';
+  }
+}
+
+/** The `<pid>:<uuid>` stamp of the CURRENT acquisition, or null if unreadable (mid-acquire/reaped). */
+function readHolder(lockDir: string): { pid: number; stamp: string } | null {
+  try {
+    const stamp = readFileSync(holderFile(lockDir), 'utf8');
+    const pid = Number(stamp.split(':')[0]);
+    return Number.isInteger(pid) && pid > 0 ? { pid, stamp } : null;
+  } catch {
+    return null;
+  }
+}
+
+/**
+ * Remove `lockDir` only when it is PROVABLY a dead holder's. Age alone is not proof: a live holder
+ * that was paused (SIGSTOP, a suspended laptop, a long GC) still owns its lock at 60s, and reaping
+ * it would let a second read-modify-write run concurrently with the first.
+ *
+ * So a reap needs three agreeing facts — stale age, a pid that no longer exists, and a stamp that
+ * did not change while we checked. The re-read is what defeats release-and-reacquire: if the holder
+ * released and someone else acquired between our two reads, the stamp differs and we leave their
+ * (live) lock alone. Residual: a reacquire landing between the re-read and the rmSync below is
+ * still possible, but it requires the dead holder's lock to be reaped by a third party inside that
+ * window; every wider path is closed. Refusing to reap is always safe — the caller just times out.
+ */
+function reapIfDead(lockDir: string): void {
+  let mtimeMs: number;
+  try {
+    mtimeMs = lstatSync(lockDir).mtimeMs;
+  } catch {
+    return; // lock vanished under us — the loop retries the mkdir
+  }
+  if (Date.now() - mtimeMs <= LOCK_STALE_MS) return;
+  const before = readHolder(lockDir);
+  // An unstamped stale lock is an acquirer that died between its mkdir and its stamp write (a
+  // microsecond window); there is no pid to interrogate, so staleness is all the evidence there is.
+  if (before && pidAlive(before.pid)) return;
+  if (readHolder(lockDir)?.stamp !== before?.stamp) return;
+  rmSync(lockDir, { recursive: true, force: true });
+}
+
+/**
+ * Atomic-mkdir mutex (flock is absent on macOS — verified). The dir IS the lock; mkdir is
+ * atomic create-or-fail on every POSIX fs, and the holder stamps its pid inside so ownership can be
+ * checked rather than inferred. We only guard a sub-ms read→write→rename, so on contention we retry
+ * up to LOCK_WAIT_MS; if still unheld we THROW rather than write unlocked (an unlocked
+ * read-modify-write would lose a parallel ship's branch entry). Shared by the two reconcile-manifest
+ * mutators named above; both lock a path under the repo's own .devkit/, so the pid check always
+ * refers to a process on this machine.
+ */
+export function withLock<T>(lockDir: string, fn: () => T): T {
+  const stamp = `${process.pid}:${randomUUID()}`;
+  const deadline = Date.now() + LOCK_WAIT_MS;
+  let held = false;
+  while (Date.now() <= deadline) {
+    try {
+      mkdirSync(lockDir);
+    } catch (e: unknown) {
+      if (!(e instanceof Error && 'code' in e && e.code === 'EEXIST')) throw e;
+      reapIfDead(lockDir);
+      sleepSync(LOCK_RETRY_MS);
+      continue;
+    }
+    try {
+      writeFileSync(holderFile(lockDir), stamp, 'utf8');
+    } catch (e: unknown) {
+      rmSync(lockDir, { recursive: true, force: true }); // never leave a lock we cannot prove is ours
+      throw e;
+    }
+    held = true;
+    break;
+  }
+  if (!held) throw new Error(`timed out acquiring manifest lock: ${lockDir}`);
+  try {
+    return fn();
+  } finally {
+    // Release OUR acquisition only. If this lock was wrongly reaped and another process now holds
+    // it, an unconditional rmSync here would strip a live holder's lock and admit a third writer.
+    if (readHolder(lockDir)?.stamp === stamp) rmSync(lockDir, { recursive: true, force: true });
+  }
+}

cli/lib/reconcile.mts

@@ -29,13 +29,11 @@
  * exact concurrent work this module declined to touch one step earlier.
  */
 import { execFileSync } from 'node:child_process';
-import { existsSync, lstatSync, mkdirSync, readFileSync, rmSync } from 'node:fs';
+import { existsSync, readFileSync } from 'node:fs';
 import { join } from 'node:path';
-import { writeFileAtomic } from './atomic-write.mts';
+import { withLock, writeFileAtomic } from './atomic-write.mts';
 
 const ABSENT = Symbol('absent'); // a file/blob that does not exist on a given side (≠ any sha)
-const LOCK_STALE_MS = 60_000;
-const LOCK_WAIT_MS = 5_000;
 
 /** A blob sha (git object id) or the ABSENT sentinel for a side where the path does not exist. */
 type Blob = string | typeof ABSENT;
@@ -165,35 +163,6 @@ export function loadManifest(mainRepo: string): ReconcileManifest {
   return { version: 1, branches: {} };
 }
 
-/** Atomic-mkdir mutex (flock is absent on macOS) — see the manifest writer for the rationale. */
-function withLock<T>(lockDir: string, fn: () => T): T {
-  const deadline = Date.now() + LOCK_WAIT_MS;
-  let held = false;
-  while (Date.now() <= deadline) {
-    try {
-      mkdirSync(lockDir);
-      held = true;
-      break;
-    } catch (e: unknown) {
-      if (!(e instanceof Error && 'code' in e && e.code === 'EEXIST')) throw e;
-      try {
-        if (Date.now() - lstatSync(lockDir).mtimeMs > LOCK_STALE_MS)
-          rmSync(lockDir, { recursive: true, force: true });
-      } catch {
-        /* lock vanished — retry */
-      }
-    }
-  }
-  // Never run fn() unlocked: an unsynchronized read-modify-write would let a concurrent ship or
-  // reconcile clobber another branch's entry (the rename is atomic, but the read+merge is not).
-  if (!held) throw new Error(`timed out acquiring manifest lock: ${lockDir}`);
-  try {
-    return fn();
-  } finally {
-    rmSync(lockDir, { recursive: true, force: true });
-  }
-}
-
 /** Remove a fully-reconciled branch entry (atomic temp+rename under the lock). */
 export function pruneBranch(mainRepo: string, branch: string): void {
   const file = manifestFile(mainRepo);

cli/lib/ship/reconcile-manifest-write.mts

@@ -36,14 +36,12 @@
  * miss only costs a manual reconcile later; it must never unwind a shipped PR.
  */
 import { execFileSync } from 'node:child_process';
-import { existsSync, lstatSync, mkdirSync, readFileSync, realpathSync, rmSync } from 'node:fs';
+import { existsSync, lstatSync, mkdirSync, readFileSync, realpathSync } from 'node:fs';
 import { dirname, join } from 'node:path';
 import { pathToFileURL } from 'node:url';
-import { writeFileAtomic } from '../atomic-write.mts';
+import { withLock, writeFileAtomic } from '../atomic-write.mts';
 import type { ReconcileManifest, ReconcilePath } from '../reconcile.mts';
 
-const LOCK_STALE_MS = 60_000; // a lock dir older than this is a dead writer/reader — reap it
-const LOCK_WAIT_MS = 5_000; // total time to retry a contended lock before throwing (never write unlocked)
 const WS_SPLIT = /\s+/; // split a `git ls-tree` line into its mode/type/sha/path columns
 const PR_DIGITS = /^\d+$/; // a non-empty --pr is an integer; anything else → null
 
@@ -112,39 +110,6 @@ function classify(gitRoot: string, baseSha: string, p: string): ReconcilePath |
   return { path: p, blobSha, mode, op: 'delete' };
 }
 
-/**
- * Atomic-mkdir mutex (flock is absent on macOS — verified). The dir IS the lock; mkdir is
- * atomic create-or-fail on every POSIX fs. We only guard a sub-ms read→write→rename, so on
- * contention we retry up to LOCK_WAIT_MS; if still unheld we THROW rather than write unlocked
- * (an unlocked read-modify-write would lose a parallel ship's branch entry). A lock older than
- * 60s is a crashed holder — reap it.
- */
-function withLock<T>(lockDir: string, fn: () => T): T {
-  const deadline = Date.now() + LOCK_WAIT_MS;
-  let held = false;
-  while (Date.now() <= deadline) {
-    try {
-      mkdirSync(lockDir);
-      held = true;
-      break;
-    } catch (e: unknown) {
-      if (!(e instanceof Error && 'code' in e && e.code === 'EEXIST')) throw e;
-      try {
-        if (Date.now() - lstatSync(lockDir).mtimeMs > LOCK_STALE_MS)
-          rmSync(lockDir, { recursive: true, force: true });
-      } catch {
-        /* lock vanished under us — loop retries the mkdir */
-      }
-    }
-  }
-  if (!held) throw new Error(`timed out acquiring manifest lock: ${lockDir}`);
-  try {
-    return fn();
-  } finally {
-    rmSync(lockDir, { recursive: true, force: true });
-  }
-}
-
 /** A well-formed v1 manifest: version 1 with a plain (non-array) branches object. */
 const isValidV1 = (m: unknown): m is ReconcileManifest =>
   typeof m === 'object' &&

gate-engine/critique/evidence-bindings.mts

@@ -2,6 +2,7 @@ import {
   canonicalPlanCritiqueRecordJson,
   type PlanCritiqueRecordV1,
   sha256Bytes,
+  validText,
 } from './evidence-record.mts';
 import { readPlanCritiqueRecord } from './evidence-store.mts';
 import {
@@ -34,7 +35,6 @@ const CRITIQUE_ID = /^pc1_[0-9a-f]{64}$/;
 const GIT_OBJECT_ID = /^(?:[0-9a-f]{40}|[0-9a-f]{64})$/;
 const ISO_TIMESTAMP = /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$/;
 const BINDING_FILE = /^[0-9a-f]{64}\.json$/;
-const MAX_TEXT_BYTES = 4 * 1024;
 const MAX_BINDING_BYTES = 16 * 1024;
 const BINDING_PATH = ['devkit', 'plan-critique-bindings', 'v1'] as const;
 
@@ -98,18 +98,6 @@ function exactObject(value: unknown, fields: readonly string[]): Record<string,
   return keys.length === fields.length && keys.every((key) => fields.includes(key)) ? value : null;
 }
 
-function validText(value: unknown): value is string {
-  return (
-    typeof value === 'string' &&
-    value.trim().length > 0 &&
-    Buffer.byteLength(value, 'utf8') <= MAX_TEXT_BYTES &&
-    ![...value].some((character) => {
-      const code = character.charCodeAt(0);
-      return code <= 0x1f || (code >= 0x7f && code <= 0x9f);
-    })
-  );
-}
-
 function canonicalBindingJson(binding: PlanCritiqueBindingV1): Buffer {
   return Buffer.from(canonicalPlanCritiqueRecordJson(binding));
 }