fix(codex): read feature flags from a TOML parse, not a line scanner (#1295)

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<string, unknown> | null {
+  return value !== null && typeof value === "object" && !Array.isArray(value)
+    ? value as Record<string, unknown>
+    : 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<string, unknown> | 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);

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);