From 7ed05c88115213153f61867f21e8d280fa5a9419 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 24 Apr 2026 18:09:01 -0400 Subject: [PATCH] fix(#2645): emit [[agents]] array-of-tables in Codex config.toml (#2664) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(#2645): emit [[agents]] array-of-tables in Codex config.toml Codex ≥0.116 rejects `[agents.]` map tables with `invalid type: map, expected a sequence`. Switch generateCodexConfigBlock to emit `[[agents]]` array-of-tables with an explicit `name` field per entry. Strip + merge paths now self-heal on reinstall — both the legacy `[agents.gsd-*]` map shape (pre-#2645 configs) and the new `[[agents]]` with `name = "gsd-*"` shape are recognized and replaced, while user-authored `[[agents]]` entries are preserved. Fixes #2645 * fix(#2645): use TOML-aware parser to strip managed [[agents]] sections CodeRabbit flagged that the prior regex-based stripper for [[agents]] array-of-tables only matched headers at column 0 and stopped at any line beginning with `[`. An indented [[agents]] header would not terminate the preceding match, so a managed `gsd-*` block could absorb a following user-authored agent and silently delete it. Replace the ad-hoc regex with the existing TOML-aware section parser (getTomlTableSections + removeContentRanges) so section boundaries are authoritative regardless of indentation. Same logic applies to legacy [agents.gsd-*] map sections. Add a comprehensive mixed-shape test covering multiple GSD entries (both legacy map and new array-of-tables, double- and single-quoted names) interleaved with multiple user-authored agents in both shapes — verifies all GSD entries are stripped and every user entry is preserved. --- bin/install.js | 62 ++++++++++++++++--- tests/codex-config.test.cjs | 115 +++++++++++++++++++++++++++++------- 2 files changed, 150 insertions(+), 27 deletions(-) diff --git a/bin/install.js b/bin/install.js index 991f93259..4cda6efc4 100755 --- a/bin/install.js +++ b/bin/install.js @@ -2066,7 +2066,10 @@ function generateCodexConfigBlock(agents, targetDir) { ]; for (const { name, description } of agents) { - lines.push(`[agents.${name}]`); + // #2645 — Codex schema requires [[agents]] array-of-tables, not [agents.] maps. + // Emitting [agents.] produces `invalid type: map, expected a sequence` on load. + lines.push(`[[agents]]`); + lines.push(`name = ${JSON.stringify(name)}`); lines.push(`description = ${JSON.stringify(description)}`); lines.push(`config_file = "${agentsPrefix}/${name}.toml"`); lines.push(''); @@ -2075,8 +2078,39 @@ function generateCodexConfigBlock(agents, targetDir) { return lines.join('\n'); } +/** + * Strip any managed GSD agent sections from a TOML string. + * + * Handles BOTH shapes so reinstall self-heals broken legacy configs: + * - Legacy: `[agents.gsd-*]` single-keyed map tables (pre-#2645). + * - Current: `[[agents]]` array-of-tables whose `name = "gsd-*"`. + * + * A section runs from its header to the next `[` header or EOF. + */ function stripCodexGsdAgentSections(content) { - return content.replace(/^\[agents\.gsd-[^\]]+\]\n(?:(?!\[)[^\n]*\n?)*/gm, ''); + // Use the TOML-aware section parser so we never absorb adjacent user-authored + // tables — even if their headers are indented or otherwise oddly placed. + const sections = getTomlTableSections(content).filter((section) => { + // Legacy `[agents.gsd-]` map tables (pre-#2645). + if (!section.array && /^agents\.gsd-/.test(section.path)) { + return true; + } + + // Current `[[agents]]` array-of-tables — only strip blocks whose + // `name = "gsd-..."`, preserving user-authored [[agents]] entries. + if (section.array && section.path === 'agents') { + const body = content.slice(section.headerEnd, section.end); + const nameMatch = body.match(/^[ \t]*name[ \t]*=[ \t]*["']([^"']+)["']/m); + return Boolean(nameMatch && /^gsd-/.test(nameMatch[1])); + } + + return false; + }); + + return removeContentRanges( + content, + sections.map(({ start, end }) => ({ start, end })), + ); } /** @@ -2774,13 +2808,27 @@ function isLegacyGsdAgentsSection(body) { function stripLeakedGsdCodexSections(content) { const leakedSections = getTomlTableSections(content) - .filter((section) => - section.path.startsWith('agents.gsd-') || - ( + .filter((section) => { + // Legacy [agents.gsd-] map tables (pre-#2645). + if (!section.array && section.path.startsWith('agents.gsd-')) return true; + + // Legacy bare [agents] table with only the old max_threads/max_depth keys. + if ( + !section.array && section.path === 'agents' && isLegacyGsdAgentsSection(content.slice(section.headerEnd, section.end)) - ) - ); + ) return true; + + // Current [[agents]] array-of-tables whose name is gsd-*. Preserve + // user-authored [[agents]] entries (other names) untouched. + if (section.array && section.path === 'agents') { + const body = content.slice(section.headerEnd, section.end); + const nameMatch = body.match(/^[ \t]*name[ \t]*=[ \t]*["']([^"']+)["']/m); + if (nameMatch && /^gsd-/.test(nameMatch[1])) return true; + } + + return false; + }); if (leakedSections.length === 0) { return content; diff --git a/tests/codex-config.test.cjs b/tests/codex-config.test.cjs index 671b5266e..2eb1704a5 100644 --- a/tests/codex-config.test.cjs +++ b/tests/codex-config.test.cjs @@ -396,16 +396,38 @@ describe('generateCodexConfigBlock', () => { assert.ok(!result.includes('[features]'), 'no features table'); assert.ok(!result.includes('multi_agent'), 'no multi_agent'); assert.ok(!result.includes('default_mode_request_user_input'), 'no request_user_input'); - // Should not have bare [agents] table header (only [agents.gsd-*] sections) + // #2645 — must NOT use the legacy `[agents.]` map shape (causes + // `invalid type: map, expected a sequence` when Codex loads config). + assert.ok(!result.match(/^\[agents\.gsd-/m), 'no legacy [agents.gsd-*] map sections'); + // Should not have bare [agents] table header either. assert.ok(!result.match(/^\[agents\]\s*$/m), 'no bare [agents] table'); assert.ok(!result.includes('max_threads'), 'no max_threads'); assert.ok(!result.includes('max_depth'), 'no max_depth'); }); + test('#2645: emits [[agents]] array-of-tables with name field', () => { + const result = generateCodexConfigBlock(agents); + // One [[agents]] header per agent. + const headerCount = (result.match(/^\[\[agents\]\]\s*$/gm) || []).length; + assert.strictEqual(headerCount, 2, 'one [[agents]] header per agent'); + // Each agent has a name field matching the input name. + assert.ok(result.includes('name = "gsd-executor"'), 'executor has name field'); + assert.ok(result.includes('name = "gsd-planner"'), 'planner has name field'); + }); + + test('#2645: block is a valid TOML array-of-tables shape (no stray map headers)', () => { + const result = generateCodexConfigBlock(agents); + // Strip comment/marker lines and count structural headers. There must be + // no `[agents.X]` or `[agents]` tables mixed in with `[[agents]]`, which + // would trigger the Codex parse error from #2645. + const stray = result.match(/^\[agents(\.[^\]]+)?\]\s*$/gm); + assert.strictEqual(stray, null, 'no map-shaped [agents] or [agents.X] headers present'); + }); + test('includes per-agent sections with relative paths (no targetDir)', () => { const result = generateCodexConfigBlock(agents); - assert.ok(result.includes('[agents.gsd-executor]'), 'has executor section'); - assert.ok(result.includes('[agents.gsd-planner]'), 'has planner section'); + assert.ok(result.includes('name = "gsd-executor"'), 'has executor entry'); + assert.ok(result.includes('name = "gsd-planner"'), 'has planner entry'); assert.ok(result.includes('config_file = "agents/gsd-executor.toml"'), 'relative config_file without targetDir'); assert.ok(result.includes('"Executes plans"'), 'has executor description'); }); @@ -462,12 +484,61 @@ describe('stripGsdFromCodexConfig', () => { assert.ok(!result.includes(GSD_CODEX_MARKER), 'strips marker'); }); - test('removes [agents.gsd-*] sections', () => { + test('removes legacy [agents.gsd-*] map sections (self-heal pre-#2645 configs)', () => { const content = `[agents.gsd-executor]\ndescription = "test"\nconfig_file = "agents/gsd-executor.toml"\n\n[agents.custom-agent]\ndescription = "user agent"\n`; const result = stripGsdFromCodexConfig(content); - assert.ok(!result.includes('[agents.gsd-executor]'), 'removes GSD agent section'); + assert.ok(!result.includes('[agents.gsd-executor]'), 'removes legacy GSD agent map section'); assert.ok(result.includes('[agents.custom-agent]'), 'preserves user agent section'); }); + + test('#2645: removes [[agents]] array-of-tables entries whose name is gsd-*', () => { + const content = `[[agents]]\nname = "gsd-executor"\ndescription = "test"\nconfig_file = "agents/gsd-executor.toml"\n\n[[agents]]\nname = "custom-agent"\ndescription = "user agent"\n`; + const result = stripGsdFromCodexConfig(content); + assert.ok(!/name = "gsd-executor"/.test(result), 'removes managed GSD [[agents]] entry'); + assert.ok(result.includes('name = "custom-agent"'), 'preserves user [[agents]] entry'); + }); + + test('#2645: handles mixed legacy + new shapes and multiple user/gsd entries in one file', () => { + // Multiple GSD entries (both legacy map and new array-of-tables) interleaved + // with multiple user-authored agents in both shapes — none of the user + // entries may be removed and all GSD entries must be stripped. + const content = [ + '[agents.gsd-executor]', + 'description = "legacy gsd"', + 'config_file = "agents/gsd-executor.toml"', + '', + '[agents.custom-legacy]', + 'description = "user legacy"', + '', + '[[agents]]', + 'name = "gsd-planner"', + 'description = "new gsd"', + '', + '[[agents]]', + 'name = "my-helper"', + 'description = "user new"', + '', + '[[agents]]', + "name = 'gsd-debugger'", + 'description = "single-quoted gsd"', + '', + '[[agents]]', + 'name = "another-user"', + 'description = "second user agent"', + '', + ].join('\n'); + const result = stripGsdFromCodexConfig(content); + // All GSD entries removed. + assert.ok(!result.includes('gsd-executor'), 'removes legacy gsd-executor'); + assert.ok(!/name\s*=\s*"gsd-planner"/.test(result), 'removes new gsd-planner'); + assert.ok(!/name\s*=\s*'gsd-debugger'/.test(result), 'removes single-quoted gsd-debugger'); + // All user-authored entries preserved. + assert.ok(result.includes('[agents.custom-legacy]'), 'preserves user legacy [agents.custom-legacy]'); + assert.ok(result.includes('user legacy'), 'preserves user legacy body'); + assert.ok(result.includes('name = "my-helper"'), 'preserves user new [[agents]]'); + assert.ok(result.includes('name = "another-user"'), 'preserves second user [[agents]]'); + assert.ok(result.includes('second user agent'), 'preserves second user body'); + }); }); // ─── mergeCodexConfig ─────────────────────────────────────────────────────────── @@ -494,7 +565,7 @@ describe('mergeCodexConfig', () => { assert.ok(fs.existsSync(configPath), 'file created'); const content = fs.readFileSync(configPath, 'utf8'); assert.ok(content.includes(GSD_CODEX_MARKER), 'has marker'); - assert.ok(content.includes('[agents.gsd-executor]'), 'has agent'); + assert.ok(content.includes('name = "gsd-executor"'), 'has agent'); assert.ok(!content.includes('[features]'), 'no features section'); assert.ok(!content.includes('multi_agent'), 'no multi_agent'); }); @@ -514,7 +585,7 @@ describe('mergeCodexConfig', () => { const content = fs.readFileSync(configPath, 'utf8'); assert.ok(content.includes('[model]'), 'preserves user content'); assert.ok(content.includes('Updated description'), 'has new description'); - assert.ok(content.includes('[agents.gsd-planner]'), 'has new agent'); + assert.ok(content.includes('name = "gsd-planner"'), 'has new agent'); // Verify no duplicate markers const markerCount = (content.match(new RegExp(GSD_CODEX_MARKER.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'), 'g')) || []).length; assert.strictEqual(markerCount, 1, 'exactly one marker'); @@ -529,7 +600,7 @@ describe('mergeCodexConfig', () => { const content = fs.readFileSync(configPath, 'utf8'); assert.ok(content.includes('[model]'), 'preserves user content'); assert.ok(content.includes(GSD_CODEX_MARKER), 'adds marker'); - assert.ok(content.includes('[agents.gsd-executor]'), 'has agent'); + assert.ok(content.includes('name = "gsd-executor"'), 'has agent'); }); test('case 3 with existing [features]: preserves user features, does not inject GSD keys', () => { @@ -543,7 +614,7 @@ describe('mergeCodexConfig', () => { assert.ok(!content.includes('multi_agent'), 'does not inject multi_agent'); assert.ok(!content.includes('default_mode_request_user_input'), 'does not inject request_user_input'); assert.ok(content.includes(GSD_CODEX_MARKER), 'adds marker for agents block'); - assert.ok(content.includes('[agents.gsd-executor]'), 'has agent'); + assert.ok(content.includes('name = "gsd-executor"'), 'has agent'); }); test('case 3 strips existing [agents.gsd-*] sections before appending fresh block', () => { @@ -566,12 +637,14 @@ describe('mergeCodexConfig', () => { mergeCodexConfig(configPath, sampleBlock); const content = fs.readFileSync(configPath, 'utf8'); - const gsdAgentCount = (content.match(/^\[agents\.gsd-executor\]\s*$/gm) || []).length; + const legacyGsdAgentCount = (content.match(/^\[agents\.gsd-executor\]\s*$/gm) || []).length; + const managedAgentCount = (content.match(/name = "gsd-executor"/g) || []).length; const markerCount = (content.match(new RegExp(GSD_CODEX_MARKER.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'), 'g')) || []).length; assert.ok(content.includes('[model]'), 'preserves user content'); assert.ok(content.includes('[agents.custom-agent]'), 'preserves non-GSD agent section'); - assert.strictEqual(gsdAgentCount, 1, 'keeps exactly one GSD agent section'); + assert.strictEqual(legacyGsdAgentCount, 0, 'strips legacy map-shape GSD agent sections'); + assert.strictEqual(managedAgentCount, 1, 'keeps exactly one managed GSD [[agents]] entry'); assert.strictEqual(markerCount, 1, 'adds exactly one marker block'); assert.ok(!/\n{3,}# GSD Agent Configuration/.test(content), 'does not leave extra blank lines before marker block'); }); @@ -598,7 +671,7 @@ describe('mergeCodexConfig', () => { const featuresCount = (content.match(/^\[features\]\s*$/gm) || []).length; assert.strictEqual(featuresCount, 1, 'exactly one [features] section'); assert.ok(content.includes('other_feature = true'), 'preserves user feature keys'); - assert.ok(content.includes('[agents.gsd-executor]'), 'has agent'); + assert.ok(content.includes('name = "gsd-executor"'), 'has agent'); // Verify no duplicate markers const markerCount = (content.match(new RegExp(GSD_CODEX_MARKER.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'), 'g')) || []).length; assert.strictEqual(markerCount, 1, 'exactly one marker'); @@ -615,7 +688,7 @@ describe('mergeCodexConfig', () => { assert.ok(!content.includes('multi_agent'), 'does not inject multi_agent'); assert.ok(!content.includes('default_mode_request_user_input'), 'does not inject request_user_input'); assert.ok(content.includes('other_feature = true'), 'preserves user feature'); - assert.ok(content.includes('[agents.gsd-executor]'), 'has agent from fresh block'); + assert.ok(content.includes('name = "gsd-executor"'), 'has agent from fresh block'); }); test('case 2 strips leaked [agents] and [agents.gsd-*] from before content', () => { @@ -645,7 +718,7 @@ describe('mergeCodexConfig', () => { const content = fs.readFileSync(configPath, 'utf8'); assert.ok(content.includes('child_agents_md = false'), 'preserves user feature keys'); - assert.ok(content.includes('[agents.gsd-executor]'), 'has agent from fresh block'); + assert.ok(content.includes('name = "gsd-executor"'), 'has agent from fresh block'); // Verify the leaked [agents] table header above marker was stripped const markerIndex = content.indexOf(GSD_CODEX_MARKER); const beforeMarker = content.substring(0, markerIndex); @@ -685,7 +758,8 @@ describe('mergeCodexConfig', () => { assert.ok(content.includes('child_agents_md = false'), 'preserves user feature keys'); assert.strictEqual(countMatches(beforeMarker, /^\[agents\]\s*$/gm), 0, 'removes leaked [agents] above marker'); assert.strictEqual(countMatches(beforeMarker, /^\[agents\.gsd-executor\]\s*$/gm), 0, 'removes leaked GSD agent section above marker'); - assert.strictEqual(countMatches(content, /^\[agents\.gsd-executor\]\s*$/gm), 1, 'keeps one managed agent section'); + assert.strictEqual(countMatches(content, /^\[agents\.gsd-executor\]\s*$/gm), 0, 'no legacy map-shape sections remain (all replaced by new [[agents]] block)'); + assert.strictEqual(countMatches(content, /name = "gsd-executor"/g), 1, 'keeps one managed agent entry'); assertUsesOnlyEol(content, '\r\n'); }); @@ -720,7 +794,8 @@ describe('mergeCodexConfig', () => { assert.ok(beforeMarker.includes('[agents]\r\ndefault = "custom-agent"\r\n'), 'preserves user-authored [agents] table'); assert.strictEqual(countMatches(beforeMarker, /^\[agents\.gsd-executor\]\s*$/gm), 0, 'removes leaked GSD agent section above marker'); - assert.strictEqual(countMatches(content, /^\[agents\.gsd-executor\]\s*$/gm), 1, 'keeps one managed agent section in the GSD block'); + assert.strictEqual(countMatches(content, /^\[agents\.gsd-executor\]\s*$/gm), 0, 'no legacy map-shape sections remain'); + assert.strictEqual(countMatches(content, /name = "gsd-executor"/g), 1, 'keeps one managed agent entry in the GSD block'); assertUsesOnlyEol(content, '\r\n'); }); @@ -792,7 +867,7 @@ describe('installCodexConfig (integration)', () => { assert.ok(fs.existsSync(configPath), 'config.toml exists'); const config = fs.readFileSync(configPath, 'utf8'); assert.ok(config.includes(GSD_CODEX_MARKER), 'has GSD marker'); - assert.ok(config.includes('[agents.gsd-executor]'), 'has executor agent'); + assert.ok(config.includes('name = "gsd-executor"'), 'has executor agent'); assert.ok(!config.includes('multi_agent'), 'no feature flags'); // Verify per-agent .toml files @@ -1103,7 +1178,7 @@ describe('Codex install hook configuration (e2e)', () => { assert.strictEqual(countMatches(content, /^\[features\]\s*$/gm), 1, 'keeps one [features] section'); assert.strictEqual(countMatches(content, /^codex_hooks = true$/gm), 1, 'adds one codex_hooks key'); assert.ok(content.indexOf('codex_hooks = true') > content.indexOf('[features]'), 'adds codex_hooks after the existing EOF features header'); - assert.ok(content.indexOf('codex_hooks = true') < content.indexOf('[agents.gsd-codebase-mapper]'), 'keeps codex_hooks before the next real table'); + assert.ok(content.indexOf('codex_hooks = true') < content.indexOf('[[agents]]'), 'keeps codex_hooks before the first managed [[agents]] entry'); assertNoDraftRootKeys(content); }); @@ -1233,7 +1308,7 @@ describe('Codex install hook configuration (e2e)', () => { assert.strictEqual(countMatches(content, /^features\.codex_hooks = true$/gm), 0, 'does not append an invalid dotted codex_hooks key'); assert.strictEqual(countMatches(content, /^\[features\]\s*$/gm), 0, 'does not prepend a features table'); assert.strictEqual(countMatches(content, /gsd-check-update\.js/g), 0, 'does not add the GSD hook block when codex_hooks cannot be enabled safely'); - assert.ok(content.includes('[agents.gsd-executor]'), 'still installs the managed agent block'); + assert.ok(content.includes('name = "gsd-executor"'), 'still installs the managed agent block'); assertNoDraftRootKeys(content); }); @@ -1254,7 +1329,7 @@ describe('Codex install hook configuration (e2e)', () => { assert.strictEqual(countMatches(content, /^features\.codex_hooks = true$/gm), 0, 'does not append an invalid dotted codex_hooks key'); assert.strictEqual(countMatches(content, /^\[features\]\s*$/gm), 0, 'does not prepend a features table'); assert.strictEqual(countMatches(content, /gsd-check-update\.js/g), 0, 'does not add the GSD hook block when codex_hooks cannot be enabled safely'); - assert.ok(content.includes('[agents.gsd-executor]'), 'still installs the managed agent block'); + assert.ok(content.includes('name = "gsd-executor"'), 'still installs the managed agent block'); assertNoDraftRootKeys(content); });