Restore Codex sessions through user-owned roots - #8321
lawrencecchen wants to merge 23 commits into
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:
📝 WalkthroughWalkthroughCodex resume handling now verifies session evidence from indexed rollout records or transcript metadata. Transcript paths propagate through stored resume records, session snapshots, CLI ownership checks, and terminal restoration, while unverified or mismatched agent-hook bindings are rejected. ChangesCodex resume verification
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AgentHook
participant CMUXCLI
participant CodexSessionResumeVerifier
participant Workspace
participant ResumeStore
AgentHook->>CMUXCLI: submit session and transcript metadata
CMUXCLI->>CodexSessionResumeVerifier: verify resume evidence
CodexSessionResumeVerifier-->>CMUXCLI: canonical session or nil
CMUXCLI->>ResumeStore: store or clear binding
Workspace->>CodexSessionResumeVerifier: verify restore evidence
CodexSessionResumeVerifier-->>Workspace: accepted evidence or nil
Workspace->>ResumeStore: resolve verified terminal binding
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 2 warnings)
✅ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: df802cb396
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard let restorableAgent, restorableAgent.kind == .codex else { return restorableAgent } | ||
| return codexResumeVerifierOwnsSession( | ||
| restorableAgent.sessionId, | ||
| environment: restorableAgent.launchCommand?.environment, | ||
| transcriptPath: restorableAgent.transcriptPath, | ||
| verifier: codexResumeVerifier | ||
| ) ? restorableAgent : nil |
There was a problem hiding this comment.
On upgrade, every previously persisted SessionRestorableAgentSnapshot decodes with transcriptPath == nil. For users on an older Codex installation without state_5.sqlite, this validation has no rollout path to check and returns nil, while the separately revalidated old agent-hook binding likewise has no transcript path; the restored terminal therefore becomes a plain shell instead of resuming an otherwise valid legacy Codex session. Resolve the rollout from the hook-store/index (or scan the legacy session files by ID) before applying this validation so the claimed legacy fallback also covers snapshots created by earlier cmux releases.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR fixes two related Codex session-restore bugs: an indexed child worker could replace the durable root identity, and snapshot reconciliation erased hook bindings when a transient empty process scan temporarily found no agent. The fix introduces
Confidence Score: 5/5Safe to merge; the restore path is well-covered by regression tests and the durable-binding change is deliberate and sound. The core logic — verifying session ownership via the SQLite thread index, following parent links, rejecting automation/exec/subagent roots, and making hook bindings durable across empty process scans — is correct and thoroughly tested. The one new finding (per-iteration SQLite reconnect inside the parent-chain traversal loop) is a cold-path efficiency issue, not a correctness bug. Previously-flagged concerns (synchronous reads on the main-actor restore path, NSHomeDirectory vs HOME mismatch) are known and carried over rather than introduced by this PR. Files Needing Attention: CodexSessionResumeVerifier.swift (indexedEvidence reconnects per parent step); Workspace.swift and DockSplitStore+SessionRestore.swift carry the synchronous SQLite-on-main-actor concern noted in earlier threads. Important Files Changed
Sequence DiagramsequenceDiagram
participant Hook as CLI Hook Event
participant CLI as CMUXCLI
participant Verifier as CodexSessionResumeVerifier
participant SQLite as state_5.sqlite
participant Store as Hook Session Store
participant App as Workspace (App)
participant Restore as RestorableAgentSessionIndex
Hook->>CLI: session-start / prompt-submit (sessionId, transcriptPath)
CLI->>Verifier: agentHookCanonicalResumeSessionId(sessionId)
Verifier->>SQLite: "SELECT rollout_path, thread_source WHERE id = sessionId"
alt Indexed subagent
SQLite-->>Verifier: "rollout_path, thread_source=subagent"
Verifier->>Verifier: read session_meta to parentSessionId
Verifier->>SQLite: "SELECT rollout_path WHERE id = parentSessionId"
SQLite-->>Verifier: "parent rollout_path, thread_source=user"
Verifier-->>CLI: "evidence(sessionId=parentId)"
else Indexed user root
SQLite-->>Verifier: "rollout_path, thread_source=user"
Verifier-->>CLI: "evidence(sessionId=sessionId)"
else Not indexed legacy
Verifier->>Verifier: read transcriptPath to session_meta
Verifier-->>CLI: evidence via legacyRollout
end
CLI->>Store: updateAgentSurfaceResumeBinding(resumeSessionId)
Note over App,Restore: App restart / panel restore
App->>Restore: RestorableAgentSessionIndex.load()
Restore->>Verifier: providerVerifiedHookRecord(record)
Verifier->>SQLite: verify and resolve parent chain
Verifier-->>Restore: verified record (parentSessionId, rolloutPath)
Restore-->>App: RestorableAgentSessionIndex
App->>App: verifiedSessionRestoreInputs(binding, restorableAgent)
App->>Verifier: codexResumeEvidence(sessionId, environment)
Verifier->>SQLite: verify session ownership
Verifier-->>App: CodexSessionResumeEvidence
App->>App: retarget binding command to parentSessionId
App->>App: createPanel(restorableAgent, resumeBinding)
Reviews (12): Last reviewed commit: "fix: verify legacy Codex restore binding..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| #if DEBUG | ||
| nonisolated static func sessionRestoreInputsForTesting( | ||
| binding: SurfaceResumeBindingSnapshot?, | ||
| restorableAgent: SessionRestorableAgentSnapshot? | ||
| ) -> (binding: SurfaceResumeBindingSnapshot?, restorableAgent: SessionRestorableAgentSnapshot?) { | ||
| let effectiveBinding = resumeBindingForSessionRestore( | ||
| binding, | ||
| restorableAgent: restorableAgent, | ||
| codexResumeVerifier: CodexSessionResumeVerifier() | ||
| ) | ||
| return ( | ||
| effectiveBinding, | ||
| restorableAgentForSessionRestore(restorableAgent, resumeBinding: effectiveBinding) | ||
| ) | ||
| } | ||
| #endif |
There was a problem hiding this comment.
#if DEBUG test seam in production Sources/
sessionRestoreInputsForTesting matches the exact prohibited naming pattern (…ForTesting) and is guarded by #if DEBUG with no production caller — it only exists so SessionPersistenceResumeBindingTests can exercise resumeBindingForSessionRestore and restorableAgentForSessionRestore together. The canonical fix is to widen those two private static helpers to internal (they're already nonisolated), drop this shim entirely, and call the helpers directly from the test target via @testable import Cmux.
Rule Used: Flag Swift files under a production Sources path (... (source)
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!
| private static let codexSessionResumeVerifier = CodexSessionResumeVerifier() | ||
|
|
||
| func agentHookProviderOwnsResumeTarget( | ||
| kind: String, |
There was a problem hiding this comment.
codexSessionResumeVerifier is a process-lifetime static with an internal CodexThreadIndexCache that loads state_5.sqlite once per database path and never re-reads it. Any Codex session created after the first hook fires won't appear in the frozen dictionary; those hooks fall through to the transcriptPath / legacyRollout path. For modern Codex hooks that include transcript_path this is a silent degradation, but for hooks that don't include it (e.g. stripped environments) the valid new session is incorrectly rejected as a review UUID. A targeted re-query on cache miss — rather than treating an empty result as final — would keep the cache warm for subsequent look-ups while still loading new entries that Codex indexed after startup.
| private func regularNonEmptyFileExists(atPath path: String, fileManager: FileManager) -> Bool { | ||
| guard let attributes = try? fileManager.attributesOfItem(atPath: path), | ||
| attributes[.type] as? FileAttributeType == .typeRegular, | ||
| let size = attributes[.size] as? NSNumber else { | ||
| return false | ||
| } | ||
| return size.int64Value > 0 | ||
| } | ||
| } | ||
|
|
||
| private func rolloutContainsSessionMetadata( | ||
| sessionId: String, | ||
| path: String, | ||
| fileManager: FileManager | ||
| ) -> Bool { | ||
| guard regularNonEmptyFileExists(atPath: path, fileManager: fileManager), | ||
| let handle = FileHandle(forReadingAtPath: path) else { | ||
| return false | ||
| } | ||
| defer { try? handle.close() } | ||
|
|
||
| let prefix = handle.readData(ofLength: 256 * 1024) | ||
| guard let text = String(data: prefix, encoding: .utf8) else { return false } | ||
| for line in text.split(whereSeparator: \Character.isNewline).prefix(32) { | ||
| guard let data = String(line).data(using: .utf8), | ||
| let object = try? JSONSerialization.jsonObject(with: data) as? [String: Any], | ||
| object["type"] as? String == "session_meta", | ||
| let payload = object["payload"] as? [String: Any], | ||
| payload["id"] as? String == sessionId else { | ||
| continue | ||
| } | ||
| return true | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| private func regularNonEmptyFileExists(atPath path: String, fileManager: FileManager) -> Bool { | ||
| guard let attributes = try? fileManager.attributesOfItem(atPath: path), | ||
| attributes[.type] as? FileAttributeType == .typeRegular, | ||
| let size = attributes[.size] as? NSNumber else { | ||
| return false | ||
| } | ||
| return size.int64Value > 0 | ||
| } |
There was a problem hiding this comment.
regularNonEmptyFileExists implementation
regularNonEmptyFileExists(atPath:fileManager:) is defined identically — same body, same signature — on both the private CodexThreadIndexCache class (line 127) and on CodexSessionResumeVerifier itself (line 163). The outer struct's copy is used only by rolloutContainsSessionMetadata; the inner class uses its own copy. Consider extracting a single fileprivate free function or a shared internal helper to avoid drift if the implementation needs to change.
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!
| let codexHome = normalizedResumeBindingValue(environment?["CODEX_HOME"]) | ||
| ?? URL(fileURLWithPath: NSHomeDirectory(), isDirectory: true) | ||
| .appendingPathComponent(".codex", isDirectory: true) | ||
| .path |
There was a problem hiding this comment.
NSHomeDirectory() skips environment HOME override
codexResumeVerifierOwnsSession falls back directly to NSHomeDirectory(), but the CLI counterpart (agentHookProviderOwnsResumeTarget) first checks environment["HOME"] before calling NSHomeDirectory(). In integration tests and sandboxed environments, HOME is overridden in the process environment while NSHomeDirectory() returns the real home directory, so the two helpers would derive different codexHome paths for the same session.
| let codexHome = normalizedResumeBindingValue(environment?["CODEX_HOME"]) | |
| ?? URL(fileURLWithPath: NSHomeDirectory(), isDirectory: true) | |
| .appendingPathComponent(".codex", isDirectory: true) | |
| .path | |
| let codexHome = normalizedResumeBindingValue(environment?["CODEX_HOME"]) | |
| ?? URL( | |
| fileURLWithPath: normalizedResumeBindingValue(ProcessInfo.processInfo.environment["HOME"]) | |
| ?? NSHomeDirectory(), | |
| isDirectory: true | |
| ).appendingPathComponent(".codex", isDirectory: true).path |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/RestorableAgentSession.swift (1)
1119-1141: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve the verifier’s authoritative rollout path in the snapshot.
hookRecordIsRestorablediscardsCodexSessionResumeEvidence, then Line 1141 stores only the hook record’s optional path. Indexed records withoutrecord.transcriptPaththerefore lose the verified rollout path. Return the evidence and persistevidence.rolloutPath.As per path instructions, resume authority must come from the provider-owned verifier rather than a secondary saved value.
Also applies to: 1356-1368
🤖 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 `@Sources/RestorableAgentSession.swift` around lines 1119 - 1141, Update hookRecordIsRestorable to return the provider-owned CodexSessionResumeEvidence rather than discarding it, and use the returned evidence when constructing each RestorableAgentSnapshot. Persist evidence.rolloutPath for transcriptPath at both affected snapshot-building paths, including indexed records without record.transcriptPath, while retaining the existing non-Codex behavior.Source: Path instructions
🤖 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`+AgentHookRestoreEvidence.swift:
- Around line 8-26: Update agentHookProviderOwnsResumeTarget to accept the
caller-selected Codex home or environment override and stop resolving Codex home
from ambient ProcessInfo state. In CLI/CMUXCLI+AgentHookRestoreEvidence.swift
lines 8-26, use the explicit value for verification; in
CLI/CMUXCLI+SessionsList.swift lines 245-250, pass the record-specific codexHome
already computed for that row so ownership verification and launch_backed use
the same authoritative source.
In `@cmuxTests/CLICodexWeakEnvironmentRestoreBindingTests.swift`:
- Around line 419-434: Strengthen the assertions in the restore-binding test
around the observed commands: collect every surface.resume.clear request, then
require that all cleared checkpoint_id values equal the rejected sessionId.
Preserve the existing assertion that the invalid checkpoint is cleared, while
ensuring no unrelated valid resume binding is removed.
In
`@Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexSessionResumeVerifier.swift`:
- Around line 57-83: The CodexThreadIndexCache rolloutPath lookup must not rely
on permanently cached SQLite mappings. Update rolloutPath to reload via
loadRolloutPaths whenever the database may have changed, or remove cross-call
caching entirely, ensuring SQLite remains authoritative and preventing stale
empty, removed, or remapped session entries from being accepted.
In `@Sources/Workspace.swift`:
- Around line 1026-1041: Remove the DEBUG-only sessionRestoreInputsForTesting
helper from Workspace.swift. Move any needed test scaffolding into the test
target and exercise the production restore helpers through `@testable` import,
ensuring tests use providerVerifiedRestorableAgentForSessionRestore and cannot
retain Codex snapshots that production drops.
---
Outside diff comments:
In `@Sources/RestorableAgentSession.swift`:
- Around line 1119-1141: Update hookRecordIsRestorable to return the
provider-owned CodexSessionResumeEvidence rather than discarding it, and use the
returned evidence when constructing each RestorableAgentSnapshot. Persist
evidence.rolloutPath for transcriptPath at both affected snapshot-building
paths, including indexed records without record.transcriptPath, while retaining
the existing non-Codex behavior.
🪄 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: 4e955234-5d23-4a9b-92d0-d4f22f4c485a
📒 Files selected for processing (11)
CLI/CMUXCLI+AgentHookRestoreEvidence.swiftCLI/CMUXCLI+SessionsList.swiftCLI/cmux.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexSessionResumeVerifier.swiftPackages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/CodexSessionResumeVerifierTests.swiftSources/RestorableAgentSession.swiftSources/Sidebar/AppKitList/Cells/SidebarWorkspaceRowSlotViews.swiftSources/Workspace.swiftcmuxTests/CLICodexWeakEnvironmentRestoreBindingTests.swiftcmuxTests/RestorableAgentSessionIndexCodexWeakRecordTests.swiftcmuxTests/SessionPersistenceResumeBindingTests.swift
| let commands = state.snapshot() | ||
| XCTAssertFalse( | ||
| commands.contains { self.jsonObject($0)?["method"] as? String == "surface.resume.set" }, | ||
| "unindexed review UUID must not become restore authority: \(commands)" | ||
| ) | ||
| XCTAssertTrue( | ||
| commands.contains { command in | ||
| guard let payload = self.jsonObject(command), | ||
| payload["method"] as? String == "surface.resume.clear", | ||
| let params = payload["params"] as? [String: Any] else { | ||
| return false | ||
| } | ||
| return params["checkpoint_id"] as? String == sessionId | ||
| }, | ||
| "the invalid checkpoint should be cleared without touching another session: \(commands)" | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that no unrelated resume binding is cleared.
The test passes if the CLI clears sessionId and another valid checkpoint. Collect all surface.resume.clear requests and require every one to target the rejected ID.
Proposed assertion
- XCTAssertTrue(
- commands.contains { command in
+ let clearRequests = commands.compactMap { command -> [String: Any]? in
guard let payload = self.jsonObject(command),
payload["method"] as? String == "surface.resume.clear",
let params = payload["params"] as? [String: Any] else {
- return false
+ return nil
}
- return params["checkpoint_id"] as? String == sessionId
- },
+ return params
+ }
+ XCTAssertEqual(clearRequests.count, 1, "\(commands)")
+ XCTAssertTrue(
+ clearRequests.allSatisfy { $0["checkpoint_id"] as? String == sessionId },
"the invalid checkpoint should be cleared without touching another session: \(commands)"
)As per path instructions, correctness-critical resume authority must use one reliable source and fail closed without disturbing another binding.
🤖 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 `@cmuxTests/CLICodexWeakEnvironmentRestoreBindingTests.swift` around lines 419
- 434, Strengthen the assertions in the restore-binding test around the observed
commands: collect every surface.resume.clear request, then require that all
cleared checkpoint_id values equal the rejected sessionId. Preserve the existing
assertion that the invalid checkpoint is cleared, while ensuring no unrelated
valid resume binding is removed.
Source: Path instructions
|
Dogfood passed on tagged build
|
…ssion-restore # Conflicts: # Sources/RestorableAgentSession.swift # Sources/Workspace.swift
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CLI/CMUXCLI+AgentHookRestoreEvidence.swift (1)
6-6: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftAvoid introducing a process-wide verifier singleton.
private static let codexSessionResumeVerifiercreates ambient runtime state for a verifier that owns shared caching. Make the verifier an injected or instance-scoped dependency so callers and tests can control its lifetime and isolation.🤖 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`+AgentHookRestoreEvidence.swift at line 6, Remove the process-wide codexSessionResumeVerifier static singleton and make CodexSessionResumeVerifier an injected or instance-scoped dependency for the callers that use it. Update those callers and tests to supply or construct the verifier explicitly, preserving cache isolation and controllable lifetime.Source: Coding guidelines
CLI/cmux.swift (1)
27695-27736: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSend the canonical checkpoint id when clearing resume bindings.
publishAgentSurfaceResumeBindingnow storescheckpoint_idasresumeSessionId, while the fail-closed clear path still sends the normalized rawsessionId.clearAgentSurfaceResumeBindingshould receive or derive the same canonical checkpoint id used by publish, or the clear should accept it explicitly, so a canonical Codex resume binding is not left uncleared.🤖 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 27695 - 27736, The fail-closed clear paths in publishAgentSurfaceResumeBinding use the raw sessionId instead of the canonical resumeSessionId stored as checkpoint_id. Derive agentHookCanonicalResumeSessionId before clearing or update clearAgentSurfaceResumeBinding to accept the canonical checkpoint id, and pass that value in every relevant clear call so Codex bindings are consistently removed.
🤖 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.
Outside diff comments:
In `@CLI/cmux.swift`:
- Around line 27695-27736: The fail-closed clear paths in
publishAgentSurfaceResumeBinding use the raw sessionId instead of the canonical
resumeSessionId stored as checkpoint_id. Derive
agentHookCanonicalResumeSessionId before clearing or update
clearAgentSurfaceResumeBinding to accept the canonical checkpoint id, and pass
that value in every relevant clear call so Codex bindings are consistently
removed.
In `@CLI/CMUXCLI`+AgentHookRestoreEvidence.swift:
- Line 6: Remove the process-wide codexSessionResumeVerifier static singleton
and make CodexSessionResumeVerifier an injected or instance-scoped dependency
for the callers that use it. Update those callers and tests to supply or
construct the verifier explicitly, preserving cache isolation and controllable
lifetime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fc0cc1b1-c561-4b41-8bdd-bd8727e23ab3
📒 Files selected for processing (2)
CLI/CMUXCLI+AgentHookRestoreEvidence.swiftCLI/cmux.swift
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
lawrencecchen
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Workspace.swift (1)
930-944: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail closed for every unverified Codex hook binding.
This check runs only when
kindis exactly"codex"andcheckpointIdnormalizes. A kindless/whitespace-padded Codex hook binding—or a Codex binding with an empty checkpoint—falls through Line 943 and is restored unchanged without provider evidence. Reject such bindings unless a provider-verified Codex snapshot establishes the same session; add regressions for missingkindand missing checkpoint IDs.As per path instructions, correctness-critical identity must use a reliable structured source and fail closed when absent.
🤖 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 `@Sources/Workspace.swift` around lines 930 - 944, Update the resume-binding validation around codexResumeVerifierOwnsSession so every Codex agent-hook binding is rejected unless it has a normalized kind of codex, a nonempty normalized checkpoint ID, and provider verification for that same session. Ensure missing or whitespace-padded kind and missing or empty checkpoint IDs cannot reach the existing binding restoration path, and add regressions covering both missing kind and missing checkpoint ID cases.Source: Path instructions
🤖 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.
Outside diff comments:
In `@Sources/Workspace.swift`:
- Around line 930-944: Update the resume-binding validation around
codexResumeVerifierOwnsSession so every Codex agent-hook binding is rejected
unless it has a normalized kind of codex, a nonempty normalized checkpoint ID,
and provider verification for that same session. Ensure missing or
whitespace-padded kind and missing or empty checkpoint IDs cannot reach the
existing binding restoration path, and add regressions covering both missing
kind and missing checkpoint ID cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 47bbb571-6b43-4b9b-af6b-9c0d0063f572
📒 Files selected for processing (3)
Sources/DockSplitStore+SessionRestore.swiftSources/Workspace.swiftcmuxTests/SessionPersistenceResumeBindingTests.swift
| let codexResumeVerifier = CodexSessionResumeVerifier() | ||
| let restoreInputs = Self.verifiedSessionRestoreInputs( | ||
| binding: snapshot.terminal?.resumeBinding, | ||
| restorableAgent: snapshot.terminal?.agent, | ||
| codexResumeVerifier: codexResumeVerifier | ||
| ) |
There was a problem hiding this comment.
verifiedSessionRestoreInputs (via providerVerifiedRestorableAgentForSessionRestore → codexResumeEvidence → verifier.evidence()) calls sqlite3_open_v2 + SELECT id, rollout_path, thread_source FROM threads and reads up to 256 KB from the rollout JSONL file, all synchronously. createPanel(from:inPane:...) is a synchronous Workspace method accessed via main-actor-bound stored properties (agentSessionAutoResumeDefaults, surfaceResumeBindingIndex, remoteConfiguration, etc.), and the fresh CodexSessionResumeVerifier() created here has an empty CodexThreadIndexCache, so every panel restore with a Codex session opens a new SQLite connection on this path.
Before this PR, the same code path called only in-memory restorableAgentForSessionRestore/resumeBindingForSessionRestore with no disk I/O. The Codex verifier's SQLite reads already happen off-main inside RestorableAgentSessionIndex.load() via providerVerifiedHookRecord; moving them here duplicates them on the restore path. The fix is to pass the snapshot's already-verified sessionId (from RestorableAgentSessionIndex) through the persisted agent snapshot rather than re-verifying on restore, or to run verifiedSessionRestoreInputs in a detached task and await the result before mutating panel state.
Rule Used: Flag production Swift that reads, decodes, or scan... (source)
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!
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. |
| !codexResumeVerifierOwnsSession( | ||
| checkpointId, | ||
| environment: binding.environment, | ||
| verifier: codexResumeVerifier | ||
| ) { |
There was a problem hiding this comment.
state_5.sqlite) silently lose their stored binding on app restart. codexResumeVerifierOwnsSession is called without a transcriptPath, so for any session whose ID doesn't appear in the SQLite index the verifier falls through to the transcript check, finds nil, and returns false — discarding the binding. But at this call site the restorableAgent parameter is already the output of providerVerifiedRestorableAgentForSessionRestore, which sets transcriptPath from evidence.rolloutPath precisely so downstream callers can use it. Passing that path here lets the same legacy-rollout fallback that approved the agent also approve its binding, keeping auto-resume intact for older Codex installations.
| !codexResumeVerifierOwnsSession( | |
| checkpointId, | |
| environment: binding.environment, | |
| verifier: codexResumeVerifier | |
| ) { | |
| !codexResumeVerifierOwnsSession( | |
| checkpointId, | |
| environment: binding.environment, | |
| transcriptPath: restorableAgent?.transcriptPath, | |
| verifier: codexResumeVerifier | |
| ) { |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexSessionResumeVerifier.swift (1)
263-276: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject nested
source.execmetadata.Line 276 only detects
"exec"whensourceis a string. A structured source such as{"exec": {...}}bypasses this check, so an indexed thread markedusercan be restored. ReusesourceContainsKind("exec", value: source)and add a nested-source regression case.Proposed fix
let source = payload["source"] let sourceIsSubagent = sourceContainsKind("subagent", value: source) +let sourceIsExec = sourceContainsKind("exec", value: source) let sourceKind = normalized(source as? String)?.lowercased() let originator = normalized(payload["originator"] as? String)?.lowercased() ... - isExec: sourceKind == "exec" || originator == "codex_exec" + isExec: sourceIsExec || sourceKind == "exec" || originator == "codex_exec" )As per path instructions, correctness-critical session identity must fail closed when ownership evidence is not reliable.
🤖 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/CodexSessionResumeVerifier.swift` around lines 263 - 276, Update SessionMetadata construction in the metadata parsing flow to determine exec ownership with sourceContainsKind("exec", value: source), including structured nested source.exec metadata rather than only sourceKind string matching. Ensure indexed user threads with nested exec metadata are rejected, and add a regression case covering this nested-source scenario while preserving existing exec detection.Source: Path instructions
🤖 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.
Outside diff comments:
In
`@Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexSessionResumeVerifier.swift`:
- Around line 263-276: Update SessionMetadata construction in the metadata
parsing flow to determine exec ownership with sourceContainsKind("exec", value:
source), including structured nested source.exec metadata rather than only
sourceKind string matching. Ensure indexed user threads with nested exec
metadata are rejected, and add a regression case covering this nested-source
scenario while preserving existing exec detection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0affe49f-7403-4e52-b17a-d254e1e9edca
📒 Files selected for processing (2)
Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexSessionResumeVerifier.swiftPackages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/CodexSessionResumeVerifierTests.swift
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0b687f262
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| @Suite(.serialized) | ||
| struct CodexSessionResumeVerifierTests { | ||
| @Test func indexedThreadWithExistingRolloutIsResumable() throws { |
There was a problem hiding this comment.
This commit introduces both CodexSessionResumeVerifier and the tests intended to prove its regressions, so the tests never exist on the parent revision and CI cannot demonstrate that they fail without the fix. Put the failing tests in a test-only commit followed by the implementation commit, as required by the repository's regression-test policy.
AGENTS.md reference: AGENTS.md:L127-L134
Useful? React with 👍 / 👎.
| let prefix = handle.readData(ofLength: 256 * 1024) | ||
| guard let text = String(data: prefix, encoding: .utf8) else { return nil } |
There was a problem hiding this comment.
When an indexed rollout's first session_meta line exceeds 256 KiB, this read truncates the JSON (and can also split a UTF-8 scalar), so sessionMetadata returns nil. For a codex exec rollout whose database thread_source is user—the exact shape exercised by indexedExecThreadIsNotResumable—indexedEvidence then misses source == "exec"/originator == "codex_exec" and incorrectly accepts the worker as resumable. Read through the terminating newline with a bounded streaming parser, as the existing CodexRolloutIdentityResolver does, before treating absent metadata as safe.
Useful? React with 👍 / 👎.
| if let binding, | ||
| binding.isAgentHookBinding, | ||
| binding.kind.flatMap(RestorableAgentKind.init(rawValue:)) == .codex, |
There was a problem hiding this comment.
When restoring a legacy agent-hook binding decoded without kind—a supported format covered by legacyAgentHookBindingWithoutKindRewritesPersistedPATHManagedAgentExecutable—this condition skips provider verification entirely. If such a snapshot has no accompanying restorable-agent record, an unindexed review or worker UUID remains an automatically resumable binding and reaches startup as codex resume <unverified-id>, defeating the validation added here. Infer Codex from the legacy command/name or migrate the binding before applying this guard.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72885eaf98
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| environment: environment, | ||
| transcriptPath: transcriptPath, | ||
| verifier: verifier | ||
| )?.sessionId == sessionId |
There was a problem hiding this comment.
When a pre-change snapshot contains only a kindful Codex agent-hook binding for an indexed subagent, evidence successfully resolves the child ID to its interactive parent, but this equality treats that canonicalization as failure because the IDs differ. verifiedSessionRestoreInputs then removes the binding and has no restorable-agent fallback, leaving a plain shell despite the database supplying a safe resume target. Retarget the saved binding's checkpoint and command to evidence.sessionId, matching the live hook publication path.
AGENTS.md reference: AGENTS.md:L148-L148
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLI/CMUXCLI+AgentHookRestoreEvidence.swift (1)
22-42: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winStill resolves Codex home from ambient
ProcessInfo, not the caller-selected value.
agentHookCanonicalResumeSessionId(andagentHookProviderOwnsResumeTarget, which delegates to it) falls back toProcessInfo.processInfo.environment["CODEX_HOME"]/HOMEwhen the launch command doesn't carryCODEX_HOME. This was already flagged on a prior commit of this function: callers with an explicit/record-specific Codex home (e.g.cmux sessions --codex-home …) can end up verifying ownership against a differentstate_5.sqliteindex than the one actually associated with the session.♻️ Suggested direction (from prior review)
-func agentHookCanonicalResumeSessionId( +func agentHookCanonicalResumeSessionId( kind: String, sessionId: String, transcriptPath: String?, - launchCommand: AgentHookLaunchCommandRecord? + launchCommand: AgentHookLaunchCommandRecord?, + codexHomeOverride: String? = nil ) -> String? { guard let sessionId = normalizedHookValue(sessionId) else { return nil } guard kind == "codex" else { return sessionId } let environment = ProcessInfo.processInfo.environment let codexHome = normalizedHookValue(launchCommand?.environment?["CODEX_HOME"]) + ?? normalizedHookValue(codexHomeOverride) ?? normalizedHookValue(environment["CODEX_HOME"]) ...🤖 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`+AgentHookRestoreEvidence.swift around lines 22 - 42, Update agentHookCanonicalResumeSessionId and the delegating agentHookProviderOwnsResumeTarget flow to use the caller-selected Codex home consistently, rather than falling back to ambient ProcessInfo environment values. Thread the explicit/record-specific Codex home through the relevant launch or hook data and use it when constructing CodexSessionResumeVerifier, preserving the existing normalization and non-Codex behavior.Source: Path instructions
🤖 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.
Outside diff comments:
In `@CLI/CMUXCLI`+AgentHookRestoreEvidence.swift:
- Around line 22-42: Update agentHookCanonicalResumeSessionId and the delegating
agentHookProviderOwnsResumeTarget flow to use the caller-selected Codex home
consistently, rather than falling back to ambient ProcessInfo environment
values. Thread the explicit/record-specific Codex home through the relevant
launch or hook data and use it when constructing CodexSessionResumeVerifier,
preserving the existing normalization and non-Codex behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 43a44037-bab2-4b9c-bc7f-a1bc7e81d465
📒 Files selected for processing (3)
CLI/CMUXCLI+AgentHookRestoreEvidence.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/CodexSessionResumeVerifier.swiftPackages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/CodexSessionResumeVerifierTests.swift
lawrencecchen
added
the
stale-revisit
2 participants
Fixes #8890.
Also addresses the stale restore behavior reported in #6209.
Supersedes #4543.
Root cause
Codex hooks can report indexed and unindexed internal workers in addition to the user-owned root. An indexed child could replace the durable root identity, while truncated or nested metadata could hide an
execworker. Legacy kindless bindings could also skip verification. Snapshot reconciliation then erased the binding when a process scan temporarily found no agent. After resume, concurrent hook events could prefer the stored pre-resume PID over the wrapper's current PID, so a later snapshot saved the dead process generation and the second restart stopped.Fix
codex exec, automation, and subagent identities as restore roots.wasAgentRunningas the automatic-launch gate.Verification
swift test --package-path Packages/macOS/CMUXAgentLaunch: 249 tests in 36 suites passed../scripts/lint-pbxproj-test-wiring.sh: all 585 test files are wired.codex exec, then completed two Command-Q relaunch cycles on the final head. Both relaunches restored root019f9c3f-330e-72e2-9afd-2d45d45e5985, preserved its transcript, accepted fresh prompts, and leftwasAgentRunning: true.cgWindowNotFound.Dictionary: A binding associates a cmux surface with a durable agent session. A process generation distinguishes a live process from a later process that reused its PID.