Fix Codex launch — deliver prompt + drop deprecated/async hook config (CROW-492)#499
Merged
Merged
Conversation
dhilgaertner
approved these changes
Jun 11, 2026
dhilgaertner
left a comment
Contributor
There was a problem hiding this comment.
Code & Security Review
Tight, well-scoped bug-fix PR. Both reported bugs are fixed correctly, comments are accurate, and the new behavior mirrors established patterns in the codebase.
Critical Issues
None.
Security Review
Strengths:
- Bug 1 (prompt argv).
launch_codexnow passes"\$(cat $prompt_path)"exactly aslaunch_cursor(setup.sh:822) andlaunch_claude_code(setup.sh:797) already do. The$(cat …)is wrapped in double quotes and expanded by the target shell at paste time; command-substitution output is not re-parsed for quotes/$/backticks, so an arbitrary prompt body can't break out of the single argv. No new injection surface — it joins a well-trodden path. - Template (
Resources/crow-workspace-setup.sh.template:721) and skill copy (skills/crow-workspace/setup.sh:844) are kept in sync — no drift introduced. escapeTomlStringstill guards thenotifypath; no untrusted data flows into the new TOML helpers (section/keyare compile-time literals).
Concerns: None.
Code Quality
- Bug 2a (deprecated key).
removeTomlSectionLineis correct and bounded:sectionEnddefaults tolines.countand is always> start, so the(start+1)..<sectionEndrange is always valid (no crash on an empty/last section). Exact-equality key matching (not prefix) means migratingcodex_hookscan't accidentally clobber the newhookskey, and order-independence holds. Migration is idempotent — confirmed by the new test assertingsecond == toml. - Bug 2b (async hooks). Dropping
asyncis the right call given Codex silently drops the field. Note the fix is self-healing for existing installs too:installGlobalConfigoverwritesexistingHooks[eventName]for all six events, so a stalehooks.jsoncarrying"async": trueonPostToolUse/Stopgets rewritten without the flag on next launch. - Tests are strong: the new
installGlobalConfigDoesNotDeclareAsyncHookswalks every event/entry rather than spot-checking, and the migration test verifies user keys survive (memories = true) plus idempotency. - Verified locally:
arch -arm64 swift test --package-path Packages/CrowCodex→ all 31 tests pass.
Consider (non-blocking, Green):
removeTomlSectionLineonly scans inside[features]and removes the first match. A theoretical legacy config with a top-levelcodex_hooks(outside any section) wouldn't be migrated — but Crow has only ever written it under[features], so this is purely hypothetical and fine as-is.
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, 1 Green] findings.
… (CROW-492) Two distinct bugs in Codex coding sessions surfaced on Codex CLI 0.139.0: 1. Crow launched Codex with a bare `codex` command, leaving the user staring at an empty TUI even though the prompt file was written to disk. Codex 0.129+ accepts the initial prompt as a positional argv that pre-fills the composer; `launch_codex` in the workspace skill (and its template copy) now mirrors `launch_cursor` and passes `"$(cat $prompt_path)"`. The stale "Codex has no prompt-argv form" log + comment go with it. The Swift `OpenAICodexAgent.autoLaunchCommand` keeps the bare `codex\n` for in-app re-launches (matches `CursorAgent` `.work` shape); only the misleading comment is refreshed to point at the skill as the first-launch prompt path. 2. `CodexHookConfigWriter` wrote `[features].codex_hooks = true` and marked `PostToolUse`/`Stop` hooks with `"async": true`. Codex 0.129 renamed the feature flag to `hooks` (the old key still works but emits a deprecation warning), and as of 0.139 async hooks are still parsed-but-dropped. The two async events were the load-bearing ones for `CodexSignalSource`'s state-signal pipeline — card-color updates, auto-respond, completion detection were silently broken for Codex sessions, not just noisy. Now: `hooks = true` is written under `[features]`, any legacy `codex_hooks` entry is migrated/removed via a new `removeTomlSectionLine` helper, and `generateHooks` no longer emits the `async` field. All six hook events now run sync and reach `CodexSignalSource`. Tests updated: TOML expectations swapped to `hooks = true`, added an idempotent migration test that pre-seeds legacy `codex_hooks` and asserts it is removed, added a test that walks every event in `hooks.json` and asserts no entry declares `async`. 🐦⬛ Generated with Claude Code, orchestrated by Crow Co-Authored-By: Claude <[email protected]> Crow-Session: 8B58E578-FAD3-4C2D-8EBF-2E6DE8651170
a5066bd to
de817f5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #492
Summary
launch_codexin the workspace skill (and its template copy) now passes\"\$(cat \$prompt_path)\"as Codex's positional argv so the TUI starts with the ticket prompt pre-filled, matching Codex 0.129+ behavior and mirroringlaunch_cursor. The stale "Codex has no prompt-argv form" log + comment go with it.OpenAICodexAgent.autoLaunchCommandkeeps the barecodex\nfor in-app re-launches (matchesCursorAgent.workshape); the misleading comment is refreshed to point at the skill as the first-launch prompt path.CodexHookConfigWriter.installGlobalTomlConfignow writes[features].hooks = true(renamed in Codex 0.129; the oldcodex_hooksalias emits a deprecation warning). Any legacycodex_hooksentry is migrated/removed via a newremoveTomlSectionLinehelper — idempotent on subsequent runs.\"async\": truefromPostToolUseandStophook entries. Codex 0.139 still silently skips async hooks at execution, which meantCodexSignalSource's state-signal pipeline (card-color updates, auto-respond, completion detection) was silently broken for those two events — not just noisy. All six hooks now run sync and reach Crow.Test plan
arch -arm64 swift test --package-path Packages/CrowCodex— all 31 tests pass, including:installGlobalTomlConfigCreatesFreshFileupdated to expecthooks = trueand rejectcodex_hooks.installGlobalTomlConfigPreservesUserSettingsupdated the same way.installGlobalTomlConfigMigratesLegacyCodexHooksKey— pre-seedscodex_hooks = true, asserts it is replaced withhooks = true, and asserts idempotency on a second run.installGlobalConfigDoesNotDeclareAsyncHooks— walks every event inhooks.jsonand asserts no entry sets"async".~/.codex/config.tomlwith[features].codex_hooks = true, restart Crow, confirm migration tohooks = true.PreToolUse→PostToolUse→Stop. None of this worked before.🐦⬛ Generated with Claude Code, orchestrated by Crow