diff --git a/src/codex/features.ts b/src/codex/features.ts index ce7e77dbc..9a588a8c2 100644 --- a/src/codex/features.ts +++ b/src/codex/features.ts @@ -83,7 +83,23 @@ function readConfigText(configPath?: string): string | null { } } -/** Body lines of a TOML table `[header]` up to (not including) the next table header. */ +/** + * Body lines of a TOML table `[header]` up to (not including) the next table header. + * + * The implementation body is deliberately unchanged by #1295 — only this comment + * is new. The scanner is line-based and string-unaware, so it ends the table at + * the first line matching `/^\s*\[/` even inside a multi-line value. Twenty call + * sites in this file consume its output, most of them by matching a regex + * against the returned text, so widening that text changes what they match. An + * earlier attempt at #1295 made this scanner string-aware and thereby gave + * `getAgentsEnabled`, `getAgentsMaxDepth`, and `getMaxConcurrentThreads` three + * new wrong answers. + * + * The readers that matter for #1295 use a real TOML parse instead (see + * `parsedTomlTable`). This stays as the fallback for documents that do not + * parse, and as the reader for the remaining call sites until they are migrated + * the same way. + */ function tomlTableBody(content: string, header: string): string | null { const lines = content.split("\n"); const escaped = header.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); @@ -116,6 +132,27 @@ export function isMultiAgentV2Enabled(configPath?: string): boolean { export function multiAgentV2EnabledFromConfigText(content: string | null): boolean { if (content === null) return false; + // Prefer a real parse. The hand-written table scanner below cannot distinguish + // an assignment from prose that looks like one — a `"""` value containing the + // line `enabled = true` reads as the key itself — and TOML has enough value + // shapes (multi-line arrays opening on the next line, escapes, comments) that + // each near-miss costs another special case (#1295). + // + // The scanner remains only for a document `Bun.TOML.parse` rejects. That is a + // statement about Bun's parser, not about Codex's — the two are separate + // implementations and no compatibility evidence is claimed here, so a document + // Bun rejects may still be one Codex loads. The fallback is therefore + // best-effort and inherits the ambiguity above. It exists because reporting a + // feature as disabled on account of an unreadable file presents a failure as + // a state. + const parsed = parsedTomlTable(content, "features"); + if (parsed !== null) { + const table = plainTomlRecord(parsed.multi_agent_v2); + if (table !== null) return table.enabled === true; + if (typeof parsed.multi_agent_v2 === "boolean") return parsed.multi_agent_v2; + return false; + } + const table = tomlTableBody(content, "features.multi_agent_v2"); if (table !== null) { const enabled = tomlBoolInBody(table, "enabled"); @@ -139,6 +176,30 @@ export function multiAgentV2EnabledFromConfigText(content: string | null): boole return false; } +function plainTomlRecord(value: unknown): Record | null { + return value !== null && typeof value === "object" && !Array.isArray(value) + ? value as Record + : null; +} + +/** + * A top-level table from a full TOML parse, or null when the document does not + * parse. A parsed document with no such table yields `{}` rather than null: that + * is a real answer ("no keys"), while null means "could not read, fall back". + */ +function parsedTomlTable(content: string, name: string): Record | null { + const toml = (globalThis as { Bun?: { TOML?: { parse(input: string): unknown } } }).Bun?.TOML; + if (!toml) return null; + try { + const root = plainTomlRecord(toml.parse(content)); + if (root === null) return null; + return plainTomlRecord(root[name]) ?? {}; + } catch { + return null; + } +} + + /** * TRUE when the codex `default_mode_request_user_input` feature is enabled in * config.toml — lets a Default-mode session pause and ask the user questions @@ -150,6 +211,11 @@ export function multiAgentV2EnabledFromConfigText(content: string | null): boole export function isDefaultModeRequestUserInputEnabled(configPath?: string): boolean { const content = readConfigText(configPath); if (content === null) return false; + // Same reason as the v2 reader: a `"""` value whose prose contains + // `default_mode_request_user_input = true` is not an assignment, and a raw + // regex over the table body cannot tell the difference (#1295). + const parsed = parsedTomlTable(content, "features"); + if (parsed !== null) return parsed[DEFAULT_MODE_REQUEST_USER_INPUT_FEATURE_KEY] === true; const features = tomlTableBody(content, "features"); if (features === null) return false; return tomlBoolInBody(features, DEFAULT_MODE_REQUEST_USER_INPUT_FEATURE_KEY) === true; @@ -164,6 +230,8 @@ export function isDefaultModeRequestUserInputEnabled(configPath?: string): boole export function hasAgentsMaxThreads(configPath?: string): boolean { const content = readConfigText(configPath); if (content === null) return false; + const parsed = parsedTomlTable(content, "agents"); + if (parsed !== null) return Object.hasOwn(parsed, "max_threads"); const agents = tomlTableBody(content, "agents"); if (agents === null) return false; return /^\s*max_threads\s*=/m.test(agents); @@ -173,6 +241,11 @@ export function hasAgentsMaxThreads(configPath?: string): boolean { export function getAgentsMaxThreads(configPath?: string): number | null { const content = readConfigText(configPath); if (content === null) return null; + const parsed = parsedTomlTable(content, "agents"); + if (parsed !== null) { + const value = parsed.max_threads; + return typeof value === "number" && Number.isInteger(value) && value >= 1 ? value : null; + } const agents = tomlTableBody(content, "agents"); if (agents === null) return null; const m = agents.match(/^\s*max_threads\s*=\s*(\d+)\s*(?:#.*)?$/m); diff --git a/tests/codex-v2-gate.test.ts b/tests/codex-v2-gate.test.ts index 2b377afb8..2c45e9b00 100644 --- a/tests/codex-v2-gate.test.ts +++ b/tests/codex-v2-gate.test.ts @@ -126,6 +126,127 @@ describe("features.ts config reader", () => { expect(isMultiAgentV2Enabled(fixtureConfig("[features]\nmulti_agent = true\n"))).toBe(false); }); + // #1295. Each hazard gets its own test: Bun stops a block at the first failing + // expectation, so bundling them would let a later assertion never run and + // still look covered by an ablation. + + test("#1295: `enabled` after a multi-line basic string with a bracketed line", () => { + // The exact shape `codex features enable` produces — it appends `enabled` at + // the END of the table, so a body cut mid-literal drops precisely that key. + expect(isMultiAgentV2Enabled(fixtureConfig( + '[features.multi_agent_v2]\nhint = """\n[some bracketed first line]\nmore prose\n"""\nenabled = true\n', + ))).toBe(true); + }); + + test("#1295: the same hazard with a literal ''' string", () => { + expect(isMultiAgentV2Enabled(fixtureConfig( + "[features.multi_agent_v2]\nhint = '''\n[literal bracketed]\n'''\nenabled = true\n", + ))).toBe(true); + }); + + test("#1295: key order does not decide the answer", () => { + expect(isMultiAgentV2Enabled(fixtureConfig( + '[features.multi_agent_v2]\nenabled = true\nhint = """\n[bracketed]\n"""\n', + ))).toBe(true); + }); + + test("#1295: prose inside a string is not an assignment", () => { + // The value contains the literal text `enabled = true`, and the table has no + // such key. Any reader that regex-matches raw body text answers `true` here. + expect(isMultiAgentV2Enabled(fixtureConfig( + '[features.multi_agent_v2]\nhint = """\n[prose]\nenabled = true\n"""\n[other]\nvalue = 1\n', + ))).toBe(false); + }); + + test("#1295: a delimiter inside a comment is not a delimiter", () => { + // Must not swallow [other] and read someone else's key. + expect(isMultiAgentV2Enabled(fixtureConfig( + '[features.multi_agent_v2]\n# """\n[other]\nenabled = true\n', + ))).toBe(false); + }); + + test("#1295: an escaped delimiter does not close a multi-line basic string", () => { + expect(isMultiAgentV2Enabled(fixtureConfig( + '[features.multi_agent_v2]\nhint = """\n\\"""\n[bracketed prose]\n"""\nenabled = true\n', + ))).toBe(true); + }); + + test("#1295: a multi-line array's nested rows are not table headers", () => { + expect(isMultiAgentV2Enabled(fixtureConfig( + '[features.multi_agent_v2]\nhint = [\n ["nested"],\n]\nenabled = true\n', + ))).toBe(true); + }); + + test("#1295: an array may open on the line after `=`", () => { + expect(isMultiAgentV2Enabled(fixtureConfig( + '[features.multi_agent_v2]\nhint =\n[\n ["nested"],\n]\nenabled = true\n', + ))).toBe(true); + }); + + test("#1295: `#` inside a string is content, not a comment", () => { + expect(isMultiAgentV2Enabled(fixtureConfig( + '[features.multi_agent_v2]\nhint = "# not a comment"\nenabled = true\n', + ))).toBe(true); + }); + + test("#1295: a header-shaped line inside a string is not the table", () => { + // This document has no real [features.multi_agent_v2] table at all. + expect(isMultiAgentV2Enabled(fixtureConfig( + '[other]\nhint = """\n[features.multi_agent_v2]\nenabled = true\n"""\n', + ))).toBe(false); + }); + + test("#1295: a following table's key is never read as this feature's", () => { + expect(isMultiAgentV2Enabled(fixtureConfig( + '[features.multi_agent_v2]\nhint = """\n[x]\n"""\n\n[other]\nenabled = true\n', + ))).toBe(false); + expect(isMultiAgentV2Enabled(fixtureConfig( + '[features.other]\nenabled = true\n\n[features.multi_agent_v2]\nhint = """\n[x]\n"""\n', + ))).toBe(false); + }); + + test("#1295: an unparseable document still falls back to the scanner", () => { + // A rejected parse must not read as "feature disabled"; the hand-written + // scanner is the fallback so a malformed file degrades rather than lying. + expect(isMultiAgentV2Enabled(fixtureConfig( + '[features.multi_agent_v2]\nenabled = true\nbroken = "unterminated\n', + ))).toBe(true); + }); + + test("#1295: the sibling feature readers do not read prose as an assignment", () => { + // These predate #1295 and had the same defect on `dev`: a `"""` value whose + // text happens to contain the key was read as the key itself. Fixed with the + // same parser-first treatment rather than left inconsistent with the v2 + // reader that shares the file. + expect(isDefaultModeRequestUserInputEnabled(fixtureConfig( + '[features]\nhint = """\ndefault_mode_request_user_input = true\n"""\n', + ))).toBe(false); + expect(isDefaultModeRequestUserInputEnabled(fixtureConfig( + '[features]\ndefault_mode_request_user_input = true\n', + ))).toBe(true); + }); + + test("#1295: [agents] max_threads is read from the parse, not from prose", () => { + expect(getAgentsMaxThreads(fixtureConfig( + '[agents]\nhint = """\nmax_threads = 7\n"""\n', + ))).toBe(null); + expect(hasAgentsMaxThreads(fixtureConfig( + '[agents]\nhint = """\nmax_threads = 7\n"""\n', + ))).toBe(false); + expect(getAgentsMaxThreads(fixtureConfig('[agents]\nmax_threads = 7\n'))).toBe(7); + expect(hasAgentsMaxThreads(fixtureConfig('[agents]\nmax_threads = 7\n'))).toBe(true); + }); + + test("#1295: presence and usability of [agents] max_threads are separate questions", () => { + // hasAgentsMaxThreads gates a codex-rs boot refusal — it must not miss a key + // that is present but unusable, while the getter correctly declines to return + // a value it cannot use. A false negative here is the dangerous direction. + expect(hasAgentsMaxThreads(fixtureConfig('[agents]\nmax_threads = 0\n'))).toBe(true); + expect(getAgentsMaxThreads(fixtureConfig('[agents]\nmax_threads = 0\n'))).toBe(null); + expect(hasAgentsMaxThreads(fixtureConfig('[agents]\nmax_threads = "seven"\n'))).toBe(true); + expect(getAgentsMaxThreads(fixtureConfig('[agents]\nmax_threads = "seven"\n'))).toBe(null); + }); + test("inline table form + absent file/key -> false", () => { expect(isMultiAgentV2Enabled(fixtureConfig("[features]\nmulti_agent_v2 = { enabled = true, tool_namespace = \"agents\" }\n"))).toBe(true); expect(isMultiAgentV2Enabled(fixtureConfig("model = \"gpt-5.5\"\n"))).toBe(false);