Fix source-safety, detector, and lock defects in Live polling work

Addresses the review findings on #371, plus several the bots did not catch. All fixes have regression coverage that fails on the prior code. Source corruption: - Vue accept dropped valueless root attrs (disabled, v-cloak) and, worse, rewrote @click="x" as a literal click="x" DOM attribute, because the attr parser was name-anchored and skipped the sigil. Tokenize the whole Vue attr grammar and normalize shorthands so accept round-trips directives. - --variant was interpolated unescaped into a RegExp, so --variant '.*' matched the original block first and reported a successful accept while silently restoring the original. Validate against the digits pattern the browser and the /events schema already enforce. - --id reached path.join unvalidated, so --id ../../../../etc/evil wrote and read receipts outside the project. Hoist the existing safeSessionId check into impeccable-paths and apply it at every id-to-path sink. Accept/lock correctness: - Plain HTML/JSX accept and discard did not catch SOURCE_LOCKED, so contention exited non-zero with empty stdout and the agent got no JSON to retry on. - Lock staleness was mtime-only and never read the pid it records: a holder whose critical section outran 60s had its live lock swept, admitting a second writer to the same file, while a crashed holder blocked accepts for a full 60s. Decide staleness by owner liveness, and release only our own lock. Detector: - isNeutralColor only parses computed color forms, so routing authored CSS through it reported inset 4px 0 0 #000 / black / #e5e7eb as chromatic side-tab stripes. Add an authored-color neutrality test covering hex and named neutrals; the fixture had no literal-color cases at all. - Rule line numbers were off by one for every rule after the first, and commented-out CSS was scanned as live rules. Server: - An error reply carries no sourceEventType, and inferSourceEventType returned undefined, which acknowledgePendingEvent treats as a wildcard: a stale generate worker's failure consumed the user's queued Accept, which then reached no agent and left the browser in SAVING forever. - The generate preflight spawned live-wrap.mjs synchronously inside the request handler, freezing the single-threaded server for the whole scaffold (~7.6s measured on this repo, 15s ceiling) and stalling Accept/Discard/SSE. Make it async, claiming the lease before the first await so no event double-delivers. - Every browser checkpoint was echoed back as variant_progress, so a Tune slider drag remounted the preview under the user's cursor and latched the *_reviewable phases from the wrong trigger. Gate on the reason. Cleanup: - Collapse four divergent benchmark argv parsers into scripts/lib/cli-args.mjs. Three silently misread flags: --iterations 20 benchmarked 5, --agent llm ran the fake agent, --median-target=0.4 used the default threshold. - Drop a snapshot cache this branch made write-only (it grew per session for the server's lifetime and was never read), a dead exported reconcile helper, and the unused deferReply branch. Prepared with AI assistance under maintainer direction. Co-Authored-By: Claude <noreply@anthropic.com>
2026-09-19 01:26:29 +03:00 · 2026-07-17 13:48:44 -07:00
parent c6ac34b929
commit 4e381305e1
25 changed files with 921 additions and 218 deletions
@@ -1,6 +1,8 @@
-import { execFileSync } from 'node:child_process';
+import { execFile } from 'node:child_process';
 import path from 'node:path';
+import { promisify } from 'node:util';

+const execFileAsync = promisify(execFile);
 const PREFLIGHT_TIMEOUT_MS = 15_000;

 export function buildGenerationPreflight(event, scriptsDir, { isolated = false } = {}) {
@@ -22,10 +24,20 @@ export function buildGenerationPreflight(event, scriptsDir, { isolated = false }
  return { script, args, mode: isInsert ? 'insert' : 'replace' };
 }

-export function runGenerationPreflight(event, {
+/**
+ * Scaffold the source for a generate event before handing it to an agent.
+ *
+ * Async on purpose. This spawns `live-wrap.mjs`, which walks the project's
+ * source tree and can take seconds (measured at ~7.6s on a large repo when the
+ * element is not found, with a 15s ceiling). The live server is single-threaded
+ * and calls this while leasing a poll, so a synchronous spawn froze the whole
+ * server for that entire window: Accept and Discard POSTs, SSE progress
+ * broadcasts, and every other poll stalled behind it.
+ */
+export async function runGenerationPreflight(event, {
  cwd = process.cwd(),
  scriptsDir,
-  execFileSyncImpl = execFileSync,
+  execFileImpl = execFileAsync,
  timeoutMs = PREFLIGHT_TIMEOUT_MS,
  isolated = false,
 } = {}) {
@@ -36,11 +48,10 @@ export function runGenerationPreflight(event, {

  const startedAt = performance.now();
  try {
-    const stdout = execFileSyncImpl(process.execPath, command.args, {
+    const { stdout } = await execFileImpl(process.execPath, command.args, {
      cwd,
      encoding: 'utf-8',
      timeout: timeoutMs,
-      stdio: ['ignore', 'pipe', 'pipe'],
    });
    const line = String(stdout).trim().split('\n').filter(Boolean).pop();
    if (!line) throw new Error('preflight returned no scaffold metadata');
@@ -13,21 +13,6 @@ export function sha256(value) {
  return createHash('sha256').update(value).digest('hex');
 }

-export function reconcilePublishedSourceVariants({ current, candidate, priorArrived = 0 } = {}) {
-  let reconciled = String(candidate || '');
-  const stable = String(current || '');
-  for (let variant = 1; variant <= Number(priorArrived || 0); variant += 1) {
-    const stableBlock = extractVariantBlock(stable, variant);
-    const candidateBlock = extractVariantBlock(reconciled, variant);
-    if (!stableBlock || !candidateBlock) {
-      return failure('published_variant_missing', { variant });
-    }
-    const offset = reconciled.indexOf(candidateBlock);
-    reconciled = reconciled.slice(0, offset) + stableBlock + reconciled.slice(offset + candidateBlock.length);
-  }
-  return { ok: true, content: reconciled };
-}
-
 export function prepareGenerationArtifact({ id, sourceFile, cwd = process.cwd() } = {}) {
  if (!id) return failure('missing_session_id');
  if (!sourceFile) return failure('missing_file');
@@ -134,12 +119,8 @@ export function publishGenerationArtifact({
      const store = createLiveSessionStore({ cwd, sessionId: id });
      const snapshot = store.getSnapshot(id, { includeCompleted: true });
      if (!snapshot?.updatedAt) return failure('session_missing');
-      if (snapshot.generationCanceled === true) {
-        return failure('stale_generation_epoch', { canceled: true, phase: snapshot.phase });
-      }
-      if (Number(snapshot.generationEpoch || 1) !== epoch) {
-        return failure('stale_generation_epoch', { expectedEpoch: snapshot.generationEpoch || 1 });
-      }
+      const stale = staleGenerationFailure(snapshot, epoch);
+      if (stale) return stale;

      const current = fs.readFileSync(sourcePath, 'utf-8');
      const currentHash = sha256(current);
@@ -194,12 +175,8 @@ export function publishGenerationArtifact({
      }

      const commitSnapshot = store.getSnapshot(id, { includeCompleted: true });
-      if (commitSnapshot?.generationCanceled === true) {
-        return failure('stale_generation_epoch', { canceled: true, phase: commitSnapshot.phase });
-      }
-      if (Number(commitSnapshot?.generationEpoch || 1) !== epoch) {
-        return failure('stale_generation_epoch', { expectedEpoch: commitSnapshot?.generationEpoch || 1 });
-      }
+      const commitStale = staleGenerationFailure(commitSnapshot, epoch);
+      if (commitStale) return commitStale;
      const artifactHash = sha256(artifact);
      const publishPath = sourceArtifactTarget?.previewPath || sourcePath;
      atomicReplace(publishPath, artifact);
@@ -366,6 +343,14 @@ function publishComponentArtifact({
    }
  }

+  // Check the fence before writing anything. The prepare→publish gap is exactly
+  // where an Accept lands, and the source-artifact path above rechecks before
+  // its only write. Without the same check here, a canceled generation still
+  // scattered variant files across the generated component dir and left them
+  // there — the `stale_generation_epoch` returns below have no rollback.
+  const preWriteStale = staleGenerationFailure(store.getSnapshot(id, { includeCompleted: true }), epoch);
+  if (preWriteStale) return preWriteStale;
+
  // Components and optional params become reachable before the manifest
  // advertises them. Committing the manifest last makes publication atomic
  // from the browser's point of view while the source lock excludes Accept.
@@ -376,13 +361,11 @@ function publishComponentArtifact({
  if (paramsContent !== null) {
    atomicReplace(path.join(target.componentPath, 'params.json'), paramsContent);
  }
+  // Re-check immediately before the manifest: the manifest is what makes the
+  // variants visible to the browser, so this is the gate that actually matters.
  const commitSnapshot = store.getSnapshot(id, { includeCompleted: true });
-  if (commitSnapshot?.generationCanceled === true) {
-    return failure('stale_generation_epoch', { canceled: true, phase: commitSnapshot.phase });
-  }
-  if (Number(commitSnapshot?.generationEpoch || 1) !== epoch) {
-    return failure('stale_generation_epoch', { expectedEpoch: commitSnapshot?.generationEpoch || 1 });
-  }
+  const commitStale = staleGenerationFailure(commitSnapshot, epoch);
+  if (commitStale) return commitStale;
  const publishedManifest = {
    ...target.manifest,
    componentDir: relative(cwd, target.componentPath),
@@ -615,3 +598,18 @@ function relative(cwd, value) {
 function failure(error, details = {}) {
  return { ok: false, error, ...details };
 }
+
+/**
+ * The generation fence: has this session been canceled (Accept/Discard landed),
+ * or has a newer generation superseded this epoch? Returns a failure result to
+ * propagate, or null when the caller may proceed.
+ */
+function staleGenerationFailure(snapshot, epoch) {
+  if (snapshot?.generationCanceled === true) {
+    return failure('stale_generation_epoch', { canceled: true, phase: snapshot.phase });
+  }
+  if (Number(snapshot?.generationEpoch || 1) !== epoch) {
+    return failure('stale_generation_epoch', { expectedEpoch: snapshot?.generationEpoch || 1 });
+  }
+  return null;
+}
@@ -1,6 +1,6 @@
 import fs from 'node:fs';
 import path from 'node:path';
-import { getLegacyLiveSessionsDir, getLiveSessionsDir } from '../lib/impeccable-paths.mjs';
+import { getLegacyLiveSessionsDir, getLiveSessionsDir, safeSessionId } from '../lib/impeccable-paths.mjs';

 const COMPLETED_PHASES = new Set(['completed', 'discarded']);
 const GENERATION_FENCED_PHASES = new Set([
@@ -15,17 +15,12 @@ export function createLiveSessionStore({ cwd = process.cwd(), sessionId } = {})
  const rootDir = getLiveSessionsDir(cwd);
  const legacyRootDir = getLegacyLiveSessionsDir(cwd);
  fs.mkdirSync(rootDir, { recursive: true });
-  const snapshotCache = new Map();
-
-  function loadCachedOrRebuild(id) {
-    const cached = snapshotCache.get(id);
-    if (cached) return cached;
-    const journalPath = getReadableJournalPath(id);
-    const rebuilt = rebuildSnapshotFromJournal(journalPath, id);
-    snapshotCache.set(id, rebuilt);
-    return rebuilt;
-  }

+  // No snapshot cache on purpose: appendEvent and getSnapshot both rebuild from
+  // the journal so sequence numbers and phase fences never come from a stale
+  // in-memory copy when the publisher/complete helpers append from another
+  // process. A cache written but never read would grow per session for the
+  // lifetime of the server without ever saving a rebuild.
  function getReadableJournalPath(id) {
    const primary = getJournalPath(rootDir, id);
    if (fs.existsSync(primary)) return primary;
@@ -59,7 +54,6 @@ export function createLiveSessionStore({ cwd = process.cwd(), sessionId } = {})
      };
      fs.appendFileSync(journalPath, JSON.stringify(entry) + '\n');
      const next = applyEvent(prior.snapshot, entry, prior.diagnostics);
-      snapshotCache.set(normalized.id, { snapshot: next, diagnostics: next.diagnostics || [], nextSeq: seq + 1 });
      writeSnapshot(snapshotPath, next);
      return next;
    },
@@ -68,7 +62,6 @@ export function createLiveSessionStore({ cwd = process.cwd(), sessionId } = {})
      const journalPath = getReadableJournalPath(id);
      const snapshotPath = getSnapshotPath(rootDir, id);
      const rebuilt = rebuildSnapshotFromJournal(journalPath, id);
-      snapshotCache.set(id, rebuilt);
      writeSnapshot(snapshotPath, rebuilt.snapshot);
      if (!opts.includeCompleted && COMPLETED_PHASES.has(rebuilt.snapshot.phase)) return null;
      return rebuilt.snapshot;
@@ -105,11 +98,6 @@ function getSnapshotPath(rootDir, id) {
  return path.join(rootDir, safeSessionId(id) + '.snapshot.json');
 }

-function safeSessionId(id) {
-  if (!/^[A-Za-z0-9_-]{1,128}$/.test(id)) throw new Error('invalid session id: ' + id);
-  return id;
-}
-
 function baseSnapshot(id) {
  return {
    id,
@@ -1,7 +1,7 @@
 import fs from 'node:fs';
 import path from 'node:path';

-import { getLiveDir } from '../lib/impeccable-paths.mjs';
+import { getLiveDir, safeSessionId } from '../lib/impeccable-paths.mjs';

 export const SOURCE_ARTIFACT_PREVIEW_MODE = 'source-artifact';

@@ -15,9 +15,7 @@ export function scaffoldSourceArtifactSession({
  previewContent,
  cwd = process.cwd(),
 } = {}) {
-  if (!/^[A-Za-z0-9_-]{1,128}$/.test(String(id || ''))) {
-    throw new Error('invalid source artifact session id');
-  }
+  safeSessionId(id);
  const sourcePath = resolveInside(cwd, sourceFile);
  if (!sourcePath || !fs.existsSync(sourcePath)) throw new Error('source artifact target missing');

@@ -43,7 +41,7 @@ export function scaffoldSourceArtifactSession({
 }

 export function findSourceArtifactManifest(id, cwd = process.cwd()) {
-  if (!/^[A-Za-z0-9_-]{1,128}$/.test(String(id || ''))) return null;
+  try { safeSessionId(id); } catch { return null; }
  const manifestPath = path.join(getLiveDir(cwd), 'previews', id, 'manifest.json');
  let manifest;
  try { manifest = JSON.parse(fs.readFileSync(manifestPath, 'utf-8')); } catch { return null; }
@@ -55,7 +53,7 @@ export function findSourceArtifactManifest(id, cwd = process.cwd()) {
 }

 export function removeSourceArtifactSession(id, cwd = process.cwd()) {
-  if (!/^[A-Za-z0-9_-]{1,128}$/.test(String(id || ''))) return false;
+  try { safeSessionId(id); } catch { return false; }
  const sessionDir = path.join(getLiveDir(cwd), 'previews', id);
  if (!fs.existsSync(sessionDir)) return false;
  fs.rmSync(sessionDir, { recursive: true, force: true });
@@ -1,9 +1,12 @@
 import fs from 'node:fs';
 import path from 'node:path';
-import { createHash } from 'node:crypto';
-import { getLiveDir } from '../lib/impeccable-paths.mjs';
+import { createHash, randomUUID } from 'node:crypto';
+import { getLiveDir, isLiveServerPidReachable } from '../lib/impeccable-paths.mjs';

-const STALE_LOCK_MS = 60_000;
+// Only used to retire a lock whose contents we cannot read (empty or truncated
+// by a crash mid-write). A readable lock's fate is decided by its owner's
+// liveness instead, so a slow critical section is never swept.
+const UNREADABLE_LOCK_STALE_MS = 60_000;

 export function sourceLockPath(file, cwd = process.cwd()) {
  const digest = createHash('sha256').update(path.resolve(cwd, file)).digest('hex').slice(0, 24);
@@ -18,12 +21,24 @@ export function withSourceLockSync(file, owner, fn, {
  const lockPath = sourceLockPath(file, cwd);
  fs.mkdirSync(path.dirname(lockPath), { recursive: true });
  const deadline = Date.now() + Math.max(0, Number(waitMs) || 0);
-  let fd;
-  while (fd === undefined) {
+  // Identifies this acquisition specifically, so release can tell our own lock
+  // from a replacement that some other writer created.
+  const token = randomUUID();
+  let acquired = false;
+
+  while (!acquired) {
    clearStaleLock(lockPath);
+    let fd;
    try {
      fd = fs.openSync(lockPath, 'wx');
-      fs.writeFileSync(fd, JSON.stringify({ owner, pid: process.pid, at: Date.now(), file: path.resolve(cwd, file) }) + '\n');
+      fs.writeFileSync(fd, JSON.stringify({
+        owner,
+        token,
+        pid: process.pid,
+        at: Date.now(),
+        file: path.resolve(cwd, file),
+      }) + '\n');
+      acquired = true;
    } catch (error) {
      if (error?.code !== 'EEXIST') throw error;
      if (Date.now() >= deadline) {
@@ -33,14 +48,15 @@ export function withSourceLockSync(file, owner, fn, {
        throw locked;
      }
      sleepSync(Math.max(1, Math.min(Number(retryMs) || 5, deadline - Date.now())));
+    } finally {
+      try { if (fd !== undefined) fs.closeSync(fd); } catch {}
    }
  }

  try {
    return fn();
  } finally {
-    try { if (fd !== undefined) fs.closeSync(fd); } catch {}
-    try { fs.unlinkSync(lockPath); } catch {}
+    releaseOwnLock(lockPath, token);
  }
 }

@@ -48,9 +64,42 @@ function sleepSync(ms) {
  Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms);
 }

+function readLock(lockPath) {
+  try { return JSON.parse(fs.readFileSync(lockPath, 'utf-8')); } catch { return null; }
+}
+
+/**
+ * Remove the lock only if it is still the one this call created. If a sweeper
+ * judged our lock stale and another writer replaced it, unlinking here would
+ * end *their* critical section and admit a third writer to the same file.
+ */
+function releaseOwnLock(lockPath, token) {
+  const held = readLock(lockPath);
+  if (held && held.token !== token) return;
+  try { fs.unlinkSync(lockPath); } catch {}
+}
+
+/**
+ * A lock is stale when its owner is gone, not when it is old.
+ *
+ * Age alone cuts both ways: it sweeps a live holder whose critical section
+ * outran the timeout (a suspended laptop, a stopped process), letting two
+ * writers into the same source file, while still making every accept on a
+ * crashed holder's file wait out the full timeout. Asking the OS whether the
+ * recorded pid is alive answers both correctly: a dead owner releases at once,
+ * and a live owner keeps its lock however long it needs.
+ */
 function clearStaleLock(lockPath) {
-  try {
-    const stat = fs.statSync(lockPath);
-    if (Date.now() - stat.mtimeMs > STALE_LOCK_MS) fs.unlinkSync(lockPath);
-  } catch {}
+  const held = readLock(lockPath);
+  if (!held) {
+    // Unreadable: either a crash truncated it, or we caught the brief window
+    // between create and write in a live acquisition. mtime distinguishes them.
+    try {
+      const stat = fs.statSync(lockPath);
+      if (Date.now() - stat.mtimeMs > UNREADABLE_LOCK_STALE_MS) fs.unlinkSync(lockPath);
+    } catch { /* gone already */ }
+    return;
+  }
+  if (typeof held.pid === 'number' && isLiveServerPidReachable(held.pid)) return;
+  try { fs.unlinkSync(lockPath); } catch {}
 }
@@ -9,6 +9,8 @@
 import fs from 'node:fs';
 import path from 'node:path';

+import { safeSessionId } from '../lib/impeccable-paths.mjs';
+
 const NUXT_CONFIG_RE = /^nuxt\.config\.(?:js|mjs|cjs|ts|mts|cts)$/;

 export function detectNuxtVueProject(cwd = process.cwd()) {
@@ -36,7 +38,7 @@ export function shouldUseVueComponentInjection(filePath, cwd = process.cwd()) {
 export function vueComponentSessionDir(id, cwd = process.cwd()) {
  const project = detectNuxtVueProject(cwd);
  if (!project) throw new Error('Nuxt project not found');
-  return path.join(cwd, project.componentRoot, id);
+  return path.join(cwd, project.componentRoot, safeSessionId(id));
 }

 export function vueManifestPathForSession(id, cwd = process.cwd()) {
@@ -274,15 +276,29 @@ function matchOpeningTag(markup) {
  } : null;
 }

+/**
+ * Tokenize the attributes of a Vue opening tag.
+ *
+ * The name pattern is deliberately permissive so directive shorthands survive
+ * a round trip: `@click.prevent`, `:aria-label`, `:[dynamicKey]`, `#default`,
+ * and `v-cloak` are all one attribute each. A name-anchored pattern such as
+ * `[A-Za-z_:][\w:.-]*` skips the `@`/`#` sigil and re-matches from the bare
+ * name, which turns `@click="submit"` into a literal `click="submit"` DOM
+ * attribute on Accept. Values are optional so valueless attributes
+ * (`disabled`, `v-cloak`) are recorded rather than dropped.
+ */
 function parseStaticAttrs(attrs) {
  const out = new Map();
-  const re = /([A-Za-z_:][\w:.-]*)\s*=\s*(["'])(.*?)\2/g;
+  const re = /([^\s"'=<>/]+)(?:\s*=\s*(?:(["'])([\s\S]*?)\2|([^\s"'=<>`]+)))?/g;
  let match;
  while ((match = re.exec(attrs))) {
-    out.set(match[1], {
+    const quoted = match[2] !== undefined;
+    const valueless = !quoted && match[4] === undefined;
+    out.set(normalizeVueAttrName(match[1]), {
      raw: match[0],
-      value: match[3],
-      quote: match[2],
+      value: valueless ? '' : (quoted ? match[3] : match[4]),
+      quote: quoted ? match[2] : '"',
+      valueless,
      start: match.index,
      end: match.index + match[0].length,
    });
@@ -290,6 +306,21 @@ function parseStaticAttrs(attrs) {
  return out;
 }

+/**
+ * Collapse Vue's directive shorthands to their canonical form for identity
+ * comparison only (the raw text is what gets written back). Without this, an
+ * original `:aria-label` and a variant `v-bind:aria-label` read as two
+ * different attributes and Accept emits both, which is a Vue compile error.
+ */
+function normalizeVueAttrName(name) {
+  const raw = String(name);
+  if (raw.startsWith('@')) return `v-on:${raw.slice(1)}`;
+  if (raw.startsWith(':')) return `v-bind:${raw.slice(1)}`;
+  if (raw.startsWith('#')) return `v-slot:${raw.slice(1)}`;
+  if (raw.startsWith('.')) return `v-bind:${raw.slice(1)}.prop`;
+  return raw;
+}
+
 export function removeVueComponentSession(id, cwd = process.cwd()) {
  try { fs.rmSync(vueComponentSessionDir(id, cwd), { recursive: true, force: true }); } catch { /* best effort */ }
 }