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

7 Commits

Author SHA1 Message Date
Tom Boucher
ba231ecbfc chore: clean up clear-cut ESLint warnings (#732) (#734) 
Pay down pre-existing error→warn lint debt. Removes dead imports/vars, unused functions, redundant regex/string escapes, and stale eslint-disable directives; converts unused `catch (_e)` to optional catch binding (src/*.cts).

No behavior change. Lint 345→125 warnings (0 errors); deferred categories (n/no-process-exit, test-sleeps, control-regex) tracked in #732 for follow-up. Full test suite green (0 failures); code-review verified all removals unused and all escape fixes semantics-preserving.

Closes #732

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
 2026-06-06 11:24:48 -04:00
Tom Boucher
a28dcec981 chore(#597): replace count-based ratchet guards with AST lint + named-set allowlists (#603) 
The windows-test-parity ratchet greps test source for fs.rmSync-without-
maxRetries (and six other Windows-portability anti-patterns), failing when an
integer offender COUNT exceeds a frozen baseline (rmSync: 95). A count ratchet
is a Goodhart metric: fixing one offender and adding another keeps the count
constant, so a new defect slips through green. Replace it — and every other
count ratchet in the repo — with a layered, masking-proof design.

Behavioral seam test
- tests/helpers-cleanup.test.cjs proves helpers.cleanup() carries the Windows
  EBUSY retry budget. cleanup() delegates retries to Node's fs.rmSync via
  maxRetries (it owns no loop), so the test asserts the option contract
  (recursive/force/maxRetries>0/retryDelay>0) + real-FS removal + the cwd-guard,
  rather than a loop that does not exist. The EBUSY risk is now tested ONCE at
  the helper, not approximated textually at every call site.

Write-time ESLint rule (AST-accurate, replaces the grep)
- eslint-rules/no-raw-rmsync-in-tests.cjs (error in tests/**/*.test.cjs) bans
  raw fs.rmSync, steering to cleanup(). Catches member, computed (fs['rmSync']),
  destructured and aliased forms; escape hatch is inline
  `// eslint-disable-next-line local/no-raw-rmsync-in-tests -- <reason>` only.
- Migrated 336 raw fs.rmSync teardown calls across ~116 test files to cleanup().
  ~18 genuinely load-bearing sites (mid-test SUT/fault-injection removals,
  error-swallowing or name-colliding local teardown helpers) keep the raw call
  with an inline eslint-disable + reason.

Shared anti-ratchet primitive
- scripts/lib/allowlist-ratchet.cjs:
  - assertWithinAllowlist: fails on NOVEL ids (new offender introduced) AND on
    STALE ids (a known offender was fixed but not pruned) — identity, not count,
    and a ratchet DOWN toward zero.
  - assertTightCeiling: a size/length budget whose ceiling must stay within a
    grace band of the high-water mark, so budgets may only tighten, never creep.

Ratchets converted onto the primitive
- windows-test-parity-guard.test.cjs: rmSync rule deleted (now ESLint-enforced);
  the remaining six patterns moved from integer baselines to named-set
  allowlists with ratchet-down.
- scripts/lint-test-file-count.{cjs,allowlist.json}: per-module integer counts →
  named filename sets (closes the swap-a-file-keep-the-count blind spot); a
  module dropping under cap now FAILS to force pruning its allowlist entry.
- enh-2790 skill-count `<= 63` → named skill allowlist (ratchets toward ~58).

Size budgets hardened (tighten-only)
- agent-size / workflow-size / feat-3039 help-tiered: ceilings lowered to the
  current high-water mark and an assertTightCeiling anti-creep check added per
  tier. Fixed external-contract limits (description ≤100 chars, agent ≤100 KB)
  are intentionally left as-is — they are not grandfathered creeping budgets.

No user-facing behavior change (tests + tooling only); no USER_FACING_PREFIXES
touched, so no changeset fragment is required.

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
 2026-06-01 22:43:49 -04:00
Tom Boucher
2d3027767b fix(3426): Codex Windows hooks use .cmd shim to avoid POSIX exec fail (#3768) 
* test(#3426): add RED test for Codex Windows hooks .cmd shim requirement

Drive buildCodexHookWindowsShimIR (typed IR) + ensureCodexHooksJsonSessionStart
integration against mocked win32 platform. Counter-tests confirm darwin/linux
paths remain unchanged.

NOTE: Windows wall-clock verification depends on Docker matrix Windows
runners. Local test exercises the generator IR shape only.

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

* fix(#3426): Codex Windows hooks use .cmd shim to avoid bash.exe POSIX-exec failure

Root cause: Codex on Windows runs hook commands from PowerShell/cmd. The previous
hooks.json command format was `"node.exe" "script.js"`. Codex's hook-dispatch shell
(Git Bash / MSYS) tried to POSIX-exec node.exe (a Windows PE binary) via execvp(),
which fails with ENOEXEC — reported as `bash.exe: cannot execute binary file`.

Fix: `ensureCodexHooksJsonSessionStart` now calls `buildCodexHookWindowsShimIR` on
win32 to write a .cmd shim alongside the .js hook file. cmd.exe executes .cmd files
natively via CreateProcess, bypassing the POSIX exec layer entirely. Non-Windows
paths (darwin, linux) are unchanged: they continue to use the node-runner command.

Also adds `gsd-check-update.cmd` to the codex-hooks-json managed-basename set so
reconcileCodexHooksJsonSessionStart correctly replaces stale node-runner entries on
reinstall.

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

* chore(3426): update changeset to reference PR #3768

* fix(3426): fail-loud on Codex Windows shim-write failure instead of silently restoring broken command

Replace the silent fallback to `projectManagedHookCommand` (the old
`node.exe script.js` form) with an explicit warn-and-skip path.

When `atomicWriteFileSync` fails to write the `.cmd` shim, the previous
code silently called `reconcileCodexHooksJsonSessionStart` with the
legacy node-runner command. That command triggers the exact
`bash.exe: cannot execute binary file` POSIX-exec failure that #3426
exists to fix — so a successful-looking install was secretly restoring
the original bug.

New behaviour:
- Emit `console.warn` with the failure reason and a remediation hint,
  matching the `${yellow}⚠${reset}  Skipped …` idiom used at line 9098.
- Return `{ changed: false, wrote: false }` to skip registration for
  this runtime entirely, so the outer caller can surface "NOT installed"
  instead of "installed (but broken)".

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

* test(3426): typed-IR assertions on .cmd shim eol/quoting/passthrough + IR extension

Extend `buildCodexHookWindowsShimIR` to expose two new typed fields on
the returned IR object (CONTRIBUTING.md L558-L565 IR-first discipline):

  eol: { cmd: '\r\n' }   — CRLF is canonical for cmd.exe .cmd files
  passthroughArgs: true  — shim forwards all args via %*

Add a new describe block (Step 2b) with three IR-level assertions:

1. `eol.cmd === '\r\n'` — prevents silent EOL regression that could
   break parsing on Windows versions that require CRLF.
2. `invocation.target` is the raw unquoted path (no shell-metachar
   leakage) — quoting happens only at render time.
3. `passthroughArgs === true` — the %* forwarding contract is
   explicitly typed so regressions fail before the text is rendered.

All assertions operate on the typed IR returned by the generator, NOT
on the rendered `.cmd` file content — text-matching is the anti-pattern
CONTRIBUTING.md L522-582 prohibits.

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

* test(3426): fix Windows CI failures — update hook-command filter patterns

Four test files filtered for managed hooks in hooks.json using the
literal string `gsd-check-update.js`.  On Windows the PR-introduced
.cmd shim changes the hooks.json command to
`"path/gsd-check-update.cmd"` (no node prefix, .cmd extension), so
those filters matched 0 entries and 24 Windows subtests failed.

Fixes:
- bug-2760-codex-install-defensive.test.cjs (7 filters): change
  `/gsd-check-update\.js/` → `/gsd-check-update/` to match both
  .js (POSIX) and .cmd (Windows) commands.
- bug-3357-codex-legacy-hooks-json-migration.test.cjs (3 filters):
  same `.js` → no-extension change.
- bug-3427-3433-codex-install-shape.test.cjs (2 filters): same fix;
  add explanatory comment to uninstall assertion.
- codex-config.test.cjs (9 filters + 1 exact-command assertion):
  bulk-replace all `hooksJsonCommands.filter(cmd => cmd.includes('gsd-check-update.js'))`
  with `gsd-check-update`; make the `fresh CODEX_HOME` test platform-
  aware — on win32 assert `.cmd` shim path, on POSIX assert the
  existing `"runner" "script.js"` form (#3017).

All four suites pass locally (macOS / darwin).  Windows subtests
verified against the Windows CI failure log patterns.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(3426): address pr-review-toolkit + codex review findings

- fix(uninstall): add gsd-check-update.cmd to gsdHooks cleanup list so
  the .cmd shim is removed from disk on Windows uninstall (was left as
  orphan artifact — silent failure post-uninstall)
- test(3426): add uninstall test asserting gsd-check-update.cmd is
  deleted from hooks dir after `uninstall(true, 'codex')` (no coverage existed)
- fix(comment): correct JSDoc on buildCodexHookWindowsShimIR — shim
  content is three-line @ECHO OFF/@SETLOCAL/@runner snippet, not bare
  `@node "script.js" %*` as the old comment claimed
- fix(comment): update stale assertion message in codex-config.test.cjs
  L1457 — said "config.toml references it" but the hook is in hooks.json

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

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
 2026-05-20 23:13:50 -04:00
Tom Boucher
1d6284a718 fix(codex): remove duplicate skill copies and consolidate hook payloads 2026-05-14 13:17:39 -04:00
Tom Boucher
deeb6deb67 fix(install): accept Codex TOML floats; idempotent rollback (#3245) (#3254) 
* test: reproduce extractFrontmatter LAST-block bug (#3240)

* test: reproduce state.update progress trampling and percent formula (#3242)

Two failing regression tests:
- Bug A: state.update "Last Activity" tramples curated progress.* frontmatter via readModifyWriteStateMd → syncStateFrontmatter
- Bug B: 12 declared ROADMAP phases / 6 realized / 6/6 plans done → percent: 100 instead of 50 (phase-fraction ignored)

* test: reproduce TOML float rejection and partial rollback (#3245)

Two failing regression tests:
1. parseTomlToObject rejects valid Codex TOML floats (tool_timeout_sec = 20.0)
2. Post-install validation failure leaves skills/, agents/, VERSION on disk
   despite restoring config.toml — hybrid state after abort

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

* fix(install): accept TOML floats; idempotent codex rollback (#3245)

Two fixes for the Codex install failure introduced by #2760 CR4 finding 3:

1. parseTomlValue now accepts TOML 1.0 float literals (decimals,
   exponents, underscore separators, signed). Codex CLI's serde schema
   requires f64 for tool_timeout_sec / startup_timeout_sec — the prior
   strict-integer-only check was the inverse of what Codex requires,
   causing every config with a float to trigger a fatal schema validation
   failure. Date/time separators (-/:T/Z) are still rejected.

2. restoreCodexSnapshot is extended into a unified idempotent rollback
   that reverts ALL Codex-specific mutations on failure:
   - config.toml (existing behavior)
   - skills/gsd-* directories (new)
   - agents/gsd-*.{md,toml} files (new)
   - get-shit-done/VERSION (new)
   - orphaned atomic-write temp files (new)
   Pre-install state is captured before the first Codex write so the
   rollback reflects the true pre-GSD state. Non-gsd-* user content is
   untouched. The rollback is safe to call multiple times and before any
   snapshots are captured.

Fixes #3245

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

* changeset: pr=3254 for #3245

* test: fix source-grep lint violation in bug-3242 test (#3242)

Replace content.includes() check with line-by-line parse of STATE.md body.
The lint enforces structural assertions over raw text matching.

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

* test: mark #3242 RED tests as todo pending fix (#3242)

The three failing tests are intentional regression tests for bugs in
state.cjs that will be fixed in a separate PR. Mark them { todo: true }
so they don't block CI on this branch.

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

* fix(install): tighten TOML underscore placement validation (CR finding 1)

The float regex used [\d_]* which accepts invalid forms like 1__0, 1_.0,
and 1._0. TOML 1.0 §2 requires underscores only between digits. Switch
both the integer pre-check and the full float pattern to (?:_?\d)* so
consecutive underscores, leading underscores on a segment, and trailing
underscores on a segment are all rejected before replace(/_/g,'') can
silently normalize them into valid JS numbers.

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

* fix(install): restore pre-existing gsd-* content on rollback (CR finding 2)

The snapshot only recorded names of pre-existing skills/gsd-* dirs and
agents/gsd-* files. On a failed reinstall the rollback could delete
newly-created dirs but could not restore the bytes of dirs/files that
were overwritten, leaving the user in a hybrid state (old config.toml,
new skill files).

Now snapshot the full file tree of every pre-existing gsd-* skill dir
into codexPreInstallSkillContents (Map<name, Map<relPath, Buffer>>) and
every pre-existing agent file into codexPreInstallAgentContents
(Map<filename, Buffer>). restoreCodexSnapshot() uses these maps to
wipe-and-restore overwritten entries and only removes entries that had
no pre-install state, giving a true atomic rollback guarantee.
Reads are best-effort so a partial snapshot is still better than none.

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

* fix(install): scope temp-file cleanup to installer-owned writes (CR finding 3)

_cleanTmpFiles() was deleting any *.tmp-<pid>-<n> file found under
targetDir. This is too broad: other tools in the user's Codex/home
directory may create temp files matching the same suffix pattern, and a
GSD install rollback would silently delete them.

Add __atomicWrittenTmps (a module-level Set<string>) populated by
atomicWriteFileSync for every temp path it creates. _cleanTmpFiles()
now checks __atomicWrittenTmps.has(full) before unlinking, so only temp
files this installer process actually wrote are eligible for cleanup.

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

* fix(test): remove no-op doesNotThrow wrapping try/catch (CR finding 4)

assert.doesNotThrow(() => { try { f(); } catch(_){} }) always passes
because the catch block swallows every exception before the outer
assertion can see it. This meant the rollback-idempotency guarantee was
never actually verified.

Replace with an explicit threw flag around runCodexInstall, assert that
the install did throw (validation failure is expected), and add a
post-rollback state assertion that skills/ was not created. This gives
a loud failure surface if runCodexInstall starts crashing from inside
the rollback path, matching the intent described in the test comment.

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

* fix(test): correct describe title for float-acceptance tests (CR nitpick 1)

The describe block title said 'rejects malformed input that previously
slipped through', but the test inside now asserts that TOML floats are
accepted (the #3245 inversion). This misled readers expecting every
sub-test to assert rejection. Update the title to reflect the mixed
behaviour: floats are accepted; dates and trailing-garbage are rejected.

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

* fix(test): rename test to match what the assertion actually checks (CR nitpick 2)

The test name 'post-install config retains float literal form (20.0 not
truncated to 20)' promised a string-form invariant, but the assertion
uses numeric equality (assert.strictEqual(parsed.tool_timeout_sec, 20))
which cannot distinguish 20 from 20.0 in JS. Rename to 'post-install
config round-trips tool_timeout_sec as numeric 20' so the description
matches what the test actually verifies.

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

* fix(test): replace raw text scan with state json assertion (CR nitpick 3)

The 'Last Activity updates the body field' test was reading STATE.md as
raw text, splitting on newlines, and using lines.find/startsWith to
locate the 'Last Activity:' line — the exact pattern-match-on-source
approach prohibited by the no-source-grep testing standard.

Replace with runGsdTools('state json', tmpDir) which surfaces the body-
extracted Last Activity value as fm.last_activity in its JSON output,
and assert against that structured field instead.

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

* fix(test): correct post-rollback state assertion for early-failure case

The previous assertion checked that skills/ didn't exist, but the
installer writes skills/ before the schema validator fires. Rollback
removes gsd-* dirs inside skills/, not skills/ itself. Update the
assertion to verify that no gsd-* skill dirs survive rollback, which
is the actual invariant the test name describes.

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

* changeset: document full rollback scope (CR finding 1)

Adds config.toml restoration and orphaned atomic-write temp-file
cleanup to the changeset description — the previous text only listed
skills/, agents/, and VERSION.

* fix(install): wrap post-snapshot scope in rollback handler (CR finding 2)

Any throw between the pre-install snapshot capture and the Codex config
block (skills copy, agents copy, VERSION write, manifest write, leaked-
path scan, etc.) now triggers _codexPreConfigRollback() so the caller
is never left in a partially-installed state.  Previously only the later
config.toml mutation paths had rollback wired in.

Introduces _codexPreConfigRollback (defined right after snapshot capture)
and wraps the intervening operations in a try/catch that invokes it on
error for Codex installs; non-Codex paths are unaffected.

* test: assert threw=true to prevent vacuous pass (CR finding 4)

Two tests used bare try/catch without asserting threw === true, so they
would silently pass even if runCodexInstall never threw (k060 pattern).
Each bare catch block is replaced with a threw flag and a
strictEqual(threw, true, ...) assertion.

CR findings 2+3 are both addressed in the preceding install commit:
finding 3 (restore from snapshot manifest, not current FS state) lands
alongside the rollback-wrapper change as part of the restoreCodexSnapshot
refactor.

* fix(install): reject leading zeros in TOML float integer part per TOML 1.0 (CR finding round 4)

TOML 1.0 §2 disallows leading zeros in the integer part of numeric
literals — `01`, `00`, `01.5`, `00e2`, `+01.0`, `-01.0` are all invalid.
The pre-check and float regexes in parseTomlValue used `\d(?:_?\d)*` which
accepted any digit as the leading digit.

Both regexes are tightened to `(0|[1-9](?:_?\d)*)` for the integer part:
- `0` alone is valid
- a non-zero leading digit followed by optional underscored digits is valid
- `01`, `00`, and any variant with a leading zero and further digits is rejected

The "still rejects bare time (07:32:00)" test assertion is broadened from
`/unsupported TOML value/` to `/unsupported TOML value|trailing bytes/`
because the parser now stops at `0` and the remainder `7:32:00` is rejected
as trailing bytes — the invariant (time literals are not accepted) is unchanged.

25 new regression tests cover all rejection cases and valid TOML forms.

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

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
 2026-05-08 10:25:59 -04:00
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