golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
2,477 Commits 4 Branches 2 Tags
f4c4ec6211b21422f40b3d675a4d997e46292f2e
Commit Graph

2 Commits

Author SHA1 Message Date
Tom Boucher
3c03a153a5 fix(#2773): emit correct Codex 0.124.0+ two-level nested hooks schema (#2809) 
* fix(#2773): emit correct Codex 0.124.0+ two-level nested hooks schema

Codex 0.124.0's stable spec requires:

  [[hooks.SessionStart]]          ← event entry (optional matcher)

  [[hooks.SessionStart.hooks]]    ← handler sub-table
  type = "command"
  command = "node ..."

Previous GSD versions wrote the flat [[hooks]] + event = "SessionStart"
form (#2637) or a single-block [[hooks.SessionStart]] without the nested
.hooks sub-table (#2760). Both are rejected by Codex 0.124.0+ at launch.

Changes:

bin/install.js
  - Hook block emission now always writes the two-level nested AoT form.
  - migrateCodexHooksMapFormat extended to also migrate flat [[hooks]]
    array-of-tables entries (event = "..." key → [[hooks.<EVENT>]] form).
    Flat [[hooks]] and [[hooks.<EVENT>]] are mutually exclusive TOML types;
    any pre-existing flat entries must be promoted before GSD appends its
    own namespaced hooks.
  - Migrated flat AoT blocks are inserted BEFORE the GSD marker so they
    stay in the "user" portion of the file and survive stripGsdFromCodexConfig.
  - stripCodexGsd* regexes cover all four historical block shapes.
  - validateCodexConfigSchema no longer rejects flat [[hooks]] at the root
    level (removing the false-positive that blocked install when users had
    their own AfterCommand hooks). The validator still enforces the nested
    [[hooks.<EVENT>.hooks]] shape for entries that have a .hooks sub-table.

tests/
  - bug-2760-codex-install-defensive.test.cjs: 29/29 passing.
    Added 5 new regression cases for fresh install, upgrade from each
    legacy shape, idempotent reinstall, and user hook preservation.
  - codex-config.test.cjs: 106/106 passing.
    All migration tests updated to assert [[hooks.<TYPE>.hooks]] sub-table
    (command now in handler level, not event-entry level).
    New tests: flat [[hooks]] migration (SessionStart, AfterCommand),
    install+uninstall preserves non-GSD AfterCommand hook.

Closes #2773

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix: address CodeRabbit review + CI regression in bug-2698-crlf-install

CI regression (#2698 tests):
  Strip GSD-managed hook blocks BEFORE running migrateCodexHooksMapFormat.
  The previous order let migration convert the stale [[hooks]] + event =
  "SessionStart" + gsd-update-check.js block to [[hooks.SessionStart]] form
  before Shape 1 strip regex could match it; Shape 1 only matches the flat
  [[hooks]] form, so the stale block survived reinstall. Swapping to
  strip-then-migrate ensures only user-authored hooks reach the migration step.
  Shape 3/4 regexes also extended to match both gsd-check-update.js and the
  legacy gsd-update-check.js filename so no variant slips through.

CodeRabbit actionable (major):
  migrateCodexHooksMapFormat now accepts single-quoted TOML event values
  (event = 'SessionStart') in the flat [[hooks]] filter and event-name
  extractor. TOML spec allows single-quoted literal strings; double-quote-only
  regexes silently skipped them, leaving the block unmigrated and triggering
  the hard-fail validator.

CodeRabbit nitpicks:
  tests/codex-config.test.cjs: replace indexOf('[[hooks.AfterCommand]]')
  ordering check with parseTomlToObject structural assertions (no-source-grep
  rule).
  tests/bug-2760-codex-install-defensive.test.cjs: replace three
  content.match(/…/g).length raw-text counts with parseTomlToObject structural
  assertions for single-handler and single-event-entry invariants.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix: address CodeRabbit review #2 — extractFlatHookEventName helper + type assertions

- bin/install.js: consolidate TOML_QUOTED_STRING + TOML_EVENT_CAPTURE into a
  single extractFlatHookEventName() helper that rejects empty-string event values
  (event = "" or event = ''); previously two independent regexes had to be kept
  in sync and neither guarded against a blank event name producing a [[hooks.]]
  header

- tests/bug-2760-codex-install-defensive.test.cjs: add comments explaining why
  the e.command fallback is retained in both allSessionStartCommands and
  afterToolCommands collectors — migration only upgrades [hooks.TYPE] map-format
  sections, not existing [[hooks.TYPE]] namespaced AoT entries authored with
  command at event-entry level; removing the fallback causes false failures for
  preserved user entries

- tests/codex-config.test.cjs: add type = "command" assertions to all migration
  tests that verify .command but were missing .type checks; buildNestedBlock
  injects type = "command" when the source body has no explicit type key, so
  every migrated handler must carry it per the Codex 0.124.0+ schema

138 tests pass, 0 fail.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix: CR round 3 + proactive audit — TOML quoting, stale AoT migration, strict validator

Three real issues from CodeRabbit round 3, plus the collateral improvements they
enable:

bin/install.js — tomlBareKey() helper (#2773 CR6a)
  buildNestedBlock interpolated the raw event name into [[hooks.${type}]] and
  [[hooks.${type}.hooks]] headers without TOML escaping. An event name containing
  spaces or punctuation (e.g. "Before Tool") would produce invalid TOML that
  parseTomlToObject would subsequently reject. Added tomlBareKey() — wraps the
  key in double-quoted TOML strings when it contains non-bare-key characters
  ([A-Za-z0-9_-]).

bin/install.js — staleNamespacedAotSections migration path (#2773 CR6b)
  migrateCodexHooksMapFormat handled [hooks.TYPE] (map-format) and flat [[hooks]]
  with event = "..." but ignored [[hooks.TYPE]] AoT entries that carried handler
  fields (command, type, timeout, statusMessage) at event-entry level without a
  nested [[hooks.TYPE.hooks]] sub-table. This is the pre-#2773 single-block shape
  that Codex 0.124.0+ rejects. Added staleNamespacedAotSections as the third
  migration category: detected by STALE_HANDLER_FIELD_PATTERN + absence of a
  [[hooks.TYPE.hooks]] sub-table in the same file; promoted to the two-level
  nested form by buildNestedBlock. Matcher-only entries (no handler fields) are
  intentionally skipped.

bin/install.js — validator now rejects event-level handler fields (#2773 CR6c)
  With migration covering the stale AoT shape, validateCodexConfigSchema can be
  strict: entries that have handler fields at event-entry level but no .hooks
  sub-array return ok: false instead of silently passing. Matcher-only entries
  (no handler fields and no .hooks) remain valid as event filters.

tests/codex-config.test.cjs — four new migration tests + missing type assertion
  Four tests cover the new stale AoT migration path: single-entry promotion,
  already-nested entry is left untouched (no double-wrap), multiple event types,
  and matcher-only entry is skipped. Added the missing type = "command" assertion
  to the CRLF migration test (the one miss from CR round 2).

tests/bug-2760-codex-install-defensive.test.cjs — strict .hooks-only collectors
  With stale AoT entries now migrated, the entry.command fallbacks in
  allSessionStartCommands and afterToolCommands are dead code. Replaced with
  strict entry.hooks-only collection guarded by an every(Array.isArray(e.hooks))
  pre-assertion, so any future regression that leaves handler fields at event
  level produces an explicit test failure rather than silently collecting them.

142 tests pass, 0 fail.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix: CR round 4 — segment-safe quoted-key detection + structural test assertions

bin/install.js — getTomlTableSections now exposes segments (#2773 CR7a)
  The staleNamespacedAotSections filter used section.path.split('.').length > 2
  to skip [[hooks.TYPE.hooks]] sub-table entries. That check misclassifies quoted
  event names containing dots: [[hooks."before.tool"]] has path hooks.before.tool
  (3 dot-parts) but only 2 true parsed segments, so it was incorrectly excluded
  from migration. Fixed by adding segments to the getTomlTableSections return
  shape (already available on record.tableHeader.segments) and replacing the
  split-based check with section.segments.length !== 2, which uses the true
  parsed key count regardless of dots inside quoted names.

tests/codex-config.test.cjs — replace raw-equality assertions (#2773 CR7b)
  The two new no-op migration tests (already-nested and matcher-only) used
  assert.strictEqual(result, content) — raw string equality that conflicts with
  the repo no-source-grep testing standard. Replaced with structural assertions
  using parseTomlToObject: the already-nested test verifies the handler stays
  under .hooks[0] and no double-wrap occurs; the matcher-only test verifies the
  matcher key is preserved and no .hooks sub-array is added.

142 tests pass, 0 fail.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* fix: CR round 5 — parseHooksBody key parser, empty-handler guard, segment-safe legacyMap filter, stronger test assertions

- parseHooksBody: replace /^([\w.]+)\s*=/ regex with parseTomlKey() so
  hyphenated keys (status-message) and quoted keys are not silently dropped
- buildNestedBlock: guard against handlerEntries.length === 0 — do not
  synthesise [[hooks.TYPE.hooks]] with type="command" but no command for
  matcher-only or otherwise handler-empty stale sections
- legacyMapSections filter: use section.segments.length === 2 (same fix
  applied to staleNamespacedAotSections in round 4) to prevent [hooks.X.Y]
  3-segment tables from being misclassified as event entries
- tests: add regression test for [[hooks."before.tool"]] quoted-dot event
  names; strengthen command path assertion to exact absolute path comparison

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
 2026-04-28 18:28:53 -04:00
Tom Boucher
b44482cf03 fix(#2760): defensive Codex install — strip legacy agents blocks, default hooks to AoT, validate post-write schema (#2785) 
* 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>
 2026-04-27 17:11:59 -04:00