Run agent lifecycle hooks in background - #5774
lawrencecchen wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR converts lifecycle hooks to background fire-and-forget execution while preserving legacy synchronous behavior. A new ChangesAgent Hook Background Execution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 errors, 1 warning)
✅ Passed checks (16 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 252da29. Configure here.
| ) -> String { | ||
| let foregroundCommand = agentHookShellCommand(command, for: def) | ||
| let workerScript = """ | ||
| payload="${CMUX_HOOK_PAYLOAD:-}"; command="${CMUX_HOOK_COMMAND:-}"; cleanup() { [ -n "$payload" ] && rm -f "$payload"; }; trap cleanup EXIT INT TERM; [ -n "$payload" ] && [ -n "$command" ] || exit 0; ( eval "$command" ) < "$payload" & child=$!; remaining=30; while [ "$remaining" -gt 0 ]; do kill -0 "$child" 2>/dev/null || break; sleep 1; remaining=$((remaining - 1)); done; if kill -0 "$child" 2>/dev/null; then kill "$child" 2>/dev/null || true; fi; wait "$child" 2>/dev/null || true |
There was a problem hiding this comment.
Background worker kills slow hooks
Medium Severity
The detached nohup worker waits at most 30 seconds, then sends kill to the child running the real cmux hooks handler. Lifecycle work that legitimately exceeds 30 seconds (slow app IPC, restore, or naming) can be terminated mid-flight while the agent already received {}, so session state, Feed telemetry, and restore records may never update.
Reviewed by Cursor Bugbot for commit 252da29. Configure here.
![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: 252da29162
ℹ️ 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".
| } else { | ||
| prerequisite = "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" | ||
| } | ||
| return ": \(backgroundHookMarker); \(prerequisite) cmux_hook_payload=\"$(mktemp \"${TMPDIR:-/tmp}/cmux-\(logSlug).json.XXXXXX\")\" || { echo '{}'; exit 0; }; cat > \"$cmux_hook_payload\" || true; cmux_hook_log_dir=\"${TMPDIR:-/tmp}/cmux-agent-hooks\"; mkdir -p \"$cmux_hook_log_dir\" 2>/dev/null || true; CMUX_HOOK_PAYLOAD=\"$cmux_hook_payload\" CMUX_HOOK_COMMAND=\(shellSingleQuote(foregroundCommand)) nohup sh -c \(shellSingleQuote(workerScript)) >> \"$cmux_hook_log_dir/\(logSlug).log\" 2>&1 & echo '{}'; else echo '{}'; fi" |
There was a problem hiding this comment.
When this wrapper is used for Grok or Antigravity, the foreground handler no longer runs as a child of the agent hook process: the original hook shell exits after nohup ... &, so the worker is reparented before it invokes cmux hooks .... Those pinned integrations are explicitly handled in runGenericAgentHook as agents that may strip CMUX_* and therefore rely on inferredAgentPID()/process binding to find the target surface; after detaching, that inference walks through the worker shell to launchd instead of the agent, so new session-start/prompt/stop hooks can resolve no workspace and silently no-op. Keep pinned/PID-routed hooks foreground or pass an explicit workspace/surface/PID into the background worker.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR converts generic agent lifecycle hooks (session-start, prompt-submit, stop, etc.) from synchronous foreground calls to fire-and-forget background dispatchers, while keeping Feed bridge hooks foreground so approval decisions and exit-code propagation still work. It also retains the old synchronous command string as a trust-hash entry so Codex installations upgraded with the new CLI continue to recognize previously approved hook commands.
Confidence Score: 3/5Safe to merge after addressing the sleep-polling loop in the worker script; the backward-compat hash strategy and feed-foreground split are sound. The background dispatch logic is correct and well-tested, but the worker script implements its 30-second child timeout via a while/sleep 1 counter loop — a wall-clock polling primitive the repo's no-hacky-sleeps rule explicitly prohibits in production shell code. On a machine where hooks fire frequently the polling overhead is real, and if timeout(1) is available the loop is unnecessary. CLI/CMUXCLI+AgentHookDefinitions.swift — the worker script embedded in backgroundAgentHookShellCommand needs attention for the polling loop and the unbounded log file. Important Files Changed
Sequence DiagramsequenceDiagram
participant Agent
participant HookShell as "Hook shell script"
participant Worker as "nohup sh -c (worker)"
participant cmux as "cmux hooks agent event"
Agent->>HookShell: "invoke hook (stdin = JSON payload)"
HookShell->>HookShell: "mktemp -> write stdin to temp file"
HookShell->>Worker: "CMUX_HOOK_PAYLOAD=... nohup sh -c '...' &"
HookShell-->>Agent: "echo '{}' (immediate return)"
Worker->>Worker: "trap cleanup EXIT INT TERM"
Worker->>cmux: "eval CMUX_HOOK_COMMAND < payload_file (background)"
Worker->>Worker: "poll kill -0 child + sleep 1 (up to 30 s)"
cmux-->>Worker: "exit (workspace state updated)"
Worker->>Worker: "cleanup: rm -f payload_file"
Reviews (1): Last reviewed commit: "Run agent lifecycle hooks in background" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ) -> String { | ||
| let foregroundCommand = agentHookShellCommand(command, for: def) | ||
| let workerScript = """ | ||
| payload="${CMUX_HOOK_PAYLOAD:-}"; command="${CMUX_HOOK_COMMAND:-}"; cleanup() { [ -n "$payload" ] && rm -f "$payload"; }; trap cleanup EXIT INT TERM; [ -n "$payload" ] && [ -n "$command" ] || exit 0; ( eval "$command" ) < "$payload" & child=$!; remaining=30; while [ "$remaining" -gt 0 ]; do kill -0 "$child" 2>/dev/null || break; sleep 1; remaining=$((remaining - 1)); done; if kill -0 "$child" 2>/dev/null; then kill "$child" 2>/dev/null || true; fi; wait "$child" 2>/dev/null || true |
There was a problem hiding this comment.
The worker script implements its 30-second timeout by decrementing a counter inside a while/sleep 1 loop. This is a wall-clock poll on process lifecycle — the exact pattern the runtime-no-hacky-sleeps rule flags for production shell code. If timeout(1) is available in the target environment (macOS 12+ ships /usr/bin/timeout; Linux always has it), the worker body can be simplified to a direct timeout 30 sh -c '...' invocation with no polling loop needed. If portability across macOS versions is the concern, the PR description or a comment should document that constraint so future maintainers don't reach for the same pattern elsewhere.
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!
| } else { | ||
| prerequisite = "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" | ||
| } | ||
| return ": \(backgroundHookMarker); \(prerequisite) cmux_hook_payload=\"$(mktemp \"${TMPDIR:-/tmp}/cmux-\(logSlug).json.XXXXXX\")\" || { echo '{}'; exit 0; }; cat > \"$cmux_hook_payload\" || true; cmux_hook_log_dir=\"${TMPDIR:-/tmp}/cmux-agent-hooks\"; mkdir -p \"$cmux_hook_log_dir\" 2>/dev/null || true; CMUX_HOOK_PAYLOAD=\"$cmux_hook_payload\" CMUX_HOOK_COMMAND=\(shellSingleQuote(foregroundCommand)) nohup sh -c \(shellSingleQuote(workerScript)) >> \"$cmux_hook_log_dir/\(logSlug).log\" 2>&1 & echo '{}'; else echo '{}'; fi" |
There was a problem hiding this comment.
Every background hook invocation appends to ${TMPDIR:-/tmp}/cmux-agent-hooks/<logSlug>.log with no rotation or size cap. On a busy machine with frequent session-start/stop or prompt-submit events, these per-slug log files can grow without bound. Even on macOS where /tmp is periodically purged by the OS, the directory under a custom TMPDIR is not. Adding a tail -c N trim or a size check before the append would bound the on-disk footprint.
![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/CLIGenericHookPersistenceTests.swift`:
- Around line 3790-3799: waitForFile currently returns as soon as the path
exists, which can race with the writer creating an empty file; update
waitForFile to also verify the marker content is fully written before returning
by reading the file and ensuring either (a) the file contains the expected
marker string or (b) the file size is > 0 and stable across two successive polls
(e.g., read size, sleep, reread size) to confirm no in-progress writes;
reference the waitForFile function and the code that reads the marker so the
check matches the expected marker content or stability criterion.
🪄 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: 5fec55ab-69f4-433a-9d1e-5bd9ec6baee5
📒 Files selected for processing (4)
CLI/CMUXCLI+AgentHookDefinitions.swiftCLI/cmux.swiftcmuxTests/CLIGenericHookPersistenceTests.swiftdocs/agent-hooks.md
| private func waitForFile(at url: URL, timeout: TimeInterval) -> Bool { | ||
| let deadline = Date().addingTimeInterval(timeout) | ||
| while Date() < deadline { | ||
| if FileManager.default.fileExists(atPath: url.path) { | ||
| return true | ||
| } | ||
| usleep(50_000) | ||
| } | ||
| return FileManager.default.fileExists(atPath: url.path) | ||
| } |
There was a problem hiding this comment.
waitForFile can return before marker content is fully written.
Line 3793 checks only path existence, but the writer creates the file before finishing printf, so Line 1091 can read partial content and flake.
Suggested fix
-private func waitForFile(at url: URL, timeout: TimeInterval) -> Bool {
+private func waitForFile(at url: URL, timeout: TimeInterval, minSize: UInt64 = 1) -> Bool {
let deadline = Date().addingTimeInterval(timeout)
while Date() < deadline {
- if FileManager.default.fileExists(atPath: url.path) {
+ if let attrs = try? FileManager.default.attributesOfItem(atPath: url.path),
+ let size = attrs[.size] as? NSNumber,
+ size.uint64Value >= minSize {
return true
}
usleep(50_000)
}
- return FileManager.default.fileExists(atPath: url.path)
+ if let attrs = try? FileManager.default.attributesOfItem(atPath: url.path),
+ let size = attrs[.size] as? NSNumber {
+ return size.uint64Value >= minSize
+ }
+ return false
}🤖 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/CLIGenericHookPersistenceTests.swift` around lines 3790 - 3799,
waitForFile currently returns as soon as the path exists, which can race with
the writer creating an empty file; update waitForFile to also verify the marker
content is fully written before returning by reading the file and ensuring
either (a) the file contains the expected marker string or (b) the file size is
> 0 and stable across two successive polls (e.g., read size, sleep, reread size)
to confirm no in-progress writes; reference the waitForFile function and the
code that reads the marker so the check matches the expected marker content or
stability criterion.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
Verification
xcodebuild -project cmux.xcodeproj -scheme cmux-cli -derivedDataPath /tmp/cmux-bghook-cli buildcmux-agent-hook-background-v1andnohup sh -ccmuxhandler: hook stdout{}, returned in 32ms, handler completed later with the payload./scripts/reload-cloud.sh --tag bghookbghookapp, created a Codex workspace throughscripts/cmux-debug-cli.sh, and confirmedBGHOOK_DONE_AUTHwith no hook timeout/exited bannersNotes
cmux-unitbuild is blocked in this worktree by missingGhosttyKit.xcframework; CI should compile and run the full test target.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes how every integrated agent invokes cmux on lifecycle events (session, prompts, stop); wrong background dispatch could drop telemetry or restore updates, while feed paths remain blocking by design.
Overview
Lifecycle agent hooks now install as fire-and-forget shell wrappers (
cmux-agent-hook-background-v1): they write the hook JSON to a temp file, run the realcmux hooks <agent> <event>vianohup, immediately print{}, and log tocmux-agent-hooks. A worker caps detached runs at ~30s.Feed bridge hooks are unchanged—still synchronous so approvals and deny exit codes (e.g. Kiro
exit 2) reach the agent.For Codex trust hashes, setup also registers the previous synchronous lifecycle command strings so reinstall can replace already-trusted hook commands.
Tests cover background vs foreground split and that lifecycle hooks return before a slow handler finishes; docs describe the behavior.
Reviewed by Cursor Bugbot for commit 252da29. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Run agent lifecycle hooks in the background so agents don’t block, while keeping Feed bridge hooks in the foreground so approval/deny behavior still works.
cmux hooks <agent> <event>withnohup, return{}immediately.Written for commit 252da29. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
Tests
Documentation