feat(agent): add process MCP custom servers#989
Open
Nikhil (shadowfax92) wants to merge 1 commit into
Open
Conversation
Contributor
✅ Tests passed — 1238/1242
|
Contributor
Greptile SummaryThis PR extends the custom MCP server system to support local process-based (stdio) servers alongside the existing HTTP/SSE URL-based servers, and ships an AnythingLLM preset as the first concrete use case. The change touches the full stack: a new form UI with command/args/env/cwd fields, shared Zod schema extensions, a payload-mapping helper, and server-side
Confidence Score: 3/5The process-spawn path will silently fail for any user who provides environment variables, including when using the bundled AnythingLLM preset. The env-merging omission in connectMcpClient means the most visible new feature — the AnythingLLM preset — produces a user-visible failure the first time someone fills in the pre-populated env fields and clicks Connect. The rest of the changes (schema, UI, tests, cache key) are solid and well-structured. packages/browseros-agent/apps/server/src/agent/mcp-builder.ts — specifically the StdioClientTransport constructor call and the resolveProcessCommand helper. Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User (Browser Extension)
participant D as AddCustomMCPDialog
participant S as McpServer Storage
participant CR as buildChatRequestBody
participant SV as Agent Server (chat-service)
participant MB as mcp-builder
participant P as Local Process (npx/stdio)
participant H as Remote HTTP MCP
U->>D: "Fill form (type = process | http)"
D->>S: addServer(config with type, command/url, env, args)
S-->>CR: customMcpServers (via buildChatCustomMcpServers)
CR->>SV: "POST /chat { browserContext.customMcpServers }"
SV->>MB: buildMcpServerSpecs(browserContext)
alt "type = process"
MB->>P: StdioClientTransport(command, args, env, cwd)
P-->>MB: tools via stdio
else "type = http"
MB->>H: detectMcpTransport then SSE or HTTP
H-->>MB: tools via HTTP
end
MB-->>SV: McpServerSpec[]
SV-->>U: streamed AI response with MCP tools
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
packages/browseros-agent/apps/server/src/agent/mcp-builder.ts:141-146
**Spawned process loses `PATH` when env is provided**
`StdioClientTransport` passes the `env` option directly to `child_process.spawn`, which replaces the entire environment — it does not merge with `process.env`. When a user fills in even a single env var (e.g. `ANYTHINGLLM_BASE_URL`), `PATH`, `HOME`, and every other inherited var are stripped. The `npx` command (the canonical preset) then cannot locate binaries, causing it to fail silently with a spawn error.
```suggestion
? new StdioClientTransport({
command: resolveProcessCommand(spec.command),
args: spec.args,
env: spec.env
? { ...process.env, ...spec.env }
: undefined,
cwd: spec.cwd,
})
```
### Issue 2 of 3
packages/browseros-agent/apps/server/src/agent/mcp-builder.ts:104-108
**`resolveProcessCommand` only covers `npx` on Windows**
Other common MCP server launchers — `node`, `python`, `uvx`, `bunx` — need the same `.cmd` suffix treatment on Windows or they will fail to spawn. Consider either handling the full set of well-known launchers, or applying the `.cmd` heuristic to any command that has no path separator and is recognised as a script launcher.
### Issue 3 of 3
packages/browseros-agent/apps/server/src/api/services/chat-service.ts:490
**Duplicated type-defaulting logic across three locations**
The expression `server.type ?? (server.command ? 'process' : 'http')` appears identically in `browser-context.ts` (schema `superRefine`), `mcp-builder.ts` (`buildMcpServerSpecs`), and here in `chat-service.ts`. Consider transforming `type` to its resolved value inside the shared Zod schema, or exporting a small helper, so downstream code never needs to re-implement the heuristic.
Reviews (1): Last reviewed commit: "feat: add process MCP custom servers (bo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+141
to
+146
| ? new StdioClientTransport({ | ||
| command: resolveProcessCommand(spec.command), | ||
| args: spec.args, | ||
| env: spec.env, | ||
| cwd: spec.cwd, | ||
| }) |
Contributor
There was a problem hiding this comment.
PATH when env is provided
StdioClientTransport passes the env option directly to child_process.spawn, which replaces the entire environment — it does not merge with process.env. When a user fills in even a single env var (e.g. ANYTHINGLLM_BASE_URL), PATH, HOME, and every other inherited var are stripped. The npx command (the canonical preset) then cannot locate binaries, causing it to fail silently with a spawn error.
Suggested change
| ? new StdioClientTransport({ | |
| command: resolveProcessCommand(spec.command), | |
| args: spec.args, | |
| env: spec.env, | |
| cwd: spec.cwd, | |
| }) | |
| ? new StdioClientTransport({ | |
| command: resolveProcessCommand(spec.command), | |
| args: spec.args, | |
| env: spec.env | |
| ? { ...process.env, ...spec.env } | |
| : undefined, | |
| cwd: spec.cwd, | |
| }) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros-agent/apps/server/src/agent/mcp-builder.ts
Line: 141-146
Comment:
**Spawned process loses `PATH` when env is provided**
`StdioClientTransport` passes the `env` option directly to `child_process.spawn`, which replaces the entire environment — it does not merge with `process.env`. When a user fills in even a single env var (e.g. `ANYTHINGLLM_BASE_URL`), `PATH`, `HOME`, and every other inherited var are stripped. The `npx` command (the canonical preset) then cannot locate binaries, causing it to fail silently with a spawn error.
```suggestion
? new StdioClientTransport({
command: resolveProcessCommand(spec.command),
args: spec.args,
env: spec.env
? { ...process.env, ...spec.env }
: undefined,
cwd: spec.cwd,
})
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+104
to
+108
| function resolveProcessCommand(command: string): string { | ||
| if (process.platform === 'win32' && command.toLowerCase() === 'npx') { | ||
| return 'npx.cmd' | ||
| } | ||
| return command |
Contributor
There was a problem hiding this comment.
resolveProcessCommand only covers npx on Windows
Other common MCP server launchers — node, python, uvx, bunx — need the same .cmd suffix treatment on Windows or they will fail to spawn. Consider either handling the full set of well-known launchers, or applying the .cmd heuristic to any command that has no path separator and is recognised as a script launcher.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/browseros-agent/apps/server/src/agent/mcp-builder.ts
Line: 104-108
Comment:
**`resolveProcessCommand` only covers `npx` on Windows**
Other common MCP server launchers — `node`, `python`, `uvx`, `bunx` — need the same `.cmd` suffix treatment on Windows or they will fail to spawn. Consider either handling the full set of well-known launchers, or applying the `.cmd` heuristic to any command that has no path separator and is recognised as a script launcher.
How can I resolve this? If you propose a fix, please make it concise.
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
Fixes #281
Test plan
git diff --check origin/dev...HEADbun --env-file=apps/server/.env.development test apps/agent/lib/mcp/customMcpServerPayload.test.ts apps/server/tests/agent/mcp-builder.test.ts(agent tests passed locally; server test blocked locally by existing @browseros/shared workspace resolution issue tracked in bosmain-bjx)bunx biome checkon touched filesbun run typecheckfrompackages/browseros-agent/apps/server(blocked locally by existing tsc resolution issue tracked in bosmain-woi)bun run typecheckfrompackages/browseros-agent/apps/agent(blocked locally by existing wxt script resolution issue tracked in bosmain-090)bun run buildfrompackages/browseros-agent/apps/agent(blocked locally by existing graphql-codegen script resolution issue tracked in bosmain-4xe)