Fix subagent session restore takeover - #4543
lawrencecchen wants to merge 47 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughSession lifecycle recording now tracks parent-child relationships via optional ChangesParent Session Tracking and Subagent Suppression
Sequence Diagram(s)sequenceDiagram
participant HookPayload
participant CLI_cmux as CLI/cmux
participant HookStore
participant RestorableIndex as RestorableAgentSessionIndex
HookPayload->>CLI_cmux: extract sessionId and parentSessionId
CLI_cmux->>HookStore: recordPromptSubmit/upsert(parentSessionId, isRestorable)
HookStore->>RestorableIndex: persist snapshot with parentSessionId
RestorableIndex->>HookStore: preferredSnapshotCandidate? (compare updatedAt)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (14 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 prevents Claude and generic agent subagent sessions from taking over panel restore by persisting
Confidence Score: 3/5The core restore-index guard for Claude subagents has a gap that allows a pre-stop subagent record to appear restorable if its transcript exists on disk, and the sticky-true protection was narrowed in a way that opens a demotion path for non-subagent sessions with a false-positive parent tag; both affect the fundamental restore invariant this PR is trying to enforce. Two distinct correctness gaps exist in the changed paths that handle the central invariant of the PR. The hookRecordIsRestorable guard for Claude only fires when isRestorable == false, but session-start never writes that flag — only stop/prompt-submit do — so the filtering is silent for the pre-stop window. The update() guard change removes sticky-true protection for any session that acquires a parentSessionId tag, and the tag-extraction logic searches broad nested keys, creating a non-trivial false-positive surface. Either defect can cause the parent restore binding to be lost or overwritten in production. Sources/RestorableAgentSession.swift (hookRecordIsRestorable Claude path) and CLI/cmux.swift (update() isRestorable guard and session-start upsert) need the closest review. Important Files Changed
Sequence DiagramsequenceDiagram
participant Hook as Claude/Agent Hook
participant Store as SessionStore
participant Repair as RepairBinding
participant TC as TerminalController
participant Index as RestoreIndex
Hook->>Store: "session-start (parentSessionId, isRestorable=nil)"
Hook->>Repair: repairClaudeSubagentResumeBinding
Repair->>TC: "surface.resume.set(parent, expected_checkpoint_id=child)"
TC-->>Repair: ok (guard passed)
Note over Store,Index: Pre-stop window: isRestorable=nil, parentSessionId set
Index->>Store: hookRecordIsRestorable?
Store-->>Index: "isRestorable==false? No, transcript check returns true"
Hook->>Store: "stop (suppressClaudeSubagentRestore, isRestorable=false, parentSessionId)"
Hook->>TC: "surface.resume.set(parent, expected_checkpoint_id=child)"
TC-->>Hook: ok (guard passed)
Index->>Store: hookRecordIsRestorable?
Store-->>Index: "isRestorable==false + parentSessionId, returns false"
Hook->>Store: "session-end (mapped.isRestorable==false, upsert)"
Hook->>Repair: repairSuppressedSubagentResumeBinding
Repair->>TC: "surface.resume.set(parent, expected_checkpoint_id=child)"
TC-->>Repair: ok
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if let normalizedParent, | ||
| let parent = peers.first(where: { $0.sessionId == normalizedParent }) { | ||
| return parent | ||
| } | ||
| return peers | ||
| .filter { !requireLaunchCommand || recordHasLaunchCommand($0) } | ||
| .max(by: { $0.updatedAt < $1.updatedAt }) |
There was a problem hiding this comment.
When normalizedParent is set (i.e., parentSessionId was supplied by the caller) but the parent record is not among peers (e.g., the parent session was already consumed/removed from the store), the function falls through to the generic .max(by:) fallback and returns any restorable peer on the same workspace+surface. shouldSuppressSubagentRestoreTakeover then treats the non-nil result as "there is a peer that should take precedence" and marks the child isRestorable: false. If an old unrelated session happens to remain in the store for that workspace+surface, the actively running child session is silently locked out of restore — permanently, since isRestorable: false is never reset. The fallback should return nil when normalizedParent is provided but the parent is not found, so only an explicitly matched parent triggers suppression in the first-check path.
| return peers | ||
| .filter { !requireLaunchCommand || recordHasLaunchCommand($0) } | ||
| .max(by: { $0.updatedAt < $1.updatedAt }) |
There was a problem hiding this comment.
sameSurfaceRestorablePeer
The outer peers collection is already filtered by requireLaunchCommand (the guard inside the closure returns recordHasLaunchCommand(record) when requireLaunchCommand is true), so the secondary .filter { !requireLaunchCommand || recordHasLaunchCommand($0) } is dead code for that branch. It adds noise and may mislead future readers into thinking the initial filter was insufficient.
| return peers | |
| .filter { !requireLaunchCommand || recordHasLaunchCommand($0) } | |
| .max(by: { $0.updatedAt < $1.updatedAt }) | |
| return peers | |
| .max(by: { $0.updatedAt < $1.updatedAt }) |
| private func firstStringRecursively( | ||
| in value: Any, | ||
| keys: Set<String>, | ||
| maxDepth: Int | ||
| ) -> String? { | ||
| guard maxDepth >= 0 else { return nil } |
There was a problem hiding this comment.
firstStringRecursively traverses one extra level beyond maxDepth
The guard maxDepth >= 0 passes when maxDepth is 0, so the function visits the current node at depth 0 and then recurses with maxDepth - 1 == -1 (which immediately fails). A call with maxDepth: 4 actually traverses 5 levels (4, 3, 2, 1, 0), not 4. Use maxDepth > 0 as the recursion guard so the parameter semantics match its name.
| private func firstStringRecursively( | |
| in value: Any, | |
| keys: Set<String>, | |
| maxDepth: Int | |
| ) -> String? { | |
| guard maxDepth >= 0 else { return nil } | |
| private func firstStringRecursively( | |
| in value: Any, | |
| keys: Set<String>, | |
| maxDepth: Int | |
| ) -> String? { | |
| guard maxDepth > 0 else { return nil } |
| func shouldSuppressSubagentRestoreTakeover( | ||
| workspaceId: String, | ||
| surfaceId: String, | ||
| launchCommand: AgentHookLaunchCommandRecord? | ||
| ) -> Bool { | ||
| guard !sessionId.isEmpty else { return false } | ||
| if let parentSessionId = input.parentSessionId, | ||
| (try? store.sameSurfaceRestorablePeer( | ||
| workspaceId: workspaceId, | ||
| surfaceId: surfaceId, | ||
| excludingSessionId: sessionId, | ||
| parentSessionId: parentSessionId | ||
| )) != nil { | ||
| return true | ||
| } | ||
| guard launchCommand == nil else { | ||
| return false | ||
| } | ||
| return (try? store.sameSurfaceRestorablePeer( | ||
| workspaceId: workspaceId, | ||
| surfaceId: surfaceId, | ||
| excludingSessionId: sessionId, | ||
| requireLaunchCommand: true | ||
| )) != nil |
There was a problem hiding this comment.
shouldSuppressSubagentRestoreTakeover reads the hook session store to decide whether the current session is a subagent. The result is correct only when the parent's session record is already written to the store before the child's hook fires. If the parent's session-start hook is delayed and the child fires first, sameSurfaceRestorablePeer finds no parent record, and the child may or may not be suppressed depending on unrelated store content. The invariant "child sessions are always subordinate to their parent" is not enforced by a signal from the owning subsystem but by a time-of-read snapshot. Treating parentSessionId being non-nil in the hook payload as sufficient to mark non-restorable would eliminate this ordering dependency entirely.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
![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 901-903: The return chain redundantly re-filters peers with
recordHasLaunchCommand even though peers were already filtered when
requireLaunchCommand was true; remove the conditional filter from the return
(the `.filter { !requireLaunchCommand || recordHasLaunchCommand($0) }`) and
simply call `.max(by: { $0.updatedAt < $1.updatedAt })` on the already-prepared
peers so the `max(by:)` uses the correct set without double-filtering; reference
symbols: peers, requireLaunchCommand, recordHasLaunchCommand, and max(by:).
🪄 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: 83ce2817-0ba3-4801-be9b-991aefc28aee
📒 Files selected for processing (4)
CLI/cmux.swiftSources/RestorableAgentSession.swiftcmuxTests/CLINotifyProcessIntegrationRegressionTests.swiftcmuxTests/RestorableAgentSessionIndexTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| let savedSubagentSuppression = mapped?.isRestorable == false | ||
| let suppressRestorableRecord = nestedAgentSuppressVisibleMutations || suppressRestoreTakeover | ||
| || savedSubagentSuppression | ||
| let suppressVisibleMutations = suppressRestorableRecord | ||
| if !sessionId.isEmpty { | ||
| try? store.upsert( | ||
| sessionId: sessionId, | ||
| parentSessionId: input.parentSessionId, | ||
| workspaceId: workspaceId, | ||
| surfaceId: surfaceId, | ||
| cwd: hookCwd ?? mapped?.cwd, | ||
| transcriptPath: input.transcriptPath ?? mapped?.transcriptPath, | ||
| pid: pid, | ||
| launchCommand: launchCommand, | ||
| isRestorable: suppressRestorableRecord ? false : nil, | ||
| runtimeStatus: suppressVisibleMutations ? nil : .running, | ||
| updateRuntimeStatus: !suppressVisibleMutations |
There was a problem hiding this comment.
nestedAgentSuppressVisibleMutations now permanently locks isRestorable: false via savedSubagentSuppression
When shouldSuppressNestedAgentVisibleMutations fires at session-start (e.g., PID-based heuristic detects a parent agent process), isRestorable: false is written to the store. On every subsequent hook call, savedSubagentSuppression = mapped?.isRestorable == false evaluates true and re-asserts suppression — even if shouldSuppressNestedAgentVisibleMutations would now return false (e.g., the parent process has since exited). Before this PR, suppression was re-evaluated on each hook call; now a single positive evaluation makes suppression permanent.
The concrete regression: a session that starts while a parent agent process is alive (PID heuristic fires → isRestorable: false stored), but the parent exits before the session's stop hook fires, will never publish a resume binding. The user's session becomes permanently non-restorable with no escape hatch short of manually editing the session store file.
The suppressRestoreTakeover path (payload-based) is correctly sticky because a parentSessionId in the payload is a structural signal. The nestedAgentSuppressVisibleMutations path is a heuristic, and its prior per-call re-evaluation was load-bearing for recovery.
There was a problem hiding this comment.
Verified against current head. This path is fixed by 723091c: transient nested-agent suppression no longer writes isRestorable=false, so savedSubagentSuppression is not made sticky by the PID/process-tree heuristic. The new focused regression is testTransientSessionEndSuppressionDoesNotClearRuntimeStatus.
— Claude Code
| record.launchCommand = launchCommand | ||
| } | ||
| if let isRestorable { |
There was a problem hiding this comment.
The old update() body contained an explicit invariant guard that protected isRestorable=true (set by transcript-backed events at session completion) from being overwritten by isRestorable=false (set by late non-promoting SessionStarts). The Claude session handler (~line 18051) passes isRestorable: false for "Non-clear SessionStart that can arrive late from startup/resume/compact after /clear". If the same session ID was already stored as isRestorable: true after a completed turn (lines 18165/18259), the new assignment record.isRestorable = isRestorable overwrites true with false. Because savedSubagentSuppression = mapped?.isRestorable == false re-confirms suppression on every subsequent hook event, the session becomes permanently non-restorable with no escape hatch. The subagent suppression path this PR introduces is gated solely on parentSessionId being present in the hook payload — the Claude handler never sets that field — so the sticky guard could be restored without breaking the new subagent logic.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1b4ea0f. Configure here.
| surfaceId: surfaceId, | ||
| sessionId: sessionId | ||
| ) | ||
| } |
There was a problem hiding this comment.
Duplicated ancestor traversal repair logic across two handlers
Low Severity
repairClaudeSubagentResumeBinding and repairSuppressedSubagentResumeBinding contain nearly identical ancestor-chain traversal logic (walking up to 8 parent hops, checking surface match and restorability, publishing with expected-checkpoint guard, and falling back to clear). The only differences are the store variable name, how sessionId/parentSessionId are obtained, and the kind/displayName used for publishing. A shared helper accepting those parameters would eliminate this duplication and reduce the risk of the two copies diverging.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1b4ea0f. Configure here.
There was a problem hiding this comment.
Verified against the current code. I am leaving the two repair paths separate for this PR because they sit in different hook owners with different store/display inputs, and collapsing them would be a behavior-neutral refactor inside a restore regression fix. The AWS focused test set covers both paths.
— Claude Code
|
Superseded by #8321, which is updated to current main and handles both unindexed Codex worker IDs and indexed subagents that must resolve to their writable parent. |
This branch was successfully deployed
1 participant
Summary
Tests
subrestore-claude-parent-red-gui-1779495275failed before the fix because the child record did not persistparentSessionId.subrestore-claude-prompt-red-gui-1779496531failed before the prompt-stop fix because the child prompt stayed restorable and emitted a child resume binding.subrestore-merge-green-1779512889oncmux-aws-m4propassed the 19 focusedcmux-unitrestore and hook regression tests.