Repository navigation
Codex: make installed stop hooks fire-and-forget - #7410
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds ChangesCodex Stop Hook Behavior and Test Refactor
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/CLICodexHookTimeoutRegressionTests.swift`:
- Around line 147-209: The two “returns before slow cmux command finishes” tests
are duplicating the same setup and assertions, so extract a shared parametrized
helper from codexInstalledHookReturnsBeforeSlowCmuxCommandFinishes and
codexInstalledStopHookReturnsBeforeSlowCmuxCommandFinishes. Make the helper
accept the event name, sleep duration, and expected routed args substring, then
reuse it for both tests while keeping the fake CLI setup, install/run flow, and
file-wait assertions in one place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5bcfc54d-2ab5-4d64-b0a5-217b4e697604
📒 Files selected for processing (2)
CLI/CMUXCLI+AgentHookDefinitions.swiftcmuxTests/CLICodexHookTimeoutRegressionTests.swift
| @Test func codexInstalledStopHookReturnsBeforeSlowCmuxCommandFinishes() throws { | ||
| let cliPath = try bundledCLIPath() | ||
| let root = FileManager.default.temporaryDirectory | ||
| .appendingPathComponent("cmux-codex-stop-hook-async-\(UUID().uuidString)", isDirectory: true) | ||
| let codexHome = root.appendingPathComponent(".codex", isDirectory: true) | ||
| let fakeCLI = root.appendingPathComponent("cmux", isDirectory: false) | ||
| let capturedStdin = root.appendingPathComponent("hook-stdin.json", isDirectory: false) | ||
| let capturedArgs = root.appendingPathComponent("hook-args.txt", isDirectory: false) | ||
| let capturedPID = root.appendingPathComponent("hook-pid.txt", isDirectory: false) | ||
| let doneFile = root.appendingPathComponent("hook-done.txt", isDirectory: false) | ||
| try FileManager.default.createDirectory(at: codexHome, withIntermediateDirectories: true) | ||
| defer { try? FileManager.default.removeItem(at: root) } | ||
|
|
||
| try makeExecutableShellFile(at: fakeCLI, lines: [ | ||
| "#!/bin/sh", | ||
| "printf '%s\\n' \"$*\" > \"$CMUX_TEST_ARGS\"", | ||
| "printf '%s\\n' \"$CMUX_CODEX_PID\" > \"$CMUX_TEST_PID\"", | ||
| "cat > \"$CMUX_TEST_STDIN\"", | ||
| "sleep 2", | ||
| "printf done > \"$CMUX_TEST_DONE\"", | ||
| ]) | ||
|
|
||
| let install = runProcess( | ||
| executablePath: cliPath, | ||
| arguments: ["hooks", "codex", "install", "--yes"], | ||
| environment: codexHookTestEnvironment(root: root, codexHome: codexHome), | ||
| timeout: 5 | ||
| ) | ||
| #expect(!install.timedOut, Comment(rawValue: install.stderr)) | ||
| #expect(install.status == 0, Comment(rawValue: install.stderr)) | ||
|
|
||
| let command = try #require( | ||
| codexHookEntries(in: codexHome).first { $0.eventName == "Stop" }?.command | ||
| ) | ||
| let payload = #"{"session_id":"codex-session","stop_hook_active":false}"# | ||
| let run = runProcess( | ||
| executablePath: "/bin/sh", | ||
| arguments: ["-c", command], | ||
| environment: [ | ||
| "HOME": root.path, | ||
| "PATH": "/usr/bin:/bin:/usr/sbin:/sbin", | ||
| "TMPDIR": root.path, | ||
| "CMUX_SURFACE_ID": "surface-123", | ||
| "CMUX_SOCKET_PATH": "/tmp/cmux-test.sock", | ||
| "CMUX_BUNDLED_CLI_PATH": fakeCLI.path, | ||
| "CMUX_CODEX_PID": "4242", | ||
| "CMUX_TEST_STDIN": capturedStdin.path, | ||
| "CMUX_TEST_ARGS": capturedArgs.path, | ||
| "CMUX_TEST_PID": capturedPID.path, | ||
| "CMUX_TEST_DONE": doneFile.path, | ||
| ], | ||
| standardInput: payload, | ||
| timeout: 1 | ||
| ) | ||
|
|
||
| #expect(!run.timedOut, Comment(rawValue: run.stderr)) | ||
| #expect(run.status == 0, Comment(rawValue: run.stderr)) | ||
| #expect(run.stdout == "{}\n") | ||
| #expect(waitForFile(capturedStdin, containing: payload, timeout: 1)) | ||
| #expect(waitForFile(capturedArgs, containing: "--socket /tmp/cmux-test.sock hooks codex stop", timeout: 1)) | ||
| #expect(waitForFile(capturedPID, containing: "4242", timeout: 1)) | ||
| #expect(waitForFile(doneFile, containing: "done", timeout: 3)) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extract shared helper for the two "returns before slow cmux command finishes" tests.
codexInstalledStopHookReturnsBeforeSlowCmuxCommandFinishes duplicates almost the entire body of codexInstalledHookReturnsBeforeSlowCmuxCommandFinishes (fake CLI setup, install, run, and assertions), differing only in event name (UserPromptSubmit vs Stop), sleep duration, and the expected routed subcommand string. Consider extracting a shared parametrized helper (event name, sleep seconds, expected args substring) to avoid the two tests drifting independently.
♻️ Sketch of a shared helper
- `@Test` func codexInstalledStopHookReturnsBeforeSlowCmuxCommandFinishes() throws {
- let cliPath = try bundledCLIPath()
- ... (duplicated setup) ...
- }
+ `@Test` func codexInstalledStopHookReturnsBeforeSlowCmuxCommandFinishes() throws {
+ try assertInstalledHookReturnsBeforeSlowCmuxCommandFinishes(
+ eventName: "Stop",
+ fakeCLISleepSeconds: 2,
+ payload: #"{"session_id":"codex-session","stop_hook_active":false}"#,
+ expectedRoutedArgsSubstring: "--socket /tmp/cmux-test.sock hooks codex stop",
+ doneWaitTimeout: 3
+ )
+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmuxTests/CLICodexHookTimeoutRegressionTests.swift` around lines 147 - 209,
The two “returns before slow cmux command finishes” tests are duplicating the
same setup and assertions, so extract a shared parametrized helper from
codexInstalledHookReturnsBeforeSlowCmuxCommandFinishes and
codexInstalledStopHookReturnsBeforeSlowCmuxCommandFinishes. Make the helper
accept the event name, sleep duration, and expected routed args substring, then
reuse it for both tests while keeping the fake CLI setup, install/run flow, and
file-wait assertions in one place.
Greptile SummaryThis PR makes the installed Codex
Confidence Score: 5/5Safe to merge — the production change is a single predicate extension with clear intent, the existing hook execution path is unchanged, and new regression tests confirm both the quick-return behavior and the stale-stop guard. The production diff is one line in codexHookCanRunFireAndForget. The surrounding call site already handles session-start and prompt-submit identically, so the stop case slots in cleanly without any new state or branching. New black-box tests directly verify that the installed stop script returns before its slow child finishes and that an out-of-date turn_id does not incorrectly mark a newer turn idle. No production-path actors, blocking primitives, or data-loading concerns are touched. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[hookCommandString called for Codex agent] --> B{codexHookCanRunFireAndForget?}
B -- "session-start / prompt-submit / stop NEW" --> C[codexFireAndForgetAgentHookShellCommand nohup sh -c wrapper]
B -- "session-end / other" --> D[agentHookShellCommand synchronous]
C --> E[codexPersistentHookScriptCommand write script to disk]
D --> E
E --> F[Codex hooks.json command = path to script]
G[Codex fires Stop event] --> H[Installed script runs]
H --> I[nohup sh -c background child started]
I --> J[Script exits immediately]
I --> K[Background child: hooks codex stop]
K --> L{turn_id matches current turn?}
L -- No --> M[Stale: skip notify/idle]
L -- Yes --> N[Mark turn Idle / notify surface]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[hookCommandString called for Codex agent] --> B{codexHookCanRunFireAndForget?}
B -- "session-start / prompt-submit / stop NEW" --> C[codexFireAndForgetAgentHookShellCommand nohup sh -c wrapper]
B -- "session-end / other" --> D[agentHookShellCommand synchronous]
C --> E[codexPersistentHookScriptCommand write script to disk]
D --> E
E --> F[Codex hooks.json command = path to script]
G[Codex fires Stop event] --> H[Installed script runs]
H --> I[nohup sh -c background child started]
I --> J[Script exits immediately]
I --> K[Background child: hooks codex stop]
K --> L{turn_id matches current turn?}
L -- No --> M[Stale: skip notify/idle]
L -- Yes --> N[Mark turn Idle / notify surface]
Reviews (4): Last reviewed commit: "Avoid codex hook test helper symbol coll..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| timeout: 10 | ||
| ) | ||
| #expect(!install.timedOut, Comment(rawValue: install.stderr)) | ||
| #expect(install.status == 0, Comment(rawValue: install.stderr)) |
There was a problem hiding this comment.
codexHookInstallations
The #expect(!install.timedOut, ...) assertion was removed when the install timeout was bumped from 5 s to 10 s. When the install does time out, runProcess kills the process (SIGTERM then SIGKILL), producing a non-zero terminationStatus, so #expect(install.status == 0, ...) still catches it — but the failure message shows only stderr content rather than clearly indicating a timeout, making slow-CI failures harder to distinguish from logic errors. Both new tests added by this PR correctly retain the timedOut check.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| #expect(waitForCondition(timeout: 2) { | ||
| commands.snapshot().count > staleStopStart | ||
| }) |
There was a problem hiding this comment.
#expect(waitForCondition(timeout: 2) { commands.snapshot().count > staleStopStart }) asserts that the stale stop's background process sends at least one socket command within 2 seconds. If the production stop handler detects staleness via a pure in-memory turn-ID check and exits before any socket call, this #expect fails even though the behavior is correct — leaving the actual correctness assertion (staleStopCommands check below) untested. Consider replacing with a fixed settling sleep or removing the gate and relying solely on the staleStopCommands content assertion, so the test doesn't couple to whether the handler queries the socket to discover staleness.
77c6fc5 to
2bb8596
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmuxTests/CLICodexHookTimeoutRegressionTests.swift (1)
289-299: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winWait for the stale Stop work to complete before asserting.
Line 289 only waits for the first post-baseline socket line. If the async Stop later emits
notify_targetorset_status codex ... Idle, this regression can pass before observing the bug. Add a real completion signal from the mock socket handler or wait for the Stop connection/batch to finish, then assert over the full Stop batch. As per coding guidelines, tests should assert on causality rather than latency; as per path instructions, liveness/Idle regressions must avoid stale reads.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmuxTests/CLICodexHookTimeoutRegressionTests.swift` around lines 289 - 299, The stale Stop regression test is only waiting for the first post-baseline socket message, so it can assert before the async Stop work has fully finished. Update the test around the existing waitForCondition and staleStopCommands capture in CLICodexHookTimeoutRegressionTests to wait on a real completion signal from the mock socket handler, or otherwise wait for the Stop connection/batch to fully drain before checking for notify_target or set_status codex Idle. Then perform the assertion over the complete Stop batch so the test reflects causality instead of timing.Sources: Coding guidelines, Path instructions
cmuxTests/CLICodexHookTimeoutRegressionTestSupport.swift (1)
233-249: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDrain subprocess output while the process is running.
waitUntilExit()runs before stdout/stderr are read, so a child that writes enough output can block on a full pipe and make this helper report a timeout for a healthy process. Drain both pipes concurrently or redirect to temporary files before waiting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmuxTests/CLICodexHookTimeoutRegressionTestSupport.swift` around lines 233 - 249, The timeout helper in CLICodexHookTimeoutRegressionTestSupport is waiting on waitUntilExit before draining stdout/stderr, which can deadlock a chatty child process on full pipes. Update this helper to consume both pipes concurrently while the process runs, or redirect output to temporary files before waiting, and keep the existing timeout/terminate logic intact around the process wait and kill flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cmuxTests/CLICodexHookTimeoutRegressionTests.swift`:
- Around line 289-299: The stale Stop regression test is only waiting for the
first post-baseline socket message, so it can assert before the async Stop work
has fully finished. Update the test around the existing waitForCondition and
staleStopCommands capture in CLICodexHookTimeoutRegressionTests to wait on a
real completion signal from the mock socket handler, or otherwise wait for the
Stop connection/batch to fully drain before checking for notify_target or
set_status codex Idle. Then perform the assertion over the complete Stop batch
so the test reflects causality instead of timing.
In `@cmuxTests/CLICodexHookTimeoutRegressionTestSupport.swift`:
- Around line 233-249: The timeout helper in
CLICodexHookTimeoutRegressionTestSupport is waiting on waitUntilExit before
draining stdout/stderr, which can deadlock a chatty child process on full pipes.
Update this helper to consume both pipes concurrently while the process runs, or
redirect output to temporary files before waiting, and keep the existing
timeout/terminate logic intact around the process wait and kill flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 043d430f-ea40-4760-8007-200ae955c51a
📒 Files selected for processing (2)
cmuxTests/CLICodexHookTimeoutRegressionTestSupport.swiftcmuxTests/CLICodexHookTimeoutRegressionTests.swift
* Codex: make installed stop hook fire-and-forget * Tests: cover async Codex stale stop hook * Tests: split Codex hook timeout regression support * Tests: wire Codex hook timeout support file * Avoid codex hook test helper symbol collisions (cherry picked from commit ed70dad)
This branch was successfully deployed
1 participant
Summary
Stophook use the same fire-and-forget wrapper as session-start and prompt-submitStopcannot mark a newer Codex turn idleVerification
CMUX_SKIP_ZIG_BUILD=1 xcodebuild -project cmux.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -derivedDataPath /tmp/cmux-stopfx4b test -only-testing:cmuxTests/CLICodexHookTimeoutRegressionTests(new async stale-stop regression passed; one existing harness-style timeout assertion remained flaky despite subprocess status 0 and expected stdout)autoreview --mode branch --base origin/maincleanNeed help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Made the installed Codex Stop hook fire-and-forget so it returns fast, matching session-start and prompt-submit. This reduces CLI blocking when stopping a turn and aligns behavior across Codex hooks.
hooks.jsonand assert fire-and-forget scripts for session-start, prompt-submit, and stop; added coverage that the installed Stop returns quickly and that a stale async Stop cannot mark a newer turn idle.CLICodexHookTimeoutRegressionTestSupport.swift, wired it into the test target, and prefixed Codex test helper symbols to avoid collisions.Written for commit 0345eb6. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests
Chores