Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds desktop runtime packaging hooks, runtime-binary normalization, packaged-runtime validation and smoke tests, a stdio↔socket MCP proxy with identity injection, centralized MCP launch resolution, and related tests and build/CI adjustments. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
arul28
force-pushed
the
ade/fix-all-other-chats-b9074194
branch
from
72a1c00 to
7812240
Compare
arul28
force-pushed
the
ade/fix-all-other-chats-b9074194
branch
from
7812240 to
8f54076
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/main/services/ai/claudeRuntimeProbe.test.ts (2)
28-43:⚠️ Potential issue | 🟡 MinorDuplicate mock for
./claudeCodeExecutablewill cause unexpected behavior.There are two
vi.mock("./claudeCodeExecutable", ...)calls (lines 28-33 and 41-43). Vitest will use the last mock definition, meaning the first mock returning"/mock/bin/claude"is overridden by the second mock returning"/usr/local/bin/claude".The assertion at lines 103-110 expects
pathToClaudeCodeExecutable: "/mock/bin/claude", but this will fail because the active mock returns"/usr/local/bin/claude".Remove the first mock (lines 28-33) since it's overridden anyway, or consolidate into a single mock definition.
🐛 Proposed fix to remove duplicate mock
-vi.mock("./claudeCodeExecutable", () => ({ - resolveClaudeCodeExecutable: () => ({ - path: "/mock/bin/claude", - source: "auth", - }), -})); - vi.mock("./providerRuntimeHealth", () => ({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/claudeRuntimeProbe.test.ts` around lines 28 - 43, The test file defines vi.mock("./claudeCodeExecutable") twice which causes the first mock (a literal resolveClaudeCodeExecutable returning {path:"/mock/bin/claude"}) to be overridden by the later mock (mockState.resolveClaudeCodeExecutable), breaking assertions that expect "/mock/bin/claude"; fix by removing the earlier vi.mock("./claudeCodeExecutable", () => ({ resolveClaudeCodeExecutable: () => ({ path: "/mock/bin/claude", source: "auth" }) })) or merge its behavior into the single vi.mock call so only one mock of resolveClaudeCodeExecutable exists (use either the literal return or mockState.resolveClaudeCodeExecutable consistently) and update any expectations if you intentionally change the returned path.
103-121:⚠️ Potential issue | 🟡 MinorAssertion conflict due to duplicate expectations.
The test has two assertions for
pathToClaudeCodeExecutable:
- Lines 105: expects
"/mock/bin/claude"- Lines 116: expects
"/usr/local/bin/claude"Both assertions use
expect(mockState.query).toHaveBeenCalledWith(...)on the same call. Given thatmockState.queryis only called once in this test (line 102 confirms the stream was closed after one call), one of these assertions will fail because the same call cannot have two different values for the same property.This appears to be a copy-paste error where the second assertion block (lines 113-121) duplicates the first (lines 103-110) but with conflicting values.
🐛 Proposed fix - remove duplicate assertion
expect(mockState.query).toHaveBeenCalledWith(expect.objectContaining({ options: expect.objectContaining({ - pathToClaudeCodeExecutable: "/mock/bin/claude", + cwd: "/tmp/project", + pathToClaudeCodeExecutable: "/usr/local/bin/claude", mcpServers: expect.objectContaining({ ade: expect.any(Object), }), }), })); expect(mockState.reportProviderRuntimeAuthFailure).toHaveBeenCalledTimes(1); expect(mockState.reportProviderRuntimeFailure).not.toHaveBeenCalled(); - expect(mockState.query).toHaveBeenCalledWith(expect.objectContaining({ - options: expect.objectContaining({ - cwd: "/tmp/project", - pathToClaudeCodeExecutable: "/usr/local/bin/claude", - mcpServers: expect.objectContaining({ - ade: expect.any(Object), - }), - }), - })); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/ai/claudeRuntimeProbe.test.ts` around lines 103 - 121, The two conflicting assertions against mockState.query (one expecting pathToClaudeCodeExecutable "/mock/bin/claude" in the first expect.objectContaining block and another expecting "/usr/local/bin/claude" in the second) are a duplicate/copy-paste error because mockState.query is only called once in this test; remove the second expect(mockState.query).toHaveBeenCalledWith(...) block (the one asserting cwd "/tmp/project" and pathToClaudeCodeExecutable "/usr/local/bin/claude") so the test only asserts the single expected call, or if the intent was to assert a second call, change the assertion to target the second call explicitly (e.g., use mockState.query.mock.calls[1] or toHaveBeenNthCalledWith) and keep the differing expected values accordingly.
🧹 Nitpick comments (2)
apps/desktop/tsconfig.json (1)
7-10: Remove unusedpathsalias foraiThe
"ai": ["./node_modules/ai"]alias at line 9 is not used anywhere in the codebase. Since there are no imports of theaipackage found in the project, this configuration is unnecessary and should be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/tsconfig.json` around lines 7 - 10, Remove the unused TypeScript path alias "ai": ["./node_modules/ai"] from the tsconfig.json "paths" section; locate the "paths" entry that contains the "ai" mapping and delete that key/value pair so the compiler config no longer contains the unnecessary "ai" alias.apps/desktop/src/main/services/runtime/adeMcpLaunch.test.ts (1)
16-109: Add aheadless_sourcecase.The resolver now has three modes, but this suite never forces
apps/mcp-server/dist/index.cjsto be absent. A brokennpx tsxfallback would still pass here. Based on learnings: For ADE MCP changes, verify both headless MCP mode and the desktop socket-backed MCP path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/services/runtime/adeMcpLaunch.test.ts` around lines 16 - 109, Add a test that forces the "headless_source" path by ensuring the built entry (apps/mcp-server/dist/index.cjs) does not exist and creating the source entry (apps/mcp-server/src/index.ts); call resolveDesktopAdeMcpLaunch with preferBundledProxy: false (and appropriate projectRoot/runtimeRoot/workspaceRoot), then assert launch.mode === "headless_source" and that the resolved entry path/cmdArgs reference the source file (e.g., that cmdArgs[0] or entryPath points to apps/mcp-server/src/index.ts) to ensure the resolver chooses the source fallback rather than a broken npx/tsx built fallback.
🤖 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/desktop/scripts/runtimeBinaryPermissions.cjs`:
- Around line 55-60: The collector currently only adds prebuilds/*/spawn-helper
to candidates (in the loop using listDirectories and path.join(rootPath,
"node_modules", "node-pty", "prebuilds")), so spawn-helper under build/Release
or build/Debug will be missed and stay non-executable; update the collector to
also push candidates for path.join(prebuildDir, "build", "Release",
"spawn-helper") and path.join(prebuildDir, "build", "Debug", "spawn-helper") (or
search for those subpaths relative to node-pty entries) so the chmod logic that
iterates candidates will include both prebuild and build-layout spawn-helper
files.
In `@apps/desktop/src/main/adeMcpProxy.ts`:
- Around line 184-187: Remove the immediate process.exit(0) calls from the
process.stdin "end" handlers so the socket close drives shutdown: in the
handlers that currently call socket.end(); process.exit(0); (around the
process.stdin.on("end") blocks and the similar block at 211-217) only call
socket.end() and let the existing socket.on("close") handler perform the final
process.exit. This ensures the socket can flush buffered writes and receive the
server's final response before the process exits.
In `@apps/desktop/src/main/packagedRuntimeSmoke.ts`:
- Around line 155-168: The current probe only calls
execFileAsync(launch.command, [...launch.cmdArgs, "--probe"]) and never
exercises the desktop socket bridge (so
net.createConnection()/relayProxyInputWithIdentity() paths aren't tested);
update the packagedRuntimeSmoke probe to spawn a temporary Unix socket server,
start the proxy process using resolveDesktopAdeMcpLaunch/execFileAsync with the
desktop socket env, send one real "initialize" message through the socket and
assert a correct round-trip response, and also keep/cover the existing headless
(--probe) check to validate both headless MCP mode and the desktop socket-backed
MCP path; locate code around resolveDesktopAdeMcpLaunch, execFileAsync, and the
probe usage to implement the socket server, message send/receive, and cleanup.
In `@apps/desktop/src/main/services/chat/agentChatService.ts`:
- Around line 5188-5196: The warning handler currently calls
buildClaudeV2SessionOpts(managed, runtime) and summarizeAdeMcpLaunch(...) inside
the warmup catch which can rethrow if runtime.v2Session is absent; precompute
any diagnostic values (claudeExecutablePath and adeMcpLaunch summary) before
entering the warmup try/catch using managed and runtime, or wrap each diagnostic
call in its own small try/catch and fall back to safe defaults, then use those
captured values in logger.warn and allow the subsequent cleanup logic to always
run without being masked by a secondary exception; focus changes around
logger.warn, buildClaudeV2SessionOpts, summarizeAdeMcpLaunch, managed, and
runtime.v2Session.
- Around line 4695-4705: The call to summarizeAdeMcpLaunch is currently executed
inline inside the logger.info hot path and can throw (blocking spawn("codex",
["app-server"])); make the MCP summary best-effort by wrapping the
summarizeAdeMcpLaunch(...) call in a try/catch (or compute it before the logger
and fall back to undefined/null on error) so
logger.info("agent_chat.codex_runtime_start", { ... }) always runs and Codex is
spawned regardless of runtime-root resolution failures; reference
summarizeAdeMcpLaunch, logger.info, spawn("codex", ["app-server"]), and
managed.session when implementing the defensive try/catch and default value.
- Around line 4817-4844: The proc.on("error") handler drops the promise returned
by finishSession(managed, "failed", { ... }) which can cause an unhandled
rejection if teardown/persistence fails; update the error handler to handle the
returned promise (e.g., call finishSession(...).catch(() => {}) or await it
inside an async wrapper) so any rejection is swallowed/handled, and ensure this
mirrors the safeguard used in the exit handler; keep the rest of the existing
logic (logging, pending rejection, emitChatEvent, runtime updates) intact and
reference finishSession(managed, "failed", ...) and proc.on("error") when
applying the change.
In
`@apps/desktop/src/main/services/orchestrator/unifiedOrchestratorAdapter.test.ts`:
- Around line 49-90: The test for resolveAdeMcpServerLaunch only exercises one
branch because preferBundledProxy is hardcoded and the later assertions depend
on the environment; update unifiedOrchestratorAdapter.test.ts to
deterministically cover both MCP modes by adding two sub-tests or a
parameterized loop that calls resolveAdeMcpServerLaunch with preferBundledProxy:
false and preferBundledProxy: true (or by stubbing any environment detection
function the resolver uses), and for each case assert the expected launch shape:
when preferBundledProxy is false assert node + dist entry and ADE_* env entries,
and when true assert the bundled-proxy/socket-backed launch shape (command,
args, and ADE_MCP_SOCKET_PATH); apply the same change pattern to the other test
block referenced (around the 425-443 region) so both headless and desktop
socket-backed paths are deterministically tested.
---
Outside diff comments:
In `@apps/desktop/src/main/services/ai/claudeRuntimeProbe.test.ts`:
- Around line 28-43: The test file defines vi.mock("./claudeCodeExecutable")
twice which causes the first mock (a literal resolveClaudeCodeExecutable
returning {path:"/mock/bin/claude"}) to be overridden by the later mock
(mockState.resolveClaudeCodeExecutable), breaking assertions that expect
"/mock/bin/claude"; fix by removing the earlier
vi.mock("./claudeCodeExecutable", () => ({ resolveClaudeCodeExecutable: () => ({
path: "/mock/bin/claude", source: "auth" }) })) or merge its behavior into the
single vi.mock call so only one mock of resolveClaudeCodeExecutable exists (use
either the literal return or mockState.resolveClaudeCodeExecutable consistently)
and update any expectations if you intentionally change the returned path.
- Around line 103-121: The two conflicting assertions against mockState.query
(one expecting pathToClaudeCodeExecutable "/mock/bin/claude" in the first
expect.objectContaining block and another expecting "/usr/local/bin/claude" in
the second) are a duplicate/copy-paste error because mockState.query is only
called once in this test; remove the second
expect(mockState.query).toHaveBeenCalledWith(...) block (the one asserting cwd
"/tmp/project" and pathToClaudeCodeExecutable "/usr/local/bin/claude") so the
test only asserts the single expected call, or if the intent was to assert a
second call, change the assertion to target the second call explicitly (e.g.,
use mockState.query.mock.calls[1] or toHaveBeenNthCalledWith) and keep the
differing expected values accordingly.
---
Nitpick comments:
In `@apps/desktop/src/main/services/runtime/adeMcpLaunch.test.ts`:
- Around line 16-109: Add a test that forces the "headless_source" path by
ensuring the built entry (apps/mcp-server/dist/index.cjs) does not exist and
creating the source entry (apps/mcp-server/src/index.ts); call
resolveDesktopAdeMcpLaunch with preferBundledProxy: false (and appropriate
projectRoot/runtimeRoot/workspaceRoot), then assert launch.mode ===
"headless_source" and that the resolved entry path/cmdArgs reference the source
file (e.g., that cmdArgs[0] or entryPath points to apps/mcp-server/src/index.ts)
to ensure the resolver chooses the source fallback rather than a broken npx/tsx
built fallback.
In `@apps/desktop/tsconfig.json`:
- Around line 7-10: Remove the unused TypeScript path alias "ai":
["./node_modules/ai"] from the tsconfig.json "paths" section; locate the "paths"
entry that contains the "ai" mapping and delete that key/value pair so the
compiler config no longer contains the unnecessary "ai" alias.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 35abca4c-ab90-4849-8116-40da253ff241
📒 Files selected for processing (24)
.github/workflows/release-core.ymlapps/desktop/package.jsonapps/desktop/scripts/after-pack-runtime-fixes.cjsapps/desktop/scripts/normalize-runtime-binaries.cjsapps/desktop/scripts/prepare-universal-mac-inputs.mjsapps/desktop/scripts/release-mac-local.mjsapps/desktop/scripts/require-macos-release-secrets.cjsapps/desktop/scripts/runtimeBinaryPermissions.cjsapps/desktop/scripts/validate-mac-artifacts.mjsapps/desktop/src/main/adeMcpProxy.tsapps/desktop/src/main/main.tsapps/desktop/src/main/packagedRuntimeSmoke.tsapps/desktop/src/main/services/ai/claudeRuntimeProbe.test.tsapps/desktop/src/main/services/ai/claudeRuntimeProbe.tsapps/desktop/src/main/services/ai/providerConnectionStatus.test.tsapps/desktop/src/main/services/chat/agentChatService.tsapps/desktop/src/main/services/orchestrator/unifiedOrchestratorAdapter.test.tsapps/desktop/src/main/services/orchestrator/unifiedOrchestratorAdapter.tsapps/desktop/src/main/services/processes/processService.tsapps/desktop/src/main/services/runtime/adeMcpLaunch.test.tsapps/desktop/src/main/services/runtime/adeMcpLaunch.tsapps/desktop/src/renderer/components/settings/ProvidersSection.test.tsxapps/desktop/tsconfig.jsonapps/desktop/tsup.config.ts
Comment on lines
+155
to
+168
| const launch = resolveDesktopAdeMcpLaunch({ | ||
| projectRoot: cwd, | ||
| workspaceRoot: cwd, | ||
| }); | ||
| const ptyProbe = await probePty(); | ||
| const claudeStartup = await probeClaudeStartup(claudeExecutable.path); | ||
|
|
||
| const proxyProbe = await execFileAsync(launch.command, [...launch.cmdArgs, "--probe"], { | ||
| cwd, | ||
| env: { | ||
| ...process.env, | ||
| ...launch.env, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
--probe never exercises the real desktop socket bridge.
With projectRoot/workspaceRoot set to cwd and the launch invoked as ... --probe, this only validates command assembly. adeMcpProxy --probe exits before net.createConnection() and relayProxyInputWithIdentity() run, so a broken socket relay or identity injection path would still let the packaged smoke pass. Consider spinning up a temporary Unix socket server and round-tripping one initialize message through the proxy here. Based on learnings: For ADE MCP changes, verify both headless MCP mode and the desktop socket-backed MCP path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/packagedRuntimeSmoke.ts` around lines 155 - 168, The
current probe only calls execFileAsync(launch.command, [...launch.cmdArgs,
"--probe"]) and never exercises the desktop socket bridge (so
net.createConnection()/relayProxyInputWithIdentity() paths aren't tested);
update the packagedRuntimeSmoke probe to spawn a temporary Unix socket server,
start the proxy process using resolveDesktopAdeMcpLaunch/execFileAsync with the
desktop socket env, send one real "initialize" message through the socket and
assert a correct round-trip response, and also keep/cover the existing headless
(--probe) check to validate both headless MCP mode and the desktop socket-backed
MCP path; locate code around resolveDesktopAdeMcpLaunch, execFileAsync, and the
probe usage to implement the socket server, message send/receive, and cleanup.
Comment on lines
+49
to
+90
| describe("resolveAdeMcpServerLaunch", () => { | ||
| it("prefers the packaged dist entry when the built MCP server exists", () => { | ||
| const runtimeRoot = fs.mkdtempSync(path.join(os.tmpdir(), "ade-mcp-runtime-")); | ||
| const projectRoot = fs.mkdtempSync(path.join(os.tmpdir(), "ade-mcp-project-")); | ||
| const workspaceRoot = path.join(projectRoot, "workspace"); | ||
| const builtEntry = path.join(runtimeRoot, "apps", "mcp-server", "dist", "index.cjs"); | ||
|
|
||
| fs.mkdirSync(path.dirname(builtEntry), { recursive: true }); | ||
| fs.mkdirSync(workspaceRoot, { recursive: true }); | ||
| fs.writeFileSync(builtEntry, "module.exports = {};\n", "utf8"); | ||
|
|
||
| const launch = resolveAdeMcpServerLaunch({ | ||
| projectRoot, | ||
| workspaceRoot, | ||
| runtimeRoot, | ||
| preferBundledProxy: false, | ||
| missionId: "mission-123", | ||
| runId: "run-456", | ||
| stepId: "step-789", | ||
| attemptId: "attempt-000", | ||
| defaultRole: "external", | ||
| }); | ||
|
|
||
| expect(launch.command).toBe("node"); | ||
| expect(launch.cmdArgs).toEqual([ | ||
| builtEntry, | ||
| "--project-root", | ||
| path.resolve(projectRoot), | ||
| "--workspace-root", | ||
| path.resolve(workspaceRoot), | ||
| ]); | ||
| expect(launch.env).toMatchObject({ | ||
| ADE_PROJECT_ROOT: path.resolve(projectRoot), | ||
| ADE_WORKSPACE_ROOT: path.resolve(workspaceRoot), | ||
| ADE_MCP_SOCKET_PATH: path.join(path.resolve(projectRoot), ".ade", "mcp.sock"), | ||
| ADE_MISSION_ID: "mission-123", | ||
| ADE_RUN_ID: "run-456", | ||
| ADE_STEP_ID: "step-789", | ||
| ADE_ATTEMPT_ID: "attempt-000", | ||
| ADE_DEFAULT_ROLE: "external", | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Cover both MCP launch modes with deterministic tests.
The new resolver test hardcodes preferBundledProxy: false, and the later if/else only asserts whichever launch mode the current environment happens to pick. That leaves either the headless path or the socket-backed bundled-proxy path untested in any given run, so one can regress without failing CI.
Based on learnings: For ADE MCP changes, verify both headless MCP mode and the desktop socket-backed MCP path.
Also applies to: 425-443
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/main/services/orchestrator/unifiedOrchestratorAdapter.test.ts`
around lines 49 - 90, The test for resolveAdeMcpServerLaunch only exercises one
branch because preferBundledProxy is hardcoded and the later assertions depend
on the environment; update unifiedOrchestratorAdapter.test.ts to
deterministically cover both MCP modes by adding two sub-tests or a
parameterized loop that calls resolveAdeMcpServerLaunch with preferBundledProxy:
false and preferBundledProxy: true (or by stubbing any environment detection
function the resolver uses), and for each case assert the expected launch shape:
when preferBundledProxy is false assert node + dist entry and ADE_* env entries,
and when true assert the bundled-proxy/socket-backed launch shape (command,
args, and ADE_MCP_SOCKET_PATH); apply the same change pattern to the other test
block referenced (around the 425-443 region) so both headless and desktop
socket-backed paths are deterministically tested.
- Extract adeMcpProxy parsing utilities into testable module (52 tests) - Fix duplicate vi.mock in claudeRuntimeProbe.test.ts causing stale assertion - Add missing mocks for resolveUnifiedRuntimeRoot and resolveClaudeCodeExecutable in agentChatService.test.ts - Simplify identity injection loop, runtime root resolution, and resource path helpers - Fix stale JSDoc on resolveUnifiedRuntimeRoot, inline single-use variable in processService - Update docs for MCP launch modes, build entry points, runtime hardening, and diagnostic logging Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/desktop/src/main/services/ai/claudeRuntimeProbe.ts`:
- Around line 91-95: The probe is launching the ADE MCP with defaultRole:
"external" in resolveDesktopAdeMcpLaunch, but the production agent uses
defaultRole: "agent" (see the agent launch in unifiedOrchestratorAdapter at the
agent startup); update the probe call to use defaultRole: "agent" so the health
check exercises the same runtime role the agent will use (or add an inline
comment documenting an intentional divergence if "external" is deliberate).
In `@apps/desktop/src/main/services/orchestrator/unifiedOrchestratorAdapter.ts`:
- Around line 83-96: The unifiedOrchestratorAdapter accepts chatSessionId but
drops it when calling resolveDesktopAdeMcpLaunch because AdeMcpLaunchArgs
doesn't include it; update the AdeMcpLaunchArgs type (and any place constructing
that type) to include chatSessionId, then forward args.chatSessionId in the
object passed to resolveDesktopAdeMcpLaunch (alongside projectRoot,
workspaceRoot, etc.), and run/adjust any callers (e.g., agentChatService.ts) and
tests to accommodate the new field so session tracking is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c38217c8-4fe9-4cb4-8fdc-ef8ee8f67902
⛔ Files ignored due to path filters (4)
docs/architecture/AI_INTEGRATION.mdis excluded by!docs/**docs/architecture/DESKTOP_APP.mdis excluded by!docs/**docs/architecture/SYSTEM_OVERVIEW.mdis excluded by!docs/**docs/features/CHAT.mdis excluded by!docs/**
📒 Files selected for processing (9)
apps/desktop/src/main/adeMcpProxy.tsapps/desktop/src/main/adeMcpProxyUtils.test.tsapps/desktop/src/main/adeMcpProxyUtils.tsapps/desktop/src/main/services/ai/claudeRuntimeProbe.test.tsapps/desktop/src/main/services/ai/claudeRuntimeProbe.tsapps/desktop/src/main/services/chat/agentChatService.test.tsapps/desktop/src/main/services/orchestrator/unifiedOrchestratorAdapter.tsapps/desktop/src/main/services/processes/processService.tsapps/desktop/src/main/services/runtime/adeMcpLaunch.ts
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/main/services/processes/processService.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/main/services/ai/claudeRuntimeProbe.test.ts
- apps/desktop/src/main/adeMcpProxy.ts
- apps/desktop/src/main/services/runtime/adeMcpLaunch.ts
Comment on lines
+91
to
+95
| const launch = resolveDesktopAdeMcpLaunch({ | ||
| projectRoot, | ||
| workspaceRoot: projectRoot, | ||
| runtimeRoot: resolveProbeRuntimeRoot(), | ||
| defaultRole: "agent", | ||
| runtimeRoot: resolveRepoRuntimeRoot(), | ||
| defaultRole: "external", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "ADE MCP launch call sites:"
rg -n -C3 '\bresolveDesktopAdeMcpLaunch\s*\(' apps
echo
echo "Explicit defaultRole wiring:"
rg -n -C2 'defaultRole\s*:\s*"(agent|external)"' apps
echo
echo "Role consumers:"
rg -n -C2 '\bADE_DEFAULT_ROLE\b' appsRepository: arul28/ADE
Length of output: 16640
🏁 Script executed:
# Check if there's documentation or tests explaining the role choice
rg -n -B5 -A5 'external.*role|role.*external' apps/desktop/src/main/services/ai/claudeRuntimeProbe.ts
# Check the orchestrator path to confirm it's the main production Claude path
rg -n 'unifiedOrchestratorAdapter' apps/desktop/src/main/services/chat --type ts -A3 -B3Repository: arul28/ADE
Length of output: 1662
🏁 Script executed:
# Check the context around the probe's defaultRole choice and any comments
cat -n apps/desktop/src/main/services/ai/claudeRuntimeProbe.ts | sed -n '80,105p'
# Also check if there's any explicit reasoning in git history or comments
rg -n -B10 'function resolveProbeMcpServers' apps/desktop/src/main/services/ai/claudeRuntimeProbe.tsRepository: arul28/ADE
Length of output: 1378
Probe ADE role should match the production Claude agent path.
Line 95 sets the health check probe to use defaultRole: "external", but the actual production agent launch path in unifiedOrchestratorAdapter.ts:137 uses defaultRole: "agent". Since the probe validates the MCP runtime environment before the agent runs, it should use the same role to ensure it tests the exact conditions the agent will experience. Change line 95 to defaultRole: "agent" or add documentation if "external" is intentionally different.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/ai/claudeRuntimeProbe.ts` around lines 91 -
95, The probe is launching the ADE MCP with defaultRole: "external" in
resolveDesktopAdeMcpLaunch, but the production agent uses defaultRole: "agent"
(see the agent launch in unifiedOrchestratorAdapter at the agent startup);
update the probe call to use defaultRole: "agent" so the health check exercises
the same runtime role the agent will use (or add an inline comment documenting
an intentional divergence if "external" is deliberate).
Comment on lines
+83
to
+96
| return resolveDesktopAdeMcpLaunch({ | ||
| projectRoot: args.projectRoot, | ||
| workspaceRoot: args.workspaceRoot, | ||
| runtimeRoot: args.runtimeRoot, | ||
| missionId: args.missionId, | ||
| runId: args.runId, | ||
| stepId: args.stepId, | ||
| attemptId: args.attemptId, | ||
| defaultRole: args.defaultRole, | ||
| ownerId: args.ownerId, | ||
| computerUsePolicy: args.computerUsePolicy, | ||
| bundledProxyPath: args.bundledProxyPath, | ||
| preferBundledProxy: args.preferBundledProxy, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if resolveDesktopAdeMcpLaunch supports chatSessionId parameter
# Find the function signature in the target module
ast-grep --pattern $'function resolveDesktopAdeMcpLaunch($$$) {
$$$
}'
# Also check the type definition for the args parameter
rg -n -A 20 'resolveDesktopAdeMcpLaunch' apps/desktop/src/main/services/runtime/adeMcpLaunch.ts | head -50Repository: arul28/ADE
Length of output: 1040
🏁 Script executed:
# Find resolveAdeMcpServerLaunch function signature
rg -n -B 5 -A 30 'resolveAdeMcpServerLaunch' apps/desktop/src/main/services/orchestrator/unifiedOrchestratorAdapter.ts | head -80Repository: arul28/ADE
Length of output: 2481
🏁 Script executed:
# Find AdeMcpLaunchArgs type definition
rg -n -B 2 -A 15 'type AdeMcpLaunchArgs\|interface AdeMcpLaunchArgs' apps/desktop/src/main/services/runtime/adeMcpLaunch.tsRepository: arul28/ADE
Length of output: 36
🏁 Script executed:
# Check if chatSessionId is used anywhere in the adeMcpLaunch module
rg -n 'chatSessionId' apps/desktop/src/main/services/runtime/adeMcpLaunch.tsRepository: arul28/ADE
Length of output: 36
🏁 Script executed:
# Find the type definition - it might be inline or in a different location
rg -n 'AdeMcpLaunchArgs' apps/desktop/src/main/services/Repository: arul28/ADE
Length of output: 279
🏁 Script executed:
# Check agentChatService.ts to see if it passes chatSessionId
rg -n -B 5 -A 10 'resolveAdeMcpServerLaunch' apps/desktop/src/main/services/orchestrator/agentChatService.tsRepository: arul28/ADE
Length of output: 157
🏁 Script executed:
# View more of resolveDesktopAdeMcpLaunch to see the full function and what it uses
rg -n -A 50 'export function resolveDesktopAdeMcpLaunch' apps/desktop/src/main/services/runtime/adeMcpLaunch.tsRepository: arul28/ADE
Length of output: 2136
🏁 Script executed:
# View the AdeMcpLaunchArgs type definition
sed -n '20,50p' apps/desktop/src/main/services/runtime/adeMcpLaunch.tsRepository: arul28/ADE
Length of output: 915
🏁 Script executed:
# Search for all callers of resolveAdeMcpServerLaunch in the entire codebase
rg -n 'resolveAdeMcpServerLaunch' apps/Repository: arul28/ADE
Length of output: 2445
🏁 Script executed:
# Check agentChatService.ts calls
rg -n -B 5 -A 15 'const launch = resolveAdeMcpServerLaunch' apps/desktop/src/main/services/chat/agentChatService.tsRepository: arul28/ADE
Length of output: 1761
🏁 Script executed:
# Check coordinatorAgent.ts call
rg -n -B 5 -A 15 'const launch = resolveAdeMcpServerLaunch' apps/desktop/src/main/services/orchestrator/coordinatorAgent.tsRepository: arul28/ADE
Length of output: 908
chatSessionId parameter accepted but not forwarded to delegate.
The function signature accepts chatSessionId (line 66), but the delegation to resolveDesktopAdeMcpLaunch (lines 83-96) cannot forward it because the AdeMcpLaunchArgs type does not include it. Callers like agentChatService.ts actively pass chatSessionId expecting it to be used in the MCP launch logic, but the parameter is silently dropped, which breaks session tracking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/services/orchestrator/unifiedOrchestratorAdapter.ts`
around lines 83 - 96, The unifiedOrchestratorAdapter accepts chatSessionId but
drops it when calling resolveDesktopAdeMcpLaunch because AdeMcpLaunchArgs
doesn't include it; update the AdeMcpLaunchArgs type (and any place constructing
that type) to include chatSessionId, then forward args.chatSessionId in the
object passed to resolveDesktopAdeMcpLaunch (alongside projectRoot,
workspaceRoot, etc.), and run/adjust any callers (e.g., agentChatService.ts) and
tests to accommodate the new field so session tracking is preserved.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
1 participant
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 by CodeRabbit
New Features
Bug Fixes
Chores
Tests