fix(mcp): emit external launcher for AGENT_RELAY_MCP_COMMAND in packaged mode#85
Conversation
|
CodeAnt AI is reviewing your PR. |
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (13)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds deterministic generation and packaging of MCP resources, platform launchers, a cross-platform resolver exported as resolveAgentRelayMcpCommand, end-to-end verification and CI wiring, and unit tests validating packaged-mode behavior. ChangesAgent Relay MCP External Process Support
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. Comment |
codeant-ai
Bot
added
the
size:L
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces changes to package and run the Agent Relay MCP server as an external resource outside of the app.asar archive, allowing external MCP clients to spawn it. It adds a verification script to smoke-test the spawned MCP process, updates the broker command resolution logic to handle packaged environments, and adds corresponding unit tests. The review feedback highlights several key improvements: wrapping paths in double quotes to handle spaces in file paths, making the dependency verification script robust against nested package-lock dependencies and non-JSON stdout, and considering bundling the MCP server with esbuild to avoid maintaining a fragile list of transitive dependencies in electron-builder.yml.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (!nodeCommand) { | ||
| throw new Error('Node.js was not found on PATH; unable to start packaged Agent Relay MCP server') | ||
| } | ||
| return assertNoAsarMcpCommand(`${nodeCommand} ${resolvePackagedAgentRelayMcpScript()}`) |
There was a problem hiding this comment.
If either nodeCommand or the resolved script path contains spaces (which is extremely common, e.g., /Applications/Pear by Agent Relay.app/... on macOS or C:\Program Files\... on Windows), the constructed command string will be split incorrectly by the shell or command parser, leading to execution failures (e.g., Cannot find module '/Applications/Pear'). Wrapping both paths in double quotes ensures they are treated as single arguments.
return assertNoAsarMcpCommand(`"${nodeCommand}" "${resolvePackagedAgentRelayMcpScript()}"`)| const lockPackage = packages[`node_modules/${packageName}`] | ||
| if (!lockPackage) fail(`package-lock entry missing for ${packageName}`) |
There was a problem hiding this comment.
In package-lock.json (v2/v3), nested dependencies (e.g., due to version conflicts or non-hoisted packages) are keyed by their full path (e.g., node_modules/agent-relay/node_modules/some-dep) rather than just node_modules/some-dep. Since visit is called with the bare package name, looking up node_modules/${packageName} directly will fail for nested dependencies, causing the verification script to crash. Adding a fallback search for keys ending with /node_modules/${packageName} makes this lookup robust.
let lockPackage = packages[`node_modules/${packageName}`]
if (!lockPackage) {
const nestedKey = Object.keys(packages).find((key) => key.endsWith(`/node_modules/${packageName}`))
if (nestedKey) {
lockPackage = packages[nestedKey]
}
}
if (!lockPackage) fail(`package-lock entry missing for ${packageName}`)| const text = buffer.toString('utf8') | ||
| const lines = text.split(/\r?\n/) | ||
| const completeLines = text.endsWith('\n') || text.endsWith('\r\n') ? lines : lines.slice(0, -1) | ||
| return completeLines.filter(Boolean).map((line) => JSON.parse(line)) |
There was a problem hiding this comment.
If the spawned child process prints any non-JSON output to stdout (such as Node.js warnings, deprecation notices, or startup logs from dependencies), JSON.parse(line) will throw a SyntaxError and crash the verification script. Wrapping the parsing logic in a try...catch block to gracefully ignore non-JSON lines makes the verification process much more resilient.
const messages = []
for (const line of completeLines) {
const trimmed = line.trim()
if (!trimmed) continue
try {
messages.push(JSON.parse(trimmed))
} catch {
// Ignore non-JSON lines (e.g. logs or warnings)
}
}
return messages| - from: node_modules | ||
| to: agent-relay-mcp/node_modules | ||
| filter: |
There was a problem hiding this comment.
Hardcoding the entire transitive dependency tree of agent-relay in electron-builder.yml is highly fragile and difficult to maintain. Any future update to agent-relay or its dependencies (including deep transitive ones) will require manual updates to this list, otherwise the packaged app's MCP server will fail at runtime due to missing modules.
Alternative Recommendation:
Consider bundling the MCP entry point (agent-relay-mcp.js) and its dependencies into a single self-contained file using esbuild (which is already in your devDependencies) during the build step. This would completely eliminate the need to ship and maintain this massive list of node_modules in extraResources.
Review of #85 — fix(mcp): external launcher for AGENT_RELAY_MCP_COMMAND in packaged modeVerdict: REQUEST CHANGES (one blocker; otherwise solid) I reviewed against the sub-contract acceptance bar in Acceptance bar status
Self-containment hole (BLOCKER)The packaged resolver emits function resolveNodeCommandForMcp(): string | undefined {
const execBasename = basename(process.execPath).toLowerCase()
if (execBasename === 'node' || execBasename === 'node.exe') {
return process.execPath
}
return resolveCommandOnPath('node')
}In packaged Electron, macOS does not ship env -i HOME=$HOME PATH=/usr/bin:/bin node --version
# env: node: No such file or directoryOn such a machine, the resolver throws This directly contradicts contract non-negotiable #1. Three viable fixes — pick one:
I'd lean toward (1) as the minimal-diff fix and (3) as the proper long-term direction. Smoke test gap related to this:
Other observations (non-blocking)Hand-maintained dependency filter (drift hazard)
Suggestion (follow-up PR): generate the filter at build time from the package-lock closure walk that No PR-time CI
Suggestion (follow-up PR): a minimal PR-time CI workflow running Removed npx fallbackRemoving the One thing worth a one-line PR-description note: in dev mode, if a contributor's Smoke test stdout parser (minor)
Test isolation
Verification I did
What I'd ask for
Code quality, scope discipline, and the regression-test discipline are all good. The blocker is real but fixable with a small follow-up commit. |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| if (!nodeCommand) { | ||
| throw new Error('Node.js was not found on PATH; unable to start packaged Agent Relay MCP server') | ||
| } | ||
| return assertNoAsarMcpCommand(`${nodeCommand} ${resolvePackagedAgentRelayMcpScript()}`) |
There was a problem hiding this comment.
Suggestion: The packaged MCP command is built as a plain space-delimited string without quoting the script path. On macOS installs, process.resourcesPath typically contains spaces (for example inside Pear by Agent Relay.app), so downstream command parsing splits the script path into multiple arguments and the MCP process fails to launch. Quote or otherwise safely escape the script path (or pass command/args separately) when composing the packaged command. [logic error]
Severity Level: Critical 🚨
- ❌ Packaged macOS app cannot launch Agent Relay MCP server.
- ⚠️ Claude Code `/mcp` unusable from packaged Pear builds.Steps of Reproduction ✅
1. Package the Electron app so `app.isPackaged === true` and `process.resourcesPath`
points inside `Pear by Agent Relay.app/Contents/Resources` (the macOS app name with spaces
is defined in `scripts/verify-mcp-spawn.mjs:7` as `APP_NAME = 'Pear by Agent Relay.app'`).
2. Run the packaged app and start a local broker, which calls `BrokerManager.start()` in
`src/main/broker.ts` (class definition around lines 178–360); in the `startBroker` closure
it computes `agentRelayMcpCommand = resolveAgentRelayMcpCommand()` and passes it into
`AgentRelayClient.spawn` via `opts.env.AGENT_RELAY_MCP_COMMAND` (see the `env: { PATH:
..., ...(agentRelayMcpCommand ? { AGENT_RELAY_MCP_COMMAND: agentRelayMcpCommand } : {}) }`
block in `start()`).
3. With `app.isPackaged` true and no `AGENT_RELAY_MCP_COMMAND` override,
`resolveAgentRelayMcpCommand()` in `src/main/broker.ts` (lines 238–250) takes the packaged
branch and builds `assertNoAsarMcpCommand(`${nodeCommand}
${resolvePackagedAgentRelayMcpScript()}`)`, where `resolvePackagedAgentRelayMcpScript()`
(lines 206–219) joins `process.resourcesPath` with
`agent-relay-mcp/node_modules/agent-relay/dist/cli/agent-relay-mcp.js`, yielding a command
string like `node /Applications/Pear by Agent
Relay.app/Contents/Resources/agent-relay-mcp/node_modules/agent-relay/dist/cli/agent-relay-mcp.js`
that contains spaces in the script path and no quoting or escaping.
4. The Agent Relay broker runtime (inside `@agent-relay/harness-driver`, which reads
`AGENT_RELAY_MCP_COMMAND` from the environment of the spawned broker) interprets this
value as a plain space-delimited command line; when the MCP server is first launched (e.g.
when a user runs `/mcp` from Claude Code, which ultimately triggers the broker's MCP
startup), the embedded spaces in `/Applications/Pear by Agent Relay.app/...` cause the
script path to be split into multiple arguments, so `node` cannot open the intended script
and the MCP process fails to start (spawn/ENOENT or similar), breaking MCP functionality
for the packaged app.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main/broker.ts
**Line:** 249:249
**Comment:**
*Logic Error: The packaged MCP command is built as a plain space-delimited string without quoting the script path. On macOS installs, `process.resourcesPath` typically contains spaces (for example inside `Pear by Agent Relay.app`), so downstream command parsing splits the script path into multiple arguments and the MCP process fails to launch. Quote or otherwise safely escape the script path (or pass command/args separately) when composing the packaged command.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
Re-review of #85 (Round 2 — commit d4c6e81)Verdict: APPROVE ✅ All four R2 deliverables from R2 acceptance bar — line-by-lineR2.1 — Electron-as-Node launcher (Round 1 BLOCKER) ✅
End-to-end manual proof I ran: env -i HOME=$HOME PATH=/usr/bin:/bin \
"/.../Pear by Agent Relay.app/Contents/Resources/agent-relay-mcp/launch.sh" \
<<<'{"jsonrpc":"2.0","id":1,"method":"initialize","params":{...}}'
# → {"result":{"protocolVersion":"2024-11-05",...,"serverInfo":{"name":"agent-rel...
R2.2 — Smoke catches the failure class ✅
R2.3 — Generated
|
| Fix | Proof of non-vacuity |
|---|---|
| Resolver packaged-mode emits launcher | Reverted mcp-command.ts + broker.ts to main while keeping broker.test.ts → 3 new tests RED |
| Launcher works under sanitized PATH | Manual env -i PATH=/usr/bin:/bin launch.sh returns valid MCP initialize response. Without launcher (or with old node <script> resolver), the smoke would fail because there's no node in /usr/bin:/bin. |
| Drift check | verify:mcp-resources-drift runs on every CI job; adding any package-lock dep to the agent-relay closure without regenerating would fail the build. |
| CI workflow itself | Already exercised by this PR's own run (in_progress → completed/success). |
Local validation I ran
npx vitest run src/main/broker.test.ts→ 12/12 passed.npm run verify:mcp-resources-drift→ "MCP extraResources config is in sync."npm run verify:mcp-spawn→ ok, emits.../launch.shas the resolver's command.- Manual sanitized-PATH launcher handshake → MCP initialize succeeds.
- CI run #27003447848 → both jobs SUCCESS.
Minor follow-ups (non-blocking; do NOT need to land in this PR)
Linux launcher executable detection
launch.sh:24-34 picks the first executable in <install>/* (filtering chrome-sandbox and *.so). On a Linux Electron build, <install>/ also contains executables like chrome_crashpad_handler that come alphabetically before the actual Pear binary. This is dead code for the current mac-only release (electron-builder.yml only targets mac), but if Pear ever ships a Linux build, the launcher would pick the wrong binary. Easy fix when needed: match against the productName slug (pear-by-agent-relay) or use electron-builder's exec name.
notarize: true is hardcoded
electron-builder.yml:33 hardcodes notarize: true. The CI smoke job builds successfully without notarize credentials because electron-builder skips notarize gracefully when API key env vars are missing, but that's an undocumented dependency. Consider making notarize env-conditional (e.g. notarize: ${NOTARIZE_PEAR:-false}) so the intent is explicit, or accepting the soft-skip behavior. Either is fine.
Excellent work. The resolver extraction into a pure function with explicit options, the harness-based smoke that exercises the real resolver under sanitized PATH, and the hoist-aware dependency walker are all real upgrades over Round 1. Ship it.
2 participants
Root Cause
Pear previously resolved
AGENT_RELAY_MCP_COMMANDfrom packaged app internals. In a packaged Electron app that can produce anapp.asar/.../node_modules/agent-relay/...path, which external MCP clients cannot traverse. The first PR iteration moved the script intoResources, but still emittednode <script>, which is not self-contained on fresh macOS installs becausenodeis not guaranteed onPATH.What Changed
Resources/agent-relay-mcp/launch.shandResources/agent-relay-mcp/launch.cmd.ELECTRON_RUN_AS_NODE=1, locate the host Pear executable relative to themselves, and runnode_modules/agent-relay/dist/cli/agent-relay-mcp.js.src/main/mcp-command.ts;src/main/broker.tswraps it with Electron state.nodeprefix and no user-local/global fallback.AGENT_RELAY_MCP_COMMANDoverrides containingapp.asar.electron-builder.mcp-resources.ymlfrompackage-lock.jsonviascripts/generate-mcp-extraResources.mjs; drift is checked bynpm run verify:mcp-resources-drift.scripts/verify-mcp-spawn.mjsto call the real packaged resolver output throughscripts/resolve-packaged-mcp-command.mjs, run with sanitizedPATH, verify the dependency closure, and complete an MCPinitializehandshake against the resolver-emitted command..github/workflows/ci.yml.No
agent-relayrepo change was needed.Verification
Passed locally:
npx vitest run src/main/broker.test.ts npm test npm run verify:mcp-resources-drift npm run build npm run dist:mac npm run verify:mcp-spawn git diff --checkLocal packaging used ad-hoc signing and skipped notarization because notarization options were unavailable.
Manual Repro
npm run dist:mac.npm run verify:mcp-spawn.resolveAgentRelayMcpCommand, asserts it isContents/Resources/agent-relay-mcp/launch.sh, runs it withPATH=/usr/bin:/bin, and verifies MCPinitializereturnsserverInfo.name === "agent-relay".Non-Negotiables Checklist
~/.local/bin/agent-relay, Homebrew, globalagent-relay, ornodeonPATH.Resources/agent-relay-mcp/launch.{sh,cmd}only and never returns an.asar/MCP path.Resources/agent-relay-mcp/node_modulesand checked by smoke.node_modules/ PATH-friendly resolution..asarrejection, launcher emission, and missing launcher failure.