Repository navigation
Scope Codex hooks per launch and serialize delivery - #8243
lawrencecchen wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughCodex hook injection now tracks persistent-hook opt-in state, removes legacy cmux hooks when appropriate, and defers to explicit persistent hooks. Fire-and-forget delivery is serialized with per-agent locks, with regression tests covering migration, setup, and event ordering. ChangesCodex hook lifecycle and delivery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CMUXCLI
participant hooks.json
participant Codex
participant HookRunner
participant cmux
CMUXCLI->>hooks.json: inspect persistent hook ownership
CMUXCLI->>Codex: emit enable-only or injected hook arguments
Codex->>HookRunner: execute hook command
HookRunner->>HookRunner: acquire per-agent queue lock
HookRunner->>cmux: deliver captured event payload
HookRunner-->>Codex: return {}
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 errors)
✅ Passed checks (21 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 |
Greptile SummaryThis PR fixes a process-ownership conflict where both the global persistent Codex hooks and the per-launch wrapper injection could simultaneously own the same Codex hook events. It introduces a per-session
Confidence Score: 4/5Safe to merge; the migration logic is self-limiting and atomic, the serialization mechanism is correct for the benchmarked workload, and tests cover the four stated behavioral regressions. Two non-blocking concerns exist in the new runner script. First, the 30-second watchdog for each background delivery starts counting from when the nohup shell spawns, not from when the lock is actually acquired — so a command that waits near 30s for a contended lock gets almost no execution time before being killed and having its payload silently deleted. Second, the non-numeric PID sanitization does not match an empty string, meaning a pathologically empty agent_pid falls through to a shared lock file. Neither issue surfaces under normal conditions given the 25ms benchmarks. CLI/CMUXCLI+CodexFireAndForgetHooks.swift — specifically the runner shell string on line 203 (watchdog timing) and the PID sanitization case on line 208. Important Files Changed
Reviews (1): Last reviewed commit: "fix: isolate Codex hook ownership" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| static func codexFireAndForgetAgentHookShellCommand(_ command: String, for def: AgentHookDef) -> String { | ||
| let routedArguments = command.hasPrefix("cmux ") ? String(command.dropFirst("cmux ".count)) : command | ||
| let runner = "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\"" | ||
| let runner = "payload=\"$1\"; queue_lock=\"$2\"; shift 2; /usr/bin/lockf -k -t 30 \"$queue_lock\" \"$@\" <\"$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\"" |
There was a problem hiding this comment.
The 30-second sleep 30; kill $child watchdog starts counting from when the nohup shell spawns, not from when lockf actually acquires the lock and execs the command. Because lockf -k -t 30 may consume up to 30s waiting for the lock, a delivery that arrives late in a contention window can get nearly zero execution time before the watchdog fires — the payload is then silently deleted via rm -f "$payload". The old code gave each command the full 30s budget. Under the benchmarked 25ms mean this won't bite, but a slow cmux call holding the lock for several seconds causes the next delivery's watchdog to fire almost immediately after it acquires the lock.
| "if [ -z \"$cmux_cli\" ] || [ ! -x \"$cmux_cli\" ]; then cmux_cli=\"$(command -v cmux 2>/dev/null || true)\"; fi", | ||
| "agent_pid=\"${CMUX_CODEX_PID:-${PPID:-}}\"", | ||
| "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 CMUX_CODEX_PID=\"$agent_pid\" nohup sh -c '\(runner)' cmux-codex-hook \"$payload\" \"$cmux_cli\" --socket \"$CMUX_SOCKET_PATH\" \(routedArguments) >/dev/null 2>&1 & else CMUX_CODEX_PID=\"$agent_pid\" nohup sh -c '\(runner)' cmux-codex-hook \"$payload\" \"$cmux_cli\" \(routedArguments) >/dev/null 2>&1 & fi; echo '{}'; else echo '{}'; fi", | ||
| "case \"$agent_pid\" in *[!0-9]*) agent_pid=\"${PPID:-$$}\" ;; esac", |
There was a problem hiding this comment.
agent_pid does not match *[!0-9]* because the bracket expression requires at least one character. Adding the empty-string arm closes this gap and ensures all non-positive-integer PIDs fall through to the PPID/$$ fallback.
| "case \"$agent_pid\" in *[!0-9]*) agent_pid=\"${PPID:-$$}\" ;; esac", | |
| "case \"$agent_pid\" in \"\" | *[!0-9]*) agent_pid=\"${PPID:-$$}\" ;; esac", |
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!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 86-93: Make persistent ownership authoritative in the hooks
configuration managed by prepareCodexWrapperHookOwnership and the related
installation/removal flow, rather than using the separate opt-in marker file.
Update installation and cleanup so the owned hook entries and ownership state
are written or removed through one atomic config transformation, ensuring the
wrapper cannot classify newly installed persistent hooks as legacy between
separate writes. Remove the parallel marker-based authority while preserving
explicit persistent installation and uninstallation behavior.
- Around line 203-210: The fire-and-forget runner in the generated `runner`
shell command lets queue-lock contention consume the entire 30-second watchdog
budget and then deletes the payload without execution. Update the runner and its
invocation in the hook construction to separate lock acquisition from the
execution watchdog, or otherwise persist and retry payloads that cannot acquire
`queue_lock`; ensure queued deliveries are not killed or removed solely because
waiting exceeded 30 seconds.
🪄 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: b163bc92-963c-481d-a867-4e84f888cd42
📒 Files selected for processing (4)
CLI/CMUXCLI+CodexFireAndForgetHooks.swiftCLI/cmux.swiftResources/bin/cmux-codex-wrappercmuxTests/CLICodexHookTimeoutRegressionTests.swift
| private func prepareCodexWrapperHookOwnership(_ def: AgentHookDef) throws -> Bool { | ||
| if Self.codexPersistentHooksAreOptedIn(for: def) { | ||
| return true | ||
| } | ||
| Self.removeCodexPersistentHookOptIn(for: def) | ||
| if Self.codexPersistentHooksAreInstalled(for: def) { | ||
| try uninstallAgentHooks(def, quiet: true) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make persistent ownership one atomic source of truth.
The marker and hooks.json are updated separately. If the wrapper runs after installAgentHooks writes the hooks but before recordCodexPersistentHookOptIn, Lines 91-92 classify those newly installed hooks as legacy and uninstall them. Explicit persistent installation can therefore return successfully while leaving no persistent hooks.
Store the opt-in marker with the owned hook entries in the same atomic config transformation, rather than maintaining a second file-backed authority.
As per coding guidelines, correctness-critical lifecycle state must use one authoritative source, without mutable side channels representing the same state. <coding_guidelines> <path_instructions>
Also applies to: 106-136
🤖 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 86 - 93, Make
persistent ownership authoritative in the hooks configuration managed by
prepareCodexWrapperHookOwnership and the related installation/removal flow,
rather than using the separate opt-in marker file. Update installation and
cleanup so the owned hook entries and ownership state are written or removed
through one atomic config transformation, ensuring the wrapper cannot classify
newly installed persistent hooks as legacy between separate writes. Remove the
parallel marker-based authority while preserving explicit persistent
installation and uninstallation behavior.
Sources: Coding guidelines, Path instructions
| let runner = "payload=\"$1\"; queue_lock=\"$2\"; shift 2; /usr/bin/lockf -k -t 30 \"$queue_lock\" \"$@\" <\"$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\"" | ||
| 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", | ||
| "agent_pid=\"${CMUX_CODEX_PID:-${PPID:-}}\"", | ||
| "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 CMUX_CODEX_PID=\"$agent_pid\" nohup sh -c '\(runner)' cmux-codex-hook \"$payload\" \"$cmux_cli\" --socket \"$CMUX_SOCKET_PATH\" \(routedArguments) >/dev/null 2>&1 & else CMUX_CODEX_PID=\"$agent_pid\" nohup sh -c '\(runner)' cmux-codex-hook \"$payload\" \"$cmux_cli\" \(routedArguments) >/dev/null 2>&1 & fi; echo '{}'; else echo '{}'; fi", | ||
| "case \"$agent_pid\" in *[!0-9]*) agent_pid=\"${PPID:-$$}\" ;; esac", | ||
| "queue_lock=\"${TMPDIR:-/tmp}/cmux-codex-hook-${agent_pid}.lock\"", | ||
| "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 CMUX_CODEX_PID=\"$agent_pid\" nohup sh -c '\(runner)' cmux-codex-hook \"$payload\" \"$queue_lock\" \"$cmux_cli\" --socket \"$CMUX_SOCKET_PATH\" \(routedArguments) >/dev/null 2>&1 & else CMUX_CODEX_PID=\"$agent_pid\" nohup sh -c '\(runner)' cmux-codex-hook \"$payload\" \"$queue_lock\" \"$cmux_cli\" \(routedArguments) >/dev/null 2>&1 & fi; echo '{}'; else echo '{}'; fi", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
# Inspect the relevant file and nearby context
FILE='CLI/CMUXCLI+CodexFireAndForgetHooks.swift'
wc -l "$FILE"
sed -n '160,245p' "$FILE"
# Look for any other uses of lockf / watchdog / payload deletion in the repo
rg -n --hidden --no-ignore-vcs 'lockf -k|-t 30|watchdog|rm -f "\$payload"|cmux-codex-hook' .Repository: manaflow-ai/cmux
Length of output: 50374
🌐 Web query:
macOS lockf -t timeout semantics command skipped if lock not acquired man page
💡 Result:
On macOS, the lockf system call does not support timeout semantics [1]. The standard lockf function, as defined by POSIX and implemented on macOS, provides advisory record locking that either blocks indefinitely (F_LOCK) or returns immediately with an error (F_TLOCK) if the lock cannot be acquired [2][3]. It is important to distinguish between the lockf function (a library/system call) and the lockf utility (a command-line tool). The confusion regarding "timeout semantics" and "skipping command if lock not acquired" likely stems from the lockf utility available on other platforms (such as FreeBSD or certain Linux distributions), which often includes a -t or -w flag to handle timeouts and conditional command execution [4]. macOS does not provide a built-in lockf command-line utility with these features [1]. Developers requiring timeout functionality for file locking on macOS must typically implement it manually, for example, by using the alarm function in conjunction with standard blocking locks, or by using more modern file locking mechanisms like flock or fcntl with appropriate application-level logic [2][3][5].
Citations:
- 1: https://www.yellowduck.be/posts/using-the-lockf-command-on-linux-and-macos
- 2: https://pubs.opengroup.org/onlinepubs/9799919799/functions/lockf.html
- 3: https://pubs.opengroup.org/onlinepubs/9699919799.2016edition/functions/lockf.html
- 4: https://man.freebsd.org/cgi/man.cgi?lockf=
- 5: https://leopard-adc.pepas.com/technotes/tn/tn2037.html
🏁 Script executed:
set -euo pipefail
# Try to read the local man page if available; otherwise fall back to help text.
( man lockf 2>/dev/null | col -b | sed -n '1,220p' ) || ( /usr/bin/lockf -h 2>&1 || true )Repository: manaflow-ai/cmux
Length of output: 247
🏁 Script executed:
set -euo pipefail
( man lockf 2>/dev/null | col -b | sed -n '1,220p' ) || ( /usr/bin/lockf -h 2>&1 || true )Repository: manaflow-ai/cmux
Length of output: 247
🏁 Script executed:
set -euo pipefail
FILE='cmuxTests/CLICodexHookTimeoutRegressionTests.swift'
wc -l "$FILE"
sed -n '1,340p' "$FILE"Repository: manaflow-ai/cmux
Length of output: 18275
🏁 Script executed:
set -euo pipefail
rg -n 'lockf|queue_lock|timed out|timeout|watchdog' cmuxTests/CLICodexHookTimeoutRegressionTests.swift CLI/CMUXCLI+CodexFireAndForgetHooks.swift
sed -n '340,520p' cmuxTests/CLICodexHookTimeoutRegressionTests.swiftRepository: manaflow-ai/cmux
Length of output: 14147
🏁 Script executed:
set -euo pipefail
sed -n '1,260p' CLI/CMUXCLI+CodexFireAndForgetHooks.swiftRepository: manaflow-ai/cmux
Length of output: 11775
🏁 Script executed:
set -euo pipefail
rg -n 'cmux-codex-hook|queue_lock|lockf -k -t 30|hooks codex prompt-submit|hooks codex session-start|hooks codex stop' CLI Packages cmuxTestsRepository: manaflow-ai/cmux
Length of output: 13068
Do not let queue wait consume the 30-second delivery budget. CLI/CMUXCLI+CodexFireAndForgetHooks.swift:203-210
lockf starts before the payload command runs, and the watchdog starts at the same time. Under contention, a delivery can spend most of its budget waiting on queue_lock, get killed before execution, and still have "$payload" deleted afterward. Split queueing from execution or persist/retry the payload instead of dropping it.
🤖 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 203 - 210, The
fire-and-forget runner in the generated `runner` shell command lets queue-lock
contention consume the entire 30-second watchdog budget and then deletes the
payload without execution. Update the runner and its invocation in the hook
construction to separate lock acquisition from the execution watchdog, or
otherwise persist and retry payloads that cannot acquire `queue_lock`; ensure
queued deliveries are not killed or removed solely because waiting exceeded 30
seconds.
Source: Coding guidelines
lawrencecchen
added
the
stale-revisit
2 participants
Fixes #8230
What changed
cmux hooks setupleaves Codex to the per-launch wrapper by default.~/.codex/hooks.jsonbefore Codex reads it, while preserving user-owned hooks.cmux hooks codex installrecords persistent mode for custom launchers; the wrapper then enables hooks without injecting duplicates.lockfqueue, preserving event order while hook commands return{}immediately.Root cause
Persistent global registration and wrapper injection both owned the same Codex events. The global scripts ran in standalone sessions even when
CMUX_SURFACE_IDwas absent, and wrapped sessions could load both producers. Transport was already fire-and-forget, but separate background processes had no ordering primitive.Verification
--enable hooks; migration removed all 10 cmux hooks and emitted one 15-argument scoped hook set.swiftc -parse,bash -n, andgit diff --checkpass.Design
This is a process-ownership fix, not a language-runtime optimization. Rust would still pay process launch and IPC costs. The existing shell producer now returns quickly, and the OS file lock supplies a bounded FIFO queue without another daemon.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Scope Codex hooks to each launch and queue background deliveries per Codex process to keep events ordered and avoid blocking. This removes duplicate producers and stabilizes hook execution.
New Features
cmux hooks setupnow leaves Codex to the per-launch wrapper by default; the wrapper migrates cmux-owned entries out of~/.codex/hooks.jsonand keeps user hooks.cmux hooks codex installpersist hooks and create a.cmux-persistent-hooks-opt-inmarker; wrapper then emits only--enable hookswithout injecting duplicates.lockfqueue so hooks return{}immediately while events run in FIFO order.Migration
cmux hooks codex install.Written for commit 98464de. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests