From 3e81f6a7d842190708452449834277eb987cebb5 Mon Sep 17 00:00:00 2001 From: luvs01 Date: Sat, 29 Aug 2026 11:56:54 +0900 Subject: [PATCH 01/16] fix(codex): coalesce prompt probes and cancel abandoned probes --- src/codex/prompt-text-probe.ts | 262 +++++++++++++++++-- src/server/management/codex-prompt-routes.ts | 2 +- tests/codex-prompt-route.test.ts | 65 ++++- tests/codex-prompt-text-probe.test.ts | 155 ++++++++++- 4 files changed, 460 insertions(+), 24 deletions(-) diff --git a/src/codex/prompt-text-probe.ts b/src/codex/prompt-text-probe.ts index 6db8d1e49e..b97ede6972 100644 --- a/src/codex/prompt-text-probe.ts +++ b/src/codex/prompt-text-probe.ts @@ -100,41 +100,217 @@ function resolveCodexBinary(): string | null { /** 8 MiB is far above any real prompt and far below anything that hurts the server. */ const MAX_PROBE_OUTPUT_BYTES = 8 * 1024 * 1024; -function runProbe(binary: string, cwd: string, timeoutMs: number): Promise { - return new Promise(resolve => { +interface ProbeCommand { + binary: string; + args: string[]; + cwd: string; + timeoutMs: number; +} + +interface PromptProbeFlight { + key: string; + controller: AbortController; + result: Promise; + closed: Promise; + waiters: number; + joinable: boolean; + resultSettled: boolean; + settled: boolean; +} + +interface PromptProbeExecution { + result: Promise; + closed: Promise; +} + +let activePromptProbe: PromptProbeFlight | null = null; +let probeCommandForTests: { binary: string; args: string[] } | null = null; +let probeSpawnAttemptsForTests = 0; +let probeCloseBarrierForTests: Promise | null = null; + +function commandKey(command: ProbeCommand): string { + return JSON.stringify([command.binary, command.args, command.cwd, command.timeoutMs]); +} + +function completedExecution(value: string | null): PromptProbeExecution { + return { result: Promise.resolve(value), closed: Promise.resolve() }; +} + +function runProbe( + command: ProbeCommand, + signal: AbortSignal, + onStopping: () => void, +): PromptProbeExecution { + if (signal.aborted) return completedExecution(null); + let resolveResult!: (value: string | null) => void; + let resolveClosed!: () => void; + const result = new Promise(resolve => { resolveResult = resolve; }); + const closed = new Promise(resolve => { resolveClosed = resolve; }); + let resultSettled = false; + let closeSettled = false; + + const finishResult = (value: string | null) => { + if (resultSettled) return; + resultSettled = true; + resolveResult(value); + }; + const finishClosed = () => { + if (closeSettled) return; + closeSettled = true; + resolveClosed(); + }; + + try { // A probe must never hang OR balloon the management API: it is bounded in // time AND in bytes, and every failure degrades to "unavailable" rather than // an error page. - const child = spawn(binary, ["debug", "prompt-input"], { - cwd, - stdio: ["ignore", "pipe", "ignore"], - }); + let child: ReturnType; + try { + if (probeCommandForTests) probeSpawnAttemptsForTests += 1; + child = spawn(command.binary, command.args, { + cwd: command.cwd, + stdio: ["ignore", "pipe", "ignore"], + }); + } catch { + finishResult(null); + finishClosed(); + return { result, closed }; + } const chunks: Buffer[] = []; let size = 0; let settled = false; - // One settlement path: a timeout that resolved before `close` used to leave - // the child streaming into a buffer nobody would ever read. - const settle = (value: string | null) => { + let stopping = false; + let timer: ReturnType | undefined; + + const finish = (value: string | null) => { if (settled) return; settled = true; - clearTimeout(timer); + if (timer) clearTimeout(timer); + signal.removeEventListener("abort", onAbort); + finishResult(value); + finishClosed(); + }; + + // Keep the flight admitted until `close`: kill() only requests termination + // and does not prove the exact child has released its process and stdio. + const terminate = () => { + if (settled || stopping) return; + stopping = true; + onStopping(); + if (timer) clearTimeout(timer); + signal.removeEventListener("abort", onAbort); child.stdout?.destroy(); - if (child.exitCode === null && child.signalCode === null) child.kill("SIGKILL"); - resolve(value); + // The caller is bounded even if OS termination later fails. Admission is + // retained separately by `closed`, and later probes fail soft while this + // exact child remains unproven terminal. + finishResult(null); + if (child.exitCode !== null || child.signalCode !== null) return; + try { + child.kill("SIGKILL"); + } catch { + // Exact child state is ambiguous. Keep admission non-joinable until its + // own `close` proves terminal instead of targeting a reusable numeric PID. + } }; - const timer = setTimeout(() => settle(null), timeoutMs); + const onAbort = () => terminate(); + + timer = setTimeout(terminate, command.timeoutMs); + signal.addEventListener("abort", onAbort, { once: true }); child.stdout?.on("data", (chunk: Buffer) => { size += chunk.length; - if (size > MAX_PROBE_OUTPUT_BYTES) { settle(null); return; } + if (size > MAX_PROBE_OUTPUT_BYTES) { terminate(); return; } chunks.push(chunk); }); - child.on("error", () => settle(null)); + child.on("error", () => { + // No PID means spawn itself failed, so there is no live child to drain. + if (child.pid === undefined) { + finish(null); + } + else terminate(); + }); child.on("close", code => { // Decode once, at the end: `String(chunk)` per chunk corrupts any UTF-8 // character that straddles a chunk boundary. - settle(code === 0 ? Buffer.concat(chunks).toString("utf8") : null); + const recordClose = () => { + finish(!stopping && code === 0 ? Buffer.concat(chunks).toString("utf8") : null); + }; + const barrier = probeCloseBarrierForTests; + if (barrier) void barrier.then(recordClose, recordClose); + else recordClose(); }); + // Close the race between the pre-spawn check and listener registration. + if (signal.aborted) terminate(); + } catch { + finishResult(null); + finishClosed(); + } + return { result, closed }; +} + +function startPromptProbeFlight(command: ProbeCommand): PromptProbeFlight { + const controller = new AbortController(); + const flight: PromptProbeFlight = { + key: commandKey(command), + controller, + result: Promise.resolve(null), + closed: Promise.resolve(), + waiters: 0, + joinable: true, + resultSettled: false, + settled: false, + }; + const execution = runProbe(command, controller.signal, () => { + flight.joinable = false; }); + flight.result = execution.result + .catch(() => null) + .finally(() => { + flight.resultSettled = true; + }); + flight.closed = execution.closed + .finally(() => { + flight.settled = true; + if (activePromptProbe === flight) activePromptProbe = null; + }); + activePromptProbe = flight; + return flight; +} + +async function runSharedPromptProbe(command: ProbeCommand, signal?: AbortSignal): Promise { + const key = commandKey(command); + if (signal?.aborted) return null; + const active = activePromptProbe; + if (!active) return waitForPromptProbeFlight(startPromptProbeFlight(command), signal); + if (active.key === key && active.joinable && !active.controller.signal.aborted) { + return waitForPromptProbeFlight(active, signal); + } + // A different or terminating flight still owns the sole process slot. Never + // wait unboundedly for an unproven close and never launch beside it. + return null; +} + +async function waitForPromptProbeFlight(flight: PromptProbeFlight, signal?: AbortSignal): Promise { + if (signal?.aborted) { + if (flight.waiters === 0 && !flight.settled) flight.controller.abort(); + return null; + } + flight.waiters += 1; + let onAbort: (() => void) | undefined; + try { + if (!signal) return await flight.result; + const aborted = new Promise(resolve => { + onAbort = () => resolve(null); + signal.addEventListener("abort", onAbort, { once: true }); + if (signal.aborted) onAbort(); + }); + return await Promise.race([flight.result, aborted]); + } finally { + if (onAbort) signal?.removeEventListener("abort", onAbort); + flight.waiters = Math.max(0, flight.waiters - 1); + if (flight.waiters === 0 && !flight.resultSettled) { + flight.controller.abort(new DOMException("All prompt probe callers cancelled", "AbortError")); + } + } } /** Pull every `...` section out of the rendered developer messages. */ @@ -182,19 +358,33 @@ export const extractSectionsForTests = extractSections; * `cwd` matters: AGENTS.md and environment context are directory-dependent, so a * probe from the wrong place would describe a prompt the user never sees. */ -export async function probePromptText(timeoutMs = 15_000): Promise { +export async function probePromptText(timeoutMs = 15_000, signal?: AbortSignal): Promise { // The probe runs in CODEX_HOME, never in a caller-supplied directory. A `cwd` // parameter let an authenticated request read any readable folder's AGENTS.md, // and it also described a prompt that depends on where Codex happened to run. // The global home is the one context this page can honestly report on. const codexHome = resolveCodexHomeDir(); - const binary = resolveCodexBinary(); + if (signal?.aborted) { + return { ok: false, codexHome, layers: {}, detail: "prompt probe cancelled" }; + } + const binary = probeCommandForTests?.binary ?? resolveCodexBinary(); if (!binary) { return { ok: false, codexHome, layers: {}, detail: "codex binary not found" }; } - const raw = await runProbe(binary, codexHome, timeoutMs); + const command: ProbeCommand = { + binary, + args: probeCommandForTests?.args ?? ["debug", "prompt-input"], + cwd: codexHome, + timeoutMs, + }; + const raw = await runSharedPromptProbe(command, signal); if (raw === null) { - return { ok: false, codexHome, layers: {}, detail: "codex debug prompt-input failed" }; + return { + ok: false, + codexHome, + layers: {}, + detail: signal?.aborted ? "prompt probe cancelled" : "codex debug prompt-input failed", + }; } const sections = extractSections(raw); if (sections.size === 0) { @@ -236,3 +426,35 @@ export async function probePromptText(timeoutMs = 15_000): Promise | null): void { + probeCloseBarrierForTests = barrier; +} + +/** Test-only fail-closed drain so one failed lifecycle case cannot poison another. */ +export async function resetPromptTextProbeForTests(): Promise { + const active = activePromptProbe; + if (active && !active.settled) { + active.controller.abort(new DOMException("Prompt probe test reset", "AbortError")); + const drained = await Promise.race([ + active.closed.then(() => true), + Bun.sleep(2_000).then(() => false), + ]); + if (!drained) throw new Error("prompt probe child did not close during test reset"); + } + if (activePromptProbe === active) activePromptProbe = null; + probeCommandForTests = null; + probeSpawnAttemptsForTests = 0; + probeCloseBarrierForTests = null; +} diff --git a/src/server/management/codex-prompt-routes.ts b/src/server/management/codex-prompt-routes.ts index 82a4b42540..48cf0f84b4 100644 --- a/src/server/management/codex-prompt-routes.ts +++ b/src/server/management/codex-prompt-routes.ts @@ -320,7 +320,7 @@ export async function handleCodexPromptRoutes(ctx: ManagementContext): Promise boolean, detail: string): Promise { + const deadline = Date.now() + 5_000; + while (!predicate()) { + if (Date.now() >= deadline) throw new Error(`timed out waiting for ${detail}`); + await Bun.sleep(10); + } +} + +function isProcessAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + /** * Sentinels must survive every verb. The decoy is installed as CODEX_HOME for the * duration of each request, so this is not a vacuous check: a regression that @@ -117,7 +139,8 @@ async function revision(fx: Fixture): Promise { return res.body.revision as string; } -afterEach(() => { +afterEach(async () => { + await resetPromptTextProbeForTests(); while (roots.length) rmSync(roots.pop()!, { recursive: true, force: true }); }); @@ -799,6 +822,46 @@ describe("020 coverage completions", () => { // Decoding per chunk corrupts UTF-8 that straddles a chunk boundary. expect(probe).toContain("Buffer.concat(chunks).toString(\"utf8\")"); }); + + test("27. the text route forwards live request cancellation to its exact child", async () => { + const fx = fixture(""); + const pidPath = join(fx.decoyHome, "probe-pid.txt"); + setPromptTextProbeCommandForTests({ + binary: process.execPath, + args: ["-e", [ + `require("node:fs").writeFileSync(${JSON.stringify(pidPath)}, String(process.pid));`, + "setInterval(() => {}, 1_000);", + ].join("")], + }); + const controller = new AbortController(); + const url = new URL("http://127.0.0.1:10100/api/codex-prompt/text"); + const req = new Request(url, { + method: "GET", + headers: { host: "127.0.0.1:10100" }, + signal: controller.signal, + }); + const previousHome = process.env.CODEX_HOME; + process.env.CODEX_HOME = fx.decoyHome; + let res: Response | null = null; + try { + const pending = handleManagementAPI(req, url, config, { + codexPromptPaths: { configPath: fx.configPath, storePath: fx.storePath, baseVariantDir: fx.baseVariantDir }, + }, "gui-session"); + await waitUntil(() => existsSync(pidPath), "route probe child pid"); + controller.abort(); + res = await pending; + } finally { + if (previousHome === undefined) delete process.env.CODEX_HOME; + else process.env.CODEX_HOME = previousHome; + } + + expect(res?.status).toBe(200); + expect(await res?.json()).toMatchObject({ ok: false, detail: "prompt probe cancelled" }); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(1); + const pid = Number(readFileSync(pidPath, "utf8")); + await waitUntil(() => !isProcessAlive(pid), "route probe child exit"); + expectDecoyUntouched(fx); + }); test("24. every ownership state is named, not collapsed into a boolean", async () => { // developerInstructionsOwned:false covers an ABSENT key and an EXTERNAL one, and // a GUI that cannot tell them apart hides its own create affordance from every diff --git a/tests/codex-prompt-text-probe.test.ts b/tests/codex-prompt-text-probe.test.ts index 11f453a6c8..164534fd26 100644 --- a/tests/codex-prompt-text-probe.test.ts +++ b/tests/codex-prompt-text-probe.test.ts @@ -6,13 +6,58 @@ * that a missing body is attributed to the right cause, because the dialog shows * that attribution to a user as an explanation. */ -import { describe, expect, test } from "bun:test"; -import { extractSectionsForTests } from "../src/codex/prompt-text-probe"; +import { afterEach, describe, expect, test } from "bun:test"; +import { existsSync, mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { + extractSectionsForTests, + probePromptText, + promptTextProbeSpawnAttemptsForTests, + resetPromptTextProbeForTests, + setPromptTextProbeCloseBarrierForTests, + setPromptTextProbeCommandForTests, +} from "../src/codex/prompt-text-probe"; + +const lifecycleRoots: string[] = []; +const VALID_PROBE_OUTPUT = JSON.stringify([{ + type: "message", + role: "developer", + content: [{ type: "input_text", text: "Skill text." }], +}]); function message(text: string): string { return JSON.stringify([{ type: "message", role: "developer", content: [{ type: "input_text", text }] }]); } +async function waitUntil(predicate: () => boolean, detail: string): Promise { + const deadline = Date.now() + 5_000; + while (!predicate()) { + if (Date.now() >= deadline) throw new Error(`timed out waiting for ${detail}`); + await Bun.sleep(10); + } +} + +function isProcessAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +function root(): string { + const path = mkdtempSync(join(tmpdir(), "ocx-prompt-probe-")); + lifecycleRoots.push(path); + return path; +} + +afterEach(async () => { + await resetPromptTextProbeForTests(); + while (lifecycleRoots.length) rmSync(lifecycleRoots.pop()!, { recursive: true, force: true }); +}); + describe("section extraction", () => { test("a tag name containing a space is still matched", () => { // Codex renders ``, with a space. A [a-z_]+ pattern @@ -68,3 +113,109 @@ describe("section extraction", () => { expect(sections.get("__agents_md")).toContain("
"); }); }); + +describe("prompt probe process lifecycle", () => { + test("a pre-aborted caller starts no child", async () => { + const marker = join(root(), "started.txt"); + setPromptTextProbeCommandForTests({ + binary: process.execPath, + args: ["-e", `require("node:fs").writeFileSync(${JSON.stringify(marker)}, "started")`], + }); + const controller = new AbortController(); + controller.abort(); + + const result = await probePromptText(2_000, controller.signal); + + expect(result.ok).toBe(false); + expect(result.detail).toBe("prompt probe cancelled"); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(0); + expect(existsSync(marker)).toBe(false); + }); + + test("concurrent callers share one child and one caller may cancel", async () => { + const started = join(root(), "started.txt"); + const source = [ + `require("node:fs").appendFileSync(${JSON.stringify(started)}, "1\\n");`, + `setTimeout(() => process.stdout.write(${JSON.stringify(VALID_PROBE_OUTPUT)}), 150);`, + ].join(""); + setPromptTextProbeCommandForTests({ binary: process.execPath, args: ["-e", source] }); + const controller = new AbortController(); + + const first = probePromptText(2_000, controller.signal); + const second = probePromptText(2_000); + controller.abort(); + + expect((await first).detail).toBe("prompt probe cancelled"); + expect((await second).ok).toBe(true); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(1); + expect(readFileSync(started, "utf8").trim().split(/\r?\n/)).toHaveLength(1); + }); + + test("the last cancellation drains the exact child before another command starts", async () => { + const dir = root(); + const pidPath = join(dir, "pid.txt"); + const overlapPath = join(dir, "overlap.txt"); + const hangingSource = [ + `require("node:fs").writeFileSync(${JSON.stringify(pidPath)}, String(process.pid));`, + "setInterval(() => {}, 1_000);", + ].join(""); + setPromptTextProbeCommandForTests({ binary: process.execPath, args: ["-e", hangingSource] }); + const controller = new AbortController(); + const hanging = probePromptText(5_000, controller.signal); + await waitUntil(() => existsSync(pidPath), "hanging child pid"); + const pid = Number(readFileSync(pidPath, "utf8")); + expect(isProcessAlive(pid)).toBe(true); + + controller.abort(); + expect((await hanging).detail).toBe("prompt probe cancelled"); + + const replacementSource = [ + `const fs = require("node:fs"); const pid = Number(fs.readFileSync(${JSON.stringify(pidPath)}, "utf8"));`, + `try { process.kill(pid, 0); fs.writeFileSync(${JSON.stringify(overlapPath)}, "overlap"); } catch {}`, + `process.stdout.write(${JSON.stringify(VALID_PROBE_OUTPUT)});`, + ].join(""); + setPromptTextProbeCommandForTests({ binary: process.execPath, args: ["-e", replacementSource] }); + const blockedDuringDrain = await probePromptText(2_000); + + expect(blockedDuringDrain.ok).toBe(false); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(1); + await resetPromptTextProbeForTests(); + setPromptTextProbeCommandForTests({ binary: process.execPath, args: ["-e", replacementSource] }); + const replacement = await probePromptText(2_000); + + expect(replacement.ok).toBe(true); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(1); + expect(existsSync(overlapPath)).toBe(false); + await waitUntil(() => !isProcessAlive(pid), "cancelled child exit"); + }); + + test("admission stays occupied between child exit and close handling", async () => { + const pidPath = join(root(), "exited-parent-pid.txt"); + let releaseClose!: () => void; + setPromptTextProbeCloseBarrierForTests(new Promise(resolve => { releaseClose = resolve; })); + const delayedCloseSource = [ + `const fs = require("node:fs");`, + `fs.writeFileSync(${JSON.stringify(pidPath)}, String(process.pid));`, + `process.stdout.write(${JSON.stringify(VALID_PROBE_OUTPUT)});`, + ].join(""); + setPromptTextProbeCommandForTests({ binary: process.execPath, args: ["-e", delayedCloseSource] }); + const first = probePromptText(2_000); + await waitUntil(() => existsSync(pidPath), "exit-close parent pid"); + const pid = Number(readFileSync(pidPath, "utf8")); + await waitUntil(() => !isProcessAlive(pid), "probe parent exit"); + + setPromptTextProbeCommandForTests({ + binary: process.execPath, + args: ["-e", `process.stdout.write(${JSON.stringify(VALID_PROBE_OUTPUT)})`], + }); + const blockedBeforeClose = await probePromptText(2_000); + + expect(blockedBeforeClose.ok).toBe(false); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(1); + releaseClose(); + expect((await first).ok).toBe(true); + const afterClose = await probePromptText(2_000); + expect(afterClose.ok).toBe(true); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(2); + }); +}); From a6ccafdc0441ad8d5524746b2ef8922f21105b87 Mon Sep 17 00:00:00 2001 From: luvs01 Date: Sat, 29 Aug 2026 12:01:56 +0900 Subject: [PATCH 02/16] test(codex): make probe liveness branch explicit --- tests/codex-prompt-text-probe.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/codex-prompt-text-probe.test.ts b/tests/codex-prompt-text-probe.test.ts index 164534fd26..b02eebe9db 100644 --- a/tests/codex-prompt-text-probe.test.ts +++ b/tests/codex-prompt-text-probe.test.ts @@ -171,7 +171,9 @@ describe("prompt probe process lifecycle", () => { const replacementSource = [ `const fs = require("node:fs"); const pid = Number(fs.readFileSync(${JSON.stringify(pidPath)}, "utf8"));`, - `try { process.kill(pid, 0); fs.writeFileSync(${JSON.stringify(overlapPath)}, "overlap"); } catch {}`, + "let priorProbeAlive = true;", + "try { process.kill(pid, 0); } catch { priorProbeAlive = false; }", + `if (priorProbeAlive) fs.writeFileSync(${JSON.stringify(overlapPath)}, "overlap");`, `process.stdout.write(${JSON.stringify(VALID_PROBE_OUTPUT)});`, ].join(""); setPromptTextProbeCommandForTests({ binary: process.execPath, args: ["-e", replacementSource] }); From 4ddb4a18e0c9c0fcefe50510ff0f88c162f68795 Mon Sep 17 00:00:00 2001 From: luvs01 Date: Sat, 29 Aug 2026 12:08:54 +0900 Subject: [PATCH 03/16] fix(codex): key prompt probes by revision --- src/codex/prompt-text-probe.ts | 16 ++++++++-- src/server/management/codex-prompt-routes.ts | 6 +++- tests/codex-prompt-route.test.ts | 33 +++++++++++++++++++- 3 files changed, 51 insertions(+), 4 deletions(-) diff --git a/src/codex/prompt-text-probe.ts b/src/codex/prompt-text-probe.ts index b97ede6972..ff71eddb3a 100644 --- a/src/codex/prompt-text-probe.ts +++ b/src/codex/prompt-text-probe.ts @@ -105,6 +105,7 @@ interface ProbeCommand { args: string[]; cwd: string; timeoutMs: number; + promptRevision: string | null; } interface PromptProbeFlight { @@ -129,7 +130,13 @@ let probeSpawnAttemptsForTests = 0; let probeCloseBarrierForTests: Promise | null = null; function commandKey(command: ProbeCommand): string { - return JSON.stringify([command.binary, command.args, command.cwd, command.timeoutMs]); + return JSON.stringify([ + command.binary, + command.args, + command.cwd, + command.timeoutMs, + command.promptRevision, + ]); } function completedExecution(value: string | null): PromptProbeExecution { @@ -358,7 +365,11 @@ export const extractSectionsForTests = extractSections; * `cwd` matters: AGENTS.md and environment context are directory-dependent, so a * probe from the wrong place would describe a prompt the user never sees. */ -export async function probePromptText(timeoutMs = 15_000, signal?: AbortSignal): Promise { +export async function probePromptText( + timeoutMs = 15_000, + signal?: AbortSignal, + promptRevision: string | null = null, +): Promise { // The probe runs in CODEX_HOME, never in a caller-supplied directory. A `cwd` // parameter let an authenticated request read any readable folder's AGENTS.md, // and it also described a prompt that depends on where Codex happened to run. @@ -376,6 +387,7 @@ export async function probePromptText(timeoutMs = 15_000, signal?: AbortSignal): args: probeCommandForTests?.args ?? ["debug", "prompt-input"], cwd: codexHome, timeoutMs, + promptRevision, }; const raw = await runSharedPromptProbe(command, signal); if (raw === null) { diff --git a/src/server/management/codex-prompt-routes.ts b/src/server/management/codex-prompt-routes.ts index 48cf0f84b4..3c8c8614e6 100644 --- a/src/server/management/codex-prompt-routes.ts +++ b/src/server/management/codex-prompt-routes.ts @@ -320,7 +320,11 @@ export async function handleCodexPromptRoutes(ctx: ManagementContext): Promise { const probe = await Bun.file(new URL("../src/codex/prompt-text-probe.ts", import.meta.url)).text(); // The probe resolves CODEX_HOME itself; it must not accept a directory. expect(probe).toContain("resolveCodexHomeDir()"); - expect(probe).toMatch(/export async function probePromptText\(timeoutMs/); + expect(probe).toMatch(/export async function probePromptText\(\s*timeoutMs/); }); test("26. the probe is bounded in bytes as well as in time", async () => { @@ -862,6 +862,37 @@ describe("020 coverage completions", () => { await waitUntil(() => !isProcessAlive(pid), "route probe child exit"); expectDecoyUntouched(fx); }); + + test("28. a post-write text read never joins a pre-write probe", async () => { + const fx = fixture("include_apps_instructions = false\n"); + const startedPath = join(fx.decoyHome, "revision-probe-started.txt"); + const probeOutput = JSON.stringify([{ + type: "message", + role: "developer", + content: [{ type: "input_text", text: "Skill text." }], + }]); + setPromptTextProbeCommandForTests({ + binary: process.execPath, + args: ["-e", [ + `require("node:fs").writeFileSync(${JSON.stringify(startedPath)}, "started");`, + `setTimeout(() => process.stdout.write(${JSON.stringify(probeOutput)}), 200);`, + ].join("")], + }); + + const beforeWrite = call("GET", "/api/codex-prompt/text", fx); + await waitUntil(() => existsSync(startedPath), "pre-write probe start"); + writeFileSync(fx.configPath, "include_apps_instructions = true\n", "utf8"); + + const afterWrite = await call("GET", "/api/codex-prompt/text", fx); + expect(afterWrite.body).toMatchObject({ ok: false, detail: "codex debug prompt-input failed" }); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(1); + expect((await beforeWrite).body.ok).toBe(true); + + const fresh = await call("GET", "/api/codex-prompt/text", fx); + expect(fresh.body.ok).toBe(true); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(2); + }); + test("24. every ownership state is named, not collapsed into a boolean", async () => { // developerInstructionsOwned:false covers an ABSENT key and an EXTERNAL one, and // a GUI that cannot tell them apart hides its own create affordance from every From 0e3c7f1ad1a1696defd480071c365deea3cd22f5 Mon Sep 17 00:00:00 2001 From: luvs01 Date: Sat, 29 Aug 2026 12:14:29 +0900 Subject: [PATCH 04/16] fix(codex): distinguish busy prompt probes --- src/codex/prompt-text-probe.ts | 33 ++++++++++++++++++++------- tests/codex-prompt-route.test.ts | 5 +++- tests/codex-prompt-text-probe.test.ts | 2 ++ 3 files changed, 31 insertions(+), 9 deletions(-) diff --git a/src/codex/prompt-text-probe.ts b/src/codex/prompt-text-probe.ts index ff71eddb3a..6282050b7a 100644 --- a/src/codex/prompt-text-probe.ts +++ b/src/codex/prompt-text-probe.ts @@ -124,6 +124,11 @@ interface PromptProbeExecution { closed: Promise; } +type SharedPromptProbeOutcome = + | { kind: "output"; raw: string } + | { kind: "failed" } + | { kind: "busy" }; + let activePromptProbe: PromptProbeFlight | null = null; let probeCommandForTests: { binary: string; args: string[] } | null = null; let probeSpawnAttemptsForTests = 0; @@ -283,17 +288,24 @@ function startPromptProbeFlight(command: ProbeCommand): PromptProbeFlight { return flight; } -async function runSharedPromptProbe(command: ProbeCommand, signal?: AbortSignal): Promise { +async function runSharedPromptProbe( + command: ProbeCommand, + signal?: AbortSignal, +): Promise { const key = commandKey(command); - if (signal?.aborted) return null; + if (signal?.aborted) return { kind: "failed" }; const active = activePromptProbe; - if (!active) return waitForPromptProbeFlight(startPromptProbeFlight(command), signal); + if (!active) { + const raw = await waitForPromptProbeFlight(startPromptProbeFlight(command), signal); + return raw === null ? { kind: "failed" } : { kind: "output", raw }; + } if (active.key === key && active.joinable && !active.controller.signal.aborted) { - return waitForPromptProbeFlight(active, signal); + const raw = await waitForPromptProbeFlight(active, signal); + return raw === null ? { kind: "failed" } : { kind: "output", raw }; } // A different or terminating flight still owns the sole process slot. Never // wait unboundedly for an unproven close and never launch beside it. - return null; + return { kind: "busy" }; } async function waitForPromptProbeFlight(flight: PromptProbeFlight, signal?: AbortSignal): Promise { @@ -389,15 +401,20 @@ export async function probePromptText( timeoutMs, promptRevision, }; - const raw = await runSharedPromptProbe(command, signal); - if (raw === null) { + const outcome = await runSharedPromptProbe(command, signal); + if (outcome.kind !== "output") { return { ok: false, codexHome, layers: {}, - detail: signal?.aborted ? "prompt probe cancelled" : "codex debug prompt-input failed", + detail: signal?.aborted + ? "prompt probe cancelled" + : outcome.kind === "busy" + ? "another prompt probe is still finishing; retry shortly" + : "codex debug prompt-input failed", }; } + const raw = outcome.raw; const sections = extractSections(raw); if (sections.size === 0) { // Zero sections from a zero-exit probe means the output did not parse, which diff --git a/tests/codex-prompt-route.test.ts b/tests/codex-prompt-route.test.ts index 737f4ab298..75a2e1ecc2 100644 --- a/tests/codex-prompt-route.test.ts +++ b/tests/codex-prompt-route.test.ts @@ -884,7 +884,10 @@ describe("020 coverage completions", () => { writeFileSync(fx.configPath, "include_apps_instructions = true\n", "utf8"); const afterWrite = await call("GET", "/api/codex-prompt/text", fx); - expect(afterWrite.body).toMatchObject({ ok: false, detail: "codex debug prompt-input failed" }); + expect(afterWrite.body).toMatchObject({ + ok: false, + detail: "another prompt probe is still finishing; retry shortly", + }); expect(promptTextProbeSpawnAttemptsForTests()).toBe(1); expect((await beforeWrite).body.ok).toBe(true); diff --git a/tests/codex-prompt-text-probe.test.ts b/tests/codex-prompt-text-probe.test.ts index b02eebe9db..69c4036e34 100644 --- a/tests/codex-prompt-text-probe.test.ts +++ b/tests/codex-prompt-text-probe.test.ts @@ -180,6 +180,7 @@ describe("prompt probe process lifecycle", () => { const blockedDuringDrain = await probePromptText(2_000); expect(blockedDuringDrain.ok).toBe(false); + expect(blockedDuringDrain.detail).toBe("another prompt probe is still finishing; retry shortly"); expect(promptTextProbeSpawnAttemptsForTests()).toBe(1); await resetPromptTextProbeForTests(); setPromptTextProbeCommandForTests({ binary: process.execPath, args: ["-e", replacementSource] }); @@ -213,6 +214,7 @@ describe("prompt probe process lifecycle", () => { const blockedBeforeClose = await probePromptText(2_000); expect(blockedBeforeClose.ok).toBe(false); + expect(blockedBeforeClose.detail).toBe("another prompt probe is still finishing; retry shortly"); expect(promptTextProbeSpawnAttemptsForTests()).toBe(1); releaseClose(); expect((await first).ok).toBe(true); From 3bd871a1322743b552bb8f4067edbddc7f7c5200 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 12:31:16 +0900 Subject: [PATCH 05/16] fix(codex): complete prompt probe invalidation --- gui/src/pages/codex-set-prompt.tsx | 12 +++--- gui/tests/codex-set-prompt-layers.test.tsx | 21 +++++++++ src/codex/prompt-layers.ts | 25 +++++++++++ src/codex/prompt-text-probe.ts | 8 ++-- src/server/management/codex-prompt-routes.ts | 12 +++--- tests/codex-prompt-route.test.ts | 45 ++++++++++++++++++++ tests/codex-prompt-text-probe.test.ts | 25 +++++++++++ 7 files changed, 133 insertions(+), 15 deletions(-) diff --git a/gui/src/pages/codex-set-prompt.tsx b/gui/src/pages/codex-set-prompt.tsx index 46c6f0fc29..91d9698b37 100644 --- a/gui/src/pages/codex-set-prompt.tsx +++ b/gui/src/pages/codex-set-prompt.tsx @@ -426,26 +426,26 @@ export default function CodexSetPrompt({ apiBase }: { apiBase: string }) { // DECIDE with, and a prompt-budget page that hides which layer costs 15 KB is // asking them to guess. if (layerText !== null) return; - let cancelled = false; + const controller = new AbortController(); void (async () => { try { - const res = await fetch(apiBase + "/api/codex-prompt/text"); + const res = await fetch(apiBase + "/api/codex-prompt/text", { signal: controller.signal }); // Status first. A 500 body still parses as JSON, and `{}` deserialized // into this shape reads as a probe that succeeded and found no layers - // so every row would silently lose its byte count and every dialog would // claim the layer sent nothing. if (!res.ok) { - if (!cancelled) setLayerText({ ok: false }); + if (!controller.signal.aborted) setLayerText({ ok: false }); return; } const body = await res.json() as { ok: boolean; layers?: Record }; - if (!cancelled) setLayerText(body); + if (!controller.signal.aborted) setLayerText(body); } catch { // A failed probe is a missing body, not a broken page. - if (!cancelled) setLayerText({ ok: false }); + if (!controller.signal.aborted) setLayerText({ ok: false }); } })(); - return () => { cancelled = true; }; + return () => { controller.abort(); }; }, [layerText, apiBase]); return ( diff --git a/gui/tests/codex-set-prompt-layers.test.tsx b/gui/tests/codex-set-prompt-layers.test.tsx index b062e2dc0c..3d654b16aa 100644 --- a/gui/tests/codex-set-prompt-layers.test.tsx +++ b/gui/tests/codex-set-prompt-layers.test.tsx @@ -258,6 +258,27 @@ test("9. the dialog names WHY text is missing rather than omitting it silently", await act(async () => { root.unmount(); }); }); +test("the prompt-text request is aborted when the panel unmounts", async () => { + let textSignal: AbortSignal | null = null; + globalThis.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { + if (String(input).endsWith("/api/codex-prompt/text")) { + textSignal = init?.signal as AbortSignal | null ?? null; + return await new Promise((_resolve, reject) => { + textSignal?.addEventListener("abort", () => reject(textSignal?.reason), { once: true }); + }); + } + return json(snapshot()); + }) as typeof fetch; + + const { root } = await mount(); + expect(textSignal).not.toBeNull(); + expect(textSignal!.aborted).toBe(false); + + await act(async () => { root.unmount(); }); + + expect(textSignal!.aborted).toBe(true); +}); + test("4. a runtime-conditional row states the condition that emits it", async () => { stubRoutes(() => json(snapshot())); const { container, root } = await mount(); diff --git a/src/codex/prompt-layers.ts b/src/codex/prompt-layers.ts index 1e00afaa89..7eeaa0668c 100644 --- a/src/codex/prompt-layers.ts +++ b/src/codex/prompt-layers.ts @@ -634,6 +634,31 @@ export function readPromptLayers(opts?: Paths): PromptLayerSnapshot { }; } +/** + * Identity for prompt-text probe admission, deliberately separate from the + * optimistic-concurrency revision above. The revision covers only config/store + * transaction bytes; an edit to the selected base variant changes the prompt + * without changing that transaction contract. + */ +export function computePromptProbeStateFingerprint(opts?: Paths): string { + const configBytes = readFileOrNull(activeConfigPath(opts)); + const storeBytes = readFileOrNull(activeStorePath(opts)); + const variants = readBaseVariants(opts); + const selection = resolveBaseSelection(configBytes, variants, opts); + const hash = createHash("sha256"); + hash.update("revision:"); + hash.update(computeRevision(configBytes, storeBytes)); + hash.update("\nselected-base:"); + hash.update(selection.kind); + if (selection.kind === "variant") { + hash.update(":"); + hash.update(selection.id); + hash.update("\nvariant-bytes:"); + hash.update(readFileOrNull(join(activeBaseVariantDir(opts), `${selection.id}.md`)) ?? "\0absent"); + } + return `sha256:${hash.digest("hex")}`; +} + // --------------------------------------------------------------------------- // Writing // --------------------------------------------------------------------------- diff --git a/src/codex/prompt-text-probe.ts b/src/codex/prompt-text-probe.ts index 6282050b7a..d0926a061e 100644 --- a/src/codex/prompt-text-probe.ts +++ b/src/codex/prompt-text-probe.ts @@ -105,7 +105,7 @@ interface ProbeCommand { args: string[]; cwd: string; timeoutMs: number; - promptRevision: string | null; + promptStateFingerprint: string | null; } interface PromptProbeFlight { @@ -140,7 +140,7 @@ function commandKey(command: ProbeCommand): string { command.args, command.cwd, command.timeoutMs, - command.promptRevision, + command.promptStateFingerprint, ]); } @@ -380,7 +380,7 @@ export const extractSectionsForTests = extractSections; export async function probePromptText( timeoutMs = 15_000, signal?: AbortSignal, - promptRevision: string | null = null, + promptStateFingerprint: string | null = null, ): Promise { // The probe runs in CODEX_HOME, never in a caller-supplied directory. A `cwd` // parameter let an authenticated request read any readable folder's AGENTS.md, @@ -399,7 +399,7 @@ export async function probePromptText( args: probeCommandForTests?.args ?? ["debug", "prompt-input"], cwd: codexHome, timeoutMs, - promptRevision, + promptStateFingerprint, }; const outcome = await runSharedPromptProbe(command, signal); if (outcome.kind !== "output") { diff --git a/src/server/management/codex-prompt-routes.ts b/src/server/management/codex-prompt-routes.ts index 3c8c8614e6..878dc604fe 100644 --- a/src/server/management/codex-prompt-routes.ts +++ b/src/server/management/codex-prompt-routes.ts @@ -28,6 +28,7 @@ import { MAX_BASE_VARIANTS, adoptDeveloperInstructions, composeProjection, + computePromptProbeStateFingerprint, findInvalidCharacter, inspectOwnership, normalizeBody, @@ -320,11 +321,12 @@ export async function handleCodexPromptRoutes(ctx: ManagementContext): Promise { expect(promptTextProbeSpawnAttemptsForTests()).toBe(2); }); + test("29. editing the selected base variant invalidates an in-flight text probe", async () => { + const fx = fixture("model = \"x\"\n"); + const created = await call("PUT", "/api/codex-prompt/base", fx, { + id: null, title: "Old", body: "old-body", revision: await revision(fx), + }); + const id = created.body.snapshot.baseVariants[0].id as string; + await call("PUT", "/api/codex-prompt/base/select", fx, { + kind: "variant", id, revision: await revision(fx), + }); + + const selectedPath = join(fx.baseVariantDir, `${id}.md`); + const startedPath = join(fx.decoyHome, "variant-probe-starts.txt"); + const source = [ + `const fs = require("node:fs");`, + `const prompt = fs.readFileSync(${JSON.stringify(selectedPath)}, "utf8");`, + `fs.appendFileSync(${JSON.stringify(startedPath)}, "1\\n");`, + `const output = JSON.stringify([{type:"message",role:"developer",content:[{type:"input_text",text:"" + prompt + ""}]}]);`, + "setTimeout(() => process.stdout.write(output), 200);", + ].join(""); + setPromptTextProbeCommandForTests({ binary: process.execPath, args: ["-e", source] }); + + const revisionBeforeEdit = await revision(fx); + const beforeEdit = call("GET", "/api/codex-prompt/text", fx); + await waitUntil(() => existsSync(startedPath), "selected-variant probe start"); + + const edited = await call("PUT", "/api/codex-prompt/base", fx, { + id, title: "New", body: "new-body", revision: revisionBeforeEdit, + }); + expect(edited.status).toBe(200); + expect(await revision(fx)).toBe(revisionBeforeEdit); + + const afterEdit = await call("GET", "/api/codex-prompt/text", fx); + expect(afterEdit.body).toMatchObject({ + ok: false, + detail: "another prompt probe is still finishing; retry shortly", + }); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(1); + expect((await beforeEdit).body.layers.skills.text).toBe("# Old\nold-body"); + + const fresh = await call("GET", "/api/codex-prompt/text", fx); + expect(fresh.body.layers.skills.text).toBe("# New\nnew-body"); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(2); + expect(readFileSync(startedPath, "utf8").trim().split(/\r?\n/)).toHaveLength(2); + }); + test("24. every ownership state is named, not collapsed into a boolean", async () => { // developerInstructionsOwned:false covers an ABSENT key and an EXTERNAL one, and // a GUI that cannot tell them apart hides its own create affordance from every diff --git a/tests/codex-prompt-text-probe.test.ts b/tests/codex-prompt-text-probe.test.ts index 69c4036e34..7244802183 100644 --- a/tests/codex-prompt-text-probe.test.ts +++ b/tests/codex-prompt-text-probe.test.ts @@ -151,6 +151,31 @@ describe("prompt probe process lifecycle", () => { expect(readFileSync(started, "utf8").trim().split(/\r?\n/)).toHaveLength(1); }); + test("concurrent callers share one failure and a later caller retries", async () => { + const started = join(root(), "failed-starts.txt"); + const source = [ + `require("node:fs").appendFileSync(${JSON.stringify(started)}, "1\\n");`, + "setTimeout(() => process.exit(1), 150);", + ].join(""); + setPromptTextProbeCommandForTests({ binary: process.execPath, args: ["-e", source] }); + + const [first, second] = await Promise.all([ + probePromptText(2_000), + probePromptText(2_000), + ]); + + expect(first).toMatchObject({ ok: false, detail: "codex debug prompt-input failed" }); + expect(second).toMatchObject({ ok: false, detail: "codex debug prompt-input failed" }); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(1); + expect(readFileSync(started, "utf8").trim().split(/\r?\n/)).toHaveLength(1); + + const later = await probePromptText(2_000); + + expect(later).toMatchObject({ ok: false, detail: "codex debug prompt-input failed" }); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(2); + expect(readFileSync(started, "utf8").trim().split(/\r?\n/)).toHaveLength(2); + }); + test("the last cancellation drains the exact child before another command starts", async () => { const dir = root(); const pidPath = join(dir, "pid.txt"); From a7504ab8e2355526d53f83bc7432ffe3b29847e1 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 12:35:41 +0900 Subject: [PATCH 06/16] fix(gui): keep the post-await state guard alongside the abort react-doctor's no-set-state-after-await-in-effect fired on this effect after the abort controller replaced the cancelled flag: the rule recognizes a boolean guard but does not follow signal.aborted, so the check was silently lost. The gui scan went from 6 warnings on dev to 7 on this branch, which is what failed the job. Both mechanisms now coexist, each doing one job: the controller aborts the request so the server can cancel the probe child it spawned, and the flag guards setState after the await. An abort also lands in the catch, so the flag is what keeps it from writing state into an unmounted panel. Verified: gui scan back to 6 warnings, identical to origin/dev; 16 pass in tests/codex-set-prompt-layers.test.tsx; tsc clean. --- gui/src/pages/codex-set-prompt.tsx | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/gui/src/pages/codex-set-prompt.tsx b/gui/src/pages/codex-set-prompt.tsx index 91d9698b37..2a03e4be08 100644 --- a/gui/src/pages/codex-set-prompt.tsx +++ b/gui/src/pages/codex-set-prompt.tsx @@ -427,6 +427,12 @@ export default function CodexSetPrompt({ apiBase }: { apiBase: string }) { // asking them to guess. if (layerText !== null) return; const controller = new AbortController(); + // Two mechanisms, two jobs. The controller aborts the in-flight request so the server can + // cancel the probe child it spawned for us; the flag is what guards `setState` after an + // await. `signal.aborted` would read the same at runtime, but the lint rule that catches + // post-await state updates does not follow it, and losing that check on this effect is a + // worse trade than carrying one extra variable. + let cancelled = false; void (async () => { try { const res = await fetch(apiBase + "/api/codex-prompt/text", { signal: controller.signal }); @@ -435,17 +441,18 @@ export default function CodexSetPrompt({ apiBase }: { apiBase: string }) { // so every row would silently lose its byte count and every dialog would // claim the layer sent nothing. if (!res.ok) { - if (!controller.signal.aborted) setLayerText({ ok: false }); + if (!cancelled) setLayerText({ ok: false }); return; } const body = await res.json() as { ok: boolean; layers?: Record }; - if (!controller.signal.aborted) setLayerText(body); + if (!cancelled) setLayerText(body); } catch { - // A failed probe is a missing body, not a broken page. - if (!controller.signal.aborted) setLayerText({ ok: false }); + // A failed probe is a missing body, not a broken page. An abort lands here too, and the + // flag is what keeps it from writing state into an unmounted panel. + if (!cancelled) setLayerText({ ok: false }); } })(); - return () => { controller.abort(); }; + return () => { cancelled = true; controller.abort(); }; }, [layerText, apiBase]); return ( From 8794da0ac34cc3994417941aa32bcefaff83148d Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 15:01:47 +0900 Subject: [PATCH 07/16] fix(codex): admit prompt probes on the instruction files they render The probe fingerprint named config.toml, the prompt store and the selected base variant, but not the AGENTS document the probe actually renders. Since the fingerprint is a component of the admission key, an AGENTS.md edit left that key unchanged: the next request matched the in-flight pre-write probe, joined it, and was served pre-write text. That is the same defect the fingerprint was added to fix. The transaction revision covered only config/store bytes, so editing the selected variant moved the prompt without moving the revision. Naming one more uncovered input does not change the shape: admission has to name what it renders. The path comes from resolveCodexHomeDir(), not from activeConfigPath()'s directory. Those differ deliberately in the route fixtures, which inject codexPromptPaths at a temp root while CODEX_HOME points at a decoy. Deriving it from the injected config would name a file the probe never reads, giving a fingerprint that agrees with itself and a test that cannot fail. AGENTS.override.md is hashed ahead of AGENTS.md, matching Codex's own precedence, and an absent file gets its own sentinel so create and delete move the key too. Bounded on purpose, and the comment says so: skill and plugin metadata, MCP availability and the clock also move the rendered prompt and cannot be observed from this process. Fingerprinting a clock would be worse than documenting the gap. Mutation: removing the instruction-file contribution turns all three new cases red (identical keys, one spawn, stale text served) while the other 52 in the file stay green. --- .../130_pr2872_probe_fingerprint.md | 78 +++++++++++++++++ src/codex/prompt-layers.ts | 32 +++++++ tests/codex-prompt-route.test.ts | 87 +++++++++++++++++++ 3 files changed, 197 insertions(+) create mode 100644 devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md diff --git a/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md b/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md new file mode 100644 index 0000000000..d7f8846d41 --- /dev/null +++ b/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md @@ -0,0 +1,78 @@ +# 130 — #2872: the probe admission fingerprint omits the instruction files it renders + +Written after an independent adversarial review of PR #2872 at head `a7504ab8e` +returned BLOCKER FOUND, and after a plan audit of the first fix draft returned +PLAN NEEDS CHANGE. Both verdicts are applied here. + +## Scope + +IN: `src/codex/prompt-layers.ts` (`computePromptProbeStateFingerprint`), +`tests/codex-prompt-route.test.ts` (route-level regression). + +OUT: the coalescing machinery itself (`runSharedPromptProbe`, waiter accounting, +the `busy` fail-closed policy) — reviewed and found sound. Also out: fingerprinting +state this process cannot observe, discussed under "What this does not cover". + +## Defect — a post-write reader joins a pre-write flight and gets stale text + +`computePromptProbeStateFingerprint` (`src/codex/prompt-layers.ts:643`) hashes +`config.toml`, `opencodex-prompt.json` (through `computeRevision`) and the selected +base variant `.md`. It does not hash `$CODEX_HOME/AGENTS.md`. + +`probePromptText` runs the child with `cwd = resolveCodexHomeDir()` +(`src/codex/prompt-text-probe.ts:400`) and extracts that file's body as the +`__agents_md` layer (`:365`). The fingerprint is a component of `commandKey()` +(`:137-145`), which is the sole admission identity in `runSharedPromptProbe` +(`:302`). So an `AGENTS.md` edit leaves the key unchanged, the next request matches +`active.key`, joins the in-flight pre-write probe, and is served pre-write text. + +Reproduced deterministically at `a7504ab8e`: identical fingerprints before and +after the write, and both callers received `"old-agent-text"`. + +This is the same class of bug the fingerprint was introduced to fix. The original +`revision` covered only config/store transaction bytes, so editing the selected base +variant changed the prompt without moving the revision. Naming one more uncovered +input does not change the shape of the defect: admission identity must name every +input the probe renders. + +## Fix + +Hash the `CODEX_HOME` instruction files into the fingerprint. + +The path is `resolveCodexHomeDir()`, **not** `dirname(activeConfigPath(opts))`. The +plan audit rejected the latter and it is right: `tests/codex-prompt-route.test.ts:115-125` +injects `codexPromptPaths` at a fixture root while setting `CODEX_HOME` to a separate +decoy, precisely so a route that ignored the injected paths is caught. Deriving the +`AGENTS.md` path from `configPath` would name a file the probe never reads, and the +regression would pass while production stayed broken. + +Both spellings are hashed, in Codex's own precedence order: `AGENTS.override.md` +is preferred over `AGENTS.md`, so an override edit must move the key too. Absent +files hash to a distinct sentinel, so create and delete both move the key. + +## What this does not cover, stated rather than implied + +The guarantee is bounded to OpenCodex-managed writes plus the `CODEX_HOME` +instruction files. It is not complete prompt-state identity, and the code says so +instead of implying otherwise: + +- Skill metadata, plugin manifests, and MCP/app availability feed + ``, `` and ``. +- Clock, timezone, shell and permission state feed ``. +- An external `model_instructions_file` target. `codex debug prompt-input` discards + base instructions, so it does not currently stale a rendered layer. + +None is writable through `/api/codex-prompt`; each needs an external edit +concurrent with an in-flight probe. A 15-second window bounded by a fail-closed +`busy` is the exposure, and pretending to fingerprint a clock would be worse than +documenting it. + +## Verification + +Route-level, in the file whose fixture separates `CODEX_HOME` from the injected +paths — the only place this can fail honestly. Two callers separated by an +`AGENTS.md` write must not share a flight: the second returns `busy`, and a later +request returns the new text. Repeated for `AGENTS.override.md`. + +Named mutation: delete the instruction-file contribution from the fingerprint. The +regression must go red with identical keys and one spawn. diff --git a/src/codex/prompt-layers.ts b/src/codex/prompt-layers.ts index 7eeaa0668c..7d2211c94b 100644 --- a/src/codex/prompt-layers.ts +++ b/src/codex/prompt-layers.ts @@ -31,6 +31,7 @@ import { dirname, join, resolve } from "node:path"; import { createHash, randomBytes } from "node:crypto"; import { expandUserPath } from "../config"; import { CODEX_CONFIG_PATH } from "./paths"; +import { resolveCodexHomeDir } from "./home"; import { OCX_SECTION_MARKER } from "./injected-marker"; import { durableWrite, @@ -181,6 +182,14 @@ export function activeBaseVariantDir(opts?: Paths): string { return opts?.baseVariantDir ?? join(activeCodexHome(), "opencodex-prompt-base"); } +/** + * Instruction documents the prompt probe renders out of CODEX_HOME, in the + * precedence order Codex itself applies: an `AGENTS.override.md` shadows + * `AGENTS.md`. Both are hashed into the probe fingerprint, because either one + * changes the rendered project document without touching a managed file. + */ +const PROBE_INSTRUCTION_FILES = ["AGENTS.override.md", "AGENTS.md"] as const; + function journalPathFor(storePath: string): string { return `${storePath.replace(/\.json$/, "")}.journal`; } @@ -639,6 +648,20 @@ export function readPromptLayers(opts?: Paths): PromptLayerSnapshot { * optimistic-concurrency revision above. The revision covers only config/store * transaction bytes; an edit to the selected base variant changes the prompt * without changing that transaction contract. + * + * The instruction documents in CODEX_HOME are hashed for the same reason, and they + * are read from `resolveCodexHomeDir()` rather than from `activeConfigPath`'s + * directory. Those two are deliberately different under test — the route fixtures + * inject `codexPromptPaths` at a temp root while CODEX_HOME points at a decoy — and + * the probe renders whatever lives in the home it actually runs in. Deriving the + * path from the injected config would name a file the probe never reads, which is + * a fingerprint that cannot fail rather than evidence. + * + * Bounded on purpose: this covers opencodex-managed writes plus the CODEX_HOME + * instruction files. Skill and plugin metadata, MCP availability, and the clock also + * move the rendered prompt and cannot be observed from this process; an external + * edit to one of those, concurrent with an in-flight probe, is still coalescible. + * See devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md. */ export function computePromptProbeStateFingerprint(opts?: Paths): string { const configBytes = readFileOrNull(activeConfigPath(opts)); @@ -656,6 +679,15 @@ export function computePromptProbeStateFingerprint(opts?: Paths): string { hash.update("\nvariant-bytes:"); hash.update(readFileOrNull(join(activeBaseVariantDir(opts), `${selection.id}.md`)) ?? "\0absent"); } + // Codex prefers AGENTS.override.md over AGENTS.md, so both spellings are hashed + // in that order: an override edit changes the rendered project document exactly + // as a plain edit does. A missing file hashes to its own sentinel so that + // creating or deleting one moves the key too. + const probeHome = resolveCodexHomeDir(); + for (const name of PROBE_INSTRUCTION_FILES) { + hash.update(`\n${name}:`); + hash.update(readFileOrNull(join(probeHome, name)) ?? "\0absent"); + } return `sha256:${hash.digest("hex")}`; } diff --git a/tests/codex-prompt-route.test.ts b/tests/codex-prompt-route.test.ts index c054ce67d2..1ea8e4ff85 100644 --- a/tests/codex-prompt-route.test.ts +++ b/tests/codex-prompt-route.test.ts @@ -941,6 +941,93 @@ describe("020 coverage completions", () => { expect(readFileSync(startedPath, "utf8").trim().split(/\r?\n/)).toHaveLength(2); }); + /** + * The probe renders AGENTS.md out of the home it runs in, so admission has to + * name that file. It is asserted here rather than in the probe unit test because + * this harness is the only one where CODEX_HOME and the injected + * `codexPromptPaths` are deliberately different directories: a fingerprint that + * derived the path from the injected config would agree with itself and pass, + * while production kept serving pre-write text. + * + * The stale value is asserted, not merely a differing key — the failure this + * covers is a caller receiving another caller's older AGENTS text. + */ + for (const instructionFile of ["AGENTS.md", "AGENTS.override.md"]) { + test(`30. editing ${instructionFile} invalidates an in-flight text probe`, async () => { + const fx = fixture("model = \"x\"\n"); + const agentsPath = join(fx.decoyHome, instructionFile); + writeFileSync(agentsPath, "old-agent-text", "utf8"); + const startedPath = join(fx.decoyHome, "agents-probe-starts.txt"); + const source = [ + `const fs = require("node:fs");`, + `const doc = fs.readFileSync(${JSON.stringify(agentsPath)}, "utf8");`, + `fs.appendFileSync(${JSON.stringify(startedPath)}, "1\\n");`, + `const output = JSON.stringify([{type:"message",role:"developer",content:[{type:"input_text",text:"" + doc + ""}]}]);`, + "setTimeout(() => process.stdout.write(output), 200);", + ].join(""); + setPromptTextProbeCommandForTests({ binary: process.execPath, args: ["-e", source] }); + + const beforeEdit = call("GET", "/api/codex-prompt/text", fx); + await waitUntil(() => existsSync(startedPath), `${instructionFile} probe start`); + + // Nothing opencodex owns has changed: no config write, no store write, so + // the transaction revision and the selected base are identical here. + writeFileSync(agentsPath, "new-agent-text", "utf8"); + + const afterEdit = await call("GET", "/api/codex-prompt/text", fx); + expect(afterEdit.body).toMatchObject({ + ok: false, + detail: "another prompt probe is still finishing; retry shortly", + }); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(1); + expect((await beforeEdit).body.layers.skills.text).toBe("old-agent-text"); + + const fresh = await call("GET", "/api/codex-prompt/text", fx); + expect(fresh.body.layers.skills.text).toBe("new-agent-text"); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(2); + expect(readFileSync(startedPath, "utf8").trim().split(/\r?\n/)).toHaveLength(2); + }); + } + + test("31. creating and deleting an instruction file both move probe admission", async () => { + const fx = fixture("model = \"x\"\n"); + const agentsPath = join(fx.decoyHome, "AGENTS.md"); + const startedPath = join(fx.decoyHome, "absent-probe-starts.txt"); + const source = [ + `const fs = require("node:fs");`, + `let doc = "\\u0000absent";`, + `try { doc = fs.readFileSync(${JSON.stringify(agentsPath)}, "utf8"); } catch {}`, + `fs.appendFileSync(${JSON.stringify(startedPath)}, "1\\n");`, + `const output = JSON.stringify([{type:"message",role:"developer",content:[{type:"input_text",text:"" + doc + ""}]}]);`, + "setTimeout(() => process.stdout.write(output), 200);", + ].join(""); + setPromptTextProbeCommandForTests({ binary: process.execPath, args: ["-e", source] }); + + // absent -> present must move the key, so a probe started with no AGENTS.md + // cannot be joined once one exists. + const beforeCreate = call("GET", "/api/codex-prompt/text", fx); + await waitUntil(() => existsSync(startedPath), "absent-state probe start"); + writeFileSync(agentsPath, "created-text", "utf8"); + expect((await call("GET", "/api/codex-prompt/text", fx)).body).toMatchObject({ + ok: false, + detail: "another prompt probe is still finishing; retry shortly", + }); + expect((await beforeCreate).body.layers.skills.text).toBe("\u0000absent"); + + const present = await call("GET", "/api/codex-prompt/text", fx); + expect(present.body.layers.skills.text).toBe("created-text"); + + // present -> absent is the same requirement in reverse. + const beforeDelete = call("GET", "/api/codex-prompt/text", fx); + await waitUntil(() => readFileSync(startedPath, "utf8").trim().split(/\r?\n/).length === 3, "present-state probe start"); + rmSync(agentsPath); + expect((await call("GET", "/api/codex-prompt/text", fx)).body).toMatchObject({ + ok: false, + detail: "another prompt probe is still finishing; retry shortly", + }); + expect((await beforeDelete).body.layers.skills.text).toBe("created-text"); + }); + test("24. every ownership state is named, not collapsed into a boolean", async () => { // developerInstructionsOwned:false covers an ABSENT key and an EXTERNAL one, and // a GUI that cannot tell them apart hides its own create affordance from every From 6b43a16444027002a984c8ee3e4955ead878fca7 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 15:04:04 +0900 Subject: [PATCH 08/16] test(codex): assert instruction-file absence instead of catching it The absent-state probe swallowed the read error to represent a missing AGENTS.md. That trips the empty_catch hygiene gate, and the gate is right: the same catch would have reported a genuinely unreadable file as absent, which is the one distinction this case exists to make. existsSync decides it explicitly. --- tests/codex-prompt-route.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/codex-prompt-route.test.ts b/tests/codex-prompt-route.test.ts index 1ea8e4ff85..108a8ead32 100644 --- a/tests/codex-prompt-route.test.ts +++ b/tests/codex-prompt-route.test.ts @@ -995,8 +995,10 @@ describe("020 coverage completions", () => { const startedPath = join(fx.decoyHome, "absent-probe-starts.txt"); const source = [ `const fs = require("node:fs");`, - `let doc = "\\u0000absent";`, - `try { doc = fs.readFileSync(${JSON.stringify(agentsPath)}, "utf8"); } catch {}`, + // Absence is a state this case asserts on, so it is tested for rather than + // caught: an empty catch here would also swallow a genuinely unreadable file + // and report it as absent. + `const doc = fs.existsSync(${JSON.stringify(agentsPath)}) ? fs.readFileSync(${JSON.stringify(agentsPath)}, "utf8") : "\\u0000absent";`, `fs.appendFileSync(${JSON.stringify(startedPath)}, "1\\n");`, `const output = JSON.stringify([{type:"message",role:"developer",content:[{type:"input_text",text:"" + doc + ""}]}]);`, "setTimeout(() => process.stdout.write(output), 200);", From 57e66ee64f800e0cea3c10efb5723fae7e77da6d Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 15:17:04 +0900 Subject: [PATCH 09/16] fix(codex): frame each probe fingerprint field by length A second adversarial review found the first version of this hash was ambiguous. Concatenating name + ":" + contents lets a file's own bytes imitate the separator that follows it: {override: "left", agents: "right\\nAGENTS.md:tail"} {override: "left\\nAGENTS.md:right", agents: "tail"} Both hashed to the same digest. Since the fingerprint IS the probe's admission key, two different prompt states sharing a digest is exactly the missed invalidation this work set out to remove: the second caller joins the first one's flight and reads its text. A byte length cannot be forged by content, so every field carries one, and absence is length -1 rather than a sentinel string. The sentinel had the same class of flaw one level down: "\\0absent" collided with a file whose bytes were literally NUL + "absent". The pre-existing revision and variant-bytes fields go through the same framing. They shared the defect and there is no reason to keep two spellings of the same decision in one function. Mutations, each observed red: unframed concatenation + string sentinel 2 fail absent measured as zero bytes 1 fail AGENTS.override.md dropped 1 fail fingerprint reads the injected config dir 7 fail 59 pass / 0 fail in tests/codex-prompt-route.test.ts, tsc clean. --- src/codex/prompt-layers.ts | 43 ++++++++++----- tests/codex-prompt-route.test.ts | 93 ++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 13 deletions(-) diff --git a/src/codex/prompt-layers.ts b/src/codex/prompt-layers.ts index 7d2211c94b..9bce2f8e62 100644 --- a/src/codex/prompt-layers.ts +++ b/src/codex/prompt-layers.ts @@ -28,7 +28,7 @@ */ import { existsSync, readFileSync, readdirSync, realpathSync } from "node:fs"; import { dirname, join, resolve } from "node:path"; -import { createHash, randomBytes } from "node:crypto"; +import { createHash, randomBytes, type Hash } from "node:crypto"; import { expandUserPath } from "../config"; import { CODEX_CONFIG_PATH } from "./paths"; import { resolveCodexHomeDir } from "./home"; @@ -190,6 +190,30 @@ export function activeBaseVariantDir(opts?: Paths): string { */ const PROBE_INSTRUCTION_FILES = ["AGENTS.override.md", "AGENTS.md"] as const; +/** + * Feed one named field into a fingerprint, framed so that no two distinct states + * can produce the same digest. + * + * Framing is the whole point. Concatenating `name + ":" + contents` is ambiguous: + * an adversarial review of the first version of this function showed that + * `{override: "left", agents: "right\nAGENTS.md:tail"}` and + * `{override: "left\nAGENTS.md:right", agents: "tail"}` hashed identically, because + * a file's own bytes can imitate the separator that follows it. That is exactly a + * missed invalidation: the fingerprint is the probe's admission key, so two + * different prompt states sharing a digest means one caller is served the other's + * stale text. + * + * A byte length cannot be forged by content, so each field carries one. Absence is + * a length of -1 rather than a sentinel string, because a sentinel is just more + * content: the same review found that `null` collided with a file whose bytes were + * literally NUL + "absent". + */ +function updateFingerprintField(hash: Hash, name: string, contents: string | null): void { + const bytes = contents === null ? -1 : Buffer.byteLength(contents, "utf8"); + hash.update(`\n${name}:${bytes}:`); + if (contents !== null) hash.update(contents); +} + function journalPathFor(storePath: string): string { return `${storePath.replace(/\.json$/, "")}.journal`; } @@ -669,24 +693,17 @@ export function computePromptProbeStateFingerprint(opts?: Paths): string { const variants = readBaseVariants(opts); const selection = resolveBaseSelection(configBytes, variants, opts); const hash = createHash("sha256"); - hash.update("revision:"); - hash.update(computeRevision(configBytes, storeBytes)); - hash.update("\nselected-base:"); - hash.update(selection.kind); + updateFingerprintField(hash, "revision", computeRevision(configBytes, storeBytes)); + updateFingerprintField(hash, "selected-base", selection.kind === "variant" ? `variant:${selection.id}` : selection.kind); if (selection.kind === "variant") { - hash.update(":"); - hash.update(selection.id); - hash.update("\nvariant-bytes:"); - hash.update(readFileOrNull(join(activeBaseVariantDir(opts), `${selection.id}.md`)) ?? "\0absent"); + updateFingerprintField(hash, "variant-bytes", readFileOrNull(join(activeBaseVariantDir(opts), `${selection.id}.md`))); } // Codex prefers AGENTS.override.md over AGENTS.md, so both spellings are hashed // in that order: an override edit changes the rendered project document exactly - // as a plain edit does. A missing file hashes to its own sentinel so that - // creating or deleting one moves the key too. + // as a plain edit does. const probeHome = resolveCodexHomeDir(); for (const name of PROBE_INSTRUCTION_FILES) { - hash.update(`\n${name}:`); - hash.update(readFileOrNull(join(probeHome, name)) ?? "\0absent"); + updateFingerprintField(hash, name, readFileOrNull(join(probeHome, name))); } return `sha256:${hash.digest("hex")}`; } diff --git a/tests/codex-prompt-route.test.ts b/tests/codex-prompt-route.test.ts index 108a8ead32..ca2d9c2ff5 100644 --- a/tests/codex-prompt-route.test.ts +++ b/tests/codex-prompt-route.test.ts @@ -1030,6 +1030,99 @@ describe("020 coverage completions", () => { expect((await beforeDelete).body.layers.skills.text).toBe("created-text"); }); + /** + * Absence and emptiness are different prompt states, and a sentinel STRING cannot + * tell them apart: an adversarial review showed the null case colliding with a file + * whose bytes were literally NUL + "absent", so deleting such a file left the + * admission key unmoved. The framing carries a byte length instead, and -1 is not a + * length any content can produce. + * + * Two single-transition cases rather than one chained walk: each in-flight probe is + * observed by its own marker file, so a request that is correctly refused as `busy` + * cannot be mistaken for a probe that never started. + */ + for (const transition of [ + { name: "deleting a file whose content is the old absent sentinel", before: "\u0000absent", after: null }, + { name: "emptying a file", before: "had-content", after: "" }, + // The one transition where absent and empty are the ONLY difference. A + // fingerprint that measured a missing file as zero bytes would hash these two + // states identically and hand the second caller the first one's text. + { name: "deleting an already-empty file", before: "", after: null }, + ]) { + test(`32. ${transition.name} moves probe admission`, async () => { + const fx = fixture("model = \"x\"\n"); + const agentsPath = join(fx.decoyHome, "AGENTS.md"); + writeFileSync(agentsPath, transition.before, "utf8"); + const startedPath = join(fx.decoyHome, "transition-probe-starts.txt"); + const source = [ + `const fs = require("node:fs");`, + `const p = ${JSON.stringify(agentsPath)};`, + `const doc = fs.existsSync(p) ? "present:" + fs.readFileSync(p, "utf8") : "missing";`, + `fs.appendFileSync(${JSON.stringify(startedPath)}, "1\\n");`, + `const output = JSON.stringify([{type:"message",role:"developer",content:[{type:"input_text",text:"" + doc + ""}]}]);`, + "setTimeout(() => process.stdout.write(output), 200);", + ].join(""); + setPromptTextProbeCommandForTests({ binary: process.execPath, args: ["-e", source] }); + + const beforeTransition = call("GET", "/api/codex-prompt/text", fx); + await waitUntil(() => existsSync(startedPath), "transition probe start"); + if (transition.after === null) rmSync(agentsPath); + else writeFileSync(agentsPath, transition.after, "utf8"); + + expect((await call("GET", "/api/codex-prompt/text", fx)).body).toMatchObject({ + ok: false, + detail: "another prompt probe is still finishing; retry shortly", + }); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(1); + expect((await beforeTransition).body.layers.skills.text).toBe(`present:${transition.before}`); + + const fresh = await call("GET", "/api/codex-prompt/text", fx); + expect(fresh.body.layers.skills.text).toBe(transition.after === null ? "missing" : `present:${transition.after}`); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(2); + }); + } + /** + * A file's own bytes must not be able to imitate the separator that frames the + * next field. Without a length prefix these two states hash identically, and the + * second request joins the first probe and is served its text. + */ + test("33. instruction-file content cannot imitate a fingerprint field boundary", async () => { + const fx = fixture("model = \"x\"\n"); + const overridePath = join(fx.decoyHome, "AGENTS.override.md"); + const agentsPath = join(fx.decoyHome, "AGENTS.md"); + writeFileSync(overridePath, "left", "utf8"); + writeFileSync(agentsPath, "right\nAGENTS.md:tail", "utf8"); + const startedPath = join(fx.decoyHome, "framing-probe-starts.txt"); + const source = [ + `const fs = require("node:fs");`, + `const read = p => fs.existsSync(p) ? fs.readFileSync(p, "utf8") : "\\u0000missing";`, + `const doc = read(${JSON.stringify(overridePath)}) + "|" + read(${JSON.stringify(agentsPath)});`, + `fs.appendFileSync(${JSON.stringify(startedPath)}, "1\\n");`, + `const output = JSON.stringify([{type:"message",role:"developer",content:[{type:"input_text",text:"" + doc + ""}]}]);`, + "setTimeout(() => process.stdout.write(output), 200);", + ].join(""); + setPromptTextProbeCommandForTests({ binary: process.execPath, args: ["-e", source] }); + + const beforeShift = call("GET", "/api/codex-prompt/text", fx); + await waitUntil(() => existsSync(startedPath), "framing probe start"); + + // Move the boundary: the concatenation of (name, contents) is byte-identical + // across this edit, so only a length-framed field distinguishes the two states. + writeFileSync(overridePath, "left\nAGENTS.md:right", "utf8"); + writeFileSync(agentsPath, "tail", "utf8"); + + expect((await call("GET", "/api/codex-prompt/text", fx)).body).toMatchObject({ + ok: false, + detail: "another prompt probe is still finishing; retry shortly", + }); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(1); + expect((await beforeShift).body.layers.skills.text).toBe("left|right\nAGENTS.md:tail"); + + const fresh = await call("GET", "/api/codex-prompt/text", fx); + expect(fresh.body.layers.skills.text).toBe("left\nAGENTS.md:right|tail"); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(2); + }); + test("24. every ownership state is named, not collapsed into a boolean", async () => { // developerInstructionsOwned:false covers an ABSENT key and an EXTERNAL one, and // a GUI that cannot tell them apart hides its own create affordance from every From 9768e70f5f8b9f44b692bed864ab5cfd14439d35 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 15:29:43 +0900 Subject: [PATCH 10/16] fix(codex): frame the revision's own two fields by length A third review found the same ambiguity one level down. The outer helper length-framed the revision digest, but computeRevision still built that digest by concatenating config and store around a bare \nstore: separator with a string absence sentinel, and framing a digest cannot recover a boundary already lost inside it. Proven collisions: cfg=left, store=right\nstore:tail == cfg=left\nstore:right, store=tail cfg absent == cfg containing NUL+absent This one reaches further than the probe. The revision is also the optimistic-concurrency token compared in commit(), so a collision means a write built on bytes that have since changed can be accepted as current. Both fields now go through updateFingerprintField. Mutation: restoring the unframed concatenation turns both new assertions red (43 pass / 2 fail). 116 pass / 0 fail across the prompt layers, route, and probe suites; tsc clean. --- src/codex/prompt-layers.ts | 11 +++++++---- tests/codex-prompt-layers.test.ts | 16 ++++++++++++++++ 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/src/codex/prompt-layers.ts b/src/codex/prompt-layers.ts index 9bce2f8e62..89e572b166 100644 --- a/src/codex/prompt-layers.ts +++ b/src/codex/prompt-layers.ts @@ -319,10 +319,13 @@ function readFileOrNull(path: string): string | null { export function computeRevision(configBytes: string | null, storeBytes: string | null): string { const hash = createHash("sha256"); - hash.update("cfg:"); - hash.update(configBytes ?? "\0absent"); - hash.update("\nstore:"); - hash.update(storeBytes ?? "\0absent"); + // Length-framed for the reason given on updateFingerprintField: with a bare + // separator, config bytes ending in "\nstore:" shift the boundary and two + // different pairs hash alike. That matters twice over — this value is both the + // probe's admission input and the optimistic-concurrency token compared in + // commit(), where a collision would let a write built on stale bytes through. + updateFingerprintField(hash, "cfg", configBytes); + updateFingerprintField(hash, "store", storeBytes); return `sha256:${hash.digest("hex")}`; } diff --git a/tests/codex-prompt-layers.test.ts b/tests/codex-prompt-layers.test.ts index 686f75adbb..64e715ab26 100644 --- a/tests/codex-prompt-layers.test.ts +++ b/tests/codex-prompt-layers.test.ts @@ -189,4 +189,20 @@ describe("revision", () => { test("is stable for identical bytes", () => { expect(computeRevision("a", "{}")).toBe(computeRevision("a", "{}")); }); + + /** + * The revision is compared in commit() to decide whether a write may proceed, and + * it feeds the prompt probe's admission key. A collision is therefore both a + * stale-write and a stale-read defect, so the boundary between the two files has + * to be unforgeable by their contents. + */ + test("config bytes cannot imitate the store field boundary", () => { + expect(computeRevision("left", "right\nstore:tail")) + .not.toBe(computeRevision("left\nstore:right", "tail")); + }); + + test("an absent file is not a file containing the old absence sentinel", () => { + expect(computeRevision(null, "{}")).not.toBe(computeRevision("\u0000absent", "{}")); + expect(computeRevision("a", null)).not.toBe(computeRevision("a", "\u0000absent")); + }); }); From 5999eb0ab214463fb306275b6e94d78e221cd88f Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 15:44:10 +0900 Subject: [PATCH 11/16] fix(codex): hash the selected base prompt whether or not we authored it A fourth review round found the last asymmetry in this fingerprint: a managed variant contributed its bytes, while an external selection contributed only the word "external". Editing the file named by model_instructions_file left the admission key unchanged, so the guarantee depended on who wrote the base prompt -- which is not a distinction the probe's caller can see. The path is hashed alongside the contents. Repointing the config key at a different file changes the prompt even when both files read alike. One correction to the reported impact, since it changes what this fixes: base-instructions is reported not-exposed unconditionally, because prompt_debug.rs discards it, so no stale text was ever rendered back to a caller. The hole in admission identity was real; the observable stale layer was not. It is worth closing on the first ground. This input was listed in the plan as unfingerprintable. That was wrong -- it is an ordinary file this process can read -- and the plan is corrected rather than left to imply the limitation still stands. Mutation: dropping the external branch turns case 34 red (59 pass / 1 fail). 117 pass / 0 fail across the three prompt suites; tsc clean. --- .../130_pr2872_probe_fingerprint.md | 29 +++++++++++- src/codex/prompt-layers.ts | 19 ++++++++ tests/codex-prompt-route.test.ts | 44 +++++++++++++++++++ 3 files changed, 90 insertions(+), 2 deletions(-) diff --git a/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md b/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md index d7f8846d41..032b88fb85 100644 --- a/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md +++ b/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md @@ -59,14 +59,39 @@ instead of implying otherwise: - Skill metadata, plugin manifests, and MCP/app availability feed ``, `` and ``. - Clock, timezone, shell and permission state feed ``. -- An external `model_instructions_file` target. `codex debug prompt-input` discards - base instructions, so it does not currently stale a rendered layer. None is writable through `/api/codex-prompt`; each needs an external edit concurrent with an in-flight probe. A 15-second window bounded by a fail-closed `busy` is the exposure, and pretending to fingerprint a clock would be worse than documenting it. +The external `model_instructions_file` target was on that list and has been moved +off it. Listing it there was the wrong call twice over: it is an ordinary file this +process can read, and leaving it out meant the guarantee depended on whether we +authored the selected base prompt. A fourth review round found the asymmetry — +managed variant bytes hashed, an external selection recorded as the bare word +`external`. Its path and bytes are now hashed like any other field. + +One correction to that round's stated impact, because the difference matters for +anyone reading this later: `base-instructions` is reported `not-exposed` +unconditionally, since `prompt_debug.rs` discards it. So the stale value was never +rendered back to a caller. The defect was a real hole in admission identity, not an +observable stale layer, and it is worth closing on the first ground alone. + +## Round-by-round record + +Four review rounds, four real defects. Worth keeping because the pattern is the +point: each fix was itself reviewed, and three of the four findings were in code +written to fix the previous finding. + +1. The fingerprint omitted `AGENTS.md` entirely. +2. Fields were concatenated unframed, so contents could imitate a separator; the + `\0absent` sentinel collided with a file holding those literal bytes. +3. `computeRevision` still had that same unframed shape inside it — and that value + is also the write-path concurrency token, so the collision reached further than + the probe. +4. An external base selection was hashed as a bare kind string. + ## Verification Route-level, in the file whose fixture separates `CODEX_HOME` from the injected diff --git a/src/codex/prompt-layers.ts b/src/codex/prompt-layers.ts index 89e572b166..de6f9f659b 100644 --- a/src/codex/prompt-layers.ts +++ b/src/codex/prompt-layers.ts @@ -701,6 +701,25 @@ export function computePromptProbeStateFingerprint(opts?: Paths): string { if (selection.kind === "variant") { updateFingerprintField(hash, "variant-bytes", readFileOrNull(join(activeBaseVariantDir(opts), `${selection.id}.md`))); } + if (selection.kind === "external") { + // The selected base file is hashed whether or not we manage it. Hashing the + // managed variant's bytes while recording an external selection as the bare + // word "external" would make the guarantee depend on who authored the file, + // which is not a distinction the probe's caller can see. + // + // Its path is part of the identity as well as its contents: pointing the key + // at a different file changes the prompt even when both files read alike. + updateFingerprintField(hash, "external-path", selection.path); + let externalBytes: string | null = null; + try { + externalBytes = readFileOrNull(resolve(expandUserPath(selection.path))); + } catch { + // An unresolvable path is a state, not a failure: it hashes as absent, and + // resolveBaseSelection has already reported the selection as external. + externalBytes = null; + } + updateFingerprintField(hash, "external-bytes", externalBytes); + } // Codex prefers AGENTS.override.md over AGENTS.md, so both spellings are hashed // in that order: an override edit changes the rendered project document exactly // as a plain edit does. diff --git a/tests/codex-prompt-route.test.ts b/tests/codex-prompt-route.test.ts index ca2d9c2ff5..d838c0f595 100644 --- a/tests/codex-prompt-route.test.ts +++ b/tests/codex-prompt-route.test.ts @@ -1123,6 +1123,50 @@ describe("020 coverage completions", () => { expect(promptTextProbeSpawnAttemptsForTests()).toBe(2); }); + /** + * An externally authored base prompt is hashed exactly like a managed variant. + * Recording only the word "external" made the admission guarantee depend on who + * wrote the file, which is not a distinction the caller can observe. The path is + * part of the identity too: repointing the key at a different file changes the + * prompt even when both files happen to read alike. + */ + test("34. editing an external base prompt invalidates an in-flight text probe", async () => { + const fx = fixture("model = \"x\"\n"); + const externalPath = join(fx.decoyHome, "external-base.md"); + writeFileSync(externalPath, "old-external", "utf8"); + await call("PUT", "/api/codex-prompt/base/select", fx, { + kind: "external", path: externalPath, revision: await revision(fx), + }); + // Selection through the route is not assumed: the fixture config is what the + // fingerprint reads, so assert the state this case depends on. + writeFileSync(fx.configPath, `model_instructions_file = "${externalPath}"\n`, "utf8"); + + const startedPath = join(fx.decoyHome, "external-probe-starts.txt"); + const source = [ + `const fs = require("node:fs");`, + `const doc = fs.readFileSync(${JSON.stringify(externalPath)}, "utf8");`, + `fs.appendFileSync(${JSON.stringify(startedPath)}, "1\\n");`, + `const output = JSON.stringify([{type:"message",role:"developer",content:[{type:"input_text",text:"" + doc + ""}]}]);`, + "setTimeout(() => process.stdout.write(output), 200);", + ].join(""); + setPromptTextProbeCommandForTests({ binary: process.execPath, args: ["-e", source] }); + + const beforeEdit = call("GET", "/api/codex-prompt/text", fx); + await waitUntil(() => existsSync(startedPath), "external base probe start"); + writeFileSync(externalPath, "new-external", "utf8"); + + expect((await call("GET", "/api/codex-prompt/text", fx)).body).toMatchObject({ + ok: false, + detail: "another prompt probe is still finishing; retry shortly", + }); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(1); + expect((await beforeEdit).body.layers.skills.text).toBe("old-external"); + + const fresh = await call("GET", "/api/codex-prompt/text", fx); + expect(fresh.body.layers.skills.text).toBe("new-external"); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(2); + }); + test("24. every ownership state is named, not collapsed into a boolean", async () => { // developerInstructionsOwned:false covers an ABSENT key and an EXTERNAL one, and // a GUI that cannot tell them apart hides its own create affordance from every From 9eef40e5edf495c84e681edf7d6138423ae8f1f4 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 15:58:35 +0900 Subject: [PATCH 12/16] fix(codex): resolve and enumerate project documents the way Codex does A fifth review round found two more inputs where the admission key disagreed with what the probe actually renders. A relative model_instructions_file was resolved with a bare resolve(), which uses this process's working directory -- the proxy's, unrelated to either the config file or the probe child's cwd. Codex resolves its relative path fields against the directory holding config.toml, so the fingerprint could hash a file that has nothing to do with the prompt. It now resolves the same way. The project-document set was the two built-in names. Codex builds its candidate list from project_doc_fallback_filenames as well, so a user who configures TEAM.md renders TEAM.md, and an edit to it moved no key. The list is now read from config, in upstream's order, using the decoder that already backs every other value read out of this file -- a double-quote-only regex would have skipped a single-quoted filename silently. Ancestor project documents stay out, and that is deliberate rather than unfinished: Codex walks from a project root to its cwd, while the probe runs in CODEX_HOME with no checkout around it, so the walk has nothing to find. The plan records that alongside the other inputs that cannot be hashed. Mutations: resolving a relative path against the process cwd turns case 35 red; ignoring the configured filenames turns both case 36 spellings red. 120 pass / 0 fail across the three prompt suites; tsc clean. --- .../130_pr2872_probe_fingerprint.md | 30 +++++++ src/codex/prompt-layers.ts | 42 ++++++++- tests/codex-prompt-route.test.ts | 85 ++++++++++++++++++- 3 files changed, 154 insertions(+), 3 deletions(-) diff --git a/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md b/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md index 032b88fb85..85c7935fa1 100644 --- a/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md +++ b/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md @@ -91,6 +91,36 @@ written to fix the previous finding. is also the write-path concurrency token, so the collision reached further than the probe. 4. An external base selection was hashed as a bare kind string. +5. Two more: a relative `model_instructions_file` was resolved against the proxy's + own working directory instead of the config file's, so it hashed an unrelated + file; and only the two built-in project-document names were considered, so a + configured `project_doc_fallback_filenames` entry could be edited unnoticed. + +## The pattern, and where it stops + +Five rounds is the interesting part of this record. Each fix was reviewed, and four +of the six findings were in code written to close the previous finding. The reason is +consistent: a cache key is only as good as its worst-covered input, and "I added the +input I was told about" is not the same as "the key names everything the output +depends on". Framing, path resolution, and candidate-set breadth each failed +separately. + +What remains uncovered is now a short and deliberate list, and every item on it has a +reason that is not "we did not get to it": + +- **Ancestor project documents.** Codex walks from a project root to its cwd. The + probe runs in `CODEX_HOME` with no checkout around it, so there is no ancestor + chain to walk. Hashing a walk that cannot happen would be noise. +- **Skill and plugin metadata.** Readable, but they are directory trees owned by + Codex, not files with a stable enumeration contract here. Hashing them means + duplicating a loader we do not own and would have to keep in sync. +- **Clock, timezone, shell, MCP availability.** Not files. A fingerprint over a clock + is not a fingerprint. + +The exposure for each is the same and it is small: an external edit landing inside a +single in-flight probe's window, whose failure mode is a fail-closed `busy` and a +retry, not corruption. The honest move is to name them here rather than to imply the +key is total. ## Verification diff --git a/src/codex/prompt-layers.ts b/src/codex/prompt-layers.ts index de6f9f659b..faf2a6d624 100644 --- a/src/codex/prompt-layers.ts +++ b/src/codex/prompt-layers.ts @@ -190,6 +190,40 @@ export function activeBaseVariantDir(opts?: Paths): string { */ const PROBE_INSTRUCTION_FILES = ["AGENTS.override.md", "AGENTS.md"] as const; +/** + * The project-document filenames Codex would look for in a given home, in its own + * order: the two built-ins first, then whatever `project_doc_fallback_filenames` + * adds, de-duplicated (`core/src/agents_md.rs` `candidate_filenames`). + * + * Read from config rather than hard-coded, because a user who configures + * `TEAM.md` renders TEAM.md, and a fingerprint that only knew about AGENTS.md + * would let an edit to it pass unnoticed. + * + * Deliberately narrower than upstream in one respect, and it is a real limit: Codex + * also walks ancestor directories from the project root to its cwd. The probe runs + * in CODEX_HOME with no project checkout around it, so the ancestor walk has nothing + * to find; this reads the home's own candidates only. + */ +function probeInstructionFilenames(configBytes: string | null): string[] { + const names: string[] = [...PROBE_INSTRUCTION_FILES]; + for (const line of rootLines(configBytes ?? "")) { + const m = /^\s*project_doc_fallback_filenames\s*=\s*\[(.*)\]\s*(?:#.*)?$/.exec(line); + if (!m) continue; + for (const raw of m[1]!.split(",")) { + const trimmed = raw.trim(); + if (trimmed === "") continue; + // Reuse the decoder that already backs every other value we read out of this + // file, so a single-quoted or escaped filename is not silently skipped. + const decoded = trimmed.startsWith("'") && trimmed.endsWith("'") && trimmed.length >= 2 + ? trimmed.slice(1, -1) + : decodeBasicString(trimmed); + if (decoded === null || decoded === "") continue; + if (!names.includes(decoded)) names.push(decoded); + } + } + return names; +} + /** * Feed one named field into a fingerprint, framed so that no two distinct states * can produce the same digest. @@ -712,7 +746,11 @@ export function computePromptProbeStateFingerprint(opts?: Paths): string { updateFingerprintField(hash, "external-path", selection.path); let externalBytes: string | null = null; try { - externalBytes = readFileOrNull(resolve(expandUserPath(selection.path))); + // Relative to the CONFIG FILE's directory, which is what Codex does with its + // relative path fields. resolve() alone would use this process's cwd — the + // proxy's working directory, which has nothing to do with either the config + // or the probe child's cwd — and would hash an unrelated file. + externalBytes = readFileOrNull(resolve(dirname(activeConfigPath(opts)), expandUserPath(selection.path))); } catch { // An unresolvable path is a state, not a failure: it hashes as absent, and // resolveBaseSelection has already reported the selection as external. @@ -724,7 +762,7 @@ export function computePromptProbeStateFingerprint(opts?: Paths): string { // in that order: an override edit changes the rendered project document exactly // as a plain edit does. const probeHome = resolveCodexHomeDir(); - for (const name of PROBE_INSTRUCTION_FILES) { + for (const name of probeInstructionFilenames(configBytes)) { updateFingerprintField(hash, name, readFileOrNull(join(probeHome, name))); } return `sha256:${hash.digest("hex")}`; diff --git a/tests/codex-prompt-route.test.ts b/tests/codex-prompt-route.test.ts index d838c0f595..10a13cd79a 100644 --- a/tests/codex-prompt-route.test.ts +++ b/tests/codex-prompt-route.test.ts @@ -9,7 +9,7 @@ import { afterEach, describe, expect, test } from "bun:test"; import { existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { dirname, join } from "node:path"; import { handleManagementAPI } from "../src/server/management-api"; import { LAYER_INVENTORY, readPromptLayers } from "../src/codex/prompt-layers"; import { @@ -1167,6 +1167,89 @@ describe("020 coverage completions", () => { expect(promptTextProbeSpawnAttemptsForTests()).toBe(2); }); + /** + * A relative model_instructions_file is resolved against the config file's own + * directory, which is what Codex does with its relative path fields. Resolving it + * against this process's cwd instead would hash whatever happens to sit beside the + * proxy's working directory — a file unrelated to the prompt. + * + * The fixture root is not the process cwd, so this fails if the base is wrong. + */ + test("35. a relative external base path resolves against the config directory", async () => { + const fx = fixture("model = \"x\"\n"); + const externalPath = join(dirname(fx.configPath), "relative-base.md"); + writeFileSync(externalPath, "old-relative", "utf8"); + writeFileSync(fx.configPath, "model_instructions_file = \"relative-base.md\"\n", "utf8"); + + const startedPath = join(fx.decoyHome, "relative-probe-starts.txt"); + const source = [ + `const fs = require("node:fs");`, + `const doc = fs.readFileSync(${JSON.stringify(externalPath)}, "utf8");`, + `fs.appendFileSync(${JSON.stringify(startedPath)}, "1\\n");`, + `const output = JSON.stringify([{type:"message",role:"developer",content:[{type:"input_text",text:"" + doc + ""}]}]);`, + "setTimeout(() => process.stdout.write(output), 200);", + ].join(""); + setPromptTextProbeCommandForTests({ binary: process.execPath, args: ["-e", source] }); + + const beforeEdit = call("GET", "/api/codex-prompt/text", fx); + await waitUntil(() => existsSync(startedPath), "relative base probe start"); + writeFileSync(externalPath, "new-relative", "utf8"); + + expect((await call("GET", "/api/codex-prompt/text", fx)).body).toMatchObject({ + ok: false, + detail: "another prompt probe is still finishing; retry shortly", + }); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(1); + expect((await beforeEdit).body.layers.skills.text).toBe("old-relative"); + + const fresh = await call("GET", "/api/codex-prompt/text", fx); + expect(fresh.body.layers.skills.text).toBe("new-relative"); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(2); + }); + + /** + * A user who configures project_doc_fallback_filenames renders those files, so the + * admission key has to know about them. Hard-coding AGENTS.md would let an edit to + * a configured TEAM.md pass unnoticed and serve a joiner stale text. + * + * Both TOML spellings are covered: the value is read with the same decoder used for + * every other field in this file, not a double-quote-only regex. + */ + for (const spelling of [ + { label: "double-quoted", literal: "[\"TEAM.md\"]" }, + { label: "single-quoted", literal: "['TEAM.md']" }, + ]) { + test(`36. a ${spelling.label} fallback project document moves probe admission`, async () => { + const fx = fixture(`project_doc_fallback_filenames = ${spelling.literal}\n`); + const teamPath = join(fx.decoyHome, "TEAM.md"); + writeFileSync(teamPath, "old-team", "utf8"); + const startedPath = join(fx.decoyHome, "team-probe-starts.txt"); + const source = [ + `const fs = require("node:fs");`, + `const doc = fs.readFileSync(${JSON.stringify(teamPath)}, "utf8");`, + `fs.appendFileSync(${JSON.stringify(startedPath)}, "1\\n");`, + `const output = JSON.stringify([{type:"message",role:"developer",content:[{type:"input_text",text:"" + doc + ""}]}]);`, + "setTimeout(() => process.stdout.write(output), 200);", + ].join(""); + setPromptTextProbeCommandForTests({ binary: process.execPath, args: ["-e", source] }); + + const beforeEdit = call("GET", "/api/codex-prompt/text", fx); + await waitUntil(() => existsSync(startedPath), "fallback doc probe start"); + writeFileSync(teamPath, "new-team", "utf8"); + + expect((await call("GET", "/api/codex-prompt/text", fx)).body).toMatchObject({ + ok: false, + detail: "another prompt probe is still finishing; retry shortly", + }); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(1); + expect((await beforeEdit).body.layers.skills.text).toBe("old-team"); + + const fresh = await call("GET", "/api/codex-prompt/text", fx); + expect(fresh.body.layers.skills.text).toBe("new-team"); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(2); + }); + } + test("24. every ownership state is named, not collapsed into a boolean", async () => { // developerInstructionsOwned:false covers an ABSENT key and an EXTERNAL one, and // a GUI that cannot tell them apart hides its own create affordance from every From c4af3fc86c01f40b33249d675735def347f7a942 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 16:06:06 +0900 Subject: [PATCH 13/16] fix(codex): walk for project documents instead of assuming none exist The previous commit documented the ancestor walk as unreachable, on the grounds that the probe runs in CODEX_HOME with no checkout around it. That was wrong. The default project-root marker is .git, and ~/.codex inside a dotfiles repository is an ordinary setup: Codex renders that repository's AGENTS.md, and editing it moved no admission key. The walk is now performed the way upstream does it -- nearest ancestor holding a configured marker, then every directory from that root down to the home, root first -- and a present-but-empty project_root_markers disables detection rather than falling back to the default. Two parsing gaps went with it. Upstream trims each configured filename and drops whitespace-only entries, and the ordinary multi-line array spelling project_doc_fallback_filenames = [ "TEAM.md", ] was invisible to a single-line regex, so a rendered document could be edited unnoticed. Array reading now spans lines and strips comments, and both keys share it. Paths go in the framed CONTENTS, never in a field name: only contents carry a byte length, so building a name from a path would reintroduce the ambiguity the helper exists to remove. Mutations: home-only iteration turns case 37 red; single-line-only parsing without trimming turns the multi-line and padded spellings of case 36 red. 123 pass / 0 fail across the three prompt suites; tsc clean. --- .../130_pr2872_probe_fingerprint.md | 28 ++-- src/codex/prompt-layers.ts | 125 +++++++++++++++--- tests/codex-prompt-route.test.ts | 52 ++++++++ 3 files changed, 181 insertions(+), 24 deletions(-) diff --git a/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md b/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md index 85c7935fa1..1b5797f08c 100644 --- a/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md +++ b/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md @@ -105,22 +105,32 @@ input I was told about" is not the same as "the key names everything the output depends on". Framing, path resolution, and candidate-set breadth each failed separately. -What remains uncovered is now a short and deliberate list, and every item on it has a -reason that is not "we did not get to it": +A sixth round then rejected the first version of this very section, and it was right. +It claimed the ancestor walk could never find anything because the probe runs in +`CODEX_HOME` with no checkout around it. The default project-root marker is `.git`, +and `~/.codex` inside a dotfiles repository is an ordinary setup: there, Codex renders +the repository's own `AGENTS.md` and the walk matters. The same round found two more +parsing gaps — upstream trims each configured filename and drops whitespace-only +entries, and the ordinary multi-line array spelling was missed by a single-line regex. + +So the walk is now performed rather than argued away: nearest ancestor holding a +configured marker, then every directory from that root down to the home, with a +present-but-empty `project_root_markers` disabling detection exactly as upstream does. + +What remains uncovered, with a reason that is not "we did not get to it": -- **Ancestor project documents.** Codex walks from a project root to its cwd. The - probe runs in `CODEX_HOME` with no checkout around it, so there is no ancestor - chain to walk. Hashing a walk that cannot happen would be noise. - **Skill and plugin metadata.** Readable, but they are directory trees owned by Codex, not files with a stable enumeration contract here. Hashing them means duplicating a loader we do not own and would have to keep in sync. - **Clock, timezone, shell, MCP availability.** Not files. A fingerprint over a clock is not a fingerprint. -The exposure for each is the same and it is small: an external edit landing inside a -single in-flight probe's window, whose failure mode is a fail-closed `busy` and a -retry, not corruption. The honest move is to name them here rather than to imply the -key is total. +The exposure for both is an external edit landing inside a single in-flight probe's +window, whose failure mode is a fail-closed `busy` and a retry, not corruption. + +Given that this section has now been wrong once, the standard it should be held to is +worth stating: an input belongs on this list only when it cannot be read from this +process, not when reading it looks inconvenient. ## Verification diff --git a/src/codex/prompt-layers.ts b/src/codex/prompt-layers.ts index faf2a6d624..a6c21f6d55 100644 --- a/src/codex/prompt-layers.ts +++ b/src/codex/prompt-layers.ts @@ -199,29 +199,115 @@ const PROBE_INSTRUCTION_FILES = ["AGENTS.override.md", "AGENTS.md"] as const; * `TEAM.md` renders TEAM.md, and a fingerprint that only knew about AGENTS.md * would let an edit to it pass unnoticed. * - * Deliberately narrower than upstream in one respect, and it is a real limit: Codex - * also walks ancestor directories from the project root to its cwd. The probe runs - * in CODEX_HOME with no project checkout around it, so the ancestor walk has nothing - * to find; this reads the home's own candidates only. */ function probeInstructionFilenames(configBytes: string | null): string[] { const names: string[] = [...PROBE_INSTRUCTION_FILES]; - for (const line of rootLines(configBytes ?? "")) { - const m = /^\s*project_doc_fallback_filenames\s*=\s*\[(.*)\]\s*(?:#.*)?$/.exec(line); + for (const entry of rootArrayEntries(configBytes, "project_doc_fallback_filenames")) { + // Upstream trims each configured name and drops whitespace-only entries + // (`core/src/config/mod.rs`), so " TEAM.md " and "TEAM.md" are one filename. + const name = entry.trim(); + if (name === "") continue; + if (!names.includes(name)) names.push(name); + } + return names; +} + +/** + * Decoded string entries of a root-scope TOML array. + * + * Spans lines. A single-line regex missed the ordinary multi-line spelling + * + * project_doc_fallback_filenames = [ + * "TEAM.md", + * ] + * + * which upstream accepts, and a missed array meant a rendered document whose edits + * moved no admission key. Values go through `decodeBasicString` — the decoder that + * already backs every other value read out of this file — with literal (single-quoted) + * strings handled separately, since those take no escapes. + */ +function rootArrayEntries(configBytes: string | null, key: string): string[] { + const lines = rootLines(configBytes ?? ""); + const opener = new RegExp(`^\\s*${key}\\s*=\\s*\\[(.*)$`); + for (let i = 0; i < lines.length; i += 1) { + const m = opener.exec(lines[i]!); if (!m) continue; - for (const raw of m[1]!.split(",")) { - const trimmed = raw.trim(); + let body = m[1]!; + // Accumulate until the closing bracket. Comments are stripped per line, so a + // trailing "# ]" cannot be mistaken for the terminator. + for (let j = i; !body.includes("]"); ) { + j += 1; + if (j >= lines.length) return []; + body += lines[j]!.replace(/#.*$/, ""); + } + body = body.slice(0, body.indexOf("]")); + const out: string[] = []; + for (const raw of body.split(",")) { + const trimmed = raw.trim().replace(/#.*$/, "").trim(); if (trimmed === "") continue; - // Reuse the decoder that already backs every other value we read out of this - // file, so a single-quoted or escaped filename is not silently skipped. const decoded = trimmed.startsWith("'") && trimmed.endsWith("'") && trimmed.length >= 2 ? trimmed.slice(1, -1) : decodeBasicString(trimmed); - if (decoded === null || decoded === "") continue; - if (!names.includes(decoded)) names.push(decoded); + if (decoded !== null) out.push(decoded); } + return out; } - return names; + return []; +} + +/** + * The directories Codex would look in for a project document, given the home the + * probe runs in. + * + * Upstream finds the nearest ancestor holding a `project_root_markers` entry + * (default `.git`) and then searches every directory from that root down to the cwd, + * inclusive; with no such ancestor it searches the cwd alone + * (`core/src/agents_md.rs` `agents_md_paths`). + * + * This was originally written off as unreachable on the grounds that the probe runs + * in CODEX_HOME with no checkout around it. That was wrong, and a review round caught + * it: `~/.codex` inside a dotfiles repository is an ordinary setup, and there the + * walk finds real documents. The walk is cheap — a bounded number of `existsSync` + * calls beside a subprocess spawn — so it is performed rather than assumed away. + */ +function probeProjectDocDirs(home: string, configBytes: string | null): string[] { + const markers = projectRootMarkers(configBytes); + // An explicitly empty array disables root detection upstream, which is not the same + // as an absent key falling back to the default. + if (markers.length === 0) return [home]; + let root: string | null = null; + for (let dir = home; ; ) { + if (markers.some(marker => existsSync(join(dir, marker)))) { root = dir; break; } + const parent = dirname(dir); + if (parent === dir) break; + dir = parent; + } + if (root === null) return [home]; + const dirs: string[] = []; + for (let dir = home; ; ) { + dirs.push(dir); + if (dir === root) break; + const parent = dirname(dir); + if (parent === dir) break; + dir = parent; + } + // Root first, matching upstream's reversed search order. Order is load-bearing: + // the digest must not change merely because the walk was traversed the other way. + return dirs.reverse(); +} + +/** `project_root_markers`, defaulting to `.git` when the key is absent. */ +function projectRootMarkers(configBytes: string | null): string[] { + if (!hasRootKey(configBytes, "project_root_markers")) return [".git"]; + // Present-but-empty disables root detection upstream, which is why presence is + // tested separately from the decoded entries rather than inferred from them. + return rootArrayEntries(configBytes, "project_root_markers").filter(m => m !== ""); +} + +/** Whether a root-scope key is present at all, regardless of what it holds. */ +function hasRootKey(configBytes: string | null, key: string): boolean { + const probe = new RegExp(`^\\s*${key}\\s*=`); + return rootLines(configBytes ?? "").some(line => probe.test(line)); } /** @@ -762,8 +848,17 @@ export function computePromptProbeStateFingerprint(opts?: Paths): string { // in that order: an override edit changes the rendered project document exactly // as a plain edit does. const probeHome = resolveCodexHomeDir(); - for (const name of probeInstructionFilenames(configBytes)) { - updateFingerprintField(hash, name, readFileOrNull(join(probeHome, name))); + const filenames = probeInstructionFilenames(configBytes); + for (const dir of probeProjectDocDirs(probeHome, configBytes)) { + for (const name of filenames) { + // The path goes in the CONTENTS, never in the field name. Only contents are + // length-framed, so a name built from a path would reintroduce exactly the + // ambiguity this helper exists to remove. Path and bytes are separate fields + // because two directories in the walk can both hold an AGENTS.md. + const path = join(dir, name); + updateFingerprintField(hash, "doc-path", path); + updateFingerprintField(hash, "doc-bytes", readFileOrNull(path)); + } } return `sha256:${hash.digest("hex")}`; } diff --git a/tests/codex-prompt-route.test.ts b/tests/codex-prompt-route.test.ts index 10a13cd79a..ab51228904 100644 --- a/tests/codex-prompt-route.test.ts +++ b/tests/codex-prompt-route.test.ts @@ -1218,6 +1218,11 @@ describe("020 coverage completions", () => { for (const spelling of [ { label: "double-quoted", literal: "[\"TEAM.md\"]" }, { label: "single-quoted", literal: "['TEAM.md']" }, + // Upstream accepts this ordinary spelling and a single-line regex missed it. + { label: "multi-line", literal: "[\n \"TEAM.md\",\n]" }, + // Upstream trims each name and drops whitespace-only entries, so a padded value + // is the same filename rather than a different one. + { label: "padded", literal: "[\" TEAM.md \", \" \"]" }, ]) { test(`36. a ${spelling.label} fallback project document moves probe admission`, async () => { const fx = fixture(`project_doc_fallback_filenames = ${spelling.literal}\n`); @@ -1250,6 +1255,53 @@ describe("020 coverage completions", () => { }); } + /** + * A CODEX_HOME inside a git checkout — `~/.codex` in a dotfiles repository is an + * ordinary setup — makes Codex search every directory from the repository root down + * to the home, so a parent AGENTS.md is rendered and has to move admission. + * + * This case exists because the first version of the fix argued the ancestor walk + * could never find anything and left it out. It could. + */ + test("37. a parent-directory project document moves probe admission", async () => { + const fx = fixture("model = \"x\"\n"); + // A repository root of this test's own, holding the home one level down, so the + // document is reachable ONLY by walking up. Built inside the fixture's tracked + // root rather than beside it: writing a .git marker into the shared temp + // directory would change root detection for every other test using tmpdir(). + const root = join(fx.baseVariantDir, "..", "ancestor-root"); + const nestedHome = join(root, "home"); + mkdirSync(join(root, ".git"), { recursive: true }); + mkdirSync(nestedHome, { recursive: true }); + const parentDoc = join(root, "AGENTS.md"); + writeFileSync(parentDoc, "old-parent", "utf8"); + const startedPath = join(nestedHome, "parent-probe-starts.txt"); + const source = [ + `const fs = require("node:fs");`, + `const doc = fs.readFileSync(${JSON.stringify(parentDoc)}, "utf8");`, + `fs.appendFileSync(${JSON.stringify(startedPath)}, "1\\n");`, + `const output = JSON.stringify([{type:"message",role:"developer",content:[{type:"input_text",text:"" + doc + ""}]}]);`, + "setTimeout(() => process.stdout.write(output), 200);", + ].join(""); + setPromptTextProbeCommandForTests({ binary: process.execPath, args: ["-e", source] }); + + const nested: Fixture = { ...fx, decoyHome: nestedHome }; + const beforeEdit = call("GET", "/api/codex-prompt/text", nested); + await waitUntil(() => existsSync(startedPath), "parent doc probe start"); + writeFileSync(parentDoc, "new-parent", "utf8"); + + expect((await call("GET", "/api/codex-prompt/text", nested)).body).toMatchObject({ + ok: false, + detail: "another prompt probe is still finishing; retry shortly", + }); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(1); + expect((await beforeEdit).body.layers.skills.text).toBe("old-parent"); + + const fresh = await call("GET", "/api/codex-prompt/text", nested); + expect(fresh.body.layers.skills.text).toBe("new-parent"); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(2); + }); + test("24. every ownership state is named, not collapsed into a boolean", async () => { // developerInstructionsOwned:false covers an ABSENT key and an EXTERNAL one, and // a GUI that cannot tell them apart hides its own create affordance from every From 8a65ea6b72c93eddb4f5d438780aeec693656ad2 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 16:17:49 +0900 Subject: [PATCH 14/16] fix(codex): parse the config instead of pattern-matching it Rounds five, six and seven each found another valid TOML spelling the hand-rolled reader missed: a multi-line array, a comment directly after the opening bracket, a quoted key. Three patches to the same regex, each closing one spelling and leaving the others. The pattern was the defect. TOML is not a line format, so no regex over lines can enumerate what a parser accepts -- it only ever covers the example in front of it. Each miss was a rendered project document whose edits moved no admission key, which is a stale read. Both keys now go through Bun.TOML.parse. The module header forbids trusting a JS TOML parser and that is worth not eroding, so the distinction is explicit in the comment: the prohibition is about verifying bytes we WRITE, where Bun and Rust toml_edit disagree on escapes and Codex reads what we wrote. This reads two arrays of plain filenames, and the failure directions are opposite -- a parse disagreement here costs a redundant probe, a missed spelling costs stale text. An unparseable file yields nothing, which is right: Codex could not load it either. Coverage now crosses five value spellings with both key forms, plus a quoted project_root_markers. Mutation: restoring the regex reader turns exactly the eight cases it cannot handle red (65 pass / 8 fail). 158 pass / 0 fail across the four prompt suites, including the write-path suite that owns the no-JS-parser-for-verification rule; tsc clean. --- .../130_pr2872_probe_fingerprint.md | 18 ++++++ src/codex/prompt-layers.ts | 64 +++++++++---------- tests/codex-prompt-route.test.ts | 51 ++++++++++++++- 3 files changed, 96 insertions(+), 37 deletions(-) diff --git a/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md b/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md index 1b5797f08c..bff4e6db18 100644 --- a/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md +++ b/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md @@ -132,6 +132,24 @@ Given that this section has now been wrong once, the standard it should be held worth stating: an input belongs on this list only when it cannot be read from this process, not when reading it looks inconvenient. +## The reader, and why it stopped being a regex + +Rounds five, six and seven each found another valid TOML spelling the hand-rolled +reader missed: a multi-line array, then a comment directly after the opening bracket, +then a quoted key. Three rounds, three patches to the same regex, each closing one +spelling and leaving the rest. + +At that point the pattern was the defect. TOML is not a line format, so no regex over +lines can enumerate what a parser accepts, and each fix was only ever going to cover +the example in front of it. `Bun.TOML.parse` reads both keys now. + +The module header forbids trusting a JS TOML parser, and that prohibition is worth +not eroding, so the distinction matters: it is about VERIFYING BYTES WE WRITE, where +Bun and Rust `toml_edit` disagree on escapes and Codex reads what we wrote. This is a +read of two arrays of plain filenames, and the failure directions are opposite. A +parse disagreement here costs a redundant probe; a missed spelling costs a stale read. +An unparseable file yields nothing, which is correct — Codex could not load it either. + ## Verification Route-level, in the file whose fixture separates `CODEX_HOME` from the injected diff --git a/src/codex/prompt-layers.ts b/src/codex/prompt-layers.ts index a6c21f6d55..1723b57d89 100644 --- a/src/codex/prompt-layers.ts +++ b/src/codex/prompt-layers.ts @@ -215,44 +215,39 @@ function probeInstructionFilenames(configBytes: string | null): string[] { /** * Decoded string entries of a root-scope TOML array. * - * Spans lines. A single-line regex missed the ordinary multi-line spelling + * Parsed, not pattern-matched. Three successive review rounds each found another + * valid spelling a hand-rolled reader missed — multi-line arrays, a comment after the + * opening bracket, a quoted key — and every miss was a rendered document whose edits + * moved no admission key. The pattern was the defect: TOML is not a line format, so + * no regex over lines can enumerate what a parser accepts. * - * project_doc_fallback_filenames = [ - * "TEAM.md", - * ] + * The module header forbids trusting a JS TOML parser to VERIFY bytes we write, + * because Bun and Rust `toml_edit` disagree on escapes and Codex reads what we wrote. + * That prohibition is about writing. This is a read of two arrays of plain filenames, + * and the failure modes differ in the safe direction: a parse disagreement here can + * only cost a redundant probe, never a corrupted config. A missed spelling costs a + * stale read, which is the defect being fixed. * - * which upstream accepts, and a missed array meant a rendered document whose edits - * moved no admission key. Values go through `decodeBasicString` — the decoder that - * already backs every other value read out of this file — with literal (single-quoted) - * strings handled separately, since those take no escapes. + * An unparseable file yields no entries. Codex could not load it either, so there is + * no configured value to honour. */ function rootArrayEntries(configBytes: string | null, key: string): string[] { - const lines = rootLines(configBytes ?? ""); - const opener = new RegExp(`^\\s*${key}\\s*=\\s*\\[(.*)$`); - for (let i = 0; i < lines.length; i += 1) { - const m = opener.exec(lines[i]!); - if (!m) continue; - let body = m[1]!; - // Accumulate until the closing bracket. Comments are stripped per line, so a - // trailing "# ]" cannot be mistaken for the terminator. - for (let j = i; !body.includes("]"); ) { - j += 1; - if (j >= lines.length) return []; - body += lines[j]!.replace(/#.*$/, ""); - } - body = body.slice(0, body.indexOf("]")); - const out: string[] = []; - for (const raw of body.split(",")) { - const trimmed = raw.trim().replace(/#.*$/, "").trim(); - if (trimmed === "") continue; - const decoded = trimmed.startsWith("'") && trimmed.endsWith("'") && trimmed.length >= 2 - ? trimmed.slice(1, -1) - : decodeBasicString(trimmed); - if (decoded !== null) out.push(decoded); - } - return out; + const value = rootValue(configBytes, key); + if (!Array.isArray(value)) return []; + return value.filter((entry): entry is string => typeof entry === "string"); +} + +/** A root-scope value, or undefined when the key is absent or the file will not parse. */ +function rootValue(configBytes: string | null, key: string): unknown { + if (configBytes === null) return undefined; + let parsed: unknown; + try { + parsed = Bun.TOML.parse(configBytes); + } catch { + return undefined; } - return []; + if (typeof parsed !== "object" || parsed === null) return undefined; + return (parsed as Record)[key]; } /** @@ -306,8 +301,7 @@ function projectRootMarkers(configBytes: string | null): string[] { /** Whether a root-scope key is present at all, regardless of what it holds. */ function hasRootKey(configBytes: string | null, key: string): boolean { - const probe = new RegExp(`^\\s*${key}\\s*=`); - return rootLines(configBytes ?? "").some(line => probe.test(line)); + return rootValue(configBytes, key) !== undefined; } /** diff --git a/tests/codex-prompt-route.test.ts b/tests/codex-prompt-route.test.ts index ab51228904..1cf0907c78 100644 --- a/tests/codex-prompt-route.test.ts +++ b/tests/codex-prompt-route.test.ts @@ -1223,9 +1223,14 @@ describe("020 coverage completions", () => { // Upstream trims each name and drops whitespace-only entries, so a padded value // is the same filename rather than a different one. { label: "padded", literal: "[\" TEAM.md \", \" \"]" }, + // A comment directly after the opening bracket. The hand-rolled reader consumed + // the first entry along with it. + { label: "comment-after-bracket", literal: "[ # team docs\n \"TEAM.md\",\n]" }, ]) { - test(`36. a ${spelling.label} fallback project document moves probe admission`, async () => { - const fx = fixture(`project_doc_fallback_filenames = ${spelling.literal}\n`); + for (const keyForm of ["bare", "quoted"]) { + test(`36. a ${spelling.label} fallback project document with a ${keyForm} key moves probe admission`, async () => { + const key = keyForm === "quoted" ? "\"project_doc_fallback_filenames\"" : "project_doc_fallback_filenames"; + const fx = fixture(`${key} = ${spelling.literal}\n`); const teamPath = join(fx.decoyHome, "TEAM.md"); writeFileSync(teamPath, "old-team", "utf8"); const startedPath = join(fx.decoyHome, "team-probe-starts.txt"); @@ -1253,6 +1258,7 @@ describe("020 coverage completions", () => { expect(fresh.body.layers.skills.text).toBe("new-team"); expect(promptTextProbeSpawnAttemptsForTests()).toBe(2); }); + } } /** @@ -1302,6 +1308,47 @@ describe("020 coverage completions", () => { expect(promptTextProbeSpawnAttemptsForTests()).toBe(2); }); + /** + * Root detection has to honour a configured marker under any valid spelling. With a + * quoted key a hand-rolled reader fell back to `.git`, found no root, and searched + * the home alone — so an ancestor document it should have covered went unhashed. + */ + test("38. a quoted project_root_markers key still selects the configured root", async () => { + const root = mkdtempSync(join(tmpdir(), "ocx-prompt-marker-")); + roots.push(root); + const nestedHome = join(root, "home"); + mkdirSync(nestedHome, { recursive: true }); + writeFileSync(join(root, ".probe-root"), "", "utf8"); + const fx = fixture("\"project_root_markers\" = [\".probe-root\"]\n"); + const parentDoc = join(root, "AGENTS.md"); + writeFileSync(parentDoc, "old-marker", "utf8"); + const startedPath = join(nestedHome, "marker-probe-starts.txt"); + const source = [ + `const fs = require("node:fs");`, + `const doc = fs.readFileSync(${JSON.stringify(parentDoc)}, "utf8");`, + `fs.appendFileSync(${JSON.stringify(startedPath)}, "1\\n");`, + `const output = JSON.stringify([{type:"message",role:"developer",content:[{type:"input_text",text:"" + doc + ""}]}]);`, + "setTimeout(() => process.stdout.write(output), 200);", + ].join(""); + setPromptTextProbeCommandForTests({ binary: process.execPath, args: ["-e", source] }); + + const nested: Fixture = { ...fx, decoyHome: nestedHome }; + const beforeEdit = call("GET", "/api/codex-prompt/text", nested); + await waitUntil(() => existsSync(startedPath), "marker doc probe start"); + writeFileSync(parentDoc, "new-marker", "utf8"); + + expect((await call("GET", "/api/codex-prompt/text", nested)).body).toMatchObject({ + ok: false, + detail: "another prompt probe is still finishing; retry shortly", + }); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(1); + expect((await beforeEdit).body.layers.skills.text).toBe("old-marker"); + + const fresh = await call("GET", "/api/codex-prompt/text", nested); + expect(fresh.body.layers.skills.text).toBe("new-marker"); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(2); + }); + test("24. every ownership state is named, not collapsed into a boolean", async () => { // developerInstructionsOwned:false covers an ABSENT key and an EXTERNAL one, and // a GUI that cannot tell them apart hides its own create affordance from every From 4cd08359cc32006f8dfcef435ec11777ec8fab74 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 16:35:03 +0900 Subject: [PATCH 15/16] fix(codex): fail over instead of failing silent, and hash skill manifests Two defects from the eighth review round, one of them introduced by the previous commit. Adopting Bun.TOML.parse fixed seven missed spellings and added a worse failure. i64 is an ordinary TOML integer, so model_context_window = 9223372036854775807 is a config Codex reads happily, while Bun rejects the entire document because the value exceeds JavaScript's safe range. Treating that as "no keys configured" dropped every fallback filename at once -- something the regex reader did not do. The parse is now the preferred reader rather than the only one, and a parse failure falls through to a deliberately loose scan that over-reports. An extra hashed filename costs one redundant probe; a missing one costs stale text. The comment claiming the header's parser warning did not apply to these reads was wrong and is replaced by what the round actually proved. Second: SKILL.md manifests are hashed. They were documented as unobservable on the grounds that skills are a directory tree with no enumeration contract here. A live description edit moved the probe's output while the fingerprint stood still. It is one directory listing and one read per skill, sorted so the digest does not depend on readdir order, beside a subprocess that costs orders of magnitude more. Only the top-level manifest: bundled scripts do not reach the rendered section. Mutations: collapsing a parse failure to absent turns case 39 red; skipping the manifests turns case 40 red. 160 pass / 0 fail across the four prompt suites; tsc clean. --- .../130_pr2872_probe_fingerprint.md | 34 +++-- src/codex/prompt-layers.ts | 121 ++++++++++++++++-- tests/codex-prompt-route.test.ts | 83 ++++++++++++ 3 files changed, 211 insertions(+), 27 deletions(-) diff --git a/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md b/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md index bff4e6db18..df18a7ded9 100644 --- a/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md +++ b/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md @@ -117,20 +117,26 @@ So the walk is now performed rather than argued away: nearest ancestor holding a configured marker, then every directory from that root down to the home, with a present-but-empty `project_root_markers` disabling detection exactly as upstream does. -What remains uncovered, with a reason that is not "we did not get to it": - -- **Skill and plugin metadata.** Readable, but they are directory trees owned by - Codex, not files with a stable enumeration contract here. Hashing them means - duplicating a loader we do not own and would have to keep in sync. -- **Clock, timezone, shell, MCP availability.** Not files. A fingerprint over a clock - is not a fingerprint. - -The exposure for both is an external edit landing inside a single in-flight probe's -window, whose failure mode is a fail-closed `busy` and a retry, not corruption. - -Given that this section has now been wrong once, the standard it should be held to is -worth stating: an input belongs on this list only when it cannot be read from this -process, not when reading it looks inconvenient. +An eighth round then rejected this section a second time. Skill metadata had been +written off as "a directory tree with no stable enumeration contract"; a live edit to +one `SKILL.md` description moved the probe's rendered output while the fingerprint +stood still. It is a directory listing and one file read per skill. The manifests are +hashed now. + +What remains uncovered: + +- **Plugin manifests and MCP/app availability.** Availability is a live connector + state, not a file this process can stat. +- **Clock, timezone, shell.** Not files. A fingerprint over a clock is not a + fingerprint. + +The exposure is an external edit landing inside a single in-flight probe's window, +whose failure mode is a fail-closed `busy` and a retry, not corruption. + +This section has now been wrong twice, in the same direction both times: something was +called unreadable when it was merely inconvenient to read. The standard that survived +is narrow — an input belongs on this list only when no file on disk determines it. +Anything with a path gets hashed. ## The reader, and why it stopped being a regex diff --git a/src/codex/prompt-layers.ts b/src/codex/prompt-layers.ts index 1723b57d89..8424096e62 100644 --- a/src/codex/prompt-layers.ts +++ b/src/codex/prompt-layers.ts @@ -221,35 +221,80 @@ function probeInstructionFilenames(configBytes: string | null): string[] { * moved no admission key. The pattern was the defect: TOML is not a line format, so * no regex over lines can enumerate what a parser accepts. * - * The module header forbids trusting a JS TOML parser to VERIFY bytes we write, - * because Bun and Rust `toml_edit` disagree on escapes and Codex reads what we wrote. - * That prohibition is about writing. This is a read of two arrays of plain filenames, - * and the failure modes differ in the safe direction: a parse disagreement here can - * only cost a redundant probe, never a corrupted config. A missed spelling costs a - * stale read, which is the defect being fixed. + * The module header's warning about JS TOML parsers does apply here, and a review + * round proved it against an earlier version of this comment that claimed otherwise. + * Bun rejects an entire document containing an integer outside JavaScript's safe + * range, such as `model_context_window = 9223372036854775807`, which Rust accepts as + * an ordinary `i64`. A whole-document parse turned that into BOTH arrays disappearing + * — a worse failure than any single missed spelling, and one the old regex did not + * have. * - * An unparseable file yields no entries. Codex could not load it either, so there is - * no configured value to honour. + * So the parse is the preferred reader, not the only one. When it fails, the scan + * below runs, and it is deliberately loose: it accepts any spelling it recognises and + * over-reports rather than under-reports, because an extra hashed filename costs one + * redundant probe while a missing one costs stale text. */ function rootArrayEntries(configBytes: string | null, key: string): string[] { const value = rootValue(configBytes, key); + if (value === PARSE_FAILED) return scanRootArrayEntries(configBytes, key); if (!Array.isArray(value)) return []; return value.filter((entry): entry is string => typeof entry === "string"); } -/** A root-scope value, or undefined when the key is absent or the file will not parse. */ +/** + * Distinguishes "the parser could not read this file" from "the key is absent". + * Collapsing the two is what made an unrelated large integer silently empty the + * project-document set. + */ +const PARSE_FAILED = Symbol("toml-parse-failed"); + +/** A root-scope value, `undefined` when the key is absent, `PARSE_FAILED` when the file will not parse. */ function rootValue(configBytes: string | null, key: string): unknown { if (configBytes === null) return undefined; let parsed: unknown; try { parsed = Bun.TOML.parse(configBytes); } catch { - return undefined; + return PARSE_FAILED; } - if (typeof parsed !== "object" || parsed === null) return undefined; + if (typeof parsed !== "object" || parsed === null) return PARSE_FAILED; return (parsed as Record)[key]; } +/** + * Fallback reader for a config this parser will not accept but Codex will. + * + * Not a second attempt at being a TOML parser — that approach failed three review + * rounds. It is a deliberately over-eager scan: it takes the first bracketed group for + * the key under either spelling, spans lines, strips comments, and keeps anything that + * decodes. Over-reporting is the safe direction here. + */ +function scanRootArrayEntries(configBytes: string | null, key: string): string[] { + const lines = rootLines(configBytes ?? ""); + const opener = new RegExp(`^\\s*"?${key}"?\\s*=\\s*\\[(.*)$`); + for (let i = 0; i < lines.length; i += 1) { + const m = opener.exec(lines[i]!); + if (!m) continue; + let body = m[1]!.replace(/#.*$/, ""); + for (let j = i; !body.includes("]"); ) { + j += 1; + if (j >= lines.length) return []; + body += lines[j]!.replace(/#.*$/, ""); + } + const out: string[] = []; + for (const raw of body.slice(0, body.indexOf("]")).split(",")) { + const trimmed = raw.trim(); + if (trimmed === "") continue; + const decoded = trimmed.startsWith("'") && trimmed.endsWith("'") && trimmed.length >= 2 + ? trimmed.slice(1, -1) + : decodeBasicString(trimmed); + if (decoded !== null) out.push(decoded); + } + return out; + } + return []; +} + /** * The directories Codex would look in for a project document, given the home the * probe runs in. @@ -299,9 +344,23 @@ function projectRootMarkers(configBytes: string | null): string[] { return rootArrayEntries(configBytes, "project_root_markers").filter(m => m !== ""); } -/** Whether a root-scope key is present at all, regardless of what it holds. */ +/** + * Whether a root-scope key is present at all, regardless of what it holds. + * + * A parse failure is not an answer, so it falls through to the scan rather than + * counting as present: reading `PARSE_FAILED` as "present" would report an empty + * marker list and disable root detection on a config Codex reads fine. + */ function hasRootKey(configBytes: string | null, key: string): boolean { - return rootValue(configBytes, key) !== undefined; + const value = rootValue(configBytes, key); + if (value === PARSE_FAILED) return scanHasRootKey(configBytes, key); + return value !== undefined; +} + +/** Textual presence check, used only when the parser cannot read the file. */ +function scanHasRootKey(configBytes: string | null, key: string): boolean { + const probe = new RegExp(`^\\s*"?${key}"?\\s*=`); + return rootLines(configBytes ?? "").some(line => probe.test(line)); } /** @@ -854,9 +913,45 @@ export function computePromptProbeStateFingerprint(opts?: Paths): string { updateFingerprintField(hash, "doc-bytes", readFileOrNull(path)); } } + for (const path of probeSkillManifests(probeHome)) { + updateFingerprintField(hash, "skill-path", path); + updateFingerprintField(hash, "skill-bytes", readFileOrNull(path)); + } return `sha256:${hash.digest("hex")}`; } +/** + * `SKILL.md` manifests under the home's skills directory. + * + * These were written off as unobservable in an earlier version of this function's + * comment. They are not: Codex reads each manifest's frontmatter and renders its + * description into ``, and a review round demonstrated a live + * description edit changing the probe's output while the fingerprint stood still. + * + * One directory listing plus one `readFileOrNull` per skill, beside a subprocess that + * costs orders of magnitude more. Sorted, because `readdirSync` order is not a + * contract and a digest must not depend on it. + * + * Only the top-level manifest per skill is read. A skill's bundled scripts and + * references do not reach the rendered section, so hashing the whole tree would buy + * redundant invalidations at a real cost on large skill sets. + */ +function probeSkillManifests(home: string): string[] { + const root = join(home, "skills"); + let entries: string[]; + try { + entries = readdirSync(root); + } catch { + return []; + } + const manifests: string[] = []; + for (const entry of entries.sort()) { + const manifest = join(root, entry, "SKILL.md"); + if (existsSync(manifest)) manifests.push(manifest); + } + return manifests; +} + // --------------------------------------------------------------------------- // Writing // --------------------------------------------------------------------------- diff --git a/tests/codex-prompt-route.test.ts b/tests/codex-prompt-route.test.ts index 1cf0907c78..093fc708c9 100644 --- a/tests/codex-prompt-route.test.ts +++ b/tests/codex-prompt-route.test.ts @@ -1349,6 +1349,89 @@ describe("020 coverage completions", () => { expect(promptTextProbeSpawnAttemptsForTests()).toBe(2); }); + /** + * A config Codex reads and this process's TOML parser refuses. `i64` is an ordinary + * TOML integer and Rust accepts it; Bun rejects the whole document because the value + * exceeds JavaScript's safe range. Reading that as "no keys configured" dropped every + * fallback filename at once — worse than the missed spellings the parser was adopted + * to fix, and a failure the earlier textual reader did not have. + */ + test("39. a config this parser rejects still contributes its project documents", async () => { + const fx = fixture([ + "project_doc_fallback_filenames = [\"TEAM.md\"]", + // Valid i64, outside Number.MAX_SAFE_INTEGER. + "model_context_window = 9223372036854775807", + "", + ].join("\n")); + // The premise: this really is unparseable here, so the case cannot silently + // degrade into testing the ordinary parsed path. + expect(() => Bun.TOML.parse(readFileSync(fx.configPath, "utf8"))).toThrow(); + + const teamPath = join(fx.decoyHome, "TEAM.md"); + writeFileSync(teamPath, "old-unparseable", "utf8"); + const startedPath = join(fx.decoyHome, "unparseable-probe-starts.txt"); + const source = [ + `const fs = require("node:fs");`, + `const doc = fs.readFileSync(${JSON.stringify(teamPath)}, "utf8");`, + `fs.appendFileSync(${JSON.stringify(startedPath)}, "1\\n");`, + `const output = JSON.stringify([{type:"message",role:"developer",content:[{type:"input_text",text:"" + doc + ""}]}]);`, + "setTimeout(() => process.stdout.write(output), 200);", + ].join(""); + setPromptTextProbeCommandForTests({ binary: process.execPath, args: ["-e", source] }); + + const beforeEdit = call("GET", "/api/codex-prompt/text", fx); + await waitUntil(() => existsSync(startedPath), "unparseable-config probe start"); + writeFileSync(teamPath, "new-unparseable", "utf8"); + + expect((await call("GET", "/api/codex-prompt/text", fx)).body).toMatchObject({ + ok: false, + detail: "another prompt probe is still finishing; retry shortly", + }); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(1); + expect((await beforeEdit).body.layers.skills.text).toBe("old-unparseable"); + + const fresh = await call("GET", "/api/codex-prompt/text", fx); + expect(fresh.body.layers.skills.text).toBe("new-unparseable"); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(2); + }); + + /** + * A skill's SKILL.md frontmatter is rendered into the skills section, so editing a + * description changes what the probe returns. This was documented as unobservable + * until a review round changed one live and watched the output move while the + * fingerprint stood still. + */ + test("40. editing a SKILL.md manifest invalidates an in-flight text probe", async () => { + const fx = fixture("model = \"x\"\n"); + const manifest = join(fx.decoyHome, "skills", "probe-skill", "SKILL.md"); + mkdirSync(dirname(manifest), { recursive: true }); + writeFileSync(manifest, "---\nname: probe-skill\ndescription: old-skill-text\n---\n", "utf8"); + const startedPath = join(fx.decoyHome, "skill-probe-starts.txt"); + const source = [ + `const fs = require("node:fs");`, + `const doc = fs.readFileSync(${JSON.stringify(manifest)}, "utf8").match(/description: (.*)/)[1];`, + `fs.appendFileSync(${JSON.stringify(startedPath)}, "1\\n");`, + `const output = JSON.stringify([{type:"message",role:"developer",content:[{type:"input_text",text:"" + doc + ""}]}]);`, + "setTimeout(() => process.stdout.write(output), 200);", + ].join(""); + setPromptTextProbeCommandForTests({ binary: process.execPath, args: ["-e", source] }); + + const beforeEdit = call("GET", "/api/codex-prompt/text", fx); + await waitUntil(() => existsSync(startedPath), "skill manifest probe start"); + writeFileSync(manifest, "---\nname: probe-skill\ndescription: new-skill-text\n---\n", "utf8"); + + expect((await call("GET", "/api/codex-prompt/text", fx)).body).toMatchObject({ + ok: false, + detail: "another prompt probe is still finishing; retry shortly", + }); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(1); + expect((await beforeEdit).body.layers.skills.text).toBe("old-skill-text"); + + const fresh = await call("GET", "/api/codex-prompt/text", fx); + expect(fresh.body.layers.skills.text).toBe("new-skill-text"); + expect(promptTextProbeSpawnAttemptsForTests()).toBe(2); + }); + test("24. every ownership state is named, not collapsed into a boolean", async () => { // developerInstructionsOwned:false covers an ABSENT key and an EXTERNAL one, and // a GUI that cannot tell them apart hides its own create affordance from every From 7a2fc0e46f805afd587ef3b49afacb77279f834c Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Sat, 29 Aug 2026 16:41:52 +0900 Subject: [PATCH 16/16] docs(codex): state the fingerprint's residual failure mode correctly Both the function comment and the plan said an uncovered input still fails closed to a busy retry. That is backwards, and a design review caught it: for an input the key does not cover, the key does not move, so the late caller joins and reads the older rendering. Fail-closed busy is what a COVERED input produces. The residual is a stale read of one layer, bounded to a single in-flight window in a read-only view. The comment also still listed skill metadata as unobservable after the previous commit started hashing it. Records why this is a bounded key rather than a total one: hashing every input is closable only against a pinned Codex, since the dependency graph is upstream's and moves independently. The enumeration-free alternative -- admit only when the probe started after the request arrived -- is sound and is written down rather than dismissed; it costs nearly all the coalescing this work exists to provide unless arrivals are batched into cohorts first, which is a different change. Comments and plan only. 160 pass / 0 fail; tsc clean. --- .../130_pr2872_probe_fingerprint.md | 33 +++++++++++++++++-- src/codex/prompt-layers.ts | 18 +++++++--- 2 files changed, 45 insertions(+), 6 deletions(-) diff --git a/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md b/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md index df18a7ded9..06f7e0ec9d 100644 --- a/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md +++ b/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md @@ -130,14 +130,43 @@ What remains uncovered: - **Clock, timezone, shell.** Not files. A fingerprint over a clock is not a fingerprint. -The exposure is an external edit landing inside a single in-flight probe's window, -whose failure mode is a fail-closed `busy` and a retry, not corruption. +The exposure is an external edit landing inside a single in-flight probe's window. Be +precise about the failure mode, because an earlier draft of this sentence got it +backwards: for an input the key does not cover, the key does not move, so the caller +DOES join and DOES receive the older rendering. Fail-closed `busy` is what happens for +a covered input. An uncovered one is a stale read of one layer's text, bounded to that +window, in a read-only inspection view. This section has now been wrong twice, in the same direction both times: something was called unreadable when it was merely inconvenient to read. The standard that survived is narrow — an input belongs on this list only when no file on disk determines it. Anything with a path gets hashed. +## Why this is a bounded key and not a total one + +Nine rounds in, the useful conclusion is about the shape of the specification rather +than any single input. "Hash everything the rendered prompt depends on" is closable +only against a pinned Codex: the dependency graph belongs to Codex, is private, and +moves independently of this repository. A new config field or a changed precedence +upstream silently widens the gap without anything here changing. + +So this is a bounded invalidation key over known local inputs, and the code says that +rather than implying identity. + +A design that needs no enumeration exists and was assessed: admit on TIME, where a +request may join a probe only if the probe started after the request arrived. The +correctness argument holds — such a probe read the filesystem after every write that +completed before the request — and it needs a monotonic in-process ordinal rather than +a clock. It was not adopted because it removes almost all the coalescing that motivated +the work: a probe spawns immediately, so the ordinary second caller arrives after the +start and would always be refused. Recovering both properties means cohort batching — +hold arrivals briefly, spawn once the cohort is closed — which is a different change +from this one. + +That is a real option, not a dismissal, and it belongs to whoever needs a strict +"never older than my arrival" contract. What ships here is the bounded key, which is +strictly better than the revision-only key it replaces. + ## The reader, and why it stopped being a regex Rounds five, six and seven each found another valid TOML spelling the hand-rolled diff --git a/src/codex/prompt-layers.ts b/src/codex/prompt-layers.ts index 8424096e62..04b7c86803 100644 --- a/src/codex/prompt-layers.ts +++ b/src/codex/prompt-layers.ts @@ -857,10 +857,20 @@ export function readPromptLayers(opts?: Paths): PromptLayerSnapshot { * path from the injected config would name a file the probe never reads, which is * a fingerprint that cannot fail rather than evidence. * - * Bounded on purpose: this covers opencodex-managed writes plus the CODEX_HOME - * instruction files. Skill and plugin metadata, MCP availability, and the clock also - * move the rendered prompt and cannot be observed from this process; an external - * edit to one of those, concurrent with an in-flight probe, is still coalescible. + * A BOUNDED invalidation key, not prompt identity. It covers opencodex-managed writes, + * the selected base prompt, the project documents Codex would discover from this home, + * and each skill's manifest. Plugin manifests, live MCP availability, and the clock + * also move the rendered prompt and are not files this process can name. + * + * The distinction is worth stating exactly, because the obvious phrasing is wrong: for + * a COVERED input the key moves and a late caller is refused with `busy`. For an + * UNCOVERED one the key does not move, so a late caller joins and reads the older + * rendering. That is the residual, bounded to one in-flight window in a read-only view. + * + * "Hash every input" is only closable against a pinned Codex — the dependency graph is + * upstream's and moves on its own. An enumeration-free alternative exists (admit only + * when the probe started after the request arrived) and is recorded in the plan; it + * costs the coalescing this work exists to provide unless arrivals are batched first. * See devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md. */ export function computePromptProbeStateFingerprint(opts?: Paths): string {