Repository navigation
Fix Claude workflow resume transcript resolution - #5242
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughDetects Claude workflow-container sessions and computes an effective record pointing at the newest sibling ChangesClaude Workflow Session Resume Resolution
sequenceDiagram
participant Client as Load loop
participant Resolver as resolvedClaudeWorkflowRecord
participant Snapshot as Snapshot builder
participant Reconciler as Entry reconciliation
Client->>Resolver: request effectiveRecord for stored hook record
Resolver-->>Client: effectiveRecord(sessionId, transcriptPath, updatedAt)
Client->>Snapshot: build restorable snapshot using effectiveRecord
Snapshot-->>Client: snapshot with resolved sessionId/transcriptPath
Client->>Reconciler: compare effectiveRecord.updatedAt with existing entries
Reconciler-->>Client: decide keep/newest and PID matching
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (15 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes Claude workflow-directory session resume by detecting when a stored session ID is a directory container (not a transcript), resolving it to the newest sibling
Confidence Score: 3/5The fix is incomplete: workflow sessions that previously had a running process will be silently dropped from the restorable index after the process exits, which is the common case for sessions the user actually wants to resume. The resolution logic correctly maps the workflow container to a sibling transcript, but resolvedClaudeWorkflowRecord does not clear pid on the resolved record. The existing dead-process filter then drops sessions whose container process is no longer running — the common case for sessions the user wants to resume. The new test does not exercise this path because its fixture record has no PID. Sources/RestorableAgentSession.swift — specifically the resolvedClaudeWorkflowRecord return site and the test fixture in RestorableAgentSessionIndexTests.swift that needs a non-nil PID variant. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[load: iterate Claude hook records] --> B[resolvedClaudeWorkflowRecord]
B --> C{transcriptPath exists as regular file?}
C -->|yes| D[return record unchanged]
C -->|no| E[claudeWorkflowProjectDirs: check sessionId as directory]
E --> G{workflow container directory found?}
G -->|no| D
G -->|yes| H[newestClaudeSiblingTranscript: scan for newest .jsonl sibling]
H --> I{sibling found?}
I -->|no| D
I -->|yes| J[update sessionId and transcriptPath in resolvedRecord]
J --> K[pid still set to container process PID]
K --> L[liveScopedProcessID returns nil - container process is dead]
L --> M{guard: pid is nil OR liveProcessID not nil}
M -->|FAIL: pid non-nil and process dead| N[session dropped from resolved map]
M -->|PASS: pid was nil| O[resolved map updated correctly]
Reviews (4): Last reviewed commit: "fix: stabilize Claude sibling transcript..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if best == nil || modifiedAt >= best!.modifiedAt { | ||
| best = (sessionId, path, modifiedAt) | ||
| } |
There was a problem hiding this comment.
>= makes the winning entry depend on the undocumented iteration order of contentsOfDirectory. Using > keeps the first-encountered file as the stable choice on a tie, which is consistent across calls.
| if best == nil || modifiedAt >= best!.modifiedAt { | |
| best = (sessionId, path, modifiedAt) | |
| } | |
| if best == nil || modifiedAt > best!.modifiedAt { | |
| best = (sessionId, path, modifiedAt) | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| for projectDir in lookup.projectDirs(configRoot: root) { | ||
| appendIfWorkflowContainer( | ||
| projectRoot: (projectsRoot as NSString).appendingPathComponent(projectDir) | ||
| ) | ||
| } |
There was a problem hiding this comment.
For every Claude record that lacks an explicit transcriptPath — including ordinary (non-workflow) sessions — the fallback loop calls fileManager.fileExists(atPath:) once per entry returned by lookup.projectDirs(configRoot:). A user who has opened many repositories in Claude can accumulate hundreds of project directories, so for N Claude records × P project dirs this is O(N × P) stat calls on every RestorableAgentSessionIndex.load. A lightweight early-exit would help: if the CWD-derived candidate already matched (i.e., projectDirs is non-empty after the first loop), skip the full-project-dirs scan entirely, since a workflow container is almost certainly under the session's own working directory.
Rule Used: Flag production code that adds nested full-collect... (source)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 1266-1328: The fallback scan in claudeWorkflowProjectDirs is doing
a full per-record walk of all projectDirs for every sessionId; change it to
short-circuit and/or memoize: first attempt only the cwdCandidates and if any
are found return immediately (avoid iterating lookup.projectDirs), and add a
simple cache keyed by (configRoot, sessionId) — e.g., a private static
dictionary on RestorableAgentSession or a method on ClaudeTranscriptLookupCache
— to store previously computed projectDirs so subsequent calls to
claudeWorkflowProjectDirs for the same config root + sessionId return the cached
[String] instead of probing every project; ensure cache lookup/insertion
surrounds the loop that iterates lookup.projectDirs and preserves the existing
behavior for misses.
- Around line 1331-1359: The current
newestClaudeSiblingTranscript(in:excludingSessionId:fileManager:) automatically
returns the most recently modified sibling .jsonl which can incorrectly
associate an unrelated later transcript; change the logic to only resolve when
there is exactly one eligible transcript across all projectDirs: iterate
projectDirs and collect candidates that pass
claudeSessionIdIsSafeFilename(sessionId) and regularNonEmptyFileExists(atPath:),
using fileManager.contentsOfDirectory(atPath:), and if the candidate count == 1
return that sessionId/path, otherwise return nil (do not pick the newest). Keep
the same helper checks (claudeSessionIdIsSafeFilename,
regularNonEmptyFileExists) and return nil for zero or multiple matches so
records remain non-restorable unless deterministic.
🪄 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: 87b073ba-5d3b-4300-86b3-fcfc8f944088
📒 Files selected for processing (2)
Sources/RestorableAgentSession.swiftcmuxTests/RestorableAgentSessionIndexTests.swift
| let roots = lookup.configRoots(for: record) | ||
| guard !roots.isEmpty else { return record } | ||
| let candidateProjectDirs = claudeWorkflowProjectDirs( | ||
| for: record, | ||
| sessionId: sessionId, | ||
| roots: roots, | ||
| fileManager: fileManager, | ||
| lookup: lookup | ||
| ) | ||
| guard let resolved = newestClaudeSiblingTranscript( | ||
| in: candidateProjectDirs, | ||
| excludingSessionId: sessionId, | ||
| fileManager: fileManager | ||
| ) else { | ||
| return record | ||
| } | ||
|
|
||
| var resolvedRecord = record | ||
| resolvedRecord.sessionId = resolved.sessionId | ||
| resolvedRecord.transcriptPath = resolved.path | ||
| return resolvedRecord | ||
| } | ||
|
|
||
| private static func claudeWorkflowProjectDirs( | ||
| for record: RestorableAgentHookSessionRecord, | ||
| sessionId: String, | ||
| roots: [String], | ||
| fileManager: FileManager, | ||
| lookup: ClaudeTranscriptLookupCache | ||
| ) -> [String] { | ||
| var projectDirs: [String] = [] | ||
| var seen: Set<String> = [] | ||
|
|
||
| func appendIfWorkflowContainer(projectRoot: String) { | ||
| let workflowContainer = (projectRoot as NSString).appendingPathComponent(sessionId) | ||
| var isDirectory: ObjCBool = false | ||
| guard fileManager.fileExists(atPath: workflowContainer, isDirectory: &isDirectory), | ||
| isDirectory.boolValue else { | ||
| return | ||
| } | ||
| let standardized = (projectRoot as NSString).standardizingPath | ||
| guard seen.insert(standardized).inserted else { return } | ||
| projectDirs.append(standardized) | ||
| } | ||
|
|
||
| let cwdCandidates = [ | ||
| normalizedWorkingDirectory(record.launchCommand?.workingDirectory), | ||
| normalizedWorkingDirectory(record.cwd), | ||
| ].compactMap { $0 } | ||
| for root in roots { | ||
| let projectsRoot = (root as NSString).appendingPathComponent("projects") | ||
| for cwd in cwdCandidates { | ||
| appendIfWorkflowContainer( | ||
| projectRoot: (projectsRoot as NSString).appendingPathComponent(encodeClaudeProjectDir(cwd)) | ||
| ) | ||
| } | ||
| for projectDir in lookup.projectDirs(configRoot: root) { | ||
| appendIfWorkflowContainer( | ||
| projectRoot: (projectsRoot as NSString).appendingPathComponent(projectDir) | ||
| ) | ||
| } | ||
| } | ||
| return projectDirs |
There was a problem hiding this comment.
Cache or bound the project-directory fallback scan.
Lines 1266-1328 run inside the per-record load loop, but they still iterate every Claude project directory and probe <projectRoot>/<sessionId> for each Claude hook record. On installs with roughly 1000 workspaces/sessions this becomes a startup-time full rescan per target record. Please move this fallback behind a cache/index keyed by config root + session id, or otherwise stop after the cwd-derived candidates fail without walking the full project set. As per coding guidelines, "fail when the diff violates .github/review-bot-rules/algorithmic-complexity.md": nested full-collection scans and per-target rescans in production paths expected to handle about 1000 workspaces or similar user-owned records.
🤖 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 1266 - 1328, The fallback
scan in claudeWorkflowProjectDirs is doing a full per-record walk of all
projectDirs for every sessionId; change it to short-circuit and/or memoize:
first attempt only the cwdCandidates and if any are found return immediately
(avoid iterating lookup.projectDirs), and add a simple cache keyed by
(configRoot, sessionId) — e.g., a private static dictionary on
RestorableAgentSession or a method on ClaudeTranscriptLookupCache — to store
previously computed projectDirs so subsequent calls to claudeWorkflowProjectDirs
for the same config root + sessionId return the cached [String] instead of
probing every project; ensure cache lookup/insertion surrounds the loop that
iterates lookup.projectDirs and preserves the existing behavior for misses.
| private static func newestClaudeSiblingTranscript( | ||
| in projectDirs: [String], | ||
| excludingSessionId excludedSessionId: String, | ||
| fileManager: FileManager | ||
| ) -> (sessionId: String, path: String)? { | ||
| var best: (sessionId: String, path: String, modifiedAt: TimeInterval)? | ||
| for projectDir in projectDirs { | ||
| guard let children = try? fileManager.contentsOfDirectory(atPath: projectDir) else { | ||
| continue | ||
| } | ||
| for child in children where child.hasSuffix(".jsonl") { | ||
| let sessionId = String(child.dropLast(".jsonl".count)) | ||
| guard sessionId != excludedSessionId, | ||
| claudeSessionIdIsSafeFilename(sessionId) else { | ||
| continue | ||
| } | ||
| let path = (projectDir as NSString).appendingPathComponent(child) | ||
| guard regularNonEmptyFileExists(atPath: path, fileManager: fileManager) else { | ||
| continue | ||
| } | ||
| let modifiedAt = ((try? fileManager.attributesOfItem(atPath: path)[.modificationDate]) as? Date)? | ||
| .timeIntervalSince1970 ?? 0 | ||
| if best == nil || modifiedAt >= best!.modifiedAt { | ||
| best = (sessionId, path, modifiedAt) | ||
| } | ||
| } | ||
| } | ||
| guard let best else { return nil } | ||
| return (best.sessionId, best.path) |
There was a problem hiding this comment.
Don't auto-resolve to the newest sibling transcript.
Lines 1331-1359 always pick the most recently modified sibling .jsonl, but a Claude project directory can legitimately contain multiple transcripts over time. cmuxTests/RestorableAgentSessionIndexTests.swift Lines 165-184 already model that shape, so a later unrelated chat in the same repo can steal the workflow entry and make resume/fork target the wrong conversation. Please only resolve when you can link the workflow container to a single transcript deterministically; otherwise leave the record non-restorable.
🤖 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 1331 - 1359, The current
newestClaudeSiblingTranscript(in:excludingSessionId:fileManager:) automatically
returns the most recently modified sibling .jsonl which can incorrectly
associate an unrelated later transcript; change the logic to only resolve when
there is exactly one eligible transcript across all projectDirs: iterate
projectDirs and collect candidates that pass
claudeSessionIdIsSafeFilename(sessionId) and regularNonEmptyFileExists(atPath:),
using fileManager.contentsOfDirectory(atPath:), and if the candidate count == 1
return that sessionId/path, otherwise return nil (do not pick the newest). Keep
the same helper checks (claudeSessionIdIsSafeFilename,
regularNonEmptyFileExists) and return nil for zero or multiple matches so
records remain non-restorable unless deterministic.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/RestorableAgentSession.swift">
<violation number="1" location="Sources/RestorableAgentSession.swift:1322">
P2: This full `projectDirs` walk runs in the load path per Claude record, which can degrade startup as records and known project directories grow. Cache or bound this fallback lookup so the same config-root/session checks are not re-scanned repeatedly.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| projectRoot: (projectsRoot as NSString).appendingPathComponent(encodeClaudeProjectDir(cwd)) | ||
| ) | ||
| } | ||
| for projectDir in lookup.projectDirs(configRoot: root) { |
There was a problem hiding this comment.
P2: This full projectDirs walk runs in the load path per Claude record, which can degrade startup as records and known project directories grow. Cache or bound this fallback lookup so the same config-root/session checks are not re-scanned repeatedly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/RestorableAgentSession.swift, line 1322:
<comment>This full `projectDirs` walk runs in the load path per Claude record, which can degrade startup as records and known project directories grow. Cache or bound this fallback lookup so the same config-root/session checks are not re-scanned repeatedly.</comment>
<file context>
@@ -1239,6 +1246,119 @@ struct RestorableAgentSessionIndex: Sendable {
+ projectRoot: (projectsRoot as NSString).appendingPathComponent(encodeClaudeProjectDir(cwd))
+ )
+ }
+ for projectDir in lookup.projectDirs(configRoot: root) {
+ appendIfWorkflowContainer(
+ projectRoot: (projectsRoot as NSString).appendingPathComponent(projectDir)
</file context>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmuxTests/RestorableAgentSessionIndexTests.swift (1)
336-364:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCover the "newest sibling transcript wins" contract.
This fixture only creates one sibling
.jsonl, so a resolver that returns any sibling transcript would still pass. The PR objective is stricter: it must pick the newest sibling transcript. Please add at least one older sibling transcript and assert the newer session ID is the one used forsnapshot.sessionIdandresumeCommand.Suggested test tightening
let workflowContainerSessionId = "aaaaaaaa-1111-1111-1111-aaaaaaaaaaaa" + let olderSessionId = "cccccccc-3333-3333-3333-cccccccccccc" let resumableSessionId = "bbbbbbbb-2222-2222-2222-bbbbbbbbbbbb" @@ + let olderTranscriptURL = projectDir.appendingPathComponent("\(olderSessionId).jsonl", isDirectory: false) + try writeClaudeTranscript(sessionId: olderSessionId, transcriptURL: olderTranscriptURL, cwd: cwd) let siblingTranscriptURL = projectDir.appendingPathComponent("\(resumableSessionId).jsonl", isDirectory: false) try writeClaudeTranscript(sessionId: resumableSessionId, transcriptURL: siblingTranscriptURL, cwd: cwd) + try fm.setAttributes([.modificationDate: Date(timeIntervalSince1970: 1)], ofItemAtPath: olderTranscriptURL.path) + try fm.setAttributes([.modificationDate: Date(timeIntervalSince1970: 2)], ofItemAtPath: siblingTranscriptURL.path)🤖 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/RestorableAgentSessionIndexTests.swift` around lines 336 - 364, Create at least one older sibling transcript before writing the current resumableSessionId so the index must prefer the newest file; specifically, add a new UUID like olderSiblingId and call writeClaudeTranscript(sessionId: olderSiblingId, transcriptURL: projectDir.appendingPathComponent("\(olderSiblingId).jsonl", isDirectory: false), cwd: cwd) before writing the existing resumableSessionId file, then load RestorableAgentSessionIndex.load(...) and assert snapshot.sessionId == resumableSessionId and that snapshot.resumeCommand contains "'--resume' '\(resumableSessionId)'" to prove the newest sibling wins (use the existing symbols writeClaudeTranscript, resumableSessionId, RestorableAgentSessionIndex.load, snapshot.sessionId, snapshot.resumeCommand).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cmuxTests/RestorableAgentSessionIndexTests.swift`:
- Around line 336-364: Create at least one older sibling transcript before
writing the current resumableSessionId so the index must prefer the newest file;
specifically, add a new UUID like olderSiblingId and call
writeClaudeTranscript(sessionId: olderSiblingId, transcriptURL:
projectDir.appendingPathComponent("\(olderSiblingId).jsonl", isDirectory:
false), cwd: cwd) before writing the existing resumableSessionId file, then load
RestorableAgentSessionIndex.load(...) and assert snapshot.sessionId ==
resumableSessionId and that snapshot.resumeCommand contains "'--resume'
'\(resumableSessionId)'" to prove the newest sibling wins (use the existing
symbols writeClaudeTranscript, resumableSessionId,
RestorableAgentSessionIndex.load, snapshot.sessionId, snapshot.resumeCommand).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 59f39dfd-c4c5-4155-8e66-1ed36b77d0e4
📒 Files selected for processing (1)
cmuxTests/RestorableAgentSessionIndexTests.swift
170cfd1 to
c9cdc70
Compare
| guard effectiveRecord.pid == nil || liveProcessID != nil else { | ||
| continue | ||
| } |
There was a problem hiding this comment.
After resolvedClaudeWorkflowRecord remaps the container to a sibling transcript, effectiveRecord.pid still holds the workflow container process's PID. For the common case — the user wants to resume a completed workflow — that process is dead, so liveProcessID is nil. The guard effectiveRecord.pid == nil || liveProcessID != nil then skips the resolved[key] = entry assignment, removing the session from the restorable index even though a valid sibling transcript was found.
The resolved entry still ends up in hookCandidatesByPanel and hookCandidatesBySession (written before this guard), but those are only consulted by the detectedSnapshots loop for process-detected entries, not for hook-only sessions. In practice this means workflow sessions that had an associated process — which is the normal state — will never appear as restorable after the process exits, defeating the purpose of this fix.
The fix is to clear pid in resolvedClaudeWorkflowRecord when the session ID changes: the container's process is irrelevant to the sibling session. The test doesn't exercise this path because hookRecord(...) defaults pid to nil; adding a non-nil PID to the test fixture would reproduce the failure.
Dismissed after follow-up fix: sibling transcript tie-breaking now uses stable greater-than comparison.
This branch was successfully deployed
1 participant
Fixes https://github.com/manaflow-ai/cmux/issues/5224.\n\nSummary:\n- Detect Claude Workflow directory session containers during restorable-session load.\n- Resolve them to the newest sibling JSONL transcript so resume/fork uses a real Claude session id.\n- Cover the workflow-directory case in RestorableAgentSessionIndex tests.\n\nVerification:\n- git diff --cached --check before commit.\n- Local xcodebuild tests not run because repo policy forbids local test actions.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes resume for Claude workflow directory sessions by resolving container IDs to the newest sibling
.jsonltranscript (chosen by last-modified time) and using that real Claude session for indexing, resume, and fork. Fixes #5224..jsonl(by mtime), excluding the container ID and skipping resolution if a valid transcript already exists; propagate the resolvedsessionId/transcriptPaththrough indexing and process matching.projectsroots from encodedlaunchCommand.workingDirectory/cwdand known project dirs with safe filename checks; add tests confirming--resumeuses the real Claude session ID.Written for commit 50abbea. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests