From 4e381305e15e6fed06febe43c1357b806624242c Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Fri, 17 Jul 2026 13:48:44 -0700 Subject: [PATCH] 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 --- cli/engine/engines/regex/detect-text.mjs | 75 +++++++++--- scripts/benchmark-live-providers.mjs | 34 +----- scripts/benchmark-live.mjs | 29 ++--- scripts/compare-live-benchmarks.mjs | 13 +-- scripts/judge-live-rendered.mjs | 26 +---- scripts/lib/cli-args.mjs | 71 ++++++++++++ scripts/test-suites.mjs | 2 + skill/scripts/lib/impeccable-paths.mjs | 14 +++ skill/scripts/live-accept.mjs | 38 +++++- skill/scripts/live-poll.mjs | 6 +- skill/scripts/live-server.mjs | 81 ++++++++++--- skill/scripts/live/generation-preflight.mjs | 21 +++- skill/scripts/live/generation-publisher.mjs | 64 +++++----- skill/scripts/live/session-store.mjs | 24 +--- skill/scripts/live/source-artifact.mjs | 10 +- skill/scripts/live/source-lock.mjs | 73 ++++++++++-- skill/scripts/live/vue-component.mjs | 41 ++++++- tests/cli-args.test.mjs | 101 ++++++++++++++++ tests/detect-antipatterns-fixtures.test.mjs | 23 +++- .../astro-inset-shadow-stripe.astro | 28 +++++ tests/live-accept.test.mjs | 56 ++++++++- tests/live-generation-preflight.test.mjs | 51 +++++++- tests/live-server.test.mjs | 71 ++++++++++++ tests/live-source-lock.test.mjs | 109 ++++++++++++++++++ tests/live-vue-component.test.mjs | 78 +++++++++++++ 25 files changed, 921 insertions(+), 218 deletions(-) create mode 100644 scripts/lib/cli-args.mjs create mode 100644 tests/cli-args.test.mjs create mode 100644 tests/live-source-lock.test.mjs diff --git a/cli/engine/engines/regex/detect-text.mjs b/cli/engine/engines/regex/detect-text.mjs index ddf1f0d99..af37aa17e 100644 --- a/cli/engine/engines/regex/detect-text.mjs +++ b/cli/engine/engines/regex/detect-text.mjs @@ -43,23 +43,48 @@ function firstOverusedGoogleFont(text) { return extractGoogleFontFamilies(text).find(f => OVERUSED_FONTS.has(f)) || ''; } +// CSS named colors whose channels are equal (achromatic). Anything outside +// this set falls through to the format parsers, and an unrecognized spelling +// stays non-neutral so a real accent is never skipped. +const NEUTRAL_COLOR_KEYWORDS = new Set([ + 'transparent', 'currentcolor', + 'black', 'white', 'gray', 'grey', 'silver', + 'dimgray', 'dimgrey', 'darkgray', 'darkgrey', 'lightgray', 'lightgrey', + 'gainsboro', 'whitesmoke', +]); + +function hexChannels(color) { + const long = color.match(/^#([0-9a-f]{2})([0-9a-f]{2})([0-9a-f]{2})(?:[0-9a-f]{2})?$/i); + if (long) return [parseInt(long[1], 16), parseInt(long[2], 16), parseInt(long[3], 16)]; + const short = color.match(/^#([0-9a-f])([0-9a-f])([0-9a-f])(?:[0-9a-f])?$/i); + if (short) return [1, 2, 3].map((i) => parseInt(short[i] + short[i], 16)); + return null; +} + +/** + * Neutrality test for colors as written in source CSS. + * + * shared/color.mjs's isNeutralColor only parses the computed function forms a + * browser or jsdom emits (rgb/oklch/lab/...) and deliberately reports every + * other spelling as chromatic so an unknown format is never silently skipped. + * That default is wrong for authored CSS, where `#000` and `black` are the + * normal spellings: calling it directly reports a plain black hairline as a + * colored stripe. Handle hex and named neutrals here, then defer. + */ +function isNeutralAuthoredColor(rawColor) { + const c = String(rawColor || '').trim().toLowerCase(); + if (!c) return false; + if (NEUTRAL_COLOR_KEYWORDS.has(c)) return true; + if (/^(?:rgba?|hsla?|oklch|oklab|lab|lch|hwb)\(/i.test(c)) return isNeutralColor(c); + const channels = hexChannels(c); + if (channels) return (Math.max(...channels) - Math.min(...channels)) < 30; + return false; +} + function isNeutralBorderColor(str) { const m = str.match(/solid\s+((?:rgba?|hsla?|oklch|oklab|lab|lch|hwb|color)\([^)]*\)|#[0-9a-f]{3,8}\b|[a-z]+)/i); if (!m) return false; - const c = m[1].toLowerCase(); - if (['gray', 'grey', 'silver', 'white', 'black', 'transparent', 'currentcolor'].includes(c)) return true; - if (/^(?:rgba?|hsla?|oklch|oklab|lab|lch|hwb)\(/i.test(c)) return isNeutralColor(c); - const hex = c.match(/^#([0-9a-f]{2})([0-9a-f]{2})([0-9a-f]{2})$/); - if (hex) { - const [r, g, b] = [parseInt(hex[1], 16), parseInt(hex[2], 16), parseInt(hex[3], 16)]; - return (Math.max(r, g, b) - Math.min(r, g, b)) < 30; - } - const shex = c.match(/^#([0-9a-f])([0-9a-f])([0-9a-f])$/); - if (shex) { - const [r, g, b] = [parseInt(shex[1] + shex[1], 16), parseInt(shex[2] + shex[2], 16), parseInt(shex[3] + shex[3], 16)]; - return (Math.max(r, g, b) - Math.min(r, g, b)) < 30; - } - return false; + return isNeutralAuthoredColor(m[1]); } const REGEX_MATCHERS = [ @@ -357,15 +382,29 @@ function insetStripeColorIsChromatic(rawColor) { const variable = color.match(/^var\(\s*(--[\w-]+)/i); if (variable) return CHROMATIC_SHADOW_TOKEN_RE.test(variable[1]); if (!/^(?:#|rgba?\(|hsla?\(|hwb\(|oklch\(|oklab\(|lch\(|lab\(|color\(|[a-z]+$)/i.test(color)) return false; - return !isNeutralColor(color); + return !isNeutralAuthoredColor(color); } -function scanInsetStripeCss(content, filePath, lineOffset = 0) { +/** + * Blank out comment bodies while preserving every byte offset (and therefore + * every line number) so commented-out CSS is not scanned as live rules. + */ +function blankCssComments(css) { + return css.replace(/\/\*[\s\S]*?\*\//g, (block) => block.replace(/[^\n]/g, ' ')); +} + +function scanInsetStripeCss(rawContent, filePath, lineOffset = 0) { + const content = blankCssComments(rawContent); const findings = []; const ruleRe = /([^{};]+)\{([^{}]*)\}/g; let match; while ((match = ruleRe.exec(content)) !== null) { - const selector = match[1].trim(); + // The selector group is `[^{};]+`, which greedily absorbs the whitespace and + // newlines trailing the previous rule. Advance past that run before deriving + // the line, or every rule after the first reports the preceding line. + const selectorStart = match.index + (match[1].length - match[1].trimStart().length); + const selector = match[1].trim().replace(/\s+/g, ' '); + if (!selector) continue; if (/:(?:hover|focus|focus-visible|focus-within|active|checked|target)\b/i.test(selector)) continue; if (/\[aria-selected\s*[*^$|~]?=\s*["']?true/i.test(selector)) continue; if (/\[aria-current(?!\s*[*^$|~]?=\s*["']?false)/i.test(selector)) continue; @@ -390,7 +429,7 @@ function scanInsetStripeCss(content, filePath, lineOffset = 0) { if (!((ax >= 3 && ax <= 12 && ay === 0) || (ay >= 3 && ay <= 12 && ax === 0))) continue; if (!insetStripeColorIsChromatic(shadow[9])) continue; const edge = ay === 0 ? (x > 0 ? 'left' : 'right') : (y > 0 ? 'top' : 'bottom'); - const line = lineOffset + content.slice(0, match.index).split('\n').length; + const line = lineOffset + content.slice(0, selectorStart).split('\n').length; findings.push(finding('side-tab', filePath, `${selector} — inset box-shadow ${ay === 0 ? ax : ay}px stripe (${edge})`, line)); break; } diff --git a/scripts/benchmark-live-providers.mjs b/scripts/benchmark-live-providers.mjs index 274348c06..4b99e8956 100644 --- a/scripts/benchmark-live-providers.mjs +++ b/scripts/benchmark-live-providers.mjs @@ -9,6 +9,7 @@ import { fileURLToPath } from 'node:url'; import { bootFixtureSession, FIXTURES_DIR } from '../tests/live-e2e/session.mjs'; import { createFakeAgent } from '../tests/live-e2e/agent.mjs'; +import { parseArgs, positiveIntFlag } from './lib/cli-args.mjs'; import { clickAccept, clickGo, @@ -34,7 +35,7 @@ const ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '..'); const FIXTURE_NAME = 'vite8-react-brand-fidelity'; const SOURCE_FILE = 'src/App.jsx'; const args = parseArgs(process.argv.slice(2)); -const iterations = positiveInt(args.iterations, 1); +const iterations = positiveIntFlag(args.iterations, 1); const providers = csv(args.providers || 'anthropic,openai,google'); const strategies = csv(args.strategies || Object.keys(STRATEGIES).join(',')); const outputPath = args.output ? resolve(ROOT, String(args.output)) : null; @@ -495,41 +496,10 @@ async function persist(report, file) { process.stderr.write(`[live-provider-bench] wrote ${file}\n`); } -function parseArgs(argv) { - const out = {}; - for (let position = 0; position < argv.length; position += 1) { - const arg = argv[position]; - if (!arg.startsWith('--')) continue; - const body = arg.slice(2); - const index = body.indexOf('='); - if (index !== -1) { - out[camel(body.slice(0, index))] = body.slice(index + 1); - continue; - } - const next = argv[position + 1]; - if (next !== undefined && !next.startsWith('--')) { - out[camel(body)] = next; - position += 1; - } else { - out[camel(body)] = true; - } - } - return out; -} - -function camel(value) { - return value.replace(/-([a-z])/g, (_, letter) => letter.toUpperCase()); -} - function csv(value) { return String(value).split(',').map((item) => item.trim()).filter(Boolean); } -function positiveInt(value, fallback) { - const parsed = Number.parseInt(value, 10); - return Number.isFinite(parsed) && parsed > 0 ? parsed : fallback; -} - function roundMs(value) { return Number(Number(value).toFixed(2)); } diff --git a/scripts/benchmark-live.mjs b/scripts/benchmark-live.mjs index 0256f3098..5e6755622 100644 --- a/scripts/benchmark-live.mjs +++ b/scripts/benchmark-live.mjs @@ -15,6 +15,7 @@ import { waitForCycling, waitForHandshake, } from '../tests/live-e2e/ui.mjs'; +import { boolFlag, parseArgs, positiveIntFlag } from './lib/cli-args.mjs'; import { buildInteractionRun, assembleSplitProgressiveOutput, @@ -26,18 +27,19 @@ import { const ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '..'); const args = parseArgs(process.argv.slice(2)); const fixtureName = String(args.fixture || 'vite8-react-plain'); -const iterations = positiveInt(args.iterations, 5); +const iterations = positiveIntFlag(args.iterations, 5); const agentMode = args.agent === 'llm' ? 'llm' : 'fake'; const scenario = args.scenario === 'annotated' ? 'annotated' : 'plain'; const delivery = args.delivery === 'progressive' ? 'progressive' : 'atomic'; -const simulatedTailMs = positiveInt(args.simulatedTailMs, 0); +const simulatedTailMs = positiveIntFlag(args.simulatedTailMs, 0); +const quiet = boolFlag(args.quiet); const outputPath = args.output ? resolve(ROOT, String(args.output)) : null; const fixture = JSON.parse(await readFile(join(FIXTURES_DIR, fixtureName, 'fixture.json'), 'utf-8')); if (!fixture.runtime) throw new Error(`fixture ${fixtureName} has no runtime configuration`); if (fixture.runtime.mode === 'insert') throw new Error('live benchmark currently measures replace-mode fixtures only'); const { chromium } = await import('playwright'); -const browser = await chromium.launch({ headless: args.headed !== true }); +const browser = await chromium.launch({ headless: !boolFlag(args.headed) }); const recorder = createTraceRecorder(); let session; @@ -56,7 +58,7 @@ try { progressive: delivery === 'progressive', progressiveDelayMs: delivery === 'progressive' ? simulatedTailMs : 0, atomicDelayMs: delivery === 'atomic' ? simulatedTailMs : 0, - log: args.quiet ? () => {} : (message) => process.stderr.write(`[live-bench] ${message}\n`), + log: quiet ? () => {} : (message) => process.stderr.write(`[live-bench] ${message}\n`), }); recorder.mark('setup.handshake.start'); @@ -107,7 +109,7 @@ try { assertScenarioEvidence(run, scenario); runs.push(run); - if (!args.quiet) process.stderr.write(formatRun(runs.at(-1)) + '\n'); + if (!quiet) process.stderr.write(formatRun(runs.at(-1)) + '\n'); await clickDiscard(session.page); await waitForReset(session.page); } @@ -277,23 +279,6 @@ function wrapTargetFromPickedElement(event) { }; } -function parseArgs(argv) { - const out = {}; - for (const arg of argv) { - if (!arg.startsWith('--')) continue; - const body = arg.slice(2); - const index = body.indexOf('='); - if (index === -1) out[body] = true; - else out[body.slice(0, index)] = body.slice(index + 1); - } - return out; -} - -function positiveInt(value, fallback) { - const parsed = Number.parseInt(value, 10); - return Number.isFinite(parsed) && parsed > 0 ? parsed : fallback; -} - function formatRun(run) { return `[live-bench] run ${run.iteration}: first=${run.goToFirstVariantMs}ms all=${run.goToAllVariantsMs}ms generation=${run.generationMs}ms overhead=${run.impeccableOverheadMs}ms`; } diff --git a/scripts/compare-live-benchmarks.mjs b/scripts/compare-live-benchmarks.mjs index 094af5531..7c98f51e6 100644 --- a/scripts/compare-live-benchmarks.mjs +++ b/scripts/compare-live-benchmarks.mjs @@ -3,6 +3,7 @@ import { readFile } from 'node:fs/promises'; import { resolve } from 'node:path'; +import { parseArgs } from './lib/cli-args.mjs'; import { compareModelBackedReports } from './lib/live-benchmark.mjs'; const args = parseArgs(process.argv.slice(2)); @@ -30,18 +31,8 @@ async function readReport(file, delivery) { return report; } -function parseArgs(argv) { - const out = {}; - for (const arg of argv) { - if (!arg.startsWith('--')) continue; - const index = arg.indexOf('='); - if (index > 2) out[arg.slice(2, index)] = arg.slice(index + 1); - } - return out; -} - function ratioArg(value, fallback) { - if (value == null) return fallback; + if (value == null || value === true) return fallback; const parsed = Number(value); if (!Number.isFinite(parsed) || parsed < 0 || parsed >= 1) throw new Error(`invalid threshold ratio: ${value}`); return parsed; diff --git a/scripts/judge-live-rendered.mjs b/scripts/judge-live-rendered.mjs index 1559fa59a..db308df9e 100644 --- a/scripts/judge-live-rendered.mjs +++ b/scripts/judge-live-rendered.mjs @@ -7,6 +7,7 @@ import { fileURLToPath } from 'node:url'; import Anthropic from '@anthropic-ai/sdk'; import { FIXTURES_DIR } from '../tests/live-e2e/session.mjs'; +import { parseArgs } from './lib/cli-args.mjs'; import { loadBenchmarkEnv } from './lib/live-provider-benchmark.mjs'; import { buildRenderedReviewContext, @@ -83,28 +84,3 @@ function required(values, key) { return String(value); } -function parseArgs(argv) { - const out = {}; - for (let index = 0; index < argv.length; index += 1) { - const arg = argv[index]; - if (!arg.startsWith('--')) continue; - const equals = arg.indexOf('='); - if (equals !== -1) { - out[toCamel(arg.slice(2, equals))] = arg.slice(equals + 1); - continue; - } - const key = toCamel(arg.slice(2)); - const next = argv[index + 1]; - if (next && !next.startsWith('--')) { - out[key] = next; - index += 1; - } else { - out[key] = true; - } - } - return out; -} - -function toCamel(value) { - return value.replace(/-([a-z])/g, (_, letter) => letter.toUpperCase()); -} diff --git a/scripts/lib/cli-args.mjs b/scripts/lib/cli-args.mjs new file mode 100644 index 000000000..3356d6b30 --- /dev/null +++ b/scripts/lib/cli-args.mjs @@ -0,0 +1,71 @@ +/** + * One argv parser for the Live benchmark / judging scripts. + * + * These scripts had four subtly different hand-rolled parsers, and the gaps + * failed silently rather than loudly: a parser without the `argv[i + 1]` + * lookahead turned `--iterations 20` into `iterations: true` and benchmarked + * the default 5 runs; a parser without kebab→camel mapping turned + * `--median-target=0.4` into a key nothing read, so the comparison ran against + * the default threshold. Both produce a clean-looking report of the wrong thing. + * + * Supported forms, per flag: + * --flag → true + * --flag=value → 'value' + * --flag value → 'value' (unless `value` itself starts with `--`) + * + * Keys are camel-cased, so `--simulated-tail-ms` and `--simulatedTailMs` both + * land on `simulatedTailMs`. + */ +export function parseArgs(argv) { + const out = {}; + for (let index = 0; index < argv.length; index += 1) { + const arg = argv[index]; + if (!arg.startsWith('--')) continue; + const body = arg.slice(2); + if (!body) continue; + const equals = body.indexOf('='); + if (equals !== -1) { + out[toCamel(body.slice(0, equals))] = body.slice(equals + 1); + continue; + } + const next = argv[index + 1]; + if (next !== undefined && !next.startsWith('--')) { + out[toCamel(body)] = next; + index += 1; + } else { + out[toCamel(body)] = true; + } + } + return out; +} + +export function toCamel(value) { + return String(value).replace(/-([a-z0-9])/gi, (_, char) => char.toUpperCase()); +} + +/** + * Read a boolean flag. `--headed` and `--headed=true` must mean the same thing; + * comparing the raw value against `true` silently ignores the second form. + */ +export function boolFlag(value, fallback = false) { + if (value === undefined) return fallback; + if (typeof value === 'boolean') return value; + const normalized = String(value).trim().toLowerCase(); + if (['', 'true', '1', 'yes', 'on'].includes(normalized)) return true; + if (['false', '0', 'no', 'off'].includes(normalized)) return false; + return fallback; +} + +/** + * Parse a positive integer flag, falling back when absent. Throws on a value + * that was clearly meant as a number but isn't one, so `--iterations abc` + * fails instead of quietly benchmarking the default. + */ +export function positiveIntFlag(value, fallback) { + if (value === undefined || value === true) return fallback; + const parsed = Number.parseInt(String(value), 10); + if (!Number.isFinite(parsed) || parsed <= 0 || String(parsed) !== String(value).trim()) { + throw new Error(`expected a positive integer, got: ${value}`); + } + return parsed; +} diff --git a/scripts/test-suites.mjs b/scripts/test-suites.mjs index 497b198c6..1733af752 100644 --- a/scripts/test-suites.mjs +++ b/scripts/test-suites.mjs @@ -54,6 +54,7 @@ export const SUITES = { runner: 'node', files: [ 'tests/ci-test-plan.test.mjs', + 'tests/cli-args.test.mjs', 'tests/context.test.mjs', 'tests/context-signals.test.mjs', 'tests/critique-storage.test.mjs', @@ -149,6 +150,7 @@ export const SUITES = { 'tests/live-rendered-quality.test.mjs', 'tests/live-server.test.mjs', 'tests/live-session-store.test.mjs', + 'tests/live-source-lock.test.mjs', 'tests/live-target-context.test.mjs', 'tests/live-vue-component.test.mjs', 'tests/live-wrap.test.mjs', diff --git a/skill/scripts/lib/impeccable-paths.mjs b/skill/scripts/lib/impeccable-paths.mjs index 2ccbe7b74..99956c0f3 100644 --- a/skill/scripts/lib/impeccable-paths.mjs +++ b/skill/scripts/lib/impeccable-paths.mjs @@ -104,6 +104,20 @@ export function removeLiveServerInfo(cwd = process.cwd(), options = {}) { } } +/** + * Session IDs become path segments (journals, snapshots, accept receipts, + * preview manifests, generated component dirs). They arrive from CLI `--id` + * arguments and HTTP payloads, so anything containing a separator or `..` must + * be rejected before it reaches path.join, which would happily escape + * `.impeccable/live/`. Real IDs are 8 hex chars; the tests use short slugs. + */ +export function safeSessionId(id) { + if (typeof id !== 'string' || !/^[A-Za-z0-9_-]{1,128}$/.test(id)) { + throw new Error('invalid session id: ' + id); + } + return id; +} + export function getLiveSessionsDir(cwd = process.cwd(), options = {}) { return path.join(getLiveDir(cwd, options), 'sessions'); } diff --git a/skill/scripts/live-accept.mjs b/skill/scripts/live-accept.mjs index 779255d96..8bd2e7507 100644 --- a/skill/scripts/live-accept.mjs +++ b/skill/scripts/live-accept.mjs @@ -16,7 +16,7 @@ import fs from 'node:fs'; import path from 'node:path'; import { isGeneratedFile } from './lib/is-generated.mjs'; -import { getLiveDir } from './lib/impeccable-paths.mjs'; +import { getLiveDir, safeSessionId } from './lib/impeccable-paths.mjs'; import { readBuffer as readManualEditsBuffer, writeBuffer as writeManualEditsBuffer } from './live/manual-edits-buffer.mjs'; import { withSourceLockSync } from './live/source-lock.mjs'; import { @@ -37,6 +37,9 @@ import { const EXTENSIONS = ['.html', '.jsx', '.tsx', '.vue', '.svelte', '.astro']; const ACCEPT_LOCK_WAIT_MS = 1_000; +// Mirrors VARIANT_ID_PATTERN in live/event-validation.mjs, which gates the same +// value arriving over HTTP. +const VARIANT_NUM_PATTERN = /^[0-9]{1,3}$/; // --------------------------------------------------------------------------- // CLI @@ -75,7 +78,19 @@ Output (JSON): const isDiscard = args.includes('--discard'); if (!id) { console.error('Missing --id'); process.exit(1); } + // `id` becomes a path segment (accept receipts, preview manifests, generated + // component dirs). Reject separators and traversal here so one check covers + // every downstream sink. + try { safeSessionId(id); } catch { console.error('Invalid --id'); process.exit(1); } if (!isDiscard && !variantNum) { console.error('Need --discard or --variant N'); process.exit(1); } + // `variantNum` is interpolated into a RegExp and into the markup written back + // to source. The browser and the /events schema both constrain it to digits; + // enforce the same here, or `--variant '.*'` matches the `original` block + // first and silently accepts the original while reporting success. + if (!isDiscard && !VARIANT_NUM_PATTERN.test(variantNum)) { + console.error('Invalid --variant'); + process.exit(1); + } const requestedOperation = isDiscard ? 'discard' : 'accept'; const priorReceipt = readAcceptReceipt(process.cwd(), id); @@ -296,10 +311,25 @@ Output (JSON): } if (isDiscard) { - const result = handleDiscard(id, lines, targetFile); + let result; + // handleDiscard takes the source lock, which throws SOURCE_LOCKED under + // contention. Without this catch the CLI exits non-zero with empty stdout + // and the agent gets no JSON to act on. + try { + result = handleDiscard(id, lines, targetFile); + } catch (err) { + emitResult({ handled: false, file: relFile, error: err.message }); + return; + } emitResult({ handled: true, file: relFile, carbonize: false, ...result }); } else { - const result = handleAccept(id, variantNum, lines, targetFile, paramValues); + let result; + try { + result = handleAccept(id, variantNum, lines, targetFile, paramValues); + } catch (err) { + emitResult({ handled: false, file: relFile, error: err.message }); + return; + } const acceptedOriginalText = result.acceptedOriginalText || ''; delete result.acceptedOriginalText; // Single-line attention-grabber when cleanup is required. The full @@ -1003,7 +1033,7 @@ function searchDir(dir, query, seen, depth) { // --------------------------------------------------------------------------- function acceptReceiptPath(cwd, id) { - return path.join(getLiveDir(cwd), 'accept-receipts', `${id}.json`); + return path.join(getLiveDir(cwd), 'accept-receipts', `${safeSessionId(id)}.json`); } function readAcceptReceipt(cwd, id) { diff --git a/skill/scripts/live-poll.mjs b/skill/scripts/live-poll.mjs index 06e17bb91..19a8b0f9b 100644 --- a/skill/scripts/live-poll.mjs +++ b/skill/scripts/live-poll.mjs @@ -198,7 +198,7 @@ export async function fetchNextEvent(base, token, { } } -export async function augmentEventWithAcceptHandling(event, base, token, { deferReply = false } = {}) { +export async function augmentEventWithAcceptHandling(event, base, token) { if (event.type !== 'accept' && event.type !== 'discard') return event; const __dirname = path.dirname(fileURLToPath(import.meta.url)); @@ -216,10 +216,6 @@ export async function augmentEventWithAcceptHandling(event, base, token, { defer event._acceptResult = { handled: false, mode: 'error', error: err.message }; } - if (deferReply) { - event._completionAck = { ok: false, deferred: true }; - return event; - } await completeAcceptHandling(event, base, token); return event; } diff --git a/skill/scripts/live-server.mjs b/skill/scripts/live-server.mjs index ade02e66d..dee1fa240 100644 --- a/skill/scripts/live-server.mjs +++ b/skill/scripts/live-server.mjs @@ -66,6 +66,10 @@ const DESIGN_MD_PATH = PROJECT_CONTEXT.designPath : null; const DEFAULT_POLL_TIMEOUT = 600_000; // 10 min — agent re-polls on timeout anyway const SSE_HEARTBEAT_INTERVAL = 30_000; // keepalive ping every 30s +// The browser checkpoints for several unrelated reasons (see checkpointPayload +// in live-browser.js). Only these two report that variant availability changed, +// and only they may drive variant_progress / the *_reviewable phases. +const VARIANT_PROGRESS_CHECKPOINT_REASONS = new Set(['variants_progress', 'variants_ready']); // --------------------------------------------------------------------------- // Port detection @@ -163,13 +167,20 @@ function findAvailablePendingEvent(now = Date.now(), types = null) { return selectAvailablePendingEvent(state.pendingEvents, { now, types }); } -function leaseEvent(entry, leaseMs) { - prepareGenerateEventForLease(entry); +async function leaseEvent(entry, leaseMs) { + // Claim the entry before awaiting anything. prepareGenerateEventForLease + // yields to the event loop, and selectAvailablePendingEvent only skips + // entries whose lease is in the future — an unclaimed entry would be handed + // to a second poll in that window and generated twice. + entry.leaseUntil = Date.now() + leaseMs; + await prepareGenerateEventForLease(entry); if (!entry.event?.id) { const idx = state.pendingEvents.indexOf(entry); if (idx !== -1) state.pendingEvents.splice(idx, 1); return entry.event; } + // Re-stamp so the lease window starts when the agent actually receives the + // work, not when scaffolding began. entry.leaseUntil = Date.now() + leaseMs; recordGenerateDelivery(entry); scheduleLeaseFlush(); @@ -186,13 +197,13 @@ function recordGenerateDelivery(entry) { recordAgentPhase(event.id, 'generation_ready', { at }); } -function prepareGenerateEventForLease(entry) { +async function prepareGenerateEventForLease(entry) { const event = entry?.event; if (!event || event.type !== 'generate' || event.scaffoldAttempted) return; recordAgentPhase(event.id, 'picked_up'); recordAgentPhase(event.id, 'scaffolding'); - const result = runGenerationPreflight(event, { + const result = await runGenerationPreflight(event, { cwd: process.cwd(), scriptsDir: __dirname, }); @@ -225,6 +236,14 @@ function recordAgentPhase(id, phase, details = {}) { function recordGenerationCheckpoint(event) { if (!event?.id || event.type !== 'checkpoint') return; if (generationIsFenced(event.id)) return; + // Only checkpoints that report a change in variant availability are + // generation progress. The browser also checkpoints for durability on Tune + // slider drags, resumes, and anchor recovery; treating those as progress + // echoed `variant_progress` straight back to the browser that sent it, which + // remounts the component preview mid-drag (reverting the user's live param + // edit and detaching the popover's element), and permanently latched the + // *_reviewable phases from the wrong trigger, corrupting generation timings. + if (!VARIANT_PROGRESS_CHECKPOINT_REASONS.has(event.reason)) return; const arrived = Number(event.arrivedVariants) || 0; const expected = Number(event.expectedVariants) || 0; if (arrived <= 0 || expected <= 0) return; @@ -335,7 +354,7 @@ function summarizePendingEventForStatus(entry) { const summary = { id: event.id, type: event.type, - leased: !!(entry.leaseUntil && entry.leaseUntil > Date.now()), + leased: isLeased(entry), leaseUntil: entry.leaseUntil || null, }; if (event.type === 'manual_edit_apply') { @@ -427,13 +446,26 @@ function flushPendingPolls() { return; } const [poll] = state.pendingPolls.splice(pollIndex, 1); - poll.resolve(leaseEvent(entry, poll.leaseMs)); + // leaseEvent is async (it may scaffold source), but it claims the entry + // synchronously, so the next loop iteration will not re-select it. Resolve + // the poll when the lease settles rather than awaiting here, so one slow + // scaffold never delays the other parked polls. On the exceptional failure + // path, answer `timeout` so the agent re-polls; the claim stays until the + // lease expires, which keeps a deterministic failure from hot-looping. + leaseEvent(entry, poll.leaseMs).then(poll.resolve, (error) => { + console.error('[live] lease failed for ' + (entry.event?.id || 'unknown') + ': ' + (error?.message || error)); + poll.resolve({ type: 'timeout' }); + }); changed = true; } scheduleLeaseFlush(); if (changed) broadcastAgentPollingIfChanged(); } +function isLeased(entry) { + return !!(entry?.leaseUntil && entry.leaseUntil > Date.now()); +} + function agentPollingConnected() { // A leased event only proves that a poll returned once. The foreground task // may have ended immediately afterward, so only an actively waiting poll is @@ -922,8 +954,19 @@ function handlePollGet(req, res, url) { const types = parsePollTypes(url.searchParams.get('types')); const available = findAvailablePendingEvent(Date.now(), types); if (available) { - res.writeHead(200, { 'Content-Type': 'application/json' }); - res.end(JSON.stringify(leaseEvent(available, leaseMs))); + // Do not await inline: leaseEvent may scaffold source, and this handler runs + // on the server's only thread. The client can disconnect during that window, + // so check the socket before replying. + leaseEvent(available, leaseMs).then((event) => { + if (res.writableEnded || res.destroyed) return; + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify(event)); + }, (error) => { + console.error('[live] lease failed for ' + (available.event?.id || 'unknown') + ': ' + (error?.message || error)); + if (res.writableEnded || res.destroyed) return; + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ type: 'timeout' })); + }); return; } const poll = { resolve, leaseMs, types }; @@ -994,11 +1037,8 @@ function sessionFileMetadataFromPollReply(file) { } function inferSourceEventType(msg = {}, pendingEvents = state.pendingEvents) { - const pendingTypes = new Set( - pendingEvents - .filter((entry) => entry.event?.id === msg.id) - .map((entry) => entry.event?.type), - ); + const entriesForId = pendingEvents.filter((entry) => entry.event?.id === msg.id); + const pendingTypes = new Set(entriesForId.map((entry) => entry.event?.type)); if (msg.type === 'discarded' || msg.type === 'discard') return 'discard'; if (msg.type === 'complete') { if (pendingTypes.has('carbonize_cleanup')) return 'carbonize_cleanup'; @@ -1008,7 +1048,20 @@ function inferSourceEventType(msg = {}, pendingEvents = state.pendingEvents) { // `agent_done` can be the automatic acknowledgement for a carbonize Accept. // New pollers send sourceEventType explicitly; default to generate only for // older callers so a late worker cannot acknowledge a queued Accept. - return msg.type === 'agent_done' || msg.type === 'done' ? 'generate' : undefined; + if (msg.type === 'agent_done' || msg.type === 'done') return 'generate'; + // `error` is reference/live.md's documented failure reply, and parseReplyArgs + // never sets sourceEventType on it (the poller is a fresh process that cannot + // know what it leased). Returning undefined here makes acknowledgePendingEvent + // match *any* event for this id: a stale generate worker's failure silently + // consumed the user's queued Accept, which was then never delivered to any + // agent and left the browser in SAVING forever. Attribute the failure to the + // event this agent actually holds a lease on, and otherwise to `generate` — + // never to a wildcard. If that generate was already retired by an Accept, the + // ack simply finds no match, which is the correct outcome for a stale reply. + if (msg.type === 'error') { + return entriesForId.find(isLeased)?.event?.type || 'generate'; + } + return undefined; } function handlePollPost(req, res) { diff --git a/skill/scripts/live/generation-preflight.mjs b/skill/scripts/live/generation-preflight.mjs index d2dcc7f7f..a52bd2052 100644 --- a/skill/scripts/live/generation-preflight.mjs +++ b/skill/scripts/live/generation-preflight.mjs @@ -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'); diff --git a/skill/scripts/live/generation-publisher.mjs b/skill/scripts/live/generation-publisher.mjs index d13f65bbf..2b64895f2 100644 --- a/skill/scripts/live/generation-publisher.mjs +++ b/skill/scripts/live/generation-publisher.mjs @@ -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; +} diff --git a/skill/scripts/live/session-store.mjs b/skill/scripts/live/session-store.mjs index 52e41d722..40a81c3d3 100644 --- a/skill/scripts/live/session-store.mjs +++ b/skill/scripts/live/session-store.mjs @@ -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, diff --git a/skill/scripts/live/source-artifact.mjs b/skill/scripts/live/source-artifact.mjs index 53f9bff3b..c4f205c01 100644 --- a/skill/scripts/live/source-artifact.mjs +++ b/skill/scripts/live/source-artifact.mjs @@ -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 }); diff --git a/skill/scripts/live/source-lock.mjs b/skill/scripts/live/source-lock.mjs index dd82989bd..9ccc557be 100644 --- a/skill/scripts/live/source-lock.mjs +++ b/skill/scripts/live/source-lock.mjs @@ -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 {} } diff --git a/skill/scripts/live/vue-component.mjs b/skill/scripts/live/vue-component.mjs index c8f4d0825..573bf6849 100644 --- a/skill/scripts/live/vue-component.mjs +++ b/skill/scripts/live/vue-component.mjs @@ -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 */ } } diff --git a/tests/cli-args.test.mjs b/tests/cli-args.test.mjs new file mode 100644 index 000000000..0acaa1a9a --- /dev/null +++ b/tests/cli-args.test.mjs @@ -0,0 +1,101 @@ +/** + * Tests for scripts/lib/cli-args.mjs — the shared argv parser for the Live + * benchmark / judging scripts. + * Run with: node --test tests/cli-args.test.mjs + */ + +import { describe, it } from 'node:test'; +import assert from 'node:assert/strict'; + +import { boolFlag, parseArgs, positiveIntFlag, toCamel } from '../scripts/lib/cli-args.mjs'; + +describe('parseArgs', () => { + it('reads space-separated values', () => { + // The regression: without the argv[i+1] lookahead this yielded + // {fixture: true, iterations: true}, silently benchmarking the defaults. + assert.deepEqual( + parseArgs(['--fixture', 'vite8-react-modal', '--iterations', '20']), + { fixture: 'vite8-react-modal', iterations: '20' }, + ); + }); + + it('reads --flag=value values', () => { + assert.deepEqual(parseArgs(['--fixture=vite8-react-plain']), { fixture: 'vite8-react-plain' }); + }); + + it('treats a flag followed by another flag as boolean', () => { + assert.deepEqual(parseArgs(['--headed', '--quiet']), { headed: true, quiet: true }); + }); + + it('treats a trailing flag as boolean', () => { + assert.deepEqual(parseArgs(['--append']), { append: true }); + }); + + it('camel-cases kebab keys so both spellings land on one key', () => { + assert.deepEqual(parseArgs(['--simulated-tail-ms=250']), { simulatedTailMs: '250' }); + assert.deepEqual(parseArgs(['--simulatedTailMs=250']), { simulatedTailMs: '250' }); + assert.deepEqual(parseArgs(['--median-target', '0.4']), { medianTarget: '0.4' }); + }); + + it('keeps a value that contains an equals sign intact', () => { + assert.deepEqual(parseArgs(['--model=claude-sonnet-4-6=x']), { model: 'claude-sonnet-4-6=x' }); + }); + + it('ignores positional args and a bare --', () => { + assert.deepEqual(parseArgs(['positional', '--', '--real', 'v']), { real: 'v' }); + }); + + it('lets a later occurrence win', () => { + assert.deepEqual(parseArgs(['--agent', 'fake', '--agent', 'llm']), { agent: 'llm' }); + }); +}); + +describe('toCamel', () => { + it('upcases after hyphens only', () => { + assert.equal(toCamel('simulated-tail-ms'), 'simulatedTailMs'); + assert.equal(toCamel('p95-target'), 'p95Target'); + assert.equal(toCamel('already'), 'already'); + }); +}); + +describe('boolFlag', () => { + it('accepts the bare-flag sentinel and the explicit spellings alike', () => { + // --headed and --headed=true must not diverge. + assert.equal(boolFlag(true), true); + assert.equal(boolFlag('true'), true); + assert.equal(boolFlag('1'), true); + assert.equal(boolFlag('yes'), true); + assert.equal(boolFlag(''), true); + }); + + it('recognizes negative spellings', () => { + assert.equal(boolFlag('false'), false); + assert.equal(boolFlag('0'), false); + assert.equal(boolFlag('no'), false); + }); + + it('falls back when absent or unrecognized', () => { + assert.equal(boolFlag(undefined), false); + assert.equal(boolFlag(undefined, true), true); + assert.equal(boolFlag('maybe', true), true); + }); +}); + +describe('positiveIntFlag', () => { + it('parses positive integers', () => { + assert.equal(positiveIntFlag('20', 5), 20); + }); + + it('falls back when absent or given as a bare flag', () => { + assert.equal(positiveIntFlag(undefined, 5), 5); + assert.equal(positiveIntFlag(true, 5), 5); + }); + + it('throws rather than silently using the default', () => { + // Quietly benchmarking 5 iterations when 20 were asked for is the failure + // this replaces. + for (const bad of ['abc', '0', '-3', '2.5', '20x']) { + assert.throws(() => positiveIntFlag(bad, 5), /positive integer/, `accepted ${bad}`); + } + }); +}); diff --git a/tests/detect-antipatterns-fixtures.test.mjs b/tests/detect-antipatterns-fixtures.test.mjs index 0339faa73..7b0902097 100644 --- a/tests/detect-antipatterns-fixtures.test.mjs +++ b/tests/detect-antipatterns-fixtures.test.mjs @@ -19,7 +19,16 @@ const __dirname = path.dirname(fileURLToPath(import.meta.url)); const FIXTURES = path.join(__dirname, 'fixtures', 'antipatterns'); describe('detectText - Astro structural CSS fixtures', () => { - const SHOULD_FLAG = ['Kinpaku Edge', 'Patina Edge', 'Accent Edge', 'Signal Blue Edge']; + const SHOULD_FLAG = [ + 'Kinpaku Edge', + 'Patina Edge', + 'Accent Edge', + 'Signal Blue Edge', + 'Chromatic Hex Edge', + 'Named Red Edge', + 'Chromatic Rgb Edge', + 'Chromatic Oklch Edge', + ]; const SHOULD_PASS = [ 'Neutral Shadow Token', 'Current Color Edge', @@ -28,6 +37,18 @@ describe('detectText - Astro structural CSS fixtures', () => { 'Thick Fill Edge', 'Blurred Edge', 'Narrow Artwork', + // Authored CSS spells neutrals as hex and keywords. isNeutralColor only + // parses the computed function forms and reports everything else as + // chromatic, so routing these through it flagged plain black and gray + // hairlines as the "colored stripe" AI tell. + 'Black Hex Edge', + 'Black Named Edge', + 'Gray Hex Edge', + 'Dimgray Named Edge', + 'Black Rgb Edge', + 'Shorthand Neutral Hex Edge', + // Commented-out CSS is not a live rule. + 'Commented Out Edge', ]; it('Astro style blocks flag unresolved chromatic inset stripes only', () => { diff --git a/tests/fixtures/antipatterns/astro-inset-shadow-stripe.astro b/tests/fixtures/antipatterns/astro-inset-shadow-stripe.astro index 8f4989977..023855c3e 100644 --- a/tests/fixtures/antipatterns/astro-inset-shadow-stripe.astro +++ b/tests/fixtures/antipatterns/astro-inset-shadow-stripe.astro @@ -10,6 +10,10 @@ const title = 'Astro inset shadow stripe regression';

