Make Codex hooks fire-and-forget - #6110
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesCodex Fire-and-Forget Hook Dispatch
Sequence DiagramsequenceDiagram
participant Caller as Agent Hook Caller
participant HookCmd as agentHookShellCommand()
participant CodexCmd as codexFireAndForgetAgentHookShellCommand()
participant Shell as Shell Execution
participant cmux as cmux CLI
Caller->>HookCmd: invoke(def.name="codex", command)
HookCmd->>CodexCmd: generate shell snippet
CodexCmd->>CodexCmd: check prerequisites<br/>(CMUX_SURFACE_ID, disable env var)
CodexCmd->>CodexCmd: resolve cmux executable
CodexCmd->>CodexCmd: create temp payload file
CodexCmd-->>Caller: return shell snippet<br/>(immediate, non-blocking)
Caller->>Shell: sh -c snippet
Shell->>cmux: nohup cmux --socket ...<br/>hooks codex prompt-submit < payload &
Shell->>Shell: arm 30-second watchdog
Shell-->>Caller: {} (instant return)
cmux->>cmux: execute asynchronously
Shell->>Shell: wait briefly, kill watchdog,<br/>rm payload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 19 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (19 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1344b56020
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if def.name == "codex" { | ||
| return codexFireAndForgetAgentHookShellCommand(command, for: def) |
There was a problem hiding this comment.
When CMUX_SURFACE_ID is set, this branch sends every Codex lifecycle hook (session-start, prompt-submit, and stop) through an independent background process. If Codex finishes a short turn while the earlier prompt-submit process is delayed starting or connecting to the socket, the later Stop process can run first; the prompt-submit handler in CLI/cmux.swift then still writes lifecycle/status back to Running after the Stop path has written Idle, leaving the sidebar/session store stale. The fire-and-forget wrapper should serialize per-session lifecycle commands or be limited to hooks where ordering cannot change state.
Useful? React with 👍 / 👎.
Greptile SummaryCodex
Confidence Score: 4/5Safe to merge with one correctness gap in the handler ordering worth addressing before it silently distorts analytics. The restructured session-start and prompt-submit handlers call CLI/cmux.swift — specifically the ordering of Important Files Changed
Sequence DiagramsequenceDiagram
participant Codex
participant HookShell as Hook Shell (fire-and-forget)
participant TmpFile as Temp File ($TMPDIR)
participant nohup as nohup bg process
participant cmux as cmux CLI
participant Store as ClaudeHookSessionStore
participant Socket as cmux Socket
Codex->>HookShell: "invoke hook (stdin = JSON payload)"
HookShell->>TmpFile: "mktemp + cat stdin > $payload"
HookShell->>nohup: "nohup sh -c runner $payload $cmux_cli ... &"
HookShell-->>Codex: "echo {} (immediate return)"
nohup->>cmux: "exec cmux hooks codex session-start < $payload"
cmux->>Store: upsertCodexSessionStartIfFresh (PID check)
alt session is stale (active turn or same-PID completed turn)
Store-->>cmux: return false
cmux-->>nohup: "print {} + return"
else session is fresh
Store-->>cmux: return true (clear lastPromptTurnId, keep terminalTurnIds)
cmux->>Socket: sendAgentFeedTelemetry
cmux->>Socket: publishAgentSurfaceResumeBinding
cmux->>Socket: set_agent_lifecycle / set_status
end
nohup->>TmpFile: rm -f $payload
Reviews (11): Last reviewed commit: "Simplify Codex prompt submit result stat..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private static func codexFireAndForgetAgentHookShellCommand(_ command: String, for def: AgentHookDef) -> String { | ||
| let routedArguments = command.hasPrefix("cmux ") ? String(command.dropFirst("cmux ".count)) : command | ||
| return "cmux_cli=\"${CMUX_BUNDLED_CLI_PATH:-}\"; if [ -z \"$cmux_cli\" ] || [ ! -x \"$cmux_cli\" ]; then cmux_cli=\"$(command -v cmux 2>/dev/null || true)\"; fi; if [ -n \"$CMUX_SURFACE_ID\" ] && [ \"$\(def.disableEnvVar)\" != \"1\" ] && [ -n \"$cmux_cli\" ]; then payload=\"$(mktemp \"${TMPDIR:-/tmp}/cmux-codex-hook.XXXXXX\" 2>/dev/null || mktemp -t cmux-codex-hook 2>/dev/null)\" || { echo '{}'; exit 0; }; cat >\"$payload\" || true; if [ -n \"${CMUX_SOCKET_PATH:-}\" ]; then nohup sh -c 'payload=\"$1\"; shift; \"$@\" <\"$payload\" >/dev/null 2>&1 & child=\"$!\"; ( sleep 30; kill \"$child\" 2>/dev/null || true ) & watchdog=\"$!\"; wait \"$child\" 2>/dev/null || true; kill \"$watchdog\" 2>/dev/null || true; rm -f \"$payload\"' cmux-codex-hook \"$payload\" \"$cmux_cli\" --socket \"$CMUX_SOCKET_PATH\" \(routedArguments) >/dev/null 2>&1 & else nohup sh -c 'payload=\"$1\"; shift; \"$@\" <\"$payload\" >/dev/null 2>&1 & child=\"$!\"; ( sleep 30; kill \"$child\" 2>/dev/null || true ) & watchdog=\"$!\"; wait \"$child\" 2>/dev/null || true; kill \"$watchdog\" 2>/dev/null || true; rm -f \"$payload\"' cmux-codex-hook \"$payload\" \"$cmux_cli\" \(routedArguments) >/dev/null 2>&1 & fi; echo '{}'; else echo '{}'; fi" | ||
| } |
There was a problem hiding this comment.
sleep 30 watchdog is a wall-clock process-lifetime timer in production shell
The shell script embedded in codexFireAndForgetAgentHookShellCommand uses ( sleep 30; kill "$child" 2>/dev/null || true ) & to cap how long the background cmux invocation can run. This is a fixed-time wall-clock wait used for process lifecycle synchronization — exactly the pattern flagged by the runtime-no-hacky-sleeps rule. If cmux blocks on a socket for slightly over 30 s under load, the watchdog silently kills it without any signal from the owning subsystem, dropping the hook payload with no indication of failure.
On macOS, /usr/bin/timeout is not available by default, but consider perl -e 'alarm(30); exec @ARGV' -- or propagating a SIGTERM-based approach via Swift Process.terminate() rather than generating a shell-level watchdog. At minimum, the watchdog timeout should be a named constant, not a hardcoded literal embedded in a long string.
File Used: .github/review-bot-rules/runtime-no-hacky-sleeps.md (source)
1344b56 to
46d4a90
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmuxTests/CLICodexHookTimeoutRegressionTests.swift (1)
1-133:⚠️ Potential issue | 🟠 MajorReorganize into two commits to satisfy regression test policy.
This PR adds the regression test and implementation in a single commit. Per coding guidelines, regression tests require a two-commit structure:
- Commit 1: Add the failing test only (CI goes red, proving the test catches the bug)
- Commit 2: Add the fix (CI goes green)
This makes it visible in the GitHub PR UI that the test genuinely fails without the fix. Currently, both
testCodexHookInstallReplacesSynchronousBundledHook()andtestCodexInstalledHookReturnsBeforeSlowCmuxCommandFinishes()are added alongside their implementation, so CI cannot prove the tests are effective.🤖 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 1 - 133, Split your changes into two separate commits per regression test policy. In the first commit, add only the two test methods testCodexHookInstallReplacesSynchronousBundledHook() and testCodexInstalledHookReturnsBeforeSlowCmuxCommandFinishes() along with their supporting helper methods (codexHookTestEnvironment, codexHookCommands, makeExecutableShellFile, waitForFile) to the test file. Do not include any implementation changes in this commit. In the second commit, add only the implementation code that makes these failing tests pass. This structure ensures the PR demonstrates that the tests actually fail without the fix, validating their effectiveness.Source: Coding guidelines
🤖 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 `@CLI/CMUXCLI`+CodexFireAndForgetHooks.swift:
- Around line 1-6: The codexFireAndForgetAgentHookShellCommand method contains a
duplicated nohup sh -c block that appears in both the socket and non-socket
branches, with the only difference being the optional --socket flag. Refactor
this by building the cmux invocation arguments conditionally first (constructing
a string that includes --socket "$CMUX_SOCKET_PATH" only when CMUX_SOCKET_PATH
is set), then use a single nohup sh -c block with the conditionally-built
arguments instead of duplicating the entire dispatch block.
---
Outside diff comments:
In `@cmuxTests/CLICodexHookTimeoutRegressionTests.swift`:
- Around line 1-133: Split your changes into two separate commits per regression
test policy. In the first commit, add only the two test methods
testCodexHookInstallReplacesSynchronousBundledHook() and
testCodexInstalledHookReturnsBeforeSlowCmuxCommandFinishes() along with their
supporting helper methods (codexHookTestEnvironment, codexHookCommands,
makeExecutableShellFile, waitForFile) to the test file. Do not include any
implementation changes in this commit. In the second commit, add only the
implementation code that makes these failing tests pass. This structure ensures
the PR demonstrates that the tests actually fail without the fix, validating
their effectiveness.
🪄 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: 26f38cc8-cef5-4002-a9a9-1c02fa5e14e1
📒 Files selected for processing (5)
CLI/CMUXCLI+AgentHookDefinitions.swiftCLI/CMUXCLI+CodexFireAndForgetHooks.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CLICodexHookTimeoutRegressionTests.swiftcmuxTests/CLIGenericHookPersistenceTests.swift
💤 Files with no reviewable changes (1)
- cmuxTests/CLIGenericHookPersistenceTests.swift
46d4a90 to
b936804
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@CLI/CMUXCLI`+CodexFireAndForgetHooks.swift:
- Around line 2-4: The codexFireAndForgetAgentHookShellCommand method generates
a shell command that lacks a stable Codex-owned marker, preventing proper
identification and replacement of stale hooks in hooks.json. The shell-rich
wrapper with semicolons and ampersands is rejected by
isLegacyCmuxOwnedHookCommand, so the system can only match by exact string
equality, allowing older variants to coexist and cause duplicate Codex
executions. Add a distinctive Codex marker comment or identifier string (similar
to the existing cmux hooks codex or cmux codex-hook patterns) to the generated
shell command that makes it reliably identifiable for cleanup purposes,
independent of minor shell syntax variations.
- Line 4: The shell script in the returned string has a file validation check
that accepts directories as valid executables because [ -x "$cmux_cli" ] returns
true for searchable directories. This causes directory-valued
CMUX_BUNDLED_CLI_PATH to bypass the fallback to command -v cmux, resulting in
silent failures. Replace the check [ ! -x "$cmux_cli" ] with [ ! -f "$cmux_cli"
] in the condition to require a non-directory file instead, mirroring the
behavior of isExecutableFilePath(_:). This change should be made in the initial
validation condition within the shell script string where CMUX_BUNDLED_CLI_PATH
is checked.
🪄 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: 33d0368f-19f8-43a4-ba93-56e92452a8ff
📒 Files selected for processing (4)
CLI/CMUXCLI+AgentHookDefinitions.swiftCLI/CMUXCLI+CodexFireAndForgetHooks.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CLICodexHookTimeoutRegressionTests.swift
| static func codexFireAndForgetAgentHookShellCommand(_ command: String, for def: AgentHookDef) -> String { | ||
| let routedArguments = command.hasPrefix("cmux ") ? String(command.dropFirst("cmux ".count)) : command | ||
| return "cmux_cli=\"${CMUX_BUNDLED_CLI_PATH:-}\"; if [ -z \"$cmux_cli\" ] || [ ! -x \"$cmux_cli\" ]; then cmux_cli=\"$(command -v cmux 2>/dev/null || true)\"; fi; if [ -n \"$CMUX_SURFACE_ID\" ] && [ \"$\(def.disableEnvVar)\" != \"1\" ] && [ -n \"$cmux_cli\" ]; then payload=\"$(mktemp \"${TMPDIR:-/tmp}/cmux-codex-hook.XXXXXX\" 2>/dev/null || mktemp -t cmux-codex-hook 2>/dev/null)\" || { echo '{}'; exit 0; }; cat >\"$payload\" || true; if [ -n \"${CMUX_SOCKET_PATH:-}\" ]; then nohup sh -c 'payload=\"$1\"; shift; \"$@\" <\"$payload\" >/dev/null 2>&1 & child=\"$!\"; ( sleep 30; kill \"$child\" 2>/dev/null || true ) & watchdog=\"$!\"; wait \"$child\" 2>/dev/null || true; kill \"$watchdog\" 2>/dev/null || true; rm -f \"$payload\"' cmux-codex-hook \"$payload\" \"$cmux_cli\" --socket \"$CMUX_SOCKET_PATH\" \(routedArguments) >/dev/null 2>&1 & else nohup sh -c 'payload=\"$1\"; shift; \"$@\" <\"$payload\" >/dev/null 2>&1 & child=\"$!\"; ( sleep 30; kill \"$child\" 2>/dev/null || true ) & watchdog=\"$!\"; wait \"$child\" 2>/dev/null || true; kill \"$watchdog\" 2>/dev/null || true; rm -f \"$payload\"' cmux-codex-hook \"$payload\" \"$cmux_cli\" \(routedArguments) >/dev/null 2>&1 & fi; echo '{}'; else echo '{}'; fi" |
There was a problem hiding this comment.
Add a stable Codex-owned marker to this wrapper.
This command no longer contains either existing Codex cleanup marker (cmux hooks codex / cmux codex-hook), and isLegacyCmuxOwnedHookCommand rejects shell-rich wrappers like this one because they contain ; and &. Reinstall/uninstall can therefore recognize the current async hook only by exact string equality, so an older fire-and-forget variant can survive in hooks.json beside the new one and reintroduce duplicate Codex executions.
♻️ Suggested fix
extension CMUXCLI {
static func codexFireAndForgetAgentHookShellCommand(_ command: String, for def: AgentHookDef) -> String {
let routedArguments = command.hasPrefix("cmux ") ? String(command.dropFirst("cmux ".count)) : command
- return "cmux_cli=\"${CMUX_BUNDLED_CLI_PATH:-}\"; if [ -z \"$cmux_cli\" ] || [ ! -x \"$cmux_cli\" ]; then cmux_cli=\"$(command -v cmux 2>/dev/null || true)\"; fi; if [ -n \"$CMUX_SURFACE_ID\" ] && [ \"$\(def.disableEnvVar)\" != \"1\" ] && [ -n \"$cmux_cli\" ]; then payload=\"$(mktemp \"${TMPDIR:-/tmp}/cmux-codex-hook.XXXXXX\" 2>/dev/null || mktemp -t cmux-codex-hook 2>/dev/null)\" || { echo '{}'; exit 0; }; cat >\"$payload\" || true; if [ -n \"${CMUX_SOCKET_PATH:-}\" ]; then nohup sh -c 'payload=\"$1\"; shift; \"$@\" <\"$payload\" >/dev/null 2>&1 & child=\"$!\"; ( sleep 30; kill \"$child\" 2>/dev/null || true ) & watchdog=\"$!\"; wait \"$child\" 2>/dev/null || true; kill \"$watchdog\" 2>/dev/null || true; rm -f \"$payload\"' cmux-codex-hook \"$payload\" \"$cmux_cli\" --socket \"$CMUX_SOCKET_PATH\" \(routedArguments) >/dev/null 2>&1 & else nohup sh -c 'payload=\"$1\"; shift; \"$@\" <\"$payload\" >/dev/null 2>&1 & child=\"$!\"; ( sleep 30; kill \"$child\" 2>/dev/null || true ) & watchdog=\"$!\"; wait \"$child\" 2>/dev/null || true; kill \"$watchdog\" 2>/dev/null || true; rm -f \"$payload\"' cmux-codex-hook \"$payload\" \"$cmux_cli\" \(routedArguments) >/dev/null 2>&1 & fi; echo '{}'; else echo '{}'; fi"
+ return ": cmux-codex-hook-v1; cmux_cli=\"${CMUX_BUNDLED_CLI_PATH:-}\"; if [ -z \"$cmux_cli\" ] || [ ! -x \"$cmux_cli\" ]; then cmux_cli=\"$(command -v cmux 2>/dev/null || true)\"; fi; if [ -n \"$CMUX_SURFACE_ID\" ] && [ \"$\(def.disableEnvVar)\" != \"1\" ] && [ -n \"$cmux_cli\" ]; then payload=\"$(mktemp \"${TMPDIR:-/tmp}/cmux-codex-hook.XXXXXX\" 2>/dev/null || mktemp -t cmux-codex-hook 2>/dev/null)\" || { echo '{}'; exit 0; }; cat >\"$payload\" || true; if [ -n \"${CMUX_SOCKET_PATH:-}\" ]; then nohup sh -c 'payload=\"$1\"; shift; \"$@\" <\"$payload\" >/dev/null 2>&1 & child=\"$!\"; ( sleep 30; kill \"$child\" 2>/dev/null || true ) & watchdog=\"$!\"; wait \"$child\" 2>/dev/null || true; kill \"$watchdog\" 2>/dev/null || true; rm -f \"$payload\"' cmux-codex-hook \"$payload\" \"$cmux_cli\" --socket \"$CMUX_SOCKET_PATH\" \(routedArguments) >/dev/null 2>&1 & else nohup sh -c 'payload=\"$1\"; shift; \"$@\" <\"$payload\" >/dev/null 2>&1 & child=\"$!\"; ( sleep 30; kill \"$child\" 2>/dev/null || true ) & watchdog=\"$!\"; wait \"$child\" 2>/dev/null || true; kill \"$watchdog\" 2>/dev/null || true; rm -f \"$payload\"' cmux-codex-hook \"$payload\" \"$cmux_cli\" \(routedArguments) >/dev/null 2>&1 & fi; echo '{}'; else echo '{}'; fi"
}
} if def.name == "codex" {
markers.append("cmux codex-hook")
+ markers.append("cmux-codex-hook-v1")
}As per coding guidelines, stale-hook replacement here needs to stay correctness-driven against the authoritative hooks.json contents instead of depending on a byte-for-byte cached shell literal.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static func codexFireAndForgetAgentHookShellCommand(_ command: String, for def: AgentHookDef) -> String { | |
| let routedArguments = command.hasPrefix("cmux ") ? String(command.dropFirst("cmux ".count)) : command | |
| return "cmux_cli=\"${CMUX_BUNDLED_CLI_PATH:-}\"; if [ -z \"$cmux_cli\" ] || [ ! -x \"$cmux_cli\" ]; then cmux_cli=\"$(command -v cmux 2>/dev/null || true)\"; fi; if [ -n \"$CMUX_SURFACE_ID\" ] && [ \"$\(def.disableEnvVar)\" != \"1\" ] && [ -n \"$cmux_cli\" ]; then payload=\"$(mktemp \"${TMPDIR:-/tmp}/cmux-codex-hook.XXXXXX\" 2>/dev/null || mktemp -t cmux-codex-hook 2>/dev/null)\" || { echo '{}'; exit 0; }; cat >\"$payload\" || true; if [ -n \"${CMUX_SOCKET_PATH:-}\" ]; then nohup sh -c 'payload=\"$1\"; shift; \"$@\" <\"$payload\" >/dev/null 2>&1 & child=\"$!\"; ( sleep 30; kill \"$child\" 2>/dev/null || true ) & watchdog=\"$!\"; wait \"$child\" 2>/dev/null || true; kill \"$watchdog\" 2>/dev/null || true; rm -f \"$payload\"' cmux-codex-hook \"$payload\" \"$cmux_cli\" --socket \"$CMUX_SOCKET_PATH\" \(routedArguments) >/dev/null 2>&1 & else nohup sh -c 'payload=\"$1\"; shift; \"$@\" <\"$payload\" >/dev/null 2>&1 & child=\"$!\"; ( sleep 30; kill \"$child\" 2>/dev/null || true ) & watchdog=\"$!\"; wait \"$child\" 2>/dev/null || true; kill \"$watchdog\" 2>/dev/null || true; rm -f \"$payload\"' cmux-codex-hook \"$payload\" \"$cmux_cli\" \(routedArguments) >/dev/null 2>&1 & fi; echo '{}'; else echo '{}'; fi" | |
| static func codexFireAndForgetAgentHookShellCommand(_ command: String, for def: AgentHookDef) -> String { | |
| let routedArguments = command.hasPrefix("cmux ") ? String(command.dropFirst("cmux ".count)) : command | |
| return ": cmux-codex-hook-v1; cmux_cli=\"${CMUX_BUNDLED_CLI_PATH:-}\"; if [ -z \"$cmux_cli\" ] || [ ! -x \"$cmux_cli\" ]; then cmux_cli=\"$(command -v cmux 2>/dev/null || true)\"; fi; if [ -n \"$CMUX_SURFACE_ID\" ] && [ \"$\(def.disableEnvVar)\" != \"1\" ] && [ -n \"$cmux_cli\" ]; then payload=\"$(mktemp \"${TMPDIR:-/tmp}/cmux-codex-hook.XXXXXX\" 2>/dev/null || mktemp -t cmux-codex-hook 2>/dev/null)\" || { echo '{}'; exit 0; }; cat >\"$payload\" || true; if [ -n \"${CMUX_SOCKET_PATH:-}\" ]; then nohup sh -c 'payload=\"$1\"; shift; \"$@\" <\"$payload\" >/dev/null 2>&1 & child=\"$!\"; ( sleep 30; kill \"$child\" 2>/dev/null || true ) & watchdog=\"$!\"; wait \"$child\" 2>/dev/null || true; kill \"$watchdog\" 2>/dev/null || true; rm -f \"$payload\"' cmux-codex-hook \"$payload\" \"$cmux_cli\" --socket \"$CMUX_SOCKET_PATH\" \(routedArguments) >/dev/null 2>&1 & else nohup sh -c 'payload=\"$1\"; shift; \"$@\" <\"$payload\" >/dev/null 2>&1 & child=\"$!\"; ( sleep 30; kill \"$child\" 2>/dev/null || true ) & watchdog=\"$!\"; wait \"$child\" 2>/dev/null || true; kill \"$watchdog\" 2>/dev/null || true; rm -f \"$payload\"' cmux-codex-hook \"$payload\" \"$cmux_cli\" \(routedArguments) >/dev/null 2>&1 & fi; echo '{}'; else echo '{}'; fi" | |
| } |
🤖 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 `@CLI/CMUXCLI`+CodexFireAndForgetHooks.swift around lines 2 - 4, The
codexFireAndForgetAgentHookShellCommand method generates a shell command that
lacks a stable Codex-owned marker, preventing proper identification and
replacement of stale hooks in hooks.json. The shell-rich wrapper with semicolons
and ampersands is rejected by isLegacyCmuxOwnedHookCommand, so the system can
only match by exact string equality, allowing older variants to coexist and
cause duplicate Codex executions. Add a distinctive Codex marker comment or
identifier string (similar to the existing cmux hooks codex or cmux codex-hook
patterns) to the generated shell command that makes it reliably identifiable for
cleanup purposes, independent of minor shell syntax variations.
Source: Coding guidelines
b936804 to
771fe80
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
771fe80 to
f8a5be5
Compare
f8a5be5 to
eba7f03
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 402-412: The timeout handling for the process termination has a
race condition and can hang indefinitely on readDataToEndOfFile() if the process
ignores signals. Replace the asynchronous DispatchQueue.asyncAfter approach with
synchronous sequential waits: after process.terminate() is called, immediately
wait for exitSignal with a timeout; if still running after that timeout, call
process.interrupt() synchronously and wait again; if the process is still
running after that second wait, use SIGKILL as a last resort (via
process.terminate() with force flag or equivalent) to guarantee the process is
dead before attempting to read from the pipes with readDataToEndOfFile(). This
ensures the pipes will actually close and EOF will occur, preventing indefinite
blocking.
- Around line 329-332: The single Darwin.write call in the mock socket response
section does not handle partial writes, which can occur with stream sockets and
cause truncated JSON responses and flaky tests. Replace the single write
operation with a loop that checks the return value from Darwin.write, advances
the buffer pointer/offset by the actual number of bytes written on each
iteration, handles EINTR errors appropriately, and continues looping until all
bytes are successfully sent to the client file descriptor. Follow the
established pattern used elsewhere in the test codebase like
CLINotifyProcessTestSupport.swift for the correct implementation.
🪄 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: 63aa4392-5ecc-4c6b-8619-4f258db8dff6
📒 Files selected for processing (5)
CLI/CMUXCLI+AgentHookDefinitions.swiftCLI/CMUXCLI+CodexFireAndForgetHooks.swiftCLI/cmux.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CLICodexHookTimeoutRegressionTests.swift
| let response = mockSocketResponse(for: line, surfaceId: surfaceId) + "\n" | ||
| _ = response.withCString { ptr in | ||
| Darwin.write(clientFD, ptr, strlen(ptr)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "CLICodexHookTimeoutRegressionTests.swift" -type fRepository: manaflow-ai/cmux
Length of output: 113
🏁 Script executed:
if [ -f "cmuxTests/CLICodexHookTimeoutRegressionTests.swift" ]; then
wc -l cmuxTests/CLICodexHookTimeoutRegressionTests.swift
fiRepository: manaflow-ai/cmux
Length of output: 115
🏁 Script executed:
if [ -f "cmuxTests/CLICodexHookTimeoutRegressionTests.swift" ]; then
sed -n '320,350p' cmuxTests/CLICodexHookTimeoutRegressionTests.swift
fiRepository: manaflow-ai/cmux
Length of output: 1327
🏁 Script executed:
rg -n "Darwin\.write" --type swiftRepository: manaflow-ai/cmux
Length of output: 4100
🏁 Script executed:
sed -n '335,370p' cmuxTests/CLICodexHookTimeoutRegressionTests.swiftRepository: manaflow-ai/cmux
Length of output: 1334
🏁 Script executed:
sed -n '1,100p' cmuxTests/CLICodexHookTimeoutRegressionTests.swiftRepository: manaflow-ai/cmux
Length of output: 5219
🏁 Script executed:
if [ -f "cmux.xcodeproj/project.pbxproj" ]; then
rg -c "CLICodexHookTimeoutRegressionTests" cmux.xcodeproj/project.pbxproj
fiRepository: manaflow-ai/cmux
Length of output: 62
🏁 Script executed:
sed -n '165,185p' cmuxTests/CLINotifyProcessTestSupport.swiftRepository: manaflow-ai/cmux
Length of output: 1008
🏁 Script executed:
sed -n '215,235p' cmuxTests/CLINotifyProcessTestSupport.swiftRepository: manaflow-ai/cmux
Length of output: 1024
Handle partial socket writes in mock responses.
Darwin.write on stream sockets can write fewer bytes than requested. Line 331 currently does a single write without checking the return value, which can truncate JSON responses and cause flaky tests. The established pattern throughout the test codebase (e.g., CLINotifyProcessTestSupport.swift) shows the correct approach: loop until all bytes are sent, advance the cursor by the bytes actually written, and handle EINTR.
Suggested fix
- let response = mockSocketResponse(for: line, surfaceId: surfaceId) + "\n"
- _ = response.withCString { ptr in
- Darwin.write(clientFD, ptr, strlen(ptr))
- }
+ let responseData = Data((mockSocketResponse(for: line, surfaceId: surfaceId) + "\n").utf8)
+ responseData.withUnsafeBytes { rawBuffer in
+ guard let base = rawBuffer.bindMemory(to: UInt8.self).baseAddress else { return }
+ var remaining = rawBuffer.count
+ var cursor = base
+ while remaining > 0 {
+ let written = Darwin.write(clientFD, cursor, remaining)
+ if written > 0 {
+ remaining -= written
+ cursor = cursor.advanced(by: written)
+ } else if written < 0 && errno == EINTR {
+ continue
+ } else {
+ return
+ }
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let response = mockSocketResponse(for: line, surfaceId: surfaceId) + "\n" | |
| _ = response.withCString { ptr in | |
| Darwin.write(clientFD, ptr, strlen(ptr)) | |
| } | |
| let responseData = Data((mockSocketResponse(for: line, surfaceId: surfaceId) + "\n").utf8) | |
| responseData.withUnsafeBytes { rawBuffer in | |
| guard let base = rawBuffer.bindMemory(to: UInt8.self).baseAddress else { return } | |
| var remaining = rawBuffer.count | |
| var cursor = base | |
| while remaining > 0 { | |
| let written = Darwin.write(clientFD, cursor, remaining) | |
| if written > 0 { | |
| remaining -= written | |
| cursor = cursor.advanced(by: written) | |
| } else if written < 0 && errno == EINTR { | |
| continue | |
| } else { | |
| return | |
| } | |
| } | |
| } |
🤖 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 329 - 332,
The single Darwin.write call in the mock socket response section does not handle
partial writes, which can occur with stream sockets and cause truncated JSON
responses and flaky tests. Replace the single write operation with a loop that
checks the return value from Darwin.write, advances the buffer pointer/offset by
the actual number of bytes written on each iteration, handles EINTR errors
appropriately, and continues looping until all bytes are successfully sent to
the client file descriptor. Follow the established pattern used elsewhere in the
test codebase like CLINotifyProcessTestSupport.swift for the correct
implementation.
eba7f03 to
c57aeb2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c57aeb20c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| extension CMUXCLI { | ||
| static func codexFireAndForgetAgentHookShellCommand(_ command: String, for def: AgentHookDef) -> String { | ||
| let routedArguments = command.hasPrefix("cmux ") ? String(command.dropFirst("cmux ".count)) : command | ||
| return "cmux_cli=\"${CMUX_BUNDLED_CLI_PATH:-}\"; if [ -z \"$cmux_cli\" ] || [ ! -x \"$cmux_cli\" ]; then cmux_cli=\"$(command -v cmux 2>/dev/null || true)\"; fi; if [ -n \"$CMUX_SURFACE_ID\" ] && [ \"$\(def.disableEnvVar)\" != \"1\" ] && [ -n \"$cmux_cli\" ]; then payload=\"$(mktemp \"${TMPDIR:-/tmp}/cmux-codex-hook.XXXXXX\" 2>/dev/null || mktemp -t cmux-codex-hook 2>/dev/null)\" || { echo '{}'; exit 0; }; cat >\"$payload\" || true; if [ -n \"${CMUX_SOCKET_PATH:-}\" ]; then nohup sh -c 'payload=\"$1\"; shift; \"$@\" <\"$payload\" >/dev/null 2>&1 & child=\"$!\"; ( sleep 30; kill \"$child\" 2>/dev/null || true ) & watchdog=\"$!\"; wait \"$child\" 2>/dev/null || true; kill \"$watchdog\" 2>/dev/null || true; rm -f \"$payload\"' cmux-codex-hook \"$payload\" \"$cmux_cli\" --socket \"$CMUX_SOCKET_PATH\" \(routedArguments) >/dev/null 2>&1 & else nohup sh -c 'payload=\"$1\"; shift; \"$@\" <\"$payload\" >/dev/null 2>&1 & child=\"$!\"; ( sleep 30; kill \"$child\" 2>/dev/null || true ) & watchdog=\"$!\"; wait \"$child\" 2>/dev/null || true; kill \"$watchdog\" 2>/dev/null || true; rm -f \"$payload\"' cmux-codex-hook \"$payload\" \"$cmux_cli\" \(routedArguments) >/dev/null 2>&1 & fi; echo '{}'; else echo '{}'; fi" |
There was a problem hiding this comment.
When Codex runs in a cmux terminal where routing must be resolved from the caller process/TTY (for example a stale leaked CMUX_SURFACE_ID or an environment that lacks CMUX_WORKSPACE_ID), this detached nohup sh -c ... & invocation makes the real cmux hooks codex ... process run with stdin from the temp file and stdout/stderr on /dev/null, and it can be reparented before inferredAgentPID()/processBinding() runs. That bypasses the existing correctedDirectSurfaceId defense in runGenericAgentHook, so session-start/prompt-submit can fall back to the stale ambient surface or fail to resolve the workspace instead of targeting the Codex pane; capture/pass the caller tty or agent PID before detaching.
Useful? React with 👍 / 👎.
c57aeb2 to
d72946b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d72946b542
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| turnId: input.turnId, | ||
| pid: pid, | ||
| launchCommand: launchCommand | ||
| )) ?? false |
There was a problem hiding this comment.
When upsertCodexPromptRunningIfFresh throws for reasons unrelated to staleness (for example the hook-state directory is missing/unwritable, so withLockedState cannot open the lock or save), try? converts that into false and the next branch returns as if this were a stale terminal turn. That skips the live socket updates that used to proceed even when store.upsert failed, so a valid Codex prompt-submit can fail to clear notifications or set the pane to Running solely because persistence is unavailable; distinguish store errors from the explicit stale-turn result.
Useful? React with 👍 / 👎.
d72946b to
718f7eb
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 718f7eb6b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !sessionId.isEmpty { | ||
| let acceptedSessionStart: Bool | ||
| if def.name == "codex" { | ||
| acceptedSessionStart = (try? store.upsertCodexSessionStartIfFresh( |
There was a problem hiding this comment.
When the Codex hook state directory is missing or unwritable, upsertCodexSessionStartIfFresh throws while opening/saving the store, but try? turns that into false and this branch treats the valid SessionStart as stale. That returns before feed telemetry, resume binding, PID, lifecycle, and status updates; before this change the store write failure was ignored and the visible session still came up, so users with a bad ~/.cmuxterm state path lose Codex UI updates rather than only persistence.
Useful? React with 👍 / 👎.
718f7eb to
4cf36e8
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4cf36e8652
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let timedOut: Bool | ||
| } | ||
|
|
||
| @Test func codexHookInstallReplacesSynchronousBundledHook() throws { |
There was a problem hiding this comment.
AGENTS.md in /workspace/cmux requires regression tests for a bug fix to use a two-commit structure: first add the failing test only, then add the fix. This commit adds CLICodexHookTimeoutRegressionTests together with the hook implementation changes, so CI can never demonstrate that these new regression tests fail without the fix. Please split the test-only change from the implementation change so the PR preserves the required red/green evidence.
Useful? React with 👍 / 👎.
4cf36e8 to
7f774b4
Compare
# Conflicts: # .github/swift-file-length-budget.tsv
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da98f613cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if max(record.activePromptDepth ?? 0, record.activePromptTurnIds?.count ?? 0) > 0 { | ||
| return true |
There was a problem hiding this comment.
When a Codex process is killed or crashes while a turn is active, the store can retain activePromptDepth/activePromptTurnIds. If the user resumes the same Codex session in a new process, the SessionStart hook arrives with a different PID, but this guard rejects it before comparing PIDs, so the hook returns without clearing the stale turn state or recording the new PID/status. This leaves the pane stuck on the old running session; only a same-process late SessionStart should be treated as stale here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 68a6d3e. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c8aef3bc8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Format: max_lines<TAB>relative path | ||
| # Reduce counts as files shrink. CI fails if tracked files exceed this budget. | ||
| 33454 CLI/cmux.swift | ||
| 33857 CLI/cmux.swift |
There was a problem hiding this comment.
The repo's .github/review-bot-rules/swift-file-package-boundaries.md says to report an existing production Swift file over 800 lines gaining more than 250 lines unless the change extracts responsibility or shrinks the oversized file. This commit adds hundreds of lines of Codex hook dispatch, persistence freshness, and socket/UI restore logic to the already 33k-line CLI/cmux.swift and accepts that growth by raising the budget here, so the smallest fix is to move the Codex hook freshness/fire-and-forget support behind a focused file or package boundary instead of increasing the monolith's allowance.
Useful? React with 👍 / 👎.
* Make Codex hooks fire-and-forget * Preserve Codex prompt baseline refreshes * Preserve Codex turn tombstones * Harden Codex stale hook ordering * Drop stale Codex prompt baseline writes * Simplify Codex prompt submit result state
This branch was successfully deployed
1 participant
Summary
Root cause
~/.codex/hooks.jsonstill had an older synchronous cmux hook installed after the newer fire-and-forget wrapper. Codex ran both, and the synchronous command could block on the cmux socket long enough to exceed Codex's 5s hook timeout.Verification
xcodebuild -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS,arch=arm64' -derivedDataPath /tmp/cmux-codex-hooks-test -only-testing:cmuxTests/CLINotifyProcessIntegrationRegressionTests/testCodexHookInstallPrefersLaunchingAppBundledCLI -only-testing:cmuxTests/CLINotifyProcessIntegrationRegressionTests/testCodexInstalledHookReturnsBeforeSlowCmuxCommandFinishes testNeed help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes Codex hook install scripts, async subprocess behavior, and session/UI mutation paths; regression tests are broad but race timing in production hooks remains possible.
Overview
Codex
session-startandprompt-submithooks installed viahooks.jsonnow use a fire-and-forget shell wrapper: hook stdin is spooled to a temp file,cmuxruns in the background (nohup+ 30s watchdog), and the hook immediately prints{}so Codex does not hit its ~5s synchronous timeout.stopand feed hooks stay blocking.The CLI hook handlers add Codex-specific freshness rules: ignore stale
session-startwhen active turn state exists or a completed turn would be revived by the same PID; ignoreprompt-submitfor terminal turns and skip visible UI / diff-baseline updates when late async work races ahead. Session store APIs (upsertCodexSessionStartIfFresh,upsertCodexPromptRunningIfFresh,recordPromptSubmitwithrejectTerminalTurn) back those checks. Agent PID resolution prefersCMUX_CODEX_PID(and analogous env vars) over PPID inference.CLICodexHookTimeoutRegressionTestscovers install dedupe/replace of legacy sync commands, fast hook return vs slowcmux, and stale session/prompt behavior; an existing integration test now expects terminal turns not to refresh last-turn diff baselines.Reviewed by Cursor Bugbot for commit 6c8aef3. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Make Codex
session-startandprompt-submitfire-and-forget. Hooks print{}immediately whilecmuxruns in the background with a 30s watchdog, and stricter staleness checks ensure late Codex events don’t change state or UI; terminal turn tombstones are preserved and stale/terminal prompt baseline writes are skipped.session-start/prompt-submit;stopand feed hooks stay synchronous; installer replaces legacy sync commands and dedupes~/.codex/hooks.json.nohup, passes--socketwhenCMUX_SOCKET_PATHis set, exportsCMUX_CODEX_PID, echoes{}, and enforces a 30s watchdog.CMUX_*_PIDover PPID for all agents (Codex included).upsertCodexSessionStartIfFreshandupsertCodexPromptRunningIfFreshguard late updates;session-startignored if any active turn exists, or if only completed-turn state exists and the incoming PID matches; fresh starts clear completed-turn state without removing terminal turn tombstones.recordPromptSubmitnow returns(staleTerminalTurn, nested)to avoid accidental UI changes.CLICodexHookTimeoutRegressionTestscover install replace/dedupe, fast return vs slowcmux, staleprompt-submit/session-starthandling, active-turn protection, fresh-start behavior, and same-PID completed-turn starts; updated prompt baseline test ensures terminal turns don’t refresh the last-turn baseline.Written for commit 6c8aef3. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests