Multi-CLI agent support, codex fast mode, browser UA spoof, PTY ADE context#253
Conversation
- Add cursor-agent, droid, and opencode CLI agent support - Codex Fast Mode service-tier JSON-RPC forwarding - Built-in browser: UA spoofing, openPanel surface - PTY: inject ADE_CHAT/MISSION/LANE context env on launch - Chat: localhost-link shortcuts in work-log, transcript row helpers - Unified permissionOptions helper across renderer - Recent-project upsert ordering preservation Tests: 4,886 passing across 8 desktop shards + ade-cli. Typecheck/lint/build/docs all green.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughLarge multifaceted change introducing browser panel UI, Codex fast-mode support across chat sessions, new CLI providers (Cursor, Droid, OpenCode), expanded terminal environment context, project icon accent colors, refined ADE project scaffolding, and extensive chat/terminal integration features. ChangesIntegrated Platform Expansion: Browser, Chat, CLI, and Project Features
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
Here's a review of the PR changes across the five main feature areas. Multi-CLI agent support (cursor-agent, droid, opencode)Duplicate permission-mode helpers (
ADE_CURSOR_CHAT_ID="$(cursor-agent create-chat)"
...
cursor-agent --resume "$ADE_CURSOR_CHAT_ID" ...If
Codex Fast Mode
- createdManaged.session.interactionMode = managed.session.interactionMode ?? sourceSession.interactionMode ?? null;This line was removed as part of adding
Built-in browser UA spoofingGoogle sign-in redirect removed: The previous
PTY ADE context injection
Chat improvements (localhost shortcuts, terminal reveal)
Terminal reveal
Project scaffold mode
Global state
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| maxStartDeltaMs?: number; | ||
| }): string | null => { | ||
| try { | ||
| const escapedCwd = args.cwd.replace(/\//g, "-"); |
There was a problem hiding this comment.
args.cwd.replace(/\//g, "-") produces identical escaped strings for /home/user/my-project and /home/user/my/project. When the colliding directory already exists in ~/.factory/sessions/, the code enters the ? [expectedProjectDir] branch and only scans that single directory — meaning a session stored under a different directory will never be found and resolveDroidSessionIdFromStorage returns null even though a valid session exists. Use encodeURIComponent or a proper hash/encode scheme to avoid the collision.
| const escapedCwd = args.cwd.replace(/\//g, "-"); | |
| const escapedCwd = encodeURIComponent(args.cwd).replace(/%/g, "_"); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/services/pty/ptyService.ts
Line: 966
Comment:
**Droid path-collision in escaped CWD lookup**
`args.cwd.replace(/\//g, "-")` produces identical escaped strings for `/home/user/my-project` and `/home/user/my/project`. When the colliding directory already exists in `~/.factory/sessions/`, the code enters the `? [expectedProjectDir]` branch and only scans that single directory — meaning a session stored under a different directory will never be found and `resolveDroidSessionIdFromStorage` returns `null` even though a valid session exists. Use `encodeURIComponent` or a proper hash/encode scheme to avoid the collision.
```suggestion
const escapedCwd = encodeURIComponent(args.cwd).replace(/%/g, "_");
```
How can I resolve this? If you propose a fix, please make it concise.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/desktop/src/renderer/browserMock.ts (1)
762-775:⚠️ Potential issue | 🟠 Major | 💤 Low valueThe implicit removal of bare
"droid"matching breaks mock chat handling for non-suffixed droid sessionsThe change removes the bare
"droid"check fromisMockChatToolType, buttoolType: "droid"(without suffix) is still actively used throughout the codebase:
apps/desktop/src/renderer/lib/sessions.ts(lines 64, 148) and tests still handle bare"droid"apps/desktop/src/main/utils/terminalSessionSignals.ts(lines 56, 397) produces and checks bare"droid"apps/desktop/src/main/services/pty/ptyService.ts(lines 50, 316, 358, 1125) still handles bare"droid"Sessions with
toolType: "droid"will now fail theisMockChatToolTypecheck and not enter the mock agent chat path. Either add back the bare"droid"check or confirm all droid sessions are guaranteed to use"droid-chat"instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/browserMock.ts` around lines 762 - 775, isMockChatToolType currently rejects the bare "droid" toolType causing existing code paths (sessions.ts, terminalSessionSignals.ts, ptyService.ts) that still use "droid" to skip mock chat handling; restore support by updating the isMockChatToolType function to also accept normalized === "droid" (i.e., add a check for the exact "droid" string alongside the other matches in isMockChatToolType) so both "droid" and suffixed forms like "droid-chat" are treated as mock chat tools.apps/desktop/src/main/main.ts (1)
3526-3533:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid reshuffling recents from background context initialization.
This write path now relies on callers to opt into
preserveRecentOrder, butensureProjectContextForMobileSync()still initializes contexts withrecordRecent: trueand the defaultpreserveRecentOrder: false. A phone-sync warm-up or failed handoff can therefore still move an existing project to the front, which undercuts the new ordering behavior.One straightforward fix
const ctx = await initContextForProjectRoot({ projectRoot: normalizedRoot, baseRef, ensureExclude: true, recordLastProject: false, recordRecent: true, + preserveRecentOrder: true, userSelectedProject: false, });🤖 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/main.ts` around lines 3526 - 3533, The background initialization path (ensureProjectContextForMobileSync) is still calling the recent-write flow with recordRecent: true while leaving preserveRecentOrder at its default false, which reshuffles recents; update ensureProjectContextForMobileSync to either set preserveRecentOrder: true when calling upsertRecentProject (or the caller that constructs the options) or to avoid performing the recent write at all by setting recordRecent: false so background warm-ups don’t change ordering; locate calls in ensureProjectContextForMobileSync and modify the options object passed to upsertRecentProject (or the helper that forwards options) to include preserveRecentOrder: true (or set recordRecent: false) accordingly.apps/desktop/src/renderer/index.css (1)
445-450:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winActive-state
box-shadowoverrides:focus-visibleshadow.
.ade-shell-project-tab[data-state="active"](Lines 445–450) setsbox-shadow: inset 0 -1px 0 …..ade-shell-project-tab:focus-visible(Lines 464–469) sets a differentbox-shadow: inset 0 0 0 1px var(--shell-project-tab-focus-ring). When an active tab is focused via keyboard, only one shadow wins (last-declared rule, i.e., focus-visible) and the active accent underline disappears while focused.If you want both indications visible simultaneously, combine them on the active+focus state:
♻️ Suggested change
.ade-shell-project-tab:focus-visible { color: var(--shell-project-tab-focus-fg); background: var(--shell-project-tab-focus-bg); border-color: var(--shell-project-tab-focus-border); box-shadow: inset 0 0 0 1px var(--shell-project-tab-focus-ring); } + +.ade-shell-project-tab[data-state="active"]:focus-visible { + box-shadow: + inset 0 0 0 1px var(--shell-project-tab-focus-ring), + inset 0 -1px 0 color-mix(in srgb, var(--project-tab-accent, var(--color-accent)) 55%, transparent); +}🤖 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/renderer/index.css` around lines 445 - 450, The active tab rule (.ade-shell-project-tab[data-state="active"]) sets an inset box-shadow that gets overridden by the focus ring (.ade-shell-project-tab:focus-visible), causing the active accent underline to disappear when focused; update the stylesheet to add a combined selector for the active+focus state (e.g., .ade-shell-project-tab[data-state="active"]:focus-visible) and set a box-shadow that composes both the active inset underline and the focus-visible ring (merge the inset 0 -1px 0 color-mix(...) and the inset 0 0 0 1px var(--shell-project-tab-focus-ring) into a single box-shadow value) so both visual indications are visible simultaneously.apps/desktop/src/renderer/components/app/TopBar.tsx (1)
642-670:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
confirmProjectTabRemovalruns beforecheckForActiveWorkloads, producing two sequential prompts.On a project tab with active workloads, the user now sees:
- The new "Close "X" and remove it from project tabs?"
window.confirm, then- The existing "You are about to close this project. The following active work items will be terminated…"
window.confirm.That's two modal prompts to remove one tab. Consider combining them into a single confirm (e.g., when active workloads exist, fold the warnings into the removal prompt) or skipping the new confirm when
checkForActiveWorkloadswill already prompt. As written, it's a UX regression for active projects.🤖 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/renderer/components/app/TopBar.tsx` around lines 642 - 670, The current flow calls confirmProjectTabRemoval before checkForActiveWorkloads, causing two sequential confirms; update handleRemoveTab so you call checkForActiveWorkloads(rootPath) first and if it returns false abort, and only call confirmProjectTabRemoval afterward when there are no active workloads (or call it with a flag to include workload warnings instead of triggering the separate workload prompt). Modify handleRemoveTab (referenced here by name) to reorder the checks between checkForActiveWorkloads and confirmProjectTabRemoval (or pass an indicator into confirmProjectTabRemoval) so the user sees a single consolidated confirmation.
🧹 Nitpick comments (13)
apps/desktop/src/renderer/lib/sessions.ts (1)
206-236: ⚡ Quick win
isGenericSessionTitleis missing the new default labels, causing inconsistent fallback behaviour.
"claude session"and"codex session"are listed as generic (Lines 217-220) soprimarySessionLabelskips them and falls through to the goal/summary. The new defaults —"Cursor CLI session","Droid CLI session","OpenCode CLI session"— are absent from this list, so sessions whose title was auto-set to one of these will surface the raw default label instead of a more meaningful goal or summary.♻️ Suggested addition
normalized === "codex worker" ) { return true; } + if ( + normalized === "cursor cli session" || + normalized === "droid cli session" || + normalized === "opencode cli session" + ) { + return true; + } if (🤖 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/renderer/lib/sessions.ts` around lines 206 - 236, Update isGenericSessionTitle to treat the new default labels as generic: add normalized comparisons for "cursor cli session", "droid cli session", and "opencode cli session" alongside the existing checks in the big string-equality block inside isGenericSessionTitle (the function that accepts TerminalSessionSummary session and value). Ensure you use the same normalization (value.trim().toLowerCase()) and include these exact normalized strings so primarySessionLabel will skip them the same way it does "claude session" and "codex session".apps/desktop/src/renderer/components/shared/permissionOptions.test.ts (1)
123-137: ⚡ Quick winConsider extending the required-fields completeness check to cover the new families.
The
every option has required fields populatedtest only samplesanthropic,openai, andlocal. The newcursorandopencodefamilies aren't included, so a missingshortDesc,detail, orallowson one of their options would go undetected here.♻️ Suggested addition
const allOptions = [ ...getPermissionOptions({ family: "anthropic", isCliWrapped: true }), ...getPermissionOptions({ family: "openai", isCliWrapped: true }), ...getPermissionOptions({ family: "local", isCliWrapped: false }), + ...getPermissionOptions({ family: "cursor", isCliWrapped: true }), + ...getPermissionOptions({ family: "opencode", isCliWrapped: true }), ];🤖 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/renderer/components/shared/permissionOptions.test.ts` around lines 123 - 137, The test "every option has required fields populated" only aggregates getPermissionOptions for families "anthropic", "openai", and "local"; add the missing families "cursor" and "opencode" (or better, iterate over a canonical list of families used by getPermissionOptions) so the test also validates options for getPermissionOptions({ family: "cursor", ... }) and getPermissionOptions({ family: "opencode", ... }); update the allOptions construction in the test to include those families (or replace the hardcoded array with a loop over the master family list) so shortDesc, detail, allows, safety, label, and value are asserted for every family.apps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsx (2)
491-492: 💤 Low valueWhy is
Date.nowspied here?const dateNowSpy = vi.spyOn(Date, "now").mockReturnValue(123);Nothing in the surrounding assertions appears to depend on a fixed
Date.now()value, and the cleanup infinallyis the only other reference. If this is required for stable internal keying/sequencing insideAgentChatPane's terminal handling, a brief comment explaining that would help future readers; otherwise it can likely be removed.🤖 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/renderer/components/chat/AgentChatPane.submit.test.tsx` around lines 491 - 492, The test currently spies on Date.now via dateNowSpy = vi.spyOn(Date, "now").mockReturnValue(123) even though no assertions rely on a fixed timestamp; either remove that spy and its cleanup in the finally block if it's not needed, or if AgentChatPane/terminal internals require stable timestamps for deterministic keys/sequencing, add an inline comment near the spy explaining that dependency (reference Date.now, dateNowSpy and AgentChatPane) and keep the mock; update the test to only mock Date.now when its value affects the component behavior.
415-456: 💤 Low valueGlobal
sessions.getoverride may collide with chat-session lookups.vi.mocked(window.ade.sessions.get).mockResolvedValue(terminalSession);This replaces the default
{ toolType: "codex-chat" }resolution for everysessions.getcall in the test, including any internal lookupAgentChatPanemay perform for the locked chat session itself. If a future change introduces such a lookup, this test could start returning aTerminalSessionDetailshaped payload for a chat session id, making the failure mode subtle. Consider scoping the override viamockImplementationkeyed on the terminal id (as in the rapid-CLI test below), e.g.:♻️ Proposed fix to scope the override
- vi.mocked(window.ade.sessions.get).mockResolvedValue(terminalSession); + vi.mocked(window.ade.sessions.get).mockImplementation(async (sessionId: string) => + sessionId === terminalSession.id ? terminalSession : { toolType: "codex-chat" } as any, + );🤖 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/renderer/components/chat/AgentChatPane.submit.test.tsx` around lines 415 - 456, The test currently globally stubs window.ade.sessions.get to always return terminalSession which can incorrectly satisfy other session lookups; change the stub to a scoped mockImplementation that returns terminalSession only when the requested id equals terminalSession.id and otherwise returns the normal/chat-shaped session (e.g., an object with toolType: "codex-chat" or the previous default), by updating the vi.mocked(window.ade.sessions.get) call in this test; keep the rest of the test (renderPane, emitSessionChanged, assertions) unchanged.apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx (1)
2768-2774: 💤 Low valueParallel toggle silently no-ops when slot handler is missing.
CodexFastModeToggledisables itself whenonToggleis falsy, but hereonToggleis a wrapper arrow function that's always truthy, so the button stays enabled even ifonParallelSlotCodexFastModeChangeisundefined(clicks are swallowed by?.). Either pass the handler directly with the index baked in (or through a smalluseCallback), or guard the wrapper:♻️ Proposed fix
- {parallelChatMode && parallelConfiguringIndex != null && fastModeSupported ? ( + {parallelChatMode && parallelConfiguringIndex != null && fastModeSupported ? ( <CodexFastModeToggle active={fastModeActive} disabled={fastModeToggleDisabled} - onToggle={(next) => onParallelSlotCodexFastModeChange?.(parallelConfiguringIndex, next)} + onToggle={ + onParallelSlotCodexFastModeChange + ? (next) => onParallelSlotCodexFastModeChange(parallelConfiguringIndex, next) + : undefined + } /> ) : null}🤖 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/renderer/components/chat/AgentChatComposer.tsx` around lines 2768 - 2774, The CodexFastModeToggle is currently given an always-truthy arrow function for onToggle, so clicks are enabled but swallowed when onParallelSlotCodexFastModeChange is undefined; fix by only supplying a real handler when one exists—either bind the handler ahead of time (e.g., create a memoized callback using parallelConfiguringIndex and onParallelSlotCodexFastModeChange) and pass that directly to CodexFastModeToggle, or guard the prop by using a conditional: pass onToggle={onParallelSlotCodexFastModeChange ? (next) => onParallelSlotCodexFastModeChange(parallelConfiguringIndex!, next) : undefined} so the toggle correctly disables when the slot handler is missing.apps/desktop/src/renderer/components/app/TopBar.tsx (2)
460-460: 💤 Low value
projectAccentColorsis never pruned when projects are removed/reordered.Entries keyed by
rootPathaccumulate indefinitely as users open/close projects within a session. While each value is small, the map grows without bound across long-lived sessions. Consider dropping entries for paths no longer inrecentProjects(e.g., inhandleRemoveTaband afterforgetRecent), or filtering on read.♻️ Suggested cleanup
const handleRemoveTab = useCallback((rootPath: string) => { void (async () => { ... setRecentProjects(rows); + setProjectAccentColors((prev) => { + if (!(rootPath in prev)) return prev; + const { [rootPath]: _removed, ...rest } = prev; + return rest; + }); // If we just removed the active project, switch to the next available or show welcome.Also applies to: 719-724
🤖 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/renderer/components/app/TopBar.tsx` at line 460, projectAccentColors (managed by useState via setProjectAccentColors) accumulates entries keyed by rootPath when projects are removed/reordered; update the state to prune keys that are no longer present in recentProjects whenever projects change — for example, remove the rootPath entry inside handleRemoveTab and after forgetRecent returns (or perform a cleanup pass whenever recentProjects updates) so projectAccentColors only contains keys that exist in recentProjects.
82-83: ⚡ Quick winCache key uses full dataUrl; LRU recency not refreshed on hits.
projectIconAccentCacheis keyed by the entire icondataUrl(potentially many KB to a few MB) andderiveIconAccentColordoes not refresh LRU recency on cache hits, unlikegetProjectIconFromCache(Lines 33–40) which deletes-and-reinserts on read. Net result: hot keys can be evicted before cold ones, and the cache holds 48× full data-URL strings as keys.Consider keying by
rootPath(or a short hash ofdataUrl) and touching the entry on reads for symmetry with the icon cache.♻️ Suggested change
async function deriveIconAccentColor(dataUrl: string): Promise<string | null> { - if (projectIconAccentCache.has(dataUrl)) return projectIconAccentCache.get(dataUrl) ?? null; + if (projectIconAccentCache.has(dataUrl)) { + const cached = projectIconAccentCache.get(dataUrl) ?? null; + // Touch on read so frequently-used accents stay hot. + projectIconAccentCache.delete(dataUrl); + projectIconAccentCache.set(dataUrl, cached); + return cached; + } if (typeof document === "undefined" || typeof Image === "undefined") return null;🤖 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/renderer/components/app/TopBar.tsx` around lines 82 - 83, deriveIconAccentColor currently keys projectIconAccentCache with the full dataUrl and does not refresh LRU recency on hits; change the cache key to a short identifier (preferably rootPath when available, or a small hash of dataUrl) instead of the full data URL, and on cache hits update recency the same way getProjectIconFromCache does (delete the entry and re-set it) so hot keys aren’t evicted; update deriveIconAccentColor to accept/derive the short key, use that for lookups/sets on projectIconAccentCache, and ensure any callers provide rootPath or the hashed key.apps/desktop/src/renderer/components/app/TopBar.test.tsx (1)
225-247: 💤 Low valueConfirm-flow tests don't cover the active-workload double-prompt path.
The two new tests verify the confirmation prompt for the simple (no active workloads) case, but do not exercise the interaction with
checkForActiveWorkloads. Given that lanes/sessions/agentChat/missions all return empty arrays in the default mock, the second internalwindow.confirmis never invoked. If the confirm sequence is intentional (see comment on TopBar.tsx Line 642), consider adding a test where lanes/sessions return active items to verify both prompts fire andforgetRecentonly proceeds when both are confirmed.🤖 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/renderer/components/app/TopBar.test.tsx` around lines 225 - 247, Add tests that exercise the double-confirm path in TopBar by mocking checkForActiveWorkloads (or the underlying window.ade.project lanes/sessions/agentChat/missions mocks) to return non-empty arrays so the second confirmation is triggered, and spy on window.confirm to control both prompts; assert that when both confirms return true globalThis.window.ade.project.forgetRecent is called with the expected path, and add a complementary test where the first confirm returns true but the second returns false to assert forgetRecent is not called. Use the existing test structure (render(<TopBar />), fireEvent.click(screen.getByTitle("Remove project")), and waitFor/expect) and reference the checkForActiveWorkloads behavior and forgetRecent call to locate the code paths to exercise.apps/desktop/src/main/services/chat/agentChatService.test.ts (2)
5999-6003: ⚡ Quick winVerify the fast-mode toggle is persisted on update, not just kept in memory.
This currently only proves the next live turn uses the updated flag. If
updateSession({ codexFastMode: true })forgot to write disk state, resume-after-restart would regress while this test still passes.Suggested assertion
const updated = await service.updateSession({ sessionId: session.id, codexFastMode: true, }); expect(updated.codexFastMode).toBe(true); + expect(readPersistedChatState(session.id).codexFastMode).toBe(true);🤖 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.test.ts` around lines 5999 - 6003, The test currently only checks the in-memory returned object from service.updateSession; change it to verify the flag was persisted by reloading the session from the persistence layer and asserting the stored value. After calling service.updateSession({ sessionId: session.id, codexFastMode: true }), call the service method that reads the session from disk/persistence (e.g., service.getSession(session.id) or the test harness’s persistence loader) and expect the reloadedSession.codexFastMode toBe(true); if needed, simulate a restart of the service or re-instantiate the persistence-backed session store before reloading to ensure the value was written to disk rather than only kept in memory.
1569-1633: ⚡ Quick winAssert the permission switch happens before the injected handoff prompt.
Right now this proves the target session eventually lands in
plan, but it would still pass if the auto-sent handoff prompt went out beforesetPermissionMode("plan"). That ordering is the part that protects the first handoff turn from running under the wrong mode.Suggested assertion
expect(result.session.provider).toBe("claude"); expect(result.session.interactionMode).toBe("plan"); expect(result.session.permissionMode).toBe("plan"); expect(setPermissionMode).toHaveBeenCalledWith("plan"); await vi.waitFor(() => { expect(send).toHaveBeenCalledWith(expect.stringContaining("This message was injected automatically by ADE during a chat handoff.")); }); + expect(setPermissionMode.mock.invocationCallOrder.at(-1)).toBeLessThan( + send.mock.invocationCallOrder[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/chat/agentChatService.test.ts` around lines 1569 - 1633, Add an assertion to ensure setPermissionMode runs before the automatically injected handoff prompt is sent: after calling service.handoffSession(...) and before/alongside the existing send expectation, read the mock invocation order (e.g., setPermissionMode.mock.invocationCallOrder[0] and send.mock.invocationCallOrder.find(...) or send.mock.invocationCallOrder[0]) and assert the setPermissionMode call index is smaller than the send call index for the injected handoff message (the string containing "This message was injected automatically by ADE during a chat handoff."); reference the setPermissionMode and send mocks and the service.handoffSession invocation to locate where to add this ordering check.apps/desktop/src/renderer/components/terminals/WorkStartSurface.tsx (2)
43-57: 💤 Low value
CLI_PROVIDER_LISTduplicatesCLI_PROVIDER_OPTIONS; one source of truth is enough.
CLI_PROVIDER_LISTre-states each entry by hand. If a newCliProvideris added later, TypeScript will still let this list go stale (it's typedCliProviderOption[], not a tuple of all keys). Deriving the list — or even just iteratingObject.values(CLI_PROVIDER_OPTIONS)in the render path — keeps it in sync, at the cost of giving up explicit display ordering. If display order matters, a smallCLI_PROVIDER_ORDER: readonly CliProvider[]tuple makes the dependency explicit and is exhaustively checkable.🤖 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/renderer/components/terminals/WorkStartSurface.tsx` around lines 43 - 57, Replace the duplicated CLI_PROVIDER_LIST with a single source of truth: either derive the list from CLI_PROVIDER_OPTIONS using Object.values(CLI_PROVIDER_OPTIONS) where needed (remove CLI_PROVIDER_LIST) or introduce an explicit CLI_PROVIDER_ORDER: readonly CliProvider[] tuple and build CLI_PROVIDER_LIST by mapping that tuple to CLI_PROVIDER_OPTIONS (e.g. CLI_PROVIDER_ORDER.map(k => CLI_PROVIDER_OPTIONS[k])) so additions are exhaustively checked and display order is preserved; update any uses of CLI_PROVIDER_LIST to the new derived value and keep the symbols CLI_PROVIDER_OPTIONS, CLI_PROVIDER_LIST (or new CLI_PROVIDER_ORDER) referenced accordingly.
22-32: ⚡ Quick winProfile type drifts from the parent prop.
This declares
profile: LaunchProfile, butWorkViewArea.tsx(line 455) declares the same callback asprofile: CliProvider | "shell". They only line up by coincidence right now; any expansion ofLaunchProfile(or rename) will leave the two callback shapes silently different and force a widening cast at the call site. Pick one canonical type — ideallyLaunchProfilefrom./cliLaunch— and use it in both files (and inuseWorkSessions.ts, which already usesLaunchProfile).As per coding guidelines: "Keep IPC contracts, preload types, shared types, and renderer usage in sync whenever an interface changes in TypeScript".
🤖 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/renderer/components/terminals/WorkStartSurface.tsx` around lines 22 - 32, The callback type for onLaunchPtySession is inconsistent: WorkStartSurface.tsx declares profile: LaunchProfile while WorkViewArea.tsx declares profile: CliProvider | "shell", causing a fragile mismatch; update the declaration in WorkViewArea.tsx (and any other callers such as useWorkSessions.ts) to use the canonical LaunchProfile type imported from ./cliLaunch so all onLaunchPtySession signatures match, remove the CliProvider | "shell" union, update imports to reference LaunchProfile, and fix any call sites/casts to conform to the single LaunchProfile definition.apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx (1)
453-462: ⚡ Quick winUse
LaunchProfiletype alias to align withWorkStartSurfaceanduseWorkSessions.
onLaunchPtySessioninWorkViewArea.tsxdeclaresprofile: CliProvider | "shell"whileWorkStartSurface.tsxanduseWorkSessions.tsboth useprofile: LaunchProfile. SinceLaunchProfile = CliProvider | "shell"(defined incliLaunch.ts), updating this component to use the shared alias keeps the type contract in one place and prevents silent drift ifLaunchProfileever evolves.♻️ Proposed alignment
-import { resolveTrackedCliResumeCommand, type CliProvider } from "./cliLaunch"; +import { resolveTrackedCliResumeCommand, type LaunchProfile } from "./cliLaunch"; @@ - onLaunchPtySession: (args: { - laneId: string; - profile: CliProvider | "shell"; + onLaunchPtySession: (args: { + laneId: string; + profile: LaunchProfile; title?: string; startupCommand?: string; command?: string; args?: string[]; env?: Record<string, string>; tracked?: boolean; }) => Promise<unknown>;🤖 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/renderer/components/terminals/WorkViewArea.tsx` around lines 453 - 462, Replace the inline union type for the onLaunchPtySession argument with the shared LaunchProfile alias so the component's API matches WorkStartSurface and useWorkSessions; locate the onLaunchPtySession declaration in WorkViewArea (the function/type that currently uses "profile: CliProvider | \"shell\"") and change its profile type to LaunchProfile (importing LaunchProfile from the module that defines it, e.g. cliLaunch.ts) so all three usages use the same LaunchProfile symbol.
🤖 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/cli.ts`:
- Around line 2290-2297: Detect and reject conflicting fast-mode flags before
setting codexFastMode: check both readFlag(args, ["--fast","--codex-fast"]) and
readFlag(args, ["--standard","--no-fast","--no-codex-fast"]) and if both are
true, surface a clear error (throw or return a user-facing error) instead of
letting one silently win; otherwise set codexFastMode as before and pass it into
collectGenericObjectArgs. Update the logic around the codexFastMode variable
(the block referencing readFlag, codexFastMode, and
collectGenericObjectArgs/actionStep) to perform this explicit conflict check and
error path.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 1272-1274: The current merge treats an empty array the same as no
override because it branches on truthy length; change the condition so an
explicit empty override is preserved by checking for presence
(overrides?.serviceTiers !== undefined) instead of length, e.g. use
(overrides?.serviceTiers !== undefined ? { serviceTiers: overrides.serviceTiers
} : (descriptor.serviceTiers ? { serviceTiers: descriptor.serviceTiers } : {}));
apply the same presence-vs-length fix to the later descriptor merges that handle
serviceTiers around the descriptor merge blocks (the same variables: overrides,
descriptor, serviceTiers) so an intentionally empty array from the app-server is
not overwritten by descriptor.serviceTiers.
In `@apps/desktop/src/main/services/config/projectConfigService.ts`:
- Around line 2077-2098: The hasSharedConfigContent function contains a dead
check for config.laneCleanup that never survives
coerceConfigFile/toCanonicalYaml, so remove the "|| config.laneCleanup" branch
from hasSharedConfigContent to avoid implying support; alternatively, if
laneCleanup should truly round-trip, add explicit preservation of laneCleanup in
coerceConfigFile and toCanonicalYaml so it appears in the saved/shared config
(update the save usage where coerceConfigFile(candidate.shared) is called and
ensure LaneBehaviorSection.tsx continues to read the field).
In `@apps/desktop/src/main/services/ipc/registerIpc.ts`:
- Around line 6562-6565: The new IPC handler for IPC.builtInBrowserShowPanel
sends URL-containing payloads; update the IPC trace/redaction logic to redact
URL fields for this channel (and other browser URL channels) before logging —
locate the IPC.builtInBrowserShowPanel handler (and related handlers using
ensureBuiltInBrowser() and parseBuiltInBrowserOpenPanelArgs()) and ensure the
trace middleware or redaction utility masks URL/query/token fields (e.g.,
replace full URLs with sanitized placeholders) when guardBuiltInBrowserIpc
triggers tracing so no raw URLs/queries are emitted.
In `@apps/desktop/src/main/services/state/globalState.ts`:
- Around line 65-70: The code slices prev to a 12-item array then assigns
updated[existingIndex] = nextEntry which can create sparse holes when
existingIndex >= 12; change the assignment to clamp the index to the retained
window (e.g. const idx = Math.min(existingIndex, updated.length - 1);
updated[idx] = nextEntry) so you never write outside the 12-item slice; update
the branch that sets next.recentProjects to use this clamped index (symbols:
options.preserveRecentOrder, prev, proj.rootPath, existingIndex, nextEntry,
next.recentProjects, updated).
In `@apps/desktop/src/main/utils/terminalSessionSignals.ts`:
- Around line 414-422: The extracted candidates array (built from
RESUME_BACKTICK_REGEX and RESUME_COMMAND_REGEX) can include config-only
fragments like "-c key=value" which are not resumable; before returning from the
loop that calls normalizeResumeCommand(candidate, preferredTool) filter out such
bogus candidates by rejecting any candidate that is only a config flag (e.g.
contains "-c" followed by a key=value and no resume indicator) — implement a
small validation check inside the for loop (or a helper) that skips candidates
matching a pattern like "-c ...=..." or containing only "-c" entries and that do
not include known resume tokens (e.g. "--resume", "resume", "-r", or whatever
resume marker your code uses) so normalizeResumeCommand is only called/returned
for true resume commands.
- Around line 299-305: The Droid parser in terminalSessionSignals.ts (provider
=== "droid" branch used by parseTrackedCliResumeCommand) fails to handle the
shell wrapper produced by buildDroidCommandLine and can capture a trailing
semicolon; update the provider === "droid" logic to (1) allow arbitrary
prefix/wrapper text before the inner "droid" invocation (i.e. don't require the
string to start with ^droid), (2) tighten the resume/session-id capture so it
excludes trailing shell punctuation (e.g. use [^\s;]+ or strip trailing ';' from
the extracted id), and (3) return that cleaned id (match[1] ?? match[2] ??
match[3] ?? null) so parseTrackedCliResumeCommand round-trips
buildDroidCommandLine output.
In `@apps/desktop/src/renderer/components/chat/ChatWorkLogBlock.tsx`:
- Around line 548-574: The async handler handleTerminalClick can reject (e.g.,
window.ade?.terminal?.activeForChat) and currently only uses finally to reset
busy, leaving rejections unhandled; wrap the await(s) in a try/catch (or add an
outer catch) so any thrown error is caught, log the error (e.g., console.error
or the app logger) and optionally show user feedback, then re-run setBusy(false)
in finally; alternatively ensure call sites invoke
handleTerminalClick().catch(...) to swallow/log errors — update references in
this function (window.ade?.terminal?.activeForChat, onRevealChatTerminal,
onInsertDraft, setBusy) accordingly so no unhandled promise rejections occur.
In `@apps/desktop/src/renderer/components/terminals/useWorkSessions.ts`:
- Around line 239-247: inferToolFromResumeCommand currently only removes a
single leading ENV=VALUE pair so commands that start with multiple env
assignments (e.g. "ADE_CHAT_ID=... ADE_LANE=... codex ...") still keep a
leftover prefix and fail the startsWith checks; update
inferToolFromResumeCommand to strip all leading env assignments before checking
the tool by either looping to repeatedly remove
/^[A-Za-z_][A-Za-z0-9_]*=(?:"[^"]*"|'[^']*'|[^\s]+)\s+/ or use a single regex
with a repeating group like
/^(?:[A-Za-z_][A-Za-z0-9_]*=(?:"[^"]*"|'[^']*'|[^\s]+)\s+)*/ to replace them
with "" so the subsequent startsWith checks (claude, codex, cursor-agent, droid,
opencode) see the actual command; adjust useWorkSessions.resumeSession callers
if they rely on the old behavior.
In `@apps/desktop/src/renderer/components/terminals/WorkStartSurface.tsx`:
- Around line 110-114: The effect currently resets an invalid cliPermissionMode
to the hardcoded string "default"; change it to pick the provider's seeded
default permission (i.e. the default present in cliPermissionOptions) instead.
In the useEffect that checks cliPermissionOptions and cliPermissionMode, replace
setCliPermissionMode("default") with logic that finds the option representing
the provider's default (e.g. const fallback = cliPermissionOptions.find(o =>
o.value === providerDefaultValue)?.value ?? cliPermissionOptions[0]?.value) and
call setCliPermissionMode(fallback); reference the existing symbols useEffect,
cliPermissionOptions, cliPermissionMode, and setCliPermissionMode and ensure
this matches how selectCliProvider seeds each provider's defaultPermission.
---
Outside diff comments:
In `@apps/desktop/src/main/main.ts`:
- Around line 3526-3533: The background initialization path
(ensureProjectContextForMobileSync) is still calling the recent-write flow with
recordRecent: true while leaving preserveRecentOrder at its default false, which
reshuffles recents; update ensureProjectContextForMobileSync to either set
preserveRecentOrder: true when calling upsertRecentProject (or the caller that
constructs the options) or to avoid performing the recent write at all by
setting recordRecent: false so background warm-ups don’t change ordering; locate
calls in ensureProjectContextForMobileSync and modify the options object passed
to upsertRecentProject (or the helper that forwards options) to include
preserveRecentOrder: true (or set recordRecent: false) accordingly.
In `@apps/desktop/src/renderer/browserMock.ts`:
- Around line 762-775: isMockChatToolType currently rejects the bare "droid"
toolType causing existing code paths (sessions.ts, terminalSessionSignals.ts,
ptyService.ts) that still use "droid" to skip mock chat handling; restore
support by updating the isMockChatToolType function to also accept normalized
=== "droid" (i.e., add a check for the exact "droid" string alongside the other
matches in isMockChatToolType) so both "droid" and suffixed forms like
"droid-chat" are treated as mock chat tools.
In `@apps/desktop/src/renderer/components/app/TopBar.tsx`:
- Around line 642-670: The current flow calls confirmProjectTabRemoval before
checkForActiveWorkloads, causing two sequential confirms; update handleRemoveTab
so you call checkForActiveWorkloads(rootPath) first and if it returns false
abort, and only call confirmProjectTabRemoval afterward when there are no active
workloads (or call it with a flag to include workload warnings instead of
triggering the separate workload prompt). Modify handleRemoveTab (referenced
here by name) to reorder the checks between checkForActiveWorkloads and
confirmProjectTabRemoval (or pass an indicator into confirmProjectTabRemoval) so
the user sees a single consolidated confirmation.
In `@apps/desktop/src/renderer/index.css`:
- Around line 445-450: The active tab rule
(.ade-shell-project-tab[data-state="active"]) sets an inset box-shadow that gets
overridden by the focus ring (.ade-shell-project-tab:focus-visible), causing the
active accent underline to disappear when focused; update the stylesheet to add
a combined selector for the active+focus state (e.g.,
.ade-shell-project-tab[data-state="active"]:focus-visible) and set a box-shadow
that composes both the active inset underline and the focus-visible ring (merge
the inset 0 -1px 0 color-mix(...) and the inset 0 0 0 1px
var(--shell-project-tab-focus-ring) into a single box-shadow value) so both
visual indications are visible simultaneously.
---
Nitpick comments:
In `@apps/desktop/src/main/services/chat/agentChatService.test.ts`:
- Around line 5999-6003: The test currently only checks the in-memory returned
object from service.updateSession; change it to verify the flag was persisted by
reloading the session from the persistence layer and asserting the stored value.
After calling service.updateSession({ sessionId: session.id, codexFastMode: true
}), call the service method that reads the session from disk/persistence (e.g.,
service.getSession(session.id) or the test harness’s persistence loader) and
expect the reloadedSession.codexFastMode toBe(true); if needed, simulate a
restart of the service or re-instantiate the persistence-backed session store
before reloading to ensure the value was written to disk rather than only kept
in memory.
- Around line 1569-1633: Add an assertion to ensure setPermissionMode runs
before the automatically injected handoff prompt is sent: after calling
service.handoffSession(...) and before/alongside the existing send expectation,
read the mock invocation order (e.g.,
setPermissionMode.mock.invocationCallOrder[0] and
send.mock.invocationCallOrder.find(...) or send.mock.invocationCallOrder[0]) and
assert the setPermissionMode call index is smaller than the send call index for
the injected handoff message (the string containing "This message was injected
automatically by ADE during a chat handoff."); reference the setPermissionMode
and send mocks and the service.handoffSession invocation to locate where to add
this ordering check.
In `@apps/desktop/src/renderer/components/app/TopBar.test.tsx`:
- Around line 225-247: Add tests that exercise the double-confirm path in TopBar
by mocking checkForActiveWorkloads (or the underlying window.ade.project
lanes/sessions/agentChat/missions mocks) to return non-empty arrays so the
second confirmation is triggered, and spy on window.confirm to control both
prompts; assert that when both confirms return true
globalThis.window.ade.project.forgetRecent is called with the expected path, and
add a complementary test where the first confirm returns true but the second
returns false to assert forgetRecent is not called. Use the existing test
structure (render(<TopBar />), fireEvent.click(screen.getByTitle("Remove
project")), and waitFor/expect) and reference the checkForActiveWorkloads
behavior and forgetRecent call to locate the code paths to exercise.
In `@apps/desktop/src/renderer/components/app/TopBar.tsx`:
- Line 460: projectAccentColors (managed by useState via setProjectAccentColors)
accumulates entries keyed by rootPath when projects are removed/reordered;
update the state to prune keys that are no longer present in recentProjects
whenever projects change — for example, remove the rootPath entry inside
handleRemoveTab and after forgetRecent returns (or perform a cleanup pass
whenever recentProjects updates) so projectAccentColors only contains keys that
exist in recentProjects.
- Around line 82-83: deriveIconAccentColor currently keys projectIconAccentCache
with the full dataUrl and does not refresh LRU recency on hits; change the cache
key to a short identifier (preferably rootPath when available, or a small hash
of dataUrl) instead of the full data URL, and on cache hits update recency the
same way getProjectIconFromCache does (delete the entry and re-set it) so hot
keys aren’t evicted; update deriveIconAccentColor to accept/derive the short
key, use that for lookups/sets on projectIconAccentCache, and ensure any callers
provide rootPath or the hashed key.
In `@apps/desktop/src/renderer/components/chat/AgentChatComposer.tsx`:
- Around line 2768-2774: The CodexFastModeToggle is currently given an
always-truthy arrow function for onToggle, so clicks are enabled but swallowed
when onParallelSlotCodexFastModeChange is undefined; fix by only supplying a
real handler when one exists—either bind the handler ahead of time (e.g., create
a memoized callback using parallelConfiguringIndex and
onParallelSlotCodexFastModeChange) and pass that directly to
CodexFastModeToggle, or guard the prop by using a conditional: pass
onToggle={onParallelSlotCodexFastModeChange ? (next) =>
onParallelSlotCodexFastModeChange(parallelConfiguringIndex!, next) : undefined}
so the toggle correctly disables when the slot handler is missing.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsx`:
- Around line 491-492: The test currently spies on Date.now via dateNowSpy =
vi.spyOn(Date, "now").mockReturnValue(123) even though no assertions rely on a
fixed timestamp; either remove that spy and its cleanup in the finally block if
it's not needed, or if AgentChatPane/terminal internals require stable
timestamps for deterministic keys/sequencing, add an inline comment near the spy
explaining that dependency (reference Date.now, dateNowSpy and AgentChatPane)
and keep the mock; update the test to only mock Date.now when its value affects
the component behavior.
- Around line 415-456: The test currently globally stubs window.ade.sessions.get
to always return terminalSession which can incorrectly satisfy other session
lookups; change the stub to a scoped mockImplementation that returns
terminalSession only when the requested id equals terminalSession.id and
otherwise returns the normal/chat-shaped session (e.g., an object with toolType:
"codex-chat" or the previous default), by updating the
vi.mocked(window.ade.sessions.get) call in this test; keep the rest of the test
(renderPane, emitSessionChanged, assertions) unchanged.
In `@apps/desktop/src/renderer/components/shared/permissionOptions.test.ts`:
- Around line 123-137: The test "every option has required fields populated"
only aggregates getPermissionOptions for families "anthropic", "openai", and
"local"; add the missing families "cursor" and "opencode" (or better, iterate
over a canonical list of families used by getPermissionOptions) so the test also
validates options for getPermissionOptions({ family: "cursor", ... }) and
getPermissionOptions({ family: "opencode", ... }); update the allOptions
construction in the test to include those families (or replace the hardcoded
array with a loop over the master family list) so shortDesc, detail, allows,
safety, label, and value are asserted for every family.
In `@apps/desktop/src/renderer/components/terminals/WorkStartSurface.tsx`:
- Around line 43-57: Replace the duplicated CLI_PROVIDER_LIST with a single
source of truth: either derive the list from CLI_PROVIDER_OPTIONS using
Object.values(CLI_PROVIDER_OPTIONS) where needed (remove CLI_PROVIDER_LIST) or
introduce an explicit CLI_PROVIDER_ORDER: readonly CliProvider[] tuple and build
CLI_PROVIDER_LIST by mapping that tuple to CLI_PROVIDER_OPTIONS (e.g.
CLI_PROVIDER_ORDER.map(k => CLI_PROVIDER_OPTIONS[k])) so additions are
exhaustively checked and display order is preserved; update any uses of
CLI_PROVIDER_LIST to the new derived value and keep the symbols
CLI_PROVIDER_OPTIONS, CLI_PROVIDER_LIST (or new CLI_PROVIDER_ORDER) referenced
accordingly.
- Around line 22-32: The callback type for onLaunchPtySession is inconsistent:
WorkStartSurface.tsx declares profile: LaunchProfile while WorkViewArea.tsx
declares profile: CliProvider | "shell", causing a fragile mismatch; update the
declaration in WorkViewArea.tsx (and any other callers such as
useWorkSessions.ts) to use the canonical LaunchProfile type imported from
./cliLaunch so all onLaunchPtySession signatures match, remove the CliProvider |
"shell" union, update imports to reference LaunchProfile, and fix any call
sites/casts to conform to the single LaunchProfile definition.
In `@apps/desktop/src/renderer/components/terminals/WorkViewArea.tsx`:
- Around line 453-462: Replace the inline union type for the onLaunchPtySession
argument with the shared LaunchProfile alias so the component's API matches
WorkStartSurface and useWorkSessions; locate the onLaunchPtySession declaration
in WorkViewArea (the function/type that currently uses "profile: CliProvider |
\"shell\"") and change its profile type to LaunchProfile (importing
LaunchProfile from the module that defines it, e.g. cliLaunch.ts) so all three
usages use the same LaunchProfile symbol.
In `@apps/desktop/src/renderer/lib/sessions.ts`:
- Around line 206-236: Update isGenericSessionTitle to treat the new default
labels as generic: add normalized comparisons for "cursor cli session", "droid
cli session", and "opencode cli session" alongside the existing checks in the
big string-equality block inside isGenericSessionTitle (the function that
accepts TerminalSessionSummary session and value). Ensure you use the same
normalization (value.trim().toLowerCase()) and include these exact normalized
strings so primarySessionLabel will skip them the same way it does "claude
session" and "codex session".
🪄 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: 7be38c6d-3c7f-4c18-976e-a14fa826bb9a
⛔ Files ignored due to path filters (12)
docs/ARCHITECTURE.mdis excluded by!docs/**docs/features/agents/README.mdis excluded by!docs/**docs/features/chat/README.mdis excluded by!docs/**docs/features/chat/agent-routing.mdis excluded by!docs/**docs/features/chat/composer-and-ui.mdis excluded by!docs/**docs/features/chat/transcript-and-turns.mdis excluded by!docs/**docs/features/linear-integration/README.mdis excluded by!docs/**docs/features/onboarding-and-settings/configuration-schema.mdis excluded by!docs/**docs/features/project-home/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/README.mdis excluded by!docs/**docs/features/terminals-and-sessions/pty-and-processes.mdis excluded by!docs/**docs/features/terminals-and-sessions/ui-surfaces.mdis excluded by!docs/**
📒 Files selected for processing (74)
apps/ade-cli/src/cli.test.tsapps/ade-cli/src/cli.tsapps/desktop/resources/ade-cli-help.txtapps/desktop/scripts/regen-ade-cli-help.cjsapps/desktop/src/main/main.tsapps/desktop/src/main/services/adeActions/registry.test.tsapps/desktop/src/main/services/adeActions/registry.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserService.test.tsapps/desktop/src/main/services/builtInBrowser/builtInBrowserService.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/config/projectConfigService.tsapps/desktop/src/main/services/cto/ctoState.test.tsapps/desktop/src/main/services/cto/linearWorkflowFileService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/projects/adeProjectService.tsapps/desktop/src/main/services/projects/projectIconResolver.test.tsapps/desktop/src/main/services/projects/projectIconResolver.tsapps/desktop/src/main/services/projects/projectLifecycle.test.tsapps/desktop/src/main/services/projects/projectScaffoldService.test.tsapps/desktop/src/main/services/projects/projectScaffoldService.tsapps/desktop/src/main/services/pty/ptyService.test.tsapps/desktop/src/main/services/pty/ptyService.tsapps/desktop/src/main/services/sessions/sessionService.test.tsapps/desktop/src/main/services/sessions/sessionService.tsapps/desktop/src/main/services/state/globalState.test.tsapps/desktop/src/main/services/state/globalState.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.tsapps/desktop/src/main/utils/terminalSessionSignals.test.tsapps/desktop/src/main/utils/terminalSessionSignals.tsapps/desktop/src/preload/global.d.tsapps/desktop/src/preload/preload.tsapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/app/TopBar.test.tsxapps/desktop/src/renderer/components/app/TopBar.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.test.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.submit.test.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/chat/ChatTerminalDrawer.test.tsxapps/desktop/src/renderer/components/chat/ChatTerminalDrawer.tsxapps/desktop/src/renderer/components/chat/ChatWorkLogBlock.tsxapps/desktop/src/renderer/components/chat/chatTranscriptRows.test.tsapps/desktop/src/renderer/components/chat/chatTranscriptRows.tsapps/desktop/src/renderer/components/lanes/useLaneWorkSessions.tsapps/desktop/src/renderer/components/shared/permissionOptions.test.tsapps/desktop/src/renderer/components/shared/permissionOptions.tsapps/desktop/src/renderer/components/terminals/TerminalsPage.tsxapps/desktop/src/renderer/components/terminals/ToolLogos.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/cliLaunch.tsapps/desktop/src/renderer/components/terminals/useWorkSessions.tsapps/desktop/src/renderer/hooks/useClickOutside.tsapps/desktop/src/renderer/index.cssapps/desktop/src/renderer/lib/sessions.test.tsapps/desktop/src/renderer/lib/sessions.tsapps/desktop/src/renderer/lib/terminalAttention.tsapps/desktop/src/shared/adeCliGuidance.tsapps/desktop/src/shared/adeLayout.tsapps/desktop/src/shared/ipc.tsapps/desktop/src/shared/modelRegistry.tsapps/desktop/src/shared/types/builtInBrowser.tsapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/sessions.tsapps/ios/ADE/Models/RemoteModels.swiftapps/ios/ADE/Services/SyncService.swiftapps/ios/ADE/Views/Work/WorkPreviews.swiftapps/ios/ADE/Views/Work/WorkSessionSettingsSheet+Actions.swiftapps/ios/ADE/Views/Work/WorkSessionSettingsSheet.swiftapps/ios/ADETests/ADETests.swift
👮 Files not reviewed due to content moderation or server errors (3)
- apps/desktop/src/renderer/components/chat/AgentChatPane.tsx
- apps/desktop/src/renderer/components/terminals/cliLaunch.ts
- apps/desktop/src/renderer/components/terminals/cliLaunch.test.ts
| const resolveOpenCodeSessionIdFromCli = (args: { | ||
| cwd: string; | ||
| startedAt?: string | null; | ||
| maxStartDeltaMs?: number; | ||
| }): string | null => { | ||
| try { | ||
| const env: NodeJS.ProcessEnv = { ...process.env, NO_COLOR: "1" }; | ||
| delete env.FORCE_COLOR; | ||
| const result = spawnSync("opencode", ["session", "list", "--format", "json", "--max-count", "80"], { | ||
| cwd: args.cwd, | ||
| encoding: "utf8", | ||
| timeout: 4000, | ||
| maxBuffer: 1024 * 1024, | ||
| env, | ||
| }); |
There was a problem hiding this comment.
Avoid synchronous OpenCode session discovery on the Electron main thread.
This helper blocks the main process for up to 4s per lookup, and it does so with a bare process.env instead of the shell/env that actually launched the session. That means sessions can launch successfully via shell init or lane-specific env and still fail to backfill here, while the UI is stalled waiting on spawnSync.
- ChatTerminalDrawer: pull setActiveTabId out of setTabs updater - cli.ts: throw on conflicting --fast and --standard flags - agentChatService: presence check (!== undefined) for serviceTiers merges - projectConfigService: drop dead config.laneCleanup branch - registerIpc: redact builtInBrowser URL params in IPC log - globalState: clamp existingIndex < 12 to avoid sparse holes - terminalSessionSignals: tighten Droid wrap regex; validate resume candidates via parseTrackedCliResumeCommand - ChatWorkLogBlock: try/catch around activeForChat; .catch on handleTerminalClick - useWorkSessions: repeating regex for leading env-var stripping - WorkStartSurface: fall back to provider's defaultPermission, then first option, never hardcoded "default" - main.ts: pass preserveRecentOrder in ensureProjectContextForMobileSync - browserMock: restore "droid" in isMockChatToolType - index.css: focus-visible accent + ring on active tabs - WorkViewArea: align on LaunchProfile import for type consistency - cliLaunch: guard cursor-agent precreate on $ADE_CURSOR_CHAT_ID Verified: terminalSessionSignals + globalState + agentChatService + TopBar + ChatTerminalDrawer + WorkViewArea + useWorkSessions vitest pass; desktop + ade-cli typecheck clean; cli.test.ts (91) pass. Skipped: Greptile Droid encodeURIComponent (would break dir lookup), CodeRabbit OpenCode async-discovery (architecture, out of scope), TopBar double-confirm reorder (broke an existing sync test — needs paired test+code change later), Copilot interactionMode claim (incorrect: code already inherits via resolveSessionClaudeInteractionMode).
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary
Test plan
Summary by CodeRabbit
Release Notes
New Features
Improvements
Greptile Summary
This PR adds first-class CLI support for cursor-agent, droid, and opencode alongside existing claude/codex providers \u2014 covering launch, resume, permission mapping, and terminal signal extraction. It also ships UA spoofing for the built-in browser (replacing the old Google sign-in redirect), codex fast-mode service-tier forwarding, PTY context env injection, a localhost-server shortcut strip in the work-log block, and dedup fixes in
ChatTerminalDrawer.WorkStartSurfaceexpands from 2 providers to 5;terminalSessionSignals.tsadds per-provider launch-config parsing, resume-command building, and permission detection for cursor, droid, and opencode;ptyServicegains session-ID backfill for droid (file scan) and opencode (spawnSyncCLI call).builtInBrowserServiceinjects a desktop-Chrome user-agent and Sec-CH-UA headers into every request viaonBeforeSendHeaders, removing the Google-sign-in external-redirect workaround.agentChatServicepropagatesserviceTier: \"fast\" | nullin every JSON-RPC call when the selected model supports it, reading the toggle from persistedcodexFastModestate.Confidence Score: 3/5
The change is functional but the two previously-flagged issues — spawnSync blocking the Electron main thread on every OpenCode resume backfill and the droid path-collision in resolveDroidSessionIdFromStorage — remain unaddressed and affect real user-facing paths.
The spawnSync("opencode", ..., { timeout: 4000 }) call runs on the main Electron process thread; any OpenCode session that reaches the backfill path will freeze the application for up to four seconds. The droid path-collision means /home/user/my-project and /home/user/my/project escape to the same string, causing silent session misidentification. Both were flagged in the previous review round and are still present.
apps/desktop/src/main/services/pty/ptyService.ts (resolveOpenCodeSessionIdFromCli spawnSync + resolveDroidSessionIdFromStorage path collision) and apps/desktop/src/main/utils/terminalSessionSignals.ts (normalizeResumeCommand returns pre-stripped string for non-Claude providers).
Important Files Changed
Sequence Diagram
sequenceDiagram participant UI as WorkStartSurface participant CLi as cliLaunch.ts participant PTY as ptyService participant Shell as Shell/PTY participant Sig as terminalSessionSignals UI->>CLi: buildTrackedCliLaunchCommand(provider, permMode) note over CLi: cursor: cursor-agent create-chat one-liner<br/>droid: ADE_DROID_SETTINGS mktemp one-liner<br/>opencode: OPENCODE_CONFIG_CONTENT env prefix CLi-->>UI: startupCommand, args, env UI->>PTY: launchPtySession PTY->>PTY: withAdeTerminalContextEnv PTY->>Shell: spawn shell, write startupCommand Shell->>Shell: CLI agent runs Shell-->>PTY: terminal output PTY->>Sig: extractResumeCommandFromOutput Sig-->>PTY: provider + targetId PTY->>PTY: backfill via resolveDroidSessionIdFromStorage or resolveOpenCodeSessionIdFromCli PTY-->>UI: resume metadata storedComments Outside Diff (2)
apps/desktop/src/main/services/pty/ptyService.ts, line 1 (link)spawnSyncblocks the Electron main processresolveOpenCodeSessionIdFromClicallsspawnSync("opencode", ..., { timeout: 4000 })with a 4-second ceiling. BecauseptyServiceruns on the main Electron process thread, any call to this backfill path (triggered whenever an OpenCode session lacks a resume ID) freezes the entire application — including window redraws and IPC — for up to 4 seconds. Usespawnwith async event handlers, or move the lookup into a worker thread / a separate child-process utility that doesn't block the main thread.Prompt To Fix With AI
apps/desktop/src/main/services/chat/agentChatService.ts, line 1 (link)interactionModesilently dropped from session forkTwo lines that propagated
interactionModeto the forked session were removed in this PR without a stated rationale: (1)interactionMode: managed.session.interactionModewas removed from thecreateSessionargs at the fork site, and (2) the post-creation assignmentcreatedManaged.session.interactionMode = managed.session.interactionMode ?? sourceSession.interactionMode ?? nullwas removed. Any session forked from a parent that hadinteractionModeset will now silently default tonull, changing the agent's behaviour in the child session. If this removal is intentional, please document the reason.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "ship: iteration 1 — address Greptile + C..." | Re-trigger Greptile