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..06f7e0ec9d --- /dev/null +++ b/devlog/_plan/260829_bugpr_lane_h_residual_issues/130_pr2872_probe_fingerprint.md @@ -0,0 +1,196 @@ +# 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 ``. + +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. +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. + +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. + +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. 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 +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 +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/gui/src/pages/codex-set-prompt.tsx b/gui/src/pages/codex-set-prompt.tsx index 46c6f0fc29..2a03e4be08 100644 --- a/gui/src/pages/codex-set-prompt.tsx +++ b/gui/src/pages/codex-set-prompt.tsx @@ -426,10 +426,16 @@ 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; + 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"); + 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 @@ -441,11 +447,12 @@ export default function CodexSetPrompt({ apiBase }: { apiBase: string }) { const body = await res.json() as { ok: boolean; layers?: Record }; if (!cancelled) setLayerText(body); } catch { - // A failed probe is a missing body, not a broken page. + // 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 () => { cancelled = true; }; + return () => { cancelled = true; 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..04b7c86803 100644 --- a/src/codex/prompt-layers.ts +++ b/src/codex/prompt-layers.ts @@ -28,9 +28,10 @@ */ 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"; import { OCX_SECTION_MARKER } from "./injected-marker"; import { durableWrite, @@ -181,6 +182,211 @@ 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; + +/** + * 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. + * + */ +function probeInstructionFilenames(configBytes: string | null): string[] { + const names: string[] = [...PROBE_INSTRUCTION_FILES]; + 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. + * + * 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. + * + * 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. + * + * 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"); +} + +/** + * 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 PARSE_FAILED; + } + 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. + * + * 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. + * + * 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 { + 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)); +} + +/** + * 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`; } @@ -286,10 +492,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")}`; } @@ -634,6 +843,125 @@ 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. + * + * 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. + * + * 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 { + const configBytes = readFileOrNull(activeConfigPath(opts)); + const storeBytes = readFileOrNull(activeStorePath(opts)); + const variants = readBaseVariants(opts); + const selection = resolveBaseSelection(configBytes, variants, opts); + const hash = createHash("sha256"); + updateFingerprintField(hash, "revision", computeRevision(configBytes, storeBytes)); + updateFingerprintField(hash, "selected-base", selection.kind === "variant" ? `variant:${selection.id}` : selection.kind); + 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 { + // 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. + 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. + const probeHome = resolveCodexHomeDir(); + 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)); + } + } + 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/src/codex/prompt-text-probe.ts b/src/codex/prompt-text-probe.ts index 6db8d1e49e..d0926a061e 100644 --- a/src/codex/prompt-text-probe.ts +++ b/src/codex/prompt-text-probe.ts @@ -100,41 +100,236 @@ 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; + promptStateFingerprint: string | null; +} + +interface PromptProbeFlight { + key: string; + controller: AbortController; + result: Promise; + closed: Promise; + waiters: number; + joinable: boolean; + resultSettled: boolean; + settled: boolean; +} + +interface PromptProbeExecution { + result: Promise; + 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; +let probeCloseBarrierForTests: Promise | null = null; + +function commandKey(command: ProbeCommand): string { + return JSON.stringify([ + command.binary, + command.args, + command.cwd, + command.timeoutMs, + command.promptStateFingerprint, + ]); +} + +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 { kind: "failed" }; + const active = activePromptProbe; + 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) { + 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 { kind: "busy" }; +} + +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,20 +377,44 @@ 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, + 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, // 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); - if (raw === null) { - return { ok: false, codexHome, layers: {}, detail: "codex debug prompt-input failed" }; + const command: ProbeCommand = { + binary, + args: probeCommandForTests?.args ?? ["debug", "prompt-input"], + cwd: codexHome, + timeoutMs, + promptStateFingerprint, + }; + const outcome = await runSharedPromptProbe(command, signal); + if (outcome.kind !== "output") { + return { + ok: false, + codexHome, + layers: {}, + 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 @@ -236,3 +455,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..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,7 +321,12 @@ export async function handleCodexPromptRoutes(ctx: ManagementContext): Promise { 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")); + }); }); diff --git a/tests/codex-prompt-route.test.ts b/tests/codex-prompt-route.test.ts index 31f0dfd421..093fc708c9 100644 --- a/tests/codex-prompt-route.test.ts +++ b/tests/codex-prompt-route.test.ts @@ -9,9 +9,14 @@ 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 { + promptTextProbeSpawnAttemptsForTests, + resetPromptTextProbeForTests, + setPromptTextProbeCommandForTests, +} from "../src/codex/prompt-text-probe"; import type { ManagementPrincipal } from "../src/server/management-auth"; import type { OcxConfig } from "../src/types"; @@ -64,6 +69,23 @@ function read(path: string): string | null { return existsSync(path) ? readFileSync(path, "utf8") : null; } +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; + } +} + /** * 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 }); }); @@ -787,7 +810,7 @@ describe("020 coverage completions", () => { 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 () => { @@ -799,6 +822,616 @@ 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("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: "another prompt probe is still finishing; retry shortly", + }); + 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("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); + }); + + /** + * 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");`, + // 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);", + ].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"); + }); + + /** + * 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); + }); + + /** + * 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); + }); + + /** + * 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']" }, + // 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 \", \" \"]" }, + // 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]" }, + ]) { + 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"); + 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); + }); + } + } + + /** + * 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); + }); + + /** + * 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); + }); + + /** + * 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 diff --git a/tests/codex-prompt-text-probe.test.ts b/tests/codex-prompt-text-probe.test.ts index 11f453a6c8..7244802183 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,138 @@ 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("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"); + 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"));`, + "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] }); + 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] }); + 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(blockedBeforeClose.detail).toBe("another prompt probe is still finishing; retry shortly"); + 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); + }); +});