Resume External Cli Sessions#712
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughThis PR adds external-session discovery, list/import APIs, chat provenance, PTY/session wiring, a desktop import browser, a TUI browser, and iOS import flows across the app surfaces. ChangesDesktop backend external sessions
Desktop renderer UI
ADE CLI
iOS app
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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-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: 5
🧹 Nitpick comments (7)
apps/desktop/src/main/services/ipc/registerIpc.ts (1)
4774-4785: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent
as constusage vs. every otherrequireAppContextServicescall site.All other calls in this file append
as constto the keys array (e.g.requireAppContextServices(ctx, ["laneService"] as const)), but these two omit it. Depending onrequireAppContextServices's generic signature, this could affect whetherctx.externalSessionsServiceis properly narrowed to non-null after the call.♻️ Proposed fix for consistency
- requireAppContextServices(ctx, ["externalSessionsService"]); + requireAppContextServices(ctx, ["externalSessionsService"] as const); return ctx.externalSessionsService.list(normalizeExternalSessionListArgs(arg)); }); ipcMain.handle(IPC.externalSessionsImport, async (_event, arg: unknown): Promise<ExternalSessionImportResult> => { const ctx = getCtx(); - requireAppContextServices(ctx, ["externalSessionsService"]); + requireAppContextServices(ctx, ["externalSessionsService"] as const); return ctx.externalSessionsService.importExternalSession(normalizeExternalSessionImportArgs(arg));🤖 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 `@apps/desktop/src/main/services/ipc/registerIpc.ts` around lines 4774 - 4785, The two external sessions IPC handlers are missing the same `as const` tuple annotation used by the other `requireAppContextServices` call sites, so `ctx.externalSessionsService` may not be narrowed correctly. Update the `externalSessionsList` and `externalSessionsImport` handlers in `registerIpc` to pass `["externalSessionsService"] as const` into `requireAppContextServices`, matching the surrounding IPC handlers and preserving the intended type narrowing.apps/ios/ADE/Views/Work/WorkImportSessionScreen.swift (1)
187-295: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueMissing disabled-state fallback when
cwdMatchesLaneis true but no CLI capability applies.When
cwdMatchesLaneis true and bothcaps.resumeInPlaceandcaps.forkare false, no CLI action is appended and, unlike the mismatched-cwd branch (Lines 280-291), no disabled placeholder explains why. Users would see only the chat actions (if any) with no indication that CLI resume/fork is unavailable for this session.🤖 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 `@apps/ios/ADE/Views/Work/WorkImportSessionScreen.swift` around lines 187 - 295, The actions(for:) builder in WorkImportSessionScreen is missing a disabled CLI fallback when cwdMatchesLane is true but both resumeInPlace and fork are unavailable, so add a placeholder WorkExternalSessionAction in that branch similar to the existing disabled resume action in the mismatched-cwd path. Update the conditional flow around caps.resumeInPlace and caps.fork so that, after checking the matching-cwd case, you append an explanatory disabled “Resume here” or equivalent action when no CLI capability applies, using lane.name/session context and setting enabled to false.apps/ade-cli/src/services/sync/syncRemoteCommandService.ts (1)
169-175: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winProvider allowlist duplicates the shared
ExternalSessionProviderunion at runtime.
EXTERNAL_SESSION_PROVIDERShand-listsclaude/codex/cursor/droid/opencodehere, while the desktopexternalSessionsService.tsindependently maintains its ownPROVIDERSarray for the same purpose (seeapps/desktop/src/main/services/externalSessions/externalSessionsService.ts, referenced inimportExternalSession'sPROVIDERS.includes(provider)check). If a provider is added/removed on one side and not the other, this remote command layer will either reject a provider the backend actually supports, or accept one it silently doesn't handle.Consider exporting a single runtime array (e.g. from the shared
externalSessions.tstypes file) and importing it in both places instead of maintaining two separate allowlists.♻️ Suggested direction
-const EXTERNAL_SESSION_PROVIDERS = new Set<ExternalSessionProvider>([ - "claude", - "codex", - "cursor", - "droid", - "opencode", -]); +import { EXTERNAL_SESSION_PROVIDERS as PROVIDER_LIST } from "../../shared/types/externalSessions"; +const EXTERNAL_SESSION_PROVIDERS = new Set<ExternalSessionProvider>(PROVIDER_LIST);🤖 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 `@apps/ade-cli/src/services/sync/syncRemoteCommandService.ts` around lines 169 - 175, The provider allowlist is duplicated between syncRemoteCommandService and the desktop external session handling, which can drift out of sync. Move the runtime provider list to a single shared export from the externalSessions types/module and have EXTERNAL_SESSION_PROVIDERS and the desktop PROVIDERS/PROVIDERS.includes(provider) logic both consume that same source. Update the related symbols in syncRemoteCommandService and externalSessionsService so additions or removals only need to happen once.apps/desktop/src/main/services/externalSessions/discoverCodex.ts (1)
47-64: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSorting re-stats each file on every comparison instead of once.
return out.sort((left, right) => (safeStat(right)?.mtimeMs ?? 0) - (safeStat(left)?.mtimeMs ?? 0));
safeStatis invoked twice per comparator call, turning an O(n) stat cost into O(n log n) syscalls (up to ~5000 files per the loop cap). On slow/network filesystems this adds unnecessary latency to a synchronous main-process discovery pass.♻️ Proposed fix: stat once per file, then sort
function walkCodexSessionFiles(root: string): string[] { const out: string[] = []; const stack = [root]; while (stack.length > 0 && out.length < 5000) { const dir = stack.pop()!; for (const entry of safeReadDir(dir)) { const filePath = path.join(dir, entry.name); if (entry.isDirectory()) { stack.push(filePath); continue; } if (entry.isFile() && (entry.name.endsWith(".jsonl") || entry.name.endsWith(".jsonl.zst"))) { out.push(filePath); } } } - return out.sort((left, right) => (safeStat(right)?.mtimeMs ?? 0) - (safeStat(left)?.mtimeMs ?? 0)); + const mtimes = new Map(out.map((filePath) => [filePath, safeStat(filePath)?.mtimeMs ?? 0])); + return out.sort((left, right) => (mtimes.get(right) ?? 0) - (mtimes.get(left) ?? 0)); }🤖 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 `@apps/desktop/src/main/services/externalSessions/discoverCodex.ts` around lines 47 - 64, The sorting in walkCodexSessionFiles is re-stat’ing files inside the comparator, which makes discovery much slower than necessary. Change the logic so each file’s mtime is fetched once before sorting, then sort using the cached timestamps rather than calling safeStat repeatedly inside the Array.sort comparator. Keep the fix localized to walkCodexSessionFiles and its out/safeStat flow.apps/desktop/src/main/services/externalSessions/discoveryUtils.ts (1)
45-59: 🚀 Performance & Scalability | 🔵 TrivialAll discovery I/O is synchronous on the Electron main process.
safeStat/safeReadDir(and thereadFilePrefix/readFileSuffixhelpers below) usestatSync/readdirSync/readSyncexclusively. Every provider scanner built on these (Claude/Codex/Droid) walks potentially large directory trees synchronously despite being declaredasync. For users with large session histories this can block the main process and stall IPC/UI responsiveness while the "import external session" browser loads.Worth considering an async fs variant (or offloading discovery to a worker/utility process) if session directories can grow large in practice. As per path instructions for
apps/desktop/src/**, main-process code should be checked for behaviors that can degrade the renderer/main separation and responsiveness.🤖 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 `@apps/desktop/src/main/services/externalSessions/discoveryUtils.ts` around lines 45 - 59, The discovery helpers in discoveryUtils are doing synchronous fs work on the Electron main process, which can block responsiveness when provider scanners like Claude/Codex/Droid traverse large session trees. Update safeStat, safeReadDir, and the readFilePrefix/readFileSuffix helpers to use async fs APIs (or move the traversal into a worker/utility process) so the scanner paths no longer rely on statSync/readdirSync/readSync. Keep the provider discovery flow async end-to-end by adjusting the callers in the related scanner logic to await the new async helpers.Source: Path instructions
apps/desktop/src/main/services/chat/agentChatService.ts (2)
9853-9865: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsolidate duplicated subscriber fan-out.
publishChatEnvelopeduplicates theonEvent?.(...)+eventSubscribersloop that already exists verbatim incommitChatEvent(Line 9583) andemitTransientChatEnvelope(Line 9657). Consider having those call sites delegate topublishChatEnvelopeto keep the error-isolation logic in one place.🤖 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 `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 9853 - 9865, The subscriber fan-out logic is duplicated across commitChatEvent, emitTransientChatEnvelope, and publishChatEnvelope, so centralize it in publishChatEnvelope and have the other two call sites delegate there. Update commitChatEvent and emitTransientChatEnvelope to remove their inline onEvent/eventSubscribers iteration and rely on publishChatEnvelope so the error-isolation and logging behavior stays in one place.
20620-20621: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShadows the module-level
claudeConfigDir.There is already a module-level
claudeConfigDir()at Line 1485. Declaring anotherclaudeConfigDirinsidecreateAgentChatServiceshadows it, and the two differ subtly (this onepath.resolves the result, the module one does not). Rename this local (e.g.resolveClaudeConfigDir) to avoid confusion and accidental divergence.🤖 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 `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 20620 - 20621, The local claudeConfigDir in createAgentChatService shadows the existing module-level claudeConfigDir() and can diverge in behavior. Rename the inner helper to something distinct like resolveClaudeConfigDir, then update its call sites in that scope so it is clear which config-dir resolver is being used and avoids confusion with the module-level function.
🤖 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 `@apps/ade-cli/src/tuiClient/app.tsx`:
- Around line 12395-12516: The external-session-browser key handler in app.tsx
is consuming bare r/R and p/P as hotkeys before the query input path can see
them, making those letters impossible to type in the search box. Update the key
handling around the external-session-browser branch so refresh/provider-cycle
only fires when the query is empty or search focus is not active, or switch
those actions to a modifier-based shortcut; keep the printableInput path free to
append r/p normally. Use the existing external-session-browser state and the
same kind of gating pattern used by ModelPicker’s searchMode to locate the right
place to adjust.
In `@apps/desktop/src/main/services/externalSessions/claudeSessionTransplant.ts`:
- Around line 96-137: The existing existence check in transplantClaudeSession is
non-atomic and the try/catch around fs.promises.access(targetPath) is relying on
a self-thrown Error, which is fragile; replace the rename-based move in
transplantClaudeSession with an atomic destination creation step that fails with
EEXIST when the target already exists, then remove the source only after the
destination is safely created. Keep the behavior centered around
transplantClaudeSession, ensure the duplicate-session path raises the intended
“already exists” error instead of overwriting, and add a test in the
externalSessions service coverage for the concurrent/duplicate target case.
In `@apps/desktop/src/main/services/externalSessions/discoveryUtils.ts`:
- Around line 197-211: The `firstUserTextFromRecords` helper is treating every
`type === "message"` record as user content even when an explicit non-user
`role` is present. Update the `isUser` logic in `firstUserTextFromRecords` so
`type: "message"` only counts as user when the resolved `role` is absent or
actually user-like, and not when nested `message.role`/`payload.message.role`
says `assistant` or another non-user role. Add a regression test covering a
`message` record with `role: "assistant"` before the real user turn to verify
the function still returns the correct user text.
In `@apps/desktop/src/main/services/externalSessions/externalSessionsService.ts`:
- Around line 287-306: The main-process capability probe in droidForkAvailable()
blocks on spawnSync, which can freeze Electron for up to 3 seconds on first use.
Replace the synchronous probe with an async spawn-based check, make
capabilitiesFor() async if needed, and update its call sites in list() and
importExternalSession() to await the result so external session handling no
longer blocks the main thread.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 4754-4772: The normalizeExternalSessionImportArgs parser currently
coerces missing or non-string sessionId and laneId to empty strings, which is
inconsistent with the other IPC validators. Update this function to validate
both fields explicitly as required strings and throw an error when either is
missing or not a string, matching the existing provider/target/mode checks in
registerIpc.ts and keeping ExternalSessionImportArgs strict.
---
Nitpick comments:
In `@apps/ade-cli/src/services/sync/syncRemoteCommandService.ts`:
- Around line 169-175: The provider allowlist is duplicated between
syncRemoteCommandService and the desktop external session handling, which can
drift out of sync. Move the runtime provider list to a single shared export from
the externalSessions types/module and have EXTERNAL_SESSION_PROVIDERS and the
desktop PROVIDERS/PROVIDERS.includes(provider) logic both consume that same
source. Update the related symbols in syncRemoteCommandService and
externalSessionsService so additions or removals only need to happen once.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 9853-9865: The subscriber fan-out logic is duplicated across
commitChatEvent, emitTransientChatEnvelope, and publishChatEnvelope, so
centralize it in publishChatEnvelope and have the other two call sites delegate
there. Update commitChatEvent and emitTransientChatEnvelope to remove their
inline onEvent/eventSubscribers iteration and rely on publishChatEnvelope so the
error-isolation and logging behavior stays in one place.
- Around line 20620-20621: The local claudeConfigDir in createAgentChatService
shadows the existing module-level claudeConfigDir() and can diverge in behavior.
Rename the inner helper to something distinct like resolveClaudeConfigDir, then
update its call sites in that scope so it is clear which config-dir resolver is
being used and avoids confusion with the module-level function.
In `@apps/desktop/src/main/services/externalSessions/discoverCodex.ts`:
- Around line 47-64: The sorting in walkCodexSessionFiles is re-stat’ing files
inside the comparator, which makes discovery much slower than necessary. Change
the logic so each file’s mtime is fetched once before sorting, then sort using
the cached timestamps rather than calling safeStat repeatedly inside the
Array.sort comparator. Keep the fix localized to walkCodexSessionFiles and its
out/safeStat flow.
In `@apps/desktop/src/main/services/externalSessions/discoveryUtils.ts`:
- Around line 45-59: The discovery helpers in discoveryUtils are doing
synchronous fs work on the Electron main process, which can block responsiveness
when provider scanners like Claude/Codex/Droid traverse large session trees.
Update safeStat, safeReadDir, and the readFilePrefix/readFileSuffix helpers to
use async fs APIs (or move the traversal into a worker/utility process) so the
scanner paths no longer rely on statSync/readdirSync/readSync. Keep the provider
discovery flow async end-to-end by adjusting the callers in the related scanner
logic to await the new async helpers.
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 4774-4785: The two external sessions IPC handlers are missing the
same `as const` tuple annotation used by the other `requireAppContextServices`
call sites, so `ctx.externalSessionsService` may not be narrowed correctly.
Update the `externalSessionsList` and `externalSessionsImport` handlers in
`registerIpc` to pass `["externalSessionsService"] as const` into
`requireAppContextServices`, matching the surrounding IPC handlers and
preserving the intended type narrowing.
In `@apps/ios/ADE/Views/Work/WorkImportSessionScreen.swift`:
- Around line 187-295: The actions(for:) builder in WorkImportSessionScreen is
missing a disabled CLI fallback when cwdMatchesLane is true but both
resumeInPlace and fork are unavailable, so add a placeholder
WorkExternalSessionAction in that branch similar to the existing disabled resume
action in the mismatched-cwd path. Update the conditional flow around
caps.resumeInPlace and caps.fork so that, after checking the matching-cwd case,
you append an explanatory disabled “Resume here” or equivalent action when no
CLI capability applies, using lane.name/session context and setting enabled to
false.
🪄 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: CHILL
Plan: Pro
Run ID: 9b503b76-4524-4fb4-b935-67915b1f8bb2
⛔ Files ignored due to path filters (4)
apps/ios/ADE.xcodeproj/project.pbxprojis excluded by!**/*.xcodeproj/project.pbxprojdocs/features/chat/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**
📒 Files selected for processing (58)
apps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/ade-cli/src/services/sync/syncHostService.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.test.tsapps/ade-cli/src/services/sync/syncRemoteCommandService.tsapps/ade-cli/src/services/sync/syncService.tsapps/ade-cli/src/tuiClient/__tests__/externalSessionBrowser.test.tsapps/ade-cli/src/tuiClient/app.tsxapps/ade-cli/src/tuiClient/components/ModelPicker/ModelPickerPane.tsxapps/ade-cli/src/tuiClient/components/RightPane.tsxapps/ade-cli/src/tuiClient/externalSessionBrowser.tsapps/ade-cli/src/tuiClient/modelState.tsapps/ade-cli/src/tuiClient/types.tsapps/desktop/resources/ade-cli-help.txtapps/desktop/src/main/main.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/externalChatHistoryImport.test.tsapps/desktop/src/main/services/chat/externalChatHistoryImport.tsapps/desktop/src/main/services/externalSessions/claudeSessionTransplant.tsapps/desktop/src/main/services/externalSessions/discoverClaude.tsapps/desktop/src/main/services/externalSessions/discoverCodex.tsapps/desktop/src/main/services/externalSessions/discoverCursor.tsapps/desktop/src/main/services/externalSessions/discoverDroid.tsapps/desktop/src/main/services/externalSessions/discoverOpenCode.tsapps/desktop/src/main/services/externalSessions/discoverProviders.test.tsapps/desktop/src/main/services/externalSessions/discoveryUtils.tsapps/desktop/src/main/services/externalSessions/externalSessionsService.test.tsapps/desktop/src/main/services/externalSessions/externalSessionsService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/sessions/sessionService.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/terminals/SessionCard.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/terminals/WorkStartSurface.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/renderer/components/terminals/importSessions/ImportSessionBrowser.tsxapps/desktop/src/renderer/components/terminals/importSessions/affordances.test.tsapps/desktop/src/renderer/components/terminals/importSessions/affordances.tsapps/desktop/src/renderer/components/terminals/importSessions/contract.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/externalSessions.tsapps/desktop/src/shared/types/index.tsapps/desktop/src/shared/types/sessions.tsapps/desktop/src/shared/types/sync.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Work/WorkImportSessionScreen.swiftapps/ios/ADE/Views/Work/WorkNewChatScreen.swiftapps/ios/ADE/Views/Work/WorkRootScreen.swift
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa42cec080
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "unsubscribe", | ||
| ], | ||
| search: ["query", "indexStatus", "rebuildIndex"], | ||
| "external-sessions": ["list", "import"], |
There was a problem hiding this comment.
When a non-CTO agent calls run_ade_action for external-sessions.list with scope:"all" or external-sessions.import, this allowlist makes the calls reachable, but the RPC dispatcher only applies non-CTO scoping for pty/terminal/chat/search (apps/ade-cli/src/adeRpcServer.ts:3177-3207). That lets ordinary agent sessions enumerate host-wide external CLI cwd/title/preview data and import a resume-in-place session that may start a tracked PTY outside the agent's lane, bypassing the existing PTY/lane authorization.
Useful? React with 👍 / 👎.
| const droidForkAvailable = (): boolean => { | ||
| if (droidForkProbe !== null) return droidForkProbe; | ||
| startDroidForkProbe(); | ||
| return PROVIDER_CAPABILITIES.droid.fork; |
There was a problem hiding this comment.
On a first list/import against an installation whose droid --help does not include --fork, droidForkProbe is still null, so this returns the default capability true while the async probe is in flight. Both the UI capability list and the import-time guard use capabilitiesFor, so a user can immediately choose fork and ADE will launch droid --fork ... even though the later probe would disable it; default to false/pending until the probe completes or make the import path await the check.
Useful? React with 👍 / 👎.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20050aa4d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let targetTranscriptPath = source.transcriptPath; | ||
| let targetLines = sourceRead.lines; | ||
| let targetTranscriptTruncated = sourceRead.truncated; | ||
| if (!isSameCwd(sourceCwd, laneWorktreePath)) { |
There was a problem hiding this comment.
When the user selects Fork as ADE chat for a Claude session whose cwd already matches the target lane, this condition skips transplantClaudeSession, so targetClaudeSessionId remains the original externalSessionId despite args.fork === true. The imported ADE chat then resumes and writes to the original Claude session instead of a copied session; the fork path needs to run whenever args.fork is requested, not only when the cwd changes.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4331902a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| && isExternalSessionProviderName(provider) | ||
| && mode | ||
| && sessionId | ||
| && (target === "chat" || externalSessionImportUsesSourceRunCwd(provider, mode)) |
There was a problem hiding this comment.
For non-CTO run_ade_action:external-sessions.import, this condition skips the source summary/cwd check whenever the CLI will run in the lane, such as Codex resume or Claude fork. Those imports still attach or copy the external thread/transcript into the new session, so a caller that knows an outside-project session ID can import history that external-sessions.list would have filtered out for the lane. Require the source summary to be within laneCwd before allowing these history-bearing imports.
Useful? React with 👍 / 👎.
| backfillDelayMs: 10_000, | ||
| }); | ||
| searchServiceHolder.current = searchService; | ||
| const externalSessionsService = createExternalSessionsService({ |
There was a problem hiding this comment.
This creates externalSessionsService for the desktop project context, but the createSyncService call below never receives getExternalSessionsService or the service itself. When the desktop app is acting as the sync host, the new iOS work.listExternalSessions / work.importExternalSession commands resolve through that hook and will fail with "External sessions service not available" instead of opening the import browser.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/main/services/chat/agentChatService.ts (2)
20742-20818: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winClean up the transplanted Claude transcript if session creation fails
The Claude import path creates a forked transcript before
createSession, but the failure path only deletes the ADE session. IfcreateSessionorensureManagedSessionfails after a successful transplant, the new Claude transcript/session is left behind in~/.claude/projects/.... Add best-effort cleanup for the transplanted session/path here, similar to the Codex fork rollback.🤖 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 `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 20742 - 20818, The Claude import flow in agentChatService creates a transplanted session before createSession, but the catch block only rolls back the ADE session. Update the failure path around transplantClaudeSession/createSession/ensureManagedSession to also best-effort clean up the transplanted Claude session and target transcript path when creation fails, similar to the Codex fork rollback, using the existing externalChatImportError and cleanup logging patterns.
20680-20690: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse a literal specifier for this dynamic import.
import(modulePath)is not statically analyzable by tsup/esbuild, soclaudeSessionTransplantmay be omitted from the desktop main output and this path will fail at runtime.♻️ Use a literal import specifier
- const modulePath = "../externalSessions/claudeSessionTransplant"; - return await import(modulePath) as ClaudeSessionTransplantModule; + return await import("../externalSessions/claudeSessionTransplant") as ClaudeSessionTransplantModule;🤖 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 `@apps/desktop/src/main/services/chat/agentChatService.ts` around lines 20680 - 20690, The dynamic import in loadClaudeSessionTransplant is using a variable specifier, which prevents tsup/esbuild from statically including the module. Replace import(modulePath) with a literal import path for "../externalSessions/claudeSessionTransplant" so the ClaudeSessionTransplantModule is bundled into the desktop main output and the runtime fallback path works correctly.Source: Coding guidelines
🧹 Nitpick comments (1)
apps/desktop/src/main/services/externalSessions/externalSessionsService.ts (1)
317-321: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
capabilitiesForreportsfork: falsefor droid on the firstlist()call.
capabilitiesForuses the synchronousdroidForkAvailable(), which returnsfalseand only kicks off the probe on first invocation. So the initiallist()surfacesfork/forkIntoDifferentCwdasfalsefor droid even when the installed CLI supports--fork; the correct value only appears on subsequent lists after the probe resolves. The actual import path is unaffected (it awaitsresolveDroidForkAvailable()), so this is a UI-only staleness. Consider warming the probe at construction so the first list is accurate.♻️ Warm the probe at service creation
const resolveDroidForkAvailable = async (): Promise<boolean> => { if (droidForkProbe !== null) return droidForkProbe; return startDroidForkProbe(); }; + // Warm the probe early so the first list() reports accurate droid fork capability. + if (droidForkProbe === null) void startDroidForkProbe();🤖 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 `@apps/desktop/src/main/services/externalSessions/externalSessionsService.ts` around lines 317 - 321, `capabilitiesFor` is reading the droid fork capability too early, so the first `list()` can return stale `fork` values. Warm the droid fork probe when `ExternalSessionsService` is created (or otherwise ensure the probe starts before `list()` uses `droidForkAvailable()`), so `capabilitiesFor` for the "droid" provider can return the resolved capability on the initial call. Keep the import path behavior unchanged, since `resolveDroidForkAvailable()` already waits for the real result.
🤖 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 `@apps/ade-cli/src/adeRpcServer.ts`:
- Around line 2484-2508: Normalize the missing target case in
scopeExternalSessionsImportArgs by treating an absent target the same as the CLI
path before the lane authorization check. Update the conditional around the
external session cwd validation so importExternalSession’s non-chat fallback is
covered, using the existing symbols scopeExternalSessionsImportArgs,
importExternalSession, externalSessionImportUsesSourceRunCwd, and
externalSessionsAccessDenied to keep the guard consistent.
In `@apps/desktop/src/main/services/externalSessions/externalSessionsService.ts`:
- Around line 398-405: Thread enforceLaneScopeCwd through the desktop chat
import flow so the lane-containment check in externalSessionsService runs for
desktop imports too. Update the payload built in ImportSessionBrowser.tsx, pass
the field through preload.ts unchanged, and extend
ExternalSessionImportArgs/related call sites so importSession receives
enforceLaneScopeCwd and can apply the existing sourceCwd vs realish(scopeRoot)
validation.
---
Outside diff comments:
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 20742-20818: The Claude import flow in agentChatService creates a
transplanted session before createSession, but the catch block only rolls back
the ADE session. Update the failure path around
transplantClaudeSession/createSession/ensureManagedSession to also best-effort
clean up the transplanted Claude session and target transcript path when
creation fails, similar to the Codex fork rollback, using the existing
externalChatImportError and cleanup logging patterns.
- Around line 20680-20690: The dynamic import in loadClaudeSessionTransplant is
using a variable specifier, which prevents tsup/esbuild from statically
including the module. Replace import(modulePath) with a literal import path for
"../externalSessions/claudeSessionTransplant" so the
ClaudeSessionTransplantModule is bundled into the desktop main output and the
runtime fallback path works correctly.
---
Nitpick comments:
In `@apps/desktop/src/main/services/externalSessions/externalSessionsService.ts`:
- Around line 317-321: `capabilitiesFor` is reading the droid fork capability
too early, so the first `list()` can return stale `fork` values. Warm the droid
fork probe when `ExternalSessionsService` is created (or otherwise ensure the
probe starts before `list()` uses `droidForkAvailable()`), so `capabilitiesFor`
for the "droid" provider can return the resolved capability on the initial call.
Keep the import path behavior unchanged, since `resolveDroidForkAvailable()`
already waits for the real result.
🪄 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: CHILL
Plan: Pro
Run ID: 546e46d9-7a63-4f72-bf5b-ad1a985eda93
📒 Files selected for processing (12)
apps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/tuiClient/app.tsxapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/externalSessions/claudeSessionTransplant.tsapps/desktop/src/main/services/externalSessions/discoverProviders.test.tsapps/desktop/src/main/services/externalSessions/discoveryUtils.test.tsapps/desktop/src/main/services/externalSessions/discoveryUtils.tsapps/desktop/src/main/services/externalSessions/externalSessionsService.test.tsapps/desktop/src/main/services/externalSessions/externalSessionsService.tsapps/desktop/src/main/services/ipc/registerIpc.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/main/services/externalSessions/discoveryUtils.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/desktop/src/main/services/externalSessions/discoverProviders.test.ts
- apps/desktop/src/main/services/ipc/registerIpc.ts
- apps/desktop/src/main/services/externalSessions/discoveryUtils.ts
- apps/ade-cli/src/tuiClient/app.tsx
| const provider = asOptionalTrimmedString(scopedArgs.provider); | ||
| const mode = asOptionalTrimmedString(scopedArgs.mode); | ||
| const target = asOptionalTrimmedString(scopedArgs.target); | ||
| const sessionId = asOptionalTrimmedString(scopedArgs.sessionId); | ||
| if (target === "chat") { | ||
| scopedArgs.enforceLaneScopeCwd = laneCwd; | ||
| } | ||
| if ( | ||
| (target === "chat" || target === "cli") | ||
| && isExternalSessionProviderName(provider) | ||
| && mode | ||
| && sessionId | ||
| && (target === "chat" || externalSessionImportUsesSourceRunCwd(provider, mode)) | ||
| ) { | ||
| const summary = await findExternalSessionSummaryForAuthorization( | ||
| runtime, | ||
| method, | ||
| provider, | ||
| sessionId, | ||
| laneId, | ||
| laneCwd, | ||
| ); | ||
| const runCwd = asOptionalTrimmedString(summary?.cwd); | ||
| if (!runCwd || !isPathInsideOrEqual(laneCwd, runCwd)) externalSessionsAccessDenied(method); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm service defaults for missing target/mode and whether resume runs in sourceCwd / fork transplants from scope "all".
fd -a 'externalSessionsService.ts' apps/desktop/src/main/services/externalSessions \
--exec sed -n '288,447p'Repository: arul28/ADE
Length of output: 7446
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the RPC-side logic around the cited lines and any helpers that normalize import args.
sed -n '2440,2525p' apps/ade-cli/src/adeRpcServer.ts
printf '\n==== search for import arg normalization ====\n'
rg -n "scopeExternalSessionsImportArgs|enforceLaneScopeCwd|externalSessionImportUsesSourceRunCwd|target === \"chat\"|mode === \"resume\"|mode === \"fork\"" apps/ade-cli/src/adeRpcServer.ts apps/ade-cli/src -g '!**/dist/**' -g '!**/build/**'Repository: arul28/ADE
Length of output: 5433
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the RPC action schema / call site for external-sessions.import and any validation of target/mode.
sed -n '3325,3395p' apps/ade-cli/src/adeRpcServer.ts
printf '\n==== search import action definitions and tests ====\n'
rg -n "external-sessions\.import|target:|mode:" apps/ade-cli/src/adeRpcServer.ts apps/ade-cli/src/adeRpcServer.test.ts apps/ade-cli/src -g '!**/dist/**' -g '!**/build/**'Repository: arul28/ADE
Length of output: 27532
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where the external-sessions.import payload is defined to see whether target/mode are required or optional.
rg -n "external-sessions\.import|externalSessionsImport|target.*optional|mode.*optional|zod|schema" apps/ade-cli/src -g '!**/dist/**' -g '!**/build/**'Repository: arul28/ADE
Length of output: 1651
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant test area around the import-scoping behavior.
sed -n '2720,2885p' apps/ade-cli/src/adeRpcServer.test.tsRepository: arul28/ADE
Length of output: 6408
Normalize missing target to the CLI path before lane authorization.
scopeExternalSessionsImportArgs() only runs the cwd check when target is exactly "chat" or "cli" and mode is present. Since importExternalSession() treats any non-chat target as CLI, omitting target skips this guard while still reaching the CLI resume path. The claude fork path is already blocked later by the service’s cwd equality check, so the gap here is the missing-target case.
🤖 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 `@apps/ade-cli/src/adeRpcServer.ts` around lines 2484 - 2508, Normalize the
missing target case in scopeExternalSessionsImportArgs by treating an absent
target the same as the CLI path before the lane authorization check. Update the
conditional around the external session cwd validation so
importExternalSession’s non-chat fallback is covered, using the existing symbols
scopeExternalSessionsImportArgs, importExternalSession,
externalSessionImportUsesSourceRunCwd, and externalSessionsAccessDenied to keep
the guard consistent.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0099f43f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const transplant = await transplantClaudeSession({ | ||
| sessionId, | ||
| sourceCwd, | ||
| targetCwd: laneCwd, | ||
| fork: true, | ||
| }); |
There was a problem hiding this comment.
When Claude sessions are discovered from a custom CLAUDE_CONFIG_DIR (or a service-level homeDir) and the user forks one into a different lane, this transplant path does not pass the config directory used during discovery. transplantClaudeSession therefore falls back to ~/.claude and fails to find the source transcript that was just listed, so Fork into this lane is broken for non-default Claude config locations. Carry the same Claude config root into this call.
Useful? React with 👍 / 👎.
| await forkedProviderRuntime.request("thread/archive", { | ||
| threadId: forkedProviderThreadId, | ||
| }, { timeoutMs: CODEX_ARCHIVE_REQUEST_TIMEOUT_MS }); |
There was a problem hiding this comment.
When a Codex chat import forks successfully but the subsequent thread/read or import work fails because this Codex runtime is unhealthy, this cleanup sends thread/archive through forkedProviderRuntime, the same process that just failed. That archive request can also reject and is only logged, leaving the provider fork orphaned while ADE deletes the chat and reports the import as failed; use a fresh runtime/retry path or keep the ADE session until cleanup succeeds.
Useful? React with 👍 / 👎.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
1 similar comment
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
@codex review |
…code) Adds discovery + import of provider CLI sessions started outside ADE — in a plain terminal — as either a tracked ADE CLI PTY or (Claude/Codex) a full ADE chat with imported history. Works on desktop, the `ade code` TUI, and iOS via sync remote commands; supports forking a session into a different lane where the provider allows it. - externalSessions service: per-provider discovery (~/.claude, ~/.codex, ~/.cursor, ~/.factory, opencode CLI), capability matrix, and import that spawns a tracked resume/fork PTY via buildTrackedCliResumeCommand. - Chat-level import: agentChatService.importExternalChatSession seeds the ADE transcript from external JSONL/thread history (32MB tail + 2000-event caps); Claude cross-cwd uses a non-destructive copy+rekey transplant, Codex uses thread/read + thread/fork. - IPC ade.externalSessions.list/import, action domain external-sessions, daemon wiring so remote-bound desktop and mobile route through the runtime. - Sync commands work.listExternalSessions/importExternalSession (viewer-allowed, same trust as work.startCliSession). - Desktop new-chat redesign (Shell + Import session under the lane picker), import browser, Imported badge; TUI import browser reusing the shared affordance helper; iOS WorkImportSessionScreen mirroring the same rules. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… TOCTOU, role detection, non-blocking probe, IPC validation - discoverProviders.test.ts: cursor fixture uses a writable dash-free mkdtemp cwd (fixes CI shard-6 EACCES on /private/tmp) with a round-trip precondition. - Codex fork chat-import: track the forked thread id and best-effort thread/archive it on failure so a failed import doesn't leak a provider-side thread. - TUI import search: r/p refresh/provider-cycle only fire when the query is empty; once typing starts they append as literal characters. - claudeSessionTransplant: replace rename/access (silent-overwrite TOCTOU + fragile self-catch) with link+unlink that fails atomically with EEXIST. - discoveryUtils firstUserTextFromRecords: explicit assistant/system/tool roles override type:"message" so assistant text no longer becomes the session title. - externalSessionsService: droid --fork probe is now memoized async execFile (1500ms), never a blocking spawnSync on the main-process list/import path. - registerIpc: reject missing/non-string sessionId/laneId instead of coercing to "". Declined the two "viewer scope" findings: ADE has a single paired-device trust class (viewerAllowed), no lesser viewer tier; the paired device is a trusted controller at parity with work.startCliSession, and machine-wide listing is an explicit opt-in. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…k probe conservative - adeRpcServer: external-sessions.list/import via run_ade_action are now scoped for non-CTO agents the same way pty/terminal are — forced to scope:"project", constrained to the caller's authorized lane, and import rejected outside it. Closes the gap where an in-lane agent could enumerate host-wide sessions or spawn a PTY outside its lane. (Sync/paired-device path unchanged — intentional.) - externalSessionsService: droid --fork capability reports false until the async probe resolves (no more optimistic fork:true race), and the import fork path awaits the probe so ADE never launches `droid --fork` on a CLI that lacks it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…fork - adeRpcServer + externalSessionsService: the non-CTO lane gate now also covers target:"chat" imports — the source session's cwd must be inside the caller's authorized lane worktree (dispatcher denial + service-level defense-in-depth), so an in-lane agent can't seed another lane's / host-wide history into its lane. - agentChatService: "Fork as ADE chat" for a Claude session whose cwd already matches the lane now always runs transplantClaudeSession(fork:true) to a fresh session id, instead of resuming and writing into the user's original session. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…desktop sync host - externalSessionsService + adeRpcServer: the source-cwd-in-lane authorization now runs before EVERY import branch (Codex resume, Claude fork, chat), not just chat, so a non-CTO agent can never pull an outside-project session's history/PTY into its lane regardless of provider or mode. Class-root fix for the prior chat-only gap. - main.ts: desktop sync host now passes getExternalSessionsService into createSyncService, so iOS work.listExternalSessions/importExternalSession resolve when hosting from the desktop app (previously failed "service not available"). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…der logos - New-chat surface: Shell / Import session buttons are now content-sized and centered (dropped the flex-1 50/50 split that cramped "Import session" and read as off-center) with comfortable padding. - Import browser: provider filter chips and session rows now use ADE's real ProviderChip / ToolLogo (Claude/Codex/Cursor/Droid/OpenCode logos) instead of text + a colored dot; refresh control aligned; consistent chip states. - iOS WorkImportSessionScreen: provider filter chips use the ProviderClaude/ Codex/Cursor/Droid/OpenCode imagesets via providerAssetName, matching desktop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nline explanations
- Rename actions to a single consistent 2×2: Open/Fork as ADE chat, Open/Fork as
CLI session (was the confusing Resume here / Resume (runs in …) / Fork into this
lane). Open = continue the original, Fork = start a copy.
- Header now makes clear these are terminal-started CLI sessions ("Import a CLI
session" + provider list), with an inline legend explaining Open/Fork and
ADE-chat/CLI-session — no more tooltip-only meanings.
- Cwd-locked providers show a quiet "runs in its original folder" note under the
row instead of baking the path into the button label; possiblyActive shows an
inline "recently active — fork to avoid conflicts" caption.
- Provider marks are bare, larger logos (no colored circle border).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Matches desktop: consistent 2×2 action labels (Open/Fork as ADE chat, Open/Fork as CLI session), header clarifying these are terminal-started CLI sessions, an inline legend explaining Open/Fork and chat/CLI, a "runs in its original folder" sub-note for cwd-locked providers, a "recently active" caption for possibly-open sessions, and bare provider logos (no circle). xcodebuild green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…le preview UI (ImportSessionBrowser): enlarge modal to ~980×860; header is just "Import session" (no subtitle); remove the search-box legend and all per-row caption lines; heading is the real session title when the tool persisted one, else a path + timestamp fallback; the first message snippet moves into a collapsible "Preview" drawer instead of duplicating the heading; compact IMPORTED / MAY BE OPEN ELSEWHERE badges kept; roomier rows. Discovery: stop deriving `title` from the first user message. `title` is now a real persisted provider title or null (cleanSessionTitle strips placeholders like "New Session"); `preview` is the distinct message snippet. Per provider: Claude/Cursor → null title; Codex → thread name (index) or null; Droid → real title unless placeholder; OpenCode → title/name. Tests updated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ading, collapsible preview Matches desktop: header is just "Import session" (no subtitle), no inline legend or per-row captions, heading = real title or path + timestamp, message snippet in a collapsible Preview disclosure, clean grouped action buttons, compact badges. xcodebuild green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ADE, faster discovery - Centering: LaneDialogShell card + BorderBeam now fill Dialog.Content width, so large modals stay centered. - Progressive loading: the browser queries each provider in parallel and renders results as they arrive (fast file-based providers show instantly; the OpenCode CLI fills in), instead of blocking on all five. - Discovery perf: stat-first, sort by mtime, read content only for the top-N files per provider instead of every session file. - Open in ADE: sessions ADE already tracks expose importedSessionRef; those rows show a single "Open in ADE" that jumps to the existing chat/CLI session (onOpenExisting threaded through Work surfaces), while Fork stays available. - Preview: heading is real-title-or-path+time; message snippet stays in the collapsible drawer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
importExternalChatSession loaded claudeSessionTransplant via a dynamic import() with a variable path. tsup leaves that unresolved in the bundled ade-cli brain (cli.cjs), so cross-cwd "Fork as ADE chat" (and any cross-cwd Claude chat import) failed at runtime with "Cannot find module .../externalSessions/claudeSessionTransplant". Import it statically so the bundler includes it. Affects the daemon/remote/mobile brain, not just desktop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… a chip above the composer - Lane picker box is taller with a larger title font. - Removed the standalone refresh button (pull-to-refresh + on-appear reload cover it). - The Import session affordance now sits just above the composer as a compact chip (like context chips in chat), only when a concrete lane is selected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… cursor discovery, iOS Open-in-ADE From a per-runtime doc audit (each finding adversarially verified): - cursor: "edit" permission no longer maps to `--mode ask` (Cursor's read-only mode) — falls through to the edit-capable default agent mode. - claude: discovery + cross-cwd CLI fork now honor CLAUDE_CONFIG_DIR consistently. - codex: imported file-change events map add/delete/modify from the tagged-enum shape instead of always "modify"; thread/resume params reconciled with the current app-server schema. - cursor: de-slug handles non-/Users roots and hyphenated paths (fewer dropped sessions); SDK-origin agent-<uuid> transcripts no longer offered as resumable (they'd start a fresh empty session). - droid: fork-capability probe no longer reports a spurious false on first list. - iOS: decode importedSessionRef; already-imported rows show a single "Open in ADE" that opens the existing session (chat/CLI) instead of re-importing a duplicate; primary styling for chat-less providers; removed a duplicate header. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds docs/features/terminals-and-sessions/external-session-import.md — intended use case, architecture (discovery/capabilities/CLI+chat import/runtime routing/ mobile/authorization), per-provider gotchas from the runtime audit, the transplant static-import bundling bug class, and the lane-vs-main single-sync-host testing constraint. Updates the terminals-and-sessions, chat, and sync-and-multi-device READMEs to cross-link it. Ignores the iOS .derivedData cache. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
arul28
force-pushed
the
ade/start-skill-know-goes-all-fa102695
branch
from
0a52936 to
54ae5f7
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54ae5f71b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| putImportedRef(refs, `${metadata.importedFrom.provider}:${metadata.importedFrom.targetId}`, ref); | ||
| } | ||
| } | ||
| for (const pointer of sessionService.listClaudeSessionPointers?.({ limit: 5000 }) ?? []) { |
There was a problem hiding this comment.
When a Claude chat import forks/transplants or any Codex chat import succeeds, the only durable link back to the source session is the chat session's importedFrom metadata, but this lookup only considers terminal resumeMetadata plus Claude pointers. Those chat imports will be listed later as alreadyImported: false, so the import browser can offer duplicate imports instead of the intended “Open in ADE” path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Already addressed on this commit — the service now also sources chat-imported sessions via chatImportedRefsProvider (externalSessionsService.ts ~:250-257), wired at both main.ts:3294 and bootstrap.ts:1429, mapping provider:importedFrom.sessionId (the original external id) → { kind: "chat", sessionId }. So chat imports report alreadyImported + the Open-in-ADE ref. The terminal-scan block you flagged is one of three sources feeding the same refs map.
There was a problem hiding this comment.
Already fixed on this branch via chatImportedRefsProvider (externalSessionsService.ts ~:250, wired at main.ts + bootstrap.ts). This terminal-scan block is one of three sources feeding the same refs map; chat imports are detected. Dismissing as a re-post.
There was a problem hiding this comment.
Re-post of an already-fixed item — chat-imported sessions are detected via chatImportedRefsProvider (~:250, wired at main.ts + bootstrap.ts). Dismissing.
There was a problem hiding this comment.
Re-post — chat-imported sessions are detected via chatImportedRefsProvider. Dismissing.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/main/services/externalSessions/discoverCodex.ts (1)
60-81: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDiscovery can under-return valid sessions because the raw-file cap is applied before content validation.
collectRecentCodexSessionCandidatesstops scanning once it has gatheredlimitraw files (Line 76), butdiscoverCodexSessionsthen drops any of those candidates that fail parsing (wrongtype, missing id, duplicate id — Lines 123-125). Because the directory walk already stopped atlimitraw files, dropped candidates are not replaced by older-but-valid sessions that were never scanned, so the caller can receive fewer than the requestedlimitsessions even when more valid ones exist on disk.♻️ Possible fix: over-collect a buffer before validating/truncating
- for (const candidate of collectRecentCodexSessionCandidates(sessionsDir, limit)) { + // Over-collect to absorb entries that fail validation below, so the final + // truncation still yields up to `limit` valid records where possible. + for (const candidate of collectRecentCodexSessionCandidates(sessionsDir, limit * 3)) {Also applies to: 89-148
🤖 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 `@apps/desktop/src/main/services/externalSessions/discoverCodex.ts` around lines 60 - 81, collectRecentCodexSessionCandidates is stopping the directory walk too early by using the raw-file count as the cap, which causes discoverCodexSessions to lose valid sessions after parsing/filtering in ExternalSessionFileCandidate handling. Update the scan logic to over-collect beyond the requested limit, then let discoverCodexSessions apply its validation/deduplication and finally truncate the valid results to the caller’s limit. Keep the fix localized around collectRecentCodexSessionCandidates and the downstream filtering in discoverCodexSessions so older valid session files can replace dropped raw candidates.
🤖 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.
Nitpick comments:
In `@apps/desktop/src/main/services/externalSessions/discoverCodex.ts`:
- Around line 60-81: collectRecentCodexSessionCandidates is stopping the
directory walk too early by using the raw-file count as the cap, which causes
discoverCodexSessions to lose valid sessions after parsing/filtering in
ExternalSessionFileCandidate handling. Update the scan logic to over-collect
beyond the requested limit, then let discoverCodexSessions apply its
validation/deduplication and finally truncate the valid results to the caller’s
limit. Keep the fix localized around collectRecentCodexSessionCandidates and the
downstream filtering in discoverCodexSessions so older valid session files can
replace dropped raw candidates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0921d0b3-41a3-4f05-bea3-fc44ba6c0fae
⛔ Files ignored due to path filters (4)
docs/features/chat/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/external-session-import.mdis excluded by!docs/**
📒 Files selected for processing (38)
.gitignoreapps/ade-cli/src/adeRpcServer.test.tsapps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/services/sync/syncService.test.tsapps/desktop/src/main/main.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/externalChatHistoryImport.test.tsapps/desktop/src/main/services/chat/externalChatHistoryImport.tsapps/desktop/src/main/services/externalSessions/claudeSessionTransplant.tsapps/desktop/src/main/services/externalSessions/discoverClaude.tsapps/desktop/src/main/services/externalSessions/discoverCodex.tsapps/desktop/src/main/services/externalSessions/discoverCursor.tsapps/desktop/src/main/services/externalSessions/discoverDroid.tsapps/desktop/src/main/services/externalSessions/discoverOpenCode.tsapps/desktop/src/main/services/externalSessions/discoverProviders.test.tsapps/desktop/src/main/services/externalSessions/discoveryUtils.test.tsapps/desktop/src/main/services/externalSessions/discoveryUtils.tsapps/desktop/src/main/services/externalSessions/externalSessionsService.test.tsapps/desktop/src/main/services/externalSessions/externalSessionsService.tsapps/desktop/src/main/utils/terminalSessionSignals.test.tsapps/desktop/src/main/utils/terminalSessionSignals.tsapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/lanes/LaneDialogShell.tsxapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/terminals/WorkStartSurface.tsxapps/desktop/src/renderer/components/terminals/WorkViewArea.tsxapps/desktop/src/renderer/components/terminals/cliLaunch.test.tsapps/desktop/src/renderer/components/terminals/importSessions/ImportSessionBrowser.tsxapps/desktop/src/renderer/components/terminals/importSessions/affordances.test.tsapps/desktop/src/renderer/components/terminals/importSessions/affordances.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.tsapps/desktop/src/shared/cliLaunch.tsapps/desktop/src/shared/types/externalSessions.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Views/Work/WorkImportSessionScreen.swiftapps/ios/ADE/Views/Work/WorkLanePickerDropdown.swiftapps/ios/ADE/Views/Work/WorkNewChatScreen.swift
💤 Files with no reviewable changes (1)
- apps/desktop/src/shared/cliLaunch.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- apps/desktop/src/renderer/components/terminals/TerminalsPage.tsx
- apps/desktop/src/main/services/externalSessions/discoverClaude.ts
- apps/desktop/src/renderer/components/terminals/WorkStartSurface.tsx
- apps/desktop/src/main/main.ts
- apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx
- apps/desktop/src/main/services/chat/externalChatHistoryImport.test.ts
- apps/desktop/src/shared/types/externalSessions.ts
- apps/desktop/src/renderer/components/terminals/useWorkSessions.ts
- apps/desktop/src/renderer/components/chat/AgentChatPane.tsx
- apps/ios/ADE/Models/RemoteModels.swift
- apps/desktop/src/main/services/externalSessions/discoverCursor.ts
- apps/desktop/src/main/services/externalSessions/claudeSessionTransplant.ts
- apps/desktop/src/renderer/components/terminals/importSessions/affordances.ts
- apps/desktop/src/main/services/externalSessions/discoverDroid.ts
- apps/desktop/src/main/services/chat/externalChatHistoryImport.ts
- apps/ade-cli/src/adeRpcServer.ts
- apps/ade-cli/src/adeRpcServer.test.ts
…-ADE
alreadyImported / importedSessionRef previously only scanned terminal
resumeMetadata + Claude pointers, so a session imported as an ADE chat (its
provenance is on the chat session's importedFrom, not a terminal row) came back
alreadyImported:false and the browser offered a duplicate chat import. The
externalSessions service now also sources chat imports via
agentChatService.listSessions importedFrom (provider + original externalSessionId
→ { kind: "chat", sessionId }), wired at both the desktop and ade-cli daemon
construction sites; chat refs outrank CLI refs.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8497b76201
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| switch provider.trimmingCharacters(in: .whitespacesAndNewlines).lowercased() { | ||
| case "claude": return "claude" | ||
| case "codex": return "codex" | ||
| case "cursor": return "cursor" |
There was a problem hiding this comment.
When importing a Cursor external session on iOS, this optimistic TerminalSessionSummary is used immediately by onCliImported before the host refresh arrives. Returning cursor here makes isWorkChatToolType classify it as a chat session (that predicate treats cursor as chat, while new CLI launches and the desktop import mapping use cursor-cli), so Cursor CLI imports/open-existing can render the chat destination instead of TerminalSessionScreen until the refreshed row replaces it. Use cursor-cli for this provider.
Useful? React with 👍 / 👎.
The optimistic TerminalSessionSummary for an imported Cursor session set toolType "cursor", which isWorkChatToolType classifies as a chat session — so a Cursor CLI import / Open-in-ADE rendered the chat destination instead of the terminal screen until the host refresh replaced the row. Use "cursor-cli", matching new CLI launches and the desktop import mapping. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0c824be7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const discovered = sortDiscoveryRecords(settled.flat(), limit * providers.length); | ||
| const scoped = rawArgs.scope === "all" | ||
| ? discovered | ||
| : discovered.filter((session) => isInProjectScope(session.cwd, scopeRoots)); |
There was a problem hiding this comment.
When scope is project, this limits the merged provider results before filtering to scopeRoots. The provider discoverers in this change already cap their scans using discoveryArgs.limit (default 50), so a user with more than limit recent Claude/Codex/etc. sessions from other folders can get an empty/partial project list even though older sessions for the current ADE project exist. Apply the project/cwd filter before the cap (or over-fetch per provider) so the requested limit is filled from in-scope sessions.
Useful? React with 👍 / 👎.
…ork-import cleanup - externalSessions list(): for project scope, over-fetch per provider then filter to scopeRoots and slice to the requested limit, so a user with many recent sessions in other folders still sees their in-project sessions (previously the recency cap could exclude all in-project rows before the scope filter ran). - Codex chat fork-import cleanup: if archiving the forked thread through the original (possibly dead) app-server runtime fails, retry thread/archive once via a fresh runtime; if both fail, log provider + forkedThreadId + sourceId at warn so a stranded fork can be reconciled instead of leaking silently. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c5eced37b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const result = spawnSync(executable, ["session", "list", "--format", "json", "--max-count", String(limit)], { | ||
| cwd: path.resolve(cwd), | ||
| encoding: "utf8", | ||
| timeout: 4000, |
There was a problem hiding this comment.
When the import browser scans providers, this discovery path runs behind externalSessionsService.list from the Electron main process, but spawnSync blocks that process until opencode session list returns or the 4s timeout expires. On machines where OpenCode startup or its session store is slow, opening or refreshing the import browser can freeze ADE IPC/windows for the full timeout; this should use an async child-process call with a timeout instead.
Useful? React with 👍 / 👎.
| const discoveryLimit = projectScoped | ||
| ? Math.max(limit, PROJECT_SCOPE_DISCOVERY_LIMIT) | ||
| : limit; |
There was a problem hiding this comment.
For project-scoped lists, providers are capped to at most 200 records before the later project filter runs. Since providers such as Claude/Codex return the newest sessions globally, a user with more than 200 newer sessions in other repos will never see older but valid sessions from the current project, so “This project only” can incorrectly show no or partial results; keep scanning/paging until enough in-project rows survive, or filter by project before applying this cap.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6cd7df0 — project-scoped discovery is now scope-aware: Claude/Cursor/Droid path-filter their project dirs before the content cap, and Codex reads session_meta cwd scanning up to a 2000-file ceiling (logs if truncated). The old 200 global cap no longer gates project scope.
- discoverOpenCode: replace spawnSync with an async child-process call so a slow `opencode session list` never blocks the Electron main process (same timeout, binary resolution, and skip-when-unavailable contract). - Project-scoped discovery is now genuinely scope-aware instead of relying on a global recency cap: Claude/Cursor/Droid path-filter their project dirs before the per-provider content-read cap (cwd is encoded in the storage path); Codex reads each rollout's session_meta first line for cwd, scanning up to a 2000-file ceiling and logging external_sessions.codex_project_scope_scan_truncated if the ceiling is hit; OpenCode is already cwd-scoped. A user with many newer sessions in other repos now still sees older in-project sessions under "This project only". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cd7df0a96
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const findExternalSummary = async (provider: ExternalSessionProvider, sessionId: string): Promise<ExternalSessionSummary | null> => { | ||
| const sessions = await list({ providers: [provider], scope: "all", limit: MAX_FIND_LIMIT }); | ||
| return sessions.find((session) => session.id === sessionId) ?? null; |
There was a problem hiding this comment.
When a project-scoped list contains a session that is not in the most recent 500 sessions globally (the listing path explicitly handles this by prefiltering project sessions), importing that visible row calls this all-scope lookup and returns null. For providers that need the recovered cwd (Claude/Cursor/Droid/OpenCode resumes/forks, and all non-CTO lane-scoped imports through enforceLaneScopeCwd), the import then fails with missing/not-permitted cwd even though the session was just shown as importable. Look up the selected session with the lane/project scope, or carry the listed cwd through the import request.
Useful? React with 👍 / 👎.
| } catch (retryError) { | ||
| logger.warn("agent_chat.external_import_codex_fork_cleanup_leaked", { | ||
| provider: "codex", | ||
| forkedThreadId: forkedProviderThreadId, | ||
| sourceId: externalThreadId, | ||
| initialError: staleRuntimeError instanceof Error ? staleRuntimeError.message : staleRuntimeError == null ? null : String(staleRuntimeError), | ||
| error: retryError instanceof Error ? retryError.message : String(retryError), | ||
| }); |
There was a problem hiding this comment.
When thread/fork succeeds but both archive attempts fail, this catch only logs the leaked fork and the outer catch still deletes the ADE chat session. The forked Codex thread remains provider-side with no tracked ADE session left to retry or clean it up later.
Artifacts
Repro: focused Vitest test file that exercises Codex fork cleanup leak
- Contains supporting evidence from the run (text/typescript; charset=utf-8).
Repro: verbose Vitest output showing failed archive attempts, leak warning, and deleted ADE session
- Keeps the command output available without making the summary code-heavy.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/services/chat/agentChatService.ts
Line: 20875-20882
Comment:
**Fork Cleanup Drops Tracking**
When `thread/fork` succeeds but both archive attempts fail, this catch only logs the leaked fork and the outer catch still deletes the ADE chat session. The forked Codex thread remains provider-side with no tracked ADE session left to retry or clean it up later.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
Import external CLI sessions into ADE
Resume CLI sessions started outside ADE — in a plain terminal — from inside ADE (desktop,
ade codeTUI, and iOS). Import as a tracked ADE CLI PTY, or (Claude/Codex) as a full ADE chat with imported history. Fork a session into a different lane where the provider allows it.What's here
apps/desktop/src/main/services/externalSessions/): per-provider discovery — Claude (~/.claude/projects), Codex (~/.codex/sessions), Cursor (~/.cursor/projects), Droid (~/.factory/sessions), OpenCode (opencode session list) — a capability matrix per provider, and import that spawns a tracked resume/fork PTY viabuildTrackedCliResumeCommand.agentChatService.importExternalChatSession+externalChatHistoryImport.ts): seeds the ADE transcript from external JSONL/thread history (32 MB tail + 2000-event caps). Claude cross-cwd uses a non-destructive copy+rekey transplant; Codex usesthread/read+thread/fork.ade.externalSessions.list/importIPC,external-sessionsaction domain, daemon construction so remote-bound desktop and mobile route through the runtime.work.listExternalSessions/work.importExternalSession(viewer-allowed — the paired phone is a trusted controller, same trust level aswork.startCliSession).WorkImportSessionScreenmirroring the same rules branch-for-branch.Capability matrix
* Droid fork gated on the installed CLI actually supporting
--fork.Concurrency model
Resume = "take the baton" (tracked continuation of the same provider session; UI warns when the source may still be open elsewhere). Fork = "make a branch" (new continuation target, original untouched).
Testing
Typechecks + iOS xcodebuild green; feature suites pass (externalSessions service, chat history import, affordance mapping, sync commands, TUI browser). Quality dual-review + two re-review passes applied; notably reverted a client-spoofable sync role field that would have widened paired-device command access.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
external-sessionsCLI text formatter with automatic detection, and updated generated CLI help (includingade search --actions).Bug Fixes
Greptile Summary
This PR adds external CLI session import across ADE clients. The main changes are:
Confidence Score: 4/5
The Codex fork-import cleanup path can still leave provider threads behind after a failed import.
The lane-scoping updates now add project filtering and service-side cwd enforcement. The Codex cleanup retry logs total archive failure, then removes the ADE session that could carry retry state.
apps/desktop/src/main/services/chat/agentChatService.ts
What T-Rex did
Important Files Changed
Prompt To Fix All With AI
Reviews (10): Last reviewed commit: "Scope-aware project discovery + async Op..." | Re-trigger Greptile