From 1a40b4faa44ce586c39e3a7a507752d4980e9a27 Mon Sep 17 00:00:00 2001 From: Vijay Yadav Date: Sun, 10 May 2026 17:03:24 -0400 Subject: [PATCH] fix: [#701] surface unresolved MCP env-var warnings to user via toast MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An unresolved `${VAR}` in an MCP server's env/headers block silently became `""`, causing the server to launch with empty credentials and fail downstream with a confusing error. The warning was logged but most users never see the log. This adds a per-server accumulator in `mcp/discover.ts` that captures unresolved env-var references during discovery, plus a one-shot `consumeUnresolvedEnvVars()` API consumed by `MCP.state` initializer, which publishes a `TuiEvent.ToastShow` warning naming the affected servers and missing variables and pointing users at `${VAR:-default}` for explicit fallbacks. Adds an `mcp_unresolved_env_vars` telemetry event to track recovery rate. Scope: external MCP configs only (`.vscode/mcp.json`, `.cursor/mcp.json`, `.mcp.json`, `~/.claude.json`, etc.), where each server has direct attribution. Unresolved vars in `opencode.json`'s `mcp.environment` go through `ConfigPaths.substitute()` at file level and are tracked in existing telemetry; surfacing those to a toast needs file→server attribution and is left as follow-up. --- .../opencode/src/altimate/telemetry/index.ts | 11 + packages/opencode/src/mcp/discover.ts | 41 ++- packages/opencode/src/mcp/index.ts | 47 ++- .../test/mcp/discover-unresolved-env.test.ts | 276 ++++++++++++++++++ 4 files changed, 373 insertions(+), 2 deletions(-) create mode 100644 packages/opencode/test/mcp/discover-unresolved-env.test.ts diff --git a/packages/opencode/src/altimate/telemetry/index.ts b/packages/opencode/src/altimate/telemetry/index.ts index b13837597..0cb2a0f76 100644 --- a/packages/opencode/src/altimate/telemetry/index.ts +++ b/packages/opencode/src/altimate/telemetry/index.ts @@ -323,6 +323,17 @@ export namespace Telemetry { server_names: string[] sources: string[] } + | { + // altimate_change — issue #701: track when user is shown the unresolved + // MCP env-var warning toast (so we can measure whether the toast actually + // helps users self-recover vs. landing in the failed-server flow). + type: "mcp_unresolved_env_vars" + timestamp: number + session_id: string + server_count: number + var_count: number + servers: string[] + } | { type: "memory_operation" timestamp: number diff --git a/packages/opencode/src/mcp/discover.ts b/packages/opencode/src/mcp/discover.ts index 916f10ac3..67f376039 100644 --- a/packages/opencode/src/mcp/discover.ts +++ b/packages/opencode/src/mcp/discover.ts @@ -16,6 +16,20 @@ const log = Log.create({ service: "mcp.discover" }) // that the launch site does NOT need a second resolution pass. // See PR #666 review — double-interpolation regression fixed by doing this once, // here, rather than twice. + +/** + * Per-server unresolved env-var accumulator. Populated during discovery, + * drained once at MCP startup so the user gets a visible warning toast + * instead of a silently-empty `${VAR}` becoming `""`. See issue #701. + */ +export interface UnresolvedEnvVarRecord { + server: string + source: string + field: "env" | "headers" + vars: string[] +} +let _unresolvedEnvVars: UnresolvedEnvVarRecord[] = [] + function resolveServerEnvVars( obj: Record, context: { server: string; source: string; field: "env" | "headers" }, @@ -27,11 +41,19 @@ function resolveServerEnvVars( out[key] = ConfigPaths.resolveEnvVarsInString(raw, stats) } if (stats.unresolvedNames.length > 0) { + // Dedupe — the same VAR can appear in multiple values within one block. + const dedupedVars = Array.from(new Set(stats.unresolvedNames)) log.warn("unresolved env var references in MCP config — substituting empty string", { server: context.server, source: context.source, field: context.field, - unresolved: stats.unresolvedNames.join(", "), + unresolved: dedupedVars.join(", "), + }) + _unresolvedEnvVars.push({ + server: context.server, + source: context.source, + field: context.field, + vars: dedupedVars, }) } return out @@ -229,6 +251,9 @@ export async function discoverExternalMcp(worktree: string): Promise<{ sources: string[] }> { log.info("Discovering MCP servers from external AI tool configs...") + // altimate_change — reset per-discovery accumulator so a re-discovery (e.g. config + // reload) does not show stale warnings from a prior run. See issue #701. + _unresolvedEnvVars = [] const result: Record = Object.create(null) const contributingSources: string[] = [] const homedir = os.homedir() @@ -283,3 +308,17 @@ export function consumeDiscoveryResult() { _lastDiscovery = null return result } + +// altimate_change start — issue #701: surface unresolved env-var warnings to user +/** + * Returns and clears the unresolved env-var records accumulated during the most + * recent `discoverExternalMcp()` call. Drained once by MCP startup so the user + * sees a single warning toast per server instead of a silent empty-string + * substitution that fails downstream with a confusing error. + */ +export function consumeUnresolvedEnvVars(): UnresolvedEnvVarRecord[] { + const result = _unresolvedEnvVars + _unresolvedEnvVars = [] + return result +} +// altimate_change end diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index b110467ce..84a48860d 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/packages/opencode/src/mcp/index.ts @@ -186,9 +186,12 @@ export namespace MCP { // altimate_change start — auto-discover MCP servers from external AI tool configs let discoveryResult: { serverNames: string[]; sources: string[] } | null = null + // altimate_change — issue #701: surface unresolved env-var warnings to user + let unresolvedEnvVars: Awaited> = [] try { - const { consumeDiscoveryResult } = await import("./discover") + const { consumeDiscoveryResult, consumeUnresolvedEnvVars } = await import("./discover") discoveryResult = consumeDiscoveryResult() + unresolvedEnvVars = consumeUnresolvedEnvVars() } catch { // Discovery module not loaded — skip } @@ -242,6 +245,48 @@ export namespace MCP { } // altimate_change end + // altimate_change start — issue #701: surface unresolved env-var warnings to user + // An unresolved `${VAR}` in an MCP server's env/headers block silently becomes "", + // which then fails downstream with a confusing error. Show a warning toast naming + // the server and the missing vars so the user can set/export them and reload. + if (unresolvedEnvVars.length > 0) { + // Group by server so the message stays readable when one config has multiple + // unresolved vars across env + headers (e.g. a Snowflake remote server). + const grouped = new Map }>() + for (const record of unresolvedEnvVars) { + const key = record.server + const existing = grouped.get(key) + if (existing) { + for (const v of record.vars) existing.vars.add(v) + } else { + grouped.set(key, { source: record.source, vars: new Set(record.vars) }) + } + } + const lines = Array.from(grouped.entries()).map( + ([server, { source, vars }]) => + `${server} (${source}): ${Array.from(vars).join(", ")}`, + ) + Bus.publish(TuiEvent.ToastShow, { + title: "MCP env vars unresolved", + message: + `Substituted "" for these — server may fail to start. Set the env vars and reload, ` + + `or use \${VAR:-default} for an explicit fallback.\n` + + lines.join("\n"), + variant: "warning", + duration: 12000, + }).catch(() => {}) + const totalVars = Array.from(grouped.values()).reduce((sum, g) => sum + g.vars.size, 0) + Telemetry.track({ + type: "mcp_unresolved_env_vars", + timestamp: Date.now(), + session_id: Telemetry.getContext().sessionId, + server_count: grouped.size, + var_count: totalVars, + servers: Array.from(grouped.keys()), + }) + } + // altimate_change end + return { status, clients, diff --git a/packages/opencode/test/mcp/discover-unresolved-env.test.ts b/packages/opencode/test/mcp/discover-unresolved-env.test.ts new file mode 100644 index 000000000..cb0f82f34 --- /dev/null +++ b/packages/opencode/test/mcp/discover-unresolved-env.test.ts @@ -0,0 +1,276 @@ +// altimate_change start — tests for issue #701: surface unresolved MCP env-var warnings +// Verifies the per-discovery accumulator correctly captures, dedupes, and resets +// unresolved ${VAR} references found in discovered MCP server env/headers blocks. +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 { consumeUnresolvedEnvVars, discoverExternalMcp } from "../../src/mcp/discover" + +let tempDir: string + +beforeEach(async () => { + tempDir = await mkdtemp(path.join(tmpdir(), "mcp-unresolved-env-")) + // Drain anything left over from prior tests so we start clean. + consumeUnresolvedEnvVars() + delete process.env["UNRESOLVED_TEST_VAR_A"] + delete process.env["UNRESOLVED_TEST_VAR_B"] + delete process.env["UNRESOLVED_TEST_TOKEN"] +}) + +afterEach(async () => { + await rm(tempDir, { recursive: true, force: true }) + // Empty the accumulator so a failed test doesn't poison the next one. + consumeUnresolvedEnvVars() + delete process.env["UNRESOLVED_TEST_VAR_A"] + delete process.env["UNRESOLVED_TEST_VAR_B"] + delete process.env["UNRESOLVED_TEST_TOKEN"] +}) + +describe("consumeUnresolvedEnvVars (issue #701)", () => { + test("captures unresolved ${VAR} in a local server's env block", async () => { + await mkdir(path.join(tempDir, ".vscode"), { recursive: true }) + await writeFile( + path.join(tempDir, ".vscode/mcp.json"), + JSON.stringify({ + servers: { + snowflake: { + command: "snowflake-mcp", + env: { + SNOWFLAKE_ACCOUNT: "myaccount", + SNOWFLAKE_PASSWORD: "${UNRESOLVED_TEST_TOKEN}", + }, + }, + }, + }), + ) + + await discoverExternalMcp(tempDir) + const records = consumeUnresolvedEnvVars() + + expect(records).toHaveLength(1) + expect(records[0]).toMatchObject({ + server: "snowflake", + field: "env", + vars: ["UNRESOLVED_TEST_TOKEN"], + }) + expect(records[0].source).toContain(".vscode/mcp.json") + }) + + test("captures unresolved ${VAR} in a remote server's headers block", async () => { + await writeFile( + path.join(tempDir, ".mcp.json"), + JSON.stringify({ + mcpServers: { + api: { + url: "https://example.com/mcp", + headers: { Authorization: "Bearer ${UNRESOLVED_TEST_TOKEN}" }, + }, + }, + }), + ) + + await discoverExternalMcp(tempDir) + const records = consumeUnresolvedEnvVars() + + expect(records).toHaveLength(1) + expect(records[0]).toMatchObject({ + server: "api", + field: "headers", + vars: ["UNRESOLVED_TEST_TOKEN"], + }) + }) + + test("dedupes when the same VAR appears in multiple keys of one block", async () => { + await writeFile( + path.join(tempDir, ".mcp.json"), + JSON.stringify({ + mcpServers: { + srv: { + command: "node", + env: { + PRIMARY: "${UNRESOLVED_TEST_VAR_A}", + SECONDARY: "${UNRESOLVED_TEST_VAR_A}-suffix", + }, + }, + }, + }), + ) + + await discoverExternalMcp(tempDir) + const records = consumeUnresolvedEnvVars() + + expect(records).toHaveLength(1) + expect(records[0].vars).toEqual(["UNRESOLVED_TEST_VAR_A"]) + }) + + test("captures multiple distinct vars in one block", async () => { + await writeFile( + path.join(tempDir, ".mcp.json"), + JSON.stringify({ + mcpServers: { + srv: { + command: "node", + env: { + A: "${UNRESOLVED_TEST_VAR_A}", + B: "${UNRESOLVED_TEST_VAR_B}", + }, + }, + }, + }), + ) + + await discoverExternalMcp(tempDir) + const records = consumeUnresolvedEnvVars() + + expect(records).toHaveLength(1) + expect(records[0].vars.sort()).toEqual(["UNRESOLVED_TEST_VAR_A", "UNRESOLVED_TEST_VAR_B"]) + }) + + test("does NOT capture ${VAR:-default} — default makes it intentional", async () => { + await writeFile( + path.join(tempDir, ".mcp.json"), + JSON.stringify({ + mcpServers: { + srv: { + command: "node", + env: { LOG_LEVEL: "${UNRESOLVED_TEST_VAR_A:-info}" }, + }, + }, + }), + ) + + await discoverExternalMcp(tempDir) + const records = consumeUnresolvedEnvVars() + + expect(records).toHaveLength(0) + }) + + test("does NOT capture $${VAR} — escaped is a literal, not a reference", async () => { + await writeFile( + path.join(tempDir, ".mcp.json"), + JSON.stringify({ + mcpServers: { + srv: { + command: "node", + env: { TEMPLATE: "$${UNRESOLVED_TEST_VAR_A}" }, + }, + }, + }), + ) + + await discoverExternalMcp(tempDir) + const records = consumeUnresolvedEnvVars() + + expect(records).toHaveLength(0) + }) + + test("does NOT capture when the env var is set", async () => { + process.env["UNRESOLVED_TEST_TOKEN"] = "actual-value" + await writeFile( + path.join(tempDir, ".mcp.json"), + JSON.stringify({ + mcpServers: { + srv: { + command: "node", + env: { TOKEN: "${UNRESOLVED_TEST_TOKEN}" }, + }, + }, + }), + ) + + await discoverExternalMcp(tempDir) + const records = consumeUnresolvedEnvVars() + + expect(records).toHaveLength(0) + }) + + test("captures separate records for env and headers in the same server", async () => { + await writeFile( + path.join(tempDir, ".mcp.json"), + JSON.stringify({ + mcpServers: { + mixed: { + url: "https://example.com/mcp", + headers: { Authorization: "Bearer ${UNRESOLVED_TEST_VAR_A}" }, + }, + local: { + command: "node", + env: { TOKEN: "${UNRESOLVED_TEST_VAR_B}" }, + }, + }, + }), + ) + + await discoverExternalMcp(tempDir) + const records = consumeUnresolvedEnvVars() + + expect(records).toHaveLength(2) + const byServer = Object.fromEntries(records.map((r) => [r.server, r])) + expect(byServer["mixed"]).toMatchObject({ field: "headers", vars: ["UNRESOLVED_TEST_VAR_A"] }) + expect(byServer["local"]).toMatchObject({ field: "env", vars: ["UNRESOLVED_TEST_VAR_B"] }) + }) + + test("consume is one-shot: a second call returns an empty array", async () => { + await writeFile( + path.join(tempDir, ".mcp.json"), + JSON.stringify({ + mcpServers: { + srv: { command: "node", env: { T: "${UNRESOLVED_TEST_TOKEN}" } }, + }, + }), + ) + + await discoverExternalMcp(tempDir) + expect(consumeUnresolvedEnvVars()).toHaveLength(1) + expect(consumeUnresolvedEnvVars()).toHaveLength(0) + }) + + test("each discovery run resets the accumulator (no stale records)", async () => { + await writeFile( + path.join(tempDir, ".mcp.json"), + JSON.stringify({ + mcpServers: { + srv: { command: "node", env: { T: "${UNRESOLVED_TEST_TOKEN}" } }, + }, + }), + ) + await discoverExternalMcp(tempDir) + // Intentionally don't consume — simulate a missed drain. + + // Now run a clean discovery in a fresh dir. + const cleanDir = await mkdtemp(path.join(tmpdir(), "mcp-clean-")) + await writeFile( + path.join(cleanDir, ".mcp.json"), + JSON.stringify({ + mcpServers: { + ok: { command: "node", env: { LITERAL: "no-vars-here" } }, + }, + }), + ) + try { + await discoverExternalMcp(cleanDir) + // The first run's record must NOT carry over. + expect(consumeUnresolvedEnvVars()).toHaveLength(0) + } finally { + await rm(cleanDir, { recursive: true, force: true }) + } + }) + + test("records the source label for project-scoped configs", async () => { + await mkdir(path.join(tempDir, ".cursor"), { recursive: true }) + await writeFile( + path.join(tempDir, ".cursor/mcp.json"), + JSON.stringify({ + mcpServers: { + srv: { command: "node", env: { T: "${UNRESOLVED_TEST_TOKEN}" } }, + }, + }), + ) + + await discoverExternalMcp(tempDir) + const records = consumeUnresolvedEnvVars() + expect(records[0].source).toBe(".cursor/mcp.json") + }) +}) +// altimate_change end