Repository navigation
Repair persisted session restore snapshots - #6692
Closed
lawrencecchen wants to merge 9 commits into
Closed
lawrencecchen wants to merge 9 commits into
lawrencecchen wants to merge 9 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a session snapshot repair pipeline that detects and removes shell-wrapper-poisoned agent-hook resume bindings on snapshot load. Introduces trust-filtering properties and a repair traversal ( ChangesSession Snapshot Repair Pipeline
Sequence Diagram(s)sequenceDiagram
participant AppDelegate
participant SessionSnapshotRepository
participant AppSessionSnapshot
participant SessionSnapshotRepairer
participant FileManager
AppDelegate->>SessionSnapshotRepository: init(repairLoadedSnapshot: AppSessionSnapshot.repairLoadedSessionSnapshot)
SessionSnapshotRepository->>FileManager: read JSON from fileURL
FileManager-->>SessionSnapshotRepository: raw data
SessionSnapshotRepository->>SessionSnapshotRepository: JSON decode → SnapshotValue
SessionSnapshotRepository->>AppSessionSnapshot: repairLoadedSessionSnapshot(snapshot)
AppSessionSnapshot->>SessionSnapshotRepairer: repair(snapshot)
SessionSnapshotRepairer->>SessionSnapshotRepairer: traverse windows → workspaces → panels
SessionSnapshotRepairer->>SessionSnapshotRepairer: trustedForSessionRestore (drop poisoned bindings)
SessionSnapshotRepairer->>SessionSnapshotRepairer: repairedForSessionRestore (normalize CWD)
SessionSnapshotRepairer-->>AppSessionSnapshot: (repairedSnapshot, didRepair: true)
AppSessionSnapshot-->>SessionSnapshotRepository: (repairedSnapshot, didRepair: true)
SessionSnapshotRepository->>FileManager: save(repairedSnapshot, to: fileURL)
SessionSnapshotRepository-->>AppDelegate: .loaded(repairedSnapshot)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (6 errors, 1 warning)
✅ Passed checks (18 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 |
Contributor
Greptile SummaryThis PR repairs persisted session snapshots on load by introducing a
Confidence Score: 5/5Safe to merge — the repair runs only on load, writes back only when it detects a change, and all three code paths (poisoned resume binding, wrong-fork launch capture, CWD recovery) are covered by focused Swift Testing regressions that passed. The logic changes are tightly scoped: the trust check for nil-launcher captures adds nativeProcessDescribesKind without weakening the existing launcher check, the isPoisonedAgentHookShellWrapperResume guard is carefully bounded (only agent-hook source, only built-in kinds, two-token resume/--resume suffix), and the repairedForSessionRestore CWD repair condition correctly distinguishes agent-owned working directories from those inherited from the stripped launch command. The public loadOutcome method remains read-only so existing crash-diagnostic callers are unaffected, and the write-back is idempotent once a session snapshot has been cleaned. No files require special attention. The SessionSnapshotRepairer caseless-enum shape was raised in a previous review thread and is the only open style concern. Important Files Changed
Reviews (6): Last reviewed commit: "Keep session load outcome read-only" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1958
to
+2034
| enum SessionSnapshotRepairer { | ||
| static func repair(_ snapshot: AppSessionSnapshot) -> (snapshot: AppSessionSnapshot, didRepair: Bool) { | ||
| var didRepair = false | ||
| var repaired = snapshot | ||
| repaired.windows = repaired.windows.map { window in | ||
| repair(window, didRepair: &didRepair) | ||
| } | ||
| return (snapshot: repaired, didRepair: didRepair) | ||
| } | ||
|
|
||
| private static func repair( | ||
| _ window: SessionWindowSnapshot, | ||
| didRepair: inout Bool | ||
| ) -> SessionWindowSnapshot { | ||
| var repaired = window | ||
| repaired.tabManager.workspaces = repaired.tabManager.workspaces.map { workspace in | ||
| repair(workspace, didRepair: &didRepair) | ||
| } | ||
| return repaired | ||
| } | ||
|
|
||
| private static func repair( | ||
| _ workspace: SessionWorkspaceSnapshot, | ||
| didRepair: inout Bool | ||
| ) -> SessionWorkspaceSnapshot { | ||
| var repaired = workspace | ||
| repaired.panels = repaired.panels.map { panel in | ||
| repair(panel, workspaceDirectory: workspace.currentDirectory, didRepair: &didRepair) | ||
| } | ||
| return repaired | ||
| } | ||
|
|
||
| private static func repair( | ||
| _ panel: SessionPanelSnapshot, | ||
| workspaceDirectory: String, | ||
| didRepair: inout Bool | ||
| ) -> SessionPanelSnapshot { | ||
| guard var terminal = panel.terminal else { return panel } | ||
| let fallbackWorkingDirectory = firstNormalizedDirectory( | ||
| terminal.workingDirectory, | ||
| panel.directory, | ||
| workspaceDirectory | ||
| ) | ||
|
|
||
| if let resumeBinding = terminal.resumeBinding { | ||
| let trustedBinding = resumeBinding.trustedForSessionRestore | ||
| if trustedBinding == nil { | ||
| didRepair = true | ||
| } | ||
| terminal.resumeBinding = trustedBinding | ||
| } | ||
|
|
||
| if let agent = terminal.agent { | ||
| let repairedAgent = agent.repairedForSessionRestore( | ||
| fallbackWorkingDirectory: fallbackWorkingDirectory | ||
| ) | ||
| if agent.launchCommand != repairedAgent.launchCommand | ||
| || agent.workingDirectory != repairedAgent.workingDirectory { | ||
| didRepair = true | ||
| } | ||
| terminal.agent = repairedAgent | ||
| } | ||
|
|
||
| var repaired = panel | ||
| repaired.terminal = terminal | ||
| return repaired | ||
| } | ||
|
|
||
| private static func firstNormalizedDirectory(_ candidates: String?...) -> String? { | ||
| for candidate in candidates { | ||
| if let normalized = SurfaceResumeCommandCanonicalizer.normalizedCWD(candidate) { | ||
| return normalized | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
SessionSnapshotRepairer is a caseless enum whose entire public and private surface is static funcs — the canonical shape flagged by cmux-no-ambient-global-state. The same file already exposes the natural seam: AppSessionSnapshot.repairLoadedSessionSnapshot is where callers land, and all the recursive helpers operate exclusively on AppSessionSnapshot and its sub-snapshots. Placing the private static helpers directly inside a private extension AppSessionSnapshot block (alongside the already-existing repairLoadedSessionSnapshot) removes the separate namespace type and keeps all repair logic co-located with the type that owns the data.
Rule Used: Flag new ambient global state in production Swift:... (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!
![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
🤖 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 `@Sources/RestorableAgentSession.swift`:
- Around line 792-808: In the repairedForSessionRestore function, add a check to
preserve the `.ignore` cwd semantics by preventing any workingDirectory
backfilling when registration?.cwd is set to .ignore. Before the existing
conditional blocks that assign fallbackWorkingDirectory or
trustedLaunchCommand?.workingDirectory to repaired.workingDirectory, add a guard
condition that checks if registration?.cwd == .ignore and returns early or skips
the workingDirectory assignment entirely. This ensures that when cwd is
explicitly set to ignore, the workingDirectory remains nil throughout the repair
process.
- Around line 1154-1160: The launchCommand sanitization using
trustedLaunchCommand is currently happening after resolvedClaudeWorkflowRecord
has already been invoked, which allows untrusted launch captures to influence
the resolution process before they are stripped. Move the sanitization of
effectiveRecord.launchCommand using
SessionRestorableAgentSnapshot.trustedLaunchCommand before the
resolvedClaudeWorkflowRecord call is made, ensuring the record is sanitized
before any derivation of transcript or session candidates can occur from the
potentially poisoned launch cwd or environment.
In `@Sources/SessionPersistence.swift`:
- Around line 1958-1965: Replace the caseless enum `SessionSnapshotRepairer`
with a private struct or class that owns the `didRepair` state as an instance
property instead of threading it through mutable parameters. Convert the static
repair method to an instance method that modifies the internal didRepair
property, then create an instance of the repairer, call the repair method on it,
and return both the repaired snapshot and the didRepair flag from the instance
property. This eliminates the static namespace pattern and properly encapsulates
the mutable state within the repairer instance.
In `@Sources/Workspace.swift`:
- Around line 901-902: The restore gate at line 901 validates
binding.trustedForSessionRestore to ensure trusted launch data, but the
retargeting fallback logic still reads directly from
restorableAgent.launchCommand?.workingDirectory, which could contain untrusted
persisted launch data. Update the retargeting fallback to use the trusted launch
command information from the binding's trustedForSessionRestore property instead
of directly accessing the untrusted
restorableAgent.launchCommand?.workingDirectory, ensuring all restore decisions
are based on validated trusted launch captures.
🪄 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: 1d03160d-b039-49a6-8e97-e4c68fbd8f22
📒 Files selected for processing (6)
Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Session/SessionSnapshotRepository.swiftSources/AppDelegate.swiftSources/RestorableAgentSession.swiftSources/SessionPersistence.swiftSources/Workspace.swiftcmuxTests/SessionPersistenceTests.swift
Comment on lines
+792
to
+808
| func repairedForSessionRestore(fallbackWorkingDirectory: String?) -> SessionRestorableAgentSnapshot { | ||
| let trustedLaunchCommand = trustedLaunchCommandForSessionRestore | ||
| var repaired = self | ||
| repaired.launchCommand = trustedLaunchCommand | ||
|
|
||
| let fallbackWorkingDirectory = Self.normalizedWorkingDirectory(fallbackWorkingDirectory) | ||
| if trustedLaunchCommand == nil { | ||
| if repaired.workingDirectory == nil | ||
| || Self.normalizedWorkingDirectory(repaired.workingDirectory) | ||
| == Self.normalizedWorkingDirectory(launchCommand?.workingDirectory) { | ||
| repaired.workingDirectory = fallbackWorkingDirectory | ||
| } | ||
| } else if repaired.workingDirectory == nil { | ||
| repaired.workingDirectory = Self.normalizedWorkingDirectory( | ||
| trustedLaunchCommand?.workingDirectory | ||
| ) ?? fallbackWorkingDirectory | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve .ignore cwd semantics during repair.
When registration?.cwd == .ignore, this repair path can still backfill workingDirectory from the trusted launch command or panel/workspace fallback. That reintroduces a cwd for agents whose restore policy explicitly suppresses one.
🐛 Proposed fix
let trustedLaunchCommand = trustedLaunchCommandForSessionRestore
var repaired = self
repaired.launchCommand = trustedLaunchCommand
+
+ if registration?.cwd == .ignore {
+ repaired.workingDirectory = nil
+ return repaired
+ }
let fallbackWorkingDirectory = Self.normalizedWorkingDirectory(fallbackWorkingDirectory)
if trustedLaunchCommand == nil {Based on learnings, registrations with cwd: .ignore must keep the resume working directory nil so both the cwd guard and terminal placement cwd are suppressed.
🤖 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 792 - 808, In the
repairedForSessionRestore function, add a check to preserve the `.ignore` cwd
semantics by preventing any workingDirectory backfilling when registration?.cwd
is set to .ignore. Before the existing conditional blocks that assign
fallbackWorkingDirectory or trustedLaunchCommand?.workingDirectory to
repaired.workingDirectory, add a guard condition that checks if
registration?.cwd == .ignore and returns early or skips the workingDirectory
assignment entirely. This ensures that when cwd is explicitly set to ignore, the
workingDirectory remains nil throughout the repair process.
Source: Learnings
Comment on lines
+1958
to
+1965
| enum SessionSnapshotRepairer { | ||
| static func repair(_ snapshot: AppSessionSnapshot) -> (snapshot: AppSessionSnapshot, didRepair: Bool) { | ||
| var didRepair = false | ||
| var repaired = snapshot | ||
| repaired.windows = repaired.windows.map { window in | ||
| repair(window, didRepair: &didRepair) | ||
| } | ||
| return (snapshot: repaired, didRepair: didRepair) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Avoid adding a static namespace repairer.
SessionSnapshotRepairer is a new caseless enum used purely as a static-function namespace. Make it a private instance repairer that owns didRepair instead of threading mutable state through static helpers.
♻️ Suggested shape
extension AppSessionSnapshot {
static func repairLoadedSessionSnapshot(
_ snapshot: AppSessionSnapshot
) -> (snapshot: AppSessionSnapshot, didRepair: Bool) {
- SessionSnapshotRepairer.repair(snapshot)
+ var repairer = SessionSnapshotRepairer()
+ return repairer.repair(snapshot)
}
}
-enum SessionSnapshotRepairer {
- static func repair(_ snapshot: AppSessionSnapshot) -> (snapshot: AppSessionSnapshot, didRepair: Bool) {
- var didRepair = false
+private struct SessionSnapshotRepairer {
+ private var didRepair = false
+
+ mutating func repair(_ snapshot: AppSessionSnapshot) -> (snapshot: AppSessionSnapshot, didRepair: Bool) {
var repaired = snapshot
repaired.windows = repaired.windows.map { window in
- repair(window, didRepair: &didRepair)
+ repair(window)
}
return (snapshot: repaired, didRepair: didRepair)
}
- private static func repair(
- _ window: SessionWindowSnapshot,
- didRepair: inout Bool
- ) -> SessionWindowSnapshot {
+ private mutating func repair(_ window: SessionWindowSnapshot) -> SessionWindowSnapshot {
var repaired = window
repaired.tabManager.workspaces = repaired.tabManager.workspaces.map { workspace in
- repair(workspace, didRepair: &didRepair)
+ repair(workspace)
}
return repaired
}
}As per coding guidelines, .github/review-bot-rules/no-ambient-global-state.md disallows a caseless enum/empty struct used purely as a static func namespace in production Swift.
🤖 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/SessionPersistence.swift` around lines 1958 - 1965, Replace the
caseless enum `SessionSnapshotRepairer` with a private struct or class that owns
the `didRepair` state as an instance property instead of threading it through
mutable parameters. Convert the static repair method to an instance method that
modifies the internal didRepair property, then create an instance of the
repairer, call the repair method on it, and return both the repaired snapshot
and the didRepair flag from the instance property. This eliminates the static
namespace pattern and properly encapsulates the mutable state within the
repairer instance.
Source: Coding guidelines
Comment on lines
+901
to
+902
| guard let binding = binding?.trustedForSessionRestore else { return nil } | ||
| guard binding.isAgentHookBinding, let restorableAgent else { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Use the trusted launch cwd in this restore gate too.
Line 901 filters the binding, but the later retargeting fallback still reads restorableAgent.launchCommand?.workingDirectory. If an older or unrepaired snapshot reaches this path, an untrusted persisted launch capture can still retarget the binding cwd.
🐛 Proposed fix
// Restore has no live hook cwd; use the snapshot's derived restorable cwd
// and fall back to launch capture only for older snapshots.
let snapshotRestorableWorkingDirectory =
- restorableAgent.workingDirectory ?? restorableAgent.launchCommand?.workingDirectory
+ restorableAgent.workingDirectory
+ ?? restorableAgent.trustedLaunchCommandForSessionRestore?.workingDirectoryThis follows the PR objective to strip untrusted persisted agent launch captures from restore decisions.
🤖 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 901 - 902, The restore gate at line 901
validates binding.trustedForSessionRestore to ensure trusted launch data, but
the retargeting fallback logic still reads directly from
restorableAgent.launchCommand?.workingDirectory, which could contain untrusted
persisted launch data. Update the retargeting fallback to use the trusted launch
command information from the binding's trustedForSessionRestore property instead
of directly accessing the untrusted
restorableAgent.launchCommand?.workingDirectory, ensuring all restore decisions
are based on validated trusted launch captures.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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 90ab037. Configure here.
| ) -> SurfaceResumeBindingSnapshot? { | ||
| guard let binding, binding.isAgentHookBinding, let restorableAgent else { | ||
| guard let binding = binding?.trustedForSessionRestore else { return nil } | ||
| guard binding.isAgentHookBinding, let restorableAgent else { |
There was a problem hiding this comment.
Untrusted launch cwd in retarget
Medium Severity
resumeBindingForSessionRestore still derives snapshotRestorableWorkingDirectory from the raw persisted launchCommand when the agent’s workingDirectory is missing, while resume/fork paths in the same change now use trustedLaunchCommandForSessionRestore. Snapshots that never pass load-time repair (e.g. closed-panel history) can retarget agent-hook bindings using a foreign launch capture’s cwd even though agent resume commands ignore that capture.
Reviewed by Cursor Bugbot for commit 90ab037. Configure here.
lawrencecchen
added
the
stale-revisit
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
2 participants
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
agent-hookbindings shaped like shell-wrapperbash resume ...commands.Testing
swift test --package-path Packages/macOS/CmuxWorkspacespassed, 91 Swift Testing tests../scripts/reload-cloud.sh --tag srfixpassed, installedcmux DEV srfix.applocally.xcodebuild testwas not run locally because the repo guard blocks local cmux app-host tests by default to avoid stealing focus and socket pollution.Notes
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes session restore and resume command generation from persisted state; mistakes could drop valid bindings or launch captures, but behavior is guarded by tests and read-only inspection paths.
Overview
Adds an on-load repair pass for app session snapshots:
SessionSnapshotRepositoryacceptsrepairLoadedSnapshot, runs it after decode, and persists cleaned JSON on normalload/ startup paths whileloadOutcomestays read-only.SessionSnapshotRepairerwalks terminals and drops poisonedagent-hookresume bindings (shellresumewrappers for built-in kinds, viatrustedForSessionRestore) and untrusted agent launch captures, then recoversworkingDirectoryfrom terminal/panel/workspace when a bad capture is stripped.Launch-capture trust is centralized on
SessionRestorableAgentSnapshot: resume/fork use trusted captures only; nil-launcher paths validate vianativeProcessDescribesKind(new Hermes andacli→rovodevaliases) and reject shell-wrapper argv. Restore wiring uses the same trust inWorkspaceandAppDelegate’s snapshot store.Regression tests cover repository repair persistence, binding poisoning, and resume-command behavior.
Reviewed by Cursor Bugbot for commit 90ab037. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Repairs persisted session snapshots on load and writes back the cleaned version when using store load paths. Inspection loads remain read‑only. Blocks poisoned
agent-hookresumes, tightens launch-capture trust (incl. nil-launcher Hermes andacli→rovodev), and restores the correct working directory.SessionSnapshotRepositorywired toAppSessionSnapshot.repairLoadedSessionSnapshotviaSessionSnapshotRepairer; persist cleaned snapshots onload/startup while keepingloadOutcomeread‑only.launcheror native process; reject shell‑wrapper argv; recognize nil‑launcher Hermes andacli→rovodev.agent-hookresume bindings, including registry-owned built‑ins; keep valid custom agent‑named wrappers.workingDirectoryfrom terminal/panel/workspace; applytrustedForSessionRestoreinWorkspace; add Swift Testing regressions.Written for commit 90ab037. Summary will update on new commits.
Summary by CodeRabbit