Conversation
Share createAcpHostClient and acquireAcpCliConnection between providers. Refactor Cursor ACP pool to delegate spawn/init and cursor_login handling. Co-authored-by: Arul Sharma <arul28@users.noreply.github.com>
- Spawn droid exec --output-format acp with pooled JSON-RPC like Cursor - Dynamic droid/* model descriptors, discovery + defaults, auth via CLI + FACTORY_API_KEY - Wire DroidRuntime through agent chat (turns, steer queue, interrupt, permissions, persistence) - Extend AI status, model picker grouping, providers settings, missions permissions for droid Co-authored-by: Arul Sharma <arul28@users.noreply.github.com>
…safety - Serialize acquire per poolKey with shared pending init; fix double inner ref on cursor/droid fast path - Evict stale entries on child close/error; kill process and log stderr tail on init/handshake failure - Resolve ACP terminal cwd within lane root; cap waitForTerminalExit with SIGKILL + close wait Co-authored-by: Arul Sharma <arul28@users.noreply.github.com>
- Prefer DROID_EXECUTABLE/FACTORY_DROID_EXECUTABLE over synthetic auth paths - Do not treat droid --version as login; use FACTORY_API_KEY, ~/.factory/settings, or auth probes - Only inject FACTORY_API_KEY cli-subscription when binary resolves (not fallback-command) - Align droid blocker and settings login hint with actual auth steps - Honor interrupt during Droid runtime setup and before prompt dispatch; avoid Claude fallback on cancel mid-setup - Approval_request detail: cursorAcp only for Cursor; add acpHost source - Map unified edit mode to Droid --auto medium Co-authored-by: Arul Sharma <arul28@users.noreply.github.com>
…interrupt events - inspectDroidCliPresence: STRONG_UNAUTH => verified true; inconclusive => verified false - detectCliAuthStatuses droid: use resolveDroidExecutable path for probes - ensureDroidRuntime: abort if managed.closed after acquire; release pool - runDroidTurn: emit interrupted status + terminal events on early exit and closed-during-setup Co-authored-by: Arul Sharma <arul28@users.noreply.github.com>
…er on close - Replace recursive acquireAcpCliConnection with retry loop (max attempts + backoff) - Clear SIGKILL timeout when process closes before WAIT_FOR_TERMINAL_EXIT_MAX_MS Co-authored-by: Arul Sharma <arul28@users.noreply.github.com>
…nd pool key normalization
- authDetector: inspect stdout+stderr for auth patterns regardless of exit code
- acpCliPool: getRootPath returns options.spawn.cwd instead of empty string
- agentChatService: prevent double releaseDroidAcpConnection via released flag
- agentChatService: droidPoolKeyFor uses resolved model ID, not raw session model
- cursorAcpPool/droidAcpPool: add generation tokens to prevent stale-generation
races — acquire returns { pooled, generation }, release validates generation
- agentChatService: thread poolGeneration through CursorRuntime/DroidRuntime
- Update test mocks to match new { pooled, generation } return shape
Introduces an autonomous PR-to-merge driver that runs automate → finalize once, then polls CI and review comments on a self-paced 12-min cadence, fixing valid comments and failing tests in place. Prefers TeamCreate agent teams when available, falls back to parallel Agent calls otherwise. Opens the PR via `ade prs create` when possible so it shows up in ADE's PR tracking; falls back to `gh pr create` only after the agent has genuinely exhausted the ADE path via `--help`-driven discovery. Also narrows /automate to run only the new and affected tests (not the full suite), and makes /finalize's 8-shard parallel run explicit so shards don't get chained serially. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> (cherry picked from commit 68ef71a)
Lane worktrees use symlinks named node_modules pointing at the main checkout's installed copies. The previous `node_modules/` pattern only matched directories, so git saw the symlinks as untracked — which in turn blocked `ade prs create` preflight and forced /shipLane to fall back to `gh pr create`. Without the slash, the pattern matches both directories (main checkout) and symlinks (worktrees). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> (cherry picked from commit 07623ee)
The 8-shard vitest run, parallel tsup builds, and typecheck fans sometimes leave worker pools behind after the phase exits. They don't fail CI but accumulate across runs and can hold file locks. New Phase 3j kills them scoped to apps/ paths so vitest instances the user may have running elsewhere aren't affected. Adds a Cleanup line to the Phase 4 summary and the completion checklist. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> (cherry picked from commit 3a5fbe5)
Resolves conflicts in package.json, finalize/automate/shipLane commands, ship-lane playbook, main.ts shutdown chain, agentChatService chat plumbing, cursorAcpPool refactor, AgentChatPane provider routing, sessionService tests, and shared utils spawn/termination helpers. Drops the lane's mcpServers wiring for ACP sessions to align with main's intentional removal in PR #166; restores Windows-portable spawn invocation in acpHostClient. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors the codexExecutable test pattern: env override beats auth path, auth path beats PATH lookup, fallback returns the bare 'droid' command.
aiIntegrationService.ts now reads providerConnections.droid.runtimeAvailable
unconditionally, but the test helper only built {claude, codex, cursor},
producing TypeError on getStatus. Add droid to the helper, ServiceFactoryOptions
availability, and the cli statuses / detectAllAuth mocks.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Owner
Author
|
@copilot review but do not make fixes |
📝 WalkthroughWalkthroughThis pull request adds comprehensive support for Factory Droid CLI as a first-class AI provider across authentication, connection pooling, model discovery, orchestration, and UI. It introduces new ACP infrastructure, auth detection logic, type definitions, and React components to integrate Droid alongside existing providers. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Copilot stopped work on behalf of
arul28 due to an error
April 28, 2026 15:41
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
|
|
||
| // The replay chunk contains text from previous turns. If the chunk's | ||
| // text is found in the accumulated history, it's a replay — drop it. | ||
| if (entry.historyText.length > 0 && entry.historyText.includes(chunkText)) { |
There was a problem hiding this comment.
[🟡 Medium] [🟡 Investigate]
The isDuplicateDroidNotification function uses String.includes() to detect replay chunks from the Droid CLI. Both historyText.includes(chunkText) (line 3376) and currentTurnText.includes(chunkText) (line 3382) match when a legitimate new streaming chunk happens to be a substring of previously accumulated text. For example, if a prior turn produced "This function returns the value" and a new turn streams a chunk "the value", it would be silently dropped as a false-positive replay. Short chunks (common in LLM streaming) are especially vulnerable. A length-ratio guard (e.g., only treat as replay if chunkText.length > historyText.length * 0.5) or a prefix-match approach would reduce false positives while still catching the described whole-conversation replay behavior.
// agentChatService.ts
if (entry.historyText.length > 0 && entry.historyText.includes(chunkText)) {
return true;
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/desktop/src/renderer/lib/modelOptions.test.ts (1)
15-24:⚠️ Potential issue | 🟡 MinorPlease add a Droid-specific assertion here.
This helper now carries
droid, but the suite still never proves that a Droid-authenticated status contributes Droid model ids/options. A provider-specific regression would still pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/modelOptions.test.ts` around lines 15 - 24, The test helper makeStatus now includes droid fields but the suite lacks a Droid-specific assertion; update the relevant test in modelOptions.test.ts to include a case where makeStatus returns detectedAuth: ['droid'] and models.droid contains one or more model entries, then assert that the code under test (the function that builds model options) includes those Droid model ids/options in its output (use the same assertions pattern as for claude/codex/cursor to verify provider-specific inclusion). Reference makeStatus, AiSettingsStatus, availableProviders, detectedAuth, models, and the droid key when adding this assertion.apps/desktop/src/shared/types/config.ts (1)
1069-1082:⚠️ Potential issue | 🟠 MajorAdd
"droid"toAiChatConfig.defaultProvidertoo.These status contracts now make Droid first-class, but Line 1206 still narrows
AiChatConfig.defaultProviderto"codex" | "claude" | "cursor" | "opencode" | "last_used". That leaves typed config flows unable to persist Droid as the default chat provider.Suggested fix
export type AiChatConfig = { - defaultProvider?: "codex" | "claude" | "cursor" | "opencode" | "last_used"; + defaultProvider?: "codex" | "claude" | "cursor" | "droid" | "opencode" | "last_used"; defaultApprovalPolicy?: "auto" | "approve_mutations" | "approve_all"; sendOnEnter?: boolean;As per coding guidelines, keep IPC contracts, preload types, shared types, and renderer usage in sync whenever an interface changes.
Also applies to: 1280-1293
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/shared/types/config.ts` around lines 1069 - 1082, The defaultProvider union for AiChatConfig still excludes "droid", causing Droid not to be persistable: update the AiChatConfig.defaultProvider type to include "droid" alongside "codex" | "claude" | "cursor" | "opencode" | "last_used" and mirror that change in the other occurrence noted (the second union around the later block), ensuring both places accept "droid" so the shared IPC/preload/renderer types stay in sync with AiSettingsStatus which already treats droid as first-class.apps/desktop/src/renderer/lib/sessions.ts (1)
52-65:⚠️ Potential issue | 🟡 MinorSync the new Droid label with the other session-label helpers.
Line 62 introduces
"Droid chat"as the autogenerated title, butisGenericSessionTitle()andSHORT_TOOL_TYPE_LABELSbelow still don't know aboutdroid-chat. That makes Droid sessions behave differently from the other chat providers: optimistic/new sessions can keep the generic title instead of falling through to goal/summary, and compact cards will renderdroid chatinstead ofDroid.💡 Suggested follow-up
const SHORT_TOOL_TYPE_LABELS: Record<string, string> = { shell: "Shell", "run-shell": "Run", cursor: "Cursor", + "droid-chat": "Droid", aider: "Aider", continue: "Continue", }; @@ if ( normalized === "opencode chat" || normalized === "claude chat" || normalized === "codex chat" || normalized === "cursor chat" || + normalized === "droid chat" || normalized === "claude code" || normalized === "claude cli" || normalized === "claude session" || normalized === "codex" ||Also applies to: 132-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/lib/sessions.ts` around lines 52 - 65, Update the other session-label helpers to recognize the new "droid-chat" tool type so Droid sessions behave like other chat providers: add "droid-chat" to the logic in isGenericSessionTitle() and include a corresponding entry ("Droid") in SHORT_TOOL_TYPE_LABELS (and any similar mappings) so the optimistic/new session title, compact cards, and summary/goal fallback match defaultSessionLabel's "Droid chat" behavior; use the exact symbol names isGenericSessionTitle and SHORT_TOOL_TYPE_LABELS to locate and modify the code.apps/desktop/src/renderer/components/shared/permissionOptions.ts (1)
28-34:⚠️ Potential issue | 🟠 MajorPersistent-identity Droid still falls back to the generic
editpreset.CLI-wrapped
factorynow has its own permission ladder, but this helper still only special-cases Anthropic. Any persistent-identity flow that uses the shared helper will label or create Droid’s guarded mode as legacyedit, which no longer matches the Droid-specific presets added below.💡 Suggested fix
export function resolvePersistentIdentityGuardedPermissionMode(opts: { family: string; isCliWrapped: boolean; }): AgentChatPermissionMode { const family = normalizePermissionFamily(opts.family); - return opts.isCliWrapped && family === "anthropic" ? "default" : "edit"; + if (!opts.isCliWrapped) return "edit"; + if (family === "anthropic") return "default"; + if (family === "factory") return "default"; // or the explicit guarded Droid tier you want here + return "edit"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/shared/permissionOptions.ts` around lines 28 - 34, The helper resolvePersistentIdentityGuardedPermissionMode currently only special-cases "anthropic" and returns "edit" for other families; update its conditional to treat CLI-wrapped "factory" the same as "anthropic" so Droid persistent-identity uses the Droid-specific guarded preset. In practice, inside resolvePersistentIdentityGuardedPermissionMode (which calls normalizePermissionFamily on opts.family), change the ternary/conditional that checks opts.isCliWrapped && family === "anthropic" to also match the normalized "factory" family (e.g., opts.isCliWrapped && (family === "anthropic" || family === "factory")) so the function returns "default" for CLI-wrapped factory flows.apps/desktop/src/main/services/ai/aiIntegrationService.ts (1)
1276-1280:⚠️ Potential issue | 🟠 MajorDroid is exposed in status, but its runtime connection is still missing.
availableProviders.droidandmodels.droidare now returned here, butbuildRuntimeConnections()still builds only{ claude, codex, cursor }. Any consumer readingruntimeConnectionswill see Droid as missing even when the provider is ready.🛠️ Suggested fix
const runtimeConnections: AiRuntimeConnections = { claude: toCliRuntimeConnection(args.providerConnections.claude), codex: toCliRuntimeConnection(args.providerConnections.codex), cursor: toCliRuntimeConnection(args.providerConnections.cursor), + droid: toCliRuntimeConnection(args.providerConnections.droid), };Also applies to: 1342-1354
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/aiIntegrationService.ts` around lines 1276 - 1280, The runtimeConnections object is missing Droid even though availability and models now include it; update buildRuntimeConnections (and any places constructing runtimeConnections) to include droid alongside claude, codex, and cursor so that availableProviders.droid and models.droid are reflected in runtimeConnections; locate the buildRuntimeConnections function and the code constructing the availability/runtimeConnections objects and add the droid entry using providerConnections.droid.runtimeAvailable (and the corresponding model/runtime info) so consumers no longer see Droid as absent.
🧹 Nitpick comments (4)
apps/desktop/src/main/services/orchestrator/orchestratorQueries.ts (1)
281-297: Consider centralizing executor-kind normalization.
normalizeExecutorKindhere and the mirrored allowlist in recovery logic now both require manual updates ("droid"in this PR). A shared helper/constant would reduce drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/orchestrator/orchestratorQueries.ts` around lines 281 - 297, The normalizeExecutorKind implementation duplicates an allowlist that's also replicated in the recovery logic, causing drift when updating executor kinds (e.g., "droid"); extract the canonical list and normalization into a single shared helper or exported constant used by both normalizeExecutorKind and the recovery logic so updates are made in one place; specifically, create an exported constant (e.g., ALLOWED_EXECUTOR_KINDS) and a single normalize function (e.g., normalizeExecutorKind or normalizeExecutorKindFromList) and replace the local array/allowlist usages in normalizeExecutorKind and the recovery code to reference this shared symbol.apps/desktop/src/main/services/orchestrator/aiOrchestratorService.ts (1)
5497-5508: Consider centralizing executor-kind allowlists to prevent enum drift.The same executor list is repeated in multiple spots. A single constant would reduce maintenance risk when providers change again.
♻️ Suggested refactor
+const PLAN_ADJUSTER_EXECUTOR_KINDS = ["claude", "codex", "cursor", "droid", "opencode", "manual"] as const; + const adjustmentSchema = { type: "object", properties: { @@ - newExecutorKind: { type: "string", enum: ["claude", "codex", "cursor", "droid", "opencode", "manual"] }, + newExecutorKind: { type: "string", enum: [...PLAN_ADJUSTER_EXECUTOR_KINDS] }, @@ - executorKind: { type: "string", enum: ["claude", "codex", "cursor", "droid", "opencode", "manual"] } + executorKind: { type: "string", enum: [...PLAN_ADJUSTER_EXECUTOR_KINDS] } @@ - const executorKind = typeof newStep.executorKind === "string" && - ["claude", "codex", "cursor", "droid", "opencode", "manual"].includes(newStep.executorKind) + const executorKind = typeof newStep.executorKind === "string" && + PLAN_ADJUSTER_EXECUTOR_KINDS.includes( + newStep.executorKind as (typeof PLAN_ADJUSTER_EXECUTOR_KINDS)[number] + ) ? (newStep.executorKind as OrchestratorExecutorKind) : ("opencode" as OrchestratorExecutorKind);Also applies to: 5565-5568
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/orchestrator/aiOrchestratorService.ts` around lines 5497 - 5508, The duplicated executor-kind enum should be centralized: create a single constant (e.g., ALLOWED_EXECUTOR_KINDS or ExecutorKindEnum) and reference it from the JSON schema definitions instead of repeating literals; update the schema entries that use newExecutorKind and newStep.properties.executorKind (and the other occurrence around the noted 5565-5568 block) to read their enum from that constant so changes to allowed executors are made in one place.apps/desktop/src/renderer/components/settings/ProvidersSection.tsx (1)
722-751: Consider extracting a shared CLI runtime card renderer.This new card duplicates the Claude/Codex/Cursor card structure almost 1:1, which increases drift risk when status or messaging logic changes later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/components/settings/ProvidersSection.tsx` around lines 722 - 751, The Factory Droid card duplicates the existing CLI runtime card layout used for Claude/Codex/Cursor; extract a reusable component (e.g., CliRuntimeCard) and replace duplicated JSX in ProvidersSection with that component. CliRuntimeCard should accept props: tool (from CLI_TOOLS), connection (providerConnections[tool.cli]), isInitialCheckInFlight, and functions/values used to compute UI like describeCredentialSource(connection), getStatusTone(connection), buildCliMessage(tool, connection), and should render ProviderLogo, status icon logic, credentialSourceDesc, and path exactly once. Update ProvidersSection to call <CliRuntimeCard .../> for the Droid entry (and reuse for other CLI entries) so status/messaging logic lives in one place and you no longer repeat the section JSX.apps/desktop/src/main/services/chat/droidModelsDiscovery.ts (1)
46-47: Key the discovery cache by executable path.The cache is global, but this function takes
droidPath. If ADE switches between bundled/user-installed Droid binaries, or tests invoke different fixtures, you'll reuse the wrong model list until the TTL expires.♻️ Suggested refactor
-let cached: { at: number; models: DroidExecHelpModelRow[] } | null = null; +const cache = new Map<string, { at: number; models: DroidExecHelpModelRow[] }>(); const TTL_MS = 120_000; @@ async function listDroidModelsFromCli(droidPath: string): Promise<DroidExecHelpModelRow[]> { const now = Date.now(); - if (cached && now - cached.at < TTL_MS) { - return cached.models; + const cached = cache.get(droidPath); + if (cached && now - cached.at < TTL_MS) { + return cached.models; } @@ const rows = parseDroidExecHelpModels(helpResult.stdout ?? ""); if (rows.length) { - cached = { at: now, models: rows }; + cache.set(droidPath, { at: now, models: rows }); return rows; } @@ if (rows.length) { - cached = { at: now, models: rows }; + cache.set(droidPath, { at: now, models: rows }); return rows; } @@ if (bare.length >= 3) { - cached = { at: now, models: bare }; + cache.set(droidPath, { at: now, models: bare }); return bare; } @@ - cached = { at: now, models: [] }; + cache.set(droidPath, { at: now, models: [] }); return []; } @@ export function clearDroidCliModelsCache(): void { - cached = null; + cache.clear(); }Also applies to: 92-105, 141-145, 163-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/chat/droidModelsDiscovery.ts` around lines 46 - 47, The global cache variable cached (and TTL_MS usage) is keyed only by time and will return models for the wrong executable when different droidPath values are used; change the cache to be keyed by the executable path (e.g., replace cached with a Map<string, { at: number; models: DroidExecHelpModelRow[] }>) and update every place that reads/writes cached (the discovery logic that accepts droidPath and the checks that compare TTL_MS) to lookup/insert by droidPath; ensure when storing you record Date.now() in at and when reading you validate TTL_MS per-path and fall back to fresh discovery if expired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ade-cli/src/cli.ts`:
- Line 1516: The new droid-related flags used at the readValue call site
(droidPermissionMode: readValue(args, ["--droid-permission-mode",
"--droid-autonomy", "--autonomy"])) are value-taking flags but were not added to
VALUE_CARRIER_FLAGS; update the VALUE_CARRIER_FLAGS set/array to include
"--droid-permission-mode", "--droid-autonomy", and "--autonomy" so the CLI
parser treats them as value carriers and prevents misparsing of "--help" as a
flag value.
In `@apps/desktop/src/main/services/ai/aiIntegrationService.ts`:
- Around line 810-827: The current gate uses hasDroidCliAuth to decide whether
to run discoverDroidCliModelDescriptors, which prevents discovery when the user
has a FACTORY_API_KEY auth path but an unauthenticated CLI; update the condition
to also allow discovery when auth contains a factory API key. Concretely, add a
predicate (e.g., hasFactoryApiKey = auth.some(entry => entry.key ===
"FACTORY_API_KEY" || entry.type === "factory-api" /* adjust to real schema */))
and change the if to if (hasDroidCliAuth || hasFactoryApiKey) { ... } so
resolveDroidExecutable and discoverDroidCliModelDescriptors still run for
env/API-key-based auth while keeping the existing try/catch behavior.
In `@apps/desktop/src/main/services/ai/authDetector.ts`:
- Around line 1040-1058: The code is re-resolving the Droid binary with
resolveDroidExecutable and treating a "fallback-command" result as installed:
false, which discards a previously-detected runnable cmd/path (from
commandExists/commandPath); change the logic in the droid branch to prefer the
already-discovered executable path (the cmd or commandPath result) and only call
resolveDroidExecutable when no prior path is available; when
resolveDroidExecutable returns "fallback-command" do not mark installed false if
a prior path exists—use the prior path and call
inspectDroidCliPresence(resolvedPathOrPrior) to set
installed/authenticated/verified via inspectDroidCliPresence instead of hiding
PATH-only/custom installs.
In `@apps/desktop/src/main/services/chat/acpCliPool.ts`:
- Around line 77-96: The acquireAcpCliConnection function captures poolEpoch
into initEpoch but doesn't fail fast when poolEpoch advances (shutdown) during
retries; add an early check that compares poolEpoch to initEpoch at the top of
each attempt (after the backoff await and before creating/claiming pendingInit
or spawning new ACL processes) and immediately throw or return a failed/aborted
error when they differ so we don't spawn/init doomed CLI processes; ensure the
same guard is added in the other affected section referenced (around lines
200-202) that creates or uses pendingInit.
In `@apps/desktop/src/main/services/chat/acpHostClient.ts`:
- Around line 274-282: killTerminal currently only sends a one-time SIGTERM (via
signalChildProcessTree) and can leave processes running if they ignore SIGTERM;
update AcpHostTermState to include killTimer?: NodeJS.Timeout | null, then in
killTerminal set a timer (e.g., 5s) that will call signalChildProcessTree(...,
"SIGKILL") if the process hasn't exited, storing the timer on the terminal
state; ensure you clear and null out killTimer when the terminal's close/exit
handler runs (and when releaseTerminal or waitForTerminalExit cleans up) so
timers don't leak or double-kill, and guard against scheduling multiple timers
if killTerminal is called repeatedly.
In `@apps/desktop/src/main/services/chat/agentChatService.test.ts`:
- Around line 9784-9882: The test currently only asserts setSessionModel was
eventually called with the translated ACP id but doesn't ensure the untranslated
Droid help id was never sent; update the test to assert setSessionModel was
never called with the original help id string (e.g.,
"custom:claude-sonnet-4-6-thinking-32000") by checking the mock for absence of
that argument before/after service.sendMessage, using the existing
setSessionModel mock to verify the untranslated modelId was not passed at any
point.
- Around line 9682-9782: The test currently only checks
unstable_setSessionModel() was called but not that it happened before the first
prompt; update the mocks so unstable_setSessionModel (the setSessionModel fn)
records a sequence/order marker (e.g., increment mockState.sequenceCounter and
push the value to mockState.droidSetModelSeq) and modify the prompt mock (the
prompt fn that pushes to mockState.droidPromptCalls) to push its own sequence
number, then add an assertion that the setSessionModel sequence for the session
is less than the prompt sequence (i.e., mockState.droidSetModelSeq[0] <
mockState.droidPromptSeq[0]) to guarantee the model switch happened before the
first prompt.
In `@apps/desktop/src/main/services/chat/cursorAcpPool.ts`:
- Around line 97-112: Before deleting stale pool entries from cursorPools (the
branches checking staleOuter / existing for args.poolKey), call
clearCursorTerminalTimers(...) with the pooled object being evicted so any
pending terminal timers are cleared; specifically, in the first branch where
staleOuter && !hasActiveAcpCliPoolEntry(innerKey) invoke
clearCursorTerminalTimers(staleOuter.pooled or staleOuter) before
cursorPools.delete(args.poolKey), and likewise in the later branch where
existing is truthy (stale existing) call
clearCursorTerminalTimers(existing.pooled or existing) before
cursorPools.delete(args.poolKey). Ensure you still perform
acquireAcpCliConnection(acpOptions) and increment existing.ref only in the
live-connection branch.
- Around line 114-149: The init waiters can receive a stale pooled object if the
pool was torn down between awaiting init and taking a retain; after awaiting
init in the non-owner path (the code around init, pooled, and the non-owner
branch that calls acquireAcpCliConnection), validate that the pooled structure
is still current by comparing its generation with the live entry in cursorPools
(use cursorPools.get(args.poolKey) and the generation field) and only increment
ref or return the pooled if they match; if they don’t match, discard the awaited
pooled and repeat/acquire the current connection (call acquireAcpCliConnection
again or re-run the acquire logic) so the waiter always returns the live pooled
instance and increments its ref atomically.
In `@apps/desktop/src/main/services/chat/droidAcpPool.ts`:
- Around line 125-132: The returned generation can mismatch the awaited pooled
because after awaiting init (stored in pooled) the pool entry at
droidPools.get(args.poolKey) may have been replaced; modify init so it resolves
an object that includes the current generation (e.g., make init return { pooled,
generation }) or, if changing init is undesirable, immediately capture the
generation atomically by reading the entry inside the same init promise context;
then use that captured generation when returning instead of re-reading
droidPools after await; reference symbols: init, initOwner, pooled, generation,
droidPools, args.poolKey, acquireAcpCliConnection.
In `@apps/desktop/src/main/services/orchestrator/orchestratorService.ts`:
- Line 3826: defaultAdapterFor currently allows "droid" but the fallback logic
later only routes to Claude/Codex causing misrouting; update defaultAdapterFor
to not include "droid" (or add an explicit early check like if (kind ===
"droid") return null) so Droid requests are never sent through the Claude/Codex
fallback path and instead fall back to the shared-adapter existence check;
locate the check in the defaultAdapterFor function and remove "droid" from the
allowed kinds or add the explicit return for "droid".
In `@apps/desktop/src/main/services/orchestrator/permissionMapping.ts`:
- Line 106: The new default droid: "full-auto" can be incorrectly set for legacy
configs because the cli.mode normalization only maps to claude/codex; update the
normalization logic and mergeMissionPermissionConfig to also derive and
propagate a droid permission from legacy cli.mode using the same mapping rules
applied to claude/codex (i.e., when computing normalized permissions from
cli.mode, set the corresponding droid property rather than leaving droid at a
hardcoded "full-auto"), so legacy restrictive CLI settings flow through to the
droid key as well.
In `@apps/desktop/src/main/services/shared/utils.ts`:
- Around line 191-215: The timeout path calls terminateChildProcessTree(child,
null, 1_500) but drops its returned escalation timer; change that call to
capture the returned timer into hardResolveHandle (e.g., hardResolveHandle =
terminateChildProcessTree(...)) and ensure settle clears it (already clearing
hardResolveHandle via clearTimeout in settle) so the delayed SIGKILL can be
cancelled if the child exits after SIGTERM; keep references to timeoutHandle and
hardResolveHandle as NodeJS.Timeout | null and make sure no other timers are
leaked, leaving destroyChildProcessStreams(child), settle,
child.once("close"/"error") and the existing clearTimeout calls intact.
In `@apps/desktop/src/main/services/sync/syncRemoteCommandService.ts`:
- Around line 552-553: The current parsing logic can set
parsed.droidPermissionMode to null when asTrimmedString returns null; change
both occurrences (the assignment for parsed.droidPermissionMode at the first
block and the duplicate at lines ~677-678) so that when
value.droidPermissionMode is null/undefined you set parsed.droidPermissionMode =
undefined, and when asTrimmedString(...) returns null/empty you also map that to
undefined (e.g. capture the trimmed result and use (trimmed ?? undefined) cast
to AgentChatCreateArgs["droidPermissionMode"]); keep the existing cursorModeId
behavior intact if desired but apply the same null→undefined normalization for
any other droidPermissionMode assignments.
In `@apps/desktop/src/renderer/browserMock.ts`:
- Around line 376-386: The new inferMockChatProvider can return "droid" for
toolType values that don't end in "-chat", but isMockChatToolType still only
accepts cursor or values ending in "-chat", causing Droid sessions to be
filtered out; update isMockChatToolType to include the same Droid detection used
in inferMockChatProvider (e.g., accept toolType === "droid" or
toolType.startsWith("droid") / toolType === "droid-chat") so that tool types
mapped to "droid" by inferMockChatProvider are preserved in the browser mock
session list.
In `@apps/desktop/src/renderer/components/chat/AgentChatPane.tsx`:
- Around line 1609-1611: The droid autonomy should prefer a legacy
session-stored permission when session.droidPermissionMode is missing; update
the logic that calls setDroidPermissionMode so it falls back to
session.permissionMode before falling back to
initialNativeControls.droidPermissionMode (i.e., use session.droidPermissionMode
?? session.permissionMode ?? initialNativeControls.droidPermissionMode), keeping
setOpenCodePermissionMode and cursorModeId logic unchanged.
In `@apps/desktop/src/renderer/components/settings/UsageGuardrailsSection.tsx`:
- Around line 161-169: The warning condition in showEmptyQuotaWarning
incorrectly includes cliAuth.droid?.authAvailable but does not check any droid
windows, causing a false warning for Droid-only auth; update the condition in
showEmptyQuotaWarning to either remove cliAuth.droid?.authAvailable from the
initial auth check or add a droid window presence check (e.g., include a
droidWindows.length check alongside claudeWindows and codexWindows) so the logic
only triggers when an authenticated provider has zero corresponding windows.
In
`@apps/desktop/src/renderer/components/shared/providerModelSelectorGrouping.ts`:
- Around line 164-173: The classifyProviderGroup function now classifies models
with family === "factory" into the "droid" group, but there is no corresponding
provider metadata for the raw provider key "factory", causing fallback
label/color/order; add a "factory" entry to the provider metadata maps used for
rendering (e.g., the provider metadata/labels/colors/order objects/constants) so
that the provider key "factory" has the intended display label, color and sort
order; update the same provider maps where other providers are defined so
"factory" uses the Droid styling and ordering to avoid default fallbacks.
---
Outside diff comments:
In `@apps/desktop/src/main/services/ai/aiIntegrationService.ts`:
- Around line 1276-1280: The runtimeConnections object is missing Droid even
though availability and models now include it; update buildRuntimeConnections
(and any places constructing runtimeConnections) to include droid alongside
claude, codex, and cursor so that availableProviders.droid and models.droid are
reflected in runtimeConnections; locate the buildRuntimeConnections function and
the code constructing the availability/runtimeConnections objects and add the
droid entry using providerConnections.droid.runtimeAvailable (and the
corresponding model/runtime info) so consumers no longer see Droid as absent.
In `@apps/desktop/src/renderer/components/shared/permissionOptions.ts`:
- Around line 28-34: The helper resolvePersistentIdentityGuardedPermissionMode
currently only special-cases "anthropic" and returns "edit" for other families;
update its conditional to treat CLI-wrapped "factory" the same as "anthropic" so
Droid persistent-identity uses the Droid-specific guarded preset. In practice,
inside resolvePersistentIdentityGuardedPermissionMode (which calls
normalizePermissionFamily on opts.family), change the ternary/conditional that
checks opts.isCliWrapped && family === "anthropic" to also match the normalized
"factory" family (e.g., opts.isCliWrapped && (family === "anthropic" || family
=== "factory")) so the function returns "default" for CLI-wrapped factory flows.
In `@apps/desktop/src/renderer/lib/modelOptions.test.ts`:
- Around line 15-24: The test helper makeStatus now includes droid fields but
the suite lacks a Droid-specific assertion; update the relevant test in
modelOptions.test.ts to include a case where makeStatus returns detectedAuth:
['droid'] and models.droid contains one or more model entries, then assert that
the code under test (the function that builds model options) includes those
Droid model ids/options in its output (use the same assertions pattern as for
claude/codex/cursor to verify provider-specific inclusion). Reference
makeStatus, AiSettingsStatus, availableProviders, detectedAuth, models, and the
droid key when adding this assertion.
In `@apps/desktop/src/renderer/lib/sessions.ts`:
- Around line 52-65: Update the other session-label helpers to recognize the new
"droid-chat" tool type so Droid sessions behave like other chat providers: add
"droid-chat" to the logic in isGenericSessionTitle() and include a corresponding
entry ("Droid") in SHORT_TOOL_TYPE_LABELS (and any similar mappings) so the
optimistic/new session title, compact cards, and summary/goal fallback match
defaultSessionLabel's "Droid chat" behavior; use the exact symbol names
isGenericSessionTitle and SHORT_TOOL_TYPE_LABELS to locate and modify the code.
In `@apps/desktop/src/shared/types/config.ts`:
- Around line 1069-1082: The defaultProvider union for AiChatConfig still
excludes "droid", causing Droid not to be persistable: update the
AiChatConfig.defaultProvider type to include "droid" alongside "codex" |
"claude" | "cursor" | "opencode" | "last_used" and mirror that change in the
other occurrence noted (the second union around the later block), ensuring both
places accept "droid" so the shared IPC/preload/renderer types stay in sync with
AiSettingsStatus which already treats droid as first-class.
---
Nitpick comments:
In `@apps/desktop/src/main/services/chat/droidModelsDiscovery.ts`:
- Around line 46-47: The global cache variable cached (and TTL_MS usage) is
keyed only by time and will return models for the wrong executable when
different droidPath values are used; change the cache to be keyed by the
executable path (e.g., replace cached with a Map<string, { at: number; models:
DroidExecHelpModelRow[] }>) and update every place that reads/writes cached (the
discovery logic that accepts droidPath and the checks that compare TTL_MS) to
lookup/insert by droidPath; ensure when storing you record Date.now() in at and
when reading you validate TTL_MS per-path and fall back to fresh discovery if
expired.
In `@apps/desktop/src/main/services/orchestrator/aiOrchestratorService.ts`:
- Around line 5497-5508: The duplicated executor-kind enum should be
centralized: create a single constant (e.g., ALLOWED_EXECUTOR_KINDS or
ExecutorKindEnum) and reference it from the JSON schema definitions instead of
repeating literals; update the schema entries that use newExecutorKind and
newStep.properties.executorKind (and the other occurrence around the noted
5565-5568 block) to read their enum from that constant so changes to allowed
executors are made in one place.
In `@apps/desktop/src/main/services/orchestrator/orchestratorQueries.ts`:
- Around line 281-297: The normalizeExecutorKind implementation duplicates an
allowlist that's also replicated in the recovery logic, causing drift when
updating executor kinds (e.g., "droid"); extract the canonical list and
normalization into a single shared helper or exported constant used by both
normalizeExecutorKind and the recovery logic so updates are made in one place;
specifically, create an exported constant (e.g., ALLOWED_EXECUTOR_KINDS) and a
single normalize function (e.g., normalizeExecutorKind or
normalizeExecutorKindFromList) and replace the local array/allowlist usages in
normalizeExecutorKind and the recovery code to reference this shared symbol.
In `@apps/desktop/src/renderer/components/settings/ProvidersSection.tsx`:
- Around line 722-751: The Factory Droid card duplicates the existing CLI
runtime card layout used for Claude/Codex/Cursor; extract a reusable component
(e.g., CliRuntimeCard) and replace duplicated JSX in ProvidersSection with that
component. CliRuntimeCard should accept props: tool (from CLI_TOOLS), connection
(providerConnections[tool.cli]), isInitialCheckInFlight, and functions/values
used to compute UI like describeCredentialSource(connection),
getStatusTone(connection), buildCliMessage(tool, connection), and should render
ProviderLogo, status icon logic, credentialSourceDesc, and path exactly once.
Update ProvidersSection to call <CliRuntimeCard .../> for the Droid entry (and
reuse for other CLI entries) so status/messaging logic lives in one place and
you no longer repeat the section JSX.
🪄 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: 7689d5a4-8886-44bc-9034-980f6af1d22f
⛔ Files ignored due to path filters (2)
apps/desktop/package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.jsonapps/desktop/src/renderer/assets/provider-logos/droid.svgis excluded by!**/*.svg
📒 Files selected for processing (69)
apps/ade-cli/src/adeRpcServer.tsapps/ade-cli/src/bootstrap.tsapps/ade-cli/src/cli.tsapps/desktop/package.jsonapps/desktop/src/main/main.tsapps/desktop/src/main/services/ai/agentExecutor.tsapps/desktop/src/main/services/ai/aiIntegrationService.test.tsapps/desktop/src/main/services/ai/aiIntegrationService.tsapps/desktop/src/main/services/ai/authDetector.test.tsapps/desktop/src/main/services/ai/authDetector.tsapps/desktop/src/main/services/ai/droidExecutable.test.tsapps/desktop/src/main/services/ai/droidExecutable.tsapps/desktop/src/main/services/ai/providerConnectionStatus.tsapps/desktop/src/main/services/chat/acpCliPool.tsapps/desktop/src/main/services/chat/acpHostClient.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/chat/cursorAcpPool.tsapps/desktop/src/main/services/chat/droidAcpPool.tsapps/desktop/src/main/services/chat/droidModelsDiscovery.test.tsapps/desktop/src/main/services/chat/droidModelsDiscovery.tsapps/desktop/src/main/services/config/projectConfigService.tsapps/desktop/src/main/services/ipc/registerIpc.tsapps/desktop/src/main/services/orchestrator/aiOrchestratorService.test.tsapps/desktop/src/main/services/orchestrator/aiOrchestratorService.tsapps/desktop/src/main/services/orchestrator/orchestratorQueries.tsapps/desktop/src/main/services/orchestrator/orchestratorService.tsapps/desktop/src/main/services/orchestrator/permissionMapping.tsapps/desktop/src/main/services/orchestrator/providerOrchestratorAdapter.tsapps/desktop/src/main/services/orchestrator/recoveryService.tsapps/desktop/src/main/services/orchestrator/workerDeliveryService.tsapps/desktop/src/main/services/orchestrator/workerTracking.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/shared/utils.test.tsapps/desktop/src/main/services/shared/utils.tsapps/desktop/src/main/services/sync/syncRemoteCommandService.tsapps/desktop/src/renderer/assets/provider-logos/README.mdapps/desktop/src/renderer/browserMock.tsapps/desktop/src/renderer/components/chat/AgentChatComposer.test.tsxapps/desktop/src/renderer/components/chat/AgentChatComposer.tsxapps/desktop/src/renderer/components/chat/AgentChatMessageList.tsxapps/desktop/src/renderer/components/chat/AgentChatPane.tsxapps/desktop/src/renderer/components/missions/CreateMissionDialog.tsxapps/desktop/src/renderer/components/missions/WorkerPermissionsEditor.tsxapps/desktop/src/renderer/components/missions/useMissionsStore.tsapps/desktop/src/renderer/components/settings/ProvidersSection.test.tsxapps/desktop/src/renderer/components/settings/ProvidersSection.tsxapps/desktop/src/renderer/components/settings/UsageGuardrailsSection.tsxapps/desktop/src/renderer/components/shared/ModelCatalogPanel.tsxapps/desktop/src/renderer/components/shared/ProviderLogos.tsxapps/desktop/src/renderer/components/shared/permissionOptions.test.tsapps/desktop/src/renderer/components/shared/permissionOptions.tsapps/desktop/src/renderer/components/shared/providerModelSelectorGrouping.test.tsapps/desktop/src/renderer/components/shared/providerModelSelectorGrouping.tsapps/desktop/src/renderer/components/terminals/ToolLogos.tsxapps/desktop/src/renderer/lib/modelOptions.test.tsapps/desktop/src/renderer/lib/modelOptions.tsapps/desktop/src/renderer/lib/sessions.tsapps/desktop/src/shared/modelRegistry.test.tsapps/desktop/src/shared/modelRegistry.tsapps/desktop/src/shared/types/chat.tsapps/desktop/src/shared/types/config.tsapps/desktop/src/shared/types/missions.tsapps/desktop/src/shared/types/models.tsapps/desktop/src/shared/types/orchestrator.tsapps/desktop/src/shared/types/sessions.ts
Comment on lines
+274
to
+282
| async killTerminal(params: KillTerminalRequest): Promise<void> { | ||
| const t = terminals.get(params.terminalId); | ||
| if (t && !t.exited) { | ||
| try { | ||
| signalChildProcessTree(t.proc, "SIGTERM"); | ||
| } catch { | ||
| // ignore | ||
| } | ||
| } |
There was a problem hiding this comment.
killTerminal() does not actually guarantee termination.
This only sends SIGTERM once. Any command that traps or ignores SIGTERM will keep running until some later waitForTerminalExit() or releaseTerminal() path happens to clean it up, which makes killTerminal() unreliable for stubborn terminals.
🛠️ Suggested direction
async killTerminal(params: KillTerminalRequest): Promise<void> {
const t = terminals.get(params.terminalId);
if (t && !t.exited) {
try {
- signalChildProcessTree(t.proc, "SIGTERM");
+ // Store the returned timer on terminal state and clear it from the
+ // close handler so SIGTERM-resistant processes are force-killed.
+ t.killTimer = terminateChildProcessTree(t.proc, t.killTimer ?? null, 1_500);
} catch {
// ignore
}
}
},You'd also want to add killTimer?: NodeJS.Timeout | null to AcpHostTermState and clear it in the terminal close handler.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/chat/acpHostClient.ts` around lines 274 - 282,
killTerminal currently only sends a one-time SIGTERM (via
signalChildProcessTree) and can leave processes running if they ignore SIGTERM;
update AcpHostTermState to include killTimer?: NodeJS.Timeout | null, then in
killTerminal set a timer (e.g., 5s) that will call signalChildProcessTree(...,
"SIGKILL") if the process hasn't exited, storing the timer on the terminal
state; ensure you clear and null out killTimer when the terminal's close/exit
handler runs (and when releaseTerminal or waitForTerminalExit cleans up) so
timers don't leak or double-kill, and guard against scheduling multiple timers
if killTerminal is called repeatedly.
Comment on lines
552
to
553
| if ("droidPermissionMode" in value) parsed.droidPermissionMode = value.droidPermissionMode == null ? undefined : asTrimmedString(value.droidPermissionMode) as AgentChatCreateArgs["droidPermissionMode"]; | ||
| if ("cursorModeId" in value) parsed.cursorModeId = value.cursorModeId == null ? null : asTrimmedString(value.cursorModeId) ?? null; |
There was a problem hiding this comment.
Normalize empty droidPermissionMode to undefined (not null).
Line 552 and Line 677 can assign null at runtime (from asTrimmedString) even though the logic intends nullish → undefined. This can leak unexpected null into downstream session/update paths.
🔧 Proposed fix
- if ("droidPermissionMode" in value) parsed.droidPermissionMode = value.droidPermissionMode == null ? undefined : asTrimmedString(value.droidPermissionMode) as AgentChatCreateArgs["droidPermissionMode"];
+ if ("droidPermissionMode" in value) {
+ const droidPermissionMode = asTrimmedString(value.droidPermissionMode);
+ parsed.droidPermissionMode = droidPermissionMode == null
+ ? undefined
+ : droidPermissionMode as AgentChatCreateArgs["droidPermissionMode"];
+ }
- if ("droidPermissionMode" in value) parsed.droidPermissionMode = value.droidPermissionMode == null ? undefined : asTrimmedString(value.droidPermissionMode) as AgentChatUpdateSessionArgs["droidPermissionMode"];
+ if ("droidPermissionMode" in value) {
+ const droidPermissionMode = asTrimmedString(value.droidPermissionMode);
+ parsed.droidPermissionMode = droidPermissionMode == null
+ ? undefined
+ : droidPermissionMode as AgentChatUpdateSessionArgs["droidPermissionMode"];
+ }Also applies to: 677-678
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/sync/syncRemoteCommandService.ts` around lines
552 - 553, The current parsing logic can set parsed.droidPermissionMode to null
when asTrimmedString returns null; change both occurrences (the assignment for
parsed.droidPermissionMode at the first block and the duplicate at lines
~677-678) so that when value.droidPermissionMode is null/undefined you set
parsed.droidPermissionMode = undefined, and when asTrimmedString(...) returns
null/empty you also map that to undefined (e.g. capture the trimmed result and
use (trimmed ?? undefined) cast to AgentChatCreateArgs["droidPermissionMode"]);
keep the existing cursorModeId behavior intact if desired but apply the same
null→undefined normalization for any other droidPermissionMode assignments.
…dress 22 review comments CI: authDetector.test.ts 'treats droid exec list-tools as a valid authenticated probe' was failing because the test mocks did not cover the new droid resolution path; fixed mock setup so installed/verified resolve. Review: 22 actionable comments addressed across Greptile (3 P1/P2), Capy (1), and CodeRabbit (18) — covers async readFileSync, concurrency guard, dead code, ACP pool races, droid auth gating, permission propagation, terminal kill escalation, generic adapter fallback, browserMock helpers, AgentChatPane legacy autonomy, UsageGuardrails droid signal, providerModelSelectorGrouping factory metadata. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
@copilot review but do not make fixes |
Copilot stopped work on behalf of
arul28 due to an error
April 28, 2026 16:16
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
Improvements
Greptile Summary
This PR adds Factory Droid CLI as a first-class AI provider, wiring it through auth detection, model discovery, ACP connection pooling, permission modes, and the full chat turn lifecycle alongside the existing Claude/Codex/Cursor/OpenCode providers. The
cursorAcpPoolsubprocess management is extracted into a reusableacpCliPoolabstraction shared by both Cursor and Droid, andutils.tsgains cross-platform SIGTERM→SIGKILL process-tree escalation helpers.Confidence Score: 5/5
Safe to merge; all findings are P2 (minor memory leak and dedup threshold quality concern)
No P0 or P1 issues found. The two P2s (stale droidSessionDedup entry on pool-key swap and the 32-char replay dedup threshold) don't affect correctness in the common path — the leak is bounded to one Map entry per model-switch and the dedup false-positive requires an exact substring match in history text.
apps/desktop/src/main/services/chat/agentChatService.ts — droidSessionDedup cleanup in ensureDroidRuntime pool-key-change branch and the isDuplicateDroidNotification replay threshold
Important Files Changed
Sequence Diagram
sequenceDiagram participant UI as Renderer participant ACS as agentChatService participant DAP as droidAcpPool participant ACP as acpCliPool participant DCLI as droid CLI (subprocess) participant DMD as droidModelsDiscovery participant AD as authDetector UI->>ACS: sendMessage (provider=droid) ACS->>AD: detectAuth() AD-->>ACS: DetectedAuth (cli-subscription droid or FACTORY_API_KEY) ACS->>DMD: discoverDroidCliModelDescriptors(droidPath) DMD->>DCLI: droid exec --help DCLI-->>DMD: model list stdout DMD-->>ACS: ModelDescriptor[] ACS->>ACS: ensureDroidRuntime(managed) ACS->>DAP: acquireDroidAcpConnection(poolKey, modelId, launchSettings) DAP->>ACP: acquireAcpCliConnection(options) ACP->>DCLI: spawn droid exec --output-format acp --auto medium DCLI-->>ACP: ACP initialize handshake ACP-->>DAP: AcpCliPooled DAP-->>ACS: DroidAcpPooled + generation ACS->>DCLI: newSession / resumeSession (ACP) DCLI-->>ACS: sessionId ACS->>DCLI: sendMessage (ACP turn) loop streaming chunks DCLI-->>ACS: agent_message_chunk ACS->>ACS: isDuplicateDroidNotification? ACS-->>UI: chat_event (text delta) end DCLI-->>ACS: turn_complete ACS-->>UI: chat_event (done)Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "ship: iteration 1 — fix test-desktop (5)..." | Re-trigger Greptile