Repository navigation
Conversation
…uilder U1: UncleanShutdownSentinel detects whether the prior run exited cleanly via a sentinel file under the cmux state dir (XDG-aware), fail-safe to 'clean'. U3: ResumeBreadcrumbBuilder builds the sanitized, single-line 'pick up where we left off' prompt anchored on the workspace name; supports Claude Code + Codex. Both fully unit-tested (14 tests) and wired into the cmuxTests target.
…(U4 core) U2: CrashRecoverySettings (offerResumeAfterCrash, injectResumeBreadcrumb, resumeAgentsAfterUpdate), all default off, wired into the cmux.json terminal section alongside autoResumeAgentSessions. U4: WorkspaceResumePlanner is the pure, shared decide(resume-or-skip + breadcrumb) core consumed by the offer (U5) and manual action (U6); full skip-reason matrix (unsupported agent, no/empty session, unproven binding). SkipReason made Hashable. 26 tests across 4 crash-recovery suites pass.
…tegration, U8 core) RelaunchIntent: single-use clean+restore-intended marker for intentional relaunches. CrashRecoveryLaunchState: captures crash vs clean vs update-relaunch once at launch, arming this run's sentinel after reading prior markers; gates the offer on crash + opt-in. AppDelegate: capture at didFinishLaunching (skipped under XCTest), markCleanExit on willTerminate, markIntentionalRelaunch in updaterWillRelaunchApplication (Sparkle update => restore-intent, never misread as a crash). 11 tests; 37 total crash-recovery tests pass.
…ver (U8 R11) Adds a secondary line under 'Update Available' so clicking Install & Relaunch no longer feels like it will lose your windows. Localized en + ja.
…re guarantee) shouldAttemptRestore gains a restoreIntended override: a Sparkle/Rosetta relaunch restores all windows even when launched with arguments; CMUX_DISABLE_SESSION_RESTORE and automated-test guards still win. Wired at the startup-snapshot gate via the captured launch state. 3 tests.
… (U4 delivery core) WorkspaceResumeCoordinator maps a live ResumableWorkspaceSurface into the pure planner decision, then runs native resume (when the agent isn't live) and delivers the breadcrumb. Outcome reported (resumed/skipped) so callers can surface reasons. Unit-tested with a fake surface (7 tests): live->breadcrumb only, dead->resume+breadcrumb, inject gating, skip paths never touch the surface.
… (U4 delivery bridge) Thin same-file glue mapping the focused terminal panel's resume binding into the coordinator: agent kind/command/proven from surfaceResumeBinding, live-agent via the focused surface, breadcrumb delivery via the private sendInputWhenReady. Exposes resumeWhereWeLeftOff()/canResumeWhereWeLeftOff(). Compiles against the full app.
WorkspaceResumeCommands shared command (cmux-shared-behavior) on the dispatcher; View-menu entry that resumes the selected workspace's agent + injects the breadcrumb, disabled when not resumable. Localized en + ja. Compiles against the full app.
CrashRecoveryOfferPresenter shows a Chrome-style 'resume where you left off?' NSAlert after the first restore when the prior run crashed AND the user opted in, then resumes all resumable workspaces on accept. Pure offer-text builder (CrashRecoveryOfferText) tested; gate keeps normal + update-relaunch launches silent. Localized en + ja.
Ties launch classification -> opt-in gate -> planner partition -> coordinator delivery over a mixed fleet (live Claude, dead Codex, no-session, unsupported, unproven): resumes the 2 eligible, skips the 3 with reasons, dead agent gets native resume, breadcrumbs name-anchored. Plus clean-quit/no-offer and update-relaunch/ restore-but-no-offer paths. App-host E2E validated by launching the tagged build.
…(U8 resumeAgentsAfterUpdate) Wires the resumeAgentsAfterUpdate opt-in: after an intentional relaunch, resume agents with no prompt (windows always restore regardless). Distinct from the crash offer.
…ted bindings Live force-quit test revealed the offer skipped a restored Claude workspace: a cold-restored agent populates restoredAgentSnapshotsByPanelId (kind/sessionId/ resumeCommand) + restoredAgentResumeStatesByPanelId, NOT surfaceResumeBindingsByPanelId which the conformance read. Now prefers the restored-agent snapshot (proven, synchronous during restore -> no async race), derives live-vs-cold from the resume state, and runs the restored agent's resumeStartupInput on cold resume.
…agent-first recovery Re-centers crash recovery on the real defect found in live testing: no durable window↔session binding (only 3/20 windows had a session ID captured by hooks), so names mis-attribute and restored agents come up fresh. Agent-first recovery, deliberately differentiated from session-search/hibernation tooling. U9–U14.
Pure ResumeFidelityGate over a ResumeBindingFacts value type: a restored window↔session binding is verified only when a binding exists, the agent is supported, a session id + constructable resume command are present, and the transcript exists at *this window's own cwd*. Transcript-at-cwd doubles as the cwd-match check for cwd-namespaced agents; a transcript found only elsewhere is .cwdMismatch (anti-Example-3), none on disk is .transcriptMissing. 13 tests cover the full verified/unverified matrix and ordered precedence.
…U12 R15)
Extend ResumeBreadcrumbBuilder with VerifiedResumeAnchor + breadcrumb(forVerified:):
when a verified binding carries a transcript path, the breadcrumb names that
exact file ('your prior transcript is at <path> - review that file') so the
restored agent reconstructs from the right source instead of grepping every
transcript (Examples 2/3). No path -> summary-only nudge. breadcrumbIfVerified
gates on the BindingVerdict so an unverified window produces no breadcrumb (R15).
sanitizedPath keeps internal spaces, strips control chars/quotes, expands tilde,
bounds length. 9 new tests; v1 builder tests still green.
RecoveryRouter turns binding facts into a binary, agent-first outcome: a verified binding resumes its exact session + the transcript-anchored breadcrumb; an unverified one yields ResumeBreadcrumbBuilder.honestRecoveryPrompt — an honest, cwd-scoped prompt that names no session, tells the agent to reconstruct only if confident (else ask), and forbids adopting another window's session. There is no third 'pick from a list' branch (R17). wouldAutoResume gates the silent path so it never auto-resumes an unverified binding (R14). WorkspaceResumeCoordinator gains a verification-gated recover() path alongside the v1 proven-binding resume()/canResume() (U5 offer / U6 manual untouched). The ResumableWorkspaceSurface protocol gains additive, defaulted verification facts so existing conformers keep compiling and an unwired surface conservatively routes to honest recovery rather than a blind resume. 8 router + 5 coordinator recover() tests; all v1 coordinator tests still green.
…U14 R18) scripts/crash-recovery-e2e.sh scripts the safe force-quit -> relaunch cycle and asserts the R18 bar (a restored window recovers its own work). It acts ONLY on a tagged, bundle-isolated Debug build: refuses without --tag, refuses any tag that resolves to the main app or a non-debug bundle, and forcequit kills only PIDs whose exec path is under this tag's DerivedData Debug dir (never killall/pkill, never the main app). Commands: build/launch/bindings/snapshot/forcequit/relaunch/ verify/guard-selftest. guard-selftest passes (refuses main + non-debug ids). U14-acceptance.md documents the procedure + the empirical items the live loop must pin (U9 hook coverage, --resume rehydration, U13 name revert, Workspace verification adapter).
…sts (U13 R16, U9 R12) U13: RestoredNameResolver encodes the name-fidelity rule (KTD14) as a pure decision — a .user title is always kept; an .auto summary is re-applied ONLY when the binding verified; otherwise the window shows a neutral name and never wears another session's summary (anti-Example-1). Absent provenance decodes as .user (matches the snapshot decoder). 7 tests. The empirical 'reverts to Claude Code on restore' wiring is a U14 live-loop item. U9: WindowSessionBindingTests pin R12's consumption contract at the coordinator seam — a panel's facts carry its OWN session/cwd/kind, two concurrent panels produce distinct non-crossed bindings, and a no-agent pane yields hasBinding=false (-> honest recovery, not a guess). The hook-capture coverage defect (only some panes recording a session) is pinned by the U14 live loop.
…guard, honest docs
- sanitizedPath now strips Unicode line/paragraph separators U+2028/U+2029
(category Zl/Zp, NOT in controlCharacters) which some agents treat as a line
break and would submit the injected prompt early. sanitizedName got this free
via its whitespace split; the path-preserving variant needed it explicit. +test.
- crash-recovery-e2e.sh: replace the buggy 'awk /txt/{next}' exec-path filter
(a no-op that also substring-dropped any tag containing 'txt', silently
no-op'ing the crash sim) with sed -n 's/^n//p' + grep -F via shared
resolve_exec_path/is_tagged_exec_path helpers; forcequit now does a REAL
per-PID exec-path re-check before kill -9 (the comment had claimed a guard that
didn't exist), defending against PID recycling. Guard self-test still passes.
- Make doc comments honest about pending live wiring: the gate referenced a
nonexistent ResumeBindingFactsResolver; the router/coordinator implied recover()
is already on the live restore path. It is consumed only via tests until the
U14 step wires it + the real Workspace supplies on-disk facts.
- U14-acceptance.md: document the adapter must-dos (feed the bare session id, not
the resume command, into bindingFacts; transcript-existence helpers are private
in RestorableAgentSession).
…ai#6741 + manaflow-ai#6631 CEO confirmed 0.64.17 fixes auto-resume; PR manaflow-ai#6741 (open) pins the Claude auto-resume binding to the launch cwd and adds shared ClaudeResumeWorkingDirectory.verifiedWorkingDirectory + ClaudeProjectDirEncoding, and manaflow-ai#6631 (open) owns the authoritative session-tracking store. Both reshape the exact APIs the live wiring would consume. Re-scope this branch as the agent-first RECOVERY DECISION LAYER on top: keep the verify-trust gate (U10), agent-first honest recovery (U11), transcript-anchored breadcrumb (U12), and restored-name fidelity (U13) — none built by either PR. Drop the binding cwd-fix / hook-coverage scope (now owned by manaflow-ai#6741/manaflow-ai#6631). The live adapter will consume manaflow-ai#6741's verifiedWorkingDirectory instead of a hand-rolled FS check. Committed work has zero file overlap with manaflow-ai#6741, so it won't conflict; wire live after both land. See PIVOT-complement-6741-6631.md.
… (U11 live) Layers the verification-gated re-entry message on top of cmux's existing native auto-resume (whose cwd PR manaflow-ai#6741 fixes), without touching the binding store manaflow-ai#6741/ manaflow-ai#6631 reshape. - ClaudeTranscriptPresenceResolver: on-disk check splitting transcript-at-window-cwd (verified) from transcript-elsewhere-only (cwd mismatch, anti-Example-3) vs absent, across the Claude config roots. Reuses encodeClaudeProjectDir + ClaudeConfigDirectoryPath; self-contained pre-manaflow-ai#6741. 8 tests over a temp tree. - Workspace.scheduleCrashRecoveryReentry: per-panel (facts from the panel's OWN snapshot, bare session id + cwd — no focused-panel coupling, no token-as-id), fired at restore for each cold-restored agent panel, gated on a real crash / intentional-update relaunch AND the opt-in injectResumeBreadcrumb setting (default false). Verified binding -> transcript-anchored breadcrumb; unverified -> honest cwd-scoped prompt. Skips hibernation restores. Opt-in + conservative under-detection (a missed transcript falls to honest recovery, never a wrong resume), so default users are unaffected. 62 v2 tests green; build green.
…klist is self-contained
|
@mvanhorn is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
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:
📝 WalkthroughWalkthroughCrash-recovery launch classification, restore-intent markers, transcript verification, recovery routing, workspace re-entry wiring, project integration, and update popover reassurance text are added. ChangesCrash recovery, workspace resume, and update reassurance
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Possibly related PRs
Suggested reviewers
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (21 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryAdds agent-first crash/update session recovery to cmux: an unclean-shutdown sentinel classifies each launch as crash vs. intentional update vs. clean restart; restored windows run a verification gate before trusting their saved binding; verified sessions auto-resume with a transcript-anchored breadcrumb and unverified ones get an honest, cwd-scoped prompt instead of a confident wrong guess. The feature is opt-in (all three
Confidence Score: 4/5Safe to merge with one targeted fix: The crash/update recovery logic is well-structured and all previously flagged issues have been addressed. One remaining defect: Sources/CrashRecovery/ClaudeTranscriptPresence.swift — the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[App Launch] --> B[captureAtLaunch]
B --> C{priorRunCrashed?}
C -->|No sentinel| D[Clean launch — no recovery]
C -->|restoreWasIntended| E[Update relaunch]
C -->|Unclean, not intended| F[Crash detected]
E --> G{resumeAgentsAfterUpdate?}
G -->|off| D
G -->|on| H[resumeAfterIntentionalRelaunchIfNeeded]
F --> I{offerResumeAfterCrash?}
I -->|off + injectResumeBreadcrumb| J[shouldDeliverSilentReentry]
I -->|on| K[presentOfferIfNeeded alert]
K -->|user accepts| L[WorkspaceResumeCoordinator.recover]
J --> L
H --> N[verifiedResumePlans]
N --> O{ResumeFidelityGate.verify}
L --> M[prepareCrashRecoveryRecoveryVerification\nTask.detached disk scan]
M --> O
O -->|verified| P[native resume + breadcrumb]
O -->|unverified| Q[honest cwd-scoped prompt\nno auto-resume]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[App Launch] --> B[captureAtLaunch]
B --> C{priorRunCrashed?}
C -->|No sentinel| D[Clean launch — no recovery]
C -->|restoreWasIntended| E[Update relaunch]
C -->|Unclean, not intended| F[Crash detected]
E --> G{resumeAgentsAfterUpdate?}
G -->|off| D
G -->|on| H[resumeAfterIntentionalRelaunchIfNeeded]
F --> I{offerResumeAfterCrash?}
I -->|off + injectResumeBreadcrumb| J[shouldDeliverSilentReentry]
I -->|on| K[presentOfferIfNeeded alert]
K -->|user accepts| L[WorkspaceResumeCoordinator.recover]
J --> L
H --> N[verifiedResumePlans]
N --> O{ResumeFidelityGate.verify}
L --> M[prepareCrashRecoveryRecoveryVerification\nTask.detached disk scan]
M --> O
O -->|verified| P[native resume + breadcrumb]
O -->|unverified| Q[honest cwd-scoped prompt\nno auto-resume]
Reviews (12): Last reviewed commit: "fix(crash-recovery): cover schema and ar..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?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: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/ClaudeTranscriptPresenceTests.swift`:
- Around line 21-24: The temp-directory helper in ClaudeTranscriptPresenceTests
uses abs(hashValue), which can trap on Int.min and fail the suite
nondeterministically. Update the unique directory name generation in the
affected helper to use a guaranteed-safe unique value such as UUID().uuidString
or ProcessInfo.processInfo.globallyUniqueString, and keep the cleanup logic
around dir unchanged.
In `@scripts/crash-recovery-e2e.sh`:
- Around line 250-253: The fixed delay in cmd_relaunch is timing-dependent and
should be replaced with an owner/readiness check before calling cmd_launch.
Update the cmd_relaunch flow to wait for the prior instance/socket cleanup
condition to be satisfied after cmd_forcequit, using the existing harness
ownership checks or a deterministic readiness probe instead of sleep. Keep the
change localized to cmd_relaunch in the crash-recovery e2e shell script and
remove the wall-clock synchronization entirely.
In `@Sources/CmuxSettingsJSONPathSupport.swift`:
- Around line 172-186: The new crash-recovery JSON keys are added in
booleanSettings but not included in supportedSettingsJSONPaths, so the validator
still won’t recognize them. Update the supported settings path list in
CmuxSettingsJSONPathSupport to add terminal.offerResumeAfterCrash,
terminal.injectResumeBreadcrumb, and terminal.resumeAgentsAfterUpdate using the
same JSON path mapping pattern as the existing crash-recovery entries.
In `@Sources/CrashRecovery/CrashRecoveryLaunchState.swift`:
- Around line 88-92: Remove the test-only resetForTesting seam from
CrashRecoveryLaunchState and keep the production type free of …ForTesting APIs.
Delete the resetForTesting() method from CrashRecoveryLaunchState, and rely on
fresh CrashRecoveryLaunchState instances in the tests via `@testable` import
instead of exposing a production reset hook.
In `@Sources/CrashRecovery/CrashRecoveryOffer.swift`:
- Around line 20-26: Use ICU pluralization for the crash recovery offer text
instead of the hardcoded workspace(s) suffix. Update the localization entry
referenced by CrashRecoveryOffer’s message key crashRecovery.offer.message to
provide one/other variants in the string catalog, and change the
CrashRecoveryOffer message formatting to use the localized pluralized string
with resumableCount rather than building a single %lld workspace(s)? template.
Ensure the new wording is driven entirely by localization so it pluralizes
correctly across locales.
In `@Sources/CrashRecovery/RelaunchIntent.swift`:
- Around line 54-64: The consumeRestoreIntent flow currently trusts any existing
path and ignores deletion failures, so make it fail closed by verifying the
marker is a real file before treating it as intent and only returning true after
successfully removing it. Update RelaunchIntent.consumeRestoreIntent to inspect
the URL from markerURL with a reliable file-attribute check, treat directories
or unexpected paths as invalid, and if removeItem(at:) fails return false so
stale markers do not keep suppressing crash recovery.
In `@Sources/CrashRecovery/RestoredNameResolver.swift`:
- Around line 40-47: In RestoredNameResolver’s restore/title handling, stop
defaulting a missing provenance source to .user because source == nil must fail
closed instead of preserving an unverified legacy title. Update the logic around
the effectiveSource check so the “keep user title” path only applies when
provenance is explicitly stamped as .user, and treat nil source as .neutral
unless a prior migration has set a real source.
In `@Sources/CrashRecovery/ResumeBreadcrumbBuilder.swift`:
- Around line 57-65: Localize all user-facing recovery copy in
ResumeBreadcrumbBuilder instead of leaving hard-coded English text. Update
breadcrumb(workspaceName:agent) and the other recovery prompt builders
referenced in this change set to use String(localized:defaultValue:) with stable
localization keys, and add matching Resources/Localizable.xcstrings entries for
every supported locale. Keep the existing behavior and interpolation, but move
every visible prompt string through localization so the crash-recovery flow
respects the app locale.
In `@Sources/CrashRecovery/ResumeFidelityGate.swift`:
- Around line 15-157: Mark the pure value types in ResumeFidelityGate.swift as
explicitly nonisolated so they do not inherit incidental `@MainActor` isolation in
Swift 6. Update ResumeBindingFacts, BindingVerdict, UnverifiedReason, and
ResumeFidelityGate themselves, and ensure the methods isSupported(_:),
verify(_:), and isVerified(_:) remain usable as cross-concurrency sendable
helpers without actor isolation.
In `@Sources/CrashRecovery/WorkspaceResumeCoordinator.swift`:
- Around line 141-153: The bindingFacts(for:) method is passing the raw
resumeSessionToken straight into ResumeBindingFacts.sessionId, which later
reaches ClaudeTranscriptPresenceResolver.resolve and breaks transcript lookup.
Update bindingFacts(for:) to extract a bare filename-safe session ID from the
token before assigning sessionId, while keeping hasToken detection based on the
full token presence. Use the existing symbols
ResumableWorkspaceSurface.resumeSessionToken, ResumeBindingFacts.sessionId, and
ClaudeTranscriptPresenceResolver.resolve to locate and align the fix.
In `@Sources/Workspace.swift`:
- Around line 12972-12988: The restore path in Workspace.swift is doing an
expensive synchronous transcript lookup by calling
ClaudeTranscriptPresenceResolver.resolve(...) for every restored agent panel,
which can block startup on large histories. Move this presence check off the hot
restore path by caching the result, precomputing it once per launch, or
resolving it asynchronously before restoration, and then reuse that value in the
agent.kind == .claude branch instead of rescanning projects/ repeatedly.
- Around line 1426-1430: Gate the crash-reentry injection in Workspace’s restore
flow on an actual agent resume, not just on `restoredHibernation == nil`. Update
the `scheduleCrashRecoveryReentry(panel:agent:)` call so it only runs when the
panel is on the true startup/resume path already identified by
`restoredAgentWillRunStartupCommand || restoredAgentWillRunStartupInput`,
preventing the breadcrumb/prompt from being injected into a plain restored
shell.
🪄 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: 7eda42d2-ad94-4fdf-a2fc-7dce36ca4778
📒 Files selected for processing (38)
Packages/macOS/CmuxUpdaterUI/Sources/CmuxUpdaterUI/UpdatePopoverView.swiftResources/Localizable.xcstringsSources/AppDelegate.swiftSources/CmuxSettingsJSONPathSupport.swiftSources/CrashRecovery/ClaudeTranscriptPresence.swiftSources/CrashRecovery/CrashRecoveryLaunchState.swiftSources/CrashRecovery/CrashRecoveryOffer.swiftSources/CrashRecovery/CrashRecoverySettings.swiftSources/CrashRecovery/RecoveryRouter.swiftSources/CrashRecovery/RelaunchIntent.swiftSources/CrashRecovery/RestoredNameResolver.swiftSources/CrashRecovery/ResumeBreadcrumbBuilder.swiftSources/CrashRecovery/ResumeFidelityGate.swiftSources/CrashRecovery/UncleanShutdownSentinel.swiftSources/CrashRecovery/WorkspaceResumeCoordinator.swiftSources/CrashRecovery/WorkspaceResumePlanner.swiftSources/SessionPersistence.swiftSources/Workspace.swiftSources/WorkspaceActionDispatcher.swiftSources/cmuxApp.swiftcmux.xcodeproj/project.pbxprojcmuxTests/ClaudeTranscriptPresenceTests.swiftcmuxTests/CrashRecoveryFlowTests.swiftcmuxTests/CrashRecoveryLaunchStateTests.swiftcmuxTests/CrashRecoveryOfferTests.swiftcmuxTests/CrashRecoveryRestoreGateTests.swiftcmuxTests/CrashRecoverySettingsTests.swiftcmuxTests/RecoveryRouterTests.swiftcmuxTests/RelaunchIntentTests.swiftcmuxTests/RestoredWorkspaceNameFidelityTests.swiftcmuxTests/ResumeBreadcrumbAnchorTests.swiftcmuxTests/ResumeBreadcrumbBuilderTests.swiftcmuxTests/ResumeFidelityGateTests.swiftcmuxTests/UncleanShutdownSentinelTests.swiftcmuxTests/WindowSessionBindingTests.swiftcmuxTests/WorkspaceResumeCoordinatorTests.swiftcmuxTests/WorkspaceResumePlannerTests.swiftscripts/crash-recovery-e2e.sh
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
Sources/Workspace+RestoredNameFidelity.swift (1)
104-112: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDon’t mark unverified auto titles as user-owned.
For Claude/Codex, initial restore often has no cached filesystem verification, so
.autotitles resolve to.neutral. These branches then set the source to.user, which blocks the async verification refresh from later applying the verified auto title (scheduleRestoredAgentVerificationRefreshchecks the source is not.user). Preserve the original auto/unverified provenance instead of converting it to a user title.Also applies to: 127-135
🤖 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`+RestoredNameFidelity.swift around lines 104 - 112, `applyRestoredWorkspaceName(_:)` is converting unverified auto-restored titles into user-owned titles by calling `setCustomTitle(..., source: .user)` for both `.keepUserTitle` and `.neutral`. Update this flow to preserve the original provenance from `RestoredName` in `Workspace+RestoredNameFidelity` so auto/unverified titles remain non-user sourced and can still be refreshed later by `scheduleRestoredAgentVerificationRefresh`; keep verified auto titles as `.auto`, and only use `.user` for actual user-authored titles.Sources/Workspace.swift (1)
1224-1288: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDon’t block update relaunch auto-resume on the no-scan verifier.
On intended update relaunches,
shouldGateAgentStartupForCrashRecoverybecomes true when update auto-resume is enabled, butcrashRecoveryVerificationWithoutFilesystemScanreturnsnilfor Claude/Codex. That makesrestorableAgentStartupAllowedfalse and there is no later silent re-entry repair becauseshouldDeliverSilentReentryreturns false for intended restores. Default Claude/Codex update relaunches can therefore restore shells without resuming agents.Also applies to: 1470-1478
🤖 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 1224 - 1288, The update relaunch auto-resume path is being blocked by the crash-recovery no-scan verifier for Claude/Codex, so restore intent is lost in Workspace’s startup flow. Adjust the restore gating in the Workspace logic around shouldGateAgentStartupForCrashRecovery, restorableAgentStartupVerification, and restorableAgentStartupAllowed so intended update relaunches can still resume agents even when crashRecoveryVerificationWithoutFilesystemScan returns nil. Make the same fix in the matching restore path referenced by the later apply location so both startup branches behave consistently.Sources/CrashRecovery/ResumeBreadcrumbBuilder.swift (1)
132-168: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftThese prompts are not shell-safe, despite being injected as terminal input.
The new sanitizers strip quotes/newlines, but they still preserve shell-active characters like
$, backticks,;,|,&, and redirects. The supplied crash-recovery path later injects this text withsendInputWhenReady(text + "\n", ...), and in the unverified path there may be no live agent yet, so a shell interprets the line first. A cwd or custom title containing$(...)or backticks will execute before the shell errors on the English sentence. Either stop sending recovery prose through a shell path, or neutralize shell metacharacters here before treating the result as safe terminal startup input.Also applies to: 183-232
🤖 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/CrashRecovery/ResumeBreadcrumbBuilder.swift` around lines 132 - 168, The crash recovery prompt built by honestRecoveryPrompt is still unsafe as terminal input because sanitizedName and sanitizedPath do not neutralize shell metacharacters. Update honestRecoveryPrompt to either avoid routing this prose through sendInputWhenReady or further escape/neutralize shell-active characters before returning the string. Keep the fix localized to ResumeBreadcrumbBuilder and its prompt-building helpers so the unverified recovery path cannot execute injected cwd or title content.
🤖 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/CrashRecovery/CrashRecoverySettings.swift`:
- Around line 56-69: `shouldGateRestoredAgentStartup(...)` is gating startup too
early for the crash/update recovery flow, which prevents the fallback prompt
from reaching an agent. Update the crash-recovery path so
`Workspace.createPanel(...)` does not suppress agent startup before
`.honestRecovery` is delivered, or ensure
`Workspace+CrashRecovery.scheduleCrashRecoveryReentry(...)` starts a fresh agent
before calling `sendInputWhenReady(...)`. Keep the gate logic in
`CrashRecoverySettings.shouldGateRestoredAgentStartup` aligned with the
agent-first recovery contract.
In `@Sources/CrashRecovery/WorkspaceResumeCoordinator.swift`:
- Around line 154-162: The verified recovery resume delivery in
performVerifiedResumeDelivery currently relies only on surface.isAgentLive,
which can schedule a duplicate native resume during silent restore. Thread the
existing nativeResumeAlreadyScheduled state from the recovery path into
performVerifiedRecovery/performVerifiedResumeDelivery, or split the logic so it
only sends the breadcrumb when native resume was already scheduled, using the
ResumableWorkspaceSurface and recovery entry path to locate the change.
In `@Sources/Panels/BrowserNavigationDelegate.swift`:
- Line 42: Replace the remaining NSLog-based browser navigation callbacks in
BrowserNavigationDelegate with the repo’s unified Logger usage; update the
relevant delegate methods that log failures and URL-related events so they emit
through Logger instead of system log, and redact any URL-bearing fields or
sensitive error text before logging. Use the existing BrowserNavigationDelegate
callback methods as the fix points, and remove all direct NSLog calls in those
paths.
In `@Sources/Workspace`+CrashRecovery.swift:
- Around line 359-376: The crash recovery verification cache key is missing
CODEX_HOME, so cached results can be reused across different Codex homes. Update
both key निर्माणs in Workspace.crashRecoveryVerification paths for the agent and
binding jobs to include the CODEX_HOME value from the relevant environment
alongside CLAUDE_CONFIG_DIR, using the existing key-building logic in
Workspace+CrashRecovery and the cachedVerification call site so transcript
verification stays scoped to the correct home.
---
Outside diff comments:
In `@Sources/CrashRecovery/ResumeBreadcrumbBuilder.swift`:
- Around line 132-168: The crash recovery prompt built by honestRecoveryPrompt
is still unsafe as terminal input because sanitizedName and sanitizedPath do not
neutralize shell metacharacters. Update honestRecoveryPrompt to either avoid
routing this prose through sendInputWhenReady or further escape/neutralize
shell-active characters before returning the string. Keep the fix localized to
ResumeBreadcrumbBuilder and its prompt-building helpers so the unverified
recovery path cannot execute injected cwd or title content.
In `@Sources/Workspace.swift`:
- Around line 1224-1288: The update relaunch auto-resume path is being blocked
by the crash-recovery no-scan verifier for Claude/Codex, so restore intent is
lost in Workspace’s startup flow. Adjust the restore gating in the Workspace
logic around shouldGateAgentStartupForCrashRecovery,
restorableAgentStartupVerification, and restorableAgentStartupAllowed so
intended update relaunches can still resume agents even when
crashRecoveryVerificationWithoutFilesystemScan returns nil. Make the same fix in
the matching restore path referenced by the later apply location so both startup
branches behave consistently.
In `@Sources/Workspace`+RestoredNameFidelity.swift:
- Around line 104-112: `applyRestoredWorkspaceName(_:)` is converting unverified
auto-restored titles into user-owned titles by calling `setCustomTitle(...,
source: .user)` for both `.keepUserTitle` and `.neutral`. Update this flow to
preserve the original provenance from `RestoredName` in
`Workspace+RestoredNameFidelity` so auto/unverified titles remain non-user
sourced and can still be refreshed later by
`scheduleRestoredAgentVerificationRefresh`; keep verified auto titles as
`.auto`, and only use `.user` for actual user-authored titles.
🪄 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: 48ad1072-1055-41c6-a2f3-0a86e58c4980
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (18)
Packages/macOS/CmuxWorkspaces/Sources/CmuxWorkspaces/Core/Values/WorkspacePendingTerminalInputReason.swiftPackages/macOS/CmuxWorkspaces/Tests/CmuxWorkspacesTests/Core/WorkspaceCoreValueTests.swiftResources/Localizable.xcstringsSources/AppDelegate.swiftSources/CrashRecovery/ClaudeTranscriptPresence.swiftSources/CrashRecovery/CrashRecoverySettings.swiftSources/CrashRecovery/ResumeBreadcrumbBuilder.swiftSources/CrashRecovery/ResumeFidelityGate.swiftSources/CrashRecovery/UncleanShutdownSentinel.swiftSources/CrashRecovery/WorkspaceResumeCoordinator.swiftSources/Panels/BrowserNavigationDelegate.swiftSources/Workspace+CrashRecovery.swiftSources/Workspace+PanelLifecycle.swiftSources/Workspace+RestoredNameFidelity.swiftSources/Workspace.swiftcmux.xcodeproj/project.pbxprojcmuxTests/ClaudeTranscriptPresenceTests.swiftcmuxTests/CrashRecoverySettingsTests.swift
💤 Files with no reviewable changes (1)
- Resources/Localizable.xcstrings
🛑 Comments failed to post (4)
Sources/CrashRecovery/CrashRecoverySettings.swift (1)
56-69: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Gated restores can suppress the very agent that should receive the fallback prompt.
shouldGateRestoredAgentStartup(...)now returnstruefor the crash/update recovery paths, but the supplied downstream flow shows thatWorkspace.createPanel(...)suppresses Claude/Codex startup when this gate is on, whileWorkspace+CrashRecovery.scheduleCrashRecoveryReentry(...)delivers.honestRecoveryby callingsendInputWhenReady(prompt + "\n", ...)without launching an agent first. In the unverified crash path, that leaves a plain shell window and types the recovery sentence there instead of to an agent, which breaks the agent-first recovery contract. Either start a fresh agent before delivering.honestRecovery, or avoid gating startup until that fallback has a real receiver.🤖 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/CrashRecovery/CrashRecoverySettings.swift` around lines 56 - 69, `shouldGateRestoredAgentStartup(...)` is gating startup too early for the crash/update recovery flow, which prevents the fallback prompt from reaching an agent. Update the crash-recovery path so `Workspace.createPanel(...)` does not suppress agent startup before `.honestRecovery` is delivered, or ensure `Workspace+CrashRecovery.scheduleCrashRecoveryReentry(...)` starts a fresh agent before calling `sendInputWhenReady(...)`. Keep the gate logic in `CrashRecoverySettings.shouldGateRestoredAgentStartup` aligned with the agent-first recovery contract.Sources/CrashRecovery/WorkspaceResumeCoordinator.swift (1)
154-162: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not key recovery resume delivery only off live-agent state.
recover(_:)is the silent restore path, but restored startup input can already be queued before the agent becomes live. The existing re-entry path carriesnativeResumeAlreadyScheduled; this helper only checks!surface.isAgentLive, so verified recovery can enqueue a second native resume during crash/update relaunch. Thread the already-scheduled state through this recovery path, or split recovery delivery so it only sends the breadcrumb when native resume was already scheduled.Also applies to: 245-249
🤖 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/CrashRecovery/WorkspaceResumeCoordinator.swift` around lines 154 - 162, The verified recovery resume delivery in performVerifiedResumeDelivery currently relies only on surface.isAgentLive, which can schedule a duplicate native resume during silent restore. Thread the existing nativeResumeAlreadyScheduled state from the recovery path into performVerifiedRecovery/performVerifiedResumeDelivery, or split the logic so it only sends the breadcrumb when native resume was already scheduled, using the ResumableWorkspaceSurface and recovery entry path to locate the change.Sources/Panels/BrowserNavigationDelegate.swift (1)
42-42: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Replace these
NSLogcalls with unified logging.These are production browser callbacks, and they currently write navigation URLs and error text directly to the system log. That violates the repo’s Swift logging rule and can leak browsing data; keep the messages in
Loggerand redact URL-bearing fields.Suggested direction
- NSLog("BrowserPanel navigation failed: %@", error.localizedDescription) + logger.error("BrowserPanel navigation failed: \(error.localizedDescription, privacy: .public)")As per coding guidelines, use
Loggerhere instead ofNSLog.Also applies to: 51-51, 345-356, 371-380
🤖 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/Panels/BrowserNavigationDelegate.swift` at line 42, Replace the remaining NSLog-based browser navigation callbacks in BrowserNavigationDelegate with the repo’s unified Logger usage; update the relevant delegate methods that log failures and URL-related events so they emit through Logger instead of system log, and redact any URL-bearing fields or sensitive error text before logging. Use the existing BrowserNavigationDelegate callback methods as the fix points, and remove all direct NSLog calls in those paths.Sources: Coding guidelines, Learnings
Sources/Workspace+CrashRecovery.swift (1)
359-376: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include
CODEX_HOMEin the verification cache key.Codex transcript resolution depends on
CODEX_HOME(Lines 229-233), but both cache keys only includeCLAUDE_CONFIG_DIR. Two restored Codex panels with the same session/cwd but different Codex homes can reuse the wrong transcript verification result.Suggested fix
let configDir = job.agent.launchCommand?.environment?["CLAUDE_CONFIG_DIR"] ?? "" - let key = "agent|\(job.agent.kind.rawValue)|\(job.agent.sessionId)|\(job.agent.workingDirectory ?? "")|\(configDir)" + let codexHome = job.agent.launchCommand?.environment?["CODEX_HOME"] ?? "" + let key = "agent|\(job.agent.kind.rawValue)|\(job.agent.sessionId)|\(job.agent.workingDirectory ?? "")|\(configDir)|\(codexHome)" @@ job.binding.command, job.binding.cwd ?? "", job.binding.environment?["CLAUDE_CONFIG_DIR"] ?? "", + job.binding.environment?["CODEX_HOME"] ?? "", ].joined(separator: "|")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let configDir = job.agent.launchCommand?.environment?["CLAUDE_CONFIG_DIR"] ?? "" let codexHome = job.agent.launchCommand?.environment?["CODEX_HOME"] ?? "" let key = "agent|\(job.agent.kind.rawValue)|\(job.agent.sessionId)|\(job.agent.workingDirectory ?? "")|\(configDir)|\(codexHome)" let verification = cachedVerification(key: key) { Workspace.crashRecoveryVerification(agent: job.agent) } agentResults.append((job.panelId, job.agent, verification)) } for job in bindingJobs { guard !Task.isCancelled else { return } let key = [ "binding", job.binding.kind ?? "", job.binding.checkpointId ?? "", job.binding.command, job.binding.cwd ?? "", job.binding.environment?["CLAUDE_CONFIG_DIR"] ?? "", job.binding.environment?["CODEX_HOME"] ?? "", ].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 `@Sources/Workspace`+CrashRecovery.swift around lines 359 - 376, The crash recovery verification cache key is missing CODEX_HOME, so cached results can be reused across different Codex homes. Update both key निर्माणs in Workspace.crashRecoveryVerification paths for the agent and binding jobs to include the CODEX_HOME value from the relevant environment alongside CLAUDE_CONFIG_DIR, using the existing key-building logic in Workspace+CrashRecovery and the cachedVerification call site so transcript verification stays scoped to the correct home.
Contributor
|
@coderabbitai review |
✅ Action performedReview finished.
|
Contributor
|
@coderabbitai review |
✅ Action performedReview finished.
|
Contributor
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/Workspace+CrashRecovery.swift (1)
592-613: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRevalidate the current restored agent before injecting reentry input.
The detached verification result is only checked against the captured
agentKind/sessionId; if the panel is reused or its restored agent changes with the same session but different cwd/env, this can inject a breadcrumb or prompt into the wrong terminal. Gate delivery on the current restored-agent fingerprint, not just the captured values.Suggested fix
guard !Task.isCancelled else { return } - guard result.verification.facts.agentKind == agentKind, - result.verification.facts.sessionId == sessionId, - let self, - let panel = self.panels[panelId] as? TerminalPanel else { return } - if self.restoredAgentSnapshotsByPanelId[panelId]?.kind == agentKind, - self.restoredAgentSnapshotsByPanelId[panelId]?.sessionId == sessionId { - self.restoredAgentVerificationByPanelId[panelId] = result.verification - } + guard let self, + let panel = self.panels[panelId] as? TerminalPanel, + let currentAgent = self.restoredAgentSnapshotsByPanelId[panelId], + Self.crashRecoveryVerificationFingerprint(agent: currentAgent) == result.verification.fingerprint else { + return + } + self.restoredAgentVerificationByPanelId[panelId] = result.verification switch result.action {🤖 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`+CrashRecovery.swift around lines 592 - 613, The restored-agent delivery path in Workspace+CrashRecovery should revalidate the current agent state before sending recovery input, since the existing check in the resumeVerified/honestRecovery flow only matches the captured agentKind and sessionId. Update the guard around the result handling to also compare the panel’s current restored-agent fingerprint/state (for example via restoredAgentSnapshotsByPanelId and related verification data) before calling sendInputWhenReady or deliverResumeBreadcrumb, so reused panels or changed restored agents do not receive stale prompts or breadcrumbs.Sources/CrashRecovery/ResumeBreadcrumbBuilder.swift (1)
143-171: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse shell-neutral template wording before sanitization.
These templates contain
I'd, butsanitizedTerminalStartupInputLine()strips', so the delivered prompt becomesI d like to work onat runtime. Rephrase the localized source strings to apostrophe-free wording such asI would like to work on, and mirror that update in the matching string-catalog entries.🤖 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/CrashRecovery/ResumeBreadcrumbBuilder.swift` around lines 143 - 171, The localized prompt templates in ResumeBreadcrumbBuilder’s honest recovery cases use apostrophes that get stripped by sanitizedTerminalStartupInputLine(), causing malformed runtime text. Update the source strings in the switch cases for namedWithCwd, namedNoCwd, unnamedWithCwd, and unnamedNoCwd to use shell-neutral wording without apostrophes (for example, replace “I’d” with “I would”), and make the same wording change in the corresponding string-catalog entries.
🤖 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/CrashRecovery/ClaudeTranscriptPresence.swift`:
- Around line 29-45: Record the actual fallback-scan execution in
ClaudeTranscriptPresence.searchedElsewhere instead of copying searchElsewhere,
so direct at-cwd hits correctly show that no sibling scan ran. Update the logic
in the resolver code that builds ClaudeTranscriptPresence to set
searchedElsewhere only when the historical-project scan actually occurs, and
include searchedElsewhere in ClaudeTranscriptPresence.== so comparisons and
caching can distinguish “not scanned” from “scanned and not found.”
In `@Sources/CrashRecovery/CrashRecoveryOffer.swift`:
- Around line 124-127: The crash recovery offer is using the total recoverable
workspace count instead of the verified resumable count, so the alert text can
overstate how many workspaces can actually resume. Keep
`recoverableWorkspaces(in:defaults:)` for gating, but change the
`CrashRecoveryOfferText.make(resumableCount:)` call in `CrashRecoveryOffer` to
use the verified resume set from `verifiedResumePlans` or `resumableWorkspaces`,
matching the behavior covered by `CrashRecoveryOfferTests`.
In `@Sources/Workspace.swift`:
- Around line 4643-4653: The binding-only resume state handling in
Workspace.swift leaves .observedAgentCommandRunning set after the shell reaches
.promptIdle, so canDeliverHonestRecoveryPrompt still thinks the panel is
promptable. Update the state transition logic in the
restoredAgentResumeStatesByPanelId / pendingResumeBreadcrumbsByPanelId switch to
clear the observed binding-only resume state when prompt idle is reached,
alongside the existing cleanup for .autoResumeCommandRunning, so the idle shell
is no longer treated as recoverable for injected text.
- Around line 4887-4895: When a resume binding is replaced or cleared in the
binding management logic around clearSurfaceResumeBinding, also remove any
pending recovery state tied to that panelId. Update the code that assigns
surfaceResumeBindingsByPanelId and the clearSurfaceResumeBinding method so they
also clear restoredAgentResumeStatesByPanelId and
pendingResumeBreadcrumbsByPanelId alongside restoredAgentVerificationByPanelId,
preventing stale breadcrumbs from being delivered after a new binding reports
.commandRunning.
In `@Sources/Workspace`+CrashRecovery.swift:
- Around line 16-21: `CrashRecoveryVerificationFingerprint` is missing
fact-affecting inputs used by `CrashRecoveryVerification(binding:)`, so stale
cached verification can be reused when process detection or command availability
changes. Update the fingerprint to include the binding state that drives
`hasBinding` and `resumeCommandConstructable` (from `binding.isProcessDetected`
and `binding.command`), and make sure the fingerprint is populated consistently
wherever `CrashRecoveryVerificationFingerprint` is built so cache freshness
reflects those facts.
---
Outside diff comments:
In `@Sources/CrashRecovery/ResumeBreadcrumbBuilder.swift`:
- Around line 143-171: The localized prompt templates in
ResumeBreadcrumbBuilder’s honest recovery cases use apostrophes that get
stripped by sanitizedTerminalStartupInputLine(), causing malformed runtime text.
Update the source strings in the switch cases for namedWithCwd, namedNoCwd,
unnamedWithCwd, and unnamedNoCwd to use shell-neutral wording without
apostrophes (for example, replace “I’d” with “I would”), and make the same
wording change in the corresponding string-catalog entries.
In `@Sources/Workspace`+CrashRecovery.swift:
- Around line 592-613: The restored-agent delivery path in
Workspace+CrashRecovery should revalidate the current agent state before sending
recovery input, since the existing check in the resumeVerified/honestRecovery
flow only matches the captured agentKind and sessionId. Update the guard around
the result handling to also compare the panel’s current restored-agent
fingerprint/state (for example via restoredAgentSnapshotsByPanelId and related
verification data) before calling sendInputWhenReady or deliverResumeBreadcrumb,
so reused panels or changed restored agents do not receive stale prompts or
breadcrumbs.
🪄 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: f8841ff7-fc04-47bb-801b-af42041d7249
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (18)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/CrashRecovery/ClaudeTranscriptPresence.swiftSources/CrashRecovery/CrashRecoveryOffer.swiftSources/CrashRecovery/ResumeBreadcrumbBuilder.swiftSources/CrashRecovery/WorkspaceResumeCoordinator.swiftSources/Workspace+CrashRecovery.swiftSources/Workspace+PanelLifecycle.swiftSources/Workspace+RestoredNameFidelity.swiftSources/Workspace.swiftSources/WorkspaceActionDispatcher.swiftcmux.xcodeproj/project.pbxprojcmuxTests/ClaudeTranscriptPresenceTests.swiftcmuxTests/CrashRecoveryOfferTests.swiftcmuxTests/ResumeBreadcrumbAnchorTests.swiftcmuxTests/ResumeBreadcrumbBuilderTests.swiftcmuxTests/WorkspaceResumeCoordinatorTests.swiftcmuxTests/WorkspaceTitleProvenanceTests.swift
Comment on lines
+29
to
+45
| /// Whether the resolver performed the historical-project fallback scan. | ||
| /// Restore-time name verification leaves this false to avoid unbounded launch | ||
| /// filesystem work; explicit recovery can recompute with the scan enabled. | ||
| var searchedElsewhere: Bool = true | ||
|
|
||
| static let absent = ClaudeTranscriptPresence( | ||
| existsAtWindowCwd: false, | ||
| existsElsewhere: false, | ||
| resolvedPathAtWindowCwd: nil, | ||
| searchedElsewhere: false | ||
| ) | ||
|
|
||
| static func == (lhs: ClaudeTranscriptPresence, rhs: ClaudeTranscriptPresence) -> Bool { | ||
| lhs.existsAtWindowCwd == rhs.existsAtWindowCwd | ||
| && lhs.existsElsewhere == rhs.existsElsewhere | ||
| && lhs.resolvedPathAtWindowCwd == rhs.resolvedPathAtWindowCwd | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Record actual fallback-scan execution in searchedElsewhere.
Line 116 currently just echoes searchElsewhere, so a direct at-cwd hit reports searchedElsewhere == true even though the sibling scan never ran. Because Lines 41-45 also omit this field from ==, callers comparing against .absent or caching the value cannot distinguish “not scanned” from “scanned and not found.”
Suggested fix
static let absent = ClaudeTranscriptPresence(
existsAtWindowCwd: false,
existsElsewhere: false,
resolvedPathAtWindowCwd: nil,
searchedElsewhere: false
)
static func == (lhs: ClaudeTranscriptPresence, rhs: ClaudeTranscriptPresence) -> Bool {
lhs.existsAtWindowCwd == rhs.existsAtWindowCwd
&& lhs.existsElsewhere == rhs.existsElsewhere
&& lhs.resolvedPathAtWindowCwd == rhs.resolvedPathAtWindowCwd
+ && lhs.searchedElsewhere == rhs.searchedElsewhere
}
...
- var existsElsewhere = false
+ var existsElsewhere = false
+ var didSearchElsewhere = false
...
if searchElsewhere,
resolvedAtCwd == nil,
!existsElsewhere,
let children = try? fileManager.contentsOfDirectory(atPath: projectsDir) {
+ didSearchElsewhere = true
for child in children where child != windowProjectDir {
let projectRoot = (projectsDir as NSString).appendingPathComponent(child)
if transcriptPath(inProjectRoot: projectRoot, sessionId: sessionId, fileManager: fileManager) != nil {
existsElsewhere = true
break
@@
return ClaudeTranscriptPresence(
existsAtWindowCwd: resolvedAtCwd != nil,
existsElsewhere: existsElsewhere,
resolvedPathAtWindowCwd: resolvedAtCwd,
- searchedElsewhere: searchElsewhere
+ searchedElsewhere: didSearchElsewhere
)Also applies to: 98-116
🤖 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/CrashRecovery/ClaudeTranscriptPresence.swift` around lines 29 - 45,
Record the actual fallback-scan execution in
ClaudeTranscriptPresence.searchedElsewhere instead of copying searchElsewhere,
so direct at-cwd hits correctly show that no sibling scan ran. Update the logic
in the resolver code that builds ClaudeTranscriptPresence to set
searchedElsewhere only when the historical-project scan actually occurs, and
include searchedElsewhere in ClaudeTranscriptPresence.== so comparisons and
caching can distinguish “not scanned” from “scanned and not found.”
Comment on lines
+124
to
+127
| let workspaces = await recoverableWorkspaces(in: managers, defaults: defaults) | ||
| guard !workspaces.isEmpty else { return } | ||
|
|
||
| let content = CrashRecoveryOfferText.make(resumableCount: workspaces.count) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Drive the alert count from verified resumes, not all recoverables.
recoverableWorkspaces intentionally includes prompt-only unverified bindings, but Line 127 passes that total into CrashRecoveryOfferText.make(resumableCount:). In the mixed case already covered by CrashRecoveryOfferTests (resumable == 1, recoverable == 2), the alert will advertise two resumable workspaces even though only one can actually resume. Keep recoverableWorkspaces for gating/execution, but feed the copy from verifiedResumePlans/resumableWorkspaces.
🤖 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/CrashRecovery/CrashRecoveryOffer.swift` around lines 124 - 127, The
crash recovery offer is using the total recoverable workspace count instead of
the verified resumable count, so the alert text can overstate how many
workspaces can actually resume. Keep `recoverableWorkspaces(in:defaults:)` for
gating, but change the `CrashRecoveryOfferText.make(resumableCount:)` call in
`CrashRecoveryOffer` to use the verified resume set from `verifiedResumePlans`
or `resumableWorkspaces`, matching the behavior covered by
`CrashRecoveryOfferTests`.
Comment on lines
+4643
to
+4653
| } else { | ||
| switch (restoredAgentResumeStatesByPanelId[panelId], state) { | ||
| case (.some(.awaitingAutoResumeCommand), .commandRunning): | ||
| restoredAgentResumeStatesByPanelId[panelId] = .autoResumeCommandRunning | ||
| deliverPendingResumeBreadcrumbIfReady(panelId: panelId) | ||
| case (.some(.autoResumeCommandRunning), .promptIdle): | ||
| restoredAgentResumeStatesByPanelId.removeValue(forKey: panelId) | ||
| pendingResumeBreadcrumbsByPanelId.removeValue(forKey: panelId) | ||
| default: | ||
| break | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clear observed binding-only resume state on prompt idle.
For binding-only panels, .observedAgentCommandRunning remains set when the shell returns to .promptIdle; canDeliverHonestRecoveryPrompt then still treats the agent as promptable and can inject recovery text into an idle shell.
Suggested fix
- case (.some(.autoResumeCommandRunning), .promptIdle):
+ case (.some(.autoResumeCommandRunning), .promptIdle),
+ (.some(.observedAgentCommandRunning), .promptIdle):
restoredAgentResumeStatesByPanelId.removeValue(forKey: panelId)
pendingResumeBreadcrumbsByPanelId.removeValue(forKey: panelId)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else { | |
| switch (restoredAgentResumeStatesByPanelId[panelId], state) { | |
| case (.some(.awaitingAutoResumeCommand), .commandRunning): | |
| restoredAgentResumeStatesByPanelId[panelId] = .autoResumeCommandRunning | |
| deliverPendingResumeBreadcrumbIfReady(panelId: panelId) | |
| case (.some(.autoResumeCommandRunning), .promptIdle): | |
| restoredAgentResumeStatesByPanelId.removeValue(forKey: panelId) | |
| pendingResumeBreadcrumbsByPanelId.removeValue(forKey: panelId) | |
| default: | |
| break | |
| } | |
| } else { | |
| switch (restoredAgentResumeStatesByPanelId[panelId], state) { | |
| case (.some(.awaitingAutoResumeCommand), .commandRunning): | |
| restoredAgentResumeStatesByPanelId[panelId] = .autoResumeCommandRunning | |
| deliverPendingResumeBreadcrumbIfReady(panelId: panelId) | |
| case (.some(.autoResumeCommandRunning), .promptIdle), | |
| (.some(.observedAgentCommandRunning), .promptIdle): | |
| restoredAgentResumeStatesByPanelId.removeValue(forKey: panelId) | |
| pendingResumeBreadcrumbsByPanelId.removeValue(forKey: panelId) | |
| default: | |
| break | |
| } |
🤖 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 4643 - 4653, The binding-only resume
state handling in Workspace.swift leaves .observedAgentCommandRunning set after
the shell reaches .promptIdle, so canDeliverHonestRecoveryPrompt still thinks
the panel is promptable. Update the state transition logic in the
restoredAgentResumeStatesByPanelId / pendingResumeBreadcrumbsByPanelId switch to
clear the observed binding-only resume state when prompt idle is reached,
alongside the existing cleanup for .autoResumeCommandRunning, so the idle shell
is no longer treated as recoverable for injected text.
Comment on lines
+4887
to
+4895
| restoredAgentVerificationByPanelId.removeValue(forKey: panelId) | ||
| surfaceResumeBindingsByPanelId[panelId] = binding | ||
| return true | ||
| } | ||
|
|
||
| @discardableResult | ||
| func clearSurfaceResumeBinding(panelId: UUID) -> Bool { | ||
| surfaceResumeBindingsByPanelId.removeValue(forKey: panelId) != nil | ||
| restoredAgentVerificationByPanelId.removeValue(forKey: panelId) | ||
| return surfaceResumeBindingsByPanelId.removeValue(forKey: panelId) != nil |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clear pending recovery state when the resume binding changes.
Replacing or clearing a binding drops verification, but leaves restoredAgentResumeStatesByPanelId and pendingResumeBreadcrumbsByPanelId. A queued breadcrumb from the previous binding can be delivered when the new binding reports .commandRunning.
Suggested fix
restoredAgentVerificationByPanelId.removeValue(forKey: panelId)
+ restoredAgentResumeStatesByPanelId.removeValue(forKey: panelId)
+ pendingResumeBreadcrumbsByPanelId.removeValue(forKey: panelId)
surfaceResumeBindingsByPanelId[panelId] = binding
return true
@@
func clearSurfaceResumeBinding(panelId: UUID) -> Bool {
restoredAgentVerificationByPanelId.removeValue(forKey: panelId)
+ restoredAgentResumeStatesByPanelId.removeValue(forKey: panelId)
+ pendingResumeBreadcrumbsByPanelId.removeValue(forKey: panelId)
return surfaceResumeBindingsByPanelId.removeValue(forKey: panelId) != nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| restoredAgentVerificationByPanelId.removeValue(forKey: panelId) | |
| surfaceResumeBindingsByPanelId[panelId] = binding | |
| return true | |
| } | |
| @discardableResult | |
| func clearSurfaceResumeBinding(panelId: UUID) -> Bool { | |
| surfaceResumeBindingsByPanelId.removeValue(forKey: panelId) != nil | |
| restoredAgentVerificationByPanelId.removeValue(forKey: panelId) | |
| return surfaceResumeBindingsByPanelId.removeValue(forKey: panelId) != nil | |
| restoredAgentVerificationByPanelId.removeValue(forKey: panelId) | |
| restoredAgentResumeStatesByPanelId.removeValue(forKey: panelId) | |
| pendingResumeBreadcrumbsByPanelId.removeValue(forKey: panelId) | |
| surfaceResumeBindingsByPanelId[panelId] = binding | |
| return true | |
| } | |
| `@discardableResult` | |
| func clearSurfaceResumeBinding(panelId: UUID) -> Bool { | |
| restoredAgentVerificationByPanelId.removeValue(forKey: panelId) | |
| restoredAgentResumeStatesByPanelId.removeValue(forKey: panelId) | |
| pendingResumeBreadcrumbsByPanelId.removeValue(forKey: panelId) | |
| return surfaceResumeBindingsByPanelId.removeValue(forKey: panelId) != nil |
🤖 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 4887 - 4895, When a resume binding is
replaced or cleared in the binding management logic around
clearSurfaceResumeBinding, also remove any pending recovery state tied to that
panelId. Update the code that assigns surfaceResumeBindingsByPanelId and the
clearSurfaceResumeBinding method so they also clear
restoredAgentResumeStatesByPanelId and pendingResumeBreadcrumbsByPanelId
alongside restoredAgentVerificationByPanelId, preventing stale breadcrumbs from
being delivered after a new binding reports .commandRunning.
Comment on lines
+16
to
+21
| nonisolated struct CrashRecoveryVerificationFingerprint: Equatable, Sendable { | ||
| var kind: RestorableAgentKind? | ||
| var sessionId: String? | ||
| var cwd: String? | ||
| var claudeConfigDir: String? | ||
| var codexHome: String? |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include fact-affecting fields in the verification fingerprint.
Cached verification freshness is gated by CrashRecoveryVerificationFingerprint, but CrashRecoveryVerification(binding:) computes hasBinding and resumeCommandConstructable from binding.isProcessDetected and binding.command, while the fingerprint ignores both. A binding that flips detected/live state or loses constructable input can reuse stale verified facts.
Suggested fix
nonisolated struct CrashRecoveryVerificationFingerprint: Equatable, Sendable {
var kind: RestorableAgentKind?
var sessionId: String?
var cwd: String?
var claudeConfigDir: String?
var codexHome: String?
+ var resumeCommandConstructable: Bool?
+ var isProcessDetected: Bool?
}
@@
CrashRecoveryVerificationFingerprint(
kind: agent.kind,
sessionId: nonEmpty(agent.sessionId),
cwd: nonEmpty(agent.workingDirectory),
claudeConfigDir: nonEmpty(agent.launchCommand?.environment?["CLAUDE_CONFIG_DIR"]),
- codexHome: nonEmpty(agent.launchCommand?.environment?["CODEX_HOME"])
+ codexHome: nonEmpty(agent.launchCommand?.environment?["CODEX_HOME"]),
+ resumeCommandConstructable: agent.resumeCommand != nil,
+ isProcessDetected: nil
)
@@
CrashRecoveryVerificationFingerprint(
kind: binding.kind.flatMap(RestorableAgentKind.init(rawValue:)),
sessionId: nonEmpty(binding.checkpointId ?? WorkspaceResumeCoordinator.bareSessionId(from: binding.command)),
cwd: nonEmpty(binding.cwd),
claudeConfigDir: nonEmpty(binding.environment?["CLAUDE_CONFIG_DIR"]),
- codexHome: nonEmpty(binding.environment?["CODEX_HOME"])
+ codexHome: nonEmpty(binding.environment?["CODEX_HOME"]),
+ resumeCommandConstructable: !binding.isProcessDetected
+ && !binding.command.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty,
+ isProcessDetected: binding.isProcessDetected
)Also applies to: 259-280
🤖 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`+CrashRecovery.swift around lines 16 - 21,
`CrashRecoveryVerificationFingerprint` is missing fact-affecting inputs used by
`CrashRecoveryVerification(binding:)`, so stale cached verification can be
reused when process detection or command availability changes. Update the
fingerprint to include the binding state that drives `hasBinding` and
`resumeCommandConstructable` (from `binding.isProcessDetected` and
`binding.command`), and make sure the fingerprint is populated consistently
wherever `CrashRecoveryVerificationFingerprint` is built so cache freshness
reflects those facts.
Comment on lines
+219
to
+226
| guard let sessionId = nonEmpty(sessionId), | ||
| isSafeFilename(sessionId), | ||
| let cwd = nonEmpty(cwd) else { | ||
| return .absent | ||
| } | ||
| guard searchElsewhere else { | ||
| return .absent | ||
| } |
Contributor
There was a problem hiding this comment.
CodexTranscriptPresenceResolver short-circuits the at-cwd check when searchElsewhere: false
Lines 224–226 return .absent immediately for any searchElsewhere: false call — the existsAtWindowCwd path is never run. ClaudeTranscriptPresenceResolver.resolve (lines 86–92) correctly does the at-cwd check regardless of searchElsewhere, using the flag only to skip the broader "elsewhere" scan. The Codex resolver must do the same.
The practical breakage: scheduleRestoredAgentVerificationRefresh always passes searchElsewhere: false, so every Codex panel's cached verification is written with existsAtWindowCwd: false, causing two observable failures: (1) auto-summary workspace titles are never re-applied for valid Codex sessions, and (2) if scheduleRestoredAgentVerificationRefresh completes after scheduleCrashRecoveryReentry (which uses searchElsewhere: true), it overwrites the correctly-verified result with the always-absent one — turning a verified recovery into a silent miss for Codex users.
Suggested change
| guard let sessionId = nonEmpty(sessionId), | |
| isSafeFilename(sessionId), | |
| let cwd = nonEmpty(cwd) else { | |
| return .absent | |
| } | |
| guard searchElsewhere else { | |
| return .absent | |
| } | |
| guard let sessionId = nonEmpty(sessionId), | |
| isSafeFilename(sessionId), | |
| let cwd = nonEmpty(cwd) else { | |
| return .absent | |
| } | |
| var resolvedAtCwd: String? | |
| var existsElsewhere = false | |
| let needle = sessionId.lowercased() |
Contributor
Author
|
Hi @austinywang - this one's gone quiet since the review round on Jun 25. I think I've addressed all the CodeRabbit/Greptile threads and CI is fully green. It is a big diff, so if the size is the blocker I'm glad to carve it into smaller PRs (e.g. split the recovery core from the UI pieces). Let me know what would make it easiest to land. |
Collaborator
|
Thanks for this work on crash and update recovery. After a crash on 2026-09-26 took five agent sessions down, we opened #14870, which reopens sessions the agent journal never saw end and resumes each through its original launcher, and #14824, which keeps rotated session snapshots. They take a different route and don't reuse code from here, but your write-up of the failure modes helped frame them. |
Contributor
Author
|
Thanks @teamleaderleo, glad the failure-mode write-up was useful. #14870 and #14824 sound like the right shape, especially resuming through the original launcher. I'll close this one out since those cover it. Happy to test either PR against the crash cases I hit if that helps. |
Contributor
Author
|
Closing this in favor of #14824 (merged) and #14870, which cover crash and update recovery through the agent journal and original launchers. Thanks again @teamleaderleo. Happy to run the crash cases I hit against #14870 if useful. |
Contributor
Author
|
@teamleaderleo thanks, and glad the failure-mode write-up was useful. Resuming through the original launcher in #14870 is a cleaner route than what I had here. I'll try both once they land. |
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
3 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.
Agent-first crash/update session recovery
Recovers agent windows after a crash or an "Update & reload all windows" relaunch, with an agent-first recovery model: when a restored window's session can be verified it resumes its own work and the agent is handed its specific transcript; when it can't be verified the agent gets an honest, cwd-scoped recovery prompt instead of a confident wrong guess.
This is opt-in (
crashRecovery.injectResumeBreadcrumb, default off) and deliberately additive. It layers on top of the existing native auto-resume and does not modify the binding store or the resume-cwd logic that #6741 and #6631 are reshaping, so it composes with that work rather than overlapping it.Why
Across several force-quit / "what was this window doing, can you resume?" tests, restored windows failed the same way: a window came up fresh (or in the wrong cwd), wore another session's name, and when asked to recover it grepped every transcript and adopted a plausible but wrong one. A confident wrong recovery is worse than an honest "I couldn't verify this window's session."
The core binding-cwd correctness is being fixed in #6741 (pin auto-resume to the launch cwd). This PR adds the recovery decision and re-entry layer on top of a now-correct binding.
What's in it
offerResumeAfterCrash,injectResumeBreadcrumb,resumeAgentsAfterUpdate) incmux.json.scripts/crash-recovery-e2e.sh) that force-quits only the isolated tagged build (hard guard against the main app).Scope and safety
Sources/CrashRecovery/; the only edit to existing hot code is an additive hook in theWorkspacerestore path. No changes toCLI/cmux.swift's binding publish orRestorableAgentSession's resume-cwd resolution (the Pin Claude auto-resume binding to the launch cwd (#4256) #6741 / Agent-session tracking: single source of truth (no title/mtime heuristics) #6631 surface).Testing
Coordination note
Happy to scope this down, rebase onto #6741/#6631 once they land, or split the foundation from the recovery layer — whatever composes best with the binding work in flight.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Recovers agent windows after a crash or an update relaunch with an agent‑first, verification‑gated flow. Verified sessions auto‑resume with a transcript‑anchored breadcrumb; unverified sessions get a safe, cwd‑scoped prompt.
New Features
cmux.jsonsettings:offerResumeAfterCrash,injectResumeBreadcrumb,resumeAgentsAfterUpdate(default off). Windows restore after updates; optional silent agent auto‑resume.Bug Fixes
Written for commit b9fcfc9. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Documentation