fix(story-lifecycle): confine ledger I/O, race-free stale locks, enforce transitions

Resolve the Greptile findings on 74262955:
- Refuse symlinked .stories/, epics/, sprints/, .txn/, .ids/ and
  symlinked ledger files, so no read or write leaves .stories/.
- Recovery accepts only regular backup-<n> files in .txn/, validates the
  whole journal before restoring, and never writes through a planted
  restore.tmp.
- Remove a stale lock only under .lock.break after a re-read, so a live
  lock is never deleted; a leftover .lock.break times out (exit 3)
  instead of spinning.
- apply enforces one-step-forward status transitions, new stories start
  as todo, and Sprint must be unassigned or an existing sprint file.
- Refuse a batch that names the same file twice (e.g. a\b vs a/b).

Symlink tests skip on Windows when symlink creation returns EPERM.
This commit is contained in:
t0kubetsu
2026-10-07 11:45:30 +02:00
parent 7426295579
commit 7f797d65c4
4 changed files with 253 additions and 18 deletions
File diff suppressed because one or more lines are too long
+12 -1
View File
@@ -65,7 +65,7 @@ updating `.stories/` for that initiative.
sprints/
sprint-<n>.md # one file per sprint
<story-id>.md # individual story files at root
.gitignore # ignores .lock, .txn/, .ids/ (written by `init`)
.gitignore # ignores .lock, .lock.break, .txn/, .ids/ (written by `init`)
```
## Naming and path contract
@@ -127,6 +127,17 @@ Guarantees the helper enforces:
backed-up file and deletes every file the batch created. Until then, `status` reports
`recoveryPending: true`, and the next write command restores a consistent ledger
before it does anything else.
- **Stale locks.** A lock whose process is dead (same host) is removed only under a
second exclusive lock, `.stories/.lock.break`, after a re-read, so a live lock is never
deleted. If a process dies while holding `.lock.break`, writers time out with code `3`;
delete `.lock.break` by hand only after checking that no ledger command is running.
- **Valid transitions and sprints.** `apply` rejects a new story whose status is not
`todo`, any status change other than one step forward, and a `Sprint` value that is not
`unassigned` or an existing `sprints/sprint-<n>.md`.
- **Confinement.** The helper refuses to run if `.stories/` or its `epics/`, `sprints/`,
`.txn/`, or `.ids/` directory is a symlink, and refuses to read or write any epic,
sprint, or story file that is a symlink. Recovery accepts only regular `backup-<n>`
files inside `.stories/.txn/`. A batch that names the same file twice is refused.
## Commands
+84 -16
View File
@@ -13,6 +13,8 @@ const STORY_FILE = /^([a-z0-9]+(?:-[a-z0-9]+)*)-([1-9][0-9]*)\.md$/;
const EPIC_FILE = /^epics\/([a-z0-9]+(?:-[a-z0-9]+)*)\.md$/;
const SPRINT_FILE = /^sprints\/sprint-([1-9][0-9]*)\.md$/;
const STATUSES = ['todo', 'in-progress', 'review', 'done'];
const SPRINT_VALUE = /^(unassigned|[1-9][0-9]*)$/;
const BACKUP = /^backup-[0-9]+$/;
const DERIVED = '<!-- Derived from story files';
class LedgerError extends Error {
@@ -34,16 +36,28 @@ function pidAlive(pid) {
try { process.kill(pid, 0); return true; } catch (err) { return err.code === 'EPERM'; }
}
// Confine a ledger-relative path to the allowed file shapes under .stories/.
// Confine a ledger-relative path to the allowed file shapes under .stories/; refuse symlinks.
function resolveLedgerPath(dir, rel) {
const norm = String(rel).replace(/\\/g, '/');
const valid = EPIC_FILE.test(norm) || SPRINT_FILE.test(norm) || STORY_FILE.test(norm);
const abs = path.resolve(dir, norm);
const back = path.relative(dir, abs);
if (!valid || back.startsWith('..') || path.isAbsolute(back)) throw new LedgerError(`invalid ledger path: ${rel}`);
let isLink = false;
try { isLink = fs.lstatSync(abs).isSymbolicLink(); } catch (err) { if (err.code !== 'ENOENT') throw err; }
if (isLink) throw new LedgerError(`refusing symlinked ledger file: ${rel}`);
return abs;
}
// A symlinked ledger directory would redirect reads or writes outside .stories/.
function assertNoSymlinkDirs(dir) {
for (const p of [dir, ...['epics', 'sprints', '.txn', '.ids'].map((sub) => path.join(dir, sub))]) {
let st;
try { st = fs.lstatSync(p); } catch (err) { if (err.code === 'ENOENT') continue; throw err; }
if (st.isSymbolicLink()) throw new LedgerError(`refusing symlinked ledger directory: ${p}`);
}
}
// Allocation markers live in the git common dir so every worktree of a clone shares them.
function registryDir(root, dir) {
try {
@@ -61,6 +75,26 @@ function readLock(dir) {
function isStale(holder) { return holder.host === os.hostname() && !pidAlive(holder.pid); }
// Remove a stale lock only while holding .lock.break, after re-reading it, so a
// waiter that saw the old stale lock can never delete a newer live one.
// Returns false if another waiter (or a crashed one) holds .lock.break.
function breakStaleLock(dir) {
const brk = path.join(dir, '.lock.break');
try {
fs.writeFileSync(brk, String(process.pid), { flag: 'wx' });
} catch (err) {
if (err.code === 'EEXIST') return false;
throw err;
}
try {
const holder = readLock(dir);
if (holder && isStale(holder)) fs.rmSync(path.join(dir, '.lock'), { force: true });
return true;
} finally {
fs.rmSync(brk, { force: true });
}
}
function acquireLock(dir, waitMs) {
const lock = path.join(dir, '.lock');
const deadline = Date.now() + waitMs;
@@ -72,13 +106,10 @@ function acquireLock(dir, waitMs) {
if (err.code !== 'EEXIST') throw err;
}
const holder = readLock(dir); // null: lock vanished or half-written; retry
// ponytail: read-then-unlink of a stale lock has a tiny TOCTOU window; only matters if two recoverers race after a crash.
if (holder && isStale(holder)) {
fs.rmSync(lock, { force: true });
continue;
}
if (holder && isStale(holder) && breakStaleLock(dir)) continue;
if (Date.now() > deadline) {
throw new LedgerError(`ledger locked by pid ${holder && holder.pid} since ${holder && holder.at}`, 3);
const brk = fs.existsSync(path.join(dir, '.lock.break')) ? '; .lock.break is held (delete it only if no ledger command is running)' : '';
throw new LedgerError(`ledger locked by pid ${holder && holder.pid} since ${holder && holder.at}${brk}`, 3);
}
sleep(25 + Math.floor(Math.random() * 50));
}
@@ -90,10 +121,24 @@ function recover(dir) {
const journalPath = path.join(txn, 'journal.json');
let rolledBack = 0;
if (fs.existsSync(journalPath)) {
for (const entry of JSON.parse(fs.readFileSync(journalPath, 'utf8')).entries) {
const { entries } = JSON.parse(fs.readFileSync(journalPath, 'utf8'));
for (const entry of entries) {
resolveLedgerPath(dir, entry.path);
if (entry.backup && !(BACKUP.test(entry.backup) && fs.lstatSync(path.join(txn, entry.backup)).isFile())) {
throw new LedgerError(`invalid journal backup: ${entry.backup}`);
}
}
for (const entry of entries) {
const target = resolveLedgerPath(dir, entry.path);
if (entry.backup) fs.copyFileSync(path.join(txn, entry.backup), target);
else fs.rmSync(target, { force: true });
if (entry.backup) {
// Fresh temp file, then rename: neither step writes through a planted symlink.
const tmp = path.join(txn, 'restore.tmp');
fs.rmSync(tmp, { force: true });
fs.copyFileSync(path.join(txn, entry.backup), tmp, fs.constants.COPYFILE_EXCL);
fs.renameSync(tmp, target);
} else {
fs.rmSync(target, { force: true });
}
rolledBack++;
}
}
@@ -115,6 +160,7 @@ function parseStory(rel, md) {
};
if (field(md, 'ID') !== id || field(md, 'Epic') !== epic) throw new LedgerError(`${rel}: ID/Epic fields must match file name`);
if (!story.title || !STATUSES.includes(story.status)) throw new LedgerError(`${rel}: missing title or invalid status`);
if (!SPRINT_VALUE.test(story.sprint || '')) throw new LedgerError(`${rel}: Sprint must be unassigned or a sprint number`);
return story;
}
@@ -129,7 +175,7 @@ function replaceTable(md, rel, header, rows) {
// Read the ledger with pending changes overlaid; return regenerated epic/sprint files that differ.
function project(dir, pending = {}) {
const read = (rel) => (rel in pending ? pending[rel] : fs.readFileSync(path.join(dir, rel), 'utf8'));
const read = (rel) => (rel in pending ? pending[rel] : fs.readFileSync(resolveLedgerPath(dir, rel), 'utf8'));
const list = (sub, re) => {
const abs = path.join(dir, sub);
const onDisk = fs.existsSync(abs) ? fs.readdirSync(abs).map((f) => (sub ? `${sub}/${f}` : f)) : [];
@@ -137,6 +183,12 @@ function project(dir, pending = {}) {
};
const stories = list('', STORY_FILE).map((rel) => parseStory(rel, read(rel)))
.sort((a, b) => a.epic.localeCompare(b.epic) || a.n - b.n);
const sprints = list('sprints', SPRINT_FILE);
for (const s of stories) {
if (s.sprint !== 'unassigned' && !sprints.includes(`sprints/sprint-${s.sprint}.md`)) {
throw new LedgerError(`${s.id}.md: sprint ${s.sprint} does not exist`);
}
}
const derived = {};
for (const rel of list('epics', EPIC_FILE)) {
const slug = rel.match(EPIC_FILE)[1];
@@ -144,7 +196,7 @@ function project(dir, pending = {}) {
const next = replaceTable(read(rel), rel, '| ID | Title | Status |\n| --- | --- | --- |', rows);
if (next !== read(rel)) derived[rel] = next;
}
for (const rel of list('sprints', SPRINT_FILE)) {
for (const rel of sprints) {
const inSprint = stories.filter((s) => s.sprint === rel.match(SPRINT_FILE)[1]);
const rows = inSprint.map((s) => `| ${s.id} | ${s.title} | ${s.points} | ${s.status} |`);
const total = inSprint.reduce((sum, s) => sum + (Number(s.points) || 0), 0);
@@ -158,7 +210,7 @@ function project(dir, pending = {}) {
function init(dir) {
for (const sub of ['epics', 'sprints']) fs.mkdirSync(path.join(dir, sub), { recursive: true });
const ignore = path.join(dir, '.gitignore');
if (!fs.existsSync(ignore)) fs.writeFileSync(ignore, '.lock\n.txn/\n.ids/\n');
if (!fs.existsSync(ignore)) fs.writeFileSync(ignore, '.lock\n.lock.break\n.txn/\n.ids/\n');
return { ok: true, dir };
}
@@ -186,16 +238,31 @@ function allocate(root, dir, epic, count) {
// changes: { create: { rel: content }, update: { rel: content } }
function apply(root, dir, changes) {
const create = changes.create || {};
const update = changes.update || {};
const keys = new Set();
const normalize = (obj = {}) => Object.fromEntries(Object.entries(obj).map(([rel, content]) => {
const key = rel.replace(/\\/g, '/');
if (keys.has(key)) throw new LedgerError(`duplicate path in batch: ${key}`);
keys.add(key);
return [key, content];
}));
const create = normalize(changes.create);
const update = normalize(changes.update);
const reg = registryDir(root, dir);
for (const rel of Object.keys(create)) {
if (fs.existsSync(resolveLedgerPath(dir, rel))) throw new LedgerError(`refusing to overwrite existing ${rel}`, 4);
const m = rel.match(STORY_FILE);
if (m && !fs.existsSync(path.join(reg, `${m[1]}-${m[2]}`))) throw new LedgerError(`${rel}: story ID was not allocated`);
if (m && parseStory(rel, create[rel]).status !== 'todo') throw new LedgerError(`${rel}: new stories start as todo`);
}
for (const rel of Object.keys(update)) {
if (!fs.existsSync(resolveLedgerPath(dir, rel))) throw new LedgerError(`cannot update missing ${rel}`);
const abs = resolveLedgerPath(dir, rel);
if (!fs.existsSync(abs)) throw new LedgerError(`cannot update missing ${rel}`);
if (!STORY_FILE.test(rel)) continue;
const from = parseStory(rel, fs.readFileSync(abs, 'utf8')).status;
const to = parseStory(rel, update[rel]).status;
if (to !== from && STATUSES.indexOf(to) !== STATUSES.indexOf(from) + 1) {
throw new LedgerError(`${rel}: status ${from} -> ${to} is not allowed; status moves forward one step at a time`);
}
}
const pending = { ...create, ...update };
const all = { ...pending, ...project(dir, pending).derived };
@@ -224,6 +291,7 @@ function main() {
const cmd = opts._[0];
const root = path.resolve(opts.root || '.');
const dir = path.join(root, '.stories');
assertNoSymlinkDirs(dir);
if (cmd === 'init') return init(dir);
if (!fs.existsSync(path.join(dir, 'epics'))) throw new LedgerError('no .stories/ ledger; run `init` first');
if (cmd === 'status') {
+156
View File
@@ -58,6 +58,17 @@ function createStories(root, titles, tag = 'batch') {
return { ids, input: writeJson(root, `${tag}.json`, { create }) };
}
// Windows allows symlinks only with admin rights or Developer Mode; skip rather than fail there.
class Skip extends Error {}
function symlink(target, file, type) {
try {
fs.symlinkSync(target, file, type);
} catch (err) {
if (err.code === 'EPERM' && process.platform === 'win32') throw new Skip('symlinks not permitted');
throw err;
}
}
const read = (root, rel) => fs.readFileSync(path.join(root, '.stories', rel), 'utf8');
const storyFiles = (root) => fs.readdirSync(path.join(root, '.stories')).filter((f) => /-\d+\.md$/.test(f)).sort();
@@ -195,6 +206,147 @@ const tests = [
assert.strictEqual(new Set(ids).size, 160, 'duplicate story IDs allocated across worktrees');
}],
['symlinked ledger directories are refused before any write', () => {
const root = newRepo();
const outside = fs.mkdtempSync(path.join(os.tmpdir(), 'story-outside-'));
fs.rmSync(path.join(root, '.stories', 'sprints'), { recursive: true });
symlink(outside, path.join(root, '.stories', 'sprints'), 'dir');
const input = writeJson(root, 's.json', { create: { 'sprints/sprint-1.md': '# Sprint 1\n' } });
const r = run(root, ['apply', '--input', input]);
assert.strictEqual(r.code, 2);
assert.match(r.err, /symlinked ledger directory/);
assert.deepStrictEqual(fs.readdirSync(outside), []);
}],
['recover rejects journal backups outside .txn/', () => {
const root = newRepo();
const txn = path.join(root, '.stories', '.txn');
fs.writeFileSync(path.join(root, 'private.txt'), 'SECRET');
fs.mkdirSync(txn);
fs.writeFileSync(path.join(txn, 'journal.json'), JSON.stringify({ entries: [{ path: 'epics/auth-flow.md', backup: '../../private.txt' }] }));
assert.strictEqual(run(root, ['recover']).code, 2);
assert.ok(!read(root, 'epics/auth-flow.md').includes('SECRET'));
}],
['recover never writes through a symlinked ledger target', () => {
const root = newRepo();
const txn = path.join(root, '.stories', '.txn');
const victim = path.join(root, 'victim.txt');
fs.writeFileSync(victim, 'ORIGINAL');
symlink(victim, path.join(root, '.stories', 'auth-flow-1.md'));
fs.mkdirSync(txn);
fs.writeFileSync(path.join(txn, 'backup-0'), 'PLANTED');
fs.writeFileSync(path.join(txn, 'journal.json'), JSON.stringify({ entries: [{ path: 'auth-flow-1.md', backup: 'backup-0' }] }));
assert.strictEqual(run(root, ['recover']).code, 2);
assert.strictEqual(fs.readFileSync(victim, 'utf8'), 'ORIGINAL');
}],
['symlinked ledger files are never read into or written through the ledger', () => {
const root = newRepo();
const { ids, input } = createStories(root, ['Login', 'Logout']);
run(root, ['apply', '--input', input]);
const outside = path.join(root, 'other-repo-story.md');
fs.writeFileSync(outside, story(ids[1], 'PRIVATE-TITLE'));
fs.rmSync(path.join(root, '.stories', `${ids[1]}.md`));
symlink(outside, path.join(root, '.stories', `${ids[1]}.md`));
const status = run(root, ['status']);
assert.strictEqual(status.code, 2);
assert.match(status.err, /symlinked ledger file/);
assert.strictEqual(run(root, ['reconcile']).code, 2);
const update = writeJson(root, 'u.json', { update: { [`${ids[1]}.md`]: story(ids[1], 'Logout', { status: 'in-progress' }) } });
assert.strictEqual(run(root, ['apply', '--input', update]).code, 2);
assert.ok(!read(root, 'epics/auth-flow.md').includes('PRIVATE-TITLE'));
assert.strictEqual(fs.readFileSync(outside, 'utf8'), story(ids[1], 'PRIVATE-TITLE'));
}],
['recover never follows symlinks planted in .txn/', () => {
const root = newRepo();
const txn = path.join(root, '.stories', '.txn');
const outside = path.join(root, 'outside.txt');
fs.writeFileSync(outside, 'ORIGINAL');
fs.mkdirSync(txn);
fs.writeFileSync(path.join(txn, 'journal.json'), JSON.stringify({ entries: [{ path: 'auth-flow-1.md', backup: 'backup-0' }] }));
symlink(outside, path.join(txn, 'backup-0'));
assert.strictEqual(run(root, ['recover']).code, 2, 'symlinked backup must be refused');
assert.ok(!fs.existsSync(path.join(root, '.stories', 'auth-flow-1.md')));
fs.rmSync(path.join(txn, 'backup-0'));
fs.writeFileSync(path.join(txn, 'backup-0'), 'GOOD');
symlink(outside, path.join(txn, 'restore.tmp'));
assert.strictEqual(run(root, ['recover']).code, 0);
assert.strictEqual(fs.readFileSync(outside, 'utf8'), 'ORIGINAL');
assert.ok(fs.lstatSync(path.join(root, '.stories', 'auth-flow-1.md')).isFile());
}],
['a leftover .lock.break makes writers time out instead of spinning', () => {
const root = newRepo();
fs.writeFileSync(path.join(root, '.stories', '.lock'), JSON.stringify({ pid: 2 ** 22 + 1, host: os.hostname(), at: 'stale' }));
fs.writeFileSync(path.join(root, '.stories', '.lock.break'), '1');
const r = spawnSync(process.execPath, [LEDGER, 'reconcile', '--root', root, '--wait-ms', '200'], { encoding: 'utf8', timeout: 5000 });
assert.strictEqual(r.status, 3, `expected lock timeout, got ${r.status} ${r.signal}`);
assert.match(r.stderr, /\.lock\.break is held/);
}],
['symlinked .ids registry is refused outside git', () => {
const root = newRepo();
fs.rmSync(path.join(root, '.git'), { recursive: true });
const outside = fs.mkdtempSync(path.join(os.tmpdir(), 'story-ids-'));
symlink(outside, path.join(root, '.stories', '.ids'), 'dir');
assert.strictEqual(run(root, ['allocate', '--epic', 'auth-flow']).code, 2);
assert.deepStrictEqual(fs.readdirSync(outside), []);
}],
['a batch naming the same file twice is refused', () => {
const root = newRepo();
const input = writeJson(root, 'dup.json', { create: { 'sprints\\sprint-1.md': '# A\n', 'sprints/sprint-1.md': '# B\n' } });
const r = run(root, ['apply', '--input', input]);
assert.strictEqual(r.code, 2);
assert.match(r.err, /duplicate path/);
assert.ok(!fs.existsSync(path.join(root, '.stories', 'sprints', 'sprint-1.md')));
}],
['a waiter that saw a stale lock never deletes the live lock that replaced it', () => {
const root = newRepo();
const lock = path.join(root, '.stories', '.lock');
fs.writeFileSync(lock, JSON.stringify({ pid: 2 ** 22 + 1, host: os.hostname(), at: 'stale' }));
// Preload: right after the waiter reads the stale lock, another writer wins and holds a live lock.
const preload = path.join(root, 'race.js');
fs.writeFileSync(preload, `const fs = require('fs'); const orig = fs.readFileSync; let raced = false;
fs.readFileSync = function (p, ...a) { const r = orig.call(fs, p, ...a);
if (!raced && String(p).endsWith('.lock')) { raced = true; fs.unlinkSync(p);
fs.writeFileSync(p, JSON.stringify({ pid: ${process.pid}, host: require('os').hostname(), at: 'live' }), { flag: 'wx' }); }
return r; };`);
const r = spawnSync(process.execPath, ['-r', preload, LEDGER, 'allocate', '--epic', 'auth-flow', '--root', root, '--wait-ms', '200'], { encoding: 'utf8' });
assert.strictEqual(r.status, 3, `expected lock timeout, got ${r.status}: ${r.stderr}`);
assert.ok(fs.readFileSync(lock, 'utf8').includes('live'), 'live lock was deleted');
}],
['story status moves forward one step at a time and new stories start as todo', () => {
const root = newRepo();
const [id] = run(root, ['allocate', '--epic', 'auth-flow']).out.ids;
const put = (kind, status) => run(root, ['apply', '--input', writeJson(root, 't.json', { [kind]: { [`${id}.md`]: story(id, 'Login', { status }) } })]).code;
assert.strictEqual(put('create', 'done'), 2);
assert.strictEqual(put('create', 'todo'), 0);
assert.strictEqual(put('update', 'done'), 2, 'todo -> done skips steps');
for (const status of ['in-progress', 'in-progress', 'review', 'done']) assert.strictEqual(put('update', status), 0, status);
assert.strictEqual(put('update', 'todo'), 2, 'done -> todo moves backward');
assert.ok(read(root, 'epics/auth-flow.md').includes(`| ${id} | Login | done |`));
}],
['stories can only be assigned to an existing sprint', () => {
const root = newRepo();
const { ids, input } = createStories(root, ['Login']);
run(root, ['apply', '--input', input]);
for (const sprint of ['2', 'bogus']) {
const plan = writeJson(root, 'p.json', { update: { [`${ids[0]}.md`]: story(ids[0], 'Login', { sprint }) } });
assert.strictEqual(run(root, ['apply', '--input', plan]).code, 2, sprint);
}
fs.writeFileSync(path.join(root, '.stories', `${ids[0]}.md`), story(ids[0], 'Login', { sprint: '7' }));
const status = run(root, ['status']);
assert.strictEqual(status.code, 2);
assert.match(status.err, /sprint 7 does not exist/);
}],
['ledger paths are confined to .stories/ file shapes', () => {
const dir = path.join(os.tmpdir(), '.stories');
for (const bad of ['../x-1.md', 'epics/../../x.md', 'Auth-1.md', 'a/b-1.md', 'epics/a.txt', 'sprints/sprint-0.md', '/abs-1.md']) {
@@ -214,6 +366,10 @@ const tests = [
console.log(` ✓ ${name}`);
passed++;
} catch (error) {
if (error instanceof Skip) {
console.log(` - ${name} (skipped: ${error.message})`);
continue;
}
console.log(` ✗ ${name}`);
console.log(` Error: ${error.message}`);
failed++;