Skip to content

Fix Codex hook config: rename to [features].hooks, drop async#496

Merged
dgershman merged 1 commit into
mainfrom
feature/crow-494-codex-hook-config-fix
Jun 11, 2026
Merged

Fix Codex hook config: rename to [features].hooks, drop async#496
dgershman merged 1 commit into
mainfrom
feature/crow-494-codex-hook-config-fix

Conversation

@dgershman

Copy link
Copy Markdown
Collaborator

Summary

  • Switch the ~/.codex/config.toml writer from the deprecated [features].codex_hooks key to [features].hooks (Codex v0.139.0+).
  • One-shot migration: strip a legacy codex_hooks = … line under [features] so users converging from older Crow installs end up with a single, current entry. Idempotent.
  • Empty out asyncEvents in CodexHookConfigWriter so no entry is written with async: true — Codex's hook runtime is sync-only and silently skipped those, breaking Crow's session-state detection (auto-respond, card-color, etc.).

Why this matters

The deprecation warning is loud; the async-skip was silent. With async hooks skipped, PostToolUse and Stop never reached CodexSignalSource, so Crow's UI never observed Codex sessions transitioning to working/done — auto-respond gates and card-color updates were silently broken.

Test plan

  • swift test --filter CodexHookConfigWriter — all 8 tests pass, including 2 new ones (installGlobalTomlConfigMigratesLegacyCodexHooksKey, installGlobalConfigEmitsNoAsyncHooks).
  • swift test for full CrowCodex package — 31 tests pass.
  • Manual: seed ~/.codex/config.toml with [features]\ncodex_hooks = true, launch Crow with a Codex session, confirm no deprecation/async-skip warnings and that the file ends up with hooks = true only.
  • Manual: run a tool in a Codex session, confirm card color reflects working → done transitions.

Closes #494

Codex v0.139.0+ renamed `[features].codex_hooks` to `[features].hooks` and
silently skips any hook marked `async: true` because its runtime is sync-only.
The deprecation warning was loud; the async-skip was silent, breaking Crow's
session-state detection (auto-respond, card-color, etc.) on every Codex
session because `PostToolUse` and `Stop` never reached `CodexSignalSource`.

- Empty out `asyncEvents` in `CodexHookConfigWriter` so no entry is written
  with `async: true`.
- Switch the TOML key from `codex_hooks` to `hooks`.
- Add `removeTomlSectionLine` helper + a one-shot migration step in
  `installGlobalTomlConfig` that strips a legacy `codex_hooks` line under
  `[features]` so users converging from older installs end up with a single,
  current entry. Idempotent.
- Update existing tests + add tests covering the migration path and the
  no-async invariant.

Closes #494
@dgershman dgershman requested a review from dhilgaertner as a code owner June 11, 2026 23:11
@dgershman dgershman added the crow:merge Crow auto-merge on green label Jun 11, 2026

@dhilgaertner dhilgaertner left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code & Security Review

Critical Issues

None.

Security Review

Strengths:

  • No untrusted input. crowPath is escaped via escapeTomlString before being embedded in the TOML notify line; the hooks.json command path flows through JSONSerialization.
  • File writes are atomic (write(toFile:atomically:true) / Data.write), avoiding partial-write corruption of user config.
  • The line-oriented merge preserves user-authored config rather than clobbering config.toml / hooks.json.

Concerns:

  • None.

Code Quality

  • The migration ordering is correct: removeTomlSectionLine(..., key: "codex_hooks") runs before upsertTomlSectionLine(..., key: "hooks"). Since lineKey treats codex_hooks and hooks as distinct keys, stripping the legacy line first is what prevents both keys lingering — verified by installGlobalTomlConfigMigratesLegacyCodexHooksKey, including the idempotency re-run assertion.
  • removeTomlSectionLine mirrors the existing upsertTomlSectionLine section-scan logic (exact [features] header match, bounded to the next section header), so it stays consistent with the file's established minimal-merge style and won't touch keys in other sections.
  • Emptying asyncEvents cleanly removes the async flag in generateHooks; because PostToolUse/Stop are in allEvents, installGlobalConfig overwrites any stale async entries left in an existing hooks.json, so the hooks.json side migrates implicitly. installGlobalConfigEmitsNoAsyncHooks guards the regression.
  • The rationale (Codex v0.139.0 sync-only, silent-skip) is captured in code comments, not just the PR body — good for the next reader.
  • Migration is wired into launch at Sources/Crow/App/AppDelegate.swift:440, and no other codex_hooks references remain in source.

Verification

  • swift test --filter CodexHookConfigWriter → 8/8 pass, including the 2 new tests.

Summary Table

Color Meaning Verdict effect
Red Must fix Request changes
Yellow Should fix Request changes
Green Consider Approve allowed

Recommendation: Approve — driven by [0 Red, 0 Yellow, 0 Green] findings. Focused, correct, well-tested config fix with a sound idempotent migration.


🐦‍⬛ Reviewed by Crow via Claude Code

@dgershman dgershman merged commit 8d1f804 into main Jun 11, 2026
2 checks passed
@dgershman dgershman deleted the feature/crow-494-codex-hook-config-fix branch June 11, 2026 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

crow:merge Crow auto-merge on green

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Codex hook config: writes deprecated [features].codex_hooks + async hooks Codex silently skips (breaks session-state detection)

2 participants