Add surface resume bindings - #4237
Merged
Merged
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:
📝 WalkthroughWalkthroughAdds persisted per-terminal surface resume bindings, tmux process detection, workspace storage and snapshot propagation, restore-time startup-input handling, V2 socket RPCs (set/get/clear), CLI surface-resume commands and agent publish wiring, and tests. ChangesSurface Resume Binding Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 2 warnings)
✅ Passed checks (11 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 introduces per-surface resume bindings — a new model persisted in session snapshots so that terminal surfaces restore by running a safe resume command (tmux attach, agent resume, or a user-approved CLI command) instead of replaying scrollback. It adds V2 socket methods and CLI commands (
Confidence Score: 3/5Not safe to merge as-is: a socket client can claim 'agent-hook' source to bypass the signed-approval system and auto-run arbitrary commands on the next restore, and several blocking main-thread operations from prior review rounds remain unresolved. The approval system introduced by this PR is the trust boundary for auto-executing persisted commands at restore time. That boundary has an exploitable gap — v2PublicSurfaceResumeSource filters 'process-detected' but not 'agent-hook', so any local process can claim trusted status without HMAC verification or user consent. Prior review issues (blocking keychain + file I/O on the main actor, approval-prompt dialog firing every autosave tick, inlineStartupInput double-quoting, Localizable.xcstrings missing translations for 15 locales) remain open and compound the risk. Sources/TerminalController.swift (v2PublicSurfaceResumeSource — agent-hook source bypass), Sources/SessionPersistence.swift (blocking I/O on main actor; inlineStartupInput double-quoting), Sources/Workspace.swift (approval-prompt dialog on autosave tick), Sources/cmuxApp.swift (keychain read per record per SwiftUI render), Resources/Localizable.xcstrings (missing translations for 15 locales)
|
| Filename | Overview |
|---|---|
| Sources/TmuxResumeParser.swift | New file implementing tmux argument parsing for generating safe resume bindings; attach branch now correctly guards sessionName != nil and new-session -A requires an explicit -s name; shortFlagCluster stops at value-option characters, addressing the bundled-flag false-positive. |
| Sources/SessionPersistence.swift | Large addition of SurfaceResumeBindingSnapshot, approval record/store, HMAC signing, and launcher-script infrastructure; several issues flagged in prior threads (blocking keychain+file I/O on MainActor hot paths, double-quoting of trimmed command in inlineStartupInput when environment is set) remain unresolved. |
| Sources/TerminalController.swift | Adds v2 socket methods for surface.resume.set/get/clear; source sanitization in v2PublicSurfaceResumeSource omits 'agent-hook', allowing any socket client to claim trusted agent status and bypass the approval flow; environment is also silently dropped from v2SurfaceResumeBindingPayload (flagged in prior thread). |
| Sources/Workspace.swift | Adds surfaceResumeBindingsByPanelId and integration into snapshot/restore flow; prior threads identified approval-prompt dialog firing on every autosave tick and mutating side effects hidden inside effectiveSurfaceResumeBinding; those concerns remain open. |
| Sources/RestorableAgentSession.swift | Adds SurfaceResumeBindingIndex and ProcessDetectedResumeIndexes; loadSynchronously correctly shares a single CmuxTopProcessSnapshot across both agent and tmux scans; synchronous main-thread invocation paths remain at quit/update-relaunch. |
| Sources/cmuxApp.swift | Adds SurfaceResumeApprovalSettingsCard SwiftUI view; isValid(record) inside ForEach body triggers a keychain read per record per render (flagged in prior thread); reload() performs synchronous disk I/O on the main thread from onAppear and notification callbacks. |
| Sources/AppDelegate.swift | Adds generation-counter gating for async autosave scans; saveSessionSnapshotIncludingProcessDetectedIndexes and saveSessionSnapshotAfterLoadingProcessDetectedIndexes cleanly separate sync-quit and async-resign paths; generation bump on every saveSessionSnapshot call risks perpetual cancellation during rapid agent activity (flagged in prior thread). |
| CLI/cmux.swift | Adds surface/surface-resume CLI subcommands; surfaceResumeTarget correctly strips --workspace/--surface before the remaining arg chain; validateSurfaceResumeSetCommandTokensBeforeSocket performs the same stripping for pre-socket validation. |
| Resources/Localizable.xcstrings | 32 new string keys added for approval dialogs and Settings UI; only en and ja translations present — 15 other supported locales show untranslated English in modal alert dialogs (flagged in prior thread and remains open). |
Sequence Diagram
sequenceDiagram
participant CLI as cmux CLI / Socket Client
participant TC as TerminalController
participant Store as SurfaceResumeApprovalStore
participant WS as Workspace
participant AD as AppDelegate (autosave)
participant Restore as Session Restore (createPanel)
CLI->>TC: "surface.resume.set {command, source, auto_resume}"
TC->>TC: v2PublicSurfaceResumeSource() remap process-detected to manual
Note over TC: agent-hook passes through unchecked
TC->>Store: applyingStoredApproval(binding)
alt process-detected
Store-->>TC: "autoResume=true (trusted unconditionally)"
else agent-hook
Store-->>TC: "autoResume=binding.autoResume (trusted by label claim only)"
else cli / other
Store->>Store: matchingRecord() HMAC-signed lookup
Store-->>TC: approvalPolicy per signed record
end
TC->>WS: setSurfaceResumeBinding(effectiveBinding, panelId)
AD->>AD: sessionAutosaveTick()
AD->>AD: ProcessDetectedResumeIndexes.load() background Task
AD->>WS: saveSessionSnapshot(surfaceResumeBindingIndex)
WS->>WS: reconcileSurfaceResumeBindings()
WS-->>AD: SessionPanelSnapshot.resumeBinding persisted
Restore->>WS: createPanel(from: snapshot)
WS->>WS: surfaceResumeStartupInput(resumeBinding)
alt allowsAutomaticResume
WS-->>Restore: startupInput sent as terminal initialInput
else "approval=prompt"
WS->>WS: NSAlert.runModal() user confirms
WS-->>Restore: startup input if confirmed
else manual or no binding
WS-->>Restore: nil scrollback replayed normally
end
Reviews (37): Last reviewed commit: "fix: keep agent-hook resume state truste..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| struct SurfaceResumeBindingIndex: Sendable { |
Contributor
There was a problem hiding this comment.
SurfaceResumeBindingIndex is a Sendable value type used from background Task.detached closures and async let bindings in AppDelegate. Without an explicit nonisolated declaration, the struct would be implicitly @MainActor in a Swift 6 module with @MainActor-by-default isolation, making its use in loadIncludingProcessDetectedBindings's detached task a concurrency violation.
Suggested change
| struct SurfaceResumeBindingIndex: Sendable { | |
| nonisolated struct SurfaceResumeBindingIndex: Sendable { |
Rule Used: Flag new or materially worsened Swift 6 actor isol... (source)
| var isDirty: Bool | ||
| } | ||
|
|
||
| struct SurfaceResumeBindingSnapshot: Codable, Equatable, Sendable { |
Contributor
There was a problem hiding this comment.
SurfaceResumeBindingSnapshot is a Sendable value model used across both MainActor and background paths (process scanning, session snapshot serialization, v2SurfaceResumeSet). Leaving it implicitly @MainActor in a Swift 6 module is a latent concurrency error whenever it is constructed or accessed off the main actor.
Suggested change
| struct SurfaceResumeBindingSnapshot: Codable, Equatable, Sendable { | |
| nonisolated struct SurfaceResumeBindingSnapshot: Codable, Equatable, Sendable { |
Rule Used: Flag new or materially worsened Swift 6 actor isol... (source)
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="CLI/cmux.swift">
<violation number="1" location="CLI/cmux.swift:4757">
P1: `surface resume set` can persist `--workspace`/`--surface` flags inside the saved command when using `-- <argv...>`, which breaks resume execution.</violation>
</file>
<file name="Sources/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:7378">
P2: `surfaceResumeBindingsByPanelId` is added but not cleared in the standard panel-close lifecycle cleanup, leaving stale per-panel bindings after closes.</violation>
</file>
<file name="Sources/VaultAgentProcessScanner.swift">
<violation number="1" location="Sources/VaultAgentProcessScanner.swift:737">
P2: `tmuxHasFlag` matches `-A` by substring, so `new-session` commands like `-sA` are misclassified as attach-mode resumes.</violation>
</file>
<file name="Sources/RestorableAgentSession.swift">
<violation number="1" location="Sources/RestorableAgentSession.swift:1025">
P2: `load()` always returns an empty index, so default snapshot saves can drop process-detected tmux resume bindings outside the autosave path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/AppDelegate.swift (1)
3761-3783:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLoad the live detected resume indices in the fallback snapshot path.
Line 3769 and Line 3770 still fall back to
.load(). The non-autosave save paths in this file hit that branch, so quit/manual snapshots can drop the latest process-detected agent/resume state unless an autosave already ran.🩹 Suggested fix
- let restorableAgentIndex = suppliedRestorableAgentIndex ?? RestorableAgentSessionIndex.load() - let surfaceResumeBindingIndex = suppliedSurfaceResumeBindingIndex ?? SurfaceResumeBindingIndex.load() + let restorableAgentIndex = + suppliedRestorableAgentIndex ?? RestorableAgentSessionIndex.loadIncludingProcessDetectedSnapshots() + let surfaceResumeBindingIndex = + suppliedSurfaceResumeBindingIndex ?? SurfaceResumeBindingIndex.loadIncludingProcessDetectedBindings()🤖 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/AppDelegate.swift` around lines 3761 - 3783, In buildSessionSnapshot replace the fallback uses of RestorableAgentSessionIndex.load() and SurfaceResumeBindingIndex.load() with the runtime/detected versions so non-autosave snapshot paths pick up the latest process-detected resume indices (e.g., use the “detected”/live-loading APIs such as RestorableAgentSessionIndex.detected() and SurfaceResumeBindingIndex.detected() or whatever the project provides for live detection) instead of .load(); update the restorableAgentIndex and surfaceResumeBindingIndex initializers (the suppliedRestorableAgentIndex/suppliedSurfaceResumeBindingIndex fallback) to call those detected/live methods.
🤖 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 10306-10313: The docs for the restore command list a --checkpoint
flag but the implementation also accepts --checkpoint-id (with --checkpoint-id
taking precedence); update the CLI help block shown (the Flags list in
CLI/cmux.swift) to include both options consistently (add a line documenting
--checkpoint-id and explain precedence) or remove the alternate parsing for
--checkpoint-id from the parsing logic so only --checkpoint remains; refer to
the flag names --checkpoint and --checkpoint-id to locate the parsing/flags
definition and update the displayed Flags section accordingly.
- Around line 4756-4779: surfaceResumeTargetParams consumes
--workspace/--surface from the argument list but currently doesn't return the
modified remainder, so the later parseOption chain (starting from rest and
producing rem7) still contains those flags and they leak into the argv used to
build commandText; fix by changing the call site and/or
surfaceResumeTargetParams to return the updated remainder and use that remainder
as the input to the subsequent parseOption chain (e.g., have
surfaceResumeTargetParams return (params, remainingArgs) or accept&mutate the
rest variable) and then start parseOption from that cleaned remainder (use the
returned remainder instead of the original rest when computing (name, rem1),
etc.), ensuring rem7 no longer contains --workspace/--surface.
In `@Sources/TabManager.swift`:
- Around line 7444-7461: The helper hashSurfaceResumeBindingSnapshot currently
omits the binding's preserved environment, so add hashing of the environment map
on snapshot (SurfaceResumeBindingSnapshot) in a deterministic key order: if
snapshot has a preserved environment dictionary, iterate its keys sorted, and
for each key combine the key and its value into the hasher (use existing helpers
like hashOptionalString or hasher.combine for values), ensuring nil/empty cases
are handled consistently (e.g., combine a flag before hashing entries) and keep
using the existing pattern with hasher to maintain compatibility with
hashOptionalDouble/hashOptionalString.
In `@Sources/TerminalController.swift`:
- Around line 6091-6103: The handler currently always binds to the incoming
TabManager/workspace via v2ResolveWorkspace and then checks surface_id (using
workspace.focusedPanelId and workspace.terminalPanel(for:)), which causes valid
surface_id values owned by other windows to be rejected; change the logic in
v2MainSync (and the similar blocks at the other noted locations) so that if
params contains a surface_id you first call
AppDelegate.shared?.locateSurface(surfaceId:) to obtain the owning
TabManager/workspace and use that resolved workspace for subsequent validation
and terminalPanel(for:) checks, falling back to the existing
v2ResolveWorkspace/tabManager-focused behavior only when no explicit surface_id
is provided or locateSurface fails.
- Around line 6072-6074: The user-facing error string currently exposes an
internal type ("TabManager not available") in the guard using
v2ResolveTabManager(params:) and its .err(...) return; replace that message with
a product-facing cmux description such as "Window is not available; please
reopen the window or try again" (or similar concise user guidance) for the
return .err(...) in the v2ResolveTabManager failure path, and make the internal
detail available only via a debug/process log call; apply the same change to the
other instances referencing v2ResolveTabManager failure (the other .err(...)
returns noted at the other occurrences).
In `@Sources/VaultAgentProcessScanner.swift`:
- Around line 626-635: tmuxExecutable currently can return a tmux-style process
title (e.g. "tmux: ...") because it checks argumentLooksLikeTmux against the raw
first arg or processPath; change it to return a normalized executable path
instead: when checking the first argument or processPath, run normalized(...) on
the candidate and only return the normalized value if
argumentLooksLikeTmux(normalizedCandidate) is true; update both tmuxExecutable
and the other identical selection block (the resume argv builder) so argv[0] is
always the normalized executable rather than the raw title.
In `@Sources/Workspace.swift`:
- Around line 842-846: The code is promoting detection-restored bindings into
workspace state and masking live updates from SurfaceResumeBindingIndex; change
the conditional that writes to surfaceResumeBindingsByPanelId so it only caches
explicit user/agent bindings (e.g. check resumeBinding.origin,
resumeBinding.source, or resumeBinding.isUserInitiated) before assigning; if
resumeBinding indicates a detection-sourced binding (e.g. .detected) do not
insert it and instead removeValue(forKey: terminalPanel.id) so the runtime will
fall back to surfaceResumeBindingIndex for future updates.
- Around line 8847-8853: The guard in setSurfaceResumeBinding(_ binding:
SurfaceResumeBindingSnapshot, panelId: UUID) only rejects nil startupInput but
allows empty or whitespace-only strings, causing resume logic to misbehave;
update the guard to also reject empty/all-whitespace binding.startupInput (e.g.
check that binding.startupInput?.trimmingCharacters(in:
.whitespacesAndNewlines).isEmpty == false) while keeping the existing
terminalPanel(for: panelId) != nil check, then continue to set
surfaceResumeBindingsByPanelId[panelId] = binding and return true as before.
---
Outside diff comments:
In `@Sources/AppDelegate.swift`:
- Around line 3761-3783: In buildSessionSnapshot replace the fallback uses of
RestorableAgentSessionIndex.load() and SurfaceResumeBindingIndex.load() with the
runtime/detected versions so non-autosave snapshot paths pick up the latest
process-detected resume indices (e.g., use the “detected”/live-loading APIs such
as RestorableAgentSessionIndex.detected() and
SurfaceResumeBindingIndex.detected() or whatever the project provides for live
detection) instead of .load(); update the restorableAgentIndex and
surfaceResumeBindingIndex initializers (the
suppliedRestorableAgentIndex/suppliedSurfaceResumeBindingIndex fallback) to call
those detected/live methods.
🪄 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: 60e264b5-3a4c-4348-ba8d-7376741ea137
📒 Files selected for processing (10)
CLI/cmux.swiftSources/AppDelegate.swiftSources/RestorableAgentSession.swiftSources/SessionPersistence.swiftSources/TabManager.swiftSources/TerminalController.swiftSources/VaultAgentProcessScanner.swiftSources/Workspace+DetachedSurfaceTransfer.swiftSources/Workspace.swiftcmuxTests/SessionPersistenceTests.swift
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/Workspace.swift (2)
14158-14177:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDetach/reattach can lose index-backed resume bindings.
Lines 14158-14177 only copy
surfaceResumeBindingsByPanelId[panelId]intoDetachedSurfaceTransfer. But Lines 187-189 already show that a panel’s live binding can come fromsurfaceResumeBindingIndex, so moving a process-detected tmux surface can silently strip its resume binding on reattach.🤖 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 14158 - 14177, The DetachedSurfaceTransfer initializer only copies surfaceResumeBindingsByPanelId[panelId], which loses resume bindings that are stored in surfaceResumeBindingIndex; update the code that builds pendingDetachedSurfaces[tabId] to compute the resume binding the same way the reattach/restore path does (i.e., prefer surfaceResumeBindingsByPanelId[panelId], but if nil fall back to the index-backed entry from surfaceResumeBindingIndex for that panelId or use the existing resolver used elsewhere), and pass that resolved binding into the resumeBinding field so index-backed bindings survive detach/reattach.
786-803:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the effective resume input when deciding scrollback replay.
Line 786 intentionally drops agent-hook input when auto-resume is disabled, but Lines 799-803 still pass the original
resumeBindingintoshouldReplaySessionScrollback(...). That disables scrollback replay even though no startup input will be sent, so the restored terminal can come back empty.Suggested fix
let autoResumeAgentSessions = AgentSessionAutoResumeSettings.isEnabled() let restoredBindingInput = resumeBinding?.source == "agent-hook" && !autoResumeAgentSessions ? nil : resumeBinding?.startupInput + let effectiveResumeBinding = restoredBindingInput == nil ? nil : resumeBinding let restorableTmuxStartCommand = restorableAgent == nil && restoredBindingInput == nil ? Self.restorableTmuxStartCommand(snapshot.terminal?.tmuxStartCommand) : nil let restoredTmuxStartupScript = restorableTmuxStartCommand.flatMap { SessionRestoredTerminalCommandStore.writeLauncherScript( @@ let shouldReplayScrollback = Self.shouldReplaySessionScrollback( restorableAgent: restorableAgent, tmuxStartCommand: restoredTmuxStartCommand, - resumeBinding: resumeBinding + resumeBinding: effectiveResumeBinding )🤖 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 786 - 803, The code computes restoredBindingInput (dropping agent-hook input when auto-resume is disabled) but still passes the original resumeBinding into shouldReplaySessionScrollback; change the call to shouldReplaySessionScrollback to pass the effective restored binding (restoredBindingInput) instead of resumeBinding so replay logic uses the actual startup input that will be sent; update the arguments to shouldReplaySessionScrollback (calling Self.shouldReplaySessionScrollback(restorableAgent:restorableAgent, tmuxStartCommand:restoredTmuxStartCommand, resumeBinding:restoredBindingInput)) and ensure any downstream uses expect the same optional type.
🤖 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 19533-19550: The code is serializing sensitive CLAUDE auth values
into the persisted resume command by appending full "key=value" pairs for keys
in claudeAuthKeys; instead, when kind == "claude" stop including secret values
in parts and only record non-sensitive selectors or the key names; specifically,
change the logic around preservedClaudeKeys / parts so you append a flag like
"CMUX_PRESERVE_CLAUDE_AUTH_SELECTION_ENV=1" and a CSV of the key names
(CMUX_PRESERVE_CLAUDE_AUTH_SELECTION_ENV_KEYS) but never the corresponding
selected[key] values, and ensure any restoration of auth uses a non-exported,
secure path (not resume_binding.command).
- Around line 19398-19410: The synthesized resume commands for the "claudeTeams"
and "omo" branches always prepend the subcommand token (e.g., "claude-teams" or
"omo") even when original.executable already points to the direct binary,
causing duplicates; fix by only injecting the subcommand when the wrapper is
being used or when you actually stripped the token from tail: in each branch
compute a boolean (e.g., removedToken = tail.first == "<token>" then
tail.removeFirst()) and only build argv with the extra subcommand when
removedToken is true or original.executable equals the wrapper/fallback (the
value passed to agentSurfaceResumeCommandParts, e.g., "cmux"); update the argv
construction in both the agentSurfaceResumeCommandParts /
AgentLaunchSanitizer.preservedArguments flows so that original.executable isn't
duplicated.
---
Outside diff comments:
In `@Sources/Workspace.swift`:
- Around line 14158-14177: The DetachedSurfaceTransfer initializer only copies
surfaceResumeBindingsByPanelId[panelId], which loses resume bindings that are
stored in surfaceResumeBindingIndex; update the code that builds
pendingDetachedSurfaces[tabId] to compute the resume binding the same way the
reattach/restore path does (i.e., prefer
surfaceResumeBindingsByPanelId[panelId], but if nil fall back to the
index-backed entry from surfaceResumeBindingIndex for that panelId or use the
existing resolver used elsewhere), and pass that resolved binding into the
resumeBinding field so index-backed bindings survive detach/reattach.
- Around line 786-803: The code computes restoredBindingInput (dropping
agent-hook input when auto-resume is disabled) but still passes the original
resumeBinding into shouldReplaySessionScrollback; change the call to
shouldReplaySessionScrollback to pass the effective restored binding
(restoredBindingInput) instead of resumeBinding so replay logic uses the actual
startup input that will be sent; update the arguments to
shouldReplaySessionScrollback (calling
Self.shouldReplaySessionScrollback(restorableAgent:restorableAgent,
tmuxStartCommand:restoredTmuxStartCommand, resumeBinding:restoredBindingInput))
and ensure any downstream uses expect the same optional type.
🪄 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: afc63c42-f994-4bd9-81c2-282913274f3c
📒 Files selected for processing (8)
CLI/cmux.swiftSources/RestorableAgentSession.swiftSources/SessionPersistence.swiftSources/VaultAgentProcessScanner.swiftSources/Workspace+PanelLifecycle.swiftSources/Workspace.swiftcmuxTests/AgentSessionAutoResumeSettingsTests.swiftcmuxTests/SessionPersistenceTests.swift
There was a problem hiding this comment.
3 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxTests/AgentSessionAutoResumeSettingsTests.swift">
<violation number="1" location="cmuxTests/AgentSessionAutoResumeSettingsTests.swift:114">
P2: This test should isolate `CMUX_AGENT_HOOK_STATE_DIR` to a temporary directory and restore it in `defer`; otherwise it can leak to/process external hook state via environment override.
(Based on your team's feedback about isolating Codex/agent hook state in tests.) [FEEDBACK_USED]</violation>
</file>
<file name="Sources/VaultAgentProcessScanner.swift">
<violation number="1" location="Sources/VaultAgentProcessScanner.swift:746">
P2: The clustered short-flag parser omits `-f` as a value-taking option, so `-f<value>` can be misread as containing `-A` attach.</violation>
</file>
<file name="Sources/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:786">
P2: Align scrollback replay gating with the effective resume-binding input decision. In the auto-resume-disabled agent-hook path, startup input is nulled here, but replay logic can still see `resumeBinding?.startupInput` and skip scrollback, leaving restores with neither input nor replay.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
CLI/cmux.swift (2)
19400-19412:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDirect
claude-teams/omolaunches still duplicate the executable token.If
original.executablealready points at the direct binary, these branches still synthesizeclaude-teams claude-teams --resume ...andomo omo --session .... Only prepend the subcommand when you actually stripped it fromtail, or whenoriginal.executableis the wrapper path that requires that extra token.🤖 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 `@CLI/cmux.swift` around lines 19400 - 19412, The branches for "claudeTeams" and "omo" always prepend the subcommand token causing duplicates; change logic in the case blocks (around agentSurfaceResumeCommandParts, original.executable, tail, and AgentLaunchSanitizer.preservedArguments) to track whether you removed the subcommand from tail (e.g., set a flag removedSubcommand = tail.first == "claude-teams"/"omo" then removeFirst()), and only include the explicit subcommand token in the constructed argv when removedSubcommand is true or when original.executable is the wrapper/fallback executable (the same value passed as fallbackExecutable, e.g., "cmux"); keep using normalizedSessionId and preservedArguments as before.
19535-19552:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftDo not persist Claude auth env values inside
resume_binding.command.
partsstill serializes selected environment entries asKEY=value, and this key set includesANTHROPIC_API_KEY/ANTHROPIC_AUTH_TOKEN. Because the binding is persisted andsurface resume showreturns the raw command, this leaks credentials into snapshots, CLI output, and API payloads. At minimum, filterclaudeAuthKeysout ofparts; the actual auth restoration needs a non-exported path.As per coding guidelines: user-facing command output, API error bodies, or recovery copy must not expose environment variables, credentials, tokens, or unredacted payload data.
🤖 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 `@CLI/cmux.swift` around lines 19535 - 19552, The current serialization builds parts from selected as "KEY=value" which includes sensitive Claude auth keys; change the parts construction to exclude any key in claudeAuthKeys when kind == "claude" so you never serialize their values into resume_binding.command (i.e. compute parts from selected.keys.sorted().filter { !(kind == "claude" && claudeAuthKeys.contains($0)) } before mapping to "\(key)=\(value)"), keep preservedClaudeKeys as only the key names (preservedClaudeKeys = selected.keys.sorted().filter { claudeAuthKeys.contains($0) }) and continue to append only a flag and the comma-joined key names (CMUX_PRESERVE_CLAUDE_AUTH_SELECTION_ENV and CMUX_PRESERVE_CLAUDE_AUTH_SELECTION_ENV_KEYS) without ever reading or including selected[key] for those sensitive keys.Sources/Workspace.swift (1)
8869-8876:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject empty or whitespace-only resume commands.
Line 8872 only rejects
nil.""and" "still get stored, and Line 579 then treats them as active startup input, which suppresses scrollback replay without sending anything to the terminal.🤖 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 8869 - 8876, In setSurfaceResumeBinding(_ binding: SurfaceResumeBindingSnapshot, panelId: UUID) reject startupInput that is empty or only whitespace: check binding.startupInput?.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty and treat that as nil (return false) instead of storing; ensure surfaceResumeBindingsByPanelId[panelId] is only set when the trimmed startupInput is non-empty and terminalPanel(for:) exists so whitespace-only strings don't suppress scrollback replay.
🤖 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 4755-4777: The issue is that parseOption and the subsequent option
extraction run over the entire target.remaining, consuming flags that should be
treated as command argv after a "--" delimiter; update the "set" branch so you
first split target.remaining at the first "--" into optionsPart and argvPart,
run surfaceResumeTarget parsing and the parseOption chain only against
optionsPart (use optionsPart as the input to parseOption calls like
parseOption(..., name: "--name") and symbols params, name, kind, checkpoint,
checkpointID, source, cwd, shellCommand), then treat argvPart as rem7 (so argv =
argvPart if rem7.first == "--" ? Array(rem7.dropFirst()) : rem7) to preserve any
flags after the delimiter as argv rather than options to be persisted.
In `@Sources/Workspace.swift`:
- Around line 571-579: The check in shouldReplaySessionScrollback wrongly uses
resumeBinding?.startupInput (the raw binding) which can be intentionally nulled
for "agent-hook" when auto-resume is disabled; change the condition to use the
post-gating/effective startup input instead (the value that createPanel or the
resume-gate produces) so that we test the actual command that will run at
resume. Concretely, replace the raw lookup resumeBinding?.startupInput in
shouldReplaySessionScrollback with the effective startup input accessor (or the
resumeBinding value returned/modified by the auto-resume gate used by
createPanel) so the predicate reflects the post-gate state when deciding to
replay scrollback; keep the other checks (restorableAgent and
restorableTmuxStartCommand) intact.
---
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 19400-19412: The branches for "claudeTeams" and "omo" always
prepend the subcommand token causing duplicates; change logic in the case blocks
(around agentSurfaceResumeCommandParts, original.executable, tail, and
AgentLaunchSanitizer.preservedArguments) to track whether you removed the
subcommand from tail (e.g., set a flag removedSubcommand = tail.first ==
"claude-teams"/"omo" then removeFirst()), and only include the explicit
subcommand token in the constructed argv when removedSubcommand is true or when
original.executable is the wrapper/fallback executable (the same value passed as
fallbackExecutable, e.g., "cmux"); keep using normalizedSessionId and
preservedArguments as before.
- Around line 19535-19552: The current serialization builds parts from selected
as "KEY=value" which includes sensitive Claude auth keys; change the parts
construction to exclude any key in claudeAuthKeys when kind == "claude" so you
never serialize their values into resume_binding.command (i.e. compute parts
from selected.keys.sorted().filter { !(kind == "claude" &&
claudeAuthKeys.contains($0)) } before mapping to "\(key)=\(value)"), keep
preservedClaudeKeys as only the key names (preservedClaudeKeys =
selected.keys.sorted().filter { claudeAuthKeys.contains($0) }) and continue to
append only a flag and the comma-joined key names
(CMUX_PRESERVE_CLAUDE_AUTH_SELECTION_ENV and
CMUX_PRESERVE_CLAUDE_AUTH_SELECTION_ENV_KEYS) without ever reading or including
selected[key] for those sensitive keys.
In `@Sources/Workspace.swift`:
- Around line 8869-8876: In setSurfaceResumeBinding(_ binding:
SurfaceResumeBindingSnapshot, panelId: UUID) reject startupInput that is empty
or only whitespace: check binding.startupInput?.trimmingCharacters(in:
.whitespacesAndNewlines).isEmpty and treat that as nil (return false) instead of
storing; ensure surfaceResumeBindingsByPanelId[panelId] is only set when the
trimmed startupInput is non-empty and terminalPanel(for:) exists so
whitespace-only strings don't suppress scrollback replay.
🪄 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: 4c45e985-0bc8-436d-94af-a9c70fd39528
📒 Files selected for processing (6)
CLI/cmux.swiftSources/TabManager.swiftSources/Workspace.swiftcmuxTests/AgentSessionAutoResumeSettingsTests.swiftcmuxTests/CLINotifyProcessIntegrationRegressionTests.swiftcmuxTests/SessionPersistenceTests.swift
Comment on lines
+1024
to
+1027
| static func load(fileManager: FileManager = .default) -> SurfaceResumeBindingIndex { | ||
| let detectedBindings = processDetectedTmuxBindings(fileManager: fileManager) | ||
| return SurfaceResumeBindingIndex(bindingsByPanel: detectedBindings.mapValues(\.binding)) | ||
| } |
Contributor
There was a problem hiding this comment.
load() blocks the main thread
SurfaceResumeBindingIndex.load() always calls processDetectedTmuxBindings, which performs a live process-table scan (CmuxTopProcessSnapshot.capture) plus per-process processArgumentsAndEnvironment syscalls. Every saveSessionSnapshot(includeScrollback:) call that does not supply a pre-loaded index (lines 1501, 1561, 1582, 1587, 2810, 3302, 3315–3317, 3954, 14103 in AppDelegate) will invoke this synchronously from @MainActor via the ?? SurfaceResumeBindingIndex.load() fallback at line 3770 of AppDelegate.swift. That includes the applicationShouldTerminate and applicationWillResignActive handlers, where blocking the main thread causes visible quit stalls and UI hangs on every app-resign. Unlike RestorableAgentSessionIndex.load() (which reads from a persisted file), this sync path always performs live process scanning, making the new fallback materially heavier than the existing pattern it mirrors.
Comment on lines
+19527
to
+19556
| private func agentSurfaceResumeEnvironmentParts( | ||
| kind: String, | ||
| environment: [String: String]? | ||
| ) -> [String] { | ||
| guard let environment else { return [] } | ||
| let selected = selectedAgentLaunchEnvironment(from: environment) | ||
| guard !selected.isEmpty else { return [] } | ||
|
|
||
| let claudeAuthKeys: Set<String> = [ | ||
| "ANTHROPIC_API_KEY", | ||
| "ANTHROPIC_AUTH_TOKEN", | ||
| "ANTHROPIC_BASE_URL", | ||
| "ANTHROPIC_MODEL", | ||
| "ANTHROPIC_SMALL_FAST_MODEL", | ||
| "CLAUDE_CODE_USE_BEDROCK", | ||
| "CLAUDE_CODE_USE_VERTEX", | ||
| "CLAUDE_CONFIG_DIR" | ||
| ] | ||
| var parts = selected.keys.sorted().compactMap { key in | ||
| selected[key].map { "\(key)=\($0)" } | ||
| } | ||
| if kind == "claude" { | ||
| let preservedClaudeKeys = selected.keys.sorted().filter { claudeAuthKeys.contains($0) } | ||
| if !preservedClaudeKeys.isEmpty { | ||
| parts.append("CMUX_PRESERVE_CLAUDE_AUTH_SELECTION_ENV=1") | ||
| parts.append("CMUX_PRESERVE_CLAUDE_AUTH_SELECTION_ENV_KEYS=\(preservedClaudeKeys.joined(separator: ","))") | ||
| } | ||
| } | ||
| return parts | ||
| } |
Contributor
There was a problem hiding this comment.
agentSurfaceResumeEnvironmentParts calls selectedAgentLaunchEnvironment, which can return ANTHROPIC_API_KEY, ANTHROPIC_AUTH_TOKEN, and related secrets present in the agent's launch environment. Every selected key is interpolated as "\(key)=\($0)" into parts and, after shell-quoting, becomes part of the resume command stored in SurfaceResumeBindingSnapshot.command. That string flows into SessionTerminalPanelSnapshot.resumeBinding and is serialized to the session snapshot JSON on disk. Any process or backup that reads the snapshot file gains access to the credentials. The CMUX_PRESERVE_CLAUDE_AUTH_SELECTION_ENV_KEYS metadata added below this block confirms that auth keys are expected to be present in selected. A safer design stores only the key names in the persisted binding and resolves values at restore time from the keychain or current environment.
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:188">
P2: Use the same resume-binding resolution strategy in autosave fingerprinting as session snapshots. Snapshot generation now applies `effectiveSurfaceResumeBinding(...)`, but fingerprinting still follows a simpler fallback path, which can miss real snapshot changes and prevent updated resume bindings from being saved.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Comment on lines
+14180
to
+14183
| let resumeBinding = effectiveSurfaceResumeBinding( | ||
| panelId: panelId, | ||
| surfaceResumeBindingIndex: SurfaceResumeBindingIndex.load() | ||
| ) |
Contributor
There was a problem hiding this comment.
SurfaceResumeBindingIndex.load() performs a live process-table scan (CmuxTopProcessSnapshot.capture + per-process processArgumentsAndEnvironment syscalls) synchronously from @MainActor inside BonsplitDelegate.didCloseTab when isDetaching == true. This path fires on every panel drag-to-split and detach operation. Unlike the saveSessionSnapshot fallback in AppDelegate, this is an interactive UI path: users splitting panels will block the main thread for however long the process scan takes, causing visible UI stalls or frame drops on a busy machine.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
CLI/cmux.swift (2)
19561-19579:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftClaude auth secrets are still persisted in the resume binding.
resolvedstarts as the fullselectedenvironment, so keys likeANTHROPIC_API_KEYandANTHROPIC_AUTH_TOKENstill get sent viasurface.resume.set. That moves the leak fromcommandinto the persistedenvironment, which can still end up in snapshots and JSON/RPC output. Strip the Claude auth value pairs here and keep only the preservation markers / selected-key names.Suggested direction
var resolved = selected if kind == "claude" { let preservedClaudeKeys = selected.keys.sorted().filter { claudeAuthKeys.contains($0) } if !preservedClaudeKeys.isEmpty { + for key in preservedClaudeKeys { + resolved.removeValue(forKey: key) + } resolved["CMUX_PRESERVE_CLAUDE_AUTH_SELECTION_ENV"] = "1" resolved["CMUX_PRESERVE_CLAUDE_AUTH_SELECTION_ENV_KEYS"] = preservedClaudeKeys.joined(separator: ",") } }As per coding guidelines: “user-facing … command output … must not expose … environment variables … credentials, tokens”.
🤖 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 `@CLI/cmux.swift` around lines 19561 - 19579, The current logic builds resolved from the full selected environment so Claude auth values (e.g., keys in claudeAuthKeys) remain persisted; change the code in the kind == "claude" branch to remove/omit those sensitive keys from resolved (or construct resolved as selected minus claudeAuthKeys) while still adding the two preservation markers ("CMUX_PRESERVE_CLAUDE_AUTH_SELECTION_ENV" and "CMUX_PRESERVE_CLAUDE_AUTH_SELECTION_ENV_KEYS" using preservedClaudeKeys.joined), ensuring no ANTHROPIC_* or CLAUDE_* secret values are retained when calling surface.resume.set or persisting environment.
4758-4779:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop parsing flags after
--.
parseOptionstill runs overtarget.remaining, so argv flags placed after the delimiter are consumed as cmux options.cmux surface resume set -- tool --name demowill persist onlytoolinstead oftool --name demo.Suggested fix
case "set": let target = try surfaceResumeTarget(rest, client: client, windowOverride: windowOverride) var params = target.params - let (name, rem1) = parseOption(target.remaining, name: "--name") + let delimiterIndex = target.remaining.firstIndex(of: "--") + let optionArgs = delimiterIndex.map { Array(target.remaining[..<$0]) } ?? target.remaining + let argvArgs = delimiterIndex.map { Array(target.remaining[$0...]) } ?? [] + + let (name, rem1) = parseOption(optionArgs, name: "--name") let (kind, rem2) = parseOption(rem1, name: "--kind") let (checkpoint, rem3) = parseOption(rem2, name: "--checkpoint") let (checkpointID, rem4) = parseOption(rem3, name: "--checkpoint-id") let (source, rem5) = parseOption(rem4, name: "--source") let (cwd, rem6) = parseOption(rem5, name: "--cwd") let (shellCommand, rem7) = parseOption(rem6, name: "--shell") @@ if let shellCommand { commandText = shellCommand.trimmingCharacters(in: .whitespacesAndNewlines) } else { - let argv = rem7.first == "--" ? Array(rem7.dropFirst()) : rem7 + let rawArgv = argvArgs.isEmpty ? rem7 : argvArgs + let argv = rawArgv.first == "--" ? Array(rawArgv.dropFirst()) : rawArgv guard !argv.isEmpty else { throw CLIError(message: "surface resume set requires --shell <command> or -- <argv...>") } commandText = argv.map(cliShellQuote).joined(separator: " ") }🤖 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 `@CLI/cmux.swift` around lines 4758 - 4779, The parser is consuming flags after the "--" delimiter because parseOption is applied to the full target.remaining; fix this by splitting target.remaining at the first "--" and only running the series of parseOption calls (the let (name, rem1) = parseOption... chain) against the slice before the delimiter, then treat the slice after the delimiter as rem7/argv directly (so argv = remAfterDelimiter rather than rem7 produced by parseOption). Update the code paths that reference rem7/argv (and the guard that throws) to use the post-"--" slice and ensure params are still set from the pre-"--" parsed results; keep function names parseOption, target.remaining, rem7, argv and existing param assignments intact.
🤖 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/SessionPersistence.swift`:
- Line 231: The session snapshot currently persists plaintext environment values
via the variable `environment` in SessionPersistence; remove storing raw
key/value pairs and instead persist an explicit non-secret allowlist or opaque
reference. Update the `environment` field (or replace it with
`environmentAllowlist` / `environmentKeysOnly`) and change the snapshot
encoding/decoding paths in SessionPersistence to: 1) only serialize allowed keys
or key names (no values), or 2) serialize an opaque reference/ID, and 3) redact
any sensitive values before any encode/stream path. Locate usage of
`environment` in SessionPersistence's encode/serialize and decode/deserialize
functions and ensure callers are adapted to consume keys-only or resolve secrets
at runtime rather than reading persisted secret values.
- Around line 272-279: normalizedEnvironment(_:) currently trims and drops
environment values which breaks semantics for empty values; change it to only
trim the key (keep using item.key.trimmingCharacters...) but do not trim or drop
the value—assign result[key] = item.value (preserving empty strings and original
whitespace), and update the guard to only reject empty keys (i.e., remove the
!value.isEmpty check).
In `@Sources/TabManager.swift`:
- Around line 7446-7463: The hash function nonisolated private static func
hashSurfaceResumeBindingSnapshot(_:into:) currently omits
SurfaceResumeBindingSnapshot.updatedAt; update it to include snapshot.updatedAt
in the hasher (e.g., combine the TimeInterval value or a normalized integer
representation) alongside the other fields so two snapshots that differ only by
updatedAt produce different fingerprints; place the combine call logically with
the other snapshot metadata (after snapshot.command or before environment) to
preserve deterministic ordering.
In `@Sources/TerminalController.swift`:
- Around line 6164-6181: The helper currently treats an explicit but invalid or
empty surface_id as absent; change it so that when v2UUID(params, "surface_id")
indicates the key is present but yields an empty/malformed/invalid UUID (e.g.,
empty string, only whitespace, or unparsable), the function returns an error
signaling invalid_params instead of falling back to the focused surface.
Specifically, inside the branch that reads explicitSurfaceId (symbol:
explicitSurfaceId from v2UUID(params, "surface_id")), validate that the value is
a non-empty, well-formed UUID before trying AppDelegate.shared?.locateSurface or
v2ResolveWorkspace; if validation fails, return the appropriate invalid_params
error result rather than nil or the fallback; preserve the existing logic that
uses fallbackTabManager and workspace.focusedPanelId only when the surface_id
key is truly absent.
In `@Sources/VaultAgentProcessScanner.swift`:
- Around line 538-802: The tmux resume parsing and binding helpers (e.g.,
SurfaceResumeBindingIndex extension, tmuxResumeBinding, tmuxResumeInvocation,
TmuxResumeInvocation, parseTmuxTopLevelArguments, appendTmuxSocketFlag,
tmuxOptionValue, tmuxHasFlag, tmuxShortFlagCluster, tmuxTailArguments,
tmuxExecutable, shellSingleQuoted, normalized) should be extracted from this
oversized Sources/VaultAgentProcessScanner.swift into a new dedicated Swift file
(e.g., TmuxResumeParser.swift) in the same module; move all private helper types
and functions above into that file, preserve their access levels and static
members on SurfaceResumeBindingIndex (or make them an internal helper type used
by SurfaceResumeBindingIndex), update imports and references so
tmuxResumeBindingForTesting and processDetectedTmuxBindings still compile, and
ensure no application logic remains in the root Sources file to keep the
original file under the size threshold.
- Around line 547-562: The tmux client/server ambiguity and short-option parsing
bug: when processing cmuxScopedProcesses() and assigning to resolved (PanelKey)
in the loop that calls tmuxResumeBinding(observed:), prefer client bindings over
server bindings by detecting client processes in isTmuxProcess (or by ranking
bindings from tmuxResumeBinding) and only overwrite an existing resolved entry
if the new binding has higher precedence (client > server) or the existing entry
is older; and fix tmuxOptionValue to stop using a blind firstIndex(of: short) on
the whole argument string — instead locate the short option only when it appears
as a real option character in an option cluster (i.e., within an arg that starts
with '-' and where the found character is not part of a previously consumed
value), validate that the found index is directly part of the option cluster (or
is immediately followed by its attached value) and fall back to the next argv
element for separate-value options; update references to isTmuxProcess,
tmuxResumeBinding, tmuxOptionValue, resolved and PanelKey when making these
changes.
- Around line 756-779: tmuxOptionValue currently uses argument.firstIndex(of:
short) which scans the whole token and can misidentify the target when
value-taking options follow; change the cluster-handling branch to
call/implement tmuxClusterValue logic (same algorithm as tmuxShortFlagCluster)
that iterates left-to-right through the option letters in the cluster, stops at
the first value-taking option (set: ["c","e","F","f","n","s","t","x","y"]), and
only returns the value for the requested short option if that short letter
appears before any value-taking boundary; update tmuxClusterValue or add the
same loop inside tmuxOptionValue so it stops at the first value-taking option
and only extracts the substring following the found short option when valid.
---
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 19561-19579: The current logic builds resolved from the full
selected environment so Claude auth values (e.g., keys in claudeAuthKeys) remain
persisted; change the code in the kind == "claude" branch to remove/omit those
sensitive keys from resolved (or construct resolved as selected minus
claudeAuthKeys) while still adding the two preservation markers
("CMUX_PRESERVE_CLAUDE_AUTH_SELECTION_ENV" and
"CMUX_PRESERVE_CLAUDE_AUTH_SELECTION_ENV_KEYS" using
preservedClaudeKeys.joined), ensuring no ANTHROPIC_* or CLAUDE_* secret values
are retained when calling surface.resume.set or persisting environment.
- Around line 4758-4779: The parser is consuming flags after the "--" delimiter
because parseOption is applied to the full target.remaining; fix this by
splitting target.remaining at the first "--" and only running the series of
parseOption calls (the let (name, rem1) = parseOption... chain) against the
slice before the delimiter, then treat the slice after the delimiter as
rem7/argv directly (so argv = remAfterDelimiter rather than rem7 produced by
parseOption). Update the code paths that reference rem7/argv (and the guard that
throws) to use the post-"--" slice and ensure params are still set from the
pre-"--" parsed results; keep function names parseOption, target.remaining,
rem7, argv and existing param assignments intact.
🪄 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: 374ab587-3da2-4692-868c-d9217ab6c61b
📒 Files selected for processing (10)
CLI/cmux.swiftSources/AppDelegate.swiftSources/RestorableAgentSession.swiftSources/SessionPersistence.swiftSources/TabManager.swiftSources/TerminalController.swiftSources/VaultAgentProcessScanner.swiftSources/Workspace.swiftcmuxTests/SessionPersistenceTests.swiftcmuxTests/WorkspaceUnitTests.swift
There was a problem hiding this comment.
2 issues found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="CLI/cmux.swift">
<violation number="1" location="CLI/cmux.swift:19389">
P2: The new environment payload path can lose exact env values during persistence (empty-string values are dropped and values are trimmed), so resume bindings may restore with a different environment than originally captured.
(Based on your team's feedback about preserving exact non-NODE_OPTIONS environment values during resume reconstruction.) [FEEDBACK_USED]</violation>
</file>
<file name="Sources/TerminalController.swift">
<violation number="1" location="Sources/TerminalController.swift:6164">
P1: Do not fall back to the focused surface when `surface_id` is present but invalid. Return an `invalid_params` error for malformed or empty IDs so bad input cannot modify the wrong surface.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 2
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)
489-495:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t serialize raw binding environments into session snapshots.
Agent-hook bindings now carry env, and this stores the whole binding in the saved session. That writes auth/session values to disk and also feeds them into autosave fingerprinting. Persist a sanitized allowlist or an opaque reference instead.
🤖 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 489 - 495, The snapshot currently embeds the full agent-hook binding (resumeBinding) which includes raw env values; instead, replace the full binding serialization with a sanitized representation or opaque reference: e.g., map resumeBinding to a BindingReference (or allowlist of safe fields) before constructing SessionTerminalPanelSnapshot and change SessionTerminalPanelSnapshot to accept that reference/allowlist rather than the full binding so env/auth secrets are never written to disk or included in autosave fingerprinting. Locate the use of resumeBinding in the terminal snapshot creation and the SessionTerminalPanelSnapshot initializer to implement the transformation and enforce the sanitized fields.
♻️ Duplicate comments (3)
CLI/cmux.swift (2)
4793-4794:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winDo not persist Claude auth values in the stored
environment.
resolvedstarts as the full selected environment, and the Claude branch only adds selector markers. That means keys likeANTHROPIC_API_KEYandANTHROPIC_AUTH_TOKENstill get written into the binding, thensurface resume show --jsonechoes them back verbatim.Suggested fix
var resolved = selected if kind == "claude" { let preservedClaudeKeys = selected.keys.sorted().filter { claudeAuthKeys.contains($0) } if !preservedClaudeKeys.isEmpty { + for key in preservedClaudeKeys { + resolved.removeValue(forKey: key) + } resolved["CMUX_PRESERVE_CLAUDE_AUTH_SELECTION_ENV"] = "1" resolved["CMUX_PRESERVE_CLAUDE_AUTH_SELECTION_ENV_KEYS"] = preservedClaudeKeys.joined(separator: ",") } }As per coding guidelines: command output must not expose "environment variables".
Also applies to: 19568-19586
🤖 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 `@CLI/cmux.swift` around lines 4793 - 4794, The code prints a JSON payload that still contains sensitive Claude auth values; before calling formatIDs/jsonString (where payload and idFormat are used) strip any anthropic/claude credentials from the payload (e.g., keys "ANTHROPIC_API_KEY" and "ANTHROPIC_AUTH_TOKEN" or any CLAUDE_* entries) and/or remove them from the resolved environment object that is merged into payload; implement this by copying payload (or resolved) to a sanitized variable, deleting those specific keys, then pass the sanitized object to formatIDs(payloadSanitized, mode: idFormat) and jsonString so the printed/stored output never contains the secret values.
4756-4780:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop parsing options after the
--argv delimiter.Lines 4758-4764 still run
parseOptionovertarget.remaining, so flag-like argv after--get consumed instead of being persisted as part of the resume command.cmux surface resume set -- tool --name demowill still drop--name demofrom the stored command.Suggested fix
case "set": let target = try surfaceResumeTarget(rest, client: client, windowOverride: windowOverride) var params = target.params - let (name, rem1) = parseOption(target.remaining, name: "--name") + let delimiterIndex = target.remaining.firstIndex(of: "--") + let optionArgs = delimiterIndex.map { Array(target.remaining[..<$0]) } ?? target.remaining + let argvArgs = delimiterIndex.map { Array(target.remaining[$0...]) } ?? [] + + let (name, rem1) = parseOption(optionArgs, name: "--name") let (kind, rem2) = parseOption(rem1, name: "--kind") let (checkpoint, rem3) = parseOption(rem2, name: "--checkpoint") let (checkpointID, rem4) = parseOption(rem3, name: "--checkpoint-id") let (source, rem5) = parseOption(rem4, name: "--source") let (cwd, rem6) = parseOption(rem5, name: "--cwd") let (shellCommand, rem7) = parseOption(rem6, name: "--shell") @@ if let shellCommand { commandText = shellCommand.trimmingCharacters(in: .whitespacesAndNewlines) } else { - let argv = rem7.first == "--" ? Array(rem7.dropFirst()) : rem7 + let rawArgv = argvArgs.isEmpty ? rem7 : argvArgs + let argv = rawArgv.first == "--" ? Array(rawArgv.dropFirst()) : rawArgv guard !argv.isEmpty else { throw CLIError(message: "surface resume set requires --shell <command> or -- <argv...>") } commandText = argv.map(cliShellQuote).joined(separator: " ") }🤖 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 `@CLI/cmux.swift` around lines 4756 - 4780, The bug is that parseOption is being run over target.remaining including argv after the "--" delimiter so flags after "--" get consumed; modify the logic in surfaceResumeTarget handling (where you call parseOption on target.remaining and produce rem7) to first split target.remaining at the first occurrence of "--" into two arrays (optionsPart and argvPart), run the series of parseOption calls only over optionsPart, and then set rem7 (used to build commandText) to argvPart (or prepend "--" if you want the existing -- handling logic) so anything after the "--" is preserved as argv rather than parsed as options.Sources/Workspace.swift (1)
463-470:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the auto-resume gate before suppressing snapshot scrollback.
When
resumeBinding.source == "agent-hook"and auto-resume is disabled,createPanel(...)later drops the startup input, but this snapshot path still suppresses scrollback. That restore path ends up with neither replayed output nor a resume command.Suggested fix
+ let autoResumeAgentSessions = AgentSessionAutoResumeSettings.isEnabled() + let replayResumeBinding = resumeBinding?.source == "agent-hook" && !autoResumeAgentSessions + ? nil + : resumeBinding let shouldPersistScrollback = Self.shouldPersistSessionScrollback( shellActivityState: panelShellActivityStates[panelId], fallbackNeedsConfirmClose: terminalPanel.needsConfirmClose() ) && Self.shouldReplaySessionScrollback( restorableAgent: effectiveRestorableAgent, tmuxStartCommand: restorableTmuxStartCommand, - resumeBinding: resumeBinding + resumeBinding: replayResumeBinding )🤖 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 463 - 470, The scrollback suppression check currently combines shouldPersistSessionScrollback and shouldReplaySessionScrollback without accounting for the auto-resume gate, which causes agent-hook resumes to drop startup input while still suppressing snapshot scrollback; update the logic that computes shouldPersistScrollback so it first applies the auto-resume gate for resumeBinding (i.e. require auto-resume to be enabled when resumeBinding.source == "agent-hook") before calling or combining with shouldReplaySessionScrollback, or short-circuit to preserve scrollback when auto-resume is disabled; adjust the use sites (references: shouldPersistSessionScrollback, shouldReplaySessionScrollback, resumeBinding, createPanel) accordingly so snapshot path won’t suppress scrollback when agent-hook resume is not allowed.
🤖 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 10303-10326: The usage text for the "surface resume" subcommands
advertises only "show" while the code accepts both "show" and the alias "get";
update the returned multi-line string in the "case \"surface\",
\"surface-resume\":" branch to list "get" alongside "show" (e.g., "cmux surface
resume show|get") and add "get" to any top-level command listing in that same
usage block so the help output matches the implemented aliases (update the
triple-quoted string returned by that case).
In `@cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift`:
- Around line 1087-1095: The test currently only unwraps the first
resume-binding publish (resumeBindingRequests.first) which allows duplicates to
pass; update the assertion sequence by asserting resumeBindingRequests.count ==
1 before unwrapping so any duplicate "surface.resume.set" publishes fail, then
continue to XCTUnwrap(resumeBindingRequests.first) and assert the checkpoint_id
equals sessionId; reference the resumeBindingRequests variable, the
jsonObject(command) filtering, state.commands collection, and the existing
request/sessionId assertions when making this change.
---
Outside diff comments:
In `@Sources/Workspace.swift`:
- Around line 489-495: The snapshot currently embeds the full agent-hook binding
(resumeBinding) which includes raw env values; instead, replace the full binding
serialization with a sanitized representation or opaque reference: e.g., map
resumeBinding to a BindingReference (or allowlist of safe fields) before
constructing SessionTerminalPanelSnapshot and change
SessionTerminalPanelSnapshot to accept that reference/allowlist rather than the
full binding so env/auth secrets are never written to disk or included in
autosave fingerprinting. Locate the use of resumeBinding in the terminal
snapshot creation and the SessionTerminalPanelSnapshot initializer to implement
the transformation and enforce the sanitized fields.
---
Duplicate comments:
In `@CLI/cmux.swift`:
- Around line 4793-4794: The code prints a JSON payload that still contains
sensitive Claude auth values; before calling formatIDs/jsonString (where payload
and idFormat are used) strip any anthropic/claude credentials from the payload
(e.g., keys "ANTHROPIC_API_KEY" and "ANTHROPIC_AUTH_TOKEN" or any CLAUDE_*
entries) and/or remove them from the resolved environment object that is merged
into payload; implement this by copying payload (or resolved) to a sanitized
variable, deleting those specific keys, then pass the sanitized object to
formatIDs(payloadSanitized, mode: idFormat) and jsonString so the printed/stored
output never contains the secret values.
- Around line 4756-4780: The bug is that parseOption is being run over
target.remaining including argv after the "--" delimiter so flags after "--" get
consumed; modify the logic in surfaceResumeTarget handling (where you call
parseOption on target.remaining and produce rem7) to first split
target.remaining at the first occurrence of "--" into two arrays (optionsPart
and argvPart), run the series of parseOption calls only over optionsPart, and
then set rem7 (used to build commandText) to argvPart (or prepend "--" if you
want the existing -- handling logic) so anything after the "--" is preserved as
argv rather than parsed as options.
In `@Sources/Workspace.swift`:
- Around line 463-470: The scrollback suppression check currently combines
shouldPersistSessionScrollback and shouldReplaySessionScrollback without
accounting for the auto-resume gate, which causes agent-hook resumes to drop
startup input while still suppressing snapshot scrollback; update the logic that
computes shouldPersistScrollback so it first applies the auto-resume gate for
resumeBinding (i.e. require auto-resume to be enabled when resumeBinding.source
== "agent-hook") before calling or combining with shouldReplaySessionScrollback,
or short-circuit to preserve scrollback when auto-resume is disabled; adjust the
use sites (references: shouldPersistSessionScrollback,
shouldReplaySessionScrollback, resumeBinding, createPanel) accordingly so
snapshot path won’t suppress scrollback when agent-hook resume is not allowed.
🪄 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: b4f5802a-522b-43d1-8ec1-a3b3f5f38443
📒 Files selected for processing (6)
CLI/cmux.swiftSources/RestorableAgentSession.swiftSources/Workspace.swiftcmuxTests/CLINotifyProcessIntegrationRegressionTests.swiftcmuxTests/SessionPersistenceTests.swiftcmuxTests/WorkspaceUnitTests.swift
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift">
<violation number="1" location="cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift:1094">
P3: Assert the expected publish count before unwrapping the first request so this regression test fails when duplicate `surface.resume.set` calls are emitted.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
This was referenced Jul 2, 2026
This was referenced Jul 10, 2026
Merged
This was referenced Jul 19, 2026
This was referenced Jul 28, 2026
This was referenced Aug 9, 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
cmux surface resume set/show/clear.Verification
git diff --check HEAD~1..HEAD./scripts/reload.sh --tag surfresNote
Medium Risk
Adds new persisted resume-command data to session snapshots plus new socket/CLI pathways that can trigger command execution on restore, which is safety-sensitive despite added sanitization/approval gating. Changes also touch autosave/restore timing and process-detection paths, increasing regression risk in session persistence.
Overview
Adds a new per-terminal-surface “resume binding” model that is persisted in session snapshots and included in autosave fingerprints, enabling restored terminals to carry a stored resume command (with optional cwd/env metadata).
Exposes this via new V2 socket methods (
surface.resume.set|get|clear) and a public CLI surface (cmux surface resume …/surface-resume), including argument parsing/validation, window/workspace/surface targeting changes, and special routing to avoid focusing for resume commands.Extends agent hooks and process detection to publish/clear resume bindings (including tmux-derived resumes), and adds a signed approval store + new Settings search strings/localizations/documentation updates for managing which command prefixes may auto-run.
Reviewed by Cursor Bugbot for commit b9df8d0. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds per-surface resume bindings so restores run a safe resume command instead of replaying scrollback. Tightens trust and restore gating; only trusted or user‑approved commands auto‑run, including trusted agent‑hook resumes.
New Features
resume_binding.surface.resume.set|get|clearandcmux surface resume ...; strict window‑scoped targeting, honorstab_id, routes explicit targets globally without focusing, and validates options/shell before socket connect (incl. long startup input).tmux, agent hooks) or approved commands can auto‑run.TmuxResumeParseremits safe foreground‑jobtmux attachcommands, preserving-L/-Sand socket env.Bug Fixes
tmux; keep agent‑hook resume states trusted; enforce window scope; honortab_id; global routing for explicit targets; refine fallback ordering; reject malformed targets/options.tmuxbindings; clear stale restored/process‑detected bindings; clear on panel close and agent session end; detached transfers drop process‑detected bindings; respect restore scopes/targets; guard races.codex-teams resume <sessionId>and argument order; harden restore gating so bindings only run when allowed.Written for commit b9df8d0. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
surface resumeCLI (set,show/get,clear) with help/examples;surface.listnow shows resume bindingsPersistence / Restore
Detection
Tests