fix: resolve env-var references in MCP server environment config (closes #656)#666
fix: resolve env-var references in MCP server environment config (closes #656)#666VJ-yadav wants to merge 3 commits intoAltimateAI:mainfrom
Conversation
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAttempts env-var interpolation when reading external MCP JSON via Changes
Sequence Diagram(s)sequenceDiagram
participant File as External MCP File
participant Discover as discover.ts (readJsonSafe)
participant ConfigPaths as ConfigPaths.parseText
participant Index as mcp/index.ts (resolveEnvVars)
participant Transport as StdioClientTransport
File->>Discover: provide raw config text
Discover->>ConfigPaths: parseText(text, filePath, "empty")
alt parseText succeeds
ConfigPaths-->>Discover: parsed + interpolated MCP definition
Discover-->>Index: return MCP definition
Index->>Index: resolveEnvVars(environment)
Index-->>Transport: pass resolved environment
else parseText fails
ConfigPaths-->>Discover: throw/error
Discover->>Discover: fallback to parseJsonc(text, errors, {allowTrailingComma:true})
Discover-->>Index: return parsed MCP definition (uninterpolated if errors)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
f77c8bd to
8e1671f
Compare
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/test/mcp/env-var-interpolation.test.ts">
<violation number="1" location="packages/opencode/test/mcp/env-var-interpolation.test.ts:19">
P1: These unit tests exercise a **copy** of `resolveEnvVars` pasted into the test file, not the actual production function from `src/mcp/index.ts` (which is not exported). If the production regex or logic changes, these 11 tests will still pass against the stale local copy, giving false confidence.
Export `resolveEnvVars` (and optionally `ENV_VAR_PATTERN`) from the source module and import it here instead of duplicating the implementation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
Testing: MCP env-var interpolation fixThe problem: When users configure MCP servers with environment variable references like Why it happens: PR #655 (v0.5.19) added
The fix:
14 new tests, all passing (importing from production source, not duplicated): 
Covers all syntaxes: Also addressed cubic-dev-ai review: |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/opencode/test/mcp/env-var-interpolation.test.ts (2)
16-33: Consider testing the actual implementation rather than duplicating it.The test file duplicates
ENV_VAR_PATTERNandresolveEnvVarsfromindex.ts. If the source implementation changes (e.g., bug fix or new pattern), these tests will still pass against the stale duplicated code, creating a false sense of coverage.Consider either:
- Exporting
resolveEnvVarsfromindex.ts(or a shared utility) and importing it here- Testing indirectly through the MCP launch flow (integration-style)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/mcp/env-var-interpolation.test.ts` around lines 16 - 33, The test duplicates ENV_VAR_PATTERN and resolveEnvVars; instead export the real resolveEnvVars (and/or ENV_VAR_PATTERN) from index.ts or a shared util and import it into packages/opencode/test/mcp/env-var-interpolation.test.ts so the test exercises the actual implementation; if resolveEnvVars is not exported, add an export (named) in the source module (index.ts or the appropriate utility module) and update the test to import and use that exported resolveEnvVars rather than the duplicated copy, or alternatively replace this unit test with an integration-style test that exercises the MCP launch flow which uses resolveEnvVars.
139-148: Usetmpdir()fromfixture/fixture.tsper coding guidelines.The test uses manual
mkdtemp/rmfor temporary directories. As per coding guidelines, tests should use thetmpdirfunction withawait usingsyntax for automatic cleanup.♻️ Suggested refactor using tmpdir()
+import { tmpdir as createTmpDir } from "../fixture/fixture" + describe("discoverExternalMcp with env-var interpolation", () => { - let tempDir: string const ORIGINAL_ENV = { ...process.env } - beforeEach(async () => { - tempDir = await mkdtemp(path.join(tmpdir(), "mcp-envvar-")) + test("resolves ${VAR} in discovered .vscode/mcp.json environment", async () => { + await using tmp = await createTmpDir() process.env["TEST_MCP_TOKEN"] = "glpat-secret-token" process.env["TEST_MCP_HOST"] = "https://gitlab.internal.com" - }) - - afterEach(async () => { - process.env = { ...ORIGINAL_ENV } - await rm(tempDir, { recursive: true, force: true }) - }) - - test("resolves ${VAR} in discovered .vscode/mcp.json environment", async () => { - await mkdir(path.join(tempDir, ".vscode"), { recursive: true }) + + await mkdir(path.join(tmp.path, ".vscode"), { recursive: true }) // ... rest of test using tmp.path instead of tempDirAs per coding guidelines: "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/env-var-interpolation.test.ts` around lines 139 - 148, Replace the manual mkdtemp/rm setup in the test (the beforeEach/afterEach that sets tempDir and calls mkdtemp and rm) with the tmpdir helper from fixture/fixture.ts and use it via "await using" so the temporary directory is created and automatically cleaned up; update the test to remove manual process.env restore in afterEach if handled elsewhere, and reference the same tempDir variable name in the test body but obtain it from await using tmpdir() instead of mkdtemp, removing the explicit rm call.
🤖 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/env-var-interpolation.test.ts`:
- Around line 16-33: The test duplicates ENV_VAR_PATTERN and resolveEnvVars;
instead export the real resolveEnvVars (and/or ENV_VAR_PATTERN) from index.ts or
a shared util and import it into
packages/opencode/test/mcp/env-var-interpolation.test.ts so the test exercises
the actual implementation; if resolveEnvVars is not exported, add an export
(named) in the source module (index.ts or the appropriate utility module) and
update the test to import and use that exported resolveEnvVars rather than the
duplicated copy, or alternatively replace this unit test with an
integration-style test that exercises the MCP launch flow which uses
resolveEnvVars.
- Around line 139-148: Replace the manual mkdtemp/rm setup in the test (the
beforeEach/afterEach that sets tempDir and calls mkdtemp and rm) with the tmpdir
helper from fixture/fixture.ts and use it via "await using" so the temporary
directory is created and automatically cleaned up; update the test to remove
manual process.env restore in afterEach if handled elsewhere, and reference the
same tempDir variable name in the test body but obtain it from await using
tmpdir() instead of mkdtemp, removing the explicit rm call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c57bcd95-245a-4e8f-a47f-0a3996dada1a
📒 Files selected for processing (3)
packages/opencode/src/mcp/discover.tspackages/opencode/src/mcp/index.tspackages/opencode/test/mcp/env-var-interpolation.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/opencode/test/mcp/env-var-interpolation.test.ts (1)
151-152: Avoidanycasts when asserting discovered server environments.These casts can be replaced with a small typed helper/assertion to keep test type-safety intact.
Typed alternative (example)
- const env = (servers["gitlab"] as any).environment + const env = (servers["gitlab"] as { environment: Record<string, string> }).environmentAlso applies to: 178-179, 202-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/opencode/test/mcp/env-var-interpolation.test.ts` around lines 151 - 152, Replace the unsafe `(servers["gitlab"] as any).environment` casts with a small typed helper to preserve type-safety: add a helper like `getServerEnv(servers, key)` (or `assertServerHasEnvironment`) that accepts the `servers` map and a server key, narrows/validates the server type, and returns the typed `environment` object; then use that helper in the three places where `as any` is used (the assertions currently using `(servers["gitlab"] as any).environment` and the similar occurrences around lines 178-179 and 202-203) so tests assert `env.GITLAB_TOKEN` (and other env keys) against a correctly typed environment rather than using `any`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/test/mcp/env-var-interpolation.test.ts`:
- Around line 3-5: Replace the manual temp-dir lifecycle (calls to mkdtemp, rm,
mkdir, writeFile) in env-var-interpolation.test.ts with the shared tmpdir
fixture: import tmpdir from "fixture/fixture.ts" and in each test use "await
using const dir = tmpdir()" to obtain a temporary directory (then create files
inside dir.path). Remove explicit mkdtemp/rm cleanup and any manual tmpdir
string handling; update tests that currently perform temp setup (the block
around the existing mkdtemp/mkdir/writeFile usage and the similar logic
referenced later) to write files into dir.path and rely on automatic cleanup
when the await-using scoped variable is released.
---
Nitpick comments:
In `@packages/opencode/test/mcp/env-var-interpolation.test.ts`:
- Around line 151-152: Replace the unsafe `(servers["gitlab"] as
any).environment` casts with a small typed helper to preserve type-safety: add a
helper like `getServerEnv(servers, key)` (or `assertServerHasEnvironment`) that
accepts the `servers` map and a server key, narrows/validates the server type,
and returns the typed `environment` object; then use that helper in the three
places where `as any` is used (the assertions currently using
`(servers["gitlab"] as any).environment` and the similar occurrences around
lines 178-179 and 202-203) so tests assert `env.GITLAB_TOKEN` (and other env
keys) against a correctly typed environment rather than using `any`.
🪄 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: 469399ce-ff20-4305-9eac-8752b3d75c9f
📒 Files selected for processing (2)
packages/opencode/src/mcp/index.tspackages/opencode/test/mcp/env-var-interpolation.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/opencode/src/mcp/index.ts
|
Thanks @VJ-yadav . Can you please fix the CI-issue? |
AltimateAI#656) ${VAR}, ${VAR:-default}, and {env:VAR} patterns in MCP server environment blocks were passed as literal strings to child processes, causing auth failures for tools like gitlab-mcp-server. Two gaps fixed: - mcp/index.ts: add resolveEnvVars() safety net at launch site that resolves env-var patterns in mcp.environment before spawning - discover.ts: use ConfigPaths.parseText() in readJsonSafe() so external MCP configs (Claude Code, Cursor, Copilot, Gemini) get interpolation before JSON parsing 14 new tests covering both ${VAR} and {env:VAR} syntax, defaults, escapes, and discovery integration.
Addresses cubic-dev-ai review: tests were exercising a copy of the function, not the production code. Now imports from src/mcp directly.
Addresses coderabbitai review: switched discovery integration tests to use await using tmpdir() from fixture/fixture.ts for automatic cleanup, matching repository test standards.
58b27a9 to
b8ab0d9
Compare
|
@anandgupta42 Rebased onto latest main (v0.5.20) and CI re-ran — same 7 failures, all pre-existing infra flakes unrelated to this PR:
None of our changed files ( |
What does this PR do?
Fixes #656 —
${VAR}and{env:VAR}patterns in MCP serverenvironmentconfig blocks were passed as literal strings to child processes instead of being resolved to actual environment variable values. This caused auth failures for MCP servers expecting tokens (e.g.gitlab-mcp-serverreceiving literal"${GITLAB_PERSONAL_ACCESS_TOKEN}"instead of the token).Root Cause
PR #655 added env-var interpolation to the config parsing pipeline (
ConfigPaths.parseText → substitute()), but two code paths bypassed it:MCP launch site (
mcp/index.ts:512) —mcp.environmentvalues were spread directly into the child process env without resolving any remaining${VAR}patterns. If interpolation failed upstream (config updates viaupdateGlobal(), timing issues), the literal string overwrote the correct value already present inprocess.env.External MCP discovery (
discover.ts:readJsonSafe()) — configs from Claude Code (.claude.json), Cursor (.cursor/mcp.json), VS Code (.vscode/mcp.json), Copilot, and Gemini were parsed viaparseJsonc()directly, completely skipping thesubstitute()interpolation pipeline.Changes
packages/opencode/src/mcp/index.tsresolveEnvVars()safety net that resolves${VAR},${VAR:-default},{env:VAR}, and$${VAR}(escape) patterns inmcp.environmentvalues before spawning the child processpackages/opencode/src/mcp/discover.tsreadJsonSafe()to useConfigPaths.parseText()which runssubstitute()on raw text before JSONC parsing, with graceful fallback to direct parse on failurepackages/opencode/test/mcp/env-var-interpolation.test.tsSupported syntaxes (all three work in MCP environment blocks)
${VAR}"TOKEN": "${GITLAB_TOKEN}"${VAR:-default}"MODE": "${APP_MODE:-production}"{env:VAR}"KEY": "{env:API_KEY}"$${VAR}${VAR}"TPL": "$${VAR}"→"${VAR}"Test plan
resolveEnvVars:${VAR},{env:VAR}, defaults, escapes, unset vars, plain passthrough, multiple vars in one value, mixed entries, bare$VARnot matched, empty object.vscode/mcp.jsonwith${VAR},.cursor/mcp.jsonwith{env:VAR}, fallback defaultsdiscover.test.tstests still passpaths-parsetext.test.tstests still passChecklist
anytypes in new codealtimate_changemarker conventionpaths.ts:substitute()for consistencyConfigPaths.parseText()fails in discovery, falls back to direct parse (no regression for malformed external configs)Fixes #656
Summary by cubic
Fixes env-var interpolation in MCP server environments so
${VAR}placeholders resolve before spawn, preventing literals from reaching child processes. Also applies interpolation to external MCP configs (Cursor, VS Code, Claude Code, Copilot, Gemini). Fixes #656.Bug Fixes
${VAR},${VAR:-default}, and{env:VAR}; support$${VAR}to escape.mcp.environmentis resolved before child process spawn.ConfigPaths.parseText()to interpolate external configs, with fallback to direct parse.resolveEnvVars; 14 tests cover syntax, defaults, escapes, and external discovery.Refactors
tmpdirfixture for automatic cleanup.Written for commit b8ab0d9. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests