b44482cf03
* fix(#2760): defensive Codex install — strip legacy agents blocks, default hooks to AoT, validate post-write schema Three defects, three defensive fixes shipped together. Issue reporter never returned with the requested diagnostic backup, but four additional users have since confirmed the same Codex breakage and ZakAnun confirmed manual cleanup is the only working workaround — defensive triple ships without the original backup grep, justified by the corroborating reports. Fix 1 (defect 3 — confirmed real). The Codex hooks emit path always appended a top-level `[[hooks]]` AoT block, which collides with users who already use the namespaced AoT form `[[hooks.SessionStart]]`. New helper `hasUserNamespacedAotHooks()` detects the user's preferred shape on parse and the install emits the GSD-managed hook in that same shape when present. Default for fresh configs stays at top-level `[[hooks]]` so status-quo behavior is preserved. Fix 2 (defects 1+2 — defensive). `stripLeakedGsdCodexSections()` (the install-time stripper) now always purges bare `[agents]` single-bracket tables and `[[agents]]` sequence tables regardless of GSD marker presence — both forms are invalid in current Codex schema and produce "invalid type: ..., expected struct AgentsToml". Previously gated on GSD-name lookup which missed marker-stripped configs and third-party authored entries. The uninstall-time stripper (`stripCodexGsdAgentSections`) keeps its old conservative behavior so user-authored entries survive uninstall. Fix 3 (defensive). Post-write schema validation parses the bytes about to be committed and asserts no bare `[agents]`, no `[[agents]]`, and no bare `[hooks.<Event>]` tables remain. On failure the install restores the pre-install backup of config.toml and aborts loudly so the user is never left with a Codex CLI that refuses to load. Pre-install snapshot is captured before installCodexConfig runs (not after) so restore returns the file to its true pre-GSD state. Tests added (10 new, 1 updated): - bug-2760-codex-install-defensive.test.cjs (10 new tests across 4 describes: hooks AoT preservation, strip robustness for both [agents] and [[agents]] without marker, schema validator behavior, abort+restore via test seam) - codex-config.test.cjs "case 2 ..." updated to reflect new defensive bare-[agents] purge Full suite: 5747 pass / 0 fail. Closes #2760 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(#2760): normalize Codex hooks emit field name across migration and managed paths The migrateCodexHooksMapFormat path emitted `type = "<TYPE>"` for legacy [hooks.TYPE] sections, while the GSD-managed Codex install emitted `event = "SessionStart"` — same target [[hooks]] schema, two different field names. Codex currently tolerates both via permissive parsing, but the moment one path tightens this becomes a silent #2760-class regression. Normalize both call sites on `event` (the existing GSD-managed convention). Update migration emit, docstring, and existing migration assertions to match. Add a parity regression test that drives both code paths and asserts the [[hooks]] field key is identical. * test(#2153): fix test isolation by building hooks/dist on demand The "Codex install copies hook file (#2153)" regression depends on hooks/dist/ being populated, but that directory is gitignored and only built by `npm run build:hooks`. The npm pretest chain runs `build:sdk` but not `build:hooks`, so when this file is run in isolation (`node --test tests/codex-config.test.cjs`) the hook copy step skips silently and the regression test fails on a stale-environment artifact rather than a real bug. Add a top-level before() hook that runs scripts/build-hooks.js when hooks/dist/ is missing or empty. Matches the pattern already used by bug-1834-sh-hooks-installed and other install integration tests, so the suite passes regardless of runner ordering or which tests are targeted. * fix(#2760): structural TOML validation, atomic writes, and behavioral test rewrites Addresses CodeRabbit review on PR #2785 plus source-grep violations the maintainer flagged in the regression test. Fix 1 (CR 3149606220) — validateCodexConfigSchema now parses the TOML into a structured object first via the new parseTomlToObject helper, then runs schema-shape checks against both the parsed structure and the table section headers. Malformed TOML with valid-looking headers no longer slips past validation. Fix 2 (CR 3149606224) — Replaced the four source-grep assertions in tests/bug-2760-codex-install-defensive.test.cjs (lines 109, 125, 169, 201) with structural assertions against the parsed TOML object via the exported parseTomlToObject helper. Tests now verify behavior (the file parses and contains the expected structure) instead of literal byte patterns. Robust to formatting changes — exactly what the regex-loosening suggestion was reaching for, done correctly. Confirmed clean by `npm run lint:tests` (0 violations). Fix 3 (CR 3149606234) — The describe block that mutates installModule.__codexSchemaValidator now runs with concurrency: false so the test seam mutation cannot leak into sibling suites that also call runCodexInstall. Fix 4 (CR outside-diff) — Approach (b): atomic temp-file + renameSync. Added atomicWriteFileSync helper used by mergeCodexConfig and the final hooks-write. A mid-write failure leaves the .tmp-<pid>-<n> sibling behind (cleaned up immediately) and never truncates the original config.toml. Paired with try/catch wrapping around the entire post-snapshot mutation sequence so any unexpected throw also triggers restoreCodexSnapshot. Two layers of defense: atomic write prevents the corruption window, snapshot restore handles non-atomic write paths. Added behavioral test for fix 4: stubs fs.renameSync to throw on the configPath rename, asserts the on-disk bytes match the pre-install snapshot byte-for-byte, asserts the parsed structure is still the user's [model] section (no half-written GSD agents block), and asserts no stray .tmp-* files remain. Marked concurrency: false because it monkey-patches a global. Test results: 5749/5749 pass, 0 fail. lint:tests clean. * test(#2760): TOML-parse based assertions for bare-agents purge and hook-field parity (CodeRabbit follow-up) * fix(#2760): treat write failures as fatal, strip legacy hooks before guard, tighten TOML parser (CR4) CR4 finding 1 (MAJOR) — Write failures silently succeeded. The inner catch around atomicWriteFileSync restored the snapshot then re-threw, but the outer catch only matched 'post-write Codex schema validation failed' and downgraded everything else to a warn-and-continue. Install finished with "Done!" while Codex had no GSD agents configured. Fix: wrap writeErr with a `post-write Codex install failed:` prefix and broaden the outer guard to `.startsWith( 'post-write')` so both schema-validation and write failures abort install. CR4 finding 2 (MAJOR) — Legacy flat [[hooks]] block prevented namespaced AoT upgrade. The `!configContent.includes('gsd-check-update')` guard short- circuited the new namespaced emit when an existing install had the legacy flat [[hooks]] block, leaving users stuck in the mixed layout this fix is designed to eliminate. Fix: strip ALL existing managed gsd-check-update hook blocks (top-level [[hooks]] AND namespaced [[hooks.SessionStart]]) BEFORE evaluating the includes guard, so every install converges on the right shape regardless of prior state. CR4 finding 3 (MAJOR) — Homegrown TOML parser silently accepted malformed input. parseTomlValue happily consumed the `0` prefix of `timeout = 0.5` and parseTomlToObject did not verify the full RHS was consumed, so `key = "x" junk` and date/time literals slipped through. Per CONTRIBUTING ("No external dependencies in core"), option (b) was chosen over adding @iarna/toml: (a) parseTomlValue rejects any integer immediately followed by `.`, `e`, `E`, `:`, `-`, `T`, or `Z` (floats / dates / times); (b) parseTomlToObject scans from parsed.end to the next newline and throws `trailing bytes after value` if anything other than whitespace + optional `# comment` is present. * test(#2760): add CR4 regression tests + scope GSD_TEST_MODE + rename rename-fault test CR4 finding 5 (NIT) — GSD_TEST_MODE leak. Saved previous value, set '1' for the require, then restored (delete if undefined). No more test-only env var leaking to siblings in the same node process. CR4 finding 4 (NIT) — Renamed the existing fix-4 test from 'fs.writeFileSync' to 'fs.renameSync' (the only call actually faulted) and added a sibling test that stubs fs.writeFileSync to throw on the .tmp- target — exercising the pre-rename branch of atomicWriteFileSync that was previously untested. Both serialize via concurrency: false on the existing describe block. CR4 finding 1 (MAJOR test) — New behavioral test asserts install throws with a `post-write Codex install failed` message AND never prints "Done!" when the hook-block atomic rename fails. Captures stdout via console.log stub, asserts byte equality of restored snapshot. Faults only the rename whose temp source contains gsd-check-update so earlier mergeCodexConfig writes are not collateral damage. CR4 finding 2 (MAJOR test) — New TOML-parsed behavioral test for the legacy-hook upgrade path: pre-install has [[hooks.SessionStart]] (user) + legacy flat [[hooks]] managed gsd-check-update entry; post-install must have hooks.SessionStart as Array-of-tables with both user hook and GSD entry, and no top-level [[hooks]] AoT remaining. Also asserts exactly one gsd-check-update entry (no duplicates). CR4 finding 3 (MAJOR test) — parseTomlToObject regression suite: rejects floats (timeout = 0.5), dates (created = 1979-05-27), trailing garbage (key = "x" junk), and accepts trailing whitespace + # comment. * fix(#2760): CR5 — pre-write fatal, TOML duplicate-key/header rejection, namespaced AoT migration Address all five CodeRabbit round-5 findings on PR #2785: Finding 1 (MAJOR) — Pre-write failures in the Codex hook configuration catch (around bin/install.js:7002) used to fall through to console.warn even though restoreCodexSnapshot() had already run. This produced "Done!" output with no Codex hooks configured. Now wraps the original error with a "(pre-write)" prefix and rethrows so install aborts loudly. Same defect class as CR4 finding 1, different layer. Finding 2 (MAJOR) — parseTomlToObject silently reused existing tables and overwrote duplicate keys. Real TOML 1.0 rejects: - duplicate scalar key in same table ([a]\nx=1\nx=2) - re-declared [a] header (two [a] sections) - [[arr]] then [arr] for same path (shape mismatch) Tracks pathShape, declaredHeaders, and per-table-instance key sets; throws "duplicate or shape-mismatched table header at <path>" or "duplicate key <name> in <path>". Finding 3 (MAJOR) — migrateCodexHooksMapFormat used to emit flat [[hooks]]\nevent="<TYPE>", which produced mixed flat+namespaced layouts when the user already had [[hooks.<OTHER>]] entries. Now emits [[hooks.<TYPE>]] directly (the namespace IS the event); managed-emit detector hasUserNamespacedAotHooks fires correctly so the install converges on a single namespaced layout regardless of pre-existing state. Finding 4 (NIT) — tests/bug-2760-codex-install-defensive.test.cjs rename-failure test tightened from "throw OR warn acceptable" to assert.equal(threw, true), locking the contract Finding 1 establishes. Finding 5 (NIT) — bug-2760 test suite snapshots and restores fs.renameSync defensively in beforeEach/afterEach (symmetric with fs.writeFileSync), removing the fragile per-test try/finally. Second test in the same suite cleaned up to drop its try/finally. Updates tests/codex-config.test.cjs to assert the new namespaced AoT migration shape via parseTomlToObject (no source-grep). Existing field- parity test reframed as shape-parity since both paths now emit namespaced. Tests: 5764 pass (+8 new). lint:tests: 0 violations. * docs(#2760): add CHANGELOG entry for Codex install defensive triple Adds the [Unreleased] Fixed entry for the Codex install fix landed in this PR — defensive strip of legacy [agents]/[[agents]] blocks, namespaced AoT hook detection across all events, atomic write + rollback, strict TOML validation rejecting duplicate keys/repeated headers/trailing bytes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
90 KiB
90 KiB