Fix Codex resume notification rebinding - #9185
Merged
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughUpdates Codex launch instrumentation, hook failure reporting, transcript stop replay, and resumed-session notification tests. The Xcode project registers the new Swift sources and test suite. ChangesCodex resume reliability
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 passed)
✨ Finishing Touches 💡 1📝 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 |
austinywang
marked this pull request as ready for review
July 29, 2026 23:21
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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`+AgentHookFailureReporting.swift:
- Around line 4-7: Mark the file-scoped agentHookDeliveryLogger declaration as
nonisolated, preserving its existing Logger initialization and
subsystem/category values.
- Around line 26-37: Update the failure reporting around agentHookDeliveryLogger
and telemetry.captureError to scrub/filter the error before it reaches any
observability sink. Log the redacted error with private or hashed OSLog privacy
instead of public, and ensure the raw error is not included in shared
cli_socket.error context or Sentry serialization; preserve the existing agent,
session, and delivery-stage metadata.
In `@tests/test_codex_wrapper_resume_hooks.py`:
- Around line 199-217: Extend the socket-state coverage in
test_every_resume_route_is_instrumented to iterate over missing, stale, and
live, and update test_direct_fork_is_instrumented to assert the live state
alongside stale. Keep the existing entrypoint routes and assertions unchanged.
🪄 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 Plus
Run ID: 85723cf3-a8ba-473b-9205-46cbd52a935d
📒 Files selected for processing (6)
CLI/CMUXCLI+AgentHookFailureReporting.swiftCLI/cmux.swiftResources/bin/cmux-codex-wrappercmux.xcodeproj/project.pbxprojcmuxTests/CLICodexResumeNotificationTests.swifttests/test_codex_wrapper_resume_hooks.py
austinywang
force-pushed
the
issue-9181-codex-resume-notifications
branch
from
July 30, 2026 00:59
bd98599 to
077b090
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/cmux.swift`:
- Line 30946: Update preferredAgentHookEventPID so non-codex hooks (resume,
stop, approvalResponse, and notification) prefer inferredPID whenever no live
mapped process exists, using mappedPID only when the mapped process is live and
falling back to the stored PID when inferredPID is unavailable.
In `@tests/test_codex_wrapper_resume_hooks.py`:
- Around line 263-276: Update test_injection_failure_preserves_cmux_context to
also assert that CMUX_CODEX_PID and CMUX_AGENT_LAUNCH_KIND remain present and
correct in observed_env when injection fails, matching the identity-context
checks used by assert_session_entrypoint_is_instrumented while preserving the
existing workspace and surface assertions.
🪄 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 Plus
Run ID: 51546cea-1245-4b44-b8fe-13a0349e84b0
📒 Files selected for processing (6)
CLI/CMUXCLI+AgentHookFailureReporting.swiftCLI/cmux.swiftResources/bin/cmux-codex-wrappercmux.xcodeproj/project.pbxprojcmuxTests/CLICodexResumeNotificationTests.swifttests/test_codex_wrapper_resume_hooks.py
austinywang
force-pushed
the
issue-9181-codex-resume-notifications
branch
from
July 30, 2026 01:11
077b090 to
713c875
Compare
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/test_codex_wrapper_resume_hooks.py (1)
264-277: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStill missing Codex identity assertions on injection failure.
test_injection_failure_preserves_cmux_contextverifies workspace/surface bindings survive, but doesn't checkCMUX_CODEX_PID/CMUX_AGENT_LAUNCH_KINDthe wayassert_session_entrypoint_is_instrumenteddoes for the success path. The fakecodexscript always logs these regardless of injection outcome, so this gap is easy to close and matters for the "preserve context during failures" resilience goal this PR targets. This was already flagged on a prior commit and remains unaddressed.♻️ Proposed addition
expect(observed_env.get("CMUX_WORKSPACE_ID") == "22222222-2222-2222-2222-222222222222", f"inject-failure: workspace binding was stripped: {observed_env}", failures) + expect(observed_env.get("CMUX_CODEX_PID") not in {None, "", "__UNSET__"}, + f"inject-failure: missing Codex process identity: {observed_env}", failures) + expect(observed_env.get("CMUX_AGENT_LAUNCH_KIND") == "codex", + f"inject-failure: missing launch kind: {observed_env}", failures)🤖 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 `@tests/test_codex_wrapper_resume_hooks.py` around lines 264 - 277, Update test_injection_failure_preserves_cmux_context to assert that observed_env retains the expected CMUX_CODEX_PID and CMUX_AGENT_LAUNCH_KIND values, matching the identity checks in assert_session_entrypoint_is_instrumented while preserving the existing surface and workspace assertions.
🤖 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.
Duplicate comments:
In `@tests/test_codex_wrapper_resume_hooks.py`:
- Around line 264-277: Update test_injection_failure_preserves_cmux_context to
assert that observed_env retains the expected CMUX_CODEX_PID and
CMUX_AGENT_LAUNCH_KIND values, matching the identity checks in
assert_session_entrypoint_is_instrumented while preserving the existing surface
and workspace assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e4acd2ca-84f5-43b4-9efc-e5c343ec125d
📒 Files selected for processing (3)
cmux.xcodeproj/project.pbxprojcmuxTests/CLICodexResumeNotificationTests.swifttests/test_codex_wrapper_resume_hooks.py
austinywang
force-pushed
the
issue-9181-codex-resume-notifications
branch
from
July 30, 2026 01:20
713c875 to
98c288c
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/CLICodexResumeNotificationTests.swift`:
- Line 17: Replace the Date() calls in CLICodexResumeNotificationTests with the
existing deterministic test-clock mechanism, using one fixed fixture timestamp
consistently for both the test and child process. Ensure all affected timestamp
assertions and inputs avoid reading the host wall clock.
In `@tests/test_codex_wrapper_resume_hooks.py`:
- Line 36: Add an explicit harness control alongside inject_args_available and
set IN_CMUX=1 for the affected test cases so the passthrough branch is exercised
deterministically. Update the fallback logic in cmux-codex-wrapper to preserve
the required CMUX_* context instead of clearing it when IN_CMUX is enabled, and
ensure the tests assert that context remains available.
🪄 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 Plus
Run ID: ef0a7192-8b6a-41ae-aada-bcfa0322c210
📒 Files selected for processing (3)
cmux.xcodeproj/project.pbxprojcmuxTests/CLICodexResumeNotificationTests.swifttests/test_codex_wrapper_resume_hooks.py
austinywang
force-pushed
the
issue-9181-codex-resume-notifications
branch
from
July 30, 2026 02:04
50f12f3 to
398823c
Compare
austinywang
force-pushed
the
issue-9181-codex-resume-notifications
branch
from
July 30, 2026 02:15
398823c to
2453522
Compare
…-9181-codex-resume-notifications # Conflicts: # CLI/ClaudeHookSessionStoreFile.swift # CLI/cmux.swift # cmux.xcodeproj/project.pbxproj
This was referenced Jul 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
task_completeevents through the shared Stop reducer, with native Stop deduplicationRoot-cause verification
Live evidence contradicted the proposed end-to-end diagnosis in #9181. The argv parser limitation, nil
excludedUpdatedAtfallthrough, debug-only delivery logging, and context-stripping passthrough were real defects, but they were not what dropped the reproduced picker-resume notification.With an isolated Codex 0.145.0 picker resume, Codex emitted
SessionStart, the original session ID and rollout file were reused, and the store rebound to the live PID. The rollout then recorded a healthytask_complete. Delivery was lost later: the detached Stop path exited before the generic hook reducer reachedagentHook.start.The rollout identity check confirmed resume appended to the original file:
session_id_match=yes same_inode=yes appended=yes matching_rollouts=1.Architecture
The wrapper no longer scrapes resume argv or gates hook installation on launch-time socket health. Every Codex entrypoint receives the same hooks and retains cmux context, while fresh launches still rely on Codex's authoritative SessionStart and therefore do not mint fallback GUI phantoms.
Codex lifecycle events use the live wrapper PID when available. The transcript monitor converts authoritative rollout
task_completerecords into the existing generic Stop path rather than implementing a second notification reducer, and late native Stops are deduplicated. This shared path covers explicit UUID,--last, picker, in-TUI resume, and future resume forms.Tests
98c288cac4introduced three behavior regressions245352283afixed the behaviorpython3 tests/test_codex_wrapper_resume_hooks.pybash -n Resources/bin/cmux-codex-wrapper./scripts/lint-pbxproj-test-wiring.sh(636 test files after syncing current main)./scripts/check-pbxproj.shNo local
xcodebuildor XCUITest was run, per repository instructions. Localization audit: no user-facing strings were added or changed; new messages are developer-only unified logging/Sentry telemetry.Closes #9181