Patina Edge

Accent Edge

Signal Blue Edge

+

Chromatic Hex Edge

+

Named Red Edge

+

Chromatic Rgb Edge

+

Chromatic Oklch Edge

Should pass

@@ -20,6 +24,13 @@ const title = 'Astro inset shadow stripe regression';

Thick Fill Edge

Blurred Edge

Narrow Artwork

+

Black Hex Edge

+

Black Named Edge

+

Gray Hex Edge

+

Dimgray Named Edge

+

Black Rgb Edge

+

Shorthand Neutral Hex Edge

+

Commented Out Edge

@@ -35,4 +46,21 @@ const title = 'Astro inset shadow stripe regression'; [data-case="Thick Fill Edge"] { box-shadow: inset 14px 0 0 var(--brand-accent); } [data-case="Blurred Edge"] { box-shadow: inset 3px 0 5px var(--brand-accent); } [data-case="Narrow Artwork"] { width: 24px; box-shadow: inset 3px 0 0 var(--brand-accent); } + + /* Literal colors: authored CSS spells neutrals as hex and keywords, not as + the computed rgb()/oklch() forms a browser emits. */ + [data-case="Chromatic Hex Edge"] { box-shadow: inset 4px 0 0 #6366f1; } + [data-case="Named Red Edge"] { box-shadow: inset 4px 0 0 red; } + [data-case="Chromatic Rgb Edge"] { box-shadow: inset 4px 0 0 rgb(99, 102, 241); } + [data-case="Chromatic Oklch Edge"] { box-shadow: inset 4px 0 0 oklch(65% 0.18 250); } + [data-case="Black Hex Edge"] { box-shadow: inset 4px 0 0 #000; } + [data-case="Black Named Edge"] { box-shadow: inset 4px 0 0 black; } + [data-case="Gray Hex Edge"] { box-shadow: inset 4px 0 0 #e5e7eb; } + [data-case="Dimgray Named Edge"] { box-shadow: inset 4px 0 0 dimgray; } + [data-case="Black Rgb Edge"] { box-shadow: inset 4px 0 0 rgb(0, 0, 0); } + [data-case="Shorthand Neutral Hex Edge"] { box-shadow: inset 4px 0 0 #1118; } + + /* Commented-out rules are not live CSS. + [data-case="Commented Out Edge"] { box-shadow: inset 4px 0 0 var(--brand-accent); } + */ diff --git a/tests/live-accept.test.mjs b/tests/live-accept.test.mjs index 81f6098a2..82df1e730 100644 --- a/tests/live-accept.test.mjs +++ b/tests/live-accept.test.mjs @@ -9,7 +9,7 @@ import { existsSync, mkdtempSync, writeFileSync, readFileSync, rmSync } from 'no import { dirname, join, resolve } from 'node:path'; import { tmpdir } from 'node:os'; import { fileURLToPath } from 'node:url'; -import { execFileSync } from 'node:child_process'; +import { execFileSync, spawnSync } from 'node:child_process'; import { scaffoldSourceArtifactSession } from '../skill/scripts/live/source-artifact.mjs'; const __dirname = dirname(fileURLToPath(import.meta.url)); @@ -30,6 +30,60 @@ function runAccept(cwd, args) { } } +describe('live-accept — session id validation', () => { + let tmp; + beforeEach(() => { tmp = mkdtempSync(join(tmpdir(), 'impeccable-accept-id-')); }); + afterEach(() => rmSync(tmp, { recursive: true, force: true })); + + // --id becomes a path segment for the accept receipt. Traversal here wrote + // JSON to arbitrary absolute paths (e.g. `--id ../../../../etc/evil`). + for (const id of ['../../../../etc/evil', 'a/b', '..', 'a\\b', '']) { + it(`refuses --id ${JSON.stringify(id)} without writing a receipt`, () => { + const res = spawnSync('node', [ACCEPT, '--id', id, '--discard'], { + cwd: tmp, + encoding: 'utf-8', + }); + assert.equal(res.status, 1, 'must exit non-zero'); + assert.match(res.stderr, /Invalid --id|Missing --id/); + assert.equal(existsSync(join(tmp, '.impeccable', 'live', 'accept-receipts')), false); + }); + } + + it('still accepts a well-formed id', () => { + const res = spawnSync('node', [ACCEPT, '--id', 'ab12cd34', '--discard'], { + cwd: tmp, + encoding: 'utf-8', + }); + assert.doesNotMatch(res.stderr || '', /Invalid --id/); + }); + + // --variant is interpolated into a RegExp and into the markup written back to + // source. `.*` matched the `original` block first, so the CLI reported a + // successful accept while actually restoring the original. + for (const variant of ['.*', '[12]', 'original', '1e2', '']) { + it(`refuses --variant ${JSON.stringify(variant)} rather than matching by regex`, () => { + writeFileSync(join(tmp, 'page.html'), [ + '', + '
ORIGINAL CONTENT
', + '
VARIANT ONE
', + '', + '', + ].join('\n')); + const res = spawnSync('node', [ACCEPT, '--id', 'ab12cd34', '--variant', variant], { + cwd: tmp, + encoding: 'utf-8', + }); + assert.equal(res.status, 1); + assert.match(res.stderr, /Invalid --variant|Need --discard/); + assert.match( + readFileSync(join(tmp, 'page.html'), 'utf-8'), + /impeccable-variants-start/, + 'a rejected variant must leave the wrapper untouched', + ); + }); + } +}); + describe('live-accept — isolated source artifacts', () => { let tmp; beforeEach(() => { tmp = mkdtempSync(join(tmpdir(), 'impeccable-accept-isolated-')); }); diff --git a/tests/live-generation-preflight.test.mjs b/tests/live-generation-preflight.test.mjs index 76be7aead..6e614d5ba 100644 --- a/tests/live-generation-preflight.test.mjs +++ b/tests/live-generation-preflight.test.mjs @@ -64,9 +64,9 @@ test('builds an insert preflight from the anchor locator', () => { ]); }); -test('returns scaffold metadata without exposing child-process details', () => { +test('returns scaffold metadata without exposing child-process details', async () => { const calls = []; - const result = runGenerationPreflight({ + const result = await runGenerationPreflight({ type: 'generate', id: 'session-3', count: 1, @@ -74,9 +74,9 @@ test('returns scaffold metadata without exposing child-process details', () => { }, { scriptsDir: SCRIPTS_DIR, cwd: '/tmp/example', - execFileSyncImpl(file, args, options) { + async execFileImpl(file, args, options) { calls.push({ file, args, options }); - return '{"file":"src/App.jsx","insertLine":12}\n'; + return { stdout: '{"file":"src/App.jsx","insertLine":12}\n', stderr: '' }; }, }); @@ -86,8 +86,8 @@ test('returns scaffold metadata without exposing child-process details', () => { assert.equal(calls[0].options.cwd, '/tmp/example'); }); -test('skips preflight when the picker has no source locator', () => { - const result = runGenerationPreflight({ +test('skips preflight when the picker has no source locator', async () => { + const result = await runGenerationPreflight({ type: 'generate', id: 'session-4', count: 3, @@ -96,3 +96,42 @@ test('skips preflight when the picker has no source locator', () => { assert.deepEqual(result, { ok: false, skipped: true, reason: 'insufficient_locator' }); }); + +test('yields to the event loop instead of blocking on the child process', async () => { + // The server is single-threaded and leases polls through this call. A + // synchronous spawn froze every other request (Accept, Discard, SSE) for the + // scaffold's full duration — measured at ~7.6s on a large repo. + let tickedDuringPreflight = false; + const pending = runGenerationPreflight({ + type: 'generate', + id: 'session-async', + count: 1, + element: { classes: ['hero'] }, + }, { + scriptsDir: SCRIPTS_DIR, + execFileImpl: () => new Promise((resolve) => { + setTimeout(() => resolve({ stdout: '{"file":"src/App.jsx"}\n', stderr: '' }), 25); + }), + }); + setTimeout(() => { tickedDuringPreflight = true; }, 5); + const result = await pending; + assert.equal(result.ok, true); + assert.equal(tickedDuringPreflight, true, 'the event loop must stay responsive during preflight'); +}); + +test('reports a child-process failure without leaking internals or throwing', async () => { + const error = new Error('spawn failed'); + error.stderr = 'live-wrap.mjs: element not found\n'; + const result = await runGenerationPreflight({ + type: 'generate', + id: 'session-fail', + count: 1, + element: { classes: ['hero'] }, + }, { + scriptsDir: SCRIPTS_DIR, + execFileImpl: () => Promise.reject(error), + }); + assert.equal(result.ok, false); + assert.equal(result.error, 'live-wrap.mjs: element not found'); + assert.ok(typeof result.durationMs === 'number'); +}); diff --git a/tests/live-server.test.mjs b/tests/live-server.test.mjs index eafd886dc..77491f5cf 100644 --- a/tests/live-server.test.mjs +++ b/tests/live-server.test.mjs @@ -3084,4 +3084,75 @@ colors: {} const data = await postRes.json(); assert.match(data.error, /freeformPrompt or annotations/i); }); + + // A stale generate worker's `error` reply used to acknowledge *any* pending + // event for its id, because inferSourceEventType returned undefined and + // acknowledgePendingEvent treats that as a wildcard. It ate the user's queued + // Accept, which was then never handed to an agent: the browser sat in SAVING + // forever and a restart could not requeue it. + it('a stale generate error reply does not consume a queued accept', async () => { + await drainPolls(server); + const id = 'ee55ff66'; + + await fetch(`http://localhost:${server.port}/events`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ + token: server.token, + type: 'generate', + id, + action: 'impeccable', + count: 1, + pageUrl: '/', + element: { outerHTML: '' }, + }), + }); + + // Agent leases the generate. + const leased = await (await fetch( + `http://localhost:${server.port}/poll?token=${server.token}&timeout=200&leaseMs=60000`, + )).json(); + assert.equal(leased.id, id); + assert.equal(leased.type, 'generate'); + + // User accepts. This retires the pending generate and queues the accept. + await fetch(`http://localhost:${server.port}/events`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ token: server.token, type: 'accept', id, variantId: '1' }), + }); + + // The stale generate worker now fails, using live.md's documented reply. + const errRes = await fetch(`http://localhost:${server.port}/poll`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ token: server.token, id, type: 'error', message: 'late failure' }), + }); + assert.equal(errRes.status, 200); + + const status = await (await fetch( + `http://localhost:${server.port}/status?token=${server.token}`, + )).json(); + assert.equal( + status.pendingEvents.some((e) => e.id === id && e.type === 'accept'), + true, + 'the queued accept must survive a stale generate error', + ); + + // And it must still be deliverable to the next agent that polls. + const next = await (await fetch( + `http://localhost:${server.port}/poll?token=${server.token}&timeout=500&leaseMs=30000`, + )).json(); + assert.equal(next.id, id); + assert.equal(next.type, 'accept', 'the accept must reach an agent'); + + // Acknowledge the accept explicitly. drainPolls replies `done`, which maps + // to `generate`, so it can never retire an accept and would re-lease it in + // a loop forever. + await fetch(`http://localhost:${server.port}/poll`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ token: server.token, id, type: 'complete', sourceEventType: 'accept' }), + }); + }); }); diff --git a/tests/live-source-lock.test.mjs b/tests/live-source-lock.test.mjs new file mode 100644 index 000000000..44ef68f56 --- /dev/null +++ b/tests/live-source-lock.test.mjs @@ -0,0 +1,109 @@ +/** + * Tests for live/source-lock.mjs — the per-source-file mutex guarding the + * accept/publish critical sections. + * Run with: node --test tests/live-source-lock.test.mjs + */ + +import { describe, it, beforeEach, afterEach } from 'node:test'; +import assert from 'node:assert/strict'; +import { existsSync, mkdtempSync, readFileSync, rmSync, utimesSync, writeFileSync, mkdirSync } from 'node:fs'; +import { dirname, join } from 'node:path'; +import { tmpdir } from 'node:os'; + +import { sourceLockPath, withSourceLockSync } from '../skill/scripts/live/source-lock.mjs'; + +const TARGET = 'src/page.html'; + +describe('live source-lock', () => { + let tmp; + + beforeEach(() => { + tmp = mkdtempSync(join(tmpdir(), 'impeccable-source-lock-')); + mkdirSync(join(tmp, 'src'), { recursive: true }); + writeFileSync(join(tmp, TARGET), '
original
\n'); + }); + afterEach(() => rmSync(tmp, { recursive: true, force: true })); + + const writeLock = (body) => { + const lockPath = sourceLockPath(TARGET, tmp); + mkdirSync(dirname(lockPath), { recursive: true }); + writeFileSync(lockPath, JSON.stringify(body) + '\n'); + return lockPath; + }; + + it('runs the critical section and releases the lock', () => { + const lockPath = sourceLockPath(TARGET, tmp); + const result = withSourceLockSync(TARGET, 'accept:a', () => { + assert.equal(existsSync(lockPath), true, 'lock must exist while held'); + return 'done'; + }, { cwd: tmp }); + assert.equal(result, 'done'); + assert.equal(existsSync(lockPath), false, 'lock must be released'); + }); + + it('throws SOURCE_LOCKED when a live owner holds the lock', () => { + // process.pid is this very process, so the recorded owner is alive. + writeLock({ owner: 'publish:x', token: 'other', pid: process.pid, at: Date.now() }); + assert.throws( + () => withSourceLockSync(TARGET, 'accept:a', () => 'should not run', { cwd: tmp }), + (err) => err.code === 'SOURCE_LOCKED', + ); + }); + + it('does not sweep a live owner’s lock no matter how old it is', () => { + // Age alone must not make a lock stale: a holder suspended mid-write would + // otherwise have a second writer admitted to the same source file. + const lockPath = writeLock({ owner: 'publish:x', token: 'other', pid: process.pid, at: 0 }); + const ancient = new Date(Date.now() - 10 * 60_000); + utimesSync(lockPath, ancient, ancient); + assert.throws( + () => withSourceLockSync(TARGET, 'accept:a', () => 'should not run', { cwd: tmp }), + (err) => err.code === 'SOURCE_LOCKED', + 'an old but live lock was stolen', + ); + }); + + it('reclaims a lock whose owner process is gone, without waiting out a timeout', () => { + // PID 2^22 is above the platform maximum, so it can never be running. + writeLock({ owner: 'publish:crashed', token: 'other', pid: 4194304, at: Date.now() }); + const result = withSourceLockSync(TARGET, 'accept:a', () => 'acquired', { cwd: tmp }); + assert.equal(result, 'acquired', 'a crashed holder must not block the next writer'); + }); + + it('leaves a replacement lock alone when its own was swept', () => { + // Simulates: our lock got reclaimed and another writer now owns the file. + // Releasing must not unlink the replacement and admit a third writer. + const lockPath = sourceLockPath(TARGET, tmp); + withSourceLockSync(TARGET, 'accept:a', () => { + writeFileSync(lockPath, JSON.stringify({ + owner: 'publish:other', token: 'a-different-token', pid: process.pid, at: Date.now(), + }) + '\n'); + }, { cwd: tmp }); + assert.equal(existsSync(lockPath), true, 'another owner’s lock must survive our release'); + assert.match(readFileSync(lockPath, 'utf-8'), /a-different-token/); + }); + + it('retires an unreadable lock only once it is older than the fallback window', () => { + const lockPath = writeLock(''); + assert.throws( + () => withSourceLockSync(TARGET, 'accept:a', () => 'x', { cwd: tmp }), + (err) => err.code === 'SOURCE_LOCKED', + 'a fresh unreadable lock is an in-flight acquisition, not garbage', + ); + const ancient = new Date(Date.now() - 120_000); + utimesSync(lockPath, ancient, ancient); + assert.equal( + withSourceLockSync(TARGET, 'accept:a', () => 'acquired', { cwd: tmp }), + 'acquired', + 'a stale unreadable lock must be retired', + ); + }); + + it('releases the lock even when the critical section throws', () => { + const lockPath = sourceLockPath(TARGET, tmp); + assert.throws(() => withSourceLockSync(TARGET, 'accept:a', () => { + throw new Error('boom'); + }, { cwd: tmp }), /boom/); + assert.equal(existsSync(lockPath), false, 'a thrown critical section must not leak the lock'); + }); +}); diff --git a/tests/live-vue-component.test.mjs b/tests/live-vue-component.test.mjs index 5aec55cfb..b552fadf0 100644 --- a/tests/live-vue-component.test.mjs +++ b/tests/live-vue-component.test.mjs @@ -129,6 +129,72 @@ describe('Nuxt Vue component preview', () => { assert.equal(existsSync(join(tmp, result.componentDir, 'v1.vue')), true, 'imported SFC remains until Live shutdown'); }); + it('preserves original root directives and valueless attrs a variant omits', () => { + const originalRoot = ' '; + writeFileSync(source, [ + '', + '', + ].join('\n')); + const result = scaffoldVueComponentSession({ + id: 'vue12345', + count: 1, + sourceFile: 'app/pages/index.vue', + sourceStartLine: 3, + sourceEndLine: 3, + originalLines: [originalRoot], + cwd: tmp, + }); + // A restyle variant that keeps only class: every behavior attribute must survive Accept. + writeFileSync(join(tmp, result.componentDir, 'v1.vue'), [ + '', + '', + '', + ].join('\n')); + + assert.equal(inlineVueComponentAccept(result.manifest, 1, tmp).handled, true); + const next = readFileSync(source, 'utf-8'); + assert.match(next, /@click="submit"/, 'v-on shorthand must not degrade to a literal click attribute'); + assert.doesNotMatch(next, /\sclick="submit"/, 'sigil-stripped event handler leaked into source'); + assert.match(next, /:aria-label="label"/); + assert.match(next, /v-bind:title="tip"/); + assert.match(next, /\bdisabled\b/, 'valueless boolean attr dropped'); + assert.match(next, /\bv-cloak\b/, 'valueless directive dropped'); + assert.match(next, /class="cta cta--bold"|class="cta--bold cta"/); + }); + + it('does not duplicate an attribute the variant wrote in the other shorthand form', () => { + const originalRoot = ' '; + writeFileSync(source, ['', ''].join('\n')); + const result = scaffoldVueComponentSession({ + id: 'vue12345', + count: 1, + sourceFile: 'app/pages/index.vue', + sourceStartLine: 3, + sourceEndLine: 3, + originalLines: [originalRoot], + cwd: tmp, + }); + writeFileSync(join(tmp, result.componentDir, 'v1.vue'), [ + '', + '', + ].join('\n')); + + assert.equal(inlineVueComponentAccept(result.manifest, 1, tmp).handled, true); + const next = readFileSync(source, 'utf-8'); + assert.doesNotMatch(next, /:aria-label="label"[^>]*v-bind:aria-label|v-bind:aria-label="label"[^>]*:aria-label/, + 'shorthand and longhand of one attr both emitted, which is a Vue compile error'); + }); + it('removes deferred SFCs, the shared runtime, and the generated root on Live shutdown', () => { const result = scaffoldVueComponentSession({ id: 'vue12345', @@ -194,6 +260,10 @@ describe('Nuxt Vue component preview', () => { assert.equal(JSON.parse(readFileSync(join(tmp, result.manifestFile), 'utf-8')).arrivedVariants, 1); const late = prepareGenerationArtifact({ id: 'vue12345', sourceFile: result.manifestFile, cwd: tmp }); + const lateManifest = JSON.parse(readFileSync(join(tmp, late.artifactFile), 'utf-8')); + lateManifest.arrivedVariants = 2; + writeFileSync(join(tmp, late.artifactFile), JSON.stringify(lateManifest, null, 2) + '\n'); + writeFileSync(join(tmp, late.componentDir, 'v2.vue'), '\n'); store.appendEvent({ type: 'accept', id: 'vue12345', variantId: '1' }); const rejected = publishGenerationArtifact({ id: 'vue12345', @@ -208,5 +278,13 @@ describe('Nuxt Vue component preview', () => { assert.equal(rejected.ok, false); assert.equal(rejected.error, 'stale_generation_epoch'); assert.equal(readFileSync(source, 'utf-8'), routeBefore); + assert.equal(JSON.parse(readFileSync(join(tmp, result.manifestFile), 'utf-8')).arrivedVariants, 1, + 'the manifest must still advertise only the variant published before Accept'); + // A rejected publish must not have touched the session dir at all: v2 still + // holds its untouched scaffold stub rather than the late variant's markup. + const v2AfterReject = readFileSync(join(tmp, result.componentDir, 'v2.vue'), 'utf-8'); + assert.doesNotMatch(v2AfterReject, /Second/, + 'a rejected late publish must not write variant files into the session dir'); + assert.match(v2AfterReject, /Variant 2: add scoped CSS here/, 'v2 must still be the scaffold stub'); }); });