Extract workspace session restore policy service - #6146
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
💤 Files with no reviewable changes (6)
📝 WalkthroughWalkthroughExtracts all session-restore policy decisions from static helpers in ChangesSession Restore Policy Service
ComposerDictation Instance Refactoring
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
note over Workspace: Session Restore Approval Flow
participant Workspace
participant WorkspaceSessionRestorePolicyService as Service
participant applyStoredApproval
participant shouldRunPromptedSurfaceResume
end
Workspace->>Service: approvedSurfaceResumeBinding(binding, autoResumeAgentSessions)
Service->>applyStoredApproval: apply stored approval
applyStoredApproval-->>Service: updated Binding
Service->>Service: rewrite Hermes agent command (provider, bootstrap)
Service->>shouldRunPromptedSurfaceResume: check prompt gate
shouldRunPromptedSurfaceResume-->>Service: Bool (approved/denied)
Service-->>Workspace: Binding? (approved or nil)
alt Approved Binding
Workspace->>Service: surfaceResumeStartupLaunch(forApprovedBinding:)
Service-->>Workspace: WorkspaceSurfaceResumeStartupLaunch (.command or .input)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Greptile SummaryThis PR extracts the workspace session restore policy cluster from
Confidence Score: 5/5Safe to merge; the extraction is behavior-preserving and all moved code is covered by 19 new package-level tests. Every policy decision (approval storage, prompt gating, Hermes bootstrap rewrite, remote reconnect, scrollback replay) was moved verbatim into WorkspaceSessionRestorePolicyService with injected seams and a full test suite. The Workspace.swift wrappers delegate directly to the new service using the same arguments; no conditional logic was altered. The two observations are dead code and a file-naming mismatch — neither affects runtime behavior. No files require special attention; WorkspaceHermesAgentCommandBootstrapper.swift has a minor dead-code note but is otherwise a faithful port. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Workspace / static shim] -->|makeSessionRestorePolicyService| B[WorkspaceSessionRestorePolicyService]
A2[Workspace instance] -->|sessionRestorePolicy| B
B --> C{approvedSurfaceResumeBinding}
C --> D[applyStoredApproval closure]
D --> E[WorkspaceHermesAgentCommandBootstrapper]
E --> F{source == agent-hook AND autoResumeAgentSessions?}
F -->|blocked| G[nil]
F -->|allowed| H{requiresPromptApproval?}
H -->|yes| I[shouldRunPromptedSurfaceResume closure]
I -->|denied| G
I -->|approved| J[Approved Binding]
H -->|no| K{allowsAutomaticResume?}
K -->|no| G
K -->|yes| J
B --> L{surfaceResumeStartupLaunch}
J --> L
L -->|isAgentHookBinding| M[.command]
L -->|otherwise| N[.input]
B --> O[shouldAutoConnectRestoredRemote]
B --> P[resolvedSnapshotTerminalScrollback]
B --> Q[shouldReplaySessionScrollback]
B --> R[restorableTmuxStartCommand / OMX HUD]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Workspace / static shim] -->|makeSessionRestorePolicyService| B[WorkspaceSessionRestorePolicyService]
A2[Workspace instance] -->|sessionRestorePolicy| B
B --> C{approvedSurfaceResumeBinding}
C --> D[applyStoredApproval closure]
D --> E[WorkspaceHermesAgentCommandBootstrapper]
E --> F{source == agent-hook AND autoResumeAgentSessions?}
F -->|blocked| G[nil]
F -->|allowed| H{requiresPromptApproval?}
H -->|yes| I[shouldRunPromptedSurfaceResume closure]
I -->|denied| G
I -->|approved| J[Approved Binding]
H -->|no| K{allowsAutomaticResume?}
K -->|no| G
K -->|yes| J
B --> L{surfaceResumeStartupLaunch}
J --> L
L -->|isAgentHookBinding| M[.command]
L -->|otherwise| N[.input]
B --> O[shouldAutoConnectRestoredRemote]
B --> P[resolvedSnapshotTerminalScrollback]
B --> Q[shouldReplaySessionScrollback]
B --> R[restorableTmuxStartCommand / OMX HUD]
Reviews (2): Last reviewed commit: "Fix dictation text merger package lint" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private let truncateScrollback: @Sendable (String?) -> String? | ||
| private let hermesCodexEnvironment: WorkspaceHermesCodexEnvironment | ||
| // Justification: FileManager is documented thread-safe but is not marked Sendable. | ||
| private nonisolated(unsafe) let fileManager: FileManager |
Contributor
There was a problem hiding this comment.
nonisolated(unsafe) disables Swift's Sendable enforcement on fileManager
nonisolated(unsafe) tells the compiler to stop tracking concurrency safety for this stored property. If any future code path on the service changes the FileManager delegate or configures it after construction (common when adding URL session proxies or temporary-directory overrides), the compiler will not catch the resulting race. For a struct that's meant to be fully Sendable, consider instead not storing FileManager at all and accepting it as a local parameter in each method that actually needs it (startupInputWithLauncherScript and startupCommandWithLauncherScript already thread it through), so the Sendable suppression can be removed.
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!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
1132-1141:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRoute remote resume startup input through the policy service.
The remote-startup branch still calls the binding DTO’s
startupInputWithLauncherScriptdirectly, while the local branch usessessionRestorePolicy.surfaceResumeStartupLaunch(...). That bypasses the extracted policy-owned Hermes/Codex rewriting for remote restores, so the same approved binding can resume differently depending on whetherremoteStartupCommandis present.Possible direction
let restoredBindingLaunch: SurfaceResumeStartupLaunch? = if remoteStartupCommand != nil { - effectiveResumeBindingForStartup? - .startupInputWithLauncherScript(allowLauncherScript: false) - .map(SurfaceResumeStartupLaunch.input) + effectiveResumeBindingForStartup.flatMap { binding in + sessionRestorePolicy + .surfaceResumeStartupLaunch( + forApprovedBinding: binding, + allowLauncherScript: false + ) + .initialInput + .map(SurfaceResumeStartupLaunch.input) + } } else { effectiveResumeBindingForStartup.flatMap { sessionRestorePolicy.surfaceResumeStartupLaunch(If
surfaceResumeStartupLaunch(forApprovedBinding:)can legally return.commandhere, add a policy-service approved-input helper instead of falling back to the DTO method.🤖 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 1132 - 1141, The remote-startup branch (when remoteStartupCommand is not nil) directly calls the binding DTO's startupInputWithLauncherScript method, bypassing the policy service, while the local branch routes through sessionRestorePolicy.surfaceResumeStartupLaunch. To ensure consistent policy-owned Hermes/Codex rewriting for both remote and local restores, replace the direct DTO call in the if branch with a call to sessionRestorePolicy instead. If needed, add a new policy-service method (similar to surfaceResumeStartupLaunch) that handles remote startup input approval through the policy service, ensuring the same approved binding resumes consistently regardless of whether remoteStartupCommand is present.
🤖 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 1132-1141: The remote-startup branch (when remoteStartupCommand is
not nil) directly calls the binding DTO's startupInputWithLauncherScript method,
bypassing the policy service, while the local branch routes through
sessionRestorePolicy.surfaceResumeStartupLaunch. To ensure consistent
policy-owned Hermes/Codex rewriting for both remote and local restores, replace
the direct DTO call in the if branch with a call to sessionRestorePolicy
instead. If needed, add a new policy-service method (similar to
surfaceResumeStartupLaunch) that handles remote startup input approval through
the policy service, ensuring the same approved binding resumes consistently
regardless of whether remoteStartupCommand is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: de4d1f6c-9f16-4500-9212-da650fa71814
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (10)
Packages/CmuxSession/Sources/CmuxSession/WorkspaceHermesCodexEnvironment.swiftPackages/CmuxSession/Sources/CmuxSession/WorkspaceSessionRemoteRestorePanelSnapshot.swiftPackages/CmuxSession/Sources/CmuxSession/WorkspaceSessionRemoteRestoreSnapshot.swiftPackages/CmuxSession/Sources/CmuxSession/WorkspaceSessionRemoteRestoreTerminalSnapshot.swiftPackages/CmuxSession/Sources/CmuxSession/WorkspaceSessionRestorePolicyService.swiftPackages/CmuxSession/Sources/CmuxSession/WorkspaceSurfaceResumeBinding.swiftPackages/CmuxSession/Sources/CmuxSession/WorkspaceSurfaceResumeStartupLaunch.swiftPackages/CmuxSession/Tests/CmuxSessionTests/WorkspaceSessionRestorePolicyServiceTests.swiftSources/SessionPersistence.swiftSources/Workspace.swift
# Conflicts: # .github/swift-file-length-budget.tsv
This was referenced Jun 19, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
WorkspaceSessionRestorePolicyServiceinCmuxSession..github/swift-file-length-budget.tsvforSources/Workspace.swiftfrom 12223 to 11868 lines.Domain rationale
I selected the workspace session restore policy / surface-resume launch planning cluster because it was the largest clean non-latency domain in
Sources/Workspace.swift: it decides restore scrollback policy, remote reconnect behavior, surface resume approval, Hermes Codex bootstrap rewriting, tmux HUD restoration, and startup launch payloads. It does not touch Ghostty terminal rendering,TabItemView,ContentView's equatableForEach,WindowTerminalHostView.hitTest, or recorder-coupled debug UI paths.Moved methods and types
resolvedSnapshotTerminalScrollbackshouldReplaySessionScrollbackshouldAutoConnectRestoredRemotesurfaceResumeStartupInputsurfaceResumeStartupLaunchapprovedSurfaceResumeBindingrestorableTmuxStartCommandshouldPersistSessionScrollbackSurfaceResumeStartupLaunchmoved asWorkspaceSurfaceResumeStartupLaunchInjected seams
applyStoredApprovalshouldRunPromptedSurfaceResumeisRunningUnderAutomatedTeststruncateScrollbackWorkspaceHermesCodexEnvironmentFileManagerandtemporaryDirectoryWorkspaceSurfaceResumeBinding,WorkspaceSessionRemoteRestoreSnapshot,WorkspaceSessionRemoteRestorePanelSnapshot,WorkspaceSessionRemoteRestoreTerminalSnapshotVerification
swift buildinPackages/CmuxSessionswift testinPackages/CmuxSession, 19 tests passedscripts/lint-ios-package-conventions.sh, no unjustified convention violationsxcodebuild -project cmux.xcodeproj -scheme cmux -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-workspacedecomp build > /tmp/cmux-workspacedecomp-build.log 2>&1grep '** BUILD SUCCEEDED **' /tmp/cmux-workspacedecomp-build.logNotes
Sources/Workspace.swiftchanged from 12223 to 11868 lines, a 355-line net reduction. I kept the scope to the coherent restore-policy domain rather than moving unrelated singleton or latency-sensitive paths just to inflate the line count.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Extracted workspace session restore policy into
WorkspaceSessionRestorePolicyServiceinCmuxSessionand wiredWorkspaceto it. Also made the composer dictation text merger injectable;Workspace.swiftdrops by 357 lines.WorkspaceSessionRestorePolicyServicewith seams for approval storage, prompt UI, test detection, scrollback truncation, Hermes Codex defaults, and filesystem paths.WorkspaceSurfaceResumeBinding,WorkspaceSessionRemoteRestoreSnapshot(+ panel/terminal), andWorkspaceSurfaceResumeStartupLaunch.WorkspaceHermesAgentCommandBootstrapperto strip old bootstrap, replace--provider openai-codex, and insert Codex bootstrap only when allowed.SurfaceResumeBindingSnapshot,Session*Snapshot) to the new protocols;Workspacenow delegates auto-connect, scrollback replay/persist, tmux HUD detection, and resume approval/launch to the service.ComposerDictationTextMergetoComposerDictationTextMerger, injected it intoComposerDictationController, and updated tests.Written for commit e4dc531. Summary will update on new commits.
Summary by CodeRabbit