From bad1f045b1e76f61fbffd44da1ad4f47d3eabb77 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Wed, 19 Aug 2026 18:38:13 -0400 Subject: [PATCH] fix(#3610): hoist surviving top-level codex config keys to file scope on merge (#3690) * test(#3610): pin top-level key hoisting above the codex managed block * fix(#3610): hoist surviving top-level keys above the codex managed block * chore(#3610): add changeset * fix(#3610): hoist to file scope (before the first table header) with reviewer-driven coverage * chore(#3610): backfill changeset pr number --------- Co-authored-by: sim --- .changeset/bold-moles-squeak.md | 5 ++ bin/install.js | 44 ++++++++++- tests/codex-config.test.cjs | 132 ++++++++++++++++++++++++++++++++ 3 files changed, 179 insertions(+), 2 deletions(-) create mode 100644 .changeset/bold-moles-squeak.md diff --git a/.changeset/bold-moles-squeak.md b/.changeset/bold-moles-squeak.md new file mode 100644 index 000000000..3f55351a3 --- /dev/null +++ b/.changeset/bold-moles-squeak.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3690 +--- +**Upgrading the Codex runtime no longer aborts when a top-level config key sits below the GSD marker** — the regenerated `[agents]` table captured such keys into its scope, so post-write schema validation rejected the merged `config.toml` and the install failed mid-flight. Surviving top-level keys are now hoisted above the managed block, preserving their file scope. (#3610) diff --git a/bin/install.js b/bin/install.js index d329a9089..9ad864406 100755 --- a/bin/install.js +++ b/bin/install.js @@ -6206,6 +6206,31 @@ const __atomicWrittenTmps = hooksSurface.__atomicWrittenTmps; * All writes go through atomicWriteFileSync so a mid-write failure leaves * the original config.toml untouched (#2760 fix 4). */ +/** + * Split TOML content into its leading TOP-LEVEL key lines and everything from + * the first table header onward (#3610). + * + * Top-level keys were file-scoped before a merge. The regenerated GSD block + * opens with a table header (`[agents]`, #2088/ADR-1239 upgrade 2), so placing + * that block ABOVE surviving top-level keys re-scopes them into `[agents]` — + * `validateCodexConfigSchema` then correctly rejects the merged file and the + * install aborts. Hoisting the keys above the block preserves their scope. + * + * Table headers inside multiline strings do not start the "rest" region (the + * record parser already excludes them via startsInMultilineString). + */ +function splitTopLevelKeys(content) { + for (const record of getTomlLineRecords(content)) { + if (record.tableHeader && !record.startsInMultilineString) { + return { + topLevel: content.slice(0, record.start).trim(), + rest: content.slice(record.start).trim(), + }; + } + } + return { topLevel: content.trim(), rest: '' }; +} + function mergeCodexConfig(configPath, gsdBlock) { // Case 1: No config.toml — create fresh if (!fs.existsSync(configPath)) { @@ -6253,10 +6278,25 @@ function mergeCodexConfig(configPath, gsdBlock) { .replace(/^\r?\n# GSD codex_hooks ownership: (?:section|root_dotted)\r?\n/, ''); const afterUser = stripLeakedGsdCodexSections(markerStripped).trim(); + // #3610: top-level keys that survived BELOW the marker were file-scoped + // before this merge; the regenerated block opens with the `[agents]` table + // header, so they must be hoisted to FILE scope or TOML re-scopes them + // into a table. File scope means BEFORE the first table header of the + // pre-marker region too — appending them after a pre-marker table (the + // default real-world layout: user tables precede the marker) would merely + // capture them into THAT table instead of [agents], and the schema + // validator is blind to non-agents tables. + const beforeSplit = before ? splitTopLevelKeys(before) : { topLevel: '', rest: '' }; + const { topLevel: afterTopLevel, rest: afterTables } = splitTopLevelKeys(afterUser); + const parts = []; - if (before) parts.push(before); + const topParts = []; + if (beforeSplit.topLevel) topParts.push(beforeSplit.topLevel); + if (afterTopLevel) topParts.push(afterTopLevel); + if (topParts.length > 0) parts.push(topParts.join(eol + eol)); + if (beforeSplit.rest) parts.push(beforeSplit.rest); parts.push(normalizedGsdBlock); - if (afterUser) parts.push(afterUser); + if (afterTables) parts.push(afterTables); atomicWriteFileSync(configPath, parts.join(eol + eol) + eol); return; } diff --git a/tests/codex-config.test.cjs b/tests/codex-config.test.cjs index 9e7b1e5cb..980a4dec9 100644 --- a/tests/codex-config.test.cjs +++ b/tests/codex-config.test.cjs @@ -2049,6 +2049,138 @@ describe('mergeCodexConfig', () => { assert.ok(content.includes('# first line wins\n[model]\r\nname = "o3"'), 'preserves the existing mixed-EOL model content'); assert.ok(content.includes(`\n\n${GSD_CODEX_MARKER}\n`), 'writes the managed block using the first newline style'); }); + + // ─── #3610: top-level keys below the marker must not be captured by [agents] ── + // + // Since #2088 the managed block opens with a bare `[agents]` table header. On + // upgrade (marker present) the block is regenerated IN PLACE, so a top-level + // key that lived below the marker (e.g. Codex Computer Use's `notify`) would + // parse as an [agents] member — validateCodexConfigSchema correctly rejected + // the merged file and the install aborted mid-flight. + + test('#3610: top-level keys below the marker are hoisted above the managed block and the merged file validates', () => { + const configPath = path.join(tmpDir, 'config.toml'); + fs.writeFileSync( + configPath, + `${GSD_CODEX_MARKER}\n\nnotify = ["x", "turn-ended"]\n\n[features]\nhooks = true\n`, + ); + + mergeCodexConfig(configPath, sampleBlock); + + const content = fs.readFileSync(configPath, 'utf8'); + const schema = validateCodexConfigSchema(content); + assert.ok(schema.ok, `merged config must pass Codex schema validation: ${schema.reason || ''}`); + const notifyIdx = content.indexOf('notify = '); + const agentsIdx = content.indexOf('[agents]'); + assert.ok(notifyIdx !== -1 && agentsIdx !== -1, 'both the key and the agents table must be present'); + assert.ok(notifyIdx < agentsIdx, 'a surviving top-level key must precede the [agents] table header, not parse as its member'); + assert.ok(content.includes('[features]'), 'user tables below the marker are preserved after the block'); + }); + + test('#3610 boundary: fresh install (no marker) with a top-level key still validates unchanged', () => { + const configPath = path.join(tmpDir, 'config.toml'); + fs.writeFileSync(configPath, 'notify = ["x", "turn-ended"]\n\n[features]\nhooks = true\n'); + + mergeCodexConfig(configPath, sampleBlock); + + const schema = validateCodexConfigSchema(fs.readFileSync(configPath, 'utf8')); + assert.ok(schema.ok, `fresh-install merge must validate: ${schema.reason || ''}`); + }); + + test('#3610 boundary: key above the marker is untouched by the hoist', () => { + const configPath = path.join(tmpDir, 'config.toml'); + fs.writeFileSync(configPath, `notify = ["x"]\n\n${GSD_CODEX_MARKER}\n\n[features]\nhooks = true\n`); + + mergeCodexConfig(configPath, sampleBlock); + + const content = fs.readFileSync(configPath, 'utf8'); + const schema = validateCodexConfigSchema(content); + assert.ok(schema.ok, `control merge must validate: ${schema.reason || ''}`); + assert.ok(content.indexOf('notify = ') < content.indexOf(GSD_CODEX_MARKER), 'the pre-marker key stays pre-marker'); + }); + + test('#3610: a multiline top-level value below the marker hoists as one unit', () => { + const configPath = path.join(tmpDir, 'config.toml'); + const multiline = 'notify = [\n "x",\n "turn-ended",\n]'; + fs.writeFileSync(configPath, `${GSD_CODEX_MARKER}\n\n${multiline}\n\n[features]\nhooks = true\n`); + + mergeCodexConfig(configPath, sampleBlock); + + const content = fs.readFileSync(configPath, 'utf8'); + const schema = validateCodexConfigSchema(content); + assert.ok(schema.ok, `multiline hoist must validate: ${schema.reason || ''}`); + const hoistedAt = content.indexOf(multiline); + assert.ok(hoistedAt !== -1, 'the multiline value must survive the hoist intact'); + assert.ok(hoistedAt < content.indexOf('[agents]'), 'the whole multiline value lands above the table header'); + }); + + test('#3610: hoisted keys land at FILE scope even when the pre-marker region ends inside a table', () => { + // The default real-world layout: user tables ABOVE the marker, a top-level + // key below it. Appending the key after the pre-marker tables would merely + // capture it into THOSE tables ([features].notify) — the same defect class, + // silent to validateCodexConfigSchema, which inspects only agents/hooks. + const configPath = path.join(tmpDir, 'config.toml'); + fs.writeFileSync( + configPath, + `[features]\nhooks = true\n\n${GSD_CODEX_MARKER}\n\nnotify = ["x", "turn-ended"]\n\n[profiles.fast]\nmodel = "gpt-5"\n`, + ); + + mergeCodexConfig(configPath, sampleBlock); + + const content = fs.readFileSync(configPath, 'utf8'); + const schema = validateCodexConfigSchema(content); + assert.ok(schema.ok, `merge must validate: ${schema.reason || ''}`); + const parsed = parseTomlToObject(content); + assert.ok(Array.isArray(parsed.notify), 'the surviving key must parse as a top-level array'); + assert.ok(!parsed.features || !('notify' in parsed.features), 'the key must NOT be captured into the pre-marker [features] table'); + assert.ok(content.indexOf('notify = ') < content.indexOf('[features]'), 'file scope means before the FIRST table header, not just above the GSD block'); + }); + + test('#3610: a top-level multiline STRING containing a table-header lookalike hoists intact', () => { + // The record parser must not treat the [looks.like.a.header] line inside + // the """ string as a table header (startsInMultilineString) — the split + // must land after the whole value. + const configPath = path.join(tmpDir, 'config.toml'); + const value = 'banner = """\nnot a [table.header] line\n"""\n'; + fs.writeFileSync(configPath, `${GSD_CODEX_MARKER}\n\n${value}\n[features]\nhooks = true\n`); + + mergeCodexConfig(configPath, sampleBlock); + + const content = fs.readFileSync(configPath, 'utf8'); + const schema = validateCodexConfigSchema(content); + assert.ok(schema.ok, `multiline-string hoist must validate: ${schema.reason || ''}`); + const hoistedAt = content.indexOf(value.trim()); + assert.ok(hoistedAt !== -1, 'the multiline string must survive intact'); + assert.ok(hoistedAt < content.indexOf('[agents]'), 'the whole string value lands above the table header'); + }); + + test('#3610: merging twice is idempotent (the first merge is a fixed point)', () => { + const configPath = path.join(tmpDir, 'config.toml'); + fs.writeFileSync( + configPath, + `[features]\nhooks = true\n\n${GSD_CODEX_MARKER}\n\nnotify = ["x"]\n\n[profiles.fast]\nmodel = "gpt-5"\n`, + ); + + mergeCodexConfig(configPath, sampleBlock); + const once = fs.readFileSync(configPath, 'utf8'); + mergeCodexConfig(configPath, sampleBlock); + assert.strictEqual(fs.readFileSync(configPath, 'utf8'), once, 'the second merge must not move anything'); + }); + + test('#3610: CRLF config with a top-level key below the marker validates', () => { + const configPath = path.join(tmpDir, 'config.toml'); + fs.writeFileSync( + configPath, + `${GSD_CODEX_MARKER}\r\n\r\nnotify = ["x", "turn-ended"]\r\n\r\n[features]\r\nhooks = true\r\n`, + ); + + mergeCodexConfig(configPath, sampleBlock); + + const content = fs.readFileSync(configPath, 'utf8'); + const schema = validateCodexConfigSchema(content); + assert.ok(schema.ok, `CRLF upgrade merge must validate: ${schema.reason || ''}`); + assert.ok(content.indexOf('notify = ') < content.indexOf('[agents]'), 'hoist holds under CRLF'); + }); });