diff --git a/CHANGELOG.md b/CHANGELOG.md index f0b169c35..117d0204b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,19 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). it). Re-run `gsd update` without `--minimal` to expand to the full surface. The install manifest now records `mode: "minimal" | "full"`. (#2762) +### Fixed +- **Codex install no longer corrupts existing `~/.codex/config.toml`** — the installer + now defensively strips legacy `[agents]` (single-bracket) and `[[agents]]` (sequence) + blocks regardless of GSD marker presence (both invalid in current Codex schema), emits + the GSD-managed hook in the user's preferred shape (`[[hooks.]]` namespaced AoT + if any user hook uses it, otherwise top-level `[[hooks]]`), migrates legacy + `[hooks.]` to namespaced AoT, and atomically writes via temp-file + + `renameSync`. A strict TOML parser validates the post-write bytes against the Codex + schema and rejects duplicate keys, repeated table headers, trailing bytes after + values, and unsupported value types. Both pre-write helper failures and write-time + failures restore the pre-install snapshot and abort with a clear error rather than + warn-and-continue. (#2760) + ## [1.38.5] - 2026-04-25 ### Fixed diff --git a/bin/install.js b/bin/install.js index 3757033fa..988713d4a 100755 --- a/bin/install.js +++ b/bin/install.js @@ -2089,6 +2089,10 @@ function generateCodexConfigBlock(agents, targetDir) { /** * Strip any managed GSD agent sections from a TOML string. * + * Used by the uninstall path (`stripGsdFromCodexConfig`). Removes only what GSD + * owns; user-authored `[agents.]` and `[[agents]]` entries are preserved + * so uninstall returns the file to its pre-GSD shape. + * * Handles BOTH shapes so reinstall self-heals configs from all GSD versions: * - Current (#2727): `[agents.gsd-*]` struct tables (Codex 0.120.0+). * - Legacy (#2645): `[[agents]]` array-of-tables whose `name = "gsd-*"`. @@ -2815,25 +2819,32 @@ function isLegacyGsdAgentsSection(body) { } function stripLeakedGsdCodexSections(content) { + // Defensive precedence (#2760): we own the `agents` namespace under our + // managed `gsd-*` names, and the legacy bare-table and sequence forms + // (`[agents]`, `[[agents]]`) are invalid in the current Codex schema — + // they trigger "invalid type: ..., expected struct AgentsToml" and break + // every Codex CLI invocation. They MUST never coexist with the new + // `[agents.]` struct format we now emit, so install-time always + // purges them regardless of GSD marker presence. Users who had legitimate + // user-authored `[[agents]]` entries before are already broken on Codex + // ≥0.124 — purging is the only path to a loadable config. const leakedSections = getTomlTableSections(content) .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; + // ANY bare [agents] single-bracket table — invalid in current Codex + // schema, always purged at install time (#2760). Previously gated + // on `isLegacyGsdAgentsSection`, which missed bare tables holding + // arbitrary user keys (`default = "..."`, etc.) that still produce + // the AgentsToml type error. + if (!section.array && section.path === 'agents') 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; - } + // ANY [[agents]] array-of-tables — invalid in current Codex schema, + // always purged at install time (#2760). Previously gated on + // `name = "gsd-..."` which preserved user-authored entries that are + // themselves rejected by Codex 0.124+. + if (section.array && section.path === 'agents') return true; return false; }); @@ -2862,15 +2873,17 @@ function stripLeakedGsdCodexSections(content) { * [hooks.shell] * command = "..." * - * to the new array-of-tables format: - * [[hooks]] - * type = "shell" + * to the new array-of-tables format. #2760 CR5 finding 3 — emit the + * namespaced AoT shape directly so a mixed flat + namespaced layout never + * arises post-install: + * [[hooks.shell]] * command = "..." * * This function detects any non-array hooks sections in the config and - * converts them to the [[hooks]] format, preserving all key-value pairs and - * user comments. Bare [hooks] container sections (no key-value content) are - * dropped. User-authored [[hooks]] array entries are left untouched. + * converts them to the namespaced `[[hooks.]]` array-of-tables form, + * preserving all key-value pairs and user comments. Bare [hooks] container + * sections (no key-value content) are dropped. User-authored AoT entries are + * left untouched. * * Returns the migrated content, or the original content unchanged if no * legacy hooks sections were found. @@ -2901,8 +2914,15 @@ function migrateCodexHooksMapFormat(content) { const type = section.path.slice('hooks.'.length); const body = content.slice(section.headerEnd, section.end); - // Build [[hooks]] block: type line + original body lines - const block = `[[hooks]]${eol}type = "${type}"${eol}${body}`; + // #2760 CR5 finding 3 — emit the namespaced AoT form directly: + // `[[hooks.]]` (no synthetic `event` field — the namespace IS the + // event). Previously we emitted flat `[[hooks]]\nevent = ""`, + // which produced mixed flat + namespaced layouts when the user already + // had `[[hooks.]]` entries. With every migration emit using the + // namespaced shape, the managed-emit detector + // (`hasUserNamespacedAotHooks`) fires correctly and the install + // converges on a single hook layout. + const block = `[[hooks.${type}]]${eol}${body}`; newHooksBlocks.push(block); } @@ -2955,6 +2975,446 @@ function migrateCodexHooksMapFormat(content) { return result; } +/** + * Detect whether the user already uses the namespaced AoT hooks form + * (`[[hooks.]]`) for the given event in the config. When true, + * the GSD-managed hook block must be emitted in the same shape so it + * coexists cleanly — mixing `[[hooks]]` (flat) with `[[hooks.SessionStart]]` + * (namespaced) in the same file confuses round-trip writers and can + * produce a config that Codex rejects (#2760, defect 3). + */ +function hasUserNamespacedAotHooks(content, event) { + const sections = getTomlTableSections(content); + return sections.some( + (section) => section.array && section.path === `hooks.${event}` + ); +} + +/** + * Parse a TOML value RHS expression starting at index `i` of `text`. + * Returns { value, end } on success or throws on parse failure. + * + * Supports the value forms GSD emits or that real Codex configs commonly use: + * - basic strings ("…" with simple escapes) + * - literal strings ('…') + * - booleans (true / false) + * - integers (optional sign, decimal digits) + * - inline arrays of the above + * - inline tables { k = v, … } + * + * This is intentionally not a complete TOML implementation — it is the + * minimal value grammar required to validate Codex config structure and to + * back behavioral assertions in tests (#2760). + */ +function parseTomlValue(text, i) { + // Skip leading whitespace. + while (i < text.length && (text[i] === ' ' || text[i] === '\t')) { + i += 1; + } + if (i >= text.length) { + throw new Error('expected value, got end of input'); + } + + const ch = text[i]; + + // Basic string + if (ch === '"') { + if (text.startsWith('"""', i)) { + const close = findMultilineBasicStringClose(text, i + 3); + if (close === -1) { + throw new Error('unterminated multi-line basic string'); + } + const raw = text.slice(i + 3, close); + return { value: raw.replace(/^\r?\n/, ''), end: close + 3 }; + } + let j = i + 1; + let out = ''; + while (j < text.length) { + const c = text[j]; + if (c === '\\') { + const next = text[j + 1]; + if (next === 'n') { out += '\n'; j += 2; continue; } + if (next === 't') { out += '\t'; j += 2; continue; } + if (next === 'r') { out += '\r'; j += 2; continue; } + if (next === '\\') { out += '\\'; j += 2; continue; } + if (next === '"') { out += '"'; j += 2; continue; } + if (next === '/') { out += '/'; j += 2; continue; } + // Pass-through unrecognized escape (Codex/GSD don't use these). + out += next === undefined ? '' : next; + j += 2; + continue; + } + if (c === '"') { + return { value: out, end: j + 1 }; + } + out += c; + j += 1; + } + throw new Error('unterminated basic string'); + } + + // Literal string + if (ch === '\'') { + if (text.startsWith('\'\'\'', i)) { + const close = text.indexOf('\'\'\'', i + 3); + if (close === -1) throw new Error('unterminated multi-line literal string'); + return { value: text.slice(i + 3, close).replace(/^\r?\n/, ''), end: close + 3 }; + } + const close = text.indexOf('\'', i + 1); + if (close === -1) throw new Error('unterminated literal string'); + return { value: text.slice(i + 1, close), end: close + 1 }; + } + + // Boolean + if (text.startsWith('true', i) && !/[A-Za-z0-9_-]/.test(text[i + 4] || '')) { + return { value: true, end: i + 4 }; + } + if (text.startsWith('false', i) && !/[A-Za-z0-9_-]/.test(text[i + 5] || '')) { + return { value: false, end: i + 5 }; + } + + // Inline array + if (ch === '[') { + const arr = []; + let j = i + 1; + while (true) { + while (j < text.length && /[\s\r\n]/.test(text[j])) j += 1; + if (j >= text.length) throw new Error('unterminated inline array'); + if (text[j] === ']') return { value: arr, end: j + 1 }; + if (text[j] === '#') { + const nl = text.indexOf('\n', j); + j = nl === -1 ? text.length : nl + 1; + continue; + } + const parsed = parseTomlValue(text, j); + arr.push(parsed.value); + j = parsed.end; + while (j < text.length && /[\s\r\n]/.test(text[j])) j += 1; + if (j < text.length && text[j] === ',') { + j += 1; + continue; + } + while (j < text.length && /[\s\r\n]/.test(text[j])) j += 1; + if (text[j] === ']') return { value: arr, end: j + 1 }; + throw new Error(`expected , or ] in inline array at offset ${j}`); + } + } + + // Inline table + if (ch === '{') { + const obj = {}; + let j = i + 1; + while (true) { + while (j < text.length && /[\s\r\n]/.test(text[j])) j += 1; + if (text[j] === '}') return { value: obj, end: j + 1 }; + const keyMatch = text.slice(j).match(/^([A-Za-z0-9_-]+|"[^"]*"|'[^']*')\s*=\s*/); + if (!keyMatch) throw new Error(`expected key in inline table at offset ${j}`); + let rawKey = keyMatch[1]; + if ((rawKey.startsWith('"') && rawKey.endsWith('"')) || (rawKey.startsWith('\'') && rawKey.endsWith('\''))) { + rawKey = rawKey.slice(1, -1); + } + j += keyMatch[0].length; + const parsed = parseTomlValue(text, j); + obj[rawKey] = parsed.value; + j = parsed.end; + while (j < text.length && /[\s\r\n]/.test(text[j])) j += 1; + if (text[j] === ',') { j += 1; continue; } + if (text[j] === '}') return { value: obj, end: j + 1 }; + throw new Error(`expected , or } in inline table at offset ${j}`); + } + } + + // Number (integer with optional sign). Float / date / time / hex / oct / bin + // are NOT supported — we reject them explicitly instead of silently truncating + // an integer prefix off a `0.5` float or `1979-05-27` date. (#2760 CR4 finding 3) + const numMatch = text.slice(i).match(/^[+-]?\d[\d_]*/); + if (numMatch) { + const after = text[i + numMatch[0].length]; + // Reject: float (`.`, `e`, `E`), date/time (`-`, `:`, `T`, `Z`), or any + // continuation digit/letter that suggests an unsupported numeric form. + if (after !== undefined && /[.eE:\-TZ]/.test(after)) { + throw new Error( + `unsupported TOML value at offset ${i}: floats, dates, and times are not supported (got ${text.slice(i, i + 20)})` + ); + } + const digits = numMatch[0].replace(/_/g, ''); + const n = Number(digits); + if (!Number.isFinite(n)) throw new Error(`invalid number: ${numMatch[0]}`); + return { value: n, end: i + numMatch[0].length }; + } + + throw new Error(`unsupported value at offset ${i}: ${text.slice(i, i + 20)}`); +} + +/** + * Parse TOML content into a JavaScript object. Throws on malformed input. + * + * Handles `[table]`, `[[array.of.tables]]`, dotted key paths, and the value + * forms supported by parseTomlValue. Sufficient for validating Codex config + * structure and for behavioral test assertions in #2760 — not a general + * TOML implementation. + */ +function parseTomlToObject(content) { + const root = {}; + const records = getTomlLineRecords(content); + // Tracks the *object* (not path) that subsequent key=value lines target. + let currentTable = root; + + // #2760 CR5 finding 2 — track shape and definition status of every path so + // we can reject duplicate header redeclarations, shape mismatches, and + // duplicate keys per real TOML 1.0 semantics. Without this, walkPath + // silently reuses existing tables and assignment overwrites existing keys — + // a real TOML parser would refuse the file. + // + // pathShape: dotted path -> 'table' | 'array' | 'inline_parent' | 'key' + // - 'table' — declared via [a.b] + // - 'array' — declared via [[a.b]] (path is the array itself; each + // element is its own implicit table) + // - 'inline_parent' — created implicitly while walking parents + // - 'key' — assigned a scalar value + // declaredHeaders: set of dotted paths explicitly declared via [hdr] (not + // [[arr]]) — used to reject duplicate [a] / [a] sections. + // tableKeys: dotted-path -> Set of keys assigned in that exact + // table instance. For [[arr]] elements we use a per-element marker. + const pathShape = new Map(); + const declaredHeaders = new Set(); + const tableKeys = new Map(); + // currentTableId — string identifier for the current table instance, used + // as the key into tableKeys so that key uniqueness is per-table-instance + // (each [[arr]] element gets its own id). + let currentTableId = '__root__'; + pathShape.set('__root__', 'table'); + tableKeys.set('__root__', new Set()); + + function ensureKeySet(id) { + if (!tableKeys.has(id)) tableKeys.set(id, new Set()); + return tableKeys.get(id); + } + + function walkPath(segments, { creatingArrayElement = false } = {}) { + let node = root; + const parents = segments.slice(0, -1); + const last = segments[segments.length - 1]; + + for (let p = 0; p < parents.length; p += 1) { + const seg = parents[p]; + const partialPath = parents.slice(0, p + 1).join('.'); + if (node[seg] === undefined) { + node[seg] = {}; + if (!pathShape.has(partialPath)) { + pathShape.set(partialPath, 'inline_parent'); + } + } else if (Array.isArray(node[seg])) { + // Walk into the latest element of an array-of-tables. + node = node[seg][node[seg].length - 1]; + continue; + } else if (typeof node[seg] !== 'object' || node[seg] === null) { + throw new Error(`path segment ${seg} is not a table`); + } + node = node[seg]; + } + + const fullPath = segments.join('.'); + + if (creatingArrayElement) { + const existingShape = pathShape.get(fullPath); + if (node[last] === undefined) { + node[last] = []; + pathShape.set(fullPath, 'array'); + } else if (!Array.isArray(node[last])) { + throw new Error( + `duplicate or shape-mismatched table header at ${fullPath}: ` + + `cannot redefine as array of tables (previously seen as ${existingShape || 'table'})` + ); + } else if (existingShape && existingShape !== 'array') { + throw new Error( + `duplicate or shape-mismatched table header at ${fullPath}: ` + + `previously seen as ${existingShape}, cannot extend as array of tables` + ); + } + const elem = {}; + node[last].push(elem); + const elemId = `${fullPath}[${node[last].length - 1}]`; + pathShape.set(elemId, 'array_element'); + tableKeys.set(elemId, new Set()); + currentTableId = elemId; + return elem; + } + + // Plain [table] header. + if (node[last] === undefined) { + node[last] = {}; + pathShape.set(fullPath, 'table'); + declaredHeaders.add(fullPath); + tableKeys.set(fullPath, new Set()); + } else if (Array.isArray(node[last])) { + throw new Error( + `duplicate or shape-mismatched table header at ${fullPath}: ` + + `previously declared as array of tables ([[${fullPath}]]), cannot redeclare as table ([${fullPath}])` + ); + } else if (typeof node[last] !== 'object') { + throw new Error(`cannot redefine ${fullPath} as table`); + } else if (declaredHeaders.has(fullPath)) { + throw new Error( + `duplicate or shape-mismatched table header at ${fullPath}: ` + + `[${fullPath}] declared more than once` + ); + } else { + // Implicitly created earlier (e.g., as a parent path); first explicit + // declaration is allowed. + pathShape.set(fullPath, 'table'); + declaredHeaders.add(fullPath); + if (!tableKeys.has(fullPath)) tableKeys.set(fullPath, new Set()); + } + currentTableId = fullPath; + return node[last]; + } + + for (let idx = 0; idx < records.length; idx += 1) { + const rec = records[idx]; + if (rec.startsInMultilineString) continue; + if (rec.tableHeader) { + const segs = rec.tableHeader.segments; + currentTable = walkPath(segs, { creatingArrayElement: rec.tableHeader.array }); + continue; + } + + const trimmed = rec.text.trim(); + if (trimmed === '' || trimmed.startsWith('#')) continue; + + const equalsIndex = findTomlAssignmentEquals(rec.text); + if (equalsIndex === -1) continue; + + const keyText = rec.text.slice(0, equalsIndex).trim(); + const segments = parseTomlKeyPath(keyText); + if (!segments) { + throw new Error(`invalid TOML key on line ${idx + 1}: ${rec.text}`); + } + + // Value RHS may span multiple lines (inline arrays, multi-line strings, + // inline tables). Parse from the absolute content offset right after `=`. + const valueStartAbs = rec.start + equalsIndex + 1; + const parsed = parseTomlValue(content, valueStartAbs); + + // #2760 CR4 finding 3 — verify the full RHS was consumed. Anything other + // than whitespace + optional # comment between parsed.end and the next + // newline (or EOF) means the parser silently accepted a prefix and + // dropped trailing bytes. Reject so malformed TOML cannot slip past + // "parse before commit" guarantees. + let scan = parsed.end; + while (scan < content.length && (content[scan] === ' ' || content[scan] === '\t')) { + scan += 1; + } + if (scan < content.length && content[scan] !== '\n' && content[scan] !== '\r' && content[scan] !== '#') { + const lineEnd = content.indexOf('\n', scan); + const trailing = content.slice(scan, lineEnd === -1 ? content.length : lineEnd); + throw new Error( + `trailing bytes after value on line ${idx + 1}: ${JSON.stringify(trailing)}` + ); + } + + // Place value into currentTable under dotted key. + // #2760 CR5 finding 2 — reject duplicate keys per real TOML 1.0. Track + // the dotted key against the current table instance id; an exact repeat + // throws. + let target = currentTable; + for (let s = 0; s < segments.length - 1; s += 1) { + const seg = segments[s]; + if (target[seg] === undefined) target[seg] = {}; + else if (typeof target[seg] !== 'object' || Array.isArray(target[seg])) { + throw new Error(`cannot descend into non-table key ${seg}`); + } + target = target[seg]; + } + const finalKey = segments[segments.length - 1]; + const dottedKey = segments.join('.'); + const keySet = ensureKeySet(currentTableId); + if (keySet.has(dottedKey) || Object.prototype.hasOwnProperty.call(target, finalKey)) { + throw new Error( + `duplicate key ${dottedKey} in ${currentTableId === '__root__' ? 'root table' : currentTableId}` + ); + } + keySet.add(dottedKey); + target[finalKey] = parsed.value; + } + + return root; +} + +/** + * Validate that the post-install config.toml matches Codex's expected schema + * (#2760, fix 3). Returns { ok: true } on success, or { ok: false, reason } + * with a human-readable explanation of the offending section. + * + * Strategy: parse the bytes into a structured object first — malformed TOML + * fails validation immediately rather than slipping past a header-only scan. + * Then enforce the schema-shape rules against the parsed structure. + * + * Schema rules enforced: + * - File MUST parse as TOML (no syntax errors). + * - `agents` MUST be a struct table (`[agents.]`) — never a bare + * table value or an array of tables. + * - `hooks.` MUST be an array of tables when present (Codex ≥0.124 + * rejects bare `[hooks.]` single-bracket maps). + */ +function validateCodexConfigSchema(content) { + let parsed; + try { + parsed = parseTomlToObject(content); + } catch (e) { + return { + ok: false, + reason: `TOML parse failed: ${e.message}`, + }; + } + + // Header-shape check: arrays-of-tables are visible in the parsed structure + // (as Array values) but bare-vs-struct distinction for `[agents]` requires + // looking at section headers too — `[agents]` with `default = "x"` parses + // to `{ agents: { default: 'x' } }`, indistinguishable from + // `[agents.foo]` writing into the same shape. Use header sections to + // disambiguate. + const sections = getTomlTableSections(content); + + for (const section of sections) { + if (section.array && section.path === 'agents') { + return { + ok: false, + reason: '[[agents]] sequence form is invalid in current Codex schema (expected [agents.] struct form)', + }; + } + + if (!section.array && section.path === 'agents') { + return { + ok: false, + reason: 'bare [agents] table is invalid in current Codex schema (expected [agents.] struct form)', + }; + } + + if (!section.array && section.path.startsWith('hooks.')) { + return { + ok: false, + reason: `bare [${section.path}] table is invalid in current Codex schema (expected [[${section.path}]] array-of-tables)`, + }; + } + } + + // Structural confirmation against parsed object: any present hooks. + // must be an array. + if (parsed.hooks && typeof parsed.hooks === 'object' && !Array.isArray(parsed.hooks)) { + for (const [event, value] of Object.entries(parsed.hooks)) { + if (!Array.isArray(value)) { + return { + ok: false, + reason: `hooks.${event} must be an array of tables, got ${typeof value}`, + }; + } + } + } + + return { ok: true }; +} + function normalizeCodexHooksLine(line, key) { const leadingWhitespace = line.match(/^\s*/)[0]; const commentStart = findTomlCommentStart(line); @@ -3092,14 +3552,38 @@ function rewriteTomlKeyLines(content, matches, key) { return rewritten; } +/** + * Atomic write — write to .tmp-- first, then renameSync over + * the target. Eliminates the partial-write corruption window: an interrupted + * write leaves the temp file (which we clean up) but never truncates the + * original target. Used for any mutation of Codex config.toml so we cannot + * leave the user with a half-written file (#2760 fix 4). + */ +let __atomicWriteCounter = 0; +function atomicWriteFileSync(target, data, options) { + __atomicWriteCounter += 1; + const tmp = `${target}.tmp-${process.pid}-${__atomicWriteCounter}`; + try { + fs.writeFileSync(tmp, data, options); + fs.renameSync(tmp, target); + } catch (e) { + // Best-effort cleanup of the partial temp file; never mask the real error. + try { fs.rmSync(tmp, { force: true }); } catch (_) { /* ignore */ } + throw e; + } +} + /** * Merge GSD config block into an existing or new config.toml. * Three cases: new file, existing with GSD marker, existing without marker. + * + * All writes go through atomicWriteFileSync so a mid-write failure leaves + * the original config.toml untouched (#2760 fix 4). */ function mergeCodexConfig(configPath, gsdBlock) { // Case 1: No config.toml — create fresh if (!fs.existsSync(configPath)) { - fs.writeFileSync(configPath, gsdBlock + '\n'); + atomicWriteFileSync(configPath, gsdBlock + '\n'); return; } @@ -3115,9 +3599,9 @@ function mergeCodexConfig(configPath, gsdBlock) { // Strip any GSD-managed sections that leaked above the marker from previous installs before = stripLeakedGsdCodexSections(before).trimEnd(); - fs.writeFileSync(configPath, before + eol + eol + normalizedGsdBlock + eol); + atomicWriteFileSync(configPath, before + eol + eol + normalizedGsdBlock + eol); } else { - fs.writeFileSync(configPath, normalizedGsdBlock + eol); + atomicWriteFileSync(configPath, normalizedGsdBlock + eol); } return; } @@ -3130,7 +3614,7 @@ function mergeCodexConfig(configPath, gsdBlock) { content = normalizedGsdBlock + eol; } - fs.writeFileSync(configPath, content); + atomicWriteFileSync(configPath, content); } /** @@ -6412,9 +6896,36 @@ function install(isGlobal, runtime = 'claude') { } if (isCodex && !isMinimalMode(installMode)) { - // Generate Codex config.toml and per-agent .toml files. - // Skipped under --minimal — same rationale as filesystem agents above. - const agentCount = installCodexConfig(targetDir, agentsSrc); + // Capture pre-install snapshot of config.toml before ANY GSD mutation + // (#2760 fix 3). On post-write schema-validation failure OR any throw + // during the mutation sequence (write failure, merge throw, etc.) we + // restore these exact bytes so the user is never left with a broken + // Codex CLI (#2760 fix 4 — extends snapshot coverage to write-failure + // paths, paired with atomic temp-file writes in mergeCodexConfig and + // the final hooks-write below). + const codexConfigPathPreInstall = path.join(targetDir, 'config.toml'); + const codexConfigPreInstallSnapshot = fs.existsSync(codexConfigPathPreInstall) + ? fs.readFileSync(codexConfigPathPreInstall) + : null; + + const restoreCodexSnapshot = () => { + if (codexConfigPreInstallSnapshot !== null) { + try { fs.writeFileSync(codexConfigPathPreInstall, codexConfigPreInstallSnapshot); } + catch (_) { /* best-effort restore — surface the original error */ } + } else if (fs.existsSync(codexConfigPathPreInstall)) { + try { fs.rmSync(codexConfigPathPreInstall); } catch (_) { /* best-effort */ } + } + }; + + let agentCount; + try { + // Generate Codex config.toml and per-agent .toml files. + // Skipped under --minimal — same rationale as filesystem agents above. + agentCount = installCodexConfig(targetDir, agentsSrc); + } catch (e) { + restoreCodexSnapshot(); + throw e; + } console.log(` ${green}✓${reset} Generated config.toml with ${agentCount} agent roles`); console.log(` ${green}✓${reset} Generated ${agentCount} agent .toml config files`); @@ -6454,6 +6965,10 @@ function install(isGlobal, runtime = 'claude') { // Add Codex hooks (SessionStart for update checking) — requires codex_hooks feature flag const configPath = path.join(targetDir, 'config.toml'); + // Use the pre-install snapshot captured before installCodexConfig ran so + // restore returns the file to its true pre-GSD state on validation + // failure (#2760 fix 3) — not to the post-agent-merge state. + const preWriteBackup = codexConfigPreInstallSnapshot; try { let configContent = fs.existsSync(configPath) ? fs.readFileSync(configPath, 'utf-8') : ''; const eol = detectLineEnding(configContent); @@ -6470,13 +6985,22 @@ function install(isGlobal, runtime = 'claude') { const codexHooksFeature = ensureCodexHooksFeature(configContent); configContent = setManagedCodexHooksOwnership(codexHooksFeature.content, codexHooksFeature.ownership); - // Add SessionStart hook for update checking + // Add SessionStart hook for update checking. Default to top-level + // `[[hooks]]` AoT with `event` field — the form GSD has emitted since + // the Codex 0.124 migration (#2637). When the user already uses the + // namespaced AoT form `[[hooks.SessionStart]]` for their own hooks, + // emit our managed entry in that same shape so the two forms don't + // collide on round-trip (#2760, defect 3). const updateCheckScript = path.resolve(targetDir, 'hooks', 'gsd-check-update.js').replace(/\\/g, '/'); - const hookBlock = - `${eol}# GSD Hooks${eol}` + - `[[hooks]]${eol}` + - `event = "SessionStart"${eol}` + - `command = "node ${updateCheckScript}"${eol}`; + const useNamespacedAot = hasUserNamespacedAotHooks(configContent, 'SessionStart'); + const hookBlock = useNamespacedAot + ? `${eol}# GSD Hooks${eol}` + + `[[hooks.SessionStart]]${eol}` + + `command = "node ${updateCheckScript}"${eol}` + : `${eol}# GSD Hooks${eol}` + + `[[hooks]]${eol}` + + `event = "SessionStart"${eol}` + + `command = "node ${updateCheckScript}"${eol}`; // Migrate legacy gsd-update-check entries from prior installs (#1755 followup) // Remove stale hook blocks that used the inverted filename or wrong path. @@ -6488,14 +7012,89 @@ function install(isGlobal, runtime = 'claude') { ); } + // #2760 CR4 finding 2 — Strip ALL existing managed gsd-check-update + // hook blocks (top-level [[hooks]] AND namespaced [[hooks.SessionStart]]) + // BEFORE evaluating the includes guard. Without this, an install that + // already has a legacy flat [[hooks]] block short-circuits the new + // namespaced AoT emit and stays stuck in the mixed layout this fix is + // designed to eliminate. Stripping first means every install converges + // on the right shape regardless of prior state. + if (configContent.includes('gsd-check-update')) { + configContent = configContent.replace( + /(?:\r?\n|^)# GSD Hooks\r?\n\[\[hooks\]\]\r?\nevent = "SessionStart"\r?\ncommand = "node [^\r\n]*gsd-check-update\.js"\r?\n/gm, + (match) => (match.startsWith('\r\n') ? '\r\n' : match.startsWith('\n') ? '\n' : ''), + ); + configContent = configContent.replace( + /(?:\r?\n|^)# GSD Hooks\r?\n\[\[hooks\.SessionStart\]\]\r?\ncommand = "node [^\r\n]*gsd-check-update\.js"\r?\n/gm, + (match) => (match.startsWith('\r\n') ? '\r\n' : match.startsWith('\n') ? '\n' : ''), + ); + } + if (hasEnabledCodexHooksFeature(configContent) && !configContent.includes('gsd-check-update')) { configContent += hookBlock; } - fs.writeFileSync(configPath, configContent, 'utf-8'); + // #2760 fix 3 — post-write schema validation. Parse the bytes we are + // about to commit and assert they match Codex's expected shape. If + // validation fails we restore the pre-install backup and abort so the + // user is never left with a Codex CLI that won't load. + // Test seam: tests can inject `__codexSchemaValidator` to force the + // validator to fail and exercise the restore-and-abort path. + const validatorFn = (typeof module !== 'undefined' && module.exports && module.exports.__codexSchemaValidator) + ? module.exports.__codexSchemaValidator + : validateCodexConfigSchema; + const validation = validatorFn(configContent); + if (!validation.ok) { + restoreCodexSnapshot(); + throw new Error( + `post-write Codex schema validation failed: ${validation.reason}. ` + + `Restored ${preWriteBackup !== null ? 'pre-install backup' : 'empty state'}.` + ); + } + + // Atomic write (#2760 fix 4) — write to a sibling temp file, then + // renameSync over the target. A mid-write failure cannot truncate the + // existing config; the snapshot restore below is a second line of + // defense if even the rename fails. + try { + atomicWriteFileSync(configPath, configContent, 'utf-8'); + } catch (writeErr) { + // #2760 CR4 finding 1 — write failure must be loud and fatal. Wrap + // with a `post-write` prefix the outer catch recognises so install + // aborts with a clear error rather than warn-and-continue (which + // produced "Done!" with no Codex agents configured). + restoreCodexSnapshot(); + const wrapped = new Error( + `post-write Codex install failed: ${writeErr && writeErr.message ? writeErr.message : String(writeErr)}. ` + + `Restored ${preWriteBackup !== null ? 'pre-install backup' : 'empty state'}.` + ); + throw wrapped; + } console.log(` ${green}✓${reset} Configured Codex hooks (SessionStart)`); } catch (e) { - console.warn(` ${yellow}⚠${reset} Could not configure Codex hooks: ${e.message}`); + // #2760 — schema-validation and write failures must be loud and fatal + // so the user is never left with a config Codex refuses to load (or no + // Codex agents configured at all). The pre-install snapshot restore has + // already run for write-side throws via the inner catch above and via + // restoreCodexSnapshot in the validation branch. + if (e && typeof e.message === 'string' && e.message.startsWith('post-write')) { + console.error(` ${red}✗${reset} ${e.message}`); + throw e; + } + // #2760 CR5 finding 1 — pre-write failures (migrateCodexHooksMapFormat, + // ensureCodexHooksFeature, config reads, configContent construction, + // etc.) must ALSO be fatal. Previously this branch downgraded to a + // console.warn, leaving the install to print "Done!" with no Codex + // hooks configured — same defect class as finding 1, different layer. + // Restore the pre-install snapshot and rethrow so the outer install + // pipeline aborts. + restoreCodexSnapshot(); + const wrapped = new Error( + `Codex hook configuration failed (pre-write): ${e && e.message ? e.message : String(e)}. ` + + `Restored ${preWriteBackup !== null ? 'pre-install backup' : 'empty state'}.` + ); + console.error(` ${red}✗${reset} ${wrapped.message}`); + throw wrapped; } return { settingsPath: null, settings: null, statuslineCommand: null, runtime, configDir: targetDir }; @@ -7638,6 +8237,9 @@ if (process.env.GSD_TEST_MODE) { generateCodexConfigBlock, stripGsdFromCodexConfig, migrateCodexHooksMapFormat, + hasUserNamespacedAotHooks, + parseTomlToObject, + validateCodexConfigSchema, mergeCodexConfig, installCodexConfig, readGsdRuntimeProfileResolver, diff --git a/tests/bug-2760-codex-install-defensive.test.cjs b/tests/bug-2760-codex-install-defensive.test.cjs new file mode 100644 index 000000000..790efbb22 --- /dev/null +++ b/tests/bug-2760-codex-install-defensive.test.cjs @@ -0,0 +1,978 @@ +/** + * Regression: issue #2760 — Codex install path corrupts existing config.toml. + * + * Three defects, three fixes (defensive triple): + * + * Defect 3 (confirmed real) — Hooks AoT downgrade. When the user already has + * `[[hooks.SessionStart]]` (namespaced AoT) entries in their config, GSD + * used to append a `[[hooks]]` (top-level AoT) block that confuses + * round-trip writers and produces a config Codex refuses to load. + * Fix: detect the user's preferred shape and emit GSD's hook in the same + * namespaced form so both coexist cleanly. + * + * Defects 1+2 (defensive) — Strip-step robustness. Pre-existing legacy + * `[agents]` (single-bracket) and `[[agents]]` (sequence) blocks are + * invalid in current Codex schema and break Codex even though GSD now + * emits the correct `[agents.]` struct form. Fix: install-time + * stripping always purges these forms regardless of GSD marker presence + * so reinstall self-heals files where the marker was edited out or never + * existed (third-party tools). + * + * Fix 3 (defensive) — Post-write validation. Parse the bytes we are about + * to commit, assert they match Codex's expected schema (no bare/sequence + * `agents`, no bare `hooks.`); on failure, restore the pre-install + * backup and abort so the user never gets a broken Codex CLI. + */ + +// Scope GSD_TEST_MODE to module load only — restore prior value (or unset) so +// downstream tests in the same node process never see test-only behaviour +// leak through (#2760 CR4 finding 5). +const previousGsdTestMode = process.env.GSD_TEST_MODE; +process.env.GSD_TEST_MODE = '1'; + +const { test, describe, beforeEach, afterEach } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('fs'); +const path = require('path'); +const os = require('os'); + +const { + install, + validateCodexConfigSchema, + hasUserNamespacedAotHooks, + stripGsdFromCodexConfig, + installCodexConfig, + parseTomlToObject, +} = require('../bin/install.js'); + +if (previousGsdTestMode === undefined) { + delete process.env.GSD_TEST_MODE; +} else { + process.env.GSD_TEST_MODE = previousGsdTestMode; +} + +function runCodexInstall(codexHome, cwd = path.join(__dirname, '..')) { + const previousCodeHome = process.env.CODEX_HOME; + const previousCwd = process.cwd(); + process.env.CODEX_HOME = codexHome; + try { + process.chdir(cwd); + return install(true, 'codex'); + } finally { + process.chdir(previousCwd); + if (previousCodeHome === undefined) { + delete process.env.CODEX_HOME; + } else { + process.env.CODEX_HOME = previousCodeHome; + } + } +} + +function readCodexConfig(codexHome) { + return fs.readFileSync(path.join(codexHome, 'config.toml'), 'utf8'); +} + +function writeCodexConfig(codexHome, content) { + fs.mkdirSync(codexHome, { recursive: true }); + fs.writeFileSync(path.join(codexHome, 'config.toml'), content, 'utf8'); +} + +describe('#2760 defect 3 — Hooks AoT preservation across install/uninstall/reinstall', () => { + let tmpDir; + let codexHome; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2760-d3-')); + codexHome = path.join(tmpDir, 'codex-home'); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + test('preserves both pre-existing [[hooks.SessionStart]] entries and adds GSD entry in namespaced form', () => { + const userConfig = [ + '[[hooks.SessionStart]]', + 'command = "echo first user hook"', + '', + '[[hooks.SessionStart]]', + 'command = "echo second user hook"', + '', + ].join('\n'); + writeCodexConfig(codexHome, userConfig); + + runCodexInstall(codexHome); + const afterInstall = readCodexConfig(codexHome); + const parsed = parseTomlToObject(afterInstall); + + // hooks.SessionStart must be an array-of-tables (namespaced AoT form). + assert.ok( + parsed.hooks && Array.isArray(parsed.hooks.SessionStart), + 'hooks.SessionStart must be an array-of-tables, got: ' + + (parsed.hooks ? typeof parsed.hooks.SessionStart : 'no hooks table') + ); + + const commands = parsed.hooks.SessionStart.map((entry) => entry.command); + + // Both pre-existing user hook entries survive in the parsed structure. + assert.ok( + commands.includes('echo first user hook'), + 'first user [[hooks.SessionStart]] entry preserved in parsed structure: ' + JSON.stringify(commands) + ); + assert.ok( + commands.includes('echo second user hook'), + 'second user [[hooks.SessionStart]] entry preserved in parsed structure: ' + JSON.stringify(commands) + ); + + // GSD's managed entry is emitted in the same namespaced AoT shape so it + // does not collide with the user's preferred form. + assert.ok( + commands.some((cmd) => typeof cmd === 'string' && /gsd-check-update\.js/.test(cmd)), + 'GSD entry must appear in hooks.SessionStart array (not top-level [[hooks]]): ' + + JSON.stringify(commands) + ); + + // Top-level [[hooks]] AoT must not coexist when namespaced form is in use — + // mixing forms is what produces the round-trip break this fix prevents. + assert.ok( + !Array.isArray(parsed.hooks) || parsed.hooks.length === 0, + 'no top-level [[hooks]] AoT entries when namespaced form is in use' + ); + }); + + test('selects top-level [[hooks]] form when user has no namespaced hooks (status-quo behavior)', () => { + writeCodexConfig(codexHome, ''); + runCodexInstall(codexHome); + const content = readCodexConfig(codexHome); + const parsed = parseTomlToObject(content); + + // Top-level hooks must be an array-of-tables; the GSD entry must be one + // of those tables and carry event = "SessionStart". + assert.ok( + Array.isArray(parsed.hooks), + 'fresh install must produce top-level [[hooks]] AoT, got: ' + typeof parsed.hooks + ); + assert.ok( + parsed.hooks.some((h) => h && h.event === 'SessionStart'), + 'top-level [[hooks]] AoT must contain an entry with event = "SessionStart": ' + + JSON.stringify(parsed.hooks) + ); + }); +}); + +describe('#2760 fix 2 — Strip purges invalid legacy [agents] / [[agents]] regardless of marker', () => { + let tmpDir; + let codexHome; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2760-f2-')); + codexHome = path.join(tmpDir, 'codex-home'); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + test('strips bare [agents] single-bracket block (no GSD marker, arbitrary user keys)', () => { + writeCodexConfig(codexHome, [ + '[agents]', + 'default = "custom-agent"', + 'extra_key = "value"', + '', + '[model]', + 'name = "o3"', + '', + ].join('\n')); + + runCodexInstall(codexHome); + const content = readCodexConfig(codexHome); + const parsed = parseTomlToObject(content); + + // Bare [agents] would have left { default, extra_key } as scalar leaves + // on parsed.agents. After strip + struct emit, every key under agents + // must itself be a table (the gsd-* struct form). + assert.ok( + parsed.agents && typeof parsed.agents === 'object' && !Array.isArray(parsed.agents), + 'agents must be a table-of-tables in parsed structure, got: ' + typeof parsed.agents + ); + assert.equal(parsed.agents.default, undefined, 'bare [agents] default key must be stripped'); + assert.equal(parsed.agents.extra_key, undefined, 'bare [agents] extra_key must be stripped'); + const gsdAgents = Object.keys(parsed.agents).filter((k) => k.startsWith('gsd-')); + assert.ok( + gsdAgents.length > 0 && gsdAgents.every((k) => typeof parsed.agents[k] === 'object'), + 'agents.gsd-* struct form must be present: ' + JSON.stringify(Object.keys(parsed.agents)) + ); + + // User's unrelated [model] section preserved structurally. + assert.ok( + parsed.model && parsed.model.name === 'o3', + 'unrelated user [model] section preserved with name = "o3", got: ' + JSON.stringify(parsed.model) + ); + }); + + test('strips [[agents]] sequence-form block without GSD marker (third-party / marker-edited-out)', () => { + writeCodexConfig(codexHome, [ + '[[agents]]', + 'name = "user-helper"', + 'description = "third-party agent"', + '', + '[[agents]]', + 'name = "another-helper"', + 'description = "second one"', + '', + '[projects."/tmp/x"]', + 'trust_level = "trusted"', + '', + ].join('\n')); + + runCodexInstall(codexHome); + const content = readCodexConfig(codexHome); + const parsed = parseTomlToObject(content); + + // [[agents]] sequence form would parse to Array — after strip it must be + // a table-of-tables with gsd-* struct keys. + assert.ok( + parsed.agents && typeof parsed.agents === 'object' && !Array.isArray(parsed.agents), + 'agents must be a table-of-tables in parsed structure (sequence form must be stripped), got: ' + + (Array.isArray(parsed.agents) ? 'array' : typeof parsed.agents) + ); + const gsdAgents = Object.keys(parsed.agents).filter((k) => k.startsWith('gsd-')); + assert.ok( + gsdAgents.length > 0, + 'agents.gsd-* struct form must be present: ' + JSON.stringify(Object.keys(parsed.agents)) + ); + + // User's unrelated [projects."/tmp/x"] section preserved structurally. + assert.ok( + parsed.projects && parsed.projects['/tmp/x'] && parsed.projects['/tmp/x'].trust_level === 'trusted', + 'unrelated user [projects."/tmp/x"] section preserved with trust_level = "trusted", got: ' + + JSON.stringify(parsed.projects) + ); + }); +}); + +// concurrency: false — the third test mutates installModule.__codexSchemaValidator, +// a module-level test seam. Other tests in this file (and in bug-2153, etc.) +// also call runCodexInstall() and would observe the injected validator if +// node:test ran them in parallel. Serializing this describe block keeps the +// seam mutation invisible to siblings. +describe('#2760 fix 3 — Post-write Codex schema validation', { concurrency: false }, () => { + test('passes a clean config produced by GSD install', () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2760-f3a-')); + try { + const codexHome = path.join(tmpDir, 'codex-home'); + runCodexInstall(codexHome); + const content = readCodexConfig(codexHome); + const result = validateCodexConfigSchema(content); + assert.equal(result.ok, true, 'GSD-emitted config passes schema validation'); + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }); + + test('rejects bare [agents] and bare [hooks.SessionStart] in arbitrary content', () => { + const bareAgents = [ + '[agents]', + 'default = "x"', + '', + ].join('\n'); + const bareHooks = [ + '[hooks.SessionStart]', + 'command = "x"', + '', + ].join('\n'); + const sequenceAgents = [ + '[[agents]]', + 'name = "x"', + '', + ].join('\n'); + + assert.equal(validateCodexConfigSchema(bareAgents).ok, false, 'bare [agents] rejected'); + assert.equal(validateCodexConfigSchema(bareHooks).ok, false, 'bare [hooks.SessionStart] rejected'); + assert.equal(validateCodexConfigSchema(sequenceAgents).ok, false, '[[agents]] sequence rejected'); + }); + + test('aborts install and restores pre-install backup when post-write validation fails', () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2760-f3b-')); + const installModule = require('../bin/install.js'); + try { + const codexHome = path.join(tmpDir, 'codex-home'); + // Pre-install file the user wants protected. + const preInstall = [ + '# user file', + '[model]', + 'name = "o3"', + '', + ].join('\n'); + writeCodexConfig(codexHome, preInstall); + + // Force the post-write validator to fail via the documented test seam. + // This simulates the writer producing legacy-form output that Codex + // would reject — install MUST abort, restore the pre-install bytes, + // and surface a clear error. + installModule.__codexSchemaValidator = () => ({ + ok: false, + reason: 'simulated invalid output for test', + }); + + let threw = false; + try { + runCodexInstall(codexHome); + } catch (e) { + threw = true; + assert.match( + e.message, + /post-write Codex schema validation failed/, + 'thrown error names the validation failure' + ); + assert.match(e.message, /simulated invalid output for test/, 'thrown error includes reason'); + } + assert.equal(threw, true, 'install threw when validator failed'); + + const afterInstall = fs.readFileSync(path.join(codexHome, 'config.toml'), 'utf8'); + assert.equal( + afterInstall, + preInstall, + 'pre-install file restored verbatim after validation failure' + ); + } finally { + delete installModule.__codexSchemaValidator; + fs.rmSync(tmpDir, { recursive: true, force: true }); + } + }); +}); + +describe('#2760 — hasUserNamespacedAotHooks helper', () => { + test('detects [[hooks.SessionStart]] AoT entries', () => { + const content = [ + '[[hooks.SessionStart]]', + 'command = "x"', + '', + ].join('\n'); + assert.equal(hasUserNamespacedAotHooks(content, 'SessionStart'), true); + }); + + test('returns false when only top-level [[hooks]] entries exist', () => { + const content = [ + '[[hooks]]', + 'event = "SessionStart"', + 'command = "x"', + '', + ].join('\n'); + assert.equal(hasUserNamespacedAotHooks(content, 'SessionStart'), false); + }); + + test('returns false when only single-bracket [hooks.SessionStart] exists', () => { + const content = [ + '[hooks.SessionStart]', + 'command = "x"', + '', + ].join('\n'); + assert.equal(hasUserNamespacedAotHooks(content, 'SessionStart'), false); + }); +}); + +// concurrency: false — these tests monkey-patch fs.writeFileSync, a global +// shared with every other suite running in parallel. Serializing prevents +// stray writes from sibling tests landing in the stub. +describe('#2760 fix 4 — Write-failure rollback (atomic write + snapshot restore)', { concurrency: false }, () => { + let tmpDir; + let codexHome; + let originalWriteFileSync; + // #2760 CR5 finding 5 — symmetric snapshot/restore for fs.renameSync. The + // first test below monkey-patches renameSync; without a beforeEach/afterEach + // pair, only the local `finally` restores it, which is fragile to future + // edits that add early-return paths. + let originalRenameSync; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2760-f4-')); + codexHome = path.join(tmpDir, 'codex-home'); + originalWriteFileSync = fs.writeFileSync; + originalRenameSync = fs.renameSync; + }); + + afterEach(() => { + fs.renameSync = originalRenameSync; + fs.writeFileSync = originalWriteFileSync; + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + test('pre-install config bytes survive when fs.renameSync throws over configPath', () => { + const preInstall = [ + '# user file', + '[model]', + 'name = "o3"', + '', + ].join('\n'); + writeCodexConfig(codexHome, preInstall); + + // After fs is restored we'll re-read the file. Capture the byte buffer + // exactly so the comparison is bit-for-bit. + const preInstallBytes = fs.readFileSync(path.join(codexHome, 'config.toml')); + + const configPath = path.join(codexHome, 'config.toml'); + const tempPattern = new RegExp('^' + configPath.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') + '\\.tmp-'); + + // Stub: allow writes to atomic temp files (which renameSync overwrites + // the target, never truncating it directly) but throw on any direct + // write to the canonical configPath. This simulates either: + // (a) an older code path doing a non-atomic write, or + // (b) a downstream module bypassing atomicWriteFileSync. + // Either way the snapshot must be restored. We let the temp write go + // through, then make renameSync throw to simulate the partial write + // never landing. + // #2760 CR5 finding 5 — fs.renameSync is restored by the suite-level + // afterEach; no local finally needed. + fs.renameSync = (src, dst) => { + if (dst === configPath) { + throw new Error('simulated rename failure mid-install'); + } + return originalRenameSync(src, dst); + }; + + let threw = false; + let thrownErr = null; + try { + runCodexInstall(codexHome); + } catch (e) { + threw = true; + thrownErr = e; + assert.ok(/rename failure|simulated|post-write/.test(e.message), + 'thrown error must surface the simulated failure or its post-write wrapper: ' + e.message); + } + // #2760 CR5 finding 4 — tighten contract per finding #1: ALL pre-write + // and write failures must be fatal. This test previously accepted either + // throw OR warn — sibling tests already require throw, so lock parity. + assert.equal(threw, true, 'rename failure must be fatal: ' + (thrownErr && thrownErr.message)); + + const afterBytes = fs.readFileSync(path.join(codexHome, 'config.toml')); + assert.deepStrictEqual( + afterBytes, + preInstallBytes, + 'pre-install config.toml bytes must survive a mid-install write/rename failure' + ); + + // And the parsed structure of the surviving file must still be the + // user's [model] section, not a half-written GSD block. + const parsed = parseTomlToObject(afterBytes.toString('utf8')); + assert.equal(parsed.model && parsed.model.name, 'o3', + 'surviving file must still be the user pre-install content'); + assert.equal(parsed.agents, undefined, + 'no GSD agents block may have leaked into the surviving file'); + + // No stray .tmp-* siblings left behind in the codex home. + const stray = fs.readdirSync(codexHome).filter((f) => tempPattern.test(path.join(codexHome, f))); + assert.equal(stray.length, 0, + 'atomic write must clean up its temp file on failure: ' + stray.join(', ')); + }); + + test('pre-install config bytes survive when fs.writeFileSync throws on the .tmp- target', () => { + const preInstall = [ + '# user file', + '[model]', + 'name = "o3"', + '', + ].join('\n'); + writeCodexConfig(codexHome, preInstall); + + const preInstallBytes = fs.readFileSync(path.join(codexHome, 'config.toml')); + const configPath = path.join(codexHome, 'config.toml'); + const tempPattern = new RegExp('^' + configPath.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') + '\\.tmp-'); + + // Stub: fault writes targeting the atomic temp file (the pre-rename branch + // of atomicWriteFileSync). Other writes (agent .toml files in CODEX_HOME) + // pass through. This exercises the failure path where the temp write itself + // throws, not the rename — the case the prior test left untested. + // #2760 CR5 finding 5 — fs.writeFileSync is restored by the suite-level + // afterEach (via originalWriteFileSync); no local finally needed. + const captured = originalWriteFileSync; + fs.writeFileSync = function patchedWriteFileSync(target, data, options) { + if (typeof target === 'string' && tempPattern.test(target)) { + throw new Error('simulated writeFileSync failure on .tmp- target'); + } + return captured.call(this, target, data, options); + }; + + let threw = false; + try { + runCodexInstall(codexHome); + } catch (e) { + threw = true; + assert.ok(/simulated writeFileSync failure|post-write Codex install failed|pre-write/.test(e.message), + 'thrown error must surface the simulated failure or its post-write wrapper: ' + e.message); + } + // Per #2760 CR4 finding 1 / CR5 finding 1, write failures must abort install (not warn). + assert.equal(threw, true, 'install must throw when atomic temp-write fails'); + + const afterBytes = fs.readFileSync(path.join(codexHome, 'config.toml')); + assert.deepStrictEqual( + afterBytes, + preInstallBytes, + 'pre-install config.toml bytes must survive a temp-write failure' + ); + + const parsed = parseTomlToObject(afterBytes.toString('utf8')); + assert.equal(parsed.model && parsed.model.name, 'o3', + 'surviving file must still be the user pre-install content'); + assert.equal(parsed.agents, undefined, + 'no GSD agents block may have leaked into the surviving file'); + + const stray = fs.readdirSync(codexHome).filter((f) => tempPattern.test(path.join(codexHome, f))); + assert.equal(stray.length, 0, + 'atomic write must clean up its temp file on failure: ' + stray.join(', ')); + }); +}); + +// concurrency: false — these tests rely on the same install path and module- +// level pre-install snapshot that the fix-3/fix-4 suites exercise. Serializing +// keeps state mutations from leaking across parallel siblings. +describe('#2760 CR4 finding 2 — Legacy flat [[hooks]] block migrates to namespaced AoT on reinstall', { concurrency: false }, () => { + let tmpDir; + let codexHome; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2760-cr4-f2-')); + codexHome = path.join(tmpDir, 'codex-home'); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + test('pre-install legacy flat [[hooks]] gsd-check-update + user namespaced [[hooks.SessionStart]] → post-install converges on namespaced AoT', () => { + // Reproduce the upgrade scenario: + // - User has [[hooks.SessionStart]] entry of their own (signal that GSD + // should emit in the namespaced shape). + // - A previous GSD install left the legacy flat [[hooks]] managed block + // for gsd-check-update. The pre-CR4 strip step would short-circuit + // the namespaced emit and leave the user stuck in the mixed layout. + const userPlusLegacy = [ + '[[hooks.SessionStart]]', + 'command = "echo user hook"', + '', + '# GSD Hooks', + '[[hooks]]', + 'event = "SessionStart"', + 'command = "node /old/path/hooks/gsd-check-update.js"', + '', + ].join('\n'); + writeCodexConfig(codexHome, userPlusLegacy); + + runCodexInstall(codexHome); + const afterInstall = readCodexConfig(codexHome); + const parsed = parseTomlToObject(afterInstall); + + // After CR4 finding 2: the legacy flat [[hooks]] managed block is stripped + // and the GSD entry is re-emitted in the namespaced AoT shape so the two + // forms do not coexist. + assert.ok( + parsed.hooks && Array.isArray(parsed.hooks.SessionStart), + 'hooks.SessionStart must be an array-of-tables, got: ' + + (parsed.hooks ? typeof parsed.hooks.SessionStart : 'no hooks table') + ); + + const namespacedCommands = parsed.hooks.SessionStart.map((entry) => entry.command); + assert.ok( + namespacedCommands.includes('echo user hook'), + 'user [[hooks.SessionStart]] entry preserved: ' + JSON.stringify(namespacedCommands) + ); + assert.ok( + namespacedCommands.some((cmd) => typeof cmd === 'string' && /gsd-check-update\.js/.test(cmd)), + 'GSD entry must appear in hooks.SessionStart array (namespaced AoT form): ' + + JSON.stringify(namespacedCommands) + ); + + // The legacy top-level [[hooks]] AoT must NOT coexist with the namespaced + // form after migration. parseTomlToObject distinguishes via Array.isArray. + assert.ok( + !Array.isArray(parsed.hooks) || parsed.hooks.length === 0, + 'no top-level [[hooks]] AoT entries may remain after legacy migration: ' + + JSON.stringify(parsed.hooks) + ); + + // No duplicate gsd-check-update entries — exactly one managed entry. + const gsdEntries = namespacedCommands.filter( + (cmd) => typeof cmd === 'string' && /gsd-check-update\.js/.test(cmd) + ); + assert.equal(gsdEntries.length, 1, + 'exactly one gsd-check-update entry after migration, got: ' + gsdEntries.length); + }); +}); + +describe('#2760 CR4 finding 3 — parseTomlToObject rejects malformed input that previously slipped through', () => { + test('rejects float values (timeout = 0.5)', () => { + const content = [ + '[server]', + 'timeout = 0.5', + '', + ].join('\n'); + assert.throws( + () => parseTomlToObject(content), + /unsupported TOML value|trailing bytes/, + 'float values must be rejected, not silently truncated to int prefix' + ); + }); + + test('rejects date values (created = 1979-05-27)', () => { + const content = [ + '[meta]', + 'created = 1979-05-27', + '', + ].join('\n'); + assert.throws( + () => parseTomlToObject(content), + /unsupported TOML value|trailing bytes/, + 'date values must be rejected, not silently truncated' + ); + }); + + test('rejects trailing garbage after a string value (key = "x" junk)', () => { + const content = [ + '[section]', + 'key = "x" junk', + '', + ].join('\n'); + assert.throws( + () => parseTomlToObject(content), + /trailing bytes/, + 'trailing bytes after a complete value must be rejected' + ); + }); + + test('accepts trailing whitespace and # comment after a value', () => { + const content = [ + '[section]', + 'key = "x" # an inline comment', + 'flag = true', + 'count = 7 ', + '', + ].join('\n'); + const parsed = parseTomlToObject(content); + assert.equal(parsed.section.key, 'x'); + assert.equal(parsed.section.flag, true); + assert.equal(parsed.section.count, 7); + }); +}); + +// concurrency: false — see the fix-3 suite above for the same rationale. +describe('#2760 CR4 finding 1 — atomicWriteFileSync failure aborts install (post-write fatal)', { concurrency: false }, () => { + let tmpDir; + let codexHome; + let originalRenameSync; + let originalConsoleLog; + let consoleOutput; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2760-cr4-f1-')); + codexHome = path.join(tmpDir, 'codex-home'); + originalRenameSync = fs.renameSync; + originalConsoleLog = console.log; + consoleOutput = []; + console.log = (...args) => { consoleOutput.push(args.join(' ')); }; + }); + + afterEach(() => { + fs.renameSync = originalRenameSync; + console.log = originalConsoleLog; + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + test('install throws and never prints "Done!" when atomicWriteFileSync fails on configPath', () => { + const preInstall = [ + '# user file', + '[model]', + 'name = "o3"', + '', + ].join('\n'); + writeCodexConfig(codexHome, preInstall); + + const configPath = path.join(codexHome, 'config.toml'); + // Only fault the hook-block atomic rename — earlier writes to config.toml + // happen via mergeCodexConfig (agent-block emit). We want to exercise the + // post-write Codex install branch specifically. Detect by reading the temp + // file's contents and only faulting when the hook block is present. + fs.renameSync = (src, dst) => { + if (dst === configPath) { + let isHookWrite = false; + try { + const data = fs.readFileSync(src, 'utf8'); + isHookWrite = /gsd-check-update\.js/.test(data); + } catch (_) { /* ignore */ } + if (isHookWrite) { + throw new Error('simulated rename failure'); + } + } + return originalRenameSync(src, dst); + }; + + let threw = false; + let thrownMessage = ''; + try { + runCodexInstall(codexHome); + } catch (e) { + threw = true; + thrownMessage = e.message; + } + + assert.equal(threw, true, 'install must throw when atomic write fails'); + assert.match( + thrownMessage, + /post-write Codex install failed/, + 'thrown error must use the post-write prefix so the outer catch treats it as fatal' + ); + + // Critical: install must NOT have printed any "Done!" success banner. + const printedDone = consoleOutput.some( + (line) => typeof line === 'string' && /Done!/i.test(line) + ); + assert.equal(printedDone, false, + 'install must NOT print "Done!" after a write failure: ' + JSON.stringify(consoleOutput.filter((l) => /Done|✓/.test(l)))); + + // And the user's pre-install bytes are intact (snapshot restore). + const after = fs.readFileSync(configPath, 'utf8'); + assert.equal(after, preInstall, 'pre-install bytes preserved after fatal abort'); + }); +}); + +// concurrency: false — patches module.exports.__codexSchemaValidator, a +// shared test seam. Serializing prevents stray patches from sibling tests. +describe('#2760 CR5 finding 1 — pre-write failures abort install (outer catch fatal)', { concurrency: false }, () => { + let tmpDir; + let codexHome; + let originalConsoleLog; + let consoleOutput; + const installModule = require('../bin/install.js'); + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2760-cr5-f1-')); + codexHome = path.join(tmpDir, 'codex-home'); + originalConsoleLog = console.log; + consoleOutput = []; + console.log = (...args) => { consoleOutput.push(args.join(' ')); }; + }); + + afterEach(() => { + console.log = originalConsoleLog; + delete installModule.__codexSchemaValidator; + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + test('pre-write throw (validator throws, not returns {ok:false}) is fatal and restores snapshot', () => { + // A validator that THROWS (vs returning {ok:false}) bypasses the + // validation branch and exits the inner try via the catch at the outer + // level. Pre-CR5, that catch downgraded to console.warn and let the + // install print "Done!" with no Codex hooks. Post-CR5 it must rethrow. + const preInstall = [ + '# user file', + '[model]', + 'name = "o3"', + '', + ].join('\n'); + writeCodexConfig(codexHome, preInstall); + + installModule.__codexSchemaValidator = () => { + throw new Error('synthetic validator-throw simulating a pre-write helper failure'); + }; + + let threw = false; + let thrownMsg = ''; + try { + runCodexInstall(codexHome); + } catch (e) { + threw = true; + thrownMsg = e.message; + } + + assert.equal(threw, true, + 'install must rethrow when a pre-write step throws (CR5 finding 1)'); + assert.match(thrownMsg, /pre-write|synthetic validator-throw/, + 'thrown error must surface the pre-write wrapper or original message: ' + thrownMsg); + + const printedDone = consoleOutput.some( + (line) => typeof line === 'string' && /Done!/i.test(line) + ); + assert.equal(printedDone, false, + 'install must NOT print "Done!" after a pre-write failure: ' + + JSON.stringify(consoleOutput.filter((l) => /Done|✓/.test(l)))); + + // Pre-install bytes intact (snapshot restored). + const after = fs.readFileSync(path.join(codexHome, 'config.toml'), 'utf8'); + assert.equal(after, preInstall, + 'pre-install bytes must survive a pre-write helper throw'); + }); +}); + +describe('#2760 CR5 finding 2 — parseTomlToObject rejects duplicate keys and shape-mismatched headers', () => { + test('rejects duplicate scalar key in same table ([a]\\nx=1\\nx=2)', () => { + const content = [ + '[a]', + 'x = 1', + 'x = 2', + '', + ].join('\n'); + assert.throws( + () => parseTomlToObject(content), + /duplicate key/, + 'real TOML 1.0 rejects duplicate keys in the same table' + ); + }); + + test('rejects duplicate scalar key in root table', () => { + const content = [ + 'x = 1', + 'x = 2', + '', + ].join('\n'); + assert.throws( + () => parseTomlToObject(content), + /duplicate key/, + 'duplicate root-table keys must be rejected' + ); + }); + + test('rejects re-declared [a] table header ([a] then [a] again)', () => { + const content = [ + '[a]', + 'x = 1', + '', + '[a]', + 'y = 2', + '', + ].join('\n'); + assert.throws( + () => parseTomlToObject(content), + /duplicate or shape-mismatched table header/, + 'real TOML 1.0 rejects re-declaring the same [a] header twice' + ); + }); + + test('rejects [[arr]] then [arr] for same path (array-of-tables → table)', () => { + const content = [ + '[[arr]]', + 'x = 1', + '', + '[arr]', + 'y = 2', + '', + ].join('\n'); + assert.throws( + () => parseTomlToObject(content), + /duplicate or shape-mismatched table header/, + 'cannot redeclare an array-of-tables path as a plain table' + ); + }); + + test('accepts repeated [[arr]] (genuine array-of-tables)', () => { + const content = [ + '[[arr]]', + 'x = 1', + '', + '[[arr]]', + 'x = 2', + '', + ].join('\n'); + const parsed = parseTomlToObject(content); + assert.ok(Array.isArray(parsed.arr)); + assert.strictEqual(parsed.arr.length, 2); + assert.strictEqual(parsed.arr[0].x, 1); + assert.strictEqual(parsed.arr[1].x, 2); + }); + + test('accepts disjoint nested headers (not duplicates)', () => { + const content = [ + '[a.b]', + 'x = 1', + '', + '[a.c]', + 'y = 2', + '', + ].join('\n'); + const parsed = parseTomlToObject(content); + assert.strictEqual(parsed.a.b.x, 1); + assert.strictEqual(parsed.a.c.y, 2); + }); +}); + +// concurrency: false — drives the same install pipeline as the other f-suites. +describe('#2760 CR5 finding 3 — migration emits namespaced AoT (no flat/namespaced mixing)', { concurrency: false }, () => { + let tmpDir; + let codexHome; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2760-cr5-f3-')); + codexHome = path.join(tmpDir, 'codex-home'); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + test('user has [[hooks.AfterTool]] AND legacy [hooks.SessionStart] → post-install both namespaced, no flat AoT', () => { + // Reproduces the mixed-form scenario from finding 3: + // - User pre-config has both a namespaced AoT entry [[hooks.AfterTool]] + // AND a legacy single-bracket [hooks.SessionStart]. + // - Pre-CR5 migration converts the legacy section to flat [[hooks]] + // with event="SessionStart", leaving a mixed flat+namespaced layout. + // - Post-CR5 migration emits [[hooks.SessionStart]] directly so both + // of the user's hooks coexist in the namespaced shape, and the + // GSD-managed entry converges on namespaced too. + const userPlusLegacy = [ + '[[hooks.AfterTool]]', + 'command = "x"', + '', + '[hooks.SessionStart]', + 'command = "y"', + '', + ].join('\n'); + writeCodexConfig(codexHome, userPlusLegacy); + + runCodexInstall(codexHome); + const after = readCodexConfig(codexHome); + const parsed = parseTomlToObject(after); + + // The pre-existing [[hooks.AfterTool]] entry is preserved. + assert.ok( + parsed.hooks && Array.isArray(parsed.hooks.AfterTool), + 'pre-existing [[hooks.AfterTool]] must remain a namespaced AoT array' + ); + assert.ok( + parsed.hooks.AfterTool.some((entry) => entry.command === 'x'), + 'user AfterTool entry must be preserved: ' + JSON.stringify(parsed.hooks.AfterTool) + ); + + // The migrated SessionStart entry is now namespaced AoT, not flat + // [[hooks]] with event="SessionStart". + assert.ok( + parsed.hooks && Array.isArray(parsed.hooks.SessionStart), + 'migrated SessionStart must be namespaced AoT (not flat [[hooks]])' + ); + const ssCommands = parsed.hooks.SessionStart.map((e) => e.command); + assert.ok( + ssCommands.includes('y'), + 'user SessionStart command "y" must be preserved in namespaced array: ' + + JSON.stringify(ssCommands) + ); + // GSD's managed gsd-check-update entry also lives in the namespaced array. + assert.ok( + ssCommands.some((cmd) => typeof cmd === 'string' && /gsd-check-update\.js/.test(cmd)), + 'managed gsd-check-update entry must appear in hooks.SessionStart array: ' + + JSON.stringify(ssCommands) + ); + + // No flat top-level [[hooks]] AoT may remain. + assert.ok( + !Array.isArray(parsed.hooks) || parsed.hooks.length === 0, + 'no flat top-level [[hooks]] AoT entries may remain after migration: ' + + JSON.stringify(parsed.hooks) + ); + + // No synthetic event field on the migrated SessionStart entries — the + // namespace IS the event. + for (const entry of parsed.hooks.SessionStart) { + assert.equal(entry.event, undefined, + 'no synthetic event field — namespace [[hooks.SessionStart]] encodes the event: ' + + JSON.stringify(entry)); + } + }); +}); diff --git a/tests/codex-config.test.cjs b/tests/codex-config.test.cjs index f7d1b603c..89523fa72 100644 --- a/tests/codex-config.test.cjs +++ b/tests/codex-config.test.cjs @@ -8,11 +8,30 @@ // Enable test exports from install.js (skips main CLI logic) process.env.GSD_TEST_MODE = '1'; -const { test, describe, beforeEach, afterEach } = require('node:test'); +const { test, describe, before, beforeEach, afterEach } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); const os = require('os'); +const { execFileSync } = require('child_process'); + +// #2153 follow-up: ensure hooks/dist/ exists before any install integration +// test runs. The Codex install path copies hook files from hooks/dist/, which +// is gitignored and only populated by `npm run build:hooks`. When this file is +// run in isolation (`node --test tests/codex-config.test.cjs`) the build step +// from the npm-test pretest chain does not run, and the "Codex install copies +// hook file" regression silently fails because hooks/dist/ is empty. +// Build on demand so the test passes regardless of runner ordering. +const HOOKS_DIST = path.join(__dirname, '..', 'hooks', 'dist'); +const BUILD_HOOKS_SCRIPT = path.join(__dirname, '..', 'scripts', 'build-hooks.js'); +before(() => { + if (!fs.existsSync(HOOKS_DIST) || fs.readdirSync(HOOKS_DIST).length === 0) { + execFileSync(process.execPath, [BUILD_HOOKS_SCRIPT], { + encoding: 'utf-8', + stdio: 'pipe', + }); + } +}); const { getCodexSkillAdapterHeader, @@ -26,6 +45,7 @@ const { install, GSD_CODEX_MARKER, CODEX_AGENT_SANDBOX, + parseTomlToObject, } = require('../bin/install.js'); function runCodexInstall(codexHome, cwd = path.join(__dirname, '..')) { @@ -575,7 +595,7 @@ describe('migrateCodexHooksMapFormat', () => { assert.strictEqual(migrateCodexHooksMapFormat(''), ''); }); - test('converts [hooks.shell] with command key to [[hooks]] with type = "shell"', () => { + test('converts [hooks.shell] to namespaced AoT [[hooks.shell]] (#2760 CR5 finding 3)', () => { const content = [ '[features]', 'codex_hooks = true', @@ -587,34 +607,38 @@ describe('migrateCodexHooksMapFormat', () => { '', ].join('\n'); const result = migrateCodexHooksMapFormat(content); - // Old format removed - assert.ok(!result.includes('[hooks.shell]'), 'removes [hooks.shell] map header'); - assert.ok(!result.match(/^\[hooks\]$/m), 'removes bare [hooks] container'); - // New format present - assert.ok(result.includes('[[hooks]]'), 'adds [[hooks]] array header'); - assert.ok(result.includes('type = "shell"'), 'adds type = "shell" key'); - assert.ok(result.includes('command = "node /home/.codex/hooks/gsd-check-update.js"'), 'preserves command value'); - // User content preserved - assert.ok(result.includes('[features]'), 'preserves [features] section'); - assert.ok(result.includes('codex_hooks = true'), 'preserves codex_hooks key'); + // Parse structurally — no source-grep on raw bytes. + const parsed = parseTomlToObject(result); + assert.ok(parsed.hooks && Array.isArray(parsed.hooks.shell), + 'hooks.shell must be an array of tables, got: ' + (parsed.hooks ? typeof parsed.hooks.shell : 'no hooks table')); + assert.strictEqual(parsed.hooks.shell.length, 1); + assert.strictEqual(parsed.hooks.shell[0].command, 'node /home/.codex/hooks/gsd-check-update.js'); + // No flat top-level [[hooks]] AoT and no synthetic event field. + assert.ok(!Array.isArray(parsed.hooks), + 'no top-level [[hooks]] AoT — namespace IS the event in CR5 form'); + assert.equal(parsed.hooks.shell[0].event, undefined, + 'no synthetic event field — namespace [[hooks.shell]] encodes the event'); + // User content preserved. + assert.equal(parsed.features && parsed.features.codex_hooks, true); }); - test('converts [hooks.exec] to [[hooks]] with type = "exec"', () => { + test('converts [hooks.exec] to namespaced AoT [[hooks.exec]] (#2760 CR5 finding 3)', () => { const content = [ '[hooks.exec]', 'command = "echo hello"', - 'event = "SessionStart"', + 'extra_key = "preserved"', '', ].join('\n'); const result = migrateCodexHooksMapFormat(content); - assert.ok(!result.includes('[hooks.exec]'), 'removes [hooks.exec] map header'); - assert.ok(result.includes('[[hooks]]'), 'adds [[hooks]] array header'); - assert.ok(result.includes('type = "exec"'), 'adds type = "exec" key'); - assert.ok(result.includes('command = "echo hello"'), 'preserves command'); - assert.ok(result.includes('event = "SessionStart"'), 'preserves event'); + const parsed = parseTomlToObject(result); + assert.ok(parsed.hooks && Array.isArray(parsed.hooks.exec)); + assert.strictEqual(parsed.hooks.exec.length, 1); + assert.strictEqual(parsed.hooks.exec[0].command, 'echo hello'); + assert.strictEqual(parsed.hooks.exec[0].extra_key, 'preserved'); + assert.equal(parsed.hooks.exec[0].event, undefined); }); - test('converts multiple [hooks.TYPE] sections to separate [[hooks]] blocks', () => { + test('converts multiple [hooks.TYPE] sections to separate namespaced AoT blocks (#2760 CR5 finding 3)', () => { const content = [ '[hooks.shell]', 'command = "node /home/.codex/hooks/gsd-check-update.js"', @@ -624,12 +648,13 @@ describe('migrateCodexHooksMapFormat', () => { '', ].join('\n'); const result = migrateCodexHooksMapFormat(content); - assert.ok(!result.includes('[hooks.shell]'), 'removes [hooks.shell]'); - assert.ok(!result.includes('[hooks.exec]'), 'removes [hooks.exec]'); - const hookHeaders = (result.match(/\[\[hooks\]\]/g) || []).length; - assert.strictEqual(hookHeaders, 2, 'produces two [[hooks]] array entries'); - assert.ok(result.includes('type = "shell"'), 'first entry has type = "shell"'); - assert.ok(result.includes('type = "exec"'), 'second entry has type = "exec"'); + const parsed = parseTomlToObject(result); + assert.ok(parsed.hooks && Array.isArray(parsed.hooks.shell)); + assert.ok(parsed.hooks && Array.isArray(parsed.hooks.exec)); + assert.strictEqual(parsed.hooks.shell.length, 1); + assert.strictEqual(parsed.hooks.exec.length, 1); + assert.strictEqual(parsed.hooks.shell[0].command, 'node /home/.codex/hooks/gsd-check-update.js'); + assert.strictEqual(parsed.hooks.exec[0].command, 'echo done'); }); test('leaves user-authored [[hooks]] array entries untouched when no legacy [hooks] map present', () => { @@ -642,7 +667,7 @@ describe('migrateCodexHooksMapFormat', () => { assert.strictEqual(migrateCodexHooksMapFormat(content), content); }); - test('end-to-end: install on config with old [hooks] map format produces [[hooks]] array format (#2637)', () => { + test('end-to-end: install on config with old [hooks] map format produces namespaced AoT (#2637, #2760 CR5)', () => { // Simulates the exact old GSD config.toml format that broke on Codex 0.124.0 const oldContent = [ '[features]', @@ -655,18 +680,15 @@ describe('migrateCodexHooksMapFormat', () => { '', ].join('\n'); const result = migrateCodexHooksMapFormat(oldContent); - // Must not contain any [hooks] or [hooks.*] map-style headers - assert.ok(!result.match(/^\s*\[hooks\]\s*$/m), 'no bare [hooks] map header'); - assert.ok(!result.match(/^\s*\[hooks\./m), 'no [hooks.TYPE] map headers'); - // Must contain [[hooks]] array format - assert.ok(result.includes('[[hooks]]'), 'has [[hooks]] array-of-tables header'); - // type key must be present - assert.ok(result.includes('type = "shell"'), 'has type = "shell" in [[hooks]] entry'); - // command is preserved - assert.ok(result.includes('command = "node /home/.codex/hooks/gsd-check-update.js"'), 'command preserved'); - // [features] user content preserved - assert.ok(result.includes('[features]'), 'preserves [features]'); - assert.ok(result.includes('codex_hooks = true'), 'preserves codex_hooks'); + const parsed = parseTomlToObject(result); + // Codex 0.124.0+: must produce array-of-tables form. CR5 finding 3: + // namespaced AoT [[hooks.shell]] (no flat [[hooks]] with synthetic event). + assert.ok(parsed.hooks && Array.isArray(parsed.hooks.shell), + 'hooks.shell must be array-of-tables in namespaced form'); + assert.strictEqual(parsed.hooks.shell.length, 1); + assert.strictEqual(parsed.hooks.shell[0].command, + 'node /home/.codex/hooks/gsd-check-update.js'); + assert.equal(parsed.features && parsed.features.codex_hooks, true); }); test('bare [hooks] section without sub-tables is dropped (no [[hooks]] block added)', () => { @@ -688,7 +710,7 @@ describe('migrateCodexHooksMapFormat', () => { assert.ok(result.includes('[model]'), 'preserves [model]'); }); - test('CRLF line endings are preserved through migration', () => { + test('CRLF line endings are preserved through migration (#2760 CR5: namespaced AoT)', () => { const content = [ '[features]', 'codex_hooks = true', @@ -698,9 +720,58 @@ describe('migrateCodexHooksMapFormat', () => { '', ].join('\r\n'); const result = migrateCodexHooksMapFormat(content); - assert.ok(result.includes('[[hooks]]\r\n'), 'uses CRLF in [[hooks]] header'); - assert.ok(result.includes('type = "shell"\r\n'), 'uses CRLF in type line'); - assert.ok(!result.includes('[hooks.shell]'), 'removes legacy [hooks.shell]'); + assert.ok(result.includes('[[hooks.shell]]\r\n'), + 'uses CRLF in namespaced [[hooks.shell]] header'); + // Round-trip parse confirms the structural shape independent of EOL. + const parsed = parseTomlToObject(result); + assert.ok(parsed.hooks && Array.isArray(parsed.hooks.shell)); + assert.strictEqual(parsed.hooks.shell[0].command, + 'node /home/.codex/hooks/gsd-check-update.js'); + }); +}); + +// ─── shape parity between migration and managed emit (#2760 CR5 finding 3) ── + +describe('Codex hooks emit: migration produces namespaced AoT so managed-emit converges', () => { + // After #2760 CR5 finding 3, the legacy migration path + // (migrateCodexHooksMapFormat) emits `[[hooks.]]` directly — the + // namespace IS the event, no synthetic `event = ...` field. The managed + // install path (writes "# GSD Hooks") detects existing namespaced AoT via + // hasUserNamespacedAotHooks and emits its block in the same shape. The two + // paths must therefore both produce a namespaced layout when a legacy + // [hooks.SessionStart] is migrated, eliminating the mixed flat+namespaced + // bug class entirely. + + let tmpDir; + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-codex-fieldparity-')); + }); + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + test('migration of legacy [hooks.SessionStart] produces namespaced AoT', () => { + const legacyContent = [ + '[features]', + 'codex_hooks = true', + '', + '[hooks.SessionStart]', + 'command = "node /home/.codex/hooks/gsd-check-update.js"', + '', + ].join('\n'); + const migrated = migrateCodexHooksMapFormat(legacyContent); + const parsed = parseTomlToObject(migrated); + assert.ok( + parsed.hooks && Array.isArray(parsed.hooks.SessionStart), + 'migration must emit [[hooks.SessionStart]] namespaced AoT' + ); + assert.equal(parsed.hooks.SessionStart[0].event, undefined, + 'migration must NOT emit a synthetic event field — namespace IS the event'); + assert.equal( + Array.isArray(parsed.hooks), + false, + 'migration must NOT emit a flat top-level [[hooks]] AoT' + ); }); }); @@ -929,7 +1000,7 @@ describe('mergeCodexConfig', () => { assertUsesOnlyEol(content, '\r\n'); }); - test('case 2 preserves user-authored [agents] tables while stripping leaked GSD sections in CRLF files', () => { + test('case 2 strips bare [agents] tables (invalid in current Codex schema, #2760) and removes leaked GSD sections in CRLF files', () => { const configPath = path.join(tmpDir, 'config.toml'); const brokenContent = [ '[features]', @@ -958,8 +1029,24 @@ describe('mergeCodexConfig', () => { const markerIndex = content.indexOf(GSD_CODEX_MARKER); const beforeMarker = content.slice(0, markerIndex); - 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'); + // Bare [agents] is invalid under Codex's current schema (rejected with + // "expected struct AgentsToml") so install-time stripping always purges + // it (#2760). User feature keys above the marker are preserved. + // Structural assertion: TOML-parse the pre-marker region and verify the + // bare [agents] block is fully gone — header AND body keys (e.g., + // `default = "custom-agent"`). A header-only check would miss a + // partial-strip regression that leaves orphan body keys reparented to a + // sibling section. + const parsedBefore = parseTomlToObject(beforeMarker); + assert.equal( + parsedBefore.agents, + undefined, + 'bare [agents] block fully purged including body keys (#2760)', + ); + assert.ok( + parsedBefore.features && parsedBefore.features.child_agents_md === false, + 'preserves user feature keys above marker', + ); // New struct format: exactly one [agents.gsd-executor] in the GSD block (after marker) assert.strictEqual(countMatches(content, /^\[agents\.gsd-executor\]\s*$/gm), 1, 'exactly one struct agent header in GSD block'); assert.strictEqual(countMatches(content, /name = "gsd-executor"/g), 0, 'no name = field in struct format');