From 529184bbe41ddc0b90bbf589b178ec27ada04e18 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Fri, 17 Jul 2026 15:58:27 -0700 Subject: [PATCH] Fix inset-order detection, the unlocked artifact discard, and stray boolean flags Three of the four open review findings. The fourth is declined below. - The inset-stripe scan only matched layers starting with `inset`, but the keyword is order-independent: `box-shadow: 4px 0 0 var(--brand-accent) inset` paints the same stripe and was silently missed. Strip the keyword wherever it sits, but only as a standalone token, so a color like var(--inset-accent) is not mangled into `var(-- -accent)` and quietly reclassified as neutral. The fixture now covers both orders plus that token, and a trailing-inset neutral still passes. - The source-artifact discard deleted the preview without the source lock, unlike every other discard path. Take the lock. Narrower than reported, though: the server journals `discard_requested` as a fenced phase before live-accept runs and the publisher checks it three times, so a publish could never land on a discarded session. What this actually prevents is deleting the artifact under a publisher mid-critical-section, turning a clean stale_generation_epoch into an ENOENT crash. - benchmark-live-providers.mjs still compared `--headed` and `--skip-cleanup-control` against a boolean sentinel, so the `=true` spelling silently did nothing. My gap: I introduced boolFlag and converted benchmark-live.mjs but not this one. skipCleanupControl is now read once rather than twice, so the two call sites cannot drift. Declined: tightening the selector guard that skips `active` / `current` / `selected` tokens. It does cause false negatives on names like `.selected-feature`, but the rule's contract makes selection and focus indicators its one exception, and `.active-tab` / `.current-step` / `.selected-row` are syntactically identical to `.selected-feature`. No regex separates them, so tightening the guard trades missed stripes for false positives on exactly the case the rule exempts. The conservative skip is the intended behavior. Prepared with AI assistance under maintainer direction. Co-Authored-By: Claude --- cli/engine/engines/regex/detect-text.mjs | 12 ++++++-- scripts/benchmark-live-providers.mjs | 9 +++--- skill/scripts/live-accept.mjs | 22 ++++++++++++-- tests/detect-antipatterns-fixtures.test.mjs | 7 +++++ .../astro-inset-shadow-stripe.astro | 11 +++++++ tests/live-accept.test.mjs | 30 ++++++++++++++++++- 6 files changed, 82 insertions(+), 9 deletions(-) diff --git a/cli/engine/engines/regex/detect-text.mjs b/cli/engine/engines/regex/detect-text.mjs index af37aa17e..ff4db4228 100644 --- a/cli/engine/engines/regex/detect-text.mjs +++ b/cli/engine/engines/regex/detect-text.mjs @@ -416,8 +416,16 @@ function scanInsetStripeCss(rawContent, filePath, lineOffset = 0) { const declaration = match[2].match(/(?:^|;)\s*box-shadow\s*:\s*([^;]+)/i); if (!declaration || !/\binset\b/i.test(declaration[1])) continue; - for (const layer of declaration[1].split(/,(?![^(]*\))/)) { - const shadow = layer.trim().match(/\binset\s+(-?\d*\.?\d+)(px)?\s+(-?\d*\.?\d+)(px)?\s+(-?\d*\.?\d+)(px)?(?:\s+(-?\d*\.?\d+)(px)?)?\s+(.+)$/i); + for (const rawLayer of declaration[1].split(/,(?![^(]*\))/)) { + const layer = rawLayer.trim(); + // `inset` is order-independent inside a box-shadow layer: `inset 4px 0 0 red` + // and `4px 0 0 red inset` paint the same stripe, and requiring it first + // silently missed the second spelling. Strip it only as a standalone + // keyword, so a color token such as var(--inset-accent) survives intact; + // an unchanged layer had no inset keyword and is not our shape. + const body = layer.replace(/(^|\s)inset(?=\s|$)/i, '$1').trim(); + if (body === layer) continue; + const shadow = body.match(/^(-?\d*\.?\d+)(px)?\s+(-?\d*\.?\d+)(px)?\s+(-?\d*\.?\d+)(px)?(?:\s+(-?\d*\.?\d+)(px)?)?\s+(.+)$/i); if (!shadow) continue; const x = Number(shadow[1]); const y = Number(shadow[3]); diff --git a/scripts/benchmark-live-providers.mjs b/scripts/benchmark-live-providers.mjs index 4b99e8956..7d6810019 100644 --- a/scripts/benchmark-live-providers.mjs +++ b/scripts/benchmark-live-providers.mjs @@ -9,7 +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 { boolFlag, parseArgs, positiveIntFlag } from './lib/cli-args.mjs'; import { clickAccept, clickGo, @@ -72,13 +72,14 @@ if (args.requireAll && available.length !== selection.length) { } if (available.length === 0) throw new Error('no provider API keys found; use --dry-run to validate without network calls'); -const needsBrowser = args.pipeline === 'e2e' || args.skipCleanupControl !== true; +const skipCleanupControl = boolFlag(args.skipCleanupControl); +const needsBrowser = args.pipeline === 'e2e' || !skipCleanupControl; const { chromium } = needsBrowser ? await import('playwright') : { chromium: null }; -const browser = chromium ? await chromium.launch({ headless: args.headed !== true }) : null; +const browser = chromium ? await chromium.launch({ headless: !boolFlag(args.headed) }) : null; const results = []; let cleanupControl = { passed: true, skipped: true }; try { - if (args.skipCleanupControl !== true) { + if (!skipCleanupControl) { process.stderr.write('[live-provider-bench] running provider-independent Accept/cleanup control\n'); cleanupControl = await runCleanupControl({ browser, fixture }); } diff --git a/skill/scripts/live-accept.mjs b/skill/scripts/live-accept.mjs index 8bd2e7507..08883c90d 100644 --- a/skill/scripts/live-accept.mjs +++ b/skill/scripts/live-accept.mjs @@ -137,9 +137,27 @@ Output (JSON): if (sourceArtifactManifest) { if (isDiscard) { - removeSourceArtifactSession(id, process.cwd()); + // Take the source lock like every other discard path. The journalled + // discard already fences publication, so this cannot admit a write to a + // discarded session; what it prevents is deleting the preview out from + // under a publisher mid-critical-section, which turns its clean + // stale_generation_epoch into an ENOENT crash. + let result; + try { + result = withSourceLockSync( + sourceArtifactManifest.sourcePath, + 'discard:' + id, + () => { + removeSourceArtifactSession(id, process.cwd()); + return { handled: true }; + }, + { waitMs: ACCEPT_LOCK_WAIT_MS }, + ); + } catch (err) { + result = { handled: false, error: err.message }; + } emitResult({ - handled: true, + ...result, file: sourceArtifactManifest.sourceFile, sourceFile: sourceArtifactManifest.sourceFile, previewMode: sourceArtifactManifest.previewMode, diff --git a/tests/detect-antipatterns-fixtures.test.mjs b/tests/detect-antipatterns-fixtures.test.mjs index 7b0902097..a6d9f9892 100644 --- a/tests/detect-antipatterns-fixtures.test.mjs +++ b/tests/detect-antipatterns-fixtures.test.mjs @@ -28,6 +28,11 @@ describe('detectText - Astro structural CSS fixtures', () => { 'Named Red Edge', 'Chromatic Rgb Edge', 'Chromatic Oklch Edge', + // `inset` may follow the offsets/color. Requiring it first missed the same + // stripe written the other legal way. + 'Trailing Inset Edge', + 'Trailing Inset Token Edge', + 'Inset Named Token Edge', ]; const SHOULD_PASS = [ 'Neutral Shadow Token', @@ -49,6 +54,8 @@ describe('detectText - Astro structural CSS fixtures', () => { 'Shorthand Neutral Hex Edge', // Commented-out CSS is not a live rule. 'Commented Out Edge', + // Trailing `inset` still respects the neutral-color exemption. + 'Trailing Inset Neutral 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 023855c3e..a93c6e396 100644 --- a/tests/fixtures/antipatterns/astro-inset-shadow-stripe.astro +++ b/tests/fixtures/antipatterns/astro-inset-shadow-stripe.astro @@ -14,6 +14,9 @@ const title = 'Astro inset shadow stripe regression';

Named Red Edge

Chromatic Rgb Edge

Chromatic Oklch Edge

+

Trailing Inset Edge

+

Trailing Inset Token Edge

+

Inset Named Token Edge

Should pass

@@ -31,6 +34,7 @@ const title = 'Astro inset shadow stripe regression';

Black Rgb Edge

Shorthand Neutral Hex Edge

Commented Out Edge

+

Trailing Inset Neutral Edge

@@ -60,6 +64,13 @@ const title = 'Astro inset shadow stripe regression'; [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; } + /* `inset` is order-independent per spec; these paint the same stripe as above. */ + [data-case="Trailing Inset Edge"] { box-shadow: 4px 0 0 #6366f1 inset; } + [data-case="Trailing Inset Token Edge"] { box-shadow: 4px 0 0 var(--brand-accent) inset; } + /* The keyword must only be stripped standalone: this token merely contains it. */ + [data-case="Inset Named Token Edge"] { box-shadow: inset 4px 0 0 var(--inset-accent); } + [data-case="Trailing Inset Neutral Edge"] { box-shadow: 4px 0 0 #000 inset; } + /* 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 82df1e730..85eb7d40f 100644 --- a/tests/live-accept.test.mjs +++ b/tests/live-accept.test.mjs @@ -5,12 +5,13 @@ import { describe, it, beforeEach, afterEach } from 'node:test'; import assert from 'node:assert/strict'; -import { existsSync, mkdtempSync, writeFileSync, readFileSync, rmSync } from 'node:fs'; +import { existsSync, mkdirSync, mkdtempSync, realpathSync, writeFileSync, readFileSync, rmSync } from 'node:fs'; import { dirname, join, resolve } from 'node:path'; import { tmpdir } from 'node:os'; import { fileURLToPath } from 'node:url'; import { execFileSync, spawnSync } from 'node:child_process'; import { scaffoldSourceArtifactSession } from '../skill/scripts/live/source-artifact.mjs'; +import { sourceLockPath } from '../skill/scripts/live/source-lock.mjs'; const __dirname = dirname(fileURLToPath(import.meta.url)); const ACCEPT = resolve(__dirname, '..', 'skill/scripts/live-accept.mjs'); @@ -131,6 +132,33 @@ describe('live-accept — isolated source artifacts', () => { assert.equal(readFileSync(join(tmp, 'page.html'), 'utf-8'), original); assert.equal(existsSync(join(tmp, session.sessionDir)), false); }); + + // Every other discard path (Vue, Svelte, plain wrapper) takes the source lock. + // This one deleted the preview bare, so it could pull the artifact out from + // under an in-flight publisher instead of serializing behind it. + it('serializes the discard behind a publisher holding the source lock', () => { + const { original, session } = scaffold('isolatedlocked'); + // realpath: on macOS mkdtemp hands back /var/... while the child process's + // cwd resolves to /private/var/..., and the lock digest hashes the absolute + // path. Hash the same string the child will. + const realTmp = realpathSync(tmp); + const lockPath = sourceLockPath(join(realTmp, 'page.html'), realTmp); + mkdirSync(dirname(lockPath), { recursive: true }); + // A live holder: process.pid is alive, so the lock is not stale. + writeFileSync(lockPath, JSON.stringify({ + owner: 'generation:isolatedlocked:1', token: 'other', pid: process.pid, at: Date.now(), + }) + '\n'); + + const result = runAccept(tmp, ['--id', 'isolatedlocked', '--discard']); + assert.equal(result.handled, false, JSON.stringify(result)); + assert.equal(result.error, 'source_locked'); + assert.equal( + existsSync(join(tmp, session.sessionDir)), + true, + 'the preview must survive: deleting it under the publisher is the race', + ); + assert.equal(readFileSync(join(tmp, 'page.html'), 'utf-8'), original); + }); }); describe('live-accept — style-element edge cases', () => {