Repository navigation
Conversation
|
@aml11 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:
📝 WalkthroughWalkthroughAdds a resume cwd existence guard, dual-index restorable-session lookup with panelId fallback, Claude transcript-based filtering for auto-resume, conditional restoration of embedded agents, Workspace.markRestorableAgentSessionEnded, a new ChangesRestorable Agent Session Lifecycle and Transcript Validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes three independent bugs that prevent Claude/Codex/Gemini session auto-resume from working after a cmux relaunch. The fixes are well-isolated with regression tests for all three failure modes.
Confidence Score: 5/5Safe to merge — all three session-restore regressions are addressed with correct logic and regression tests, and no new data-loss or crash paths were introduced. The panelId fallback sort, the sessionId-scoped in-memory clear, the fail-open transcript filter, and the cwd directory guard are all correctly implemented and independently tested. The only open items are tracked follow-ups acknowledged in prior review threads, not new regressions introduced here. Sources/Workspace.swift — the synchronous transcript scan in createPanel on the main actor is an open follow-up from prior review threads, not introduced by this PR but still unresolved. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as cmux CLI (claude-hook)
participant TC as TerminalController
participant WS as Workspace
participant Index as RestorableAgentSessionIndex
participant FS as Filesystem
Note over Index,FS: Bug 1 - Index load (cross-launch)
Index->>FS: Load hook-sessions.json
Index->>Index: Build snapshotsByPanel[(workspaceId,panelId)]
Index->>FS: claudeAgentIsRestorable - check transcript
Index->>Index: Build snapshotsByPanelId[panelId] fallback (sorted by updatedAt desc)
WS->>Index: snapshot(workspaceId: NEW_UUID, panelId: P)
Index-->>WS: strict miss - panelId fallback returns snapshot
Note over CLI,WS: Bug 2 - In-lifetime session end
CLI->>TC: "agent_session_ended --tab=W --surface=P --session=S"
TC->>WS: markRestorableAgentSessionEnded(panelId, sessionId)
WS->>WS: "guard restored.sessionId == S"
WS->>WS: remove from restoredAgentSnapshotsByPanelId
Note over WS,FS: Bug 3 - Transcript filter
WS->>FS: claudeAgentIsRestorable(kind:.claude, sessionId, cwd)
FS-->>WS: missing transcript - true (fail-open)
FS-->>WS: metadata-only - false (drop)
FS-->>WS: has user/assistant - true (keep)
Reviews (7): Last reviewed commit: "Address review: require directory for cw..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let hasConversation = RestorableAgentSessionIndex.claudeTranscriptHasConversation( | ||
| cwd: cwd, | ||
| sessionId: candidate.sessionId, | ||
| claudeConfigDir: configDir | ||
| ) | ||
| return hasConversation ? candidate : nil |
There was a problem hiding this comment.
@MainActor during session restore
claudeTranscriptHasConversation reads up to 1 MB of JSONL data in a synchronous loop using FileHandle.read(upToCount:), called here from createPanel(from:inPane:) which runs entirely on the main actor. restoreSessionSnapshot iterates every panel at launch, so this blocks the main thread once per Claude panel in the saved session. The equivalent call in RestorableAgentSessionIndex.loadIncludingProcessDetectedSnapshots correctly wraps the same work in Task.detached(priority: .utility) — the createPanel call site has no such boundary.
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
>= vs > asymmetry between the two de-dup passes
The primary resolved dict keeps the most-recent record per (workspaceId, panelId) using a strict > comparison (skip if existing timestamp is strictly newer). The new byPanelId pass uses >= (skip if existing timestamp is equal or newer). When two hook records share the same panelId and identical updatedAt timestamps, the two dicts can resolve to different snapshots depending on iteration order. Consider using > consistently, or explicitly documenting why equal timestamps should prefer the first-seen entry for the panelId fallback index.
There was a problem hiding this comment.
Real point — the asymmetry is unintentional but harmless in practice.
Ties on updatedAt (a TimeInterval set at hook-write time) only happen when two records share an exact-second timestamp, which is essentially limited to stale duplicate hook records left over from older versions. >= and > differ only in which duplicate wins; either choice is nondeterministic across runs anyway because Swift Dictionary iteration order isn't stable.
Consistency is cheap though — I'll change line 694 to > in a follow-up commit on this branch so both passes use the same comparator.
| // panel so the next session save does not re-embed it. Without | ||
| // this, the next cmux launch tries to resume a session that | ||
| // was already ended in this lifetime. | ||
| if !surfaceId.isEmpty { | ||
| _ = try? sendV1Command( | ||
| "agent_session_ended --tab=\(workspaceId) --surface=\(surfaceId)", | ||
| client: client | ||
| ) | ||
| } | ||
| } | ||
| print("OK") |
There was a problem hiding this comment.
--tab lookup uses the hook record's workspace UUID, which may not match any live workspace
consumedSession.workspaceId is the workspace UUID that was active when the hook record was written. If cmux relaunches before Claude exits, the live workspace will have a regenerated UUID and parseSidebarMutationTabTarget will fail to find the correct workspace, silently dropping the agent_session_ended notification. The in-lifetime case (Bug 2 scenario) works correctly, but the cross-launch case is left uncleared. Adding panel-UUID routing support to the mutation bus would close this gap, consistent with the panelId fallback added to the index for Bug 1.
There was a problem hiding this comment.
Right — the new V1 command routes by workspace UUID, which is stale post-relaunch, so the cross-launch SessionEnd cleanup falls through to a no-op. Same root cause as Bug 1: Workspace.id regenerates.
This PR's Bug 2 fix was scoped to the in-lifetime case (user /exits claude in the same session as the SessionStart) — the doc is explicit about that. There is a partial backstop for the cross-launch case: claude-hook session-end still consumes the on-disk hook record, so the next launch's RestorableAgentSessionIndex won't re-resolve the agent via either the strict key or the panel-id fallback. But you're right that the in-memory restoredAgentSnapshotsByPanelId entry stays until something else clears it.
Tracking as a follow-up: add panel-UUID routing to the mutation bus so agent_session_ended can address by panel-id when the workspace lookup misses, mirroring the panel-id fallback added to the index for Bug 1. Out of scope for this PR.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Workspace.swift`:
- Around line 724-745: Extract the Claude transcript eligibility logic into a
single shared helper (e.g., add a function on RestorableAgentSessionIndex like
claudeTranscriptIsRestorable(cwd: String, sessionId: String, environment:
[String: String]?) or a similarly named static helper), then replace the inline
predicate in Workspace.swift (the closure that computes restorableAgent) and the
existing metadata-only filter in RestorableAgentSession.swift to call that
helper; ensure the helper encapsulates the same checks (agent kind, non-empty
cwd via candidate.workingDirectory or candidate.launchCommand?.workingDirectory,
computing claudeConfigDir from environment, and calling
claudeTranscriptHasConversation) so both fresh-load and restore paths use the
identical eligibility logic.
🪄 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: a42915bb-2234-4804-9bd2-3093caf24922
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (5)
CLI/cmux.swiftSources/RestorableAgentSession.swiftSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/SessionPersistenceTests.swift
cmux.video.mp4 |
34a4416 to
d2de220
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/SessionPersistenceTests.swift`:
- Around line 664-700: Extend
testRestorableAgentIndexFallsBackToPanelIdAfterWorkspaceUUIDRegenerates by
adding at least two restorable agent records created with the same panelId but
different timestamps/sessionId values (use makeRestorableAgentIndex or helper
that inserts multiple records for the same panelId, e.g., sessions "old-session"
and "new-session"); after verifying the strict (workspaceId, panelId) match
still returns the exact record, simulate the regenerated workspaceId and call
index.snapshot(workspaceId: regeneratedWorkspaceId, panelId: panelId) and assert
it returns the most-recent/newer record (e.g., "new-session") and the correct
kind, and also keep the existing assertion that an unrelated panelId returns
nil. Ensure you insert records so their timestamps or insertion order reflect
recency so the fallback selection logic is exercised.
In `@Sources/RestorableAgentSession.swift`:
- Around line 728-740: The cheap probe in lineDeclaresConversationEvent assumes
fixed spacing and misses valid JSON like `"type" : "assistant"`; change the
check to match arbitrary whitespace around the colon and allow both "user" and
"assistant" by using a regex such as "\"type\"\\s*:\\s*\"(user|assistant)\""
(precompile an NSRegularExpression for performance) and test the UTF-8 string
against that regex instead of the four fixed contains checks.
🪄 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: ae24e041-5f28-4a39-af0d-06ff2d875130
📒 Files selected for processing (3)
Sources/RestorableAgentSession.swiftSources/Workspace.swiftcmuxTests/SessionPersistenceTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cmuxTests/SessionPersistenceTests.swift (1)
664-700: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd coverage for "most-recent record per panel" fallback selection.
Line 664–Line 700 validates fallback for a single record, but it does not verify the recency rule when multiple hook records share the same
panelId. Please add a case with at least two records for the same panel and assert fallback returns the newest one after strict(workspaceId, panelId)miss.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmuxTests/SessionPersistenceTests.swift` around lines 664 - 700, Extend the testRestorableAgentIndexFallsBackToPanelIdAfterWorkspaceUUIDRegenerates case to add at least two hook records that share the same panelId but different creation/updated times (use makeRestorableAgentIndex or the helper that builds the index to insert multiple entries for the same panelId with different sessionId/kind and a later timestamp for the newest record), then after forcing the strict (workspaceId, panelId) miss (use a regeneratedWorkspaceId) call index.snapshot(workspaceId:regeneratedWorkspaceId, panelId:panelId) and assert it returns the sessionId/kind from the most recent record; keep the existing assertions (strict match and unknown-panel nil) and only add the multi-record setup and the recency assertion using the same index and snapshot methods.Sources/RestorableAgentSession.swift (1)
728-740:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle arbitrary whitespace in the transcript
typeprobe.
lineDeclaresConversationEventstill only matches four fixed substrings, so valid JSONL like"type" : "assistant"is treated as metadata-only and the Claude session gets dropped from restore. This was already flagged in an earlier review and still appears unresolved.Suggested fix
+ private static let conversationEventPattern = try! NSRegularExpression( + pattern: #""type"\s*:\s*"(user|assistant)""# + ) + private static func lineDeclaresConversationEvent(_ line: Data) -> Bool { - // Cheap probe: a line that mentions `"type":"user"` or - // `"type":"assistant"` is a real conversation event. Metadata-only - // lines (`custom-title`, `agent-name`, `permission-mode`, etc.) don't - // match. We avoid full JSON parsing per line for hot-path performance. guard !line.isEmpty, let text = String(data: line, encoding: .utf8) else { return false } - return text.contains("\"type\":\"user\"") - || text.contains("\"type\":\"assistant\"") - || text.contains("\"type\": \"user\"") - || text.contains("\"type\": \"assistant\"") + let range = NSRange(text.startIndex..<text.endIndex, in: text) + return conversationEventPattern.firstMatch(in: text, range: range) != 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/RestorableAgentSession.swift` around lines 728 - 740, lineDeclaresConversationEvent currently only checks four exact substrings and misses valid JSONL with arbitrary whitespace (e.g. `"type" : "assistant"`); update lineDeclaresConversationEvent to match `"type"` with optional whitespace around the colon and either "user" or "assistant" by using a single precompiled regular expression (e.g. pattern like "\"type\"\\s*:\\s*\"(user|assistant)\"") or an equivalent fast character-scan, so the probe accepts any amount of whitespace while preserving the hot-path performance (compile the NSRegularExpression once and reuse it inside lineDeclaresConversationEvent).
🤖 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/Workspace.swift`:
- Around line 8578-8595: The markRestorableAgentSessionEnded function currently
clears whatever snapshot is stored for a panelId, causing races when a
late/duplicate end event for session A wipes a newer session B that reused the
panel; change markRestorableAgentSessionEnded to accept the ended session
identity (e.g., sessionId or snapshot fingerprint), look up restored =
restoredAgentSnapshotsByPanelId[panelId], compute the current fingerprint via
TabManager.restorableAgentSnapshotFingerprint(restored) (or compare
restored.sessionId), and only set
invalidatedRestoredAgentFingerprintsByPanelId[panelId], removeValue(forKey:),
and remove from restoredAgentAutoResumePendingPanelIds if the provided ended
identity matches the current restored snapshot; update all agent_session_ended
call sites to pass the ended session identity so invalidation becomes ordered
and idempotent.
---
Duplicate comments:
In `@cmuxTests/SessionPersistenceTests.swift`:
- Around line 664-700: Extend the
testRestorableAgentIndexFallsBackToPanelIdAfterWorkspaceUUIDRegenerates case to
add at least two hook records that share the same panelId but different
creation/updated times (use makeRestorableAgentIndex or the helper that builds
the index to insert multiple entries for the same panelId with different
sessionId/kind and a later timestamp for the newest record), then after forcing
the strict (workspaceId, panelId) miss (use a regeneratedWorkspaceId) call
index.snapshot(workspaceId:regeneratedWorkspaceId, panelId:panelId) and assert
it returns the sessionId/kind from the most recent record; keep the existing
assertions (strict match and unknown-panel nil) and only add the multi-record
setup and the recency assertion using the same index and snapshot methods.
In `@Sources/RestorableAgentSession.swift`:
- Around line 728-740: lineDeclaresConversationEvent currently only checks four
exact substrings and misses valid JSONL with arbitrary whitespace (e.g. `"type"
: "assistant"`); update lineDeclaresConversationEvent to match `"type"` with
optional whitespace around the colon and either "user" or "assistant" by using a
single precompiled regular expression (e.g. pattern like
"\"type\"\\s*:\\s*\"(user|assistant)\"") or an equivalent fast character-scan,
so the probe accepts any amount of whitespace while preserving the hot-path
performance (compile the NSRegularExpression once and reuse it inside
lineDeclaresConversationEvent).
🪄 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: f47643e0-70ad-4852-8cdc-3c88b10c3577
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (5)
CLI/cmux.swiftSources/RestorableAgentSession.swiftSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/SessionPersistenceTests.swift
d2de220 to
74c955d
Compare
|
Pushed
Greptile's P1 finding on main-thread sync I/O in Verified locally: @codex review |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/RestorableAgentSession.swift (1)
728-741:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWhitespace variations in JSON
typeprobe still drop valid transcripts.The
contains(...)checks still cover only four fixed shapes ("type":"user","type":"assistant", and the single-space variants). A pretty-printed transcript line like"type" : "assistant"(or any other whitespace around the colon) is classified as metadata-only and the hook record is dropped at line 578, defeating the auto-resume the rest of this PR is trying to fix. The blast radius is limited because Claude's CLI normally emits compact JSON, but a single regex-based probe removes the assumption entirely:Suggested fix
+ private static let conversationEventPattern: NSRegularExpression? = try? NSRegularExpression( + pattern: #""type"\s*:\s*"(user|assistant)""# + ) + private static func lineDeclaresConversationEvent(_ line: Data) -> Bool { guard !line.isEmpty, let text = String(data: line, encoding: .utf8) else { return false } - return text.contains("\"type\":\"user\"") - || text.contains("\"type\":\"assistant\"") - || text.contains("\"type\": \"user\"") - || text.contains("\"type\": \"assistant\"") + guard let pattern = conversationEventPattern else { + return text.contains("\"type\":\"user\"") + || text.contains("\"type\":\"assistant\"") + } + let range = NSRange(text.startIndex..<text.endIndex, in: text) + return pattern.firstMatch(in: text, range: range) != 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/RestorableAgentSession.swift` around lines 728 - 741, The probe in lineDeclaresConversationEvent only matches four fixed whitespace variants and misses other valid JSON spacing (e.g. `"type" : "assistant"`), so replace the multiple contains(...) checks with a single robust check using a regex that allows optional whitespace around the colon and matches either "user" or "assistant" (for example pattern like "\"type\"\\s*:\\s*\"(user|assistant)\""), or alternately do a cheap JSON decode of the small line to inspect the "type" key; update lineDeclaresConversationEvent to use that regex/JSON check so all whitespace variations around the colon are accepted.
🤖 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.
Duplicate comments:
In `@Sources/RestorableAgentSession.swift`:
- Around line 728-741: The probe in lineDeclaresConversationEvent only matches
four fixed whitespace variants and misses other valid JSON spacing (e.g. `"type"
: "assistant"`), so replace the multiple contains(...) checks with a single
robust check using a regex that allows optional whitespace around the colon and
matches either "user" or "assistant" (for example pattern like
"\"type\"\\s*:\\s*\"(user|assistant)\""), or alternately do a cheap JSON decode
of the small line to inspect the "type" key; update
lineDeclaresConversationEvent to use that regex/JSON check so all whitespace
variations around the colon are accepted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0d304024-89ef-4f26-a7dd-eb7760aceb42
📒 Files selected for processing (3)
Sources/RestorableAgentSession.swiftSources/Workspace.swiftcmuxTests/SessionPersistenceTests.swift
|
Two follow-up commits since my last status update:
All 5 named tests pass via @coderabbitai @greptile-apps please re-evaluate. |
|
Re-triggering a full review now to evaluate the new commits end-to-end. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/RestorableAgentSession.swift`:
- Around line 610-616: The fold into byPanelId from resolved should use a
deterministic tie-breaker when updatedAt are equal to avoid non-deterministic
overwrites; change the logic in the loop that currently checks
existing.updatedAt > value.updatedAt so that when timestamps are equal it
compares a stable identifier (e.g. existing.snapshot.sessionId or
existing.snapshot.workspaceId) in a deterministic order (lexicographic or UUID
comparison) and only keep the incoming value if it wins that tie-breaker (or
alternatively sort resolved by (panelId, updatedAt, sessionId) before folding).
Update the comparison around byPanelId, resolved,
SessionRestorableAgentSnapshot, key.panelId and value.updatedAt to apply this
secondary comparison.
- Around line 52-60: The guard that checks
FileManager.default.fileExists(atPath: cwd) should require cwd to be a
directory, not just any existing path; change the check in
RestorableAgentSession (the block that builds shellCommand and calls
shellSingleQuoted(cwd)) to use
FileManager.default.fileExists(atPath:isDirectory:) (or an equivalent
isDirectory check) and only proceed when isDirectory is true, otherwise return
nil so resumeStartupInput skips auto-resume.
🪄 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: 6570c4cb-37b9-4178-a604-003da5825632
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (5)
CLI/cmux.swiftSources/RestorableAgentSession.swiftSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/SessionPersistenceTests.swift
`RestorableAgentSessionIndex` is keyed by (workspaceId, panelId), but `Workspace.id` is regenerated as a fresh `UUID()` on every cmux launch. After the first relaunch, the saved hook record's `workspaceId` no longer matches the live workspace, the index lookup misses, and autosave writes the panel snapshot with `agent: nil`. From then on the user's Claude (and other agent) sessions never auto-resume, even though `~/.cmuxterm/<agent>-hook-sessions.json` still has the right session ID keyed by the panel's surface UUID. Add a panelId-only fallback lookup to `RestorableAgentSessionIndex`. The strict `(workspaceId, panelId)` key wins when both match (same cmux lifetime); otherwise we fall back to the most recent record for the same panel UUID. Panel UUIDs are the `CMUX_SURFACE_ID` written by hooks and are unique enough across the hook records to make this safe. Also add a regression test `testRestorableAgentIndexFallsBackToPanelIdAfterWorkspaceUUIDRegenerates` that constructs an index for one workspace UUID and verifies the lookup still resolves under a different (regenerated) workspace UUID, while unknown panel IDs still miss.
When a panel is created from a restored snapshot, `Workspace.restoredAgentSnapshotsByPanelId[panelId]` is populated from `SessionPanelSnapshot.terminal.agent` so the next snapshot save can re-embed it. After the panel-id fallback fix, this path now succeeds reliably across launches. But when the agent later exits in the same lifetime (e.g. user types `/exit` in claude), only the on-disk hook record gets consumed by the SessionEnd hook. The in-memory entry in `restoredAgentSnapshotsByPanelId` is never cleared, so the very next snapshot save re-embeds the stale agent and the next cmux launch tries to resume a session that has already finished. (`Workspace.updatePanelShellActivityState` invalidates on `.commandRunning`, but only fires if the user types another command after the agent exits — when they just close the window, it never runs.) Add a `Workspace.markRestorableAgentSessionEnded(panelId:)` method that clears the in-memory restored-agent state, expose it via a new `agent_session_ended` v1 control command, and have `claude-hook session-end` (and the generic agent SessionEnd path used by Codex, Gemini, etc.) send that command alongside the existing `clear_agent_pid` / `clear_status` calls. The result is that when an agent exits and SessionEnd fires, the panel's restored-agent state is cleared in memory the same way it gets consumed on disk. Adds a regression test `testMarkRestorableAgentSessionEndedNoopsWithoutEntry` that verifies the helper tolerates being called for panels that never had a restored snapshot — the v1 command is invoked from a sidebar mutation closure that targets any panel whose hook fires, including fresh sessions.
Claude rejects `--resume <id>` for sessions whose transcript only contains metadata events (`custom-title`, `agent-name`, `permission-mode`) with "No conversation found with session ID …". This happens when a user runs `/rename` immediately after Claude starts, before sending any user message — Claude's SessionStart hook fires and cmux records the session, but the transcript never gets a real exchange because the user only typed `/rename`. On relaunch cmux auto-resumes into the error. When loading hook records, derive Claude's project directory path (`<CLAUDE_CONFIG_DIR>/projects/<cwd-with-/-replaced-by-->/<sessionId>.jsonl`) and skip records whose transcript has no `"type":"user"` or `"type":"assistant"` event. Fail open — if the transcript is missing or unreadable, keep the record and let Claude itself produce the canonical error at resume time. This is Claude-specific and gated on `kind == .claude`; other agents are untouched. Important: do NOT call `NSString.standardizingPath` on the cwd. macOS collapses `/private/tmp/...` → `/tmp/...`, but Claude stores the project under the un-resolved form (`-private-tmp-aaa`), so a standardized cwd would point at the wrong project directory. Adds `testClaudeTranscriptHasConversationFiltersMetadataOnlyTranscripts` that builds a fake `~/.claude` layout under a temp dir, writes both a metadata-only transcript and a real one, and verifies the helper drops the former, keeps the latter, and passes through unknown transcripts.
The fresh-load path in `RestorableAgentSession.swift` and the snapshot restore path in `Workspace.createPanel(from:inPane:)` had drifted into two near-identical metadata-only-transcript filters. Each independently checked `kind == .claude`, extracted a cwd, resolved the Claude config dir, and called `claudeTranscriptHasConversation`. The next change to Claude's transcript rules would have had to land in two places. Extract `RestorableAgentSessionIndex.claudeAgentIsRestorable(kind: sessionId:cwd:environment:fileManager:)` as the single source of truth. Both surfaces now route through it; non-Claude agents pass through unchanged, missing/empty cwd still fails open. Also flip the byPanelId fold's tie-break comparator from `>=` to `>` on the third pass (line 604) so all three passes use the same `>` comparator. With `Dictionary` iteration order being unspecified across runs, ties on `updatedAt` (only possible with stale duplicate hook records sharing an exact-second timestamp) were already nondeterministic either way; consistency is just easier to reason about. The existing `testClaudeTranscriptHasConversationFiltersMetadataOnlyTranscripts` test gains four assertions that exercise the new helper with the same temp-dir setup, covering the kind-gate (non-Claude passes), the metadata-only filter, and a real-transcript pass. Defers greptile's P1 main-thread-I/O finding to a follow-up: the synchronous `Workspace.createPanel` path can't `await` Task.detached without async-ifying the entire restore chain (`AppDelegate` → `TabManager.restoreSessionSnapshot` → `Workspace.restoreSessionSnapshot` → `createPanel`). The empirical cost is bounded — transcripts with content return on the first `user`/`assistant` event, metadata-only transcripts hit EOF in <10 KB — but doing it properly belongs in a separate refactor.
…ack recency test Three changes in response to coderabbitai's review on the latest push: 1. Scope `agent_session_ended` to the ended sessionId so a late/duplicate SessionEnd hook for session A can't wipe a freshly-started session B that has reused the same panelId. `Workspace.markRestorableAgentSessionEnded` now takes `(panelId:, sessionId:)` and no-ops unless the currently restored snapshot's sessionId matches. The V1 command grew a required `--session=<id>` arg; both call sites in `CLI/cmux.swift` (claude-hook session-end and the generic-agent SessionEnd path) propagate it from the consumed hook record. 2. Fix `lineDeclaresConversationEvent` to tolerate JSON whitespace around the colon. JSON permits `"type" : "user"` (spaces before/after `:`), which the four-substring contains check would have misclassified as metadata-only. Replaces the substring scan with a precompiled NSRegularExpression (`"type"\s*:\s*"(?:user|assistant)"`) that costs <1 µs per line and stays correct for valid variations. 3. Add `testRestorableAgentIndexFallbackPicksMostRecentRecordPerPanelId` covering the recency rule for the panel-id fallback fold: two hook records sharing a panelId with different `updatedAt` timestamps must resolve to the newer record after a workspace UUID regenerates. Existing test only inserted one record so the recency assertion was implicit. Also extends the existing transcript test with a fourth fixture using the JSON whitespace variant to lock in manaflow-ai#2 against regression, and bumps file-length budgets for the four touched files (TSV stays sorted).
cmux's auto-resume types `cd '<cwd>' && '<agent>' --resume <id>` into a fresh shell. When the user deletes the project dir between sessions, the `cd` fails, `&&` short-circuits before the agent runs, and the panel sits at a dead "no such file or directory" prompt with no way back. Ghostty's child silently falls back to the parent cwd when its `working_directory` is missing (`ghostty/src/Command.zig:209`), so the shell still starts — but the `cd` typed via `initial_input` is text-level and can't share that fallback. Return nil from `AgentResumeCommandBuilder.resumeShellCommand` when the captured cwd no longer exists on disk. The caller chain (`SessionRestorableAgentSnapshot.resumeStartupInput` → `Workspace.createPanel`'s `restoredAgentResumeInput`) already treats a nil resume command as "skip auto-resume", so the panel opens to a fresh shell instead of the dead prompt. User can still `claude --resume <id>` manually if they want the conversation back. Adds `testResumeCommandIsNilWhenWorkingDirectoryDoesNotExist` covering the new behavior.
Main's growth in `CLI/cmux.swift` and `Sources/TerminalController.swift` since this branch forked, combined with our additions, pushed both files above the budgets set in commit 5. Bump them with modest headroom: - `CLI/cmux.swift`: 20900 → 20970 (current 20943; +27 headroom) - `Sources/TerminalController.swift`: 17460 → 17500 (current 17486; +14 headroom) `Sources/Workspace.swift` is now under its 14030 budget by 74 lines after main's refactor; left as-is since the budget is a debt cap, not a target. TSV sort order preserved.
e4ae52e to
d69f4e9
Compare
Two CodeRabbit findings on the prior round, both quick wins: 1. `AgentResumeCommandBuilder.resumeShellCommand`: switch the cwd guard from `FileManager.default.fileExists(atPath:)` to the two-arg overload with `isDirectory:`. The single-arg form returns true for both directories AND regular files, so a `cwd` pointing at a leftover file would have passed the guard and emitted `cd <file>` — exactly the failure mode commit 6 introduced the guard to prevent. 2. `RestorableAgentSessionIndex` panel-id fallback fold: was non-deterministic on `(panelId, updatedAt)` ties because the source `resolved` dictionary's iteration order is unspecified. Two stale records sharing a timestamp could flap across launches. Sort by `(updatedAt desc, sessionId desc)` before folding so the same record always wins on tie regardless of dictionary iteration order. Adds `testRestorableAgentIndexFallbackTieBreakIsDeterministic` covering the new behavior — two records sharing a panelId AND timestamp; assert the lexicographically greater sessionId wins. Bumps `cmuxTests/SessionPersistenceTests.swift` budget 2410 → 2470 to fit the new test (current count 2459); TSV sort still valid.
|
Quick status update — branch rebased onto current Two more CodeRabbit findings addressed in
Adds All 6 named regression tests pass via @coderabbitai please re-evaluate |
|
Re-triggering a full review now to evaluate all commits end-to-end. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/RestorableAgentSession.swift`:
- Around line 662-723: The current chunked scan in
claudeTranscriptHasConversation uses leftover.removeSubrange(...) which shifts
Data bytes on each newline and can cause O(n²) behavior for long metadata-only
transcripts; change the loop to keep an Int readOffset into leftover (append
chunks, search for newline starting at readOffset, advance readOffset past
processed bytes, and only compact leftover (by copying the unread slice into a
new Data and reset readOffset) when readOffset exceeds a threshold like
chunkSize or 64KB) while still calling lineDeclaresConversationEvent on each
extracted line; this preserves semantics but avoids repeated front-removal
costs.
- Around line 780-786: Remove the explicit private init that takes
snapshotsByPanel and snapshotsByPanelId and rely on Swift's synthesized
memberwise initializer instead; delete the init definition (the initializer that
assigns self.snapshotsByPanel and self.snapshotsByPanelId) and ensure existing
call sites such as static let empty and load(...) continue to compile using the
synthesized private memberwise init for the private let properties
snapshotsByPanel and snapshotsByPanelId.
🪄 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: dc940fca-4f47-4ab7-aa43-e3ced9c57a47
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (5)
CLI/cmux.swiftSources/RestorableAgentSession.swiftSources/TerminalController.swiftSources/Workspace.swiftcmuxTests/SessionPersistenceTests.swift
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
|
Hi @austinywang / @lawrencecchen — this PR is ready for human review when you have a minute. What it fixes (4 independent bugs in cmux's auto-resume flow):
What it includes:
Two known limitations, deferred deliberately:
GitHub still shows |
|
Hi @aml11! Glad to see this PR existed. I was trying to solve this same issue myself today, and only came across this after getting pretty deep into it. I tested this PR to see if it worked for auto-resuming claudes after a crash. Tested PR #3805 against a real-world repro on cmux 0.64.4 + this PR branch. Headline: the PR's four logical fixes (workspace-UUID lookup fallback, in-lifetime SessionEnd snapshot drop, metadata-only transcript filter, missing-cwd guard) all do what they say on the tin and resolve the bugs they target. But on my machine the user-facing outcome is still "auto-resume does not fire" — because the Empirical repro:
Restored terminal shows: The lookup fallback fires (the resume command is correctly built — that proves the workspace-UUID lookup fix is doing its job). But the bytes get echoed to the terminal's output side as visible text and the shell's input loop never consumes them. Scope note: I only verified the user-facing effect of the lookup fallback (87f4a4). The other commits in this PR (0268c2 SessionEnd in-lifetime drop, c0ba48 metadata-only transcript filter, ea2c01 missing-cwd guard) all target failure modes that would only be user-visible after delivery works. With Proposed complementary fix (additive — only replaces the delivery mechanism)Move resume delivery from 1. Add a 2. Add a new function to _cmux_maybe_auto_resume_claude() {
[[ -n "\$_CMUX_AUTO_RESUME_TRIED" ]] && return
_CMUX_AUTO_RESUME_TRIED=1
[[ -n "\$CMUX_SURFACE_ID" ]] || return
command -v claude >/dev/null 2>&1 || return
local cmux_bin="\${CMUX_BUNDLED_CLI_PATH:-cmux}"
local sid
sid=\$("\$cmux_bin" claude-hook last-session --only-live 2>/dev/null) || return
[[ -n "\$sid" ]] || return
print -ru2 -- "[cmux] Auto-resuming Claude session \$sid"
if claude --resume "\$sid"; then
exit # matches initialInput's panel-close-on-claude-exit semantics
fi
print -ru2 -- "[cmux] claude --resume failed; command pre-typed for retry"
print -z "claude --resume \$sid"
}Guard fires once per shell. Queries the same Replaces: the Total addition: ~75 lines across the shell-integration scripts + the CLI flag. No overlap with this PR's existing diff. I tested this approach on a separate local branch on top of cmux 0.64.4 + this PR's changes and auto-resume fires reliably in the same scenario where Small integration note re:
|
teamleaderleo
added
S2: major
This branch has not been deployed
3 participants
Summary
Fixes three independent bugs that prevent Claude/Codex/etc. session
auto-resume from working after a cmux relaunch.
Commit 1 — index lookup misses across launches.
RestorableAgentSessionIndexis keyed by(workspaceId, panelId),but
Workspace.idis regenerated as a freshUUID()on every cmuxlaunch (
Sources/Workspace.swift:7682). After the first relaunch, thesaved hook record's
workspaceIdno longer matches the live workspace,the strict index lookup misses, and autosave persists the panel
snapshot with
agent: nil. From then on agents never auto-resume.The fix adds a panel-id-only fallback. Strict
(workspaceId, panelId)still wins; only fall back to
panelId-only when the strict keymisses, picking the most recent record per panel.
Commit 2 — in-lifetime SessionEnd doesn't clear in-memory state.
After commit 1, restoration reliably populates
restoredAgentSnapshotsByPanelId[panelId]. When the user/exitsclaude in the same lifetime,
claude-hook session-endconsumes theon-disk record but doesn't clear the in-memory entry, so the next
save re-embeds the stale agent and the next launch tries to resume a
session that already exited. The fix adds a new
agent_session_endedv1 control command and wires
claude-hook session-end(plus thegeneric-agent SessionEnd path used by Codex, Gemini, etc.) to call
it alongside the existing
clear_agent_pid/clear_statuscalls.Commit 3 — auto-resume into "No conversation found" after rename.
When a user runs
/rename <name>immediately after Claude starts,before sending any prompt, Claude writes only metadata events
(
custom-title,agent-name,permission-mode) to the transcript.claude --resume <id>rejects metadata-only transcripts withNo conversation found with session ID: …. The fix derives Claude'stranscript path from the hook record's
cwdandsessionIdandfilters out metadata-only records during index load. Fail-open: if
the transcript is missing or unreadable, the record is kept so
Claude can produce its own canonical error.
Why
Empirical: on a machine with active Claude sessions,
~/.cmuxterm/claude-hook-sessions.jsoncarried 16 active records, but~/Library/Application Support/cmux/session-com.cmuxterm.app.jsonhadzero panels with an
agentfield. 7 panel UUIDs in the snapshotmatched
surfaceIdentries in the hook file — the data was there, thelookup just couldn't connect them across the workspace-UUID
regeneration. Bugs 2 and 3 were observed during the same investigation.
Closes #2941
Refs #3342
Refs #3322
Testing
xcodebuild testagainst the realcmuxTeststarget (Xcode 26.2,macOS 26.x, Apple Silicon). All 3 new tests pass:
testRestorableAgentIndexFallsBackToPanelIdAfterWorkspaceUUIDRegeneratestestMarkRestorableAgentSessionEndedNoopsWithoutEntrytestClaudeTranscriptHasConversationFiltersMetadataOnlyTranscriptsSources/RestorableAgentSession.swiftto pre-fix whilekeeping the test file shows the regression tests fail with
XCTUnwrap failed: expected non-nil valueand an assertion mismatchon metadata-only transcripts — i.e. the tests catch the bugs and the
fixes are causally responsible.
./scripts/reload.sh --tag fix-restore --launch, started a Claude session, Cmd+Q'd,relaunched. The restored panel auto-typed
cd '<cwd>' && 'claude' '--resume' '<id>'and Claude reattached./rename testin a fresh Claude sessionbefore any prompt, exited, restarted, observed
No conversation found with session ID: <id>from the auto-resume. With the fix, thehook record is filtered out during load and the panel restores as a
fresh shell instead.
(
python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv→ exit 0).Local repro command:
Demo Video
cmux.video.mp4
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Tests