From 60468f738957079d4a9450bec95b5bdb1642bf5a Mon Sep 17 00:00:00 2001 From: Haider Date: Thu, 27 Aug 2026 00:47:43 +0530 Subject: [PATCH] fix: surface the real reason an MCP server is unavailable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects where the diagnostic information already exists in the process and is discarded before it reaches the user. `server unavailable` logged `status.status` — the constant string `"failed"` on that branch — and dropped `status.error`, the field holding the actual message (`401 Unauthorized`, a transport error, `Invalid MCP URL for ""`). Extracted `unavailableLogFields()` as a pure function so the payload is testable without standing up a transport, and so a later edit cannot quietly drop the field again. Environment variables that resolve to empty were never named. A `{env:VAR}` with nothing set becomes `""`, the config parses clean, and the server launches with a blank credential — usually a password — failing later with an error naming neither the variable nor the file. The names are now recorded at both substitution sites: per-server for discovered external configs, per-file for the main config. They surface in `/mcps` and `mcp list`, shown even when the server reports connected, because a blank credential often connects and fails on first real use. An unresolved bare `${VAR}` is deliberately left literal by the config layer so a later runtime layer can fill it (the bedrock provider fills `${AWS_REGION}` from the effective region). That case is not reported. Closes #1121 Closes #701 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018fJ3X7pcGT4R9yzjsJnqsV --- packages/opencode/src/cli/cmd/mcp.ts | 22 ++++ packages/opencode/src/config/variable.ts | 29 ++++- packages/opencode/src/mcp/discover.ts | 19 +++ packages/opencode/src/mcp/index.ts | 22 +++- packages/opencode/src/session/prompt.ts | 23 +++- .../test/cli/mcp-env-diagnostics.test.ts | 119 ++++++++++++++++++ .../opencode/test/mcp/unavailable-log.test.ts | 43 +++++++ .../test/session/mcps-command.test.ts | 34 +++++ 8 files changed, 304 insertions(+), 7 deletions(-) create mode 100644 packages/opencode/test/cli/mcp-env-diagnostics.test.ts create mode 100644 packages/opencode/test/mcp/unavailable-log.test.ts diff --git a/packages/opencode/src/cli/cmd/mcp.ts b/packages/opencode/src/cli/cmd/mcp.ts index 06a1178dec..6739ca22a6 100644 --- a/packages/opencode/src/cli/cmd/mcp.ts +++ b/packages/opencode/src/cli/cmd/mcp.ts @@ -9,6 +9,10 @@ import { LATEST_PROTOCOL_VERSION } from "@modelcontextprotocol/sdk/types.js" import * as prompts from "@clack/prompts" import { UI } from "../ui" import { MCP } from "../../mcp" +// altimate_change start — upstream_fix (#701): env diagnostics surfaced by `mcp list`. +import * as McpDiscover from "../../mcp/discover" +import { ConfigVariable } from "../../config/variable" +// altimate_change end import { McpAuth } from "../../mcp/auth" import { McpOAuthProvider } from "../../mcp/oauth-provider" import { Config } from "@/config/config" @@ -167,12 +171,30 @@ export const McpListCommand = effectCmd({ hint = "\n " + status.error } + // altimate_change start — upstream_fix (#701): name variables that resolved to "". + // A blank `${SNOWFLAKE_PASSWORD}` often connects and only fails on first real use, so + // this is appended regardless of status rather than only on the failure branch. + const unresolved = McpDiscover.unresolvedEnvVars(name) + if (unresolved.length > 0) { + hint += "\n unresolved env: " + unresolved.join(", ") + " (set or remove)" + } + // altimate_change end + const typeHint = serverConfig.type === "remote" ? serverConfig.url : serverConfig.command.join(" ") prompts.log.info( `${statusIcon} ${name} ${UI.Style.TEXT_DIM}${statusText}${hint}\n ${UI.Style.TEXT_DIM}${typeHint}`, ) } + // altimate_change start — upstream_fix (#701): a missing `{env:VAR}` becomes "" and the config + // parses clean, so a blank credential reaches the server and fails much later with an error + // naming neither. Attribution to a single server is not available here (substitution runs on + // raw config text, before any structure exists), so this is reported against the file. + for (const { source, names } of ConfigVariable.blankedEnvVars()) { + prompts.log.warn(`${names.join(", ")} resolved to empty in ${source} (set or remove)`) + } + // altimate_change end + prompts.outro(`${servers.length} server(s)`) }), }) diff --git a/packages/opencode/src/config/variable.ts b/packages/opencode/src/config/variable.ts index 4c989cacd0..009a576334 100644 --- a/packages/opencode/src/config/variable.ts +++ b/packages/opencode/src/config/variable.ts @@ -28,6 +28,22 @@ type SubstituteInput = ParseSource & { // altimate_change end } +// altimate_change start — upstream_fix (#701): keep the names of variables that silently blanked. +// An unresolved bare `${VAR}` is left LITERAL above on purpose, so it stays visible and is not +// recorded here. `{env:VAR}` has no such deferral — it becomes "" and the config parses clean, so +// a missing `{env:SNOWFLAKE_PASSWORD}` launches an MCP server with a blank credential and fails +// later with an error naming neither the variable nor this file. Keyed by config source; the +// newest parse of a file replaces its entry so a fixed variable stops being reported. +const _blankedEnv = new Map>() + +/** Variable names that silently became "" during config substitution, grouped by config source. */ +export function blankedEnvVars(): { source: string; names: string[] }[] { + return [..._blankedEnv.entries()] + .map(([src, names]) => ({ source: src, names: [...names].sort() })) + .sort((a, b) => a.source.localeCompare(b.source)) +} +// altimate_change end + function source(input: ParseSource) { return input.type === "path" ? input.path : input.source } @@ -42,6 +58,9 @@ export async function substitute(input: SubstituteInput) { // altimate_change start — upstream_fix: restore ${VAR}/${VAR:-default}/$${VAR} config interpolation const format = input.format ?? "json" const encode = (value: string) => (format === "raw" ? value : JSON.stringify(value).slice(1, -1)) + // altimate_change — upstream_fix (#701): collect blanked names for this parse, replacing any + // earlier entry for the same source rather than accumulating stale ones. + const blanked = new Set() let text = input.text.replace(ConfigPaths.ENV_VAR_PATTERN, (match, escaped, dollarVar, dollarDefault, braceVar) => { if (escaped !== undefined) return "$" + escaped if (dollarVar !== undefined) { @@ -56,12 +75,20 @@ export async function substitute(input: SubstituteInput) { return match } if (braceVar !== undefined) { - return (input.env?.[braceVar] ?? process.env[braceVar]) || "" + const value = input.env?.[braceVar] ?? process.env[braceVar] + // altimate_change — upstream_fix (#701): record the blank, then behave exactly as before. + if (!value) blanked.add(braceVar) + return value || "" } return match }) // altimate_change end + // altimate_change start — upstream_fix (#701): publish after the whole text is scanned. + if (blanked.size > 0) _blankedEnv.set(source(input), blanked) + else _blankedEnv.delete(source(input)) + // altimate_change end + const fileMatches = Array.from(text.matchAll(/\{file:[^}]+\}/g)) if (!fileMatches.length) return text diff --git a/packages/opencode/src/mcp/discover.ts b/packages/opencode/src/mcp/discover.ts index a92c377026..86314bf278 100644 --- a/packages/opencode/src/mcp/discover.ts +++ b/packages/opencode/src/mcp/discover.ts @@ -34,11 +34,30 @@ function resolveServerEnvVars( field: context.field, unresolved: stats.unresolvedNames.join(", "), }) + // altimate_change start — upstream_fix: remember it for the user, not just the log (#701). + // An unresolved `${SNOWFLAKE_PASSWORD}` becomes "" and the server launches with a blank + // credential, failing later with something that names neither the variable nor the config + // file. The log line already had the answer; nobody reads it. Recorded here so `/mcps` can + // say so. Mirrors the `setDiscoveryResult` handoff below. + const seen = _unresolvedEnv.get(context.server) ?? new Set() + for (const name of stats.unresolvedNames) seen.add(name) + _unresolvedEnv.set(context.server, seen) + // altimate_change end } return out } // altimate_change end +// altimate_change start — upstream_fix: unresolved-variable record for the user surface (#701). +/** Server name -> variable names that resolved to "" during discovery. */ +const _unresolvedEnv = new Map>() + +/** Variable names that silently became "" for `server`, newest discovery wins. */ +export function unresolvedEnvVars(server: string): string[] { + return [...(_unresolvedEnv.get(server) ?? [])].sort() +} +// altimate_change end + interface ExternalMcpSource { /** Relative path from base directory */ file: string diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index 27f2e85a14..ef9f5aad7c 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/packages/opencode/src/mcp/index.ts @@ -143,6 +143,24 @@ export const Status = Schema.Union([ ]).annotate({ identifier: "MCPStatus", discriminator: "status" }) export type Status = Schema.Schema.Type +// altimate_change start — upstream_fix: do not swallow the connect error (#1121). +// The failure path already carries the real message — `401 Unauthorized`, a transport +// error, `Invalid MCP URL for ""` — in `status.error`, but the warning logged only +// `status.status`, which is the constant string "failed". An external user had to read +// this source to find out why their server would not connect. +// +// Split out as a pure function so the payload is testable without standing up a +// transport, and so a future edit cannot quietly drop the field again. +export function unavailableLogFields( + key: string, + type: string, + status: Status, +): { key: string; type: string; status: string; error?: string } { + const error = "error" in status && typeof status.error === "string" ? status.error : undefined + return error ? { key, type, status: status.status, error } : { key, type, status: status.status } +} +// altimate_change end + // Store transports for OAuth servers to allow finishing auth type TransportWithAuth = StreamableHTTPClientTransport | SSEClientTransport const pendingOAuthTransports = new Map() @@ -627,7 +645,9 @@ export const layer = Layer.effect( if (!mcpClient) { if (status.status !== "connected" && status.status !== "disabled") { - yield* Effect.logWarning("server unavailable", { key, type: mcp.type, status: status.status }) + // altimate_change start — upstream_fix: include the real error (#1121). + yield* Effect.logWarning("server unavailable", unavailableLogFields(key, mcp.type, status)) + // altimate_change end } return { status } satisfies CreateResult } diff --git a/packages/opencode/src/session/prompt.ts b/packages/opencode/src/session/prompt.ts index 268babfc66..e0181dbed8 100644 --- a/packages/opencode/src/session/prompt.ts +++ b/packages/opencode/src/session/prompt.ts @@ -30,6 +30,8 @@ import PROMPT_PLAN from "../session/prompt/plan.txt" import BUILD_SWITCH from "../session/prompt/build-switch.txt" import MAX_STEPS from "../session/prompt/max-steps.txt" import { defer } from "../util/defer" +// altimate_change — upstream_fix (#701): unresolved-env record for the /mcps view. +import * as McpDiscover from "../mcp/discover" import { ToolRegistry } from "../tool/registry" import { MCP } from "../mcp" import { LSP } from "../lsp" @@ -2871,11 +2873,19 @@ NOTE: At any point in time through this workflow you should feel free to ask the // altimate_change start — shared text formatter for /mcps runtime status (#972) /** @internal Exported for tests. */ - export function formatMcpStatusForDisplay(name: string, status: MCP.Status) { + export function formatMcpStatusForDisplay(name: string, status: MCP.Status, unresolvedEnv: string[] = []) { const icon = status.status === "connected" ? "\u2713" : "\u25cb" - if (status.status === "failed") return icon + " " + status.status + " (" + status.error + ")" - if (status.status === "needs_auth") return icon + " Needs authentication (run: altimate mcp auth " + name + ")" - return icon + " " + status.status + // upstream_fix (#701): a server whose `${VAR}` did not resolve launched with that value + // blank — most often a password. It then fails with a downstream error naming neither the + // variable nor the config file, and the only trace is a log line nobody opens. Say it here, + // where the user is already looking, and say it even when the server appears connected: a + // blank credential often connects and fails on first use. + const blanks = + unresolvedEnv.length > 0 ? " \u2014 unresolved: " + unresolvedEnv.join(", ") + " (set or remove)" : "" + if (status.status === "failed") return icon + " " + status.status + " (" + status.error + ")" + blanks + if (status.status === "needs_auth") + return icon + " Needs authentication (run: altimate mcp auth " + name + ")" + blanks + return icon + " " + status.status + blanks } // altimate_change end @@ -2930,7 +2940,10 @@ NOTE: At any point in time through this workflow you should feel free to ask the const model = await lastModel(input.sessionID) const statusMap = await MCP.status() const rows = Object.entries(statusMap) - .map(([srv, s]) => "| `" + srv + "` | " + formatMcpStatusForDisplay(srv, s) + " |") + .map( + ([srv, s]) => + "| `" + srv + "` | " + formatMcpStatusForDisplay(srv, s, McpDiscover.unresolvedEnvVars(srv)) + " |", + ) .join("\n") const responseText = rows ? "MCP servers:\n\n| Server | Status |\n|---|---|\n" + rows diff --git a/packages/opencode/test/cli/mcp-env-diagnostics.test.ts b/packages/opencode/test/cli/mcp-env-diagnostics.test.ts new file mode 100644 index 0000000000..195848b78a --- /dev/null +++ b/packages/opencode/test/cli/mcp-env-diagnostics.test.ts @@ -0,0 +1,119 @@ +// altimate_change start — upstream_fix (#701): the server listing must name environment variables +// that silently resolved to "". This is user-facing CLI behaviour, so it drives the real binary in +// an isolated HOME rather than calling the handler directly. +import { describe, expect, test } from "bun:test" +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "fs" +import { tmpdir } from "os" +import path from "path" +import { spawnSync } from "child_process" + +// Each test boots the real CLI in a subprocess; the default 5s budget is not enough. +const SUBPROCESS_TIMEOUT_MS = 120_000 + +const repoRoot = path.resolve(import.meta.dir, "..", "..", "..", "..") +const opencodeDir = path.join(repoRoot, "packages", "opencode") +const cliEntry = path.join(opencodeDir, "src", "index.ts") + +function withIsolatedCli( + mcp: Record, + fn: (output: (args: string[]) => string) => void, + extraFiles: Record = {}, +) { + const root = mkdtempSync(path.join(tmpdir(), "altimate-mcp-status-")) + const home = path.join(root, "home") + const configHome = path.join(root, "config") + const configDir = path.join(configHome, "altimate-code") + mkdirSync(home, { recursive: true }) + mkdirSync(configDir, { recursive: true }) + writeFileSync(path.join(configDir, "altimate-code.json"), JSON.stringify({ mcp }), "utf-8") + for (const [rel, content] of Object.entries(extraFiles)) { + const target = path.join(root, rel) + mkdirSync(path.dirname(target), { recursive: true }) + writeFileSync(target, content, "utf-8") + } + + const run = (args: string[]) => + // `bun run --cwd ` would make the CLI's working directory the repo package, so it would + // read the repo's own .opencode config and never see this temp project. Spawn cwd is the + // project instead; module resolution still follows cliEntry's location. + spawnSync("bun", ["--conditions=browser", cliEntry, ...args], { + cwd: root, + encoding: "utf-8", + timeout: 90_000, + env: { + ...process.env, + HOME: home, + XDG_CONFIG_HOME: configHome, + XDG_DATA_HOME: path.join(root, "data"), + XDG_CACHE_HOME: path.join(root, "cache"), + XDG_STATE_HOME: path.join(root, "state"), + OPENCODE_DISABLE_TELEMETRY: "1", + OPENCODE_DISABLE_SHARE: "1", + OPENCODE_DISABLE_AUTOUPDATE: "1", + OPENCODE_DISABLE_AUTOCOMPACT: "1", + OPENCODE_DISABLE_MODELS_FETCH: "1", + OPENCODE_PURE: "1", + TERM: "dumb", + CI: "1", + }, + }) + + const output = (args: string[]) => { + const r = run(args) + return String(r.stdout ?? "") + String(r.stderr ?? "") + } + + try { + fn(output) + } finally { + rmSync(root, { recursive: true, force: true }) + } +} + +const brokenServer = { + broken: { + type: "local", + command: ["/nonexistent-binary-for-mcp-status-test"], + environment: { API_TOKEN: "{env:ALTIMATE_TEST_VAR_THAT_IS_NEVER_SET}" }, + enabled: true, + }, +} + +describe("altimate-code mcp list — env diagnostics", () => { + test( + "`status` reaches the server listing", + () => + withIsolatedCli(brokenServer, (output) => { + const out = output(["mcp", "list"]) + expect(out, out).toContain("broken") + expect(out, out).not.toContain("Unknown argument") + }), + SUBPROCESS_TIMEOUT_MS, + ) + + test( + "names the config env var that silently resolved to empty", + () => + withIsolatedCli(brokenServer, (output) => { + const out = output(["mcp", "list"]) + expect(out, out).toContain("ALTIMATE_TEST_VAR_THAT_IS_NEVER_SET") + expect(out, out).toContain("resolved to empty") + }), + SUBPROCESS_TIMEOUT_MS, + ) + + test( + "says nothing about env when every variable resolves", + () => + withIsolatedCli( + { fine: { type: "local", command: ["/nonexistent-binary-for-mcp-status-test"], enabled: true } }, + (output) => { + const out = output(["mcp", "list"]) + expect(out, out).toContain("fine") + expect(out, out).not.toContain("resolved to empty") + }, + ), + SUBPROCESS_TIMEOUT_MS, + ) +}) +// altimate_change end diff --git a/packages/opencode/test/mcp/unavailable-log.test.ts b/packages/opencode/test/mcp/unavailable-log.test.ts new file mode 100644 index 0000000000..0641df9c03 --- /dev/null +++ b/packages/opencode/test/mcp/unavailable-log.test.ts @@ -0,0 +1,43 @@ +import { describe, expect, test } from "bun:test" +import { unavailableLogFields } from "../../src/mcp/index" + +// altimate_change start — upstream_fix: regression guard for #1121. +// The connect path stores the real reason a server would not start — `401 Unauthorized`, +// a transport error, an invalid URL — in `status.error`, but the warning logged only +// `status.status`, which is always the constant "failed". The reporter of #1121 had to +// read this module's source to find out why their server was unreachable. +describe("unavailableLogFields", () => { + test("carries the real error for a failed connection", () => { + expect(unavailableLogFields("exodus-mcp", "remote", { status: "failed", error: "401 Unauthorized" })).toEqual({ + key: "exodus-mcp", + type: "remote", + status: "failed", + error: "401 Unauthorized", + }) + }) + + test("carries the error for needs_client_registration too", () => { + // The other failure state that has something worth reading. + expect( + unavailableLogFields("gh", "remote", { status: "needs_client_registration", error: "registration rejected" }), + ).toEqual({ key: "gh", type: "remote", status: "needs_client_registration", error: "registration rejected" }) + }) + + test("omits the key entirely when the status carries no error", () => { + // `needs_auth` is not a fault — logging `error: undefined` would imply one. + expect(unavailableLogFields("github", "remote", { status: "needs_auth" })).toEqual({ + key: "github", + type: "remote", + status: "needs_auth", + }) + expect("error" in unavailableLogFields("github", "remote", { status: "needs_auth" })).toBe(false) + }) + + test("never loses the server key or transport type", () => { + // These are what let an operator find the offending entry in their config. + const fields = unavailableLogFields("local-one", "local", { status: "failed", error: "spawn ENOENT" }) + expect(fields.key).toBe("local-one") + expect(fields.type).toBe("local") + }) +}) +// altimate_change end diff --git a/packages/opencode/test/session/mcps-command.test.ts b/packages/opencode/test/session/mcps-command.test.ts index 5ef867160c..17260ce9d5 100644 --- a/packages/opencode/test/session/mcps-command.test.ts +++ b/packages/opencode/test/session/mcps-command.test.ts @@ -14,3 +14,37 @@ describe("/mcps command status formatting", () => { ) }) }) + +// altimate_change start — upstream_fix: unresolved env vars reach the user (#701). +// An unresolved `${SNOWFLAKE_PASSWORD}` silently became "" and the server launched with a +// blank credential; the only trace was a log line. These pin that /mcps says so instead. +describe("formatMcpStatusForDisplay — unresolved env vars", () => { + test("names the variables on a failed server", () => { + const out = SessionPrompt.formatMcpStatusForDisplay("snow", { status: "failed", error: "auth failed" }, [ + "SNOWFLAKE_PASSWORD", + ]) + expect(out).toContain("auth failed") + expect(out).toContain("SNOWFLAKE_PASSWORD") + }) + + test("warns even when the server looks connected", () => { + // A blank credential frequently connects and only fails on first real use, so the + // connected row is exactly where this needs saying. + const out = SessionPrompt.formatMcpStatusForDisplay("snow", { status: "connected" }, ["TOKEN"]) + expect(out).toContain("connected") + expect(out).toContain("TOKEN") + }) + + test("lists every unresolved variable, not just the first", () => { + const out = SessionPrompt.formatMcpStatusForDisplay("s", { status: "connected" }, ["A_TOKEN", "B_SECRET"]) + expect(out).toContain("A_TOKEN") + expect(out).toContain("B_SECRET") + }) + + test("says nothing extra when everything resolved", () => { + expect(SessionPrompt.formatMcpStatusForDisplay("ok", { status: "connected" }, [])).toBe( + SessionPrompt.formatMcpStatusForDisplay("ok", { status: "connected" }), + ) + }) +}) +// altimate_change end