fix: surface the real reason an MCP server is unavailable

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)`)
   }),
 })

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<string, Set<string>>()
+
+/** 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<string>()
   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
 

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<string>()
+    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<string, Set<string>>()
+
+/** 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

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<typeof Status>
 
+// 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 "<key>"` — 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<string, TransportWithAuth>()
@@ -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
         }

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

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<string, unknown>,
+  fn: (output: (args: string[]) => string) => void,
+  extraFiles: Record<string, string> = {},
+) {
+  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 <pkg>` 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

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