Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@

### Fixed

* A search is recognised by substring rather than an exact item type. The guard required exactly `web_search`, while Codex has been seen emitting `web_search_call`; under the previous anchored form a real search was terminated as a tool attempt, which is the opposite of what the allowance is for
* A terminated run names what terminated it. `Worker rejected` carried no indication of the cause, so a legitimate item type the guard did not recognise was indistinguishable from a genuine isolation breach
* A researched run that records no search now reports every distinct item and event kind it emitted. The exact vocabulary Codex uses is not reliably documented, and one real run reports it rather than another round of inference from silence

* `features.web_search_request` is no longer passed. It is deprecated because Codex enables web search by default, so it settled nothing and only earned a deprecation notice. The search mode is the whole control
* Each run's `config.toml` is written with its own search mode rather than resting at `disabled` and relying on the command line to override it. The worker passes the same value, so the two agree and no precedence has to be assumed correct
* A researched run that records no search keeps the tail of Codex's stderr and shows it on the phone. A successful run discarded its diagnostics, which is exactly the run where the silence needed explaining — the deprecation notice that identified this was never visible in the app at all
Expand Down
20 changes: 15 additions & 5 deletions lib/sandbox-runtime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ interface WorkerEnvelope {
diagnostics: string;
research?: boolean;
searchLog?: string[];
/** Every distinct item and event kind the run emitted, reported rather than assumed. */
observedItems?: string[];
/** What terminated the run, named, so a rejection is a diagnosis instead of a guess. */
blockedBy?: string;
}

const baseKey = "sol:sandbox:base";
Expand Down Expand Up @@ -146,18 +150,24 @@ function observedSearches(envelope: WorkerEnvelope): string[] | undefined {

/**
* A successful run discards its diagnostics, because the candidate is what matters. That is exactly
* the run where "research was on and nothing searched" needs explaining, and Codex says why on
* stderr -- a deprecation notice, a rejected setting, a provider message. Keep the tail for that
* one case so the phone can show the reason instead of leaving it to be guessed at.
* the run where "research was on and nothing searched" needs explaining. Keep three things for that
* case: what the run emitted, what stopped it if anything did, and what Codex said on stderr. Every
* previous answer to this question was inferred from silence, and each one was wrong.
*/
export function researchNoteForTests(envelope: WorkerEnvelope): string | undefined {
return researchNote(envelope);
}

function researchNote(envelope: WorkerEnvelope): string | undefined {
if (!envelope.research || (envelope.searchLog || []).length > 0) return undefined;
const text = String(envelope.diagnostics || "").trim();
return text ? text.slice(-1_200) : undefined;
const parts: string[] = [];
if (envelope.blockedBy) parts.push(`The run was stopped by ${envelope.blockedBy}.`);
const items = (envelope.observedItems || []).filter((entry) => typeof entry === "string" && entry.trim());
if (items.length) parts.push(`This run emitted: ${items.map((entry) => entry.slice(0, 60)).join(", ")}.`);
const stderr = String(envelope.diagnostics || "").trim();
if (stderr) parts.push(stderr.slice(-1_000));
const text = parts.join("\n\n").trim();
return text ? text.slice(-1_600) : undefined;
}

export function normalizeCodexEvents(value: string, fallbackAt = Date.now(), scope = "codex"): ReviewEvent[] {
Expand Down
24 changes: 17 additions & 7 deletions sandbox/worker.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@ let stderr = "";
let finalText = "";
let toolAttempt = false;
let malformedEvents = false;
// Every distinct kind of thing this run emitted, and what ended it if anything did. Guessing the
// vocabulary from documentation has been wrong repeatedly; one real run reports it instead.
const observedItems = new Set();
let blockedBy = "";
// Observed searches, counted from the transport stream rather than taken from the model's own
// account of what it did. A run that claims sources it never looked up is visible by comparison.
const searchLog = [];
Expand All @@ -101,17 +105,20 @@ child.stdout.on("data", async (chunk) => {
const itemType = String(event.item?.type || "");
const itemMessage = String(event.item?.message || "");
if (itemType === "error" && itemMessage.includes("--dangerously-bypass-hook-trust")) continue;
// Deny by default. The previous form listed the item types to kill on, which let anything it
// failed to name through: apply_patch, list_dir, view_image, and every tool type Codex adds
// after this was written. A hermetic review is the claim, so the benign set is the one that
// gets enumerated, and an item type nobody anticipated ends the run.
const benignItem = /^(agent_message|reasoning|agent_reasoning(_delta)?|todo_list|plan|error)$/i.test(itemType);
if (itemType || eventType) observedItems.add(itemType ? `item:${itemType}` : `event:${eventType}`);
// Deny by default: the benign set is enumerated, so a tool type nobody anticipated ends the
// run rather than passing unnoticed. Matching is on a substring, because the exact names are
// not reliably documented -- a search has been seen as both `web_search` and `web_search_call`
// -- and an anchored list would terminate the very run it is supposed to permit.
const benignItem = /^(agent_message|reasoning|agent_reasoning|todo_list|plan|error|token_count|usage)/i.test(itemType);
// A live search issues queries and reads the pages they return. Both are the same channel,
// disclosed at approval and counted in the trace.
const searching = research && /^(web_search|web_search_request|web_fetch|browser_search)$/i.test(itemType);
const structural = !itemType && /^(thread\.started|turn\.started|turn\.completed|turn\.failed|error)$/i.test(eventType);
const searching = research && /web_search|web_fetch|browser_search/i.test(itemType);
const structural = !itemType && /^(thread\.|turn\.|item\.|error|session)/i.test(eventType);
if (!benignItem && !searching && !structural) {
toolAttempt = true;
// Naming what stopped the run is the difference between a diagnosis and another guess.
if (!blockedBy) blockedBy = itemType ? `item type "${itemType}"` : `event "${eventType}"`;
child.kill("SIGKILL");
}
if (searching) recordSearch(event, itemType || eventType);
Expand Down Expand Up @@ -155,6 +162,9 @@ const envelope = {
candidate: redactedFinal,
diagnostics: redact(stderr, secrets).slice(-12_000),
research,
// What the run actually emitted, so the next failure is read rather than inferred.
observedItems: [...observedItems].slice(0, 40),
blockedBy,
// The queries leave this sandbox, so the operator sees exactly what left. Phone only, like the
// rest of the candidate record.
searchLog: searchLog.map((entry) => redact(entry, secrets)),
Expand Down
9 changes: 9 additions & 0 deletions tests/core/research.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,15 @@ test("the worker counts a search once across its started and completed events",
const recordSearch = build(searchLog, seenSearches, 50) as (event: unknown, fallback: string) => void;

// One search, reported twice by the same item id.
// Both spellings have been seen for a search item, and the guard matches on a substring for
// exactly that reason: an anchored list would terminate the run it is meant to permit.
const searchGuard = (itemType: string) => /web_search|web_fetch|browser_search/i.test(itemType);
for (const itemType of ["web_search", "web_search_call", "web_search_request", "web_fetch"]) {
assert.ok(searchGuard(itemType), `${itemType} is not recognised as a search`);
}
assert.ok(!searchGuard("command_execution"));
assert.ok(!searchGuard("apply_patch"));

recordSearch({ type: "item.started", item: { id: "ws_1", type: "web_search_call", query: "codex web_search variants" } }, "web_search_call");
recordSearch({ type: "item.completed", item: { id: "ws_1", type: "web_search_call", query: "codex web_search variants" } }, "web_search_call");
assert.deepEqual(searchLog, ["codex web_search variants"]);
Expand Down
28 changes: 27 additions & 1 deletion tests/core/sandbox-runtime.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,32 @@ test("the worker only ever asks Codex for a documented web_search variant", asyn
assert.throws(() => applyWebSearchMode(Buffer.from('web_search = "live"', "utf8"), false), /CONFIG_MISSING_WEB_SEARCH/);
});

test("a stopped run names what stopped it, and a quiet one lists what it emitted", () => {
const base = { version: 1, exitCode: 0, toolAttempt: false, malformedEvents: false, secretLeak: false, candidate: "{}", research: true, searchLog: [] };
const note = (envelope: Record<string, unknown>) => researchNoteForTests({ ...base, ...envelope } as never) || "";

// The guard terminating a run used to produce WORKER_REJECTED with no indication of the cause.
assert.match(note({ blockedBy: 'item type "web_search_call"' }), /stopped by item type "web_search_call"/);

// A run that simply did not search reports its vocabulary, which is how the next unknown item
// type gets identified from one real run instead of another round of guessing.
assert.match(note({ observedItems: ["event:thread.started", "item:agent_message"] }), /emitted: event:thread\.started, item:agent_message/);

// All three signals travel together when all three exist.
const full = note({ blockedBy: 'item type "x"', observedItems: ["item:x"], diagnostics: "codex said something" });
assert.match(full, /stopped by/);
assert.match(full, /emitted/);
assert.match(full, /codex said something/);

// Nothing to report stays undefined rather than an empty box on the phone.
assert.equal(researchNoteForTests({ ...base } as never), undefined);
assert.equal(researchNoteForTests({ ...base, searchLog: ["a query"], diagnostics: "noise" } as never), undefined);
assert.equal(researchNoteForTests({ ...base, research: false, diagnostics: "noise" } as never), undefined);

// Bounded, so a chatty run cannot fill the record.
assert.ok(note({ diagnostics: "x".repeat(9_000), observedItems: ["item:a"] }).length <= 1_600);
});

test("a researched run that searched nothing keeps Codex's own explanation", () => {
const base = { version: 1, exitCode: 0, toolAttempt: false, malformedEvents: false, secretLeak: false, candidate: "{}" };
const note = (envelope: Record<string, unknown>) => researchNoteForTests({ ...base, ...envelope } as never);
Expand All @@ -119,7 +145,7 @@ test("a researched run that searched nothing keeps Codex's own explanation", ()
assert.equal(note({ research: false, diagnostics: "noise" }), undefined);
assert.equal(note({ research: true, searchLog: [], diagnostics: " " }), undefined);
// Bounded, so a chatty run cannot fill the record.
assert.equal(note({ research: true, searchLog: [], diagnostics: "x".repeat(9_000) })?.length, 1_200);
assert.ok((note({ research: true, searchLog: [], diagnostics: "x".repeat(9_000) }) || "").length <= 1_600);
});

test("the worker denies by default, so an unnamed tool type ends the run", () => {
Expand Down