Fix Codex trust gates after session restore - #8783
lawrencecchen wants to merge 135 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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:
📝 WalkthroughWalkthroughCodex resume flows now evaluate project trust, inject validated overrides, preserve captured executables, harden wrapper execution, and rebind sessions using process-generation identity. Supporting async process handling, probe caching, shell parsing, and regression tests were added. ChangesCodex resume trust evaluation
Resume argument injection
Captured executable routing
Resume session rebinding
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (6 errors, 1 warning)
✅ Passed checks (18 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 Codex trust-gate and rebinding behavior after session restore. It routes all Codex resume/fork commands through the
Confidence Score: 4/5Safe to merge with one fix: the new The trust-probe pipeline routes the git worktree lookup ( Files Needing Attention: CLI/CMUXCLI+CodexFireAndForgetHooks.swift — Important Files Changed
Sequence DiagramsequenceDiagram
participant W as cmux-codex-wrapper
participant C as cmux CLI (inject-resume-args)
participant AS as Codex app-server
participant CX as Codex (resumed)
participant S as ClaudeHookSessionStore
W->>W: find_real_codex() – project-boundary validated
W->>W: cmux_codex_read_inject_args() – hook prefix args
W->>C: hooks codex inject-resume-args
C->>C: codexResumeTrustOverride()
C->>AS: config/read (via runJSONLinesProcess, async)
AS-->>C: effective projects map
C->>C: CodexResumeTrustProbeCache.resolve()
C-->>W: "NUL-separated [-c, projects={cwd={trust_level=untrusted}}]"
W->>CX: exec codex [hook-args] resume id [-c trust-override] [--]
CX->>S: "SessionStart (cmux_resume_rebind=true, processLeaseId)"
S->>S: prepareCodexProcessEventIfFresh()
S->>S: resumedProcessGenerationRelation → promote newer PID
CX->>S: UserPromptSubmit / Stop (processLeaseId)
S->>S: preparedMappedSession() → accept only same lease
Reviews (28): Last reviewed commit: "fix Codex resume race test harness" | 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CLI/cmux.swift (1)
1011-1037: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRequire prior-PID liveness before accepting wrapper rebind
In
CLI/cmux.swift:1022-1025,cmux_resume_rebind: truewithincomingPID != record.pidbypassesactivePromptDepth/activePromptTurnIdsstaleness checks before updatingrecord.pid. The wrapper emits this flag directly fromCMUX_CODEX_PID=$$without checking whether the previously recordedrecord.pidhas exited, so another livecodex resume <same-session-id>invocation can silently steal ownership from the still-running pane. Grant this bypass only when the prior PID is absent or has exited (using an existingprocessExists-style check), or fail closed when the prior process is still alive.🤖 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/cmux.swift` around lines 1011 - 1037, Update codexSessionStartIsStale so allowResumedProcessReplacement can bypass active-turn staleness only when record.pid is absent or no longer running; check the prior PID with the existing processExists-style helper before returning false. If the prior process is still alive, retain the normal staleness checks and fail closed against replacing its ownership.Source: Coding guidelines
Packages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentResumeArgvTests.swift (1)
308-344: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a direct unit case for absolute codex path with no
envprefix.The new assertions cover bare
codexand an absolute path with an existingenvprefix, but not an absolute path with no prefix at all (e.g.parts: ["/opt/bin/codex", "resume", "SID"]). That exact shape is exercised indirectly viacmuxTests/SessionPersistenceTests.swift'stestCodexResumeCommandDropsStartupImagesAndPlacesSessionBeforeFlags(environment: nil), which confirms the renderer correctly synthesizes a newenv CMUX_CUSTOM_CODEX_PATH=...prefix. Adding it here too would isolate regressions inrenderedPortableCodexResumeShellCommanditself without relying on the higher-level snapshot test.🤖 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 `@Packages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentResumeArgvTests.swift` around lines 308 - 344, The test renderedPortableCodexResumeShellCommand lacks direct coverage for an absolute Codex executable without an existing env prefix. Add an assertion using parts ["/opt/bin/codex", "resume", "SID"] that verifies the renderer synthesizes the CMUX_CUSTOM_CODEX_PATH env assignment, routes the executable through codexWrapperShellExecutableToken, and preserves the /bin/sh -c quoting.
🤖 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/CLINotifyProcessIntegrationRegressionTests.swift`:
- Around line 1784-1842: Update
testCodexWrapperResumeSessionStartRebindsInterruptedActivePrompt to read the
session record from the codex-hook-sessions.json state file in context.root
instead of using readClaudeHookSession, which targets the Claude state file.
Preserve the existing assertions so they validate the session-start rebind
updates the written Codex record.
In
`@Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexResumeTrustPolicy.swift`:
- Around line 143-156: Add a guard in projectPathFromEffectiveCLITrustKey after
computing start and end to require start <= end before slicing, returning nil
for malformed or empty project segments. Preserve valid project-path parsing and
add a regression test in CodexResumeTrustPolicyTests for the
projects.trust_level override through undecidedProjectOverride.
In `@Resources/bin/cmux-codex-wrapper`:
- Line 307: Update the resume-session handling around cmux_codex_resume_payload
and the final exec_real_codex_passthrough path so unavailable hook injection
fails closed: either add the required resume hook arguments explicitly or
delegate to Codex’s trust picker. Ensure codex resume never launches with
missing hook args and an unhandled trust prompt when CMUX_CODEX_HOOK_CMUX_BIN or
CMUX_INJECT_CLI is unavailable.
---
Outside diff comments:
In `@CLI/cmux.swift`:
- Around line 1011-1037: Update codexSessionStartIsStale so
allowResumedProcessReplacement can bypass active-turn staleness only when
record.pid is absent or no longer running; check the prior PID with the existing
processExists-style helper before returning false. If the prior process is still
alive, retain the normal staleness checks and fail closed against replacing its
ownership.
In
`@Packages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentResumeArgvTests.swift`:
- Around line 308-344: The test renderedPortableCodexResumeShellCommand lacks
direct coverage for an absolute Codex executable without an existing env prefix.
Add an assertion using parts ["/opt/bin/codex", "resume", "SID"] that verifies
the renderer synthesizes the CMUX_CUSTOM_CODEX_PATH env assignment, routes the
executable through codexWrapperShellExecutableToken, and preserves the /bin/sh
-c quoting.
🪄 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: 3df9e430-3b3d-4a76-9cc7-2436497e0e20
📒 Files selected for processing (14)
CLI/CMUXCLI+CodexFireAndForgetHooks.swiftCLI/cmux.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentResumeArgv.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexResumeTrustPolicy.swiftPackages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentResumeArgvTests.swiftPackages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/CodexResumeTrustPolicyTests.swiftResources/bin/cmux-codex-wrapperSources/RestorableAgentSession.swiftSources/SurfaceResumeCommandCanonicalizer+PortableAgentExecutable.swiftcmuxTests/CLINotifyProcessIntegrationRegressionTests.swiftcmuxTests/SessionPersistenceResumeBindingTests.swiftcmuxTests/SessionPersistenceTests.swiftcmuxTests/SurfaceResumeBindingCodexUpdateCheckTests.swifttests/test_codex_wrapper_resume_trust.py
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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Resources/bin/cmux-codex-wrapper (1)
356-387: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the resume-helper exit status before appending its output.
The consumer status from the process substitution is different from the subprocess status, so
cmux_codex_read_resume_argscan return success after capturing partial output. Use a status-preserving capture, such asmapfile -td '' cmux_codex_resume_args < <(...) || return $?, and reset the array on failure before appending${cmux_codex_resume_args[@]}.🤖 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 `@Resources/bin/cmux-codex-wrapper` around lines 356 - 387, The function cmux_codex_read_resume_args must preserve the helper subprocess exit status instead of relying on the process-substitution read loop. Replace both capture loops with status-preserving array capture, such as mapfile, and return the captured failure status; clear cmux_codex_resume_args when the capture fails so the caller never appends partial output. Keep the existing socket/non-socket command selection and success validation intact.
♻️ Duplicate comments (1)
Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexResumeTrustPolicy.swift (1)
211-246: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winUnresolved: Range crash on malformed
projects.<key>.trust_levelCLI override.This is the same defect flagged on a previous commit of this PR and it is still present, unfixed.
projectPathFromEffectiveCLITrustKey(lines 232-245) sliceskey[start..<end]using fixed offsets from the string's start/end without checkingstart <= end. Whenkey == "projects.trust_level"(no path segment — e.g. a resume-scoped-c 'projects.trust_level="trusted"'), the literals"projects."and".trust_level"overlap on the shared., givingstart == 9 > end == 8. Formingkey[9..<8]traps at runtime.This is reachable from production code:
undecidedProjectOverride→argumentsContainProjectTrustDecision(line 37, scanning resume-scoped args) →projectTrustOverrideMatches(line 222-223) → this function — and is invoked fromCMUXCLI+CodexFireAndForgetHooks.swift'scodexResumeTrustOverride()on every Codex resume, for any-c/--configargument in the captured launch argv. A resume invocation like["codex", "resume", "SID", "-c", "projects.trust_level=\"trusted\""]would crash the wrapper's hook-injection step this PR is meant to make unattended-safe.No regression test for this scenario was added in
CodexResumeTrustPolicyTests.swifteither.💥 Proposed fix: bound-check before slicing
private func projectPathFromEffectiveCLITrustKey(_ key: String) -> String? { - guard key.hasPrefix("projects."), - key.hasSuffix(".trust_level") else { + let prefix = "projects." + let suffix = ".trust_level" + guard key.hasPrefix(prefix), + key.hasSuffix(suffix), + key.count >= prefix.count + suffix.count else { return nil } - let start = key.index(key.startIndex, offsetBy: "projects.".count) - let end = key.index(key.endIndex, offsetBy: -".trust_level".count) + let start = key.index(key.startIndex, offsetBy: prefix.count) + let end = key.index(key.endIndex, offsetBy: -suffix.count) + guard start <= end else { return nil } let path = String(key[start..<end]) guard path.hasPrefix("/"), !path.contains(".") else { return nil } return path }Consider adding a regression test in
CodexResumeTrustPolicyTests.swiftforarguments: ["codex", "resume", "SID", "-c", #"projects.trust_level="trusted""#]to lock in the fix.🤖 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 `@Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexResumeTrustPolicy.swift` around lines 211 - 246, Prevent projectPathFromEffectiveCLITrustKey from forming an invalid range when the projects prefix and trust_level suffix overlap or leave no path segment; validate the computed boundaries before slicing and return nil for malformed keys such as projects.trust_level. Add a regression test in CodexResumeTrustPolicyTests.swift covering this resume argument and verifying it does not crash and is not treated as a project override.
🤖 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 1042-1056: Update resumedProcessGenerationIsNewer to corroborate
the start-time comparison with a process identity check using the existing
processName(for:) helper, accepting only incoming PIDs whose executable still
identifies them as codex; otherwise return false. Preserve the current timestamp
ordering logic after identity validation so unrelated PID reuse cannot authorize
resumed-process replacement.
In `@tests/test_codex_wrapper_resume_trust.py`:
- Around line 101-138: Extend the resume-wrapper tests around inject-resume-args
to cover helper failures after no output and after partial output, asserting the
original arguments remain unchanged without a malformed trust suffix. Add cases
for persisted explicit trust and command-line explicit trust decisions,
asserting neither path appends TRUST_OVERRIDE; reuse the existing run_wrapper
setup and argument assertions.
---
Outside diff comments:
In `@Resources/bin/cmux-codex-wrapper`:
- Around line 356-387: The function cmux_codex_read_resume_args must preserve
the helper subprocess exit status instead of relying on the process-substitution
read loop. Replace both capture loops with status-preserving array capture, such
as mapfile, and return the captured failure status; clear cmux_codex_resume_args
when the capture fails so the caller never appends partial output. Keep the
existing socket/non-socket command selection and success validation intact.
---
Duplicate comments:
In
`@Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexResumeTrustPolicy.swift`:
- Around line 211-246: Prevent projectPathFromEffectiveCLITrustKey from forming
an invalid range when the projects prefix and trust_level suffix overlap or
leave no path segment; validate the computed boundaries before slicing and
return nil for malformed keys such as projects.trust_level. Add a regression
test in CodexResumeTrustPolicyTests.swift covering this resume argument and
verifying it does not crash and is not treated as a project override.
🪄 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: 2c141b86-6dbb-4f84-bd8c-9b9c7f7eca73
📒 Files selected for processing (11)
CLI/CMUXCLI+CodexFireAndForgetHooks.swiftCLI/cmux.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentResumeArgv.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexResumeTrustPolicy.swiftPackages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentResumeArgvTests.swiftPackages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/CodexResumeTrustPolicyTests.swiftResources/bin/cmux-codex-wrapperSources/RestorableAgentSession.swiftcmuxTests/CLINotifyProcessIntegrationRegressionTests.swiftcmuxTests/RestorableCodexForkTagTests.swifttests/test_codex_wrapper_resume_trust.py
…st-gates # Conflicts: # web/tests/feedback-route.test.ts
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. |
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. |
lawrencecchen
added
the
stale-revisit
2 participants
Summary
Verification
48f0768d98019f91b7-f454-7221-b3f2-d278b218a154in/private/tmp/cxrsm-final-head.0s8npF/nested/workrestored through PIDs36225 → 90191 → 27996across two consecutive hard app crashes--dangerously-bypass-hook-trustbeforeresumeandprojects={"/private/tmp/cxrsm-final-head.0s8npF/nested/work"={trust_level="untrusted"}}afterresumeidentifyandread-screenpreflight checks passSecurity
Restored invocations use Codex hook trust bypass, so enabled user or project hooks can run outside the sandbox without another review prompt.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Keeps Codex auto‑resume non‑interactive and safe after session restore without persisting project trust or changing the captured Codex binary. All resumes and forks route back through
cmux-codex-wrapper; we inject a resume‑scoped untrusted override only when Codex has no decision and fire a best‑effort session‑start for named resumes.Bug Fixes
resume/before--only when undecided; discard partial/malformed/oversized output; coalesce concurrent probes with a process‑shared on‑disk cache and per‑invocation freshness.CMUX_CUSTOM_CODEX_PATH(honorsCMUX_CODEX_WRAPPER_SHIM); preserve symlink identity; acceptVAR=value/envprefixes and-C/--cd; robust argv parsing for--and attached‑image flags; preserve argument order, fork tag order, and explicit update‑check settings; keep custom env portable./bin/bashand emit NUL‑separated args; rebind repeated restores using start‑time identity plus process leases and a hidden process‑identity helper; authenticate delayed stop/teardown; treatforkas a session start; clear interrupted turn state; bound app‑server reaping.Refactors
CodexResumeTrustPolicy,CodexResumeTrustProbeCache, a JSONL runner,emitCodexWrapperResumeArgs, and a hidden process‑identity helper; portable resume commands now route captured executables through the wrapper.Written for commit 60dcafa. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests