Repository navigation
Fix Codex no-TTY hook live surface misroutes - #6975
austinywang wants to merge 25 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesPID-instance liveness and routing guard
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 SummaryFixes the no-TTY Codex hook surface-misroute bug (#5676) by adding
Confidence Score: 5/5Production logic is correct and safe to merge; the only gap is a duplicated test case that leaves one regression scenario unexercised. The production surfaceHasDifferentLiveOwner guard, recycled-PID detection, and pidCapturedAt tracking are all correctly implemented. The snapshot-outside-lock pattern avoids holding the store lock across sysctl calls, the pid_t clamp prevents integer overflow on corrupted records, and legacy records without pidCapturedAt continue to be treated conservatively. The one concern is in the test file: the "newerRunning" owner case is identical to "running", leaving the pre-seeded newcomer session / incomingSurfaceId-preservation branch untested, but this does not affect shipping behavior. The codexHookWithoutTTYDoesNotRouteOntoDifferentLiveSurfaceOwner loop in CLICodexHookTimeoutRegressionTests.swift — verify whether the "newerRunning" case should have ownerStartedAfterIncoming: true. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Hook as no-TTY Codex Hook
participant Store as ClaudeHookSessionStore
participant OS as sysctl (kernel)
Hook->>Store: surfaceHasDifferentLiveOwner(workspace, surface, incomingSession)
Store->>Store: withLockedState — snapshot candidates
Store-->>Hook: candidate list (sessionId, pid, pidCapturedAt, updatedAt)
loop for each candidate (outside lock)
Store->>OS: kill(pid, 0) — process exists?
OS-->>Store: 0 / EPERM / ESRCH
alt process dead or PID recycled
Store->>Store: withLockedState — clear runtimeStatus
else process live and same instance
Store->>Store: withLockedState — confirm still owns surface
Store-->>Hook: true → block route
end
end
Store-->>Hook: false → allow route
Hook->>Hook: targetUnlessBlockedByDifferentLiveOwner returns nil
%%{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"}}}%%
sequenceDiagram
participant Hook as no-TTY Codex Hook
participant Store as ClaudeHookSessionStore
participant OS as sysctl (kernel)
Hook->>Store: surfaceHasDifferentLiveOwner(workspace, surface, incomingSession)
Store->>Store: withLockedState — snapshot candidates
Store-->>Hook: candidate list (sessionId, pid, pidCapturedAt, updatedAt)
loop for each candidate (outside lock)
Store->>OS: kill(pid, 0) — process exists?
OS-->>Store: 0 / EPERM / ESRCH
alt process dead or PID recycled
Store->>Store: withLockedState — clear runtimeStatus
else process live and same instance
Store->>Store: withLockedState — confirm still owns surface
Store-->>Hook: true → block route
end
end
Store-->>Hook: false → allow route
Hook->>Hook: targetUnlessBlockedByDifferentLiveOwner returns nil
Reviews (13): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | 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: 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 `@CLI/cmux.swift`:
- Around line 1313-1327: The processIsLiveForSession(pid:capturedAt:) check
still force-casts pid to pid_t without an upper bound, which can trap for
corrupted session data. Add the same Int32.max clamp used by
processStartTime(pid:) before calling kill(pid_t(pid), 0), and return false when
pid is nil, non-positive, or exceeds the safe range so the session validation
fails closed.
🪄 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: 46db665d-48bb-43a2-a4f8-277caf5198ec
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (2)
CLI/cmux.swiftcmuxTests/CLICodexHookTimeoutRegressionTests.swift
…-no-tty-misroute-corrupts-a-live-se # Conflicts: # .github/swift-file-length-budget.tsv
…-no-tty-misroute-corrupts-a-live-se # Conflicts: # .github/swift-file-length-budget.tsv
…-no-tty-misroute-corrupts-a-live-se # Conflicts: # .github/swift-file-length-budget.tsv
…-no-tty-misroute-corrupts-a-live-se # Conflicts: # .github/swift-file-length-budget.tsv
…-no-tty-misroute-corrupts-a-live-se # Conflicts: # .github/swift-file-length-budget.tsv
…-no-tty-misroute-corrupts-a-live-se # Conflicts: # .github/swift-file-length-budget.tsv
…-no-tty-misroute-corrupts-a-live-se
…-no-tty-misroute-corrupts-a-live-se
…-no-tty-misroute-corrupts-a-live-se # Conflicts: # .github/swift-file-length-budget.tsv
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
3 participants
Fixes #5676
Summary
pid_tconversion so corrupted records fail closed instead of crashing.Testing
xcrun swiftc -parse CLI/cmux.swift cmuxTests/CLICodexHookTimeoutRegressionTests.swift cmuxTests/CLIGenericHookPersistenceTests.swiftpython3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv./tests/test_ci_swift_file_length_budget.sh./scripts/lint-pbxproj-test-wiring.shgit diff --check7e168f303376d00e05b23fd1d72791236e8cad77.Demo Video
N/A - CLI hook routing fix with mock-socket regression coverage.
Review Trigger
Bug fix for issue #5676, preserving the required two-commit failing-test then fix structure and subsequent review/CI follow-up commits.
Checklist
reload.sh,reload-cloud.sh, or barexcodebuildrun per task instruction.Summary by CodeRabbit