test: mcp discover — env-var interpolation in external configs#695
test: mcp discover — env-var interpolation in external configs#695anandgupta42 wants to merge 1 commit intomainfrom
Conversation
Cover the readJsonSafe env-var resolution path added in f030bf8. Users with ${VAR} or ${VAR:-default} in .vscode/mcp.json now get their MCP servers resolved correctly; these tests guard against regressions in that interpolation path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
📝 WalkthroughWalkthroughTwo new test cases were added to Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/opencode/test/mcp/discover.test.ts (1)
389-404: Use the sharedtmpdir()fixture pattern in these new tests.The new tests are good functionally, but they still depend on the file’s manual temp-dir lifecycle. Please migrate touched tests to
await using ... = tmpdir()for consistent cleanup semantics with the project test standard.As per coding guidelines, "Use the
tmpdirfunction fromfixture/fixture.tsto create temporary directories for tests with automatic cleanup in test files" 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 389 - 404, The test "resolves ${VAR:-default} with fallback when env var is unset" currently manages a manual tempDir lifecycle; replace it with the shared tmpdir fixture by using "await using temp = tmpdir()" (or whatever fixture alias is exported) to obtain the temp directory and use temp.path (or the fixture's API) instead of the manual tempDir variable, remove any manual cleanup, and update calls that reference tempDir (mkdir, writeFile, and the call to discoverExternalMcp) to use the fixture-provided path; ensure the test still deletes the env var key and calls discoverExternalMcp(tempPath) so the assertions on result["default-test"] remain unchanged.
🤖 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/test/mcp/discover.test.ts`:
- Around line 389-404: The test "resolves ${VAR:-default} with fallback when env
var is unset" currently manages a manual tempDir lifecycle; replace it with the
shared tmpdir fixture by using "await using temp = tmpdir()" (or whatever
fixture alias is exported) to obtain the temp directory and use temp.path (or
the fixture's API) instead of the manual tempDir variable, remove any manual
cleanup, and update calls that reference tempDir (mkdir, writeFile, and the call
to discoverExternalMcp) to use the fixture-provided path; ensure the test still
deletes the env var key and calls discoverExternalMcp(tempPath) so the
assertions on result["default-test"] remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2388238e-97c3-4d6a-8e91-c3e76ddb13ec
📒 Files selected for processing (1)
packages/opencode/test/mcp/discover.test.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
2 participants
What does this PR do?
1.
discoverExternalMcpenv-var interpolation —src/mcp/discover.ts(2 new tests)The
readJsonSafefunction gained env-var interpolation support in commit f030bf8 (#656). When external MCP configs (.vscode/mcp.json,.mcp.json,.gemini/settings.json, etc.) contain${VAR}or${VAR:-default}patterns, these are now resolved viaConfigPaths.parseTextbefore JSON parsing. Zero tests existed for this interpolation path — all 18 existing discover tests used hardcoded string values.This matters because users migrating configs from Claude Code, VS Code, or docker-compose commonly use
${API_KEY}patterns in their MCP server configs. A regression in the interpolation path would silently produce broken MCP servers with literal${VAR}strings instead of resolved values.New coverage includes:
${VAR}basic resolution: Sets a process env var, writes a.vscode/mcp.jsonreferencing it, verifies the resolved value appears in the discovered server's command array${VAR:-default}fallback: Uses an unset env var with a:-fallbackdefault, verifies the fallback value is used when the variable is not in the environmentReconnaissance findings
During reconnaissance, I also examined and ruled out several other candidates:
File.search()(zero test coverage) — rejected by critic due to race condition: the internal file scanner populates cache asynchronously via ripgrep, andfiles()returns cache without waiting for scan completion, making integration tests inherently flakyresolveEnvVarsinmcp/index.ts— already well covered intest/mcp/env-var-interpolation.test.tsaltimate-backend defaultModelinprovider.ts— already covered intest/provider/provider.test.tsTrace.listTracesPaginated— already covered intest/altimate/tracing.test.tsType of change
Issue for this PR
N/A — proactive test coverage for recently added env-var interpolation in MCP discovery
How did you verify your code works?
Marker guard check:
bun run script/upstream/analyze.ts --markers --base main --strict→ all clear.Checklist
https://claude.ai/code/session_01K8qyA8V76y6zrCYuEVVk3x
Summary by cubic
Add two tests to
discoverExternalMcpthat cover env-var interpolation in external MCP configs. Ensures${VAR}and${VAR:-default}resolve into the command array via thereadJsonSafeinterpolation path.Written for commit 129e635. Summary will update on new commits.
Summary by CodeRabbit