From f4d939e07daa982ad01095db7a27a88d9864b0ea Mon Sep 17 00:00:00 2001 From: luvs01 Date: Fri, 7 Aug 2026 22:37:54 +0900 Subject: [PATCH] fix(codex): bind prompt journal recovery paths --- src/codex/prompt-journal.ts | 17 ++++++++++++- src/codex/prompt-layers.ts | 2 +- tests/codex-prompt-journal.test.ts | 39 +++++++++++++++++++++++------- 3 files changed, 47 insertions(+), 11 deletions(-) diff --git a/src/codex/prompt-journal.ts b/src/codex/prompt-journal.ts index 74c9e12d6d..9df077fdb3 100644 --- a/src/codex/prompt-journal.ts +++ b/src/codex/prompt-journal.ts @@ -217,7 +217,11 @@ export type RecoveryOutcome = * The single exception is not a roll-forward: when BOTH targets already hold the * post-image the writes had finished and only the journal deletion was missing. */ -export function recoverIfNeeded(journalPath: string): RecoveryOutcome { +export function recoverIfNeeded( + journalPath: string, + expectedConfigPath: string, + expectedStorePath: string, +): RecoveryOutcome { const raw = readOrNull(journalPath); if (raw === null) return { ok: true, action: "none" }; @@ -230,6 +234,17 @@ export function recoverIfNeeded(journalPath: string): RecoveryOutcome { }; } + // The checksum detects corruption, not forgery. Bind the record to the + // paths selected by the active prompt-layer configuration before reading or + // writing either target. + if (record.configPath !== expectedConfigPath || record.storePath !== expectedStorePath) { + return { + ok: false, + error: "recovery_required", + detail: `journal at ${journalPath} does not match the active prompt-layer paths; nothing was read from it`, + }; + } + const config = classify(readOrNull(record.configPath), record.preConfig, record.postConfig); const store = classify(readOrNull(record.storePath), record.preStore, record.postStore); diff --git a/src/codex/prompt-layers.ts b/src/codex/prompt-layers.ts index 0dd53a2fb2..d75348ce94 100644 --- a/src/codex/prompt-layers.ts +++ b/src/codex/prompt-layers.ts @@ -650,7 +650,7 @@ function commit( try { // 1. recovery first: a journal on disk means an earlier attempt never // committed, and we must not stack a second transaction on top of it. - const recovered = recoverJournal(journalPath); + const recovered = recoverJournal(journalPath, configPath, storePath); if (!recovered.ok) return { ok: false, error: "recovery_required", detail: recovered.detail }; // 2. re-read and compare against the caller's edit base. diff --git a/tests/codex-prompt-journal.test.ts b/tests/codex-prompt-journal.test.ts index c2d7146699..0bb7c4b7fd 100644 --- a/tests/codex-prompt-journal.test.ts +++ b/tests/codex-prompt-journal.test.ts @@ -74,6 +74,10 @@ function read(path: string): string | null { return existsSync(path) ? readFileSync(path, "utf8") : null; } +function recover(s: ReturnType) { + return recoverIfNeeded(s.journalPath, s.configPath, s.storePath); +} + describe("envelope", () => { test("round-trips a record", () => { const record = { configPath: "/c", storePath: "/s" } as JournalRecord; @@ -114,14 +118,15 @@ describe("classification", () => { describe("recovery", () => { test("no journal is a no-op", () => { const dir = root(); - expect(recoverIfNeeded(join(dir, "absent.journal"))).toEqual({ ok: true, action: "none" }); + expect(recoverIfNeeded(join(dir, "absent.journal"), join(dir, "config.toml"), join(dir, "store.json"))) + .toEqual({ ok: true, action: "none" }); }); test("both targets at post-image: commit by deleting the journal", () => { // The writes finished; only step 6 was missing. This is the ONE case that // does not roll back, and it is not a roll-forward. const s = scenario({ config: "POST_C", store: "POST_S" }); - expect(recoverIfNeeded(s.journalPath)).toEqual({ ok: true, action: "committed" }); + expect(recover(s)).toEqual({ ok: true, action: "committed" }); expect(read(s.configPath)).toBe("POST_C"); expect(read(s.storePath)).toBe("POST_S"); expect(existsSync(s.journalPath)).toBe(false); @@ -131,7 +136,7 @@ describe("recovery", () => { // Crash after config.toml but before the store. A journal on disk means // commit never happened, so the transaction is undone. const s = scenario({ config: "POST_C", store: "PRE_S" }); - expect(recoverIfNeeded(s.journalPath)).toEqual({ ok: true, action: "rolled-back" }); + expect(recover(s)).toEqual({ ok: true, action: "rolled-back" }); expect(read(s.configPath)).toBe("PRE_C"); expect(read(s.storePath)).toBe("PRE_S"); expect(existsSync(s.journalPath)).toBe(false); @@ -139,7 +144,7 @@ describe("recovery", () => { test("nothing applied: leave both alone", () => { const s = scenario({ config: "PRE_C", store: "PRE_S" }); - expect(recoverIfNeeded(s.journalPath).ok).toBe(true); + expect(recover(s).ok).toBe(true); expect(read(s.configPath)).toBe("PRE_C"); expect(read(s.storePath)).toBe("PRE_S"); }); @@ -148,7 +153,7 @@ describe("recovery", () => { // THE case this module exists for: crash, then Codex or the user edits // config.toml. Recovery must not overwrite it with a stale image. const s = scenario({ config: "SOMEONE ELSE WROTE THIS", store: "PRE_S" }); - const result = recoverIfNeeded(s.journalPath); + const result = recover(s); expect(result.ok).toBe(false); expect(read(s.configPath)).toBe("SOMEONE ELSE WROTE THIS"); expect(existsSync(s.journalPath)).toBe(true); @@ -158,7 +163,7 @@ describe("recovery", () => { // One unknown target aborts the WHOLE recovery: we never repair one file // while the other carries a stranger's edit. const s = scenario({ config: "POST_C", store: "SOMEONE ELSE" }); - expect(recoverIfNeeded(s.journalPath).ok).toBe(false); + expect(recover(s).ok).toBe(false); expect(read(s.configPath)).toBe("POST_C"); expect(read(s.storePath)).toBe("SOMEONE ELSE"); }); @@ -166,7 +171,7 @@ describe("recovery", () => { test("a corrupt journal is recovery_required, and nothing is written", () => { const s = scenario({ config: "POST_C", store: "PRE_S" }); writeFileSync(s.journalPath, "ocx-journal-v1 deadbeef\n{\"configPath\":\"/x\"}", "utf8"); - const result = recoverIfNeeded(s.journalPath); + const result = recover(s); expect(result.ok).toBe(false); expect(read(s.configPath)).toBe("POST_C"); expect(read(s.storePath)).toBe("PRE_S"); @@ -177,7 +182,7 @@ describe("recovery", () => { const s = scenario({ config: "POST_C", store: "PRE_S" }); const encoded = readFileSync(s.journalPath, "utf8"); writeFileSync(s.journalPath, encoded.slice(0, encoded.length - 20), "utf8"); - expect(recoverIfNeeded(s.journalPath).ok).toBe(false); + expect(recover(s).ok).toBe(false); expect(read(s.configPath)).toBe("POST_C"); }); @@ -195,9 +200,25 @@ describe("recovery", () => { preConfigBytes: null, postConfigBytes: "POST_C", preStoreBytes: "PRE_S", postStoreBytes: "POST_S", }), "utf8"); - expect(recoverIfNeeded(journalPath).ok).toBe(true); + expect(recoverIfNeeded(journalPath, configPath, storePath).ok).toBe(true); expect(existsSync(configPath)).toBe(false); }); + + test("a valid journal for different targets is rejected without touching them", () => { + const s = scenario({ config: "PRE_C", store: "PRE_S" }); + const otherConfigPath = join(root(), "other-config.toml"); + writeFileSync(otherConfigPath, "POST_C", "utf8"); + writeFileSync(s.journalPath, encodeJournal({ + ...s.record, + configPath: otherConfigPath, + }), "utf8"); + + const result = recoverIfNeeded(s.journalPath, s.configPath, s.storePath); + expect(result.ok).toBe(false); + expect(read(otherConfigPath)).toBe("POST_C"); + expect(read(s.configPath)).toBe("PRE_C"); + expect(existsSync(s.journalPath)).toBe(true); + }); }); describe("durable write", () => {