feat: auto-discover MCP servers from external AI tool configs#311
feat: auto-discover MCP servers from external AI tool configs#311anandgupta42 merged 2 commits intomainfrom
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdd a new external-MCP discovery pipeline: scan known tool config files, normalize discovered servers, optionally merge into startup config, surface discoveries at MCP init (toast + telemetry), expose an Changes
Sequence DiagramsequenceDiagram
participant ConfigLoader as Config Loader
participant Discover as Discovery Module
participant FS as Filesystem
participant Parser as JSONC Parser
participant MCP as MCP Init
participant UI as UI / Telemetry
ConfigLoader->>Discover: discoverExternalMcp(worktree)
loop sources (project/home)
Discover->>FS: read candidate file
alt file exists
FS-->>Discover: file content
Discover->>Parser: parse JSON/JSONC
Parser-->>Discover: parsed object
Discover->>Discover: normalize -> Config.Mcp entry
Discover-->>ConfigLoader: add server + source (first-source-wins)
else missing / unreadable
FS-->>Discover: error / not found (ignored)
end
end
Discover-->>ConfigLoader: { servers, sources }
ConfigLoader->>MCP: merge discovered (skip existing names)
MCP->>UI: show toast (new servers)
MCP->>UI: send mcp_discovery telemetry
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| try { | ||
| // Use JSONC parser to handle comments (common in VS Code configs) | ||
| parsed = parseJsonc(text) | ||
| } catch { | ||
| log.debug("failed to parse external MCP config", { file: source.file }) | ||
| continue |
There was a problem hiding this comment.
Bug: The try/catch block around parseJsonc is ineffective because the function doesn't throw on parse errors, potentially allowing partially malformed JSON to be accepted silently.
Severity: MEDIUM
Suggested Fix
Remove the try/catch block. Instead, pass an errors array to the parseJsonc function, like parseJsonc(text, errors). After the call, check if errors.length > 0 to properly detect and handle any parsing failures, which aligns with the established pattern in the codebase.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/opencode/src/mcp/discover.ts#L82-L87
Potential issue: The code attempts to handle JSON parsing errors using a `try/catch`
block around `parseJsonc`. However, `parseJsonc` is a fault-tolerant parser and does not
throw exceptions for malformed input. Instead, it returns a best-effort parsed object.
While a subsequent check for `!parsed` catches completely invalid JSON that results in
an `undefined` return, it allows partially valid or corrupted JSON to be processed
silently. This could lead to incomplete or malformed MCP server configurations being
registered, causing subtle and hard-to-debug issues. This approach is inconsistent with
other parts of the codebase that correctly check for parsing errors.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Fixed. Switched to errors array approach.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/opencode/src/config/config.ts (1)
1531-1532:⚠️ Potential issue | 🟡 MinorRemove stray code artifacts.
These lines appear to be debug or copy-paste artifacts. They're expression statements that evaluate
Filesystem.writebut don't call it, serving no purpose.🧹 Proposed fix
-Filesystem.write -Filesystem.write🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/config/config.ts` around lines 1531 - 1532, Remove the stray expression statements that just reference Filesystem.write (the duplicated "Filesystem.write" lines) since they are dead/no-op artifacts; delete those two lines from the code (and if Filesystem is now unused, remove its import or require) so only actual calls to Filesystem.write or intended logic remain.
🧹 Nitpick comments (3)
.opencode/command/discover-and-add-mcps.md (1)
26-28: Minor inconsistency in config file paths.The documentation mentions writing to
.altimate-code/altimate-code.jsonfor project scope but~/.config/opencode/opencode.jsonfor global scope. Consider aligning the naming for consistency, or clarify if this is intentional due to backwards compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.opencode/command/discover-and-add-mcps.md around lines 26 - 28, Documentation is inconsistent about config file naming: it refers to the project config as `.altimate-code/altimate-code.json` but the global config as `~/.config/opencode/opencode.json`; update the docs in discover-and-add-mcps.md to either use consistent naming (e.g., `.altimate-code/opencode.json` vs `~/.config/opencode/opencode.json`) or add a short clarifying note that the different filenames are intentional for backwards compatibility; specifically mention the `mcp` key and the `$ARGUMENTS` flags (`--scope global` and `--scope project`) so readers know exactly which file is written to in `mcp` operations.packages/opencode/test/mcp/discover.test.ts (1)
1-15: Consider using the project'stmpdirfixture for consistency.The tests manually manage temp directories with
mkdtemp/rminbeforeEach/afterEach. The project's coding guidelines recommend using thetmpdirfunction fromfixture/fixture.tswithawait usingsyntax for automatic cleanup.This is a stylistic preference for consistency with other tests in the codebase, but the current approach is functionally correct.
♻️ Example refactor using tmpdir fixture
-import { describe, test, expect, beforeEach, afterEach } from "bun:test" -import { mkdtemp, rm, mkdir, writeFile } from "fs/promises" -import { tmpdir } from "os" +import { describe, test, expect } from "bun:test" +import { mkdir, writeFile } from "fs/promises" import path from "path" import { discoverExternalMcp } from "../../src/mcp/discover" +import { tmpdir } from "../fixture/fixture" -let tempDir: string - -beforeEach(async () => { - tempDir = await mkdtemp(path.join(tmpdir(), "mcp-discover-")) -}) - -afterEach(async () => { - await rm(tempDir, { recursive: true, force: true }) -}) - describe("discoverExternalMcp", () => { test("parses .vscode/mcp.json with servers key", async () => { + await using tmp = await tmpdir() + const tempDir = tmp.path await mkdir(path.join(tempDir, ".vscode"), { recursive: true }) // ... rest of test })Based on learnings: "Use the
tmpdirfunction fromfixture/fixture.tsto create temporary directories for tests with automatic cleanup" and "Always useawait usingsyntax withtmpdir()for automatic cleanup when the variable goes out of scope".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/mcp/discover.test.ts` around lines 1 - 15, Replace the manual temp dir lifecycle in discover.test.ts (tempDir variable, beforeEach using mkdtemp and afterEach using rm) with the project's tmpdir fixture: import tmpdir from "fixture/fixture.ts", then in each test use "await using (const dir = await tmpdir())" (or the equivalent pattern used in the codebase) to obtain the temp directory path and rely on automatic cleanup; remove the mkdtemp/mkdir/writeFile/rm setup and any beforeEach/afterEach that manage tempDir, and update references from tempDir to the tmpdir-provided directory variable so tests remain functionally identical but follow the project's fixture pattern.packages/opencode/src/mcp/discover.ts (1)
27-57: Consider adding type validation for headers and environment values.The transform function checks
typeof entry.headers === "object"but doesn't validate that all values are strings as required byConfig.McpRemote.headers(same forenv/environment). While the Zod schema will validate at config merge time, invalid values could slip through discovery and cause confusing validation errors later.This is a minor robustness improvement - the current code will work correctly for well-formed external configs.
♻️ Optional: Add value type validation
+function isStringRecord(obj: unknown): obj is Record<string, string> { + if (!obj || typeof obj !== "object") return false + return Object.values(obj).every((v) => typeof v === "string") +} + function transform(entry: Record<string, any>): Config.Mcp | undefined { // Remote server if (entry.url && typeof entry.url === "string") { const result: Record<string, any> = { type: "remote" as const, url: entry.url, } - if (entry.headers && typeof entry.headers === "object") { + if (isStringRecord(entry.headers)) { result.headers = entry.headers } return result as Config.Mcp } // Local server if (entry.command) { const cmd = Array.isArray(entry.command) ? entry.command.map(String) : [String(entry.command), ...(Array.isArray(entry.args) ? entry.args.map(String) : [])] const result: Record<string, any> = { type: "local" as const, command: cmd, } - if (entry.env && typeof entry.env === "object") { + if (isStringRecord(entry.env)) { result.environment = entry.env } return result as Config.Mcp } return undefined }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/mcp/discover.ts` around lines 27 - 57, The transform function should validate/coerce header and env values to strings before returning Config.Mcp so invalid types don't slip through; in transform, when handling entry.headers (for remote) and entry.env (for local), replace the current passthrough with a new object that iterates Object.entries(...) and sets each key to String(value) (or skip non-serializable keys) so the returned result.headers and result.environment conform to Config.McpRemote.headers and Config.McpLocal.environment; keep the rest of transform logic the same and reference the transform function, entry.headers -> result.headers, and entry.env -> result.environment when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/opencode/src/config/config.ts`:
- Around line 1531-1532: Remove the stray expression statements that just
reference Filesystem.write (the duplicated "Filesystem.write" lines) since they
are dead/no-op artifacts; delete those two lines from the code (and if
Filesystem is now unused, remove its import or require) so only actual calls to
Filesystem.write or intended logic remain.
---
Nitpick comments:
In @.opencode/command/discover-and-add-mcps.md:
- Around line 26-28: Documentation is inconsistent about config file naming: it
refers to the project config as `.altimate-code/altimate-code.json` but the
global config as `~/.config/opencode/opencode.json`; update the docs in
discover-and-add-mcps.md to either use consistent naming (e.g.,
`.altimate-code/opencode.json` vs `~/.config/opencode/opencode.json`) or add a
short clarifying note that the different filenames are intentional for backwards
compatibility; specifically mention the `mcp` key and the `$ARGUMENTS` flags
(`--scope global` and `--scope project`) so readers know exactly which file is
written to in `mcp` operations.
In `@packages/opencode/src/mcp/discover.ts`:
- Around line 27-57: The transform function should validate/coerce header and
env values to strings before returning Config.Mcp so invalid types don't slip
through; in transform, when handling entry.headers (for remote) and entry.env
(for local), replace the current passthrough with a new object that iterates
Object.entries(...) and sets each key to String(value) (or skip non-serializable
keys) so the returned result.headers and result.environment conform to
Config.McpRemote.headers and Config.McpLocal.environment; keep the rest of
transform logic the same and reference the transform function, entry.headers ->
result.headers, and entry.env -> result.environment when making the change.
In `@packages/opencode/test/mcp/discover.test.ts`:
- Around line 1-15: Replace the manual temp dir lifecycle in discover.test.ts
(tempDir variable, beforeEach using mkdtemp and afterEach using rm) with the
project's tmpdir fixture: import tmpdir from "fixture/fixture.ts", then in each
test use "await using (const dir = await tmpdir())" (or the equivalent pattern
used in the codebase) to obtain the temp directory path and rely on automatic
cleanup; remove the mkdtemp/mkdir/writeFile/rm setup and any
beforeEach/afterEach that manage tempDir, and update references from tempDir to
the tmpdir-provided directory variable so tests remain functionally identical
but follow the project's fixture pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8aeb9ba9-f6df-4cd1-b123-3c325add28f4
📒 Files selected for processing (4)
.opencode/command/discover-and-add-mcps.mdpackages/opencode/src/config/config.tspackages/opencode/src/mcp/discover.tspackages/opencode/test/mcp/discover.test.ts
| ## Instructions | ||
|
|
||
| 1. Look for MCP server configurations in the current project directory from these sources: | ||
| - `.vscode/mcp.json` (key: `servers`) — VS Code / Copilot |
There was a problem hiding this comment.
Great call! Added .cursor/mcp.json support in latest push.
kulvirgit
force-pushed
the
feat/mcp-discovery
branch
from
e38f96f to
de8e47b
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/opencode/src/mcp/index.ts (1)
227-234: Telemetrysourcesfield is always empty.The
mcp_discoverytelemetry event hardcodessources: []becauseconsumeDiscoveryResult()only returns server names, not which config files they came from. This reduces the analytical value of the telemetry.Consider either:
- Removing the
sourcesfield from the telemetry schema if it won't be populated- Updating
discover.tsto track and return source information🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/mcp/index.ts` around lines 227 - 234, The mcp_discovery event is sending an empty sources array because consumeDiscoveryResult()/discover.ts only returns server names; either remove the unused sources field from Telemetry.track call or (preferred) update discover.ts and the consumeDiscoveryResult return shape to include source information (e.g., a sources array or a map of serverName→sourceFile), then update the call site in packages/opencode/src/mcp/index.ts (Telemetry.track invocation) to set sources: discoveryResult.sources; reference symbols: Telemetry.track, mcp_discovery event, discoveryResult.serverNames, consumeDiscoveryResult, and discover.ts to locate and implement the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/commands/pr-ce.md:
- Around line 12-14: The shell command in .claude/commands/pr-ce.md uses a
hardcoded absolute path (/home/kulvir/altimate-code/packages/opencode) which
breaks portability; change the command to use a relative path or a repo-root
lookup instead (e.g., change the cd target used in the command and keep the
rest: `cd ... && bun test 2>&1 | tail -5`) so contributors can run it from
different machines—update the command string in that file to compute/point to
packages/opencode via a relative path or a repo-root helper rather than the
absolute /home/kulvir/... path.
- Around line 22-24: The markdown contains a hardcoded absolute path in the
command string `cd /home/kulvir/altimate-code && bun run
script/upstream/analyze.ts --markers --base main 2>&1`; replace that absolute
path with a portable approach (e.g. change the `cd` target to the repository
root discovered at runtime or a relative path or an env var like PROJECT_ROOT)
so the script works on other machines; update the command in the
`.claude/commands/pr-ce.md` file where that exact command string appears to use
one of these portable alternatives.
- Around line 58-60: The backticked shell command contains a hardcoded absolute
path ("cd /home/kulvir/altimate-code && git add -A"); replace this with a
non‑hardcoded approach such as running git add from the repository root or
computing the repo root dynamically (for example using git rev-parse
--show-toplevel) or using a relative path, so the command no longer relies on
/home/kulvir/altimate-code.
In `@packages/opencode/src/altimate/tools/mcp-discover.ts`:
- Around line 89-95: The metadata incorrectly reports new: 0 when toAdd.length
=== 0; update the return object in the early-exit branch (inside the
toAdd.length === 0 check in mcp-discover.ts) to set metadata.new to the actual
new servers count (use newServers.length or the appropriate variable that holds
newly discovered-but-not-configured servers) instead of 0, keeping the rest of
the fields (discovered: discoveredNames.length, existing: alreadyAdded.length,
added: 0) the same and leaving the output text unchanged.
- Around line 64-69: Remove the unnecessary "as any" casts on the discriminated
Mcp union: use the narrowed properties directly on server (when server.type ===
"remote" access server.url; when server.type !== "remote" access
server.command.join(" ")). Update the block around the discovered[name] usage in
mcp-discover.ts so lines that currently use "(server as any).url" and "(server
as any).command" reference server.url and server.command instead, relying on
TypeScript's discriminated union narrowing.
---
Nitpick comments:
In `@packages/opencode/src/mcp/index.ts`:
- Around line 227-234: The mcp_discovery event is sending an empty sources array
because consumeDiscoveryResult()/discover.ts only returns server names; either
remove the unused sources field from Telemetry.track call or (preferred) update
discover.ts and the consumeDiscoveryResult return shape to include source
information (e.g., a sources array or a map of serverName→sourceFile), then
update the call site in packages/opencode/src/mcp/index.ts (Telemetry.track
invocation) to set sources: discoveryResult.sources; reference symbols:
Telemetry.track, mcp_discovery event, discoveryResult.serverNames,
consumeDiscoveryResult, and discover.ts to locate and implement the change.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 910f5cf4-dd35-4cbd-a2d1-7d2e39c194e0
📒 Files selected for processing (10)
.claude/commands/pr-ce.md.opencode/command/discover-and-add-mcps.md.opencode/opencode.jsoncpackages/opencode/src/altimate/telemetry/index.tspackages/opencode/src/altimate/tools/mcp-discover.tspackages/opencode/src/config/config.tspackages/opencode/src/mcp/discover.tspackages/opencode/src/mcp/index.tspackages/opencode/src/tool/registry.tspackages/opencode/test/mcp/discover.test.ts
✅ Files skipped from review due to trivial changes (3)
- .opencode/opencode.jsonc
- packages/opencode/src/config/config.ts
- packages/opencode/test/mcp/discover.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/src/mcp/discover.ts
.claude/commands/pr-ce.md
Outdated
| ```bash | ||
| cd /home/kulvir/altimate-code && bun run script/upstream/analyze.ts --markers --base main 2>&1 | ||
| ``` |
There was a problem hiding this comment.
Another hardcoded absolute path.
Same portability issue as above.
Proposed fix
-cd /home/kulvir/altimate-code && bun run script/upstream/analyze.ts --markers --base main 2>&1
+bun run script/upstream/analyze.ts --markers --base main 2>&1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/commands/pr-ce.md around lines 22 - 24, The markdown contains a
hardcoded absolute path in the command string `cd /home/kulvir/altimate-code &&
bun run script/upstream/analyze.ts --markers --base main 2>&1`; replace that
absolute path with a portable approach (e.g. change the `cd` target to the
repository root discovered at runtime or a relative path or an env var like
PROJECT_ROOT) so the script works on other machines; update the command in the
`.claude/commands/pr-ce.md` file where that exact command string appears to use
one of these portable alternatives.
There was a problem hiding this comment.
Personal command file — hardcoded paths are intentional for my local machine.
There was a problem hiding this comment.
@kulvirgit, understood — since .claude/commands/pr-ce.md is a personal command file intended for your local machine, hardcoded absolute paths are perfectly reasonable. I'll keep this in mind for future reviews.
✏️ Learnings added
Learnt from: kulvirgit
Repo: AltimateAI/altimate-code PR: 311
File: .claude/commands/pr-ce.md:22-24
Timestamp: 2026-03-20T01:58:14.971Z
Learning: The file `.claude/commands/pr-ce.md` in the altimate-code repository is a personal Claude Code command file for the repository author's local machine. Hardcoded absolute paths (e.g., `/home/kulvir/altimate-code`) in this file are intentional and should not be flagged as portability issues in code reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
.claude/commands/pr-ce.md
Outdated
| ```bash | ||
| cd /home/kulvir/altimate-code && git add -A | ||
| ``` |
There was a problem hiding this comment.
Third hardcoded absolute path.
Proposed fix
-cd /home/kulvir/altimate-code && git add -A
+git add -A📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```bash | |
| cd /home/kulvir/altimate-code && git add -A | |
| ``` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/commands/pr-ce.md around lines 58 - 60, The backticked shell command
contains a hardcoded absolute path ("cd /home/kulvir/altimate-code && git add
-A"); replace this with a non‑hardcoded approach such as running git add from
the repository root or computing the repo root dynamically (for example using
git rev-parse --show-toplevel) or using a relative path, so the command no
longer relies on /home/kulvir/altimate-code.
There was a problem hiding this comment.
Same — personal command, not project code.
There was a problem hiding this comment.
@kulvirgit, understood — since this is a personal Claude Code command file intended for your local environment, the hardcoded path is fine. Acknowledged!
(ʃƪ˘ᵕ˘)
kulvirgit
force-pushed
the
feat/mcp-discovery
branch
4 times, most recently
from
0d3393d to
03ef8ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/commands/pr-ce.md:
- Around line 96-100: The checklist currently pre-checks "TypeScript compiles"
which is a false attestation because the workflow that builds the PR body
doesn't run a TS compile/typecheck first; either add an actual compile/typecheck
step (e.g., run your repo's typecheck/build script) before the PR-body
generation in the workflow that produces .claude/commands/pr-ce.md, or change
the checklist item "[x] TypeScript compiles" to an unchecked "- [ ] TypeScript
compiles" so it isn't pre-certified; refer to the checklist line text
"TypeScript compiles" to find and update the item and the PR-body generation
step that creates it.
- Around line 12-14: The pipeline currently uses "bun test 2>&1 | tail -5" which
loses the exit code of bun because the pipeline returns tail's status; fix by
preserving the first command's exit status either by enabling bash pipefail
(e.g., run set -o pipefail before the pipeline) or by capturing the status from
PIPESTATUS (use PIPESTATUS[0] after the pipeline and exit with that value) so
that a failing "bun test" is propagated correctly; update the shell snippet that
contains "bun test 2>&1 | tail -5" accordingly.
In @.opencode/command/discover-and-add-mcps.md:
- Around line 13-19: Ensure the agent exits early with a clear “nothing to add”
when the discovery `list` step returns zero new servers instead of
unconditionally calling `mcp_discover(action: "add")`; only call `mcp_discover`
when the selected `servers` array is non-empty and report that nothing was added
otherwise. Also detect if `ARGUMENTS` contains `--scope global` and in that case
set `scope` to "global" and skip prompting the user for scope (otherwise default
to "project"); use the identifiers `mcp_discover`, `ARGUMENTS`, and the
discovery `list` result to locate and update the logic.
In `@docs/docs/configure/tools.md`:
- Around line 133-136: Update the wording to correctly state that
`/discover-and-add-mcps` is a command (not a "skill") and that discovery occurs
only at startup (it does not pick up external config changes after launch),
revise the toast sentence to indicate discovered servers are loaded into the
current session at startup, and adjust the tip to say that making them permanent
via the `mcp_discover(action: "add")` tool preserves them across restarts but
will not track later external config changes.
- Line 131: Update the docs to reflect the actual config filename behaviour:
change the example showing mcp_discover writing to
~/.config/opencode/opencode.json to instead indicate
~/.config/opencode/altimate-code.json (or explain that resolveConfigPath()
checks candidates ["altimate-code.json","opencode.json","opencode.jsonc"] and
returns the first existing file or altimate-code.json if none exist). Reference
mcp_discover and resolveConfigPath to make clear the documentation matches the
code's resolution order and default.
In `@packages/opencode/src/config/config.ts`:
- Around line 267-280: The auto-discovery flow currently merges external MCP
servers into the live config (see discoverExternalMcp, setDiscoveryResult and
result.mcp) during startup; change it so discovered project-scoped servers are
not enabled automatically: either require a repo-trust/consent check (e.g. gate
behind a trust flag or call to an Instance.isTrustedRepo/confirmApproval) before
merging into result.mcp, or instead store discovered entries in a separate
pending/discovered bucket (e.g. result.experimental.discoveredMcp) and do not
copy them into result.mcp until an explicit approve action runs; ensure the
existing Flag.OPENCODE_DISABLE_PROJECT_CONFIG and
result.experimental?.auto_mcp_discovery checks are extended to enforce this
gating and update setDiscoveryResult usage accordingly.
In `@packages/opencode/src/mcp/discover.ts`:
- Around line 84-90: The code currently uses a plain object accumulator (result)
and assigns result[name] = transformed with repo-controlled names, which allows
inherited keys or prototype pollution; change the accumulator creation to a
null-prototype object (e.g., initialize result via Object.create(null)) and
before inserting validate/reject reserved keys (e.g., skip "__proto__",
"constructor", and any names that would conflict with Object.prototype) when
iterating over servers in the for (const [name, entry] of
Object.entries(servers)) loop and the other analogous accumulation site; keep
using the existing transform(...) and only assign when the name passes the
reserved-name guard.
- Around line 107-111: The try/catch in readJsonSafe() is ineffective because
parseJsonc(text) does not throw on syntax errors; update readJsonSafe() to call
parseJsonc with an errors array (or use jsonc-parser.parse with the errors
param), inspect that errors array after parsing, and if there are any syntax
errors log the failure (use log.debug or log.error with file: filePath) and
return undefined instead of returning a partial result; ensure you reference
parseJsonc, readJsonSafe, filePath, and log in the change so malformed JSONC is
rejected rather than silently accepted.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f599d9c5-2379-4985-93f7-c98765115eed
📒 Files selected for processing (11)
.claude/commands/pr-ce.md.opencode/command/discover-and-add-mcps.md.opencode/opencode.jsoncdocs/docs/configure/tools.mdpackages/opencode/src/altimate/telemetry/index.tspackages/opencode/src/altimate/tools/mcp-discover.tspackages/opencode/src/config/config.tspackages/opencode/src/mcp/discover.tspackages/opencode/src/mcp/index.tspackages/opencode/src/tool/registry.tspackages/opencode/test/mcp/discover.test.ts
✅ Files skipped from review due to trivial changes (5)
- packages/opencode/src/tool/registry.ts
- .opencode/opencode.jsonc
- packages/opencode/src/mcp/index.ts
- packages/opencode/src/altimate/telemetry/index.ts
- packages/opencode/src/altimate/tools/mcp-discover.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/test/mcp/discover.test.ts
.claude/commands/pr-ce.md
Outdated
| ```bash | ||
| cd /home/kulvir/altimate-code/packages/opencode && bun test 2>&1 | tail -5 | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -u
set +e
bash -lc 'false | tail -5'
echo "without pipefail/PIPESTATUS: $?"
bash -lc '{ false | tail -5; test ${PIPESTATUS[0]} -eq 0; }'
echo "with PIPESTATUS check: $?"Repository: AltimateAI/altimate-code
Length of output: 338
🏁 Script executed:
find . -name "pr-ce.md" -type fRepository: AltimateAI/altimate-code
Length of output: 95
🏁 Script executed:
if [ -f ".claude/commands/pr-ce.md" ]; then
echo "=== Lines 12-14 ===";
sed -n '12,14p' ".claude/commands/pr-ce.md"
echo "=== Full file context (lines 1-30) ===";
head -30 ".claude/commands/pr-ce.md"
fiRepository: AltimateAI/altimate-code
Length of output: 1383
Preserve the test exit code in Step 1.
The pipeline bun test 2>&1 | tail -5 returns the exit code of tail, not bun test. A failing test run will still report success unless you preserve the first command's exit status using PIPESTATUS or pipefail.
Suggested fix
-cd /home/kulvir/altimate-code/packages/opencode && bun test 2>&1 | tail -5
+cd /home/kulvir/altimate-code/packages/opencode && { bun test 2>&1 | tail -5; test ${PIPESTATUS[0]} -eq 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/commands/pr-ce.md around lines 12 - 14, The pipeline currently uses
"bun test 2>&1 | tail -5" which loses the exit code of bun because the pipeline
returns tail's status; fix by preserving the first command's exit status either
by enabling bash pipefail (e.g., run set -o pipefail before the pipeline) or by
capturing the status from PIPESTATUS (use PIPESTATUS[0] after the pipeline and
exit with that value) so that a failing "bun test" is propagated correctly;
update the shell snippet that contains "bun test 2>&1 | tail -5" accordingly.
There was a problem hiding this comment.
Personal command — not project code.
There was a problem hiding this comment.
@kulvirgit, understood — since this is a personal Claude Code slash-command for your own workflow rather than project or CI code, the exit-code concern doesn't apply. Thanks for the context!
.claude/commands/pr-ce.md
Outdated
| ## Checklist | ||
| - [x] Tests pass | ||
| - [x] TypeScript compiles | ||
| - [x] Upstream markers verified | ||
| - [x] Docs updated (if applicable) |
There was a problem hiding this comment.
Don't pre-check the compile box yet.
This workflow never runs a TypeScript compile/typecheck before creating the PR body, so this checklist item can become a false attestation. Add the real compile step earlier or leave it unchecked here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/commands/pr-ce.md around lines 96 - 100, The checklist currently
pre-checks "TypeScript compiles" which is a false attestation because the
workflow that builds the PR body doesn't run a TS compile/typecheck first;
either add an actual compile/typecheck step (e.g., run your repo's
typecheck/build script) before the PR-body generation in the workflow that
produces .claude/commands/pr-ce.md, or change the checklist item "[x] TypeScript
compiles" to an unchecked "- [ ] TypeScript compiles" so it isn't pre-certified;
refer to the checklist line text "TypeScript compiles" to find and update the
item and the PR-body generation step that creates it.
There was a problem hiding this comment.
Acknowledged — removed tsgo step earlier, will update checklist.
There was a problem hiding this comment.
@kulvirgit, sounds good! Once you've updated the checklist item from [x] to [ ] (or added a real typecheck step before it), this will be fully addressed. Let me know if you'd like help wiring in the actual typecheck command.
| // altimate_change start — auto-discover MCP servers from external AI tool configs | ||
| if (!Flag.OPENCODE_DISABLE_PROJECT_CONFIG && result.experimental?.auto_mcp_discovery !== false) { | ||
| const { discoverExternalMcp, setDiscoveryResult } = await import("../mcp/discover") | ||
| const { servers: externalMcp, sources } = await discoverExternalMcp(Instance.worktree) | ||
| if (Object.keys(externalMcp).length > 0) { | ||
| result.mcp ??= {} | ||
| const added: string[] = [] | ||
| for (const [name, server] of Object.entries(externalMcp)) { | ||
| if (!(name in result.mcp)) { | ||
| result.mcp[name] = server | ||
| added.push(name) | ||
| } | ||
| } | ||
| setDiscoveryResult(added, sources) |
There was a problem hiding this comment.
Project MCP auto-discovery is a repo-open code-execution path.
This merges repo-owned MCP definitions into the live config before startup completes. Since packages/opencode/src/mcp/discover.ts turns local MCP entries into executable commands, opening an untrusted repo with .vscode/mcp.json or .mcp.json can start arbitrary local processes without an approval step. Please gate project-scope discovery behind repo trust / explicit consent, or keep discovered project servers disabled until approved.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/opencode/src/config/config.ts` around lines 267 - 280, The
auto-discovery flow currently merges external MCP servers into the live config
(see discoverExternalMcp, setDiscoveryResult and result.mcp) during startup;
change it so discovered project-scoped servers are not enabled automatically:
either require a repo-trust/consent check (e.g. gate behind a trust flag or call
to an Instance.isTrustedRepo/confirmApproval) before merging into result.mcp, or
instead store discovered entries in a separate pending/discovered bucket (e.g.
result.experimental.discoveredMcp) and do not copy them into result.mcp until an
explicit approve action runs; ensure the existing
Flag.OPENCODE_DISABLE_PROJECT_CONFIG and result.experimental?.auto_mcp_discovery
checks are extended to enforce this gating and update setDiscoveryResult usage
accordingly.
There was a problem hiding this comment.
Added security warning in docs and experimental.auto_mcp_discovery: false opt-out. This matches VS Code and Claude Code behavior — they also auto-load MCP servers from project files.
There was a problem hiding this comment.
@kulvirgit, thanks for adding the docs warning and the opt-out flag — those are helpful additions. However, the comparison to VS Code doesn't quite hold up on closer inspection.
VS Code prompts users to trust each MCP server before it runs, and re-prompts after configuration changes. This is a separate trust boundary from Workspace Trust — an untrusted workspace runs in restricted mode, which also disables agents. When you add an MCP server to your workspace or change its configuration, VS Code shows a dialog to confirm that you trust the server when you start a server for the first time. If you don't trust the MCP server, it will not be started.
So VS Code's model is actually: opt-in via explicit per-server consent dialog, not opt-out via a config flag. The current implementation is closer to the reverse — silently merging and executing project-owned MCP commands at startup, with a global kill-switch to turn the feature off entirely.
The remaining concern is specifically the project-scope discovery path (.vscode/mcp.json, .mcp.json). A developer cloning an untrusted repository will have those servers executed automatically unless they know to set experimental.auto_mcp_discovery: false in advance — which requires pre-existing knowledge of the feature.
A middle ground that would narrow the risk without adding a full consent UI:
- Keep global/home-directory sources (user's own
~/.claude.json,~/.gemini/settings.json) auto-applied as-is, since those are user-owned files. - For project-scope sources (
.vscode/mcp.json,.mcp.json,.github/copilot/mcp.jsoninside the worktree), treat discovered servers as disabled-by-default (enabled: false) so they're visible via/discover-and-add-mcpsbut don't spawn processes until explicitly approved.
This would still surface the configurations for easy adoption while preventing automatic code execution from repo-committed files. Would that tradeoff work for your use case?
There was a problem hiding this comment.
Fair point on VS Code's trust prompt. We match Claude Code's behavior (auto-loads .mcp.json without prompting). The auto_mcp_discovery: false flag is the opt-out. A trust prompt is a reasonable follow-up but out of scope for this PR — tracking as future work.
There was a problem hiding this comment.
@kulvirgit, understood — the Claude Code precedent is a reasonable baseline, and the auto_mcp_discovery: false flag provides a way for security-conscious users to opt out. I'll drop the blocker.
Would you like me to open a GitHub issue to track the future trust-prompt work? I can include the relevant context (per-server consent dialog, project-scope vs. global-scope distinction) so it's easy to pick up later.
🐰 noted and hopping along
✏️ Learnings added
Learnt from: kulvirgit
Repo: AltimateAI/altimate-code PR: 311
File: packages/opencode/src/config/config.ts:267-280
Timestamp: 2026-03-20T18:08:52.042Z
Learning: In `packages/opencode/src/config/config.ts`, the `auto_mcp_discovery` feature intentionally auto-loads MCP servers from project-scope config files (`.vscode/mcp.json`, `.mcp.json`, `.github/copilot/mcp.json`, `.gemini/settings.json`) at startup without a per-server trust prompt, matching Claude Code's behavior. The `experimental.auto_mcp_discovery: false` flag is the designated opt-out. A future trust/consent prompt for project-scoped MCP servers is tracked as follow-up work, not part of this PR.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
kulvirgit
force-pushed
the
feat/mcp-discovery
branch
2 times, most recently
from
ad701e4 to
dd177f1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/opencode/src/mcp/discover.ts (1)
206-221: Module-level state for one-time toast is acceptable but fragile.The
_lastDiscoverypattern works for the intended one-time startup notification use case. However, this mutable module-level state could be problematic for testing or if discovery is called multiple times in a session.Consider documenting the single-use contract in the function comments, or returning the discovery result directly from the config merge rather than storing it globally.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/mcp/discover.ts` around lines 206 - 221, The module-level mutable _lastDiscovery used by setDiscoveryResult and consumeDiscoveryResult is fragile; either document the single-use contract clearly in both function comments (state that setDiscoveryResult stores a one-time value and consumeDiscoveryResult clears it and may return null on subsequent calls) or refactor the config merge caller to return the discovery result directly instead of calling setDiscoveryResult (remove reliance on _lastDiscovery), updating all call sites that currently expect the global storage to instead accept and pass through the returned { serverNames, sources } payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/opencode/src/mcp/discover.ts`:
- Around line 206-221: The module-level mutable _lastDiscovery used by
setDiscoveryResult and consumeDiscoveryResult is fragile; either document the
single-use contract clearly in both function comments (state that
setDiscoveryResult stores a one-time value and consumeDiscoveryResult clears it
and may return null on subsequent calls) or refactor the config merge caller to
return the discovery result directly instead of calling setDiscoveryResult
(remove reliance on _lastDiscovery), updating all call sites that currently
expect the global storage to instead accept and pass through the returned {
serverNames, sources } payload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0ab3e3f5-81c3-405b-b00a-1d7ed08310fb
📒 Files selected for processing (11)
.claude/commands/pr-ce.md.opencode/command/discover-and-add-mcps.md.opencode/opencode.jsoncdocs/docs/configure/tools.mdpackages/opencode/src/altimate/telemetry/index.tspackages/opencode/src/altimate/tools/mcp-discover.tspackages/opencode/src/config/config.tspackages/opencode/src/mcp/discover.tspackages/opencode/src/mcp/index.tspackages/opencode/src/tool/registry.tspackages/opencode/test/mcp/discover.test.ts
✅ Files skipped from review due to trivial changes (4)
- .opencode/opencode.jsonc
- packages/opencode/src/altimate/telemetry/index.ts
- packages/opencode/src/mcp/index.ts
- packages/opencode/test/mcp/discover.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/opencode/src/tool/registry.ts
- packages/opencode/src/config/config.ts
✅ Tests — All PassedTypeScript — passedcc @kulvirgit |
kulvirgit
force-pushed
the
feat/mcp-discovery
branch
2 times, most recently
from
328c825 to
788e3d4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/opencode/src/altimate/tools/mcp-discover.ts (1)
111-112:⚠️ Potential issue | 🟡 MinorDon't say the added servers are already active unconditionally.
This tool re-runs discovery on demand, so that sentence is false when startup auto-discovery was disabled or when the external config changed after launch. Rephrase it as conditional, or tell the user a restart is required to load the persisted config.
Suggested fix
- lines.push("These servers are already active in the current session via auto-discovery.") + lines.push( + "If these servers were auto-discovered at startup they may already be active in this session; otherwise restart to load them from config.", + )Based on learnings,
experimental.auto_mcp_discovery: falseis the designated opt-out for startup auto-loading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/altimate/tools/mcp-discover.ts` around lines 111 - 112, The message unconditionally claims added servers are already active; update the text produced via the lines.push calls that use toAdd and configPath so it only asserts "already active" when startup auto-discovery was enabled (check the experimental.auto_mcp_discovery setting) or otherwise state that a restart (or re-run of discovery) is required to load the persisted config; adjust the two lines.push entries to produce a conditional sentence based on the auto_mcp_discovery flag (or a generic "restart required to load persisted config" message when that flag is false).docs/docs/configure/tools.md (1)
134-137:⚠️ Potential issue | 🟡 MinorThe startup note still says
skilland implies live reload.
/discover-and-add-mcpsis a command, and discovery only happens at startup, so “no restart needed” is still inaccurate if the external config changes after launch.Suggested wording
-**Auto-discovery:** At startup, altimate-code automatically discovers external MCP servers and loads them into the current session. A toast notification shows what was found. Use the `/discover-and-add-mcps` skill or the `mcp_discover` tool to make them permanent. +**Auto-discovery:** At startup, altimate-code automatically discovers external MCP servers and loads them into the current session. A toast notification shows what was found. Use the `/discover-and-add-mcps` command or the `mcp_discover` tool to make them permanent. - Servers discovered from external configs are loaded automatically — no restart needed. Making them permanent via `mcp_discover(action: "add")` ensures they persist even if the original config file is removed. + Servers discovered from external configs are loaded automatically at startup. Making them permanent via `mcp_discover(action: "add")` ensures they persist even if the original config file is removed, but external config changes after launch still require a restart.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docs/configure/tools.md` around lines 134 - 137, Update the documentation text to refer to "/discover-and-add-mcps" as a command (not a skill), clarify that auto-discovery occurs only at startup (so changes to external configs after launch require a restart to be picked up), and adjust the "no restart needed" tip to state that discovered servers are loaded at startup and must be made permanent via the mcp_discover(action: "add") tool to survive config removal or app restarts. Ensure references to "/discover-and-add-mcps" and "mcp_discover" remain intact..claude/commands/pr-ce.md (1)
96-100:⚠️ Potential issue | 🟡 MinorDon't pre-check
TypeScript compiles.This workflow never runs a typecheck/build step before generating the PR body, so this is still a false attestation.
Suggested fix
-- [x] TypeScript compiles +- [ ] TypeScript compiles🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/commands/pr-ce.md around lines 96 - 100, The checklist wrongly pre-checks "TypeScript compiles"; update the PR template in .claude/commands/pr-ce.md so the "TypeScript compiles" item is not pre-checked (leave "- [ ] TypeScript compiles") or wire it to an actual typecheck/build result instead of hardcoding; locate the checklist entry string "TypeScript compiles" and change the box from checked to unchecked or connect it to the real workflow output.packages/opencode/src/mcp/discover.ts (1)
36-67:⚠️ Potential issue | 🟠 MajorSanitize known fields before casting to
Config.Mcp.These objects are merged after config parsing, so
as Config.Mcpis the only validation they get. Right nowcommand: {}becomes["[object Object]"], arrays or non-string values can land inheaders/environment, andtimeout: 0or1.5is preserved. That still lets invalid MCP shapes reach startup.Suggested hardening
function transform(entry: Record<string, any>): Config.Mcp | undefined { // Remote server — handle both "url" and Claude Code's "type: http" format if (entry.url && typeof entry.url === "string") { const result: Record<string, any> = { type: "remote" as const, url: entry.url, } - if (entry.headers && typeof entry.headers === "object") { - result.headers = entry.headers + if (entry.headers && typeof entry.headers === "object" && !Array.isArray(entry.headers)) { + const headers = Object.fromEntries( + Object.entries(entry.headers).filter(([, value]) => typeof value === "string"), + ) + if (Object.keys(headers).length > 0) result.headers = headers } - if (typeof entry.timeout === "number") result.timeout = entry.timeout + if (Number.isInteger(entry.timeout) && entry.timeout > 0) result.timeout = entry.timeout if (typeof entry.enabled === "boolean") result.enabled = entry.enabled return result as Config.Mcp } // Local server - if (entry.command) { + if (typeof entry.command === "string" || Array.isArray(entry.command)) { const cmd = Array.isArray(entry.command) ? entry.command.map(String) : [String(entry.command), ...(Array.isArray(entry.args) ? entry.args.map(String) : [])] const result: Record<string, any> = { type: "local" as const, command: cmd, } - if (entry.env && typeof entry.env === "object") { - result.environment = entry.env + if (entry.env && typeof entry.env === "object" && !Array.isArray(entry.env)) { + const environment = Object.fromEntries( + Object.entries(entry.env).filter(([, value]) => typeof value === "string"), + ) + if (Object.keys(environment).length > 0) result.environment = environment } - if (typeof entry.timeout === "number") result.timeout = entry.timeout + if (Number.isInteger(entry.timeout) && entry.timeout > 0) result.timeout = entry.timeout if (typeof entry.enabled === "boolean") result.enabled = entry.enabled return result as Config.Mcp }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/src/mcp/discover.ts` around lines 36 - 67, The transform function returns unvalidated objects cast to Config.Mcp allowing bad shapes through; update transform to strictly sanitize known fields before casting: validate that entry.command is either a non-empty array of primitive/string values (map each to String only for primitives; if an element is object/undefined, treat the whole entry as invalid and return undefined) or a single primitive convertible to a single-string command, and only accept entry.args when it's an array of primitives; coerce headers and env into string-to-string maps by iterating their keys and keeping only primitive values (String(value)), reject non-object headers/env; normalize timeout to an integer >= 1 (floor or round) otherwise omit it; ensure enabled is boolean before including it; finally, only cast to Config.Mcp after these field-by-field validations and return undefined if required fields are invalid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/docs/configure/tools.md`:
- Around line 120-132: Update the docs to list all discovered config locations
and clarify write behavior: state that discovery checks ~/.mcp.json and
~/.gemini/settings.json in addition to the ones already listed, and clarify that
mcp_discover(action: "add", scope: "project") only defaults to writing to
.altimate-code/altimate-code.json when no project config exists — otherwise
resolveConfigPath() will reuse an existing .opencode/* or a root-level project
config; also mention the global write target is the platform config dir (e.g.,
~/.config/opencode/) for scope: "global" and keep the commands
(mcp_discover(action: "list"), mcp_discover(action: "add", scope: "project"),
mcp_discover(action: "add", scope: "global")) as examples.
In `@packages/opencode/src/altimate/tools/mcp-discover.ts`:
- Around line 34-60: The description and empty-state "Checked:" message in
mcp-discover.ts are out of sync with the actual discovery sources; fix by
importing and using the shared source labels constant from the discovery logic
(use the same exported symbol from packages/opencode/src/mcp/discover.ts — e.g.,
EXTERNAL_MCP_SOURCE_LABELS or SOURCE_LABELS) instead of hardcoding the list,
then replace the description text and the output Checked: line to join that
constant's labels so Cursor and the project/home variants are always reflected;
update the description string and the empty-state output that currently
references .vscode/mcp.json, .github/copilot/mcp.json, .mcp.json,
~/.gemini/settings.json, ~/.claude.json to derive from the shared labels and
include the exported symbol where discoverExternalMcp is used (in execute).
---
Duplicate comments:
In @.claude/commands/pr-ce.md:
- Around line 96-100: The checklist wrongly pre-checks "TypeScript compiles";
update the PR template in .claude/commands/pr-ce.md so the "TypeScript compiles"
item is not pre-checked (leave "- [ ] TypeScript compiles") or wire it to an
actual typecheck/build result instead of hardcoding; locate the checklist entry
string "TypeScript compiles" and change the box from checked to unchecked or
connect it to the real workflow output.
In `@docs/docs/configure/tools.md`:
- Around line 134-137: Update the documentation text to refer to
"/discover-and-add-mcps" as a command (not a skill), clarify that auto-discovery
occurs only at startup (so changes to external configs after launch require a
restart to be picked up), and adjust the "no restart needed" tip to state that
discovered servers are loaded at startup and must be made permanent via the
mcp_discover(action: "add") tool to survive config removal or app restarts.
Ensure references to "/discover-and-add-mcps" and "mcp_discover" remain intact.
In `@packages/opencode/src/altimate/tools/mcp-discover.ts`:
- Around line 111-112: The message unconditionally claims added servers are
already active; update the text produced via the lines.push calls that use toAdd
and configPath so it only asserts "already active" when startup auto-discovery
was enabled (check the experimental.auto_mcp_discovery setting) or otherwise
state that a restart (or re-run of discovery) is required to load the persisted
config; adjust the two lines.push entries to produce a conditional sentence
based on the auto_mcp_discovery flag (or a generic "restart required to load
persisted config" message when that flag is false).
In `@packages/opencode/src/mcp/discover.ts`:
- Around line 36-67: The transform function returns unvalidated objects cast to
Config.Mcp allowing bad shapes through; update transform to strictly sanitize
known fields before casting: validate that entry.command is either a non-empty
array of primitive/string values (map each to String only for primitives; if an
element is object/undefined, treat the whole entry as invalid and return
undefined) or a single primitive convertible to a single-string command, and
only accept entry.args when it's an array of primitives; coerce headers and env
into string-to-string maps by iterating their keys and keeping only primitive
values (String(value)), reject non-object headers/env; normalize timeout to an
integer >= 1 (floor or round) otherwise omit it; ensure enabled is boolean
before including it; finally, only cast to Config.Mcp after these field-by-field
validations and return undefined if required fields are invalid.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 44432a76-f1f9-4231-87a3-a416c8e07bce
📒 Files selected for processing (11)
.claude/commands/pr-ce.md.opencode/command/discover-and-add-mcps.md.opencode/opencode.jsoncdocs/docs/configure/tools.mdpackages/opencode/src/altimate/telemetry/index.tspackages/opencode/src/altimate/tools/mcp-discover.tspackages/opencode/src/config/config.tspackages/opencode/src/mcp/discover.tspackages/opencode/src/mcp/index.tspackages/opencode/src/tool/registry.tspackages/opencode/test/mcp/discover.test.ts
✅ Files skipped from review due to trivial changes (4)
- .opencode/opencode.jsonc
- packages/opencode/src/altimate/telemetry/index.ts
- packages/opencode/src/mcp/index.ts
- .opencode/command/discover-and-add-mcps.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/opencode/src/tool/registry.ts
- packages/opencode/src/config/config.ts
- packages/opencode/test/mcp/discover.test.ts
kulvirgit
force-pushed
the
feat/mcp-discovery
branch
2 times, most recently
from
94fcf5c to
a78f36e
Compare
anandgupta42
force-pushed
the
feat/mcp-discovery
branch
from
a78f36e to
1a61b48
Compare
…pilot, and Gemini configs
anandgupta42
force-pushed
the
feat/mcp-discovery
branch
from
1a61b48 to
49455a4
Compare
Bugs found by adversarial testing (56 new tests):
1. **Prototype pollution** — result object inherited from Object.prototype,
so `servers["constructor"]` and `servers["__proto__"]` returned inherited
values even when those names were correctly filtered. Fix: use
`Object.create(null)` for the result map.
2. **Crash on malformed args** — `entry.args.map(String)` throws TypeError
when args contain objects with overridden `toString` (e.g., `{toString:
"hacked"}`). Fix: wrap in `safeStr()` with try/catch fallback to
`"[invalid]"`, and filter out null/undefined args.
3. **JSONC trailing commas rejected** — `jsonc-parser` reports errors for
trailing commas by default, causing VS Code configs (which commonly use
trailing commas) to be silently ignored. Fix: pass
`{allowTrailingComma: true}`.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…n `opencode.jsonc` The `experimental` config added in #311 was missing upstream markers, causing the Marker Guard CI check to fail on main. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Documents the new auto-discovery feature (#311) in the security FAQ, covering supported config sources, trust model for home vs project-scoped servers, security hardening, and how to disable the feature. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Documents the new auto-discovery feature (#311) in the security FAQ, covering supported config sources, trust model for home vs project-scoped servers, security hardening, and how to disable the feature. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4 participants
Summary
.vscode/mcp.json,.github/copilot/mcp.json,.mcp.json(Claude Code), and.gemini/settings.json, transforming them into native format at lowest priority (user config always wins)/discover-and-add-mcpscommand: Lets users permanently write discovered servers to project or global config with--scope project|globalTest plan
bun test test/mcp/discover.test.ts— 17/17 pass.vscode/mcp.jsonwith test server → altimate shows it at startup.mcp.json(Claude Code format) → also discoveredaltimate-code.json→ user config winsbun test→ no regressions🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests