Preserve restored agent resume bindings - #8885
azooz2003-bit wants to merge 5 commits into
Conversation
📝 WalkthroughWalkthroughAuto-resume eligibility now considers restored agent state and trusted checkpoint bindings through a shared Workspace helper. Prompt-idle cleanup retains durable agent-hook bindings until liveness pruning, with expanded restore and lifecycle regression tests. ChangesAgent resume lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SessionSnapshot
participant Workspace
participant BindingSnapshot
participant TerminalRestore
SessionSnapshot->>Workspace: evaluate restored agent state and binding
Workspace->>BindingSnapshot: validate checkpoint identity
BindingSnapshot-->>Workspace: return binding eligibility
Workspace-->>SessionSnapshot: return auto-resume decision
TerminalRestore->>Workspace: apply shared auto-resume policy
Workspace-->>TerminalRestore: restore or skip resume command
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (22 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 a bug where
Confidence Score: 5/5Safe to merge. The change is a targeted three-line removal in updateBindingOnlyRestoredAgentResumeState, a pure nonisolated helper extraction, and a new computed property; all pruning still executes via reconcileSurfaceResumeBindings on the liveness-scan path. The fix correctly narrows the eager clear to only happen via the liveness scan, which has the live-process check the eagerly removed code was bypassing. The shouldAutoResumeRestoredAgent helper is pure and nonisolated, consolidating previously duplicated inline logic across three call sites. All three are updated consistently. The regression tests exercise the full preserve-then-prune lifecycle and the same-surface/same-checkpoint liveness matching, covering the key invariants. No actor isolation issues, no ambient global state, no test seams added to production source. Files Needing Attention: No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "Allow trusted hook checkpoints past stal..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| surfaceResumeBindingIndex: .empty | ||
| ) | ||
| #expect(prunedSnapshot.panels.first?.terminal?.resumeBinding == nil) | ||
| #expect(restored.sessionSnapshot(includeScrollback: false).panels.first?.terminal?.resumeBinding == nil) |
There was a problem hiding this comment.
sessionSnapshot(includeScrollback: false) (no explicit indexes) does NOT call reconcileSurfaceResumeBindings, so it cannot prune the binding on its own. This == nil assertion only holds because the prunedSnapshot call at the preceding lines already invoked reconcileSurfaceResumeBindings(using: .empty) as a side effect and removed the binding from surfaceResumeBindingsByPanelId. If someone later reorders or removes the prunedSnapshot block, this line will silently flip from documenting the correct new behavior back to testing the old (now-deleted) eager-clear path — and it would pass for the wrong reason until a real scenario exercises the difference. A short comment above the assertion (e.g. // binding removed from workspace state by liveness-scan side effect above) would make the dependency explicit.
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!
| autoResume: Bool? = nil | ||
| ) -> SurfaceResumeBindingSnapshot { | ||
| SurfaceResumeBindingSnapshot( | ||
| command: "claude --resume session-1", |
There was a problem hiding this comment.
kind addition silently fixes existing tests
Adding kind: "claude" to the factory changes the behavior of the two pre-existing tests. isStaleAgentHookBinding guards on binding.kind being non-nil and non-empty; without kind, the guard exits false regardless of liveness, so localAgentHookBindingWithNoLiveProcessIsStale would have been returning the wrong value before this change. The fix is correct, but it would be worth a brief note in the commit or test comment that the kind field is required for isStaleAgentHookBinding to reach the liveness check — this prevents a future refactor of the factory from silently re-introducing the gap.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/AgentSessionAutoResumeSettingsTests.swift`:
- Around line 387-390: The policy tests should stop inspecting generated
launcher script text and instead verify observable binding-policy outcomes and
restored state. In cmuxTests/AgentSessionAutoResumeSettingsTests.swift lines
387-390, replace the scriptContains assertion with binding-policy and
restored-state assertions; at lines 442-445, assert the unknown-state policy
outcome. Keep launcher shell behavior assertions exclusively in
AgentResumeReturnShellStartupTests.swift.
In `@Sources/DockSplitStore`+SessionSnapshot.swift:
- Around line 136-142: Before the autoResumeAgentSessions eligibility call in
the Dock snapshot restore flow, scan the supplied indexes for a live matching
agent and clear effectiveSessionResumeBinding when none exists, matching
Workspace’s stale-binding pruning behavior. Ensure the override cannot convert a
stale local binding into startup work, and add a Dock snapshot regression
covering empty indexes and a dead checkpoint.
🪄 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: 4cd320d9-c2c5-41f3-88d4-73cf26759161
📒 Files selected for processing (6)
Sources/DockSplitStore+SessionRestore.swiftSources/DockSplitStore+SessionSnapshot.swiftSources/SessionPersistence.swiftSources/Workspace.swiftcmuxTests/AgentSessionAutoResumeSettingsTests.swiftcmuxTests/WorkspaceIsStaleAgentHookBindingTests.swift
| try assertAgentAutoResumeUsesStartupCommand( | ||
| restoredPanel, | ||
| scriptContains: ["codex resume codex-exited-binding-session"] | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Keep launcher-script assertions in the authoritative return-shell suite.
These policy tests now inspect generated script text, coupling them to launcher implementation details.
cmuxTests/AgentSessionAutoResumeSettingsTests.swift#L387-L390: assert the binding-policy outcome and restored state rather than command-script contents.cmuxTests/AgentSessionAutoResumeSettingsTests.swift#L442-L445: assert the unknown-state policy outcome; keep shell behavior inAgentResumeReturnShellStartupTests.
Based on learnings, AgentResumeReturnShellStartupTests.swift is the authoritative auto-resumed return-shell contract; prefer observable shell behavior over brittle launcher assertions here.
📍 Affects 1 file
cmuxTests/AgentSessionAutoResumeSettingsTests.swift#L387-L390(this comment)cmuxTests/AgentSessionAutoResumeSettingsTests.swift#L442-L445
🤖 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/AgentSessionAutoResumeSettingsTests.swift` around lines 387 - 390,
The policy tests should stop inspecting generated launcher script text and
instead verify observable binding-policy outcomes and restored state. In
cmuxTests/AgentSessionAutoResumeSettingsTests.swift lines 387-390, replace the
scriptContains assertion with binding-policy and restored-state assertions; at
lines 442-445, assert the unknown-state policy outcome. Keep launcher shell
behavior assertions exclusively in AgentResumeReturnShellStartupTests.swift.
Source: Learnings
| autoResumeAgentSessions: Workspace.shouldAutoResumeRestoredAgent( | ||
| autoResumeAgentSessions: AgentSessionAutoResumeSettings.isEnabled( | ||
| defaults: agentSessionAutoResumeDefaults | ||
| ), | ||
| wasAgentRunning: agentWasRunning, | ||
| resumeBinding: resumeBinding | ||
| ), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prune stale local agent-hook bindings before granting this override.
When the supplied indexes contain no matching live agent, sessionAgentWasRunning becomes false, but effectiveSessionResumeBinding still retains the stored binding. This new call then turns that stale local binding into startup work, so a later Dock restore resumes a dead checkpoint. Apply the same scan-backed stale-binding pruning as Workspace before this eligibility check, and add a Dock snapshot regression using empty indexes.
As per coding guidelines, persistence paths must handle stale cached state and correctness-critical state must use a reliable source of truth.
🤖 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/DockSplitStore`+SessionSnapshot.swift around lines 136 - 142, Before
the autoResumeAgentSessions eligibility call in the Dock snapshot restore flow,
scan the supplied indexes for a live matching agent and clear
effectiveSessionResumeBinding when none exists, matching Workspace’s
stale-binding pruning behavior. Ensure the override cannot convert a stale local
binding into startup work, and add a Dock snapshot regression covering empty
indexes and a dead checkpoint.
Source: Coding guidelines
lawrencecchen
added
the
stale-revisit
3 participants
Summary
agent-hookresume bindings when prompt-idle reconciliation clears transient restored-agent stateVerification
./scripts/lint-pbxproj-test-wiring.shxcodebuild test -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination platform=macOS -derivedDataPath ~/Library/Developer/Xcode/DerivedData/cmux-rsbind -only-testing:cmuxTests/WorkspaceIsStaleAgentHookBindingTestsblocked before executing tests by unrelated sidebar test compile errors already present in the test target:SidebarWorkspaceRowRetirementTests.swift,SidebarWorkspaceRowSuspensionTests.swift,SidebarWorkspaceTableSuspensionTests.swift. Result bundle:~/Library/Developer/Xcode/DerivedData/cmux-rsbind/Logs/Test/Test-cmux-unit-2026.07.24_14-52-15--0700.xcresult./scripts/reload-cloud.sh --tag rsbindsucceeded, run https://github.com/manaflow-ai/cmux/actions/runs/30130017800cmux DEV rsbind.app, verifiedidentifytargeted/tmp/cmux-debug-rsbind.sock, setsurface.resume.setvia raw RPC withsource=agent-hookandauto_resume=true, confirmed immediate binding, then confirmed fake checkpoint with no matching live process was pruned after the liveness scanNotes
No user-facing strings changed; localization audit: source/test-only behavior change, no UI strings, menus, settings, docs, or command help modified.
Autoreview was not invoked.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Preserve binding-only
agent-hookresume bindings after prompt-idle so auto-resume survives restore, and allow trusted hook checkpoints to auto-resume even if the snapshot marked the agent as exited or unknown; stale bindings are still pruned by liveness. Adds regression coverage.agent-hookresume bindings when a restored panel enters prompt-idle.agent-hookcheckpoints (valid kind + checkpoint withautoResume=true) even whenwasAgentRunningis false or unknown.Written for commit 737e2f2. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests