From c9dd4aa95107e64f885fe3a903a48369878e4fbf Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 4 Aug 2026 22:51:27 -0400 Subject: [PATCH] fix(#2940): preserve codex config.toml content after the GSD marker on update (#3067) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#2940): prove gsd-update discards codex config after the GSD marker Failing-first regression for #2940. mergeCodexConfig Case 2 (marker present) preserves content before the marker but discards everything from the marker to EOF, so user/Codex-CLI settings ([model], [mcp_servers.*], [profiles.*]) added after a fresh install are wiped on every update. Rows 1-2 assert trailing user content survives; rows 3-7 cover idempotency, the #2406 leaked-section non-regression, the [agents] namespace, both regions, and the zero-trailing boundary. * fix(#2940): preserve codex config.toml content after the GSD marker on update mergeCodexConfig Case 2 (marker present) preserved content before the marker but discarded everything from the marker to EOF, so user/Codex-CLI settings added after a fresh install ([model], [mcp_servers.*], [profiles.*]) were wiped on every gsd-update. Route the trailing region through the existing stripLeakedGsdCodexSections, which removes GSD's own managed/leaked sections (the bare [agents] table GSD regenerates, legacy [agents.gsd-*], [[agents]]) while preserving genuine user TOML. The marker comment line (and the optional codex_hooks ownership line) is stripped from the trailing region first so it is not duplicated alongside the regenerated block. #2406's de-dup still holds (leaked sections after the marker are still stripped), and re-merge is idempotent. * fix(#2940): use CRLF-safe regex in regression test (lint:ci no-crlf-fragile-split) The Row 7 blank-line assertion used a bare \n which trips local/no-crlf-fragile-split under Windows git-autocrlf. Use \r?\n. * fix(#2940): correct Row 5 to valid single-[agents] TOML shape The isolated adversarial review found Row 5 (bareAgentsAfterMarkerHandled) constructed TWO [agents] tables (the GSD-managed one + a user trailing one), which is invalid TOML — a duplicate table definition. extractCodexUserAgentsScalars reads only the first [agents] and stripLeakedGsdCodexSections removes ALL bare [agents] tables, so the user's max_threads would be silently dropped. The valid, realistic shape is the user folding max_threads INTO the managed [agents] block (which the existing spliceCodexAgentsScalars path preserves) plus a separate trailing [model]. Reword Row 5 to that shape. A duplicate-[agents] input is invalid TOML Codex itself rejects, so it is out of scope for this fix. * chore(#2940): add changeset fragment * chore(#2940): backfill changeset PR number 3067 --------- Co-authored-by: sim --- .changeset/happy-zebras-roar.md | 5 + bin/install.js | 33 ++- ...-2940-codex-config-merge-trailing.test.cjs | 194 ++++++++++++++++++ 3 files changed, 227 insertions(+), 5 deletions(-) create mode 100644 .changeset/happy-zebras-roar.md create mode 100644 tests/issue-2940-codex-config-merge-trailing.test.cjs diff --git a/.changeset/happy-zebras-roar.md b/.changeset/happy-zebras-roar.md new file mode 100644 index 000000000..5d4f07346 --- /dev/null +++ b/.changeset/happy-zebras-roar.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3067 +--- +**Updating GSD on Codex no longer deletes user settings from config.toml** — the config merge preserved content before the GSD marker block but discarded everything after it, so any model preference, MCP server, or profile added after a fresh install was wiped on every update. The merge now preserves genuine user TOML after the block by routing it through the existing section stripper, which removes only GSD-owned sections while keeping user tables, and #2406's leaked-section de-dup still holds. Re-merging is idempotent. diff --git a/bin/install.js b/bin/install.js index 083302556..ed383e3c4 100755 --- a/bin/install.js +++ b/bin/install.js @@ -6322,17 +6322,40 @@ function mergeCodexConfig(configPath, gsdBlock) { const normalizedGsdBlock = mergedGsdBlock.replace(/\r?\n/g, eol); const markerIndex = existing.indexOf(GSD_CODEX_MARKER); - // Case 2: Has GSD marker — truncate and re-append + // Case 2: Has GSD marker — preserve user content on BOTH sides, regenerate the GSD block. + // + // #2940: the marker delimits where GSD's OWN block begins, NOT where every post-marker byte + // is GSD-owned. A fresh install writes the GSD block as the file's entire content, so any + // settings the user or Codex CLI later adds ([model], [mcp_servers.*], [profiles.*]) land + // AFTER the block. The previous truncate-to-marker logic discarded that trailing region on + // every update, destroying user config. The fix routes the trailing region through the + // existing AST-based `stripLeakedGsdCodexSections`, which removes GSD's own managed/leaked + // sections (the bare [agents] table GSD regenerates, legacy [agents.gsd-*], [[agents]]) + // while preserving genuine user TOML — so #2406's de-dup still holds AND user content survives. if (markerIndex !== -1) { let before = existing.substring(0, markerIndex).trimEnd(); if (before) { // Strip any GSD-managed sections that leaked above the marker from previous installs before = stripLeakedGsdCodexSections(before).trimEnd(); - - atomicWriteFileSync(configPath, before + eol + eol + normalizedGsdBlock + eol); - } else { - atomicWriteFileSync(configPath, normalizedGsdBlock + eol); } + // Capture and preserve genuine user content AFTER the GSD-managed region. The whole + // post-marker region is passed through stripLeakedGsdCodexSections: GSD's own previously- + // emitted [agents] table (regenerated above as normalizedGsdBlock) and any leaked sections + // are removed, while user tables ([model], [mcp_servers.*], [profiles.*]) are kept. The + // marker comment line itself (and the optional codex_hooks ownership line right under it) + // is GSD-owned and is stripped from the trailing region so it is not duplicated alongside + // the freshly regenerated block. + const rawAfter = existing.substring(markerIndex); + const markerStripped = rawAfter + .replace(GSD_CODEX_MARKER, '') + .replace(/^\r?\n# GSD codex_hooks ownership: (?:section|root_dotted)\r?\n/, ''); + const afterUser = stripLeakedGsdCodexSections(markerStripped).trim(); + + const parts = []; + if (before) parts.push(before); + parts.push(normalizedGsdBlock); + if (afterUser) parts.push(afterUser); + atomicWriteFileSync(configPath, parts.join(eol + eol) + eol); return; } diff --git a/tests/issue-2940-codex-config-merge-trailing.test.cjs b/tests/issue-2940-codex-config-merge-trailing.test.cjs new file mode 100644 index 000000000..dc1739af0 --- /dev/null +++ b/tests/issue-2940-codex-config-merge-trailing.test.cjs @@ -0,0 +1,194 @@ +'use strict'; +process.env.GSD_TEST_MODE = '1'; + +/** + * Regression test for #2940 — `gsd-update` overwrites `~/.codex/config.toml`, + * removing any user/Codex-CLI settings added after the GSD-managed marker block. + * + * Root cause: `mergeCodexConfig`'s Case 2 (marker present) preserved content + * BEFORE the marker but unconditionally discarded everything from the marker to + * EOF, replacing it with a freshly generated GSD block. Since a fresh install + * writes the GSD block as the file's entire content, any settings the user or + * Codex CLI later adds (`[model]`, `[mcp_servers.*]`, `[profiles.*]`) land AFTER + * the block, and every subsequent update wiped them. + * + * The fix preserves genuine trailing TOML by routing the post-marker region + * through the existing `stripLeakedGsdCodexSections` (which removes GSD's own + * managed/leaked sections while keeping user tables), then re-appending it after + * the regenerated GSD block — without regressing #2406's de-dup. + * + * Matrix: .gsd/bug/fix/2940-codex-config-merge-preserves-trailing-content/50-test-matrix.md + */ + +const { describe, test, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const os = require('node:os'); +const { cleanup } = require('./helpers.cjs'); + +const { + generateCodexConfigBlock, + mergeCodexConfig, + GSD_CODEX_MARKER, +} = require('../bin/install.js'); + +describe('mergeCodexConfig trailing-content preservation (#2940)', () => { + let tmpDir; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2940-merge-')); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + /** A GSD block with one agent (the shape installCodexConfig passes). */ + const block = () => + generateCodexConfigBlock([{ name: 'gsd-executor', description: 'Executes plans' }]); + + test('trailingUserModelSectionPreserved', () => { + // Row 1 (failing-first regression): a config with the GSD block FIRST, then a user + // [model] section after it (the real-world layout — fresh install fills the file, + // user settings land after). Re-merge must preserve [model] byte-for-byte. + const configPath = path.join(tmpDir, 'config.toml'); + const trailing = '[model]\nname = "gpt-5.4"\n'; + // First write: GSD block + user content after it (no content before the marker). + fs.writeFileSync(configPath, block() + '\n' + trailing); + + mergeCodexConfig(configPath, block()); + + const content = fs.readFileSync(configPath, 'utf8'); + assert.ok(content.includes('[model]'), 'user [model] section preserved after re-merge'); + assert.ok(content.includes('name = "gpt-5.4"'), 'user model value preserved verbatim'); + assert.ok(content.includes(GSD_CODEX_MARKER), 'GSD marker still present'); + const markerCount = (content.match(new RegExp(GSD_CODEX_MARKER.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'), 'g')) || []).length; + assert.strictEqual(markerCount, 1, 'exactly one marker (no duplication)'); + assert.ok(content.includes('max_depth ='), 'GSD-managed [agents] block regenerated'); + }); + + test('multipleTrailingTablesPreserved', () => { + // Row 2: multiple trailing user tables ([mcp_servers.*], [profiles.*]). + const configPath = path.join(tmpDir, 'config.toml'); + const trailing = [ + '[mcp_servers.figma]', + 'command = "npx"', + 'args = ["-y", "figma-mcp"]', + '', + '[profiles.dev]', + 'model = "o3"', + 'sandbox_mode = "workspace-write"', + ].join('\n'); + fs.writeFileSync(configPath, block() + '\n' + trailing + '\n'); + + mergeCodexConfig(configPath, block()); + + const content = fs.readFileSync(configPath, 'utf8'); + assert.ok(content.includes('[mcp_servers.figma]'), 'mcp_servers table preserved'); + assert.ok(content.includes('[profiles.dev]'), 'profiles table preserved'); + assert.ok(content.includes('sandbox_mode = "workspace-write"'), 'profile value preserved'); + assert.ok(content.includes(GSD_CODEX_MARKER), 'GSD block regenerated'); + }); + + test('reMergeIsIdempotent', () => { + // Row 3 (acceptance #2): merging the result of a merge again yields identical content. + const configPath = path.join(tmpDir, 'config.toml'); + fs.writeFileSync(configPath, block() + '\n[model]\nname = "o3"\n'); + + mergeCodexConfig(configPath, block()); + const afterFirst = fs.readFileSync(configPath, 'utf8'); + + mergeCodexConfig(configPath, block()); + const afterSecond = fs.readFileSync(configPath, 'utf8'); + + assert.strictEqual(afterSecond, afterFirst, 'second merge is idempotent (no further change)'); + }); + + test('leakedGsdSectionAfterMarkerStillStripped', () => { + // Row 4 (#2406 non-regression): a leaked GSD-managed [agents.gsd-*] section AFTER the + // marker is still REMOVED (not regrown), while genuine user content after it is preserved. + const configPath = path.join(tmpDir, 'config.toml'); + const leakedAndUser = [ + '[agents.gsd-executor]', + 'description = "stale leaked"', + 'config_file = "agents/gsd-executor.toml"', + '', + '[model]', + 'name = "o3"', + ].join('\n'); + fs.writeFileSync(configPath, block() + '\n' + leakedAndUser + '\n'); + + mergeCodexConfig(configPath, block()); + + const content = fs.readFileSync(configPath, 'utf8'); + const gsdStructCount = (content.match(/^\[agents\.gsd-executor\]\s*$/gm) || []).length; + assert.strictEqual(gsdStructCount, 0, 'leaked [agents.gsd-executor] after marker is stripped (not regrown)'); + assert.ok(content.includes('[model]'), 'genuine user [model] after the leaked section still preserved'); + }); + + test('bareAgentsAfterMarkerHandled', () => { + // Row 5: a user AgentsToml scalar (max_threads) the user folded INTO the managed [agents] + // block (the valid, realistic shape — two [agents] tables would be invalid TOML), PLUS a + // separate trailing [model] section. The fix must preserve the user scalar via the existing + // spliceCodexAgentsScalars path AND preserve the trailing [model] via the new trailing-region + // logic, while regenerating exactly one managed [agents] table. + const configPath = path.join(tmpDir, 'config.toml'); + // Simulate: fresh install wrote the GSD block; the user then added max_threads into the + // [agents] table and added a [model] section after it. + const existing = [ + GSD_CODEX_MARKER, + '', + '[agents]', + 'max_depth = 1', + 'max_threads = 4', + '', + '[model]', + 'name = "o3"', + ].join('\n'); + fs.writeFileSync(configPath, existing + '\n'); + + mergeCodexConfig(configPath, block()); + + const content = fs.readFileSync(configPath, 'utf8'); + // The user's max_threads scalar is preserved (spliced into the regenerated managed [agents]); + // there is exactly one [agents] table (the managed one). + assert.ok(content.includes('max_threads = 4'), 'user AgentsToml scalar (max_threads) preserved in managed block'); + const agentsHeaders = (content.match(/^\[agents\]\s*$/gm) || []).length; + assert.strictEqual(agentsHeaders, 1, 'exactly one [agents] table (the managed one)'); + assert.ok(content.includes('max_depth = 1'), 'GSD-managed max_depth still present'); + assert.ok(content.includes('[model]'), 'trailing [model] still preserved'); + }); + + test('beforeAndAfterMarkerBothPreserved', () => { + // Row 6: content both BEFORE and AFTER the marker is preserved; GSD block regenerated once. + const configPath = path.join(tmpDir, 'config.toml'); + const before = '[profiles.work]\nmodel = "gpt-5.4"\n'; + const after = '[mcp_servers.github]\ncommand = "gh-mcp"\n'; + fs.writeFileSync(configPath, before + '\n' + block() + '\n' + after + '\n'); + + mergeCodexConfig(configPath, block()); + + const content = fs.readFileSync(configPath, 'utf8'); + assert.ok(content.includes('[profiles.work]'), 'content before marker preserved'); + assert.ok(content.includes('[mcp_servers.github]'), 'content after marker preserved'); + const markerCount = (content.match(new RegExp(GSD_CODEX_MARKER.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'), 'g')) || []).length; + assert.strictEqual(markerCount, 1, 'exactly one marker'); + }); + + test('noTrailingContentUnchanged', () => { + // Row 7 (zero-trailing boundary): a config with ONLY the GSD block (fresh-install case) + // re-merges to just the regenerated block — no spurious blank-line artifacts introduced + // by the trailing-preservation logic. + const configPath = path.join(tmpDir, 'config.toml'); + fs.writeFileSync(configPath, block() + '\n'); + + mergeCodexConfig(configPath, block()); + + const content = fs.readFileSync(configPath, 'utf8'); + // No spurious trailing blank lines beyond the single trailing newline. Use a CRLF-safe + // pattern (\r?\n) so the assertion holds under Windows git-autocrlf line endings. + assert.ok(!/(?:\r?\n){3,}$/.test(content), 'no spurious run of blank lines at end of file'); + assert.strictEqual(content.trim(), block().trim(), 'content is exactly the regenerated block (whitespace-trimmed)'); + }); +});