From 91589f49eb95d3da4a51d63bf1b6d7ee7a7c10df Mon Sep 17 00:00:00 2001 From: Danny Gillespie Date: Sat, 1 Aug 2026 10:30:14 +0100 Subject: [PATCH 1/5] =?UTF-8?q?fix(wal):=20the=20review=20round's=20six=20?= =?UTF-8?q?=E2=80=94=20strict=20bytes,=20honest=20doctor,=20truthful=20mir?= =?UTF-8?q?rors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex 4b.3 verification verdict was NO on six contract gaps; all six accepted and fixed red-first (falsifiers W1-W6, each seen red against main @ b029c81): - W1 parseIntent decodes fatally: invalid UTF-8 refuses, never normalizes. - W2 validateMirrorReceipt matches the receipt's tool to the intent's. - W3 mirror validity = agreement on status/severity/summary, not link cardinality; the transition mirror op carries summary again; a stale-but- unique mirror is trued by one committed repeat. - W4 doctor diagnoses the slot BEFORE the writability probe and defers the probe under a pending slot — doctor no longer recovers what it reports. - W5 MCP read paths peek (byte-pure): no .corrupt backup, no record creation on a read; unprovable records answer the allowlisted sentence. - W6 clear re-checks the slot bytes it proved before deleting (spec CLEAR). Ruled NOT defects: pre-commit .tmp- residue (inert by design, 4b.1), CLI/MCP byte-equality and the MCP retry wording (both match the ratified spec; the review claims overstated the contract). Test assertion changed, stated out loud: S20 counted state.loadState calls to pin the one-snapshot contract; score.confidence now deliberately peeks, so S20 counts peekState (still exactly 1) AND newly pins loadState === 0 on the read path. The contract it guards is unchanged and strictly tighter. npm test: one fully green run on record (11/11 suite summaries, 0 failed); individual runs still flake 1 test with the CHANGELOG-documented host EPERM signature (roaming, retryable). doctor green. CI is the arbiter. Traced by: claude-fable-5 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01AU1qyUKzzmJ3NPH2HwWiPe --- CHANGELOG.md | 38 ++++++++++++ src/artifacts.js | 18 ++++-- src/cli.js | 41 ++++++++----- src/mcp/server.js | 26 ++++++--- src/receipt.js | 18 ++++-- src/state.js | 65 +++++++++++++++++++-- src/wal.js | Bin 11169 -> 11621 bytes test/mcp-server.test.js | 12 +++- test/mcp-wal.test.js | 126 ++++++++++++++++++++++++++++++++++++++++ 9 files changed, 304 insertions(+), 40 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 72c70f3..779e3b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,44 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- **4b.3 review round (independent Codex verification, verdict "NO" on six contract + gaps — all six accepted, fixed red-first; three claim-wording findings ruled + not-defects).** All CLI-enforced, each with a falsifier (W1–W6) seen red against + `main @ b029c81` before its fix: + - **Strict slots decode strictly (W1):** `parseIntent` now uses a fatal UTF-8 decode; + a slot with invalid bytes refuses as unrecoverable instead of being lossily + normalized into an operationId nobody published — and then cleared. + - **The receipt must carry the intent's tool (W2):** `validateMirrorReceipt` compares + `receipt.tool` to `intent.tool`; a post-state slot whose tool contradicts the + receipt it names is ambiguous, never completed. + - **Mirror validity means agreement, not linkage (W3):** the spec projection carries + status, severity, AND summary. The transition mirror op now writes all three + (summary was dropped), and the exact-repeat validity check compares all three + instead of counting linked rows — a stale-but-unique mirror is now trued by one + committed repeat, then repeats no-op again. + - **Doctor observes without recovering (W4):** the WAL diagnosis now runs BEFORE the + writability probe, and under a pending slot the probe defers — previously doctor's + own `initProject` lock acquisition ran recovery, consumed the slot, and reported + "no pending intent" about the store it had just changed. + - **Resource reads are byte-pure, enforced (W5):** the MCP read paths (state/ledger + resources, the receipt resource, `score.confidence`) now use pure peeks that never + create, back up, or reinitialize a record; an unprovable record answers the one + allowlisted MirrorUnrecoverable sentence. Previously a resource read over a + malformed ledger wrote a `.corrupt` backup file — a write on a read path. + - **CLEAR re-checks identity (W6):** the slot delete (spec CLEAR step) re-reads the + slot and refuses if the bytes are not the bytes this pass proved — on both the + transaction's own clear and recovery's fenced clear. + - Ruled NOT defects, on the record: pre-commit `.tmp-` residue (documented inert by + design in 4b.1), CLI/MCP byte-equality (ids and receipts differ by ratified + design), and the MCP retry wording (the spec deliberately gives MCP the generic + `WriteFailed`; the CLI carries the "re-run" sentence). S20's loader count was + updated to the new peek — the one-snapshot contract it pins is unchanged and it + now additionally pins that reads never touch the creating loader. + + + ### Added - **Known limitation (parked, owner: Danny): a host-environment hold can stall the diff --git a/src/artifacts.js b/src/artifacts.js index 6c34694..0f56e3c 100644 --- a/src/artifacts.js +++ b/src/artifacts.js @@ -239,9 +239,10 @@ function mirrorOpFor(ledger, defect, now, mintId) { const linked = String(defect.ledgerId || ''); const matches = linked ? (ledger.defects || []).filter((x) => x && x.id === linked) : []; if (matches.length === 1) { - // The mirror follows the record on both axes the transitions own; carrying - // an unchanged value is a no-op inside the same replace. - const after = { ...matches[0], severity: defect.severity, status: defect.status || 'open', updatedAt: now }; + // The mirror follows the record on every axis the spec projection names — + // status, severity, summary; carrying an unchanged value is a no-op inside + // the same replace. + const after = { ...matches[0], severity: defect.severity, summary: defect.summary, status: defect.status || 'open', updatedAt: now }; return { collection: 'defects', id: linked, mode: 'replace', after }; } const mirror = { @@ -427,8 +428,13 @@ function prepareDefectTransition(s, ledger, id, toStatus, meta, mintId) { if (from === toStatus) { const [recorded, offered] = transitionProof(d, toStatus, meta_); const linked = String(d.ledgerId || ''); + // Valid means TRUTHFUL, not merely linked: one mirror row that agrees on + // every axis the projection names. A stale-but-unique row would otherwise + // pass as valid forever, and the no-op below would refuse to repair it. + const rows = ledger ? (ledger.defects || []).filter((x) => x && x.id === linked) : []; const mirrorValid = ledger - ? linked && (ledger.defects || []).filter((x) => x && x.id === linked).length === 1 + ? Boolean(linked) && rows.length === 1 && + rows[0].status === (d.status || 'open') && rows[0].severity === d.severity && rows[0].summary === d.summary : true; if (recorded === offered && mirrorValid) { // The exact repeat, mirror already truthful: no log line, no history, no @@ -442,8 +448,8 @@ function prepareDefectTransition(s, ledger, id, toStatus, meta, mintId) { 'replace the original. Reopen it first if the recorded proof is wrong.' ); } - // Exact repeat but the mirror is missing or ambiguous: commit once solely - // to perform the D2b admission below. + // Exact repeat but the mirror is missing, ambiguous, or stale: commit once + // solely to perform the D2b admission (or the in-place truing) below. } d.status = toStatus; diff --git a/src/cli.js b/src/cli.js index f625f63..b7e778f 100644 --- a/src/cli.js +++ b/src/cli.js @@ -804,22 +804,16 @@ function cmdDoctor(cwd, asJson) { } add('every skills/*/SKILL.md has frontmatter + description', skillProblems.length === 0, skillProblems.join('; ')); - let stateDetail = ''; - let stateOk = true; - try { - state.initProject(cwd); - stateDetail = state.projectDir(cwd); - } catch (e) { - stateOk = false; - stateDetail = e.message; - } - add('state dir writable', stateOk, stateDetail); - - // 4b WAL slot — read-only diagnosis, never a repair. A recoverable slot is - // informational (any supported write recovers it on its way in); a slot - // strict recovery cannot prove legal is the operator's, by name. + // 4b WAL slot — read-only diagnosis, never a repair, and it goes FIRST: + // initProject below acquires the workspace lock, whose choke point RUNS + // recovery — doctor observing a slot through that door would consume the + // very thing it reports. A recoverable slot is informational (any supported + // write recovers it on its way in); a slot strict recovery cannot prove + // legal is the operator's, by name. + let slotPending = false; try { const slot = state.diagnoseIntent(cwd); + slotPending = Boolean(slot.pending); if (!slot.pending) add('WAL intent slot', true, 'no pending intent'); else if (slot.verdict === 'ambiguous') { add('WAL intent slot', false, @@ -833,9 +827,28 @@ function cmdDoctor(cwd, asJson) { add('WAL intent slot', true, `pending intent, ${meaning[slot.verdict] || slot.verdict}`); } } catch (e) { + slotPending = true; // an undiagnosable slot is still a slot — do not recover it add('WAL intent slot', false, e.message); } + // The writability probe runs initProject, and the lock it takes would + // recover a pending slot on its way in — so under a pending slot the probe + // defers instead of quietly consuming the evidence doctor just reported. + let stateDetail = ''; + let stateOk = true; + if (slotPending) { + stateDetail = 'deferred — a pending WAL intent is reported above, and this probe would recover it'; + } else { + try { + state.initProject(cwd); + stateDetail = state.projectDir(cwd); + } catch (e) { + stateOk = false; + stateDetail = e.message; + } + } + add('state dir writable', stateOk, stateDetail); + let snapOk = true; let snapDetail = ''; try { diff --git a/src/mcp/server.js b/src/mcp/server.js index c1e382a..ae85c38 100644 --- a/src/mcp/server.js +++ b/src/mcp/server.js @@ -1276,8 +1276,8 @@ function createServer(options) { // let a write that landed in between make the revision describe a // record these layers never saw — a number that certifies the wrong // bytes is worse than no number. - const snapshot = state.loadState(record.root); - const ledger = state.loadLedger(record.root); + const snapshot = state.peekState(record.root); + const ledger = state.peekLedger(record.root); // readEventsWithHealth, not readEvents: the malformed-line warning goes // to stderr, and no MCP client reads stderr. Damage that only appears // there is damage the wire silently certifies as clean. @@ -1581,12 +1581,22 @@ function createServer(options) { const available = resourceRecord(params.uri); // 4b: every resource states the WAL observation out loud. The flag is // derived at read time and injected into the projection only — the disk - // bytes never gain it. Reads stay byte-pure: the sampler never repairs. - const sampled = samplePendingIntent(available.record.root, () => { - if (available.parsed.name === 'state') return state.loadState(available.record.root); - if (available.parsed.name === 'ledger') return state.loadLedger(available.record.root); - return receipt.assemble(available.record.root); - }); + // bytes never gain it. Reads stay byte-pure: peeks, not the resilient + // loaders — a resource read must never create a record or back one up. + let sampled; + try { + sampled = samplePendingIntent(available.record.root, () => { + if (available.parsed.name === 'state') return state.peekState(available.record.root); + if (available.parsed.name === 'ledger') return state.peekLedger(available.record.root); + return receipt.assemble(available.record.root, { peek: true }); + }); + } catch (e) { + if (e && e.code === 'ERATCHETMIRROR') { + // The conservative refusal: the one allowlisted sentence, no store path. + throw rpc.rpcError(-32603, WRITE_REFUSALS.MirrorUnrecoverable); + } + throw e; + } const value = Object.assign({}, sampled.value, { pendingIntent: sampled.pendingIntent }); return withCache({ diff --git a/src/receipt.js b/src/receipt.js index 908dff1..6de4d5b 100644 --- a/src/receipt.js +++ b/src/receipt.js @@ -42,13 +42,19 @@ function authorityState(git) { return { level: 'committed-local', label: 'committed-local — HEAD is on no remote branch' }; } -function assemble(cwd = process.cwd()) { - const s = state.loadState(cwd); +function assemble(cwd = process.cwd(), opts = {}) { + // MCP resource reads peek (byte-pure, refuse on unprovable bytes); the CLI + // cold read keeps its resilient loaders — it may be the first touch ever. + const s = opts.peek ? state.peekState(cwd) : state.loadState(cwd); let ledger = {}; - try { - ledger = state.loadLedger(cwd); - } catch (_e) { - ledger = {}; + if (opts.peek) { + ledger = state.peekLedger(cwd); + } else { + try { + ledger = state.loadLedger(cwd); + } catch (_e) { + ledger = {}; + } } let events = []; diff --git a/src/state.js b/src/state.js index 265d815..4551d49 100644 --- a/src/state.js +++ b/src/state.js @@ -760,7 +760,7 @@ function recoverPendingIntentLocked(cwd) { throw e; } }, - clearIntent: () => clearIntentFile(file), + clearIntent: (proven) => clearIntentFile(file, proven), validateMcpReceipt: validateMirrorReceipt, }); } finally { @@ -772,11 +772,28 @@ function recoverPendingIntentLocked(cwd) { // rename: a scanner holding intent.json for an instant must not fail an // operation whose work is already done. A persistent refusal throws and the // slot survives — recovery clears it later, which is exactly what it is for. -function clearIntentFile(file) { +// The spec's CLEAR step re-checks intent identity before the delete: the bytes +// removed must be the bytes this pass proved, or the delete would erase a slot +// some other story published. A mismatch is ambiguous and the slot survives. +function clearIntentFile(file, provenBytes) { const deadline = Date.now() + envMs('RATCHET_PUBLISH_TIMEOUT_MS', 10000); for (;;) { try { fenceForFile(file); + if (provenBytes) { + let current; + try { + current = fs.readFileSync(file); + } catch (e) { + if (e && e.code === 'ENOENT') return; // already cleared — done is done + throw e; + } + if (!current.equals(Buffer.from(provenBytes))) { + const err = new Error('WAL intent unrecoverable: the slot changed identity before its clear'); + err.code = 'ERATCHETMIRROR'; + throw err; + } + } fs.unlinkSync(file); return; } catch (e) { @@ -798,6 +815,7 @@ function validateMirrorReceipt(stateObj, intent) { if (hits.length !== 1) return false; const hit = hits[0]; return ( + hit.tool === intent.tool && hit.argsHash === intent.argsHash && String(hit.gen || '') === intent.stateGen && hit.rev === intent.targetStateRev && @@ -1537,8 +1555,10 @@ function runMirrored(cwd, o, action, prepare) { }; // Self-check: the slot we publish must be one our own recovery accepts — // cap included. Failing closed here costs nothing; failing open costs a - // store nobody can recover. - wal.parseIntent(Buffer.from(wal.serializeRecord(intent), 'utf8')); + // store nobody can recover. The bytes are kept: the clear below re-checks + // it deletes exactly the slot this transaction published. + const intentBytes = Buffer.from(wal.serializeRecord(intent), 'utf8'); + wal.parseIntent(intentBytes); if (!createJsonExclusive(intentPath(cwd), intent)) { const err = new Error('the intent slot is occupied — recovery should have resolved it; refusing to overwrite'); err.code = 'ERATCHETMIRROR'; @@ -1553,7 +1573,7 @@ function runMirrored(cwd, o, action, prepare) { // recovery. The operation HAS happened; only the answer must say "pending". try { writeFileAtomic(ledgerPath(cwd), ledgerAfterBytes, () => fenceForFile(ledgerPath(cwd))); - clearIntentFile(intentPath(cwd)); + clearIntentFile(intentPath(cwd), intentBytes); } catch (e) { const err = new Error( `the state change committed (rev ${s.rev}) but the mirror is pending recovery — re-run the command: ${e && e.message}` @@ -1564,6 +1584,39 @@ function runMirrored(cwd, o, action, prepare) { return { committed: true, rev: s.rev, state: s, result: prep.result }; } +// Pure reads for the MCP read paths (spec 4b: every resource read is byte-pure, +// a stable projection or a conservative refusal). Unlike the resilient loaders +// they never create, back up, or reinitialize — present-but-unprovable bytes +// refuse with the mirror code the funnel already maps to its one sentence, and +// absence after open (which initializes both records) is the same condition. +function peekCanonical(file, what) { + let raw; + try { + raw = fs.readFileSync(file, 'utf8'); + } catch (e) { + const err = new Error(`${what} is ${e && e.code === 'ENOENT' ? 'absent' : 'unreadable'} on a read path — run ratchet doctor`); + err.code = 'ERATCHETMIRROR'; + throw err; + } + try { + const parsed = JSON.parse(raw); + if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) throw new Error('not a record'); + return parsed; + } catch (_e) { + const err = new Error(`${what} is present but not a readable record — run ratchet doctor`); + err.code = 'ERATCHETMIRROR'; + throw err; + } +} + +function peekState(cwd) { + return peekCanonical(statePath(cwd), 'state record'); +} + +function peekLedger(cwd) { + return peekCanonical(ledgerPath(cwd), 'ledger record'); +} + function loadLedger(cwd) { const existing = readJsonResilient(ledgerPath(cwd)); if (existing) return existing; @@ -1627,6 +1680,8 @@ module.exports = { withFileLock, diagnoseIntent, loadLedger, + peekState, + peekLedger, saveLedger, makeId, proposeOnlyAgent, diff --git a/src/wal.js b/src/wal.js index 50dce58be18b9d03e65238fb4d14054a6b9d5d99..d00373c8410a9815750dd70e04ced6691fbd04bb 100644 GIT binary patch delta 482 zcmYjN!Ab)$6hy^K4_ZOb4pMBZ?Rpi(iGxq?=b8U6Yc$t(GGGf>7}0!55(T?sl4{9Vn}@;e~ocenTp_A$i5d*&{7d zy{^+^kUU~o^l}phq?5?QbjLV=rCXfJwG-X>M!n#Ed$-=g1pU(X9@rOz`)2a%zY(2G zKdPP8?rAE>=DyI~%Vs~uWcFTtU0NG@v11QQ&&^gv9G7d_uGglI>njggPRbuyCd)E! zMrK3-JRs1W$`~P+fTD^tO~6nEIZ8&|LU0kODJQ0qIg^n+wX*?b*DwtE;OWMVB-MzF zY|_6f^d;#su8PD0Cwe}aJ+EIZR!T?`1!rHi=ECw{9cP<<*EVOzjqUmWz{%F;+z(Z; Bq=End delta 67 zcmaDFwJ?0cGy%Di)QS=XTZN>`lGI|olKkM3qRhN>4fWEJGz)c2>&=G**YQoRR { diff --git a/test/mcp-wal.test.js b/test/mcp-wal.test.js index 3cf8a83..bbd30e6 100644 --- a/test/mcp-wal.test.js +++ b/test/mcp-wal.test.js @@ -891,6 +891,132 @@ ok('T5 the wire transitions mean the CLI meaning; replay answers the retry; supe 'every wire transition kept the mirror truthful'); }); +// --------------------------------------------------------------------------- +// 4b.3: falsifiers from the independent review round (Codex, 2026-08-01). +// Each one was seen RED against main @ b029c81 before its fix landed. +// --------------------------------------------------------------------------- + +ok('W1 a slot whose bytes are not UTF-8 refuses instead of parsing a lossy normalization', () => { + const repo = fixture('w1'); + craftedSlot(repo); + const slotFile = state.intentPath(repo); + const bytes = Buffer.from(bytesOf(slotFile)); + const at = bytes.indexOf(Buffer.from('wal-crafted-operation', 'utf8')); + assert.ok(at > 0, 'the crafted operationId is in the slot'); + bytes[at + 4] = 0xff; // one invalid byte inside a JSON string + fs.writeFileSync(slotFile, bytes); + const before = storeSnapshot(repo); + assert.throws(() => triggerRecovery(repo), (e) => Boolean(e) && e.code === 'ERATCHETMIRROR', + 'invalid bytes are ambiguous — a U+FFFD substitute is a different operationId, not this one'); + assert.deepStrictEqual(storeSnapshot(repo), before, 'the refusal preserved every byte'); +}); + +ok('W2 recovery refuses a post-state slot whose tool contradicts the receipt it names', () => { + const repo = initRepo('w2'); + const conn = service([repo], true).createConnection(); + const open = openWorkspace(conn, repo); + const origRename = fs.renameSync; + fs.renameSync = (a, b) => { + if (path.basename(String(b)) === 'ledger.json') { + const e = new Error('injected mirror failure'); + e.code = 'EIO'; + throw e; + } + return origRename(a, b); + }; + let failed; + try { + failed = refusal(callTool(conn, 'defect.add', envelopeFor(open, { item: { severity: 'high', summary: 'tool binding' } }))); + } finally { + fs.renameSync = origRename; + } + assert.strictEqual(failed.error, 'WriteFailed'); + const slot = readIntent(repo); + assert.strictEqual(slot.tool, 'defect.add'); + slot.tool = 'defect.resolve'; // a known tool — just not the one this receipt earned + fs.writeFileSync(state.intentPath(repo), wal.serializeRecord(slot)); + const before = storeSnapshot(repo); + assert.throws(() => triggerRecovery(repo), (e) => Boolean(e) && e.code === 'ERATCHETMIRROR', + 'an after-image receipt must carry the tool the intent names'); + assert.deepStrictEqual(storeSnapshot(repo), before, 'nothing moved under the contradiction'); +}); + +ok('W3 an exact repeat over a stale mirror commits once and trues status, severity, and summary', () => { + const repo = fixture('w3'); + initStore(repo); + const added = settled(() => artifacts.addDefect(repo, { severity: 'high', summary: 'truth drifts' })); + settled(() => artifacts.transitionDefect(repo, added.state.id, 'resolved', { evidence: 'fixed', note: 'resolved: fixed' })); + const ledgerId = readState(repo).defects[0].ledgerId; + const ledger = readLedger(repo); + const row = ledger.defects.find((d) => d.id === ledgerId); + row.status = 'open'; + row.severity = 'low'; + row.summary = 'STALE SUMMARY'; + fs.writeFileSync(state.ledgerPath(repo), wal.serializeRecord(ledger)); + const repeat = settled(() => artifacts.transitionDefect(repo, added.state.id, 'resolved', { evidence: 'fixed', note: 'resolved: fixed' })); + assert.strictEqual(repeat.status, 'resolved'); + const trued = readLedger(repo).defects.find((d) => d.id === ledgerId); + assert.deepStrictEqual( + { status: trued.status, severity: trued.severity, summary: trued.summary }, + { status: 'resolved', severity: 'high', summary: 'truth drifts' }, + 'the one committed repeat trues every axis the spec projection names'); + const settledBytes = storeSnapshot(repo); + settled(() => artifacts.transitionDefect(repo, added.state.id, 'resolved', { evidence: 'fixed', note: 'resolved: fixed' })); + assert.deepStrictEqual(storeSnapshot(repo), settledBytes, 'once truthful, the repeat is a no-op again'); +}); + +ok('W4 doctor observes a pending slot without recovering it', () => { + const repo = fixture('w4'); + craftedSlot(repo, 'after-state'); // recoverable: the mirror is owed + const before = storeSnapshot(repo); + const run = childProcess.spawnSync(process.execPath, [path.join(__dirname, '..', 'bin', 'ratchet'), 'doctor'], + { cwd: repo, encoding: 'utf8', env: cleanGitEnv(), windowsHide: true }); + assert.match(String(run.stdout), /pending/i, 'doctor names the slot it sees'); + assert.deepStrictEqual(storeSnapshot(repo), before, 'diagnosis moved zero bytes — doctor never recovers'); +}); + +ok('W5 a resource read over an unreadable ledger refuses conservatively and writes nothing', () => { + const repo = initRepo('w5'); + const conn = service([repo], true).createConnection(); + const open = openWorkspace(conn, repo); + craftedSlot(repo); // occupy the slot so the store is mid-story + fs.writeFileSync(state.ledgerPath(repo), '{ this is not a record'); + const files = () => fs.readdirSync(state.projectDir(repo)).filter((n) => n !== '.lock' && !n.includes('.tmp-')).sort(); + const filesBefore = files(); + const ledgerBytes = bytesOf(state.ledgerPath(repo)); + const res = modern(conn, 'resources/read', { uri: open.resources.ledger }); + assert.ok(res.error, 'an unprovable ledger is a refusal, not a fresh projection'); + assert.strictEqual(res.error.message, mcp.WRITE_REFUSALS.MirrorUnrecoverable, + 'the refusal speaks the allowlisted sentence, never a store path'); + assert.deepStrictEqual(files(), filesBefore, 'the read created no backup, no fresh record, no residue'); + assert.ok(ledgerBytes.equals(bytesOf(state.ledgerPath(repo))), 'the unreadable bytes are untouched'); +}); + +ok('W6 clear verifies the slot it deletes is the slot it proved', () => { + const repo = fixture('w6'); + craftedSlot(repo); // before/before: recovery would discard, then clear + const slotFile = state.intentPath(repo); + const substitute = Object.assign(readIntent(repo), { operationId: 'wal-substituted-operation' }); + const origRead = fs.readFileSync; + let swapped = false; + fs.readFileSync = function (file, ...rest) { + const out = origRead.call(fs, file, ...rest); + if (!swapped && String(file) === slotFile) { + swapped = true; // swap AFTER recovery has read and proven the original + fs.writeFileSync(slotFile, wal.serializeRecord(substitute)); + } + return out; + }; + try { + assert.throws(() => triggerRecovery(repo), (e) => Boolean(e) && e.code === 'ERATCHETMIRROR', + 'deleting bytes that no longer match the proven slot is ambiguous, not a clear'); + } finally { + fs.readFileSync = origRead; + } + assert.strictEqual(readIntent(repo).operationId, 'wal-substituted-operation', + 'the substituted slot survived the refused clear'); +}); + // --------------------------------------------------------------------------- process.stdout.write(`\n${passed} passed, ${failures.length} failed\n`); From fe0505b7fe02b290fe59d176f1ab2ca3731ed908 Mon Sep 17 00:00:00 2001 From: Danny Gillespie Date: Sat, 1 Aug 2026 11:01:26 +0100 Subject: [PATCH 2/5] fix(wal): round two closes the races it can and names the one it cannot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 2 verdict was NO: two round-1 fixes incomplete under tighter timing, three new gaps. Ruling and action, all falsifiers seen red against 91589f4 before their fix: - W4b (accepted, was STILL-OPEN): doctor's writability probe no longer travels the write door at all — an existing store is probed with a scratch file, initProject only ever creates a store that does not exist yet (and a missing directory holds no slot). The reordering fix was beatable by a slot landing after the diagnosis sample; lock-free-by-construction is not. The round-1 deferral branch (which reported ok:true without testing — round-2 finding 5) is gone with it. - W7 (accepted, NEW): peekCanonical decodes fatally, same rule as the slot parser — an invalid-UTF-8 record refuses with the allowlisted sentence instead of serving a U+FFFD-normalized projection. - W3 extended (accepted, NEW): the stale-mirror truing commit returns the mirror op alone — no duplicate d.log/history line, no restamped resolvedAt. "Commits once solely to admit it" now means solely. - Clear TOCTOU (PARKED as a named limit, CHANGELOG): compare-and-unlink does not exist in a pathname API; the spec's re-check is implemented literally, and the residual window requires a writer already bypassing the workspace lock. Same class as the 2.5 check-to-read gap, recorded not machinery'd. npm test: full suite green (exit 0, 11/11 summaries) on this tree; earlier runs each dropped one roaming WAL test with the CHANGELOG-documented host EPERM signature (M1, R5, X1 — never the same test, never a W falsifier). doctor green. CI remains the clean-environment arbiter. Traced by: claude-fable-5 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01AU1qyUKzzmJ3NPH2HwWiPe --- CHANGELOG.md | 25 ++++++++++++++++---- src/artifacts.js | 6 ++++- src/cli.js | 40 ++++++++++++++++--------------- src/state.js | 6 +++-- test/mcp-wal.test.js | 56 ++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 107 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 779e3b4..6093b92 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,10 +24,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 (summary was dropped), and the exact-repeat validity check compares all three instead of counting linked rows — a stale-but-unique mirror is now trued by one committed repeat, then repeats no-op again. - - **Doctor observes without recovering (W4):** the WAL diagnosis now runs BEFORE the - writability probe, and under a pending slot the probe defers — previously doctor's - own `initProject` lock acquisition ran recovery, consumed the slot, and reported - "no pending intent" about the store it had just changed. + - **Doctor observes without recovering (W4, W4b):** the WAL diagnosis runs first, and + the writability probe no longer travels the write door at all — an existing store + is probed with a scratch file instead of `initProject`, whose lock acquisition ran + recovery. Previously doctor consumed the slot and reported "no pending intent" + about the store it had just changed; round 2 showed even a reordered probe loses a + slot that lands after the diagnosis sample, so the probe is now lock-free by + construction (W4b races a slot in after the sample and proves it survives). - **Resource reads are byte-pure, enforced (W5):** the MCP read paths (state/ledger resources, the receipt resource, `score.confidence`) now use pure peeks that never create, back up, or reinitialize a record; an unprovable record answers the one @@ -36,6 +39,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **CLEAR re-checks identity (W6):** the slot delete (spec CLEAR step) re-reads the slot and refuses if the bytes are not the bytes this pass proved — on both the transaction's own clear and recovery's fenced clear. + - **Round 2 of the same review added three more, fixed the same way:** the peeks + decode fatally like the slot parser — a record with invalid UTF-8 answers the + allowlisted sentence instead of serving a U+FFFD-normalized projection (W7); the + stale-mirror truing commit moves ONLY the mirror — no duplicate log/history line, + no restamped proof timestamp; "commits once solely to admit it" now means exactly + that (W3 extended); and the doctor probe race above (W4b). + - **Known limitation (parked, review round 2): the identity-checked clear is two + syscalls.** The CLEAR step re-reads and compares before deleting, exactly as the + spec words it — but compare and unlink cannot be one atomic operation through a + pathname API (no compare-and-unlink exists; same class as the 2.5 check→read gap). + A substitution landing in that window is deleted. Reaching it requires a writer + that bypasses the workspace lock — every lawful writer publishes and clears slots + under it — so this is recorded as a named limit, not repaired with machinery the + contract cannot honor. - Ruled NOT defects, on the record: pre-commit `.tmp-` residue (documented inert by design in 4b.1), CLI/MCP byte-equality (ids and receipts differ by ratified design), and the MCP retry wording (the spec deliberately gives MCP the generic diff --git a/src/artifacts.js b/src/artifacts.js index 0f56e3c..aa7882e 100644 --- a/src/artifacts.js +++ b/src/artifacts.js @@ -449,7 +449,11 @@ function prepareDefectTransition(s, ledger, id, toStatus, meta, mintId) { ); } // Exact repeat but the mirror is missing, ambiguous, or stale: commit once - // solely to perform the D2b admission (or the in-place truing) below. + // SOLELY to admit or true the mirror — the mirror op alone. The transition + // already happened; re-applying it would restamp proof-owned timestamps + // and append duplicate log/history lines, forging a second audit trail + // for one event (review round 2, finding 3). + return { kind: 'commit', record: d, ledgerOps: [mirrorOpFor(ledger, d, now, mintId)] }; } d.status = toStatus; diff --git a/src/cli.js b/src/cli.js index b7e778f..3b13158 100644 --- a/src/cli.js +++ b/src/cli.js @@ -804,16 +804,11 @@ function cmdDoctor(cwd, asJson) { } add('every skills/*/SKILL.md has frontmatter + description', skillProblems.length === 0, skillProblems.join('; ')); - // 4b WAL slot — read-only diagnosis, never a repair, and it goes FIRST: - // initProject below acquires the workspace lock, whose choke point RUNS - // recovery — doctor observing a slot through that door would consume the - // very thing it reports. A recoverable slot is informational (any supported - // write recovers it on its way in); a slot strict recovery cannot prove - // legal is the operator's, by name. - let slotPending = false; + // 4b WAL slot — read-only diagnosis, never a repair. A recoverable slot is + // informational (any supported write recovers it on its way in); a slot + // strict recovery cannot prove legal is the operator's, by name. try { const slot = state.diagnoseIntent(cwd); - slotPending = Boolean(slot.pending); if (!slot.pending) add('WAL intent slot', true, 'no pending intent'); else if (slot.verdict === 'ambiguous') { add('WAL intent slot', false, @@ -827,25 +822,32 @@ function cmdDoctor(cwd, asJson) { add('WAL intent slot', true, `pending intent, ${meaning[slot.verdict] || slot.verdict}`); } } catch (e) { - slotPending = true; // an undiagnosable slot is still a slot — do not recover it add('WAL intent slot', false, e.message); } - // The writability probe runs initProject, and the lock it takes would - // recover a pending slot on its way in — so under a pending slot the probe - // defers instead of quietly consuming the evidence doctor just reported. + // The writability probe never travels the write door: initProject acquires + // the workspace lock, whose choke point RUNS recovery — a slot present now, + // or landing at any point during this command, would be consumed by the + // very tool that exists to report it. An existing store is probed with a + // scratch file (named .tmp- like every inert residue); only a store whose + // directory does not exist yet is created through initProject, and a + // directory that does not exist holds no slot to consume. let stateDetail = ''; let stateOk = true; - if (slotPending) { - stateDetail = 'deferred — a pending WAL intent is reported above, and this probe would recover it'; - } else { - try { + try { + const dir = state.projectDir(cwd); // still names a store conflict out loud + if (fs.existsSync(dir)) { + const probe = path.join(dir, `.doctor-probe.tmp-${process.pid.toString(36)}`); + fs.writeFileSync(probe, 'doctor writability probe\n'); + fs.unlinkSync(probe); + stateDetail = dir; + } else { state.initProject(cwd); stateDetail = state.projectDir(cwd); - } catch (e) { - stateOk = false; - stateDetail = e.message; } + } catch (e) { + stateOk = false; + stateDetail = e.message; } add('state dir writable', stateOk, stateDetail); diff --git a/src/state.js b/src/state.js index 4551d49..4e782e0 100644 --- a/src/state.js +++ b/src/state.js @@ -1592,14 +1592,16 @@ function runMirrored(cwd, o, action, prepare) { function peekCanonical(file, what) { let raw; try { - raw = fs.readFileSync(file, 'utf8'); + raw = fs.readFileSync(file); } catch (e) { const err = new Error(`${what} is ${e && e.code === 'ENOENT' ? 'absent' : 'unreadable'} on a read path — run ratchet doctor`); err.code = 'ERATCHETMIRROR'; throw err; } try { - const parsed = JSON.parse(raw); + // Fatal decode, same rule as the slot parser: a lossy read would serve a + // U+FFFD-normalized projection of a record nobody wrote. + const parsed = JSON.parse(new TextDecoder('utf-8', { fatal: true }).decode(raw)); if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) throw new Error('not a record'); return parsed; } catch (_e) { diff --git a/test/mcp-wal.test.js b/test/mcp-wal.test.js index bbd30e6..5534768 100644 --- a/test/mcp-wal.test.js +++ b/test/mcp-wal.test.js @@ -947,6 +947,12 @@ ok('W3 an exact repeat over a stale mirror commits once and trues status, severi const added = settled(() => artifacts.addDefect(repo, { severity: 'high', summary: 'truth drifts' })); settled(() => artifacts.transitionDefect(repo, added.state.id, 'resolved', { evidence: 'fixed', note: 'resolved: fixed' })); const ledgerId = readState(repo).defects[0].ledgerId; + const settledState = readState(repo); + const audit = { + history: settledState.history.length, + log: settledState.defects[0].log.length, + resolvedAt: settledState.defects[0].resolvedAt, + }; const ledger = readLedger(repo); const row = ledger.defects.find((d) => d.id === ledgerId); row.status = 'open'; @@ -955,6 +961,11 @@ ok('W3 an exact repeat over a stale mirror commits once and trues status, severi fs.writeFileSync(state.ledgerPath(repo), wal.serializeRecord(ledger)); const repeat = settled(() => artifacts.transitionDefect(repo, added.state.id, 'resolved', { evidence: 'fixed', note: 'resolved: fixed' })); assert.strictEqual(repeat.status, 'resolved'); + const truedState = readState(repo); + assert.deepStrictEqual( + { history: truedState.history.length, log: truedState.defects[0].log.length, resolvedAt: truedState.defects[0].resolvedAt }, + audit, + 'the truing commit moved ONLY the mirror — no duplicate audit line, no restamped proof'); const trued = readLedger(repo).defects.find((d) => d.id === ledgerId); assert.deepStrictEqual( { status: trued.status, severity: trued.severity, summary: trued.summary }, @@ -975,6 +986,51 @@ ok('W4 doctor observes a pending slot without recovering it', () => { assert.deepStrictEqual(storeSnapshot(repo), before, 'diagnosis moved zero bytes — doctor never recovers'); }); +ok('W4b a slot landing after the diagnosis sample still survives doctor', () => { + const repo = initRepo('w4b'); + const cli = require('../src/cli'); + const realDiagnose = state.diagnoseIntent; + const realWrite = process.stdout.write; + const realExit = process.exitCode; + const realCwd = process.cwd(); + let planted = null; + try { + state.diagnoseIntent = function raced(cwd) { + const res = realDiagnose.call(state, cwd); + planted = craftedSlot(repo, 'after-state'); // the slot lands right after the sample + return res; + }; + process.chdir(repo); + process.stdout.write = () => true; // doctor's report is not under test; its writes are + cli.run(['node', 'ratchet', 'doctor', '--json']); + } finally { + process.stdout.write = realWrite; + process.chdir(realCwd); + state.diagnoseIntent = realDiagnose; + process.exitCode = realExit; + } + assert.ok(fs.existsSync(state.intentPath(repo)), 'the late slot is not consumed by the probe'); + assert.strictEqual(hashOf(state.ledgerPath(repo)), planted.intent.ledgerBeforeHash, + 'the owed mirror is still owed — doctor recovered nothing'); +}); + +ok('W7 a peeked record with invalid UTF-8 refuses instead of serving a normalized projection', () => { + const repo = initRepo('w7'); + const conn = service([repo], true).createConnection(); + const open = openWorkspace(conn, repo); + const stateFile = state.statePath(repo); + const bytes = Buffer.from(bytesOf(stateFile)); + const at = bytes.indexOf(Buffer.from('objective', 'utf8')); + assert.ok(at > 0, 'the state record carries an objective field'); + bytes[at + 3] = 0xff; // one invalid byte inside a JSON string + fs.writeFileSync(stateFile, bytes); + const res = modern(conn, 'resources/read', { uri: open.resources.state }); + assert.ok(res.error, 'an unprovable record is a refusal, not a lossy U+FFFD projection'); + assert.strictEqual(res.error.message, mcp.WRITE_REFUSALS.MirrorUnrecoverable, + 'the refusal speaks the allowlisted sentence, never a store path'); + assert.ok(bytes.equals(bytesOf(stateFile)), 'the read changed nothing'); +}); + ok('W5 a resource read over an unreadable ledger refuses conservatively and writes nothing', () => { const repo = initRepo('w5'); const conn = service([repo], true).createConnection(); From 17dcc09ec30cc309a5c96a46af6120a98ea93021 Mon Sep 17 00:00:00 2001 From: Danny Gillespie Date: Sat, 1 Aug 2026 11:44:23 +0100 Subject: [PATCH 3/5] =?UTF-8?q?fix(wal):=20round=20three=20=E2=80=94=20eve?= =?UTF-8?q?ry=20canonical=20read=20decodes=20fatally,=20doctor=20touches?= =?UTF-8?q?=20no=20lock?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 3 (fresh session; V1/V3/V4/V5 verified PASS, lock-window ruling upheld) found two remaining gaps; both accepted, fixed red-first against fe0505b: - W8/W9 (high): readForMirror and recovery's state/ledger parses now use the fatal decode the slot parser and peeks already use. Before: a lawful mirrored write over a record carrying one invalid byte parsed lossily, committed a U+FFFD-normalized serialization, and settled the two files in permanent disagreement — pendingIntent false, nothing owed, the exact outcome the WAL exists to make impossible. - W4c (medium): doctor's writability probe never reaches initProject even for a missing store — it probes the nearest existing ancestor instead, so a first store created by another process in the sample-to-probe window cannot be recovered by the diagnosis tool. Behavior change stated in CHANGELOG: doctor no longer creates the store; the first write does. WAL suite 38/0 on a clean run; the roaming host EPERM signature hit D1, X5, M2, D1 across other runs (never a W falsifier; one X5 look-alike investigated and ruled the flake — a clean run containing X5 green exists on this tree). doctor green. CI remains the clean-environment arbiter. Traced by: claude-fable-5 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01AU1qyUKzzmJ3NPH2HwWiPe --- CHANGELOG.md | 9 ++++++ src/cli.js | 34 +++++++++++---------- src/state.js | 5 +++- src/wal.js | Bin 11621 -> 11835 bytes test/mcp-wal.test.js | 69 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 100 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6093b92..2714c88 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 stale-mirror truing commit moves ONLY the mirror — no duplicate log/history line, no restamped proof timestamp; "commits once solely to admit it" now means exactly that (W3 extended); and the doctor probe race above (W4b). + - **Round 3 of the same review added two more, fixed the same way:** the strict + mirrored-write reads and recovery's two record parses decode fatally like + everything else (W8, W9) — previously a lawful write over a record carrying one + invalid byte would commit a U+FFFD-normalized serialization and settle state and + ledger in permanent disagreement with no pending intent left behind; and doctor's + missing-store branch no longer reaches `initProject` at all (W4c) — a missing + store probes its nearest existing ancestor, so no first store appearing in the + sample-to-probe window can be recovered by the diagnosis tool. Doctor no longer + creates the store as a side effect; the first write does. - **Known limitation (parked, review round 2): the identity-checked clear is two syscalls.** The CLEAR step re-reads and compares before deleting, exactly as the spec words it — but compare and unlink cannot be one atomic operation through a diff --git a/src/cli.js b/src/cli.js index 3b13158..9181f18 100644 --- a/src/cli.js +++ b/src/cli.js @@ -825,26 +825,28 @@ function cmdDoctor(cwd, asJson) { add('WAL intent slot', false, e.message); } - // The writability probe never travels the write door: initProject acquires - // the workspace lock, whose choke point RUNS recovery — a slot present now, - // or landing at any point during this command, would be consumed by the - // very tool that exists to report it. An existing store is probed with a - // scratch file (named .tmp- like every inert residue); only a store whose - // directory does not exist yet is created through initProject, and a - // directory that does not exist holds no slot to consume. + // The writability probe never travels the write door — not even for a store + // that does not exist yet: initProject acquires the workspace lock, whose + // choke point RUNS recovery, and a first store created by another process + // in the sample-to-probe window would be recovered by the very tool that + // exists to report it (seen live in review round 3). An existing store is + // probed with a scratch file (named .tmp- like every inert residue); a + // missing store probes its nearest existing ancestor — the first write + // creates the store itself. let stateDetail = ''; let stateOk = true; try { const dir = state.projectDir(cwd); // still names a store conflict out loud - if (fs.existsSync(dir)) { - const probe = path.join(dir, `.doctor-probe.tmp-${process.pid.toString(36)}`); - fs.writeFileSync(probe, 'doctor writability probe\n'); - fs.unlinkSync(probe); - stateDetail = dir; - } else { - state.initProject(cwd); - stateDetail = state.projectDir(cwd); - } + let probeDir = dir; + while (!fs.existsSync(probeDir)) { + const up = path.dirname(probeDir); + if (up === probeDir) break; + probeDir = up; + } + const probe = path.join(probeDir, `.doctor-probe.tmp-${process.pid.toString(36)}`); + fs.writeFileSync(probe, 'doctor writability probe\n'); + fs.unlinkSync(probe); + stateDetail = probeDir === dir ? dir : `${dir} (not created yet — the first write creates it)`; } catch (e) { stateOk = false; stateDetail = e.message; diff --git a/src/state.js b/src/state.js index 4e782e0..d9da425 100644 --- a/src/state.js +++ b/src/state.js @@ -1496,7 +1496,10 @@ function readForMirror(file, what) { throw err; } try { - const parsed = JSON.parse(bytes.toString('utf8')); + // Fatal decode, same rule as the slot parser: a lossy read here would let + // a lawful write serialize a U+FFFD-normalized record and settle the two + // files in permanent disagreement with no pending intent left behind. + const parsed = JSON.parse(new TextDecoder('utf-8', { fatal: true }).decode(bytes)); if (!parsed || typeof parsed !== 'object' || Array.isArray(parsed)) throw new Error('not a record'); return { bytes, parsed }; } catch (_e) { diff --git a/src/wal.js b/src/wal.js index d00373c8410a9815750dd70e04ced6691fbd04bb..762e86750f29662e197463381def5d5589dd933b 100644 GIT binary patch delta 205 zcmYk!Jqp4=6a-+jwD9Ku!YhM>1kuU^2)34D^WqLBxY@+rmtbr>L@pv&dLB!`Bbda- zsRm~FUc2Z1zVL2gNNX0>q;SX-eA3AYl(Ti%7=e)$#q46jxnP}0MY+vNIFc9+#1?HE zP?yHCjbarPMU}w$6cy4QejufV6LOMhO$H?_nPFzr#=Uk-8q~fwt(TMe$O#L!U=?)G TBlbSMb{K1@BkbRDnZ5g8OK(X3 delta 90 zcmdlT^E7IMvBu=%8e;OrC5a`eL8*x;dP$Wfsl|FF`N1VcnR)3N>ZK)V7V4AzwDmT7 WYaU`$$Vp8}Pc6c&a 'the owed mirror is still owed — doctor recovered nothing'); }); +ok('W4c doctor never travels the write door — a missing store is probed, not initialized', () => { + const repo = initRepo('w4c'); // no initStore: the store directory does not exist + const cli = require('../src/cli'); + const realInit = state.initProject; + const realWrite = process.stdout.write; + const realExit = process.exitCode; + const realCwd = process.cwd(); + let entered = 0; + let captured = ''; + try { + state.initProject = function counted(...a) { + entered++; + return realInit.apply(state, a); + }; + process.chdir(repo); + process.stdout.write = (s) => { + captured += String(s); + return true; + }; + cli.run(['node', 'ratchet', 'doctor', '--json']); + } finally { + process.stdout.write = realWrite; + process.chdir(realCwd); + state.initProject = realInit; + process.exitCode = realExit; + } + const writable = JSON.parse(captured).checks.find((c) => c.name === 'state dir writable'); + assert.strictEqual(entered, 0, + 'the probe must never enter initProject — its lock would recover whatever store just appeared'); + assert.strictEqual(writable.ok, true, 'the writability answer is still a real answer'); + assert.ok(!fs.existsSync(state.projectDir(repo)), 'diagnosis created no store'); +}); + +ok('W8 a mirrored write over a state record with invalid UTF-8 refuses and moves nothing', () => { + const repo = fixture('w8'); + initStore(repo); + settled(() => artifacts.addDefect(repo, { severity: 'high', summary: 'utf8 sentinel state' })); + const stateFile = state.statePath(repo); + const bytes = Buffer.from(bytesOf(stateFile)); + const at = bytes.indexOf(Buffer.from('utf8 sentinel state', 'utf8')); + assert.ok(at > 0); + bytes[at + 2] = 0xff; // one invalid byte inside the recorded summary + fs.writeFileSync(stateFile, bytes); + const before = storeSnapshot(repo); + assert.throws( + () => artifacts.addDefect(repo, { severity: 'low', summary: 'a different lawful finding' }), + (e) => Boolean(e) && e.code === 'ERATCHETMIRROR', + 'a strict read never normalizes canonical bytes into a record nobody wrote'); + assert.deepStrictEqual(storeSnapshot(repo), before, 'the refusal moved zero bytes'); +}); + +ok('W9 a mirrored write over a ledger record with invalid UTF-8 refuses and moves nothing', () => { + const repo = fixture('w9'); + initStore(repo); + settled(() => artifacts.addDefect(repo, { severity: 'high', summary: 'utf8 sentinel ledger' })); + const ledgerFile = state.ledgerPath(repo); + const bytes = Buffer.from(bytesOf(ledgerFile)); + const at = bytes.indexOf(Buffer.from('utf8 sentinel ledger', 'utf8')); + assert.ok(at > 0); + bytes[at + 2] = 0xff; + fs.writeFileSync(ledgerFile, bytes); + const before = storeSnapshot(repo); + assert.throws( + () => artifacts.addDefect(repo, { severity: 'low', summary: 'a different lawful finding' }), + (e) => Boolean(e) && e.code === 'ERATCHETMIRROR', + 'the mirror side is held to the same strict decode as the state side'); + assert.deepStrictEqual(storeSnapshot(repo), before, 'the refusal moved zero bytes'); +}); + ok('W7 a peeked record with invalid UTF-8 refuses instead of serving a normalized projection', () => { const repo = initRepo('w7'); const conn = service([repo], true).createConnection(); From a4c9cba92f5f375fa52b3d1925564727f23c08b2 Mon Sep 17 00:00:00 2001 From: Danny Gillespie Date: Sat, 1 Aug 2026 12:30:36 +0100 Subject: [PATCH 4/5] =?UTF-8?q?fix(wal):=20round=20four=20=E2=80=94=20the?= =?UTF-8?q?=20ordinary=20doors=20join=20the=20fatal-decode=20rule?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 4 (fresh session; V1 PASS — both round-3 fixes verified closed) found the same defect class one layer out, plus a probe blind spot. Both accepted, fixed red-first (W10-W13 seen red against the pre-fix sources): - W10/W11 (high): readJson and readJsonResilient decoded permissively, so a lawful ORDINARY write (state.set, ledger save) over a record carrying one invalid byte would silently serialize a U+FFFD normalization — settled state/ledger disagreement, valid JSON, no backup, no intent. Now the fast path answers null on undecodable bytes and the resilient loader routes them through its existing loud backup-then-reinitialize contract, backup preserving the exact original bytes (reads as Buffer end to end). - W13 (medium): the doctor probe creates a scratch DIRECTORY with a file inside, not just a file — Windows ACLs grant CreateFiles and CreateDirectories separately, the workspace lock is a mkdir, and the file-only probe said "writable" where the first real write failed its mkdir. - W12: suite coverage for recovery's fatal decode (round-3 fix previously proven only by review probes) — an adversarially consistent slot whose after-hash certifies undecodable state bytes refuses, every byte preserved. WAL suite 42/0; full suite green (exit 0, 11/11 summaries) first attempt. doctor green. Review trajectory: 10 → 5 → 2 → 2 findings; every canonical byte path now decodes fatally — the class should be closed, round 5 checks. Traced by: claude-fable-5 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01AU1qyUKzzmJ3NPH2HwWiPe --- CHANGELOG.md | 12 ++++++ src/cli.js | 11 +++-- src/state.js | 22 ++++++++-- test/mcp-wal.test.js | 96 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 134 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2714c88..02b34db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 store probes its nearest existing ancestor, so no first store appearing in the sample-to-probe window can be recovered by the diagnosis tool. Doctor no longer creates the store as a side effect; the first write does. + - **Round 4 of the same review added two more, fixed the same way:** the ordinary + loaders joined the fatal-decode rule (W10, W11) — `readJson`'s fast-path peek + answers null on undecodable bytes, and `readJsonResilient` routes invalid UTF-8 + through the same loud backup-then-reinitialize path it has always used for invalid + JSON, with the backup now preserving the exact original bytes; previously a lawful + ordinary write would silently serialize a U+FFFD-normalized record and settle the + two canonical files in disagreement with no backup and no intent. And the doctor + writability probe now proves directory creation as well as file creation (W13) — + Windows ACLs grant the two separately, the lock is a mkdir, and a file-only probe + answered "writable" where the first real write then failed. W12 additionally pins + recovery's own fatal decode against an adversarially consistent slot whose hash + certifies undecodable bytes. - **Known limitation (parked, review round 2): the identity-checked clear is two syscalls.** The CLEAR step re-reads and compares before deleting, exactly as the spec words it — but compare and unlink cannot be one atomic operation through a diff --git a/src/cli.js b/src/cli.js index 9181f18..75d4798 100644 --- a/src/cli.js +++ b/src/cli.js @@ -843,9 +843,14 @@ function cmdDoctor(cwd, asJson) { if (up === probeDir) break; probeDir = up; } - const probe = path.join(probeDir, `.doctor-probe.tmp-${process.pid.toString(36)}`); - fs.writeFileSync(probe, 'doctor writability probe\n'); - fs.unlinkSync(probe); + // The probe proves BOTH creations a real writer needs: a directory (the + // lock is a mkdir; a missing store starts as one) and a file inside it — + // Windows ACLs grant these separately, and a file-only probe said + // "writable" where the first real write then failed its mkdir. + const scratch = path.join(probeDir, `.doctor-probe.tmp-${process.pid.toString(36)}`); + fs.mkdirSync(scratch); + fs.writeFileSync(path.join(scratch, 'probe'), 'doctor writability probe\n'); + fs.rmSync(scratch, { recursive: true, force: true }); stateDetail = probeDir === dir ? dir : `${dir} (not created yet — the first write creates it)`; } catch (e) { stateOk = false; diff --git a/src/state.js b/src/state.js index d9da425..9ff2ee9 100644 --- a/src/state.js +++ b/src/state.js @@ -161,7 +161,11 @@ function ensureDir(dir) { function readJson(file) { try { - return JSON.parse(fs.readFileSync(file, 'utf8')); + // Fatal decode: this is the healthy-record fast path, and a lossy read + // here hands callers a U+FFFD-normalized object that a lawful write then + // serializes. Undecodable bytes are not healthy — answer null and let the + // slow path preserve them. + return JSON.parse(new TextDecoder('utf-8', { fatal: true }).decode(fs.readFileSync(file))); } catch (_e) { return null; } @@ -272,7 +276,7 @@ function backupCorrupt(file, raw) { function readJsonResilient(file) { let raw; try { - raw = fs.readFileSync(file, 'utf8'); + raw = fs.readFileSync(file); } catch (e) { // ENOENT is the only error that means "there is no record yet". Anything // else — an ACL that denies read but permits write, a lock, EIO — means the @@ -284,9 +288,19 @@ function readJsonResilient(file) { 'refusing to reinitialize over a record that is present but unreadable. Fix access, then re-run.' ); } - if (!raw.trim()) return null; // empty file → fresh, no noisy backup + let text; + try { + text = new TextDecoder('utf-8', { fatal: true }).decode(raw); + } catch (_e) { + // Invalid bytes travel the SAME preservation path as invalid JSON. The + // lossy decode used to parse them into a U+FFFD-normalized record that a + // lawful ordinary write then serialized — the two canonical files settled + // in disagreement with no backup, no intent, and no warning. + return rejectUnusable(file, raw, 'it is not valid UTF-8'); + } + if (!text.trim()) return null; // empty file → fresh, no noisy backup try { - const parsed = JSON.parse(raw); + const parsed = JSON.parse(text); // `null`, `false`, `0`, `""` and `[]` all parse. They are not records, and the // caller reinitializes over anything falsey — so they have to travel the SAME // preservation path as malformed bytes, or a valid-but-unusable document gets diff --git a/test/mcp-wal.test.js b/test/mcp-wal.test.js index fac3854..3fb10fe 100644 --- a/test/mcp-wal.test.js +++ b/test/mcp-wal.test.js @@ -1142,6 +1142,102 @@ ok('W6 clear verifies the slot it deletes is the slot it proved', () => { 'the substituted slot survived the refused clear'); }); +ok('W10 an ordinary state writer never serializes a normalized record — bytes back up, loudly', () => { + const repo = fixture('w10'); + initStore(repo); + settled(() => artifacts.addDefect(repo, { severity: 'high', summary: 'ordinary state sentinel' })); + const stateFile = state.statePath(repo); + const damaged = Buffer.from(bytesOf(stateFile)); + const at = damaged.indexOf(Buffer.from('ordinary state sentinel', 'utf8')); + assert.ok(at > 0); + damaged[at + 2] = 0xff; + fs.writeFileSync(stateFile, damaged); + settled(() => state.withWorkspaceMutation(repo, { action: 'w10 ordinary write' }, (s) => { + s.objective = 'a lawful unrelated write'; + })); + assert.ok(!bytesOf(stateFile).toString('utf8').includes('�'), + 'no U+FFFD ever reaches a canonical file — normalization is not recovery'); + const dir = path.dirname(stateFile); + const backups = fs.readdirSync(dir).filter((n) => n.startsWith('state.json.corrupt.')); + assert.ok(backups.length >= 1, 'the undecodable bytes were preserved before anything replaced them'); + for (const b of backups) { + assert.ok(damaged.equals(bytesOf(path.join(dir, b))), 'the backup is the exact original bytes'); + } +}); + +ok('W11 an ordinary ledger writer never serializes a normalized record — bytes back up, loudly', () => { + const repo = fixture('w11'); + initStore(repo); + settled(() => artifacts.addDefect(repo, { severity: 'high', summary: 'ordinary ledger sentinel' })); + const ledgerFile = state.ledgerPath(repo); + const damaged = Buffer.from(bytesOf(ledgerFile)); + const at = damaged.indexOf(Buffer.from('ordinary ledger sentinel', 'utf8')); + assert.ok(at > 0); + damaged[at + 2] = 0xff; + fs.writeFileSync(ledgerFile, damaged); + const loaded = state.loadLedger(repo); + settled(() => state.saveLedger(repo, loaded)); + assert.ok(!bytesOf(ledgerFile).toString('utf8').includes('�'), 'no U+FFFD ever reaches a canonical file'); + const dir = path.dirname(ledgerFile); + const backups = fs.readdirSync(dir).filter((n) => n.startsWith('ledger.json.corrupt.')); + assert.ok(backups.length >= 1, 'the undecodable bytes were preserved'); + for (const b of backups) { + assert.ok(damaged.equals(bytesOf(path.join(dir, b))), 'the backup is the exact original bytes'); + } +}); + +ok('W12 recovery refuses a state after-image whose bytes are not UTF-8, hash match or not', () => { + const repo = fixture('w12'); + craftedSlot(repo, 'after-state'); + const stateFile = state.statePath(repo); + const damaged = Buffer.from(bytesOf(stateFile)); + const at = damaged.indexOf(Buffer.from('crafted', 'utf8')); + assert.ok(at > 0); + damaged[at + 2] = 0xff; + fs.writeFileSync(stateFile, damaged); + const slot = readIntent(repo); + slot.stateAfterHash = wal.hashBytes(damaged); // an adversarially consistent slot + fs.writeFileSync(state.intentPath(repo), wal.serializeRecord(slot)); + const before = storeSnapshot(repo); + assert.throws(() => triggerRecovery(repo), (e) => Boolean(e) && e.code === 'ERATCHETMIRROR', + 'a hash can certify damaged bytes; the parse must still refuse them'); + assert.deepStrictEqual(storeSnapshot(repo), before, 'the refusal preserved every byte'); +}); + +ok('W13 the doctor probe proves directory creation, not just file creation', () => { + const repo = initRepo('w13'); + const cli = require('../src/cli'); + const realMkdir = fs.mkdirSync; + const realWrite = process.stdout.write; + const realExit = process.exitCode; + const realCwd = process.cwd(); + let captured = ''; + try { + fs.mkdirSync = function denied(target, ...rest) { + if (String(target).includes('.doctor-probe')) { + const e = new Error(`EPERM: operation not permitted, mkdir '${target}'`); + e.code = 'EPERM'; + throw e; // the ACL shape Windows grants: files allowed, directories denied + } + return realMkdir.call(fs, target, ...rest); + }; + process.chdir(repo); + process.stdout.write = (s) => { + captured += String(s); + return true; + }; + cli.run(['node', 'ratchet', 'doctor', '--json']); + } finally { + fs.mkdirSync = realMkdir; + process.stdout.write = realWrite; + process.chdir(realCwd); + process.exitCode = realExit; + } + const writable = JSON.parse(captured).checks.find((c) => c.name === 'state dir writable'); + assert.strictEqual(writable.ok, false, + 'a place where directories cannot be created is not writable for a store'); +}); + // --------------------------------------------------------------------------- process.stdout.write(`\n${passed} passed, ${failures.length} failed\n`); From c278bcf8745652180169436115d14f8f13f65997 Mon Sep 17 00:00:00 2001 From: Danny Gillespie Date: Sat, 1 Aug 2026 12:53:04 +0100 Subject: [PATCH 5/5] =?UTF-8?q?fix(wal):=20round=20five=20=E2=80=94=20YES?= =?UTF-8?q?=20on=20the=20record;=20the=20probe=20cleans=20up=20after=20its?= =?UTF-8?q?elf?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 5 verdict: YES — "one operation, two files, one crash story — the mirror guarantee holds." V1-V4 all PASS: both round-4 fixes verified closed, the canonical decode sweep found no remaining permissive parse (seven fatal decoders enumerated, the permissive reads left in src/ touch no canonical file), the probe matrix answered all four ACL shapes correctly, and its own third full run was green after two documented-signature holds. Its one LOW, fixed red-first (W14 seen red before the fix): a probe whose mkdir succeeded but whose file write failed left the scratch directory behind — inert, but residue a cold-start scan could misread. Cleanup now rides a best-effort finally; the writability answer stands either way. Review trajectory closed: 10 → 5 → 2 → 2 → 1 low → YES. WAL suite 43/0; full suite green (exit 0, 11/11 summaries); doctor healthy. Traced by: claude-fable-5 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01AU1qyUKzzmJ3NPH2HwWiPe --- CHANGELOG.md | 5 +++++ src/cli.js | 13 ++++++++++--- test/mcp-wal.test.js | 37 +++++++++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 02b34db..85e9b33 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -66,6 +66,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 answered "writable" where the first real write then failed. W12 additionally pins recovery's own fatal decode against an adversarially consistent slot whose hash certifies undecodable bytes. + - **Round 5 returned the review's YES** ("one operation, two files, one crash story — + the mirror guarantee holds"): both round-4 fixes verified, the decode sweep found + no remaining permissive parse on any canonical byte path, and the probe matrix + passed all four ACL shapes. Its one LOW — a half-failed doctor probe left its + scratch directory behind — is fixed (cleanup in a best-effort finally, W14). - **Known limitation (parked, review round 2): the identity-checked clear is two syscalls.** The CLEAR step re-reads and compares before deleting, exactly as the spec words it — but compare and unlink cannot be one atomic operation through a diff --git a/src/cli.js b/src/cli.js index 75d4798..5447a68 100644 --- a/src/cli.js +++ b/src/cli.js @@ -848,9 +848,16 @@ function cmdDoctor(cwd, asJson) { // Windows ACLs grant these separately, and a file-only probe said // "writable" where the first real write then failed its mkdir. const scratch = path.join(probeDir, `.doctor-probe.tmp-${process.pid.toString(36)}`); - fs.mkdirSync(scratch); - fs.writeFileSync(path.join(scratch, 'probe'), 'doctor writability probe\n'); - fs.rmSync(scratch, { recursive: true, force: true }); + try { + fs.mkdirSync(scratch); + fs.writeFileSync(path.join(scratch, 'probe'), 'doctor writability probe\n'); + } finally { + // Best-effort, always: a probe that failed half-way must not leave + // residue a cold-start scan could misread as an interrupted write. + try { + fs.rmSync(scratch, { recursive: true, force: true }); + } catch (_e) { /* the writability answer stands either way */ } + } stateDetail = probeDir === dir ? dir : `${dir} (not created yet — the first write creates it)`; } catch (e) { stateOk = false; diff --git a/test/mcp-wal.test.js b/test/mcp-wal.test.js index 3fb10fe..fb59aef 100644 --- a/test/mcp-wal.test.js +++ b/test/mcp-wal.test.js @@ -1238,6 +1238,43 @@ ok('W13 the doctor probe proves directory creation, not just file creation', () 'a place where directories cannot be created is not writable for a store'); }); +ok('W14 a half-failed doctor probe cleans up after itself', () => { + const repo = initRepo('w14'); + initStore(repo); + const cli = require('../src/cli'); + const realWriteFile = fs.writeFileSync; + const realWrite = process.stdout.write; + const realExit = process.exitCode; + const realCwd = process.cwd(); + let captured = ''; + try { + fs.writeFileSync = function denied(target, ...rest) { + if (String(target).includes('.doctor-probe')) { + const e = new Error(`EACCES: permission denied, open '${target}'`); + e.code = 'EACCES'; + throw e; // the mkdir succeeded; the file inside is what fails + } + return realWriteFile.call(fs, target, ...rest); + }; + process.chdir(repo); + process.stdout.write = (s) => { + captured += String(s); + return true; + }; + cli.run(['node', 'ratchet', 'doctor', '--json']); + } finally { + fs.writeFileSync = realWriteFile; + process.stdout.write = realWrite; + process.chdir(realCwd); + process.exitCode = realExit; + } + const writable = JSON.parse(captured).checks.find((c) => c.name === 'state dir writable'); + assert.strictEqual(writable.ok, false, 'the half-failed probe still answers honestly'); + const residue = fs.readdirSync(state.projectDir(repo)).filter((n) => n.includes('.doctor-probe')); + assert.deepStrictEqual(residue, [], + 'a failed diagnostic leaves nothing a cold-start scan could misread as an interrupted write'); +}); + // --------------------------------------------------------------------------- process.stdout.write(`\n${passed} passed, ${failures.length} failed\n`);