Repository navigation
Notify on fatal Codex turn errors - #8170
Merged
Merged
Conversation
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. |
📝 WalkthroughWalkthroughThe Codex stop hook now treats terminal error payloads as authoritative failure candidates. A new integration test runs the hook against a temporary transcript and UNIX socket server, while Xcode project settings include the test in the build. ChangesCodex terminal error handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Transcript
participant CodexStopHook
participant UnixSocketServer
CodexStopHook->>Transcript: Read task_complete payload
Transcript-->>CodexStopHook: Return terminal error
CodexStopHook->>UnixSocketServer: Send error notification and status
UnixSocketServer-->>CodexStopHook: Return focused-surface response
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 1 warning)
✅ 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 |
Contributor
Greptile SummaryThis PR fixes missed
Confidence Score: 5/5Safe to merge — the production change is a targeted fix that correctly adds error extraction before the assistant-message branch, and the regression test validates the primary failure scenario end-to-end. The core logic change is small and well-scoped: two new else-if branches in the task_complete handler that fire only when payload error is present, with the existing last_agent_message path preserved as a fall-through. The return-value logic is unaffected for all pre-existing cases. The test spawns the real CLI binary and confirms the notification and status commands, making the regression coverage meaningful. The string-error branch in CLI/cmux.swift (the else-if codexHookStringValue path) has no corresponding test in cmuxTests/CodexTerminalErrorNotificationTests.swift. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Transcript line: task_complete / turn_complete"] --> B{"payload.error\nis a dict?"}
B -->|Yes| C["codexHookFailureCandidate\nrequireFailureSignal=false"]
C --> D["candidate = structured failure\ncandidateCanPublishBeforeTerminal = false"]
B -->|No| E{"payload.error\nis a string?"}
E -->|Yes| F["candidate = string failure\ncandidateCanPublishBeforeTerminal = false"]
E -->|No| G{"last_agent_message\nnon-empty?"}
G -->|Yes| H["sawAssistantMessage = true\ncandidate = nil"]
G -->|No| I{"candidate==nil AND\nnot sawAssistantMessage?"}
I -->|Yes| J["candidate = no-final-response error"]
I -->|No| K["no change"]
D --> L["return .failure(candidate)"]
F --> L
H --> M["return .healthy"]
J --> L
%%{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["Transcript line: task_complete / turn_complete"] --> B{"payload.error\nis a dict?"}
B -->|Yes| C["codexHookFailureCandidate\nrequireFailureSignal=false"]
C --> D["candidate = structured failure\ncandidateCanPublishBeforeTerminal = false"]
B -->|No| E{"payload.error\nis a string?"}
E -->|Yes| F["candidate = string failure\ncandidateCanPublishBeforeTerminal = false"]
E -->|No| G{"last_agent_message\nnon-empty?"}
G -->|Yes| H["sawAssistantMessage = true\ncandidate = nil"]
G -->|No| I{"candidate==nil AND\nnot sawAssistantMessage?"}
I -->|Yes| J["candidate = no-final-response error"]
I -->|No| K["no change"]
D --> L["return .failure(candidate)"]
F --> L
H --> M["return .healthy"]
J --> L
Reviews (2): Last reviewed commit: "Harden Codex error notification test" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 `@cmuxTests/CodexTerminalErrorNotificationTests.swift`:
- Around line 11-13: Update the temporary socket setup around root and
socketPath to use a deliberately short directory and socket filename, then
validate the socketPath’s UTF-8 byte length against sockaddr_un.sun_path before
binding. Reject oversized paths explicitly before bind, and ensure the exact
validated path is passed to both the server and hook.
- Around line 244-252: The timeout handling around process.terminate() must
remain bounded and avoid reading pipes from a still-running process. After the
initial one-second wait, escalate termination if needed, confirm completion
before accessing terminationStatus or performing readDataToEndOfFile(), and
return a safe timeout result when completion cannot be confirmed.
🪄 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: 8028de85-a43f-483c-a75f-a1a0d12084ad
📒 Files selected for processing (3)
CLI/cmux.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CodexTerminalErrorNotificationTests.swift
hhsw2015
pushed a commit
to hhsw2015/cmux
that referenced
this pull request
Jul 16, 2026
* Add regression test for Codex terminal errors * Notify on Codex terminal errors * Harden Codex error notification test (cherry picked from commit e3cba0e)
6 tasks
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.
Fixes missing cmux notifications when Codex ends a turn with a fatal error, including model capacity failures.
The transcript parser now treats task_complete.error as authoritative even when Codex emitted partial assistant output. The regression test uses the current Codex rollout schema and verifies both the notification and high-priority error status.
Testing:
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Ensure
cmuxsends a notification and sets a high-priority error when a Codex turn ends with a terminal error, even if partial assistant output was emitted. Fixes missed alerts for capacity and other fatal failures.task_complete.erroras authoritative; accept structured objects and plain strings.CodexTerminalErrorNotificationTestswith a socket-backed server; assertnotify_targetcontent andset_status(priority 100, red icon, color#FF453A).Written for commit 2f87150. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests
CodexTerminalErrorNotificationTeststo verify that persisted terminal error notifications win over partial assistant output, including validation of the expected notification command and terminal error status indicators.