From 563a888928c299e1fd1383387e62bb7661ab2013 Mon Sep 17 00:00:00 2001 From: Vijay Yadav Date: Wed, 8 Apr 2026 23:17:22 -0400 Subject: [PATCH 1/3] fix: resolve env-var references in MCP server environment config (closes #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. --- packages/opencode/src/mcp/discover.ts | 12 + packages/opencode/src/mcp/index.ts | 27 ++- .../test/mcp/env-var-interpolation.test.ts | 229 ++++++++++++++++++ 3 files changed, 267 insertions(+), 1 deletion(-) create mode 100644 packages/opencode/test/mcp/env-var-interpolation.test.ts diff --git a/packages/opencode/src/mcp/discover.ts b/packages/opencode/src/mcp/discover.ts index eef0d97236..d56312e952 100644 --- a/packages/opencode/src/mcp/discover.ts +++ b/packages/opencode/src/mcp/discover.ts @@ -117,6 +117,18 @@ async function readJsonSafe(filePath: string): Promise { } catch { return undefined } + // altimate_change start — apply env-var interpolation to external MCP configs + // External configs (Claude Code, Cursor, Copilot, Gemini) may contain ${VAR} + // or {env:VAR} references that need resolving before JSON parsing. + // Uses ConfigPaths.parseText which runs substitute() then parses JSONC. + try { + const { ConfigPaths } = await import("../config/paths") + return await ConfigPaths.parseText(text, filePath, "empty") + } catch { + // Substitution or parse failure — fall back to direct parse without interpolation + log.debug("env-var interpolation failed for external MCP config, falling back to direct parse", { file: filePath }) + } + // altimate_change end const errors: any[] = [] const result = parseJsonc(text, errors, { allowTrailingComma: true }) if (errors.length > 0) { diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index 4c80b0ef83..d195d1535c 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/packages/opencode/src/mcp/index.ts @@ -25,6 +25,29 @@ import { TuiEvent } from "@/cli/cmd/tui/event" import open from "open" import { Telemetry } from "@/telemetry" +// altimate_change start — resolve env-var references in MCP environment values +// Handles ${VAR}, ${VAR:-default}, and {env:VAR} patterns that may have survived +// config parsing (e.g. discovered external configs, config updates via updateGlobal). +const ENV_VAR_PATTERN = + /\$\$(\{[A-Za-z_][A-Za-z0-9_]*(?::-[^}]*)?\})|(?): Record { + const resolved: Record = {} + for (const [key, value] of Object.entries(environment)) { + resolved[key] = value.replace(ENV_VAR_PATTERN, (match, escaped, dollarVar, dollarDefault, braceVar) => { + if (escaped !== undefined) return "$" + escaped + if (dollarVar !== undefined) { + const envValue = process.env[dollarVar] + return envValue !== undefined && envValue !== "" ? envValue : (dollarDefault ?? "") + } + if (braceVar !== undefined) return process.env[braceVar] || "" + return match + }) + } + return resolved +} +// altimate_change end + export namespace MCP { const log = Log.create({ service: "mcp" }) const DEFAULT_TIMEOUT = 30_000 @@ -509,7 +532,9 @@ export namespace MCP { env: { ...process.env, ...(cmd === "altimate" || cmd === "altimate-code" ? { BUN_BE_BUN: "1" } : {}), - ...mcp.environment, + // altimate_change start — resolve ${VAR} / {env:VAR} patterns that survived config parsing + ...(mcp.environment ? resolveEnvVars(mcp.environment) : {}), + // altimate_change end }, }) transport.stderr?.on("data", (chunk: Buffer) => { diff --git a/packages/opencode/test/mcp/env-var-interpolation.test.ts b/packages/opencode/test/mcp/env-var-interpolation.test.ts new file mode 100644 index 0000000000..61b05e65d4 --- /dev/null +++ b/packages/opencode/test/mcp/env-var-interpolation.test.ts @@ -0,0 +1,229 @@ +// altimate_change start — tests for MCP env-var interpolation (closes #656) +import { describe, test, expect, beforeEach, afterEach } from "bun:test" +import { mkdtemp, rm, mkdir, writeFile } from "fs/promises" +import { tmpdir } from "os" +import path from "path" + +// ------------------------------------------------------------------------- +// resolveEnvVars — safety-net resolver at MCP launch site +// ------------------------------------------------------------------------- + +// Import the module to access resolveEnvVars indirectly via the env spread. +// Since resolveEnvVars is a module-level function (not inside the MCP namespace), +// we test it directly by importing the file and extracting the function. +// For now, inline the same logic here for unit testing. + +const ENV_VAR_PATTERN = + /\$\$(\{[A-Za-z_][A-Za-z0-9_]*(?::-[^}]*)?\})|(?): Record { + const resolved: Record = {} + for (const [key, value] of Object.entries(environment)) { + resolved[key] = value.replace(ENV_VAR_PATTERN, (match, escaped, dollarVar, dollarDefault, braceVar) => { + if (escaped !== undefined) return "$" + escaped + if (dollarVar !== undefined) { + const envValue = process.env[dollarVar] + return envValue !== undefined && envValue !== "" ? envValue : (dollarDefault ?? "") + } + if (braceVar !== undefined) return process.env[braceVar] || "" + return match + }) + } + return resolved +} + +describe("resolveEnvVars", () => { + const ORIGINAL_ENV = { ...process.env } + + beforeEach(() => { + process.env["TEST_TOKEN"] = "secret-123" + process.env["TEST_HOST"] = "gitlab.example.com" + }) + + afterEach(() => { + process.env = { ...ORIGINAL_ENV } + }) + + test("resolves ${VAR} syntax", () => { + const result = resolveEnvVars({ + API_TOKEN: "${TEST_TOKEN}", + HOST: "${TEST_HOST}", + }) + expect(result.API_TOKEN).toBe("secret-123") + expect(result.HOST).toBe("gitlab.example.com") + }) + + test("resolves {env:VAR} syntax", () => { + const result = resolveEnvVars({ + API_TOKEN: "{env:TEST_TOKEN}", + }) + expect(result.API_TOKEN).toBe("secret-123") + }) + + test("resolves ${VAR:-default} with fallback when unset", () => { + const result = resolveEnvVars({ + MODE: "${UNSET_VAR:-production}", + }) + expect(result.MODE).toBe("production") + }) + + test("resolves ${VAR:-default} to env value when set", () => { + const result = resolveEnvVars({ + TOKEN: "${TEST_TOKEN:-fallback}", + }) + expect(result.TOKEN).toBe("secret-123") + }) + + test("preserves $${VAR} as literal ${VAR}", () => { + const result = resolveEnvVars({ + TEMPLATE: "$${TEST_TOKEN}", + }) + expect(result.TEMPLATE).toBe("${TEST_TOKEN}") + }) + + test("resolves unset variable to empty string", () => { + const result = resolveEnvVars({ + MISSING: "${COMPLETELY_UNSET_VAR_XYZ}", + }) + expect(result.MISSING).toBe("") + }) + + test("passes through plain values without modification", () => { + const result = resolveEnvVars({ + PLAIN: "just-a-string", + URL: "https://gitlab.com/api/v4", + }) + expect(result.PLAIN).toBe("just-a-string") + expect(result.URL).toBe("https://gitlab.com/api/v4") + }) + + test("resolves multiple variables in a single value", () => { + const result = resolveEnvVars({ + URL: "https://${TEST_HOST}/api?token=${TEST_TOKEN}", + }) + expect(result.URL).toBe("https://gitlab.example.com/api?token=secret-123") + }) + + test("handles mixed resolved and plain entries", () => { + const result = resolveEnvVars({ + TOKEN: "${TEST_TOKEN}", + STATIC_URL: "https://gitlab.com/api/v4", + HOST: "{env:TEST_HOST}", + }) + expect(result.TOKEN).toBe("secret-123") + expect(result.STATIC_URL).toBe("https://gitlab.com/api/v4") + expect(result.HOST).toBe("gitlab.example.com") + }) + + test("does not interpolate bare $VAR without braces", () => { + const result = resolveEnvVars({ + TOKEN: "$TEST_TOKEN", + }) + expect(result.TOKEN).toBe("$TEST_TOKEN") + }) + + test("handles empty environment object", () => { + const result = resolveEnvVars({}) + expect(result).toEqual({}) + }) +}) + +// ------------------------------------------------------------------------- +// Discovery integration — env vars in external MCP configs +// ------------------------------------------------------------------------- + +describe("discoverExternalMcp with env-var interpolation", () => { + let tempDir: string + const ORIGINAL_ENV = { ...process.env } + + beforeEach(async () => { + tempDir = await mkdtemp(path.join(tmpdir(), "mcp-envvar-")) + 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 writeFile( + path.join(tempDir, ".vscode/mcp.json"), + JSON.stringify({ + servers: { + gitlab: { + command: "node", + args: ["gitlab-server.js"], + env: { + GITLAB_TOKEN: "${TEST_MCP_TOKEN}", + GITLAB_HOST: "${TEST_MCP_HOST}", + STATIC_VALUE: "no-interpolation-needed", + }, + }, + }, + }), + ) + + const { discoverExternalMcp } = await import("../../src/mcp/discover") + const { servers } = await discoverExternalMcp(tempDir) + + expect(servers["gitlab"]).toBeDefined() + expect(servers["gitlab"].type).toBe("local") + const env = (servers["gitlab"] as any).environment + expect(env.GITLAB_TOKEN).toBe("glpat-secret-token") + expect(env.GITLAB_HOST).toBe("https://gitlab.internal.com") + expect(env.STATIC_VALUE).toBe("no-interpolation-needed") + }) + + test("resolves {env:VAR} in discovered .cursor/mcp.json environment", async () => { + await mkdir(path.join(tempDir, ".cursor"), { recursive: true }) + await writeFile( + path.join(tempDir, ".cursor/mcp.json"), + JSON.stringify({ + mcpServers: { + "my-tool": { + command: "npx", + args: ["-y", "my-mcp-tool"], + env: { + API_KEY: "{env:TEST_MCP_TOKEN}", + }, + }, + }, + }), + ) + + const { discoverExternalMcp } = await import("../../src/mcp/discover") + const { servers } = await discoverExternalMcp(tempDir) + + expect(servers["my-tool"]).toBeDefined() + const env = (servers["my-tool"] as any).environment + expect(env.API_KEY).toBe("glpat-secret-token") + }) + + test("resolves ${VAR:-default} with fallback in discovered config", async () => { + await mkdir(path.join(tempDir, ".vscode"), { recursive: true }) + await writeFile( + path.join(tempDir, ".vscode/mcp.json"), + JSON.stringify({ + servers: { + svc: { + command: "node", + args: ["svc.js"], + env: { + MODE: "${UNSET_VAR_ABC:-production}", + }, + }, + }, + }), + ) + + const { discoverExternalMcp } = await import("../../src/mcp/discover") + const { servers } = await discoverExternalMcp(tempDir) + + const env = (servers["svc"] as any).environment + expect(env.MODE).toBe("production") + }) +}) +// altimate_change end From f7f5ca291c455f0adc195a16ae30280b82c97989 Mon Sep 17 00:00:00 2001 From: Vijay Yadav Date: Wed, 8 Apr 2026 23:26:03 -0400 Subject: [PATCH 2/3] fix: export resolveEnvVars and import in tests instead of duplicating Addresses cubic-dev-ai review: tests were exercising a copy of the function, not the production code. Now imports from src/mcp directly. --- packages/opencode/src/mcp/index.ts | 2 +- .../test/mcp/env-var-interpolation.test.ts | 25 +------------------ 2 files changed, 2 insertions(+), 25 deletions(-) diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index d195d1535c..4042b984d0 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/packages/opencode/src/mcp/index.ts @@ -31,7 +31,7 @@ import { Telemetry } from "@/telemetry" const ENV_VAR_PATTERN = /\$\$(\{[A-Za-z_][A-Za-z0-9_]*(?::-[^}]*)?\})|(?): Record { +export function resolveEnvVars(environment: Record): Record { const resolved: Record = {} for (const [key, value] of Object.entries(environment)) { resolved[key] = value.replace(ENV_VAR_PATTERN, (match, escaped, dollarVar, dollarDefault, braceVar) => { diff --git a/packages/opencode/test/mcp/env-var-interpolation.test.ts b/packages/opencode/test/mcp/env-var-interpolation.test.ts index 61b05e65d4..7501cd3a2f 100644 --- a/packages/opencode/test/mcp/env-var-interpolation.test.ts +++ b/packages/opencode/test/mcp/env-var-interpolation.test.ts @@ -3,35 +3,12 @@ import { describe, test, expect, beforeEach, afterEach } from "bun:test" import { mkdtemp, rm, mkdir, writeFile } from "fs/promises" import { tmpdir } from "os" import path from "path" +import { resolveEnvVars } from "../../src/mcp" // ------------------------------------------------------------------------- // resolveEnvVars — safety-net resolver at MCP launch site // ------------------------------------------------------------------------- -// Import the module to access resolveEnvVars indirectly via the env spread. -// Since resolveEnvVars is a module-level function (not inside the MCP namespace), -// we test it directly by importing the file and extracting the function. -// For now, inline the same logic here for unit testing. - -const ENV_VAR_PATTERN = - /\$\$(\{[A-Za-z_][A-Za-z0-9_]*(?::-[^}]*)?\})|(?): Record { - const resolved: Record = {} - for (const [key, value] of Object.entries(environment)) { - resolved[key] = value.replace(ENV_VAR_PATTERN, (match, escaped, dollarVar, dollarDefault, braceVar) => { - if (escaped !== undefined) return "$" + escaped - if (dollarVar !== undefined) { - const envValue = process.env[dollarVar] - return envValue !== undefined && envValue !== "" ? envValue : (dollarDefault ?? "") - } - if (braceVar !== undefined) return process.env[braceVar] || "" - return match - }) - } - return resolved -} - describe("resolveEnvVars", () => { const ORIGINAL_ENV = { ...process.env } From b8ab0d958649ddaae07360c9c23e8b92908b3d5e Mon Sep 17 00:00:00 2001 From: Vijay Yadav Date: Wed, 8 Apr 2026 23:39:50 -0400 Subject: [PATCH 3/3] refactor: use shared tmpdir fixture instead of manual temp-dir lifecycle Addresses coderabbitai review: switched discovery integration tests to use await using tmpdir() from fixture/fixture.ts for automatic cleanup, matching repository test standards. --- .../test/mcp/env-var-interpolation.test.ts | 35 ++++++++++--------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/packages/opencode/test/mcp/env-var-interpolation.test.ts b/packages/opencode/test/mcp/env-var-interpolation.test.ts index 7501cd3a2f..a79ef40be5 100644 --- a/packages/opencode/test/mcp/env-var-interpolation.test.ts +++ b/packages/opencode/test/mcp/env-var-interpolation.test.ts @@ -1,9 +1,9 @@ // altimate_change start — tests for MCP env-var interpolation (closes #656) import { describe, test, expect, beforeEach, afterEach } from "bun:test" -import { mkdtemp, rm, mkdir, writeFile } from "fs/promises" -import { tmpdir } from "os" +import { mkdir, writeFile } from "fs/promises" import path from "path" import { resolveEnvVars } from "../../src/mcp" +import { tmpdir } from "../fixture/fixture" // ------------------------------------------------------------------------- // resolveEnvVars — safety-net resolver at MCP launch site @@ -110,24 +110,23 @@ describe("resolveEnvVars", () => { // ------------------------------------------------------------------------- describe("discoverExternalMcp with env-var interpolation", () => { - let tempDir: string const ORIGINAL_ENV = { ...process.env } - beforeEach(async () => { - tempDir = await mkdtemp(path.join(tmpdir(), "mcp-envvar-")) + beforeEach(() => { process.env["TEST_MCP_TOKEN"] = "glpat-secret-token" process.env["TEST_MCP_HOST"] = "https://gitlab.internal.com" }) - afterEach(async () => { + afterEach(() => { 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 using tmp = await tmpdir() + const dir = tmp.path + await mkdir(path.join(dir, ".vscode"), { recursive: true }) await writeFile( - path.join(tempDir, ".vscode/mcp.json"), + path.join(dir, ".vscode/mcp.json"), JSON.stringify({ servers: { gitlab: { @@ -144,7 +143,7 @@ describe("discoverExternalMcp with env-var interpolation", () => { ) const { discoverExternalMcp } = await import("../../src/mcp/discover") - const { servers } = await discoverExternalMcp(tempDir) + const { servers } = await discoverExternalMcp(dir) expect(servers["gitlab"]).toBeDefined() expect(servers["gitlab"].type).toBe("local") @@ -155,9 +154,11 @@ describe("discoverExternalMcp with env-var interpolation", () => { }) test("resolves {env:VAR} in discovered .cursor/mcp.json environment", async () => { - await mkdir(path.join(tempDir, ".cursor"), { recursive: true }) + await using tmp = await tmpdir() + const dir = tmp.path + await mkdir(path.join(dir, ".cursor"), { recursive: true }) await writeFile( - path.join(tempDir, ".cursor/mcp.json"), + path.join(dir, ".cursor/mcp.json"), JSON.stringify({ mcpServers: { "my-tool": { @@ -172,7 +173,7 @@ describe("discoverExternalMcp with env-var interpolation", () => { ) const { discoverExternalMcp } = await import("../../src/mcp/discover") - const { servers } = await discoverExternalMcp(tempDir) + const { servers } = await discoverExternalMcp(dir) expect(servers["my-tool"]).toBeDefined() const env = (servers["my-tool"] as any).environment @@ -180,9 +181,11 @@ describe("discoverExternalMcp with env-var interpolation", () => { }) test("resolves ${VAR:-default} with fallback in discovered config", async () => { - await mkdir(path.join(tempDir, ".vscode"), { recursive: true }) + await using tmp = await tmpdir() + const dir = tmp.path + await mkdir(path.join(dir, ".vscode"), { recursive: true }) await writeFile( - path.join(tempDir, ".vscode/mcp.json"), + path.join(dir, ".vscode/mcp.json"), JSON.stringify({ servers: { svc: { @@ -197,7 +200,7 @@ describe("discoverExternalMcp with env-var interpolation", () => { ) const { discoverExternalMcp } = await import("../../src/mcp/discover") - const { servers } = await discoverExternalMcp(tempDir) + const { servers } = await discoverExternalMcp(dir) const env = (servers["svc"] as any).environment expect(env.MODE).toBe("production")