Repository navigation
Preserve Pi sessions after workspace restore - #7628
austinywang merged 6 commits into
Conversation
When restored cmux workspaces receive fresh workspace IDs, Pi process detection can fall back to the newest session file in a shared cwd and collapse panels onto one session. Preserve same-kind hook-store identity by stable panel ID for inferred latest-file detections, and add a regression test for workspace ID rotation.
|
Someone is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
To use Codex here, create a Codex account and connect to github. |
|
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:
📝 WalkthroughWalkthroughRestorableAgentSessionIndex.load now tracks stable panelId+kind hook candidates and uses them to reconcile inferred-latest sessions when workspace-panel matching is unavailable or ambiguous. Pi tests cover workspace-id rotation and ambiguous panel-id cases. ChangesStable Panel-Based Hook Resolution
Estimated code review effort: 4 (Complex) | ~40 minutes Sequence Diagram(s)sequenceDiagram
participant RestorableAgentSessionIndex
participant detectedSnapshots
participant SessionStore
RestorableAgentSessionIndex->>RestorableAgentSessionIndex: load hook candidates by panelId+kind
RestorableAgentSessionIndex->>RestorableAgentSessionIndex: resolve workspace+panel or stable panelId+kind candidate
RestorableAgentSessionIndex->>detectedSnapshots: merge live process evidence
RestorableAgentSessionIndex->>SessionStore: restore per-panel session entry
Possibly related PRs
🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 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 SummaryThis PR preserves Pi sessions after workspace restore. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (6): 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}
| let sameKindStablePanelCandidate = sameKindPanelCandidate ?? hookCandidatesByPanelIdAndKind[ | ||
| PanelIDKindKey(panelId: key.panelId, kind: detected.snapshot.kind) | ||
| ] |
There was a problem hiding this comment.
When the hook store contains more than one Pi record with the same stable panelId and kind across different old workspaces, this fallback drops the workspace from the lookup and lets shouldReplaceHookEntry choose a single winner. A restored process whose exact workspace key misses can then receive another workspace's hook snapshot while keeping its own live process IDs, so the UI can show or resume the wrong Pi session for that panel.
Rule Used: Flag correctness-critical detection/identity deriv... (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 `@cmuxTests/RestorableAgentSessionIndexTests.swift`:
- Around line 778-789: The test in RestorableAgentSessionIndexTests is too weak
because a single detected session can satisfy detectedSessionIds.count == 1 even
if only one panel was detected, allowing snapshot(workspaceId:panelId:) to fall
back by panelId without proving full reconciliation. Strengthen the test by
asserting that every restored workspace/panel pair in restoredWorkspaceIds and
panels is present in detectedSnapshots before checking restored session IDs,
using the existing RestorableAgentSessionIndex.load and
snapshot(workspaceId:panelId:) flow to validate all restored panels were
actually detected.
🪄 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: c3fea1cf-8abb-481c-851f-9e1b5e4b7bf1
📒 Files selected for processing (2)
Sources/RestorableAgentSession.swiftcmuxTests/RestorableAgentSessionIndexTests.swift
Only use the panel-id restore fallback when a panel/kind maps back to one old workspace key. Add a regression test that keeps latest-file detection when stale hook records reuse the same panel ID across old workspaces.
|
Addressed the panel-only collision concern in f717de4. The panel-id fallback now tracks whether a |
Harden the workspace-rotation regression so it first verifies process detection produced snapshots for every restored workspace/panel key before asserting latest-file collapse and restoration behavior.
| hookCandidatesByPanelIdAndKind[panelIDKindKey] = PanelIDKindCandidate( | ||
| panelKey: shouldReplace ? key : existingPanelIDCandidate.panelKey, | ||
| entry: shouldReplace ? entry : existingPanelIDCandidate.entry, | ||
| isAmbiguous: existingPanelIDCandidate.isAmbiguous || existingPanelIDCandidate.panelKey != key |
There was a problem hiding this comment.
This ambiguity check only flips when another hook record has a different PanelKey. A stale hook store can still contain two Pi sessions for the same old workspace, panel, and kind after the panel was reused or relaunched. In that case isAmbiguous stays false, shouldReplaceHookEntry picks one record by recency, and a restored process whose new workspace id misses the exact key can inherit the wrong hook-store session while keeping its live process IDs. Please treat a second distinct hook entry for the same (panelId, kind) as ambiguous, not only a second old workspace key.
Rule Used: Flag correctness-critical detection/identity deriv... (source)
There was a problem hiding this comment.
Addressed in ceb8389. The ambiguity check now also treats a second distinct hook session for the same (panelId, kind) as ambiguous, even when it has the same old workspace/panel key. I expanded testPiDetectedLatestSessionDoesNotUsePanelOnlyFallbackWhenPanelIdIsAmbiguous to cover both same-key stale sessions and different-old-workspace stale sessions.
Harden the panel-id restore fallback so a second distinct hook session for the same panel/kind is ambiguous even when it uses the same old workspace key. Expand the ambiguity regression to cover both same-key and different-workspace stale hook records.
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/RestorableAgentSession.swift (1)
1221-1246: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winComment undersells the ambiguity criteria — mention the same-workspace hardening too.
The comment says panel-only restore is unsafe only when the surface "maps back to one old workspace," but the actual check at Line 1193 also marks the candidate ambiguous when two hook records share the same old
(workspaceId, panelId)but have differentsessionIds (per the PR's own hardening commit). A maintainer reading only this comment could "simplify" the ambiguity check to just the panelKey comparison and silently reintroduce the arbitrary-snapshot-pick bug this PR just closed.📝 Suggested comment clarification
- // Panel-only restore is safe only when this surface/kind maps back to one old workspace. - // Stale hook stores can otherwise reuse a surface id and make the fallback ambiguous. + // Panel-only restore is safe only when this surface/kind maps back to exactly one + // (workspace, session) pair. Both a stale hook store reusing a surface id across old + // workspaces, and a single old workspace recording more than one distinct session for + // this surface (e.g. the agent restarted before that workspace was ever rotated), must + // make the fallback ambiguous — otherwise shouldReplaceHookEntry would pick one session + // arbitrarily.🤖 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 1221 - 1246, Clarify the ambiguity guard around the panel-only restore path in RestorableAgentSession so it also reflects the same-workspace hardening, not just the “one old workspace” case. Update the nearby comment and any related logic around sameKindStablePanelCandidate / matchingHookEntry to state that a candidate is ambiguous when multiple hook records share the same old workspaceId and panelId but differ by sessionId, so the restore path must not collapse this to a simple panelKey check. Keep the explanation aligned with the existing restore flow symbols like detected.sessionIDSource, sameKindPanelIDCandidate, and sameKindStablePanelCandidate.
🤖 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/RestorableAgentSession.swift`:
- Around line 1221-1246: Clarify the ambiguity guard around the panel-only
restore path in RestorableAgentSession so it also reflects the same-workspace
hardening, not just the “one old workspace” case. Update the nearby comment and
any related logic around sameKindStablePanelCandidate / matchingHookEntry to
state that a candidate is ambiguous when multiple hook records share the same
old workspaceId and panelId but differ by sessionId, so the restore path must
not collapse this to a simple panelKey check. Keep the explanation aligned with
the existing restore flow symbols like detected.sessionIDSource,
sameKindPanelIDCandidate, and sameKindStablePanelCandidate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a04b53c1-8b85-4b51-b82b-f7dc40f51709
📒 Files selected for processing (2)
Sources/RestorableAgentSession.swiftcmuxTests/RestorableAgentSessionIndexTests.swift
Update the restore-index comment so it documents both ambiguity cases: panel reuse across old workspaces and multiple stale sessions for the same old workspace/panel.
…ore-collapse # Conflicts: # Sources/RestorableAgentSession.swift
* Preserve Pi sessions after workspace restore When restored cmux workspaces receive fresh workspace IDs, Pi process detection can fall back to the newest session file in a shared cwd and collapse panels onto one session. Preserve same-kind hook-store identity by stable panel ID for inferred latest-file detections, and add a regression test for workspace ID rotation. * Guard Pi panel fallback against ambiguous hook records Only use the panel-id restore fallback when a panel/kind maps back to one old workspace key. Add a regression test that keeps latest-file detection when stale hook records reuse the same panel ID across old workspaces. * Assert all restored Pi panels are detected Harden the workspace-rotation regression so it first verifies process detection produced snapshots for every restored workspace/panel key before asserting latest-file collapse and restoration behavior. * Treat same-panel distinct Pi hooks as ambiguous Harden the panel-id restore fallback so a second distinct hook session for the same panel/kind is ambiguous even when it uses the same old workspace key. Expand the ambiguity regression to cover both same-key and different-workspace stale hook records. * Clarify Pi panel fallback ambiguity guard Update the restore-index comment so it documents both ambiguity cases: panel reuse across old workspaces and multiple stale sessions for the same old workspace/panel. --------- Co-authored-by: Silouan Wright <silouan@example.com> (cherry picked from commit b041531)
2 participants
Closes #7752.
Follow-up to #5607 / #5534.
Summary
Root cause
#5607 preserves exact per-panel Pi hook records when process detection can only infer the newest JSONL session in a shared cwd. That exact guard depends on the hook record and process snapshot sharing the same
(workspaceId, panelId).On session restore, cmux can recreate workspaces with fresh workspace UUIDs while preserving the terminal surface/panel ID. In that case, the exact
(workspaceId, panelId)lookup misses the old hook record, and Pi process detection can again collapse same-cwd panels to the newest JSONL session file.This patch keeps the exact-key path first, then uses a same-kind panel-ID fallback only when that panel/kind maps to one old workspace/session pair. If stale hook records make the panel-only fallback ambiguous, cmux leaves the inferred process-detected session alone instead of choosing an arbitrary hook snapshot.
Verification
git diff --checkxcodebuild build-for-testingwith external DerivedData/SourcePackages/TMP andCMUX_SKIP_ZIG_BUILD=1.Demo Video
N/A — unit-test-only session restore indexing fix.
Review Trigger (Copy/Paste as PR comment)
Checklist
Summary by CodeRabbit
Note
Medium Risk
Changes session-index merge logic for Pi and other agents using latest-file inference; wrong ambiguity handling could mis-attribute sessions, but scope is limited to restore indexing with new regression coverage.
Overview
Fixes Pi session restore when cmux recreates workspaces with new UUIDs but keeps the same panel/surface IDs, so ambiguous “newest JSONL in shared cwd” process detection no longer overwrites per-panel hook identities.
RestorableAgentSessionIndex.loadnow indexes hook records by(panelId, agent kind)and marks candidates ambiguous when one panel ID maps to multiple workspace/session pairs (stale stores, restarts, cross-workspace reuse). ForinferredLatestSessionFiledetections, it still prefers exact(workspaceId, panelId)hook matches, then falls back to a stable panel-ID hook entry only when unambiguous, merging in live process IDs without replacing the session id. If the panel-only map is ambiguous, it does not pick an arbitrary hook snapshot and leaves the inferred detected session.Adds regression tests for workspace-ID rotation (three panels keep distinct Pi session ids) and for ambiguous panel-ID hook history (restored snapshot stays on the detected newest session).
Reviewed by Cursor Bugbot for commit ed18399. Bugbot is set up for automated code reviews on this repo. Configure here.