mirror of
https://github.com/pbakaus/impeccable.git
synced 2026-09-12 14:16:28 +03:00
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>
110 lines
4.6 KiB
JavaScript
110 lines
4.6 KiB
JavaScript
/**
|
||
* 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), '<div>original</div>\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');
|
||
});
|
||
});
|