diff --git a/packages/opencode/src/cli/cmd/mcp.ts b/packages/opencode/src/cli/cmd/mcp.ts index 6739ca22a..2fd3c56ea 100644 --- a/packages/opencode/src/cli/cmd/mcp.ts +++ b/packages/opencode/src/cli/cmd/mcp.ts @@ -9,7 +9,7 @@ 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`. +// altimate_change start — upstream_fix: diagnostics surfaced by `mcp status` (#701, #878). import * as McpDiscover from "../../mcp/discover" import { ConfigVariable } from "../../config/variable" // altimate_change end @@ -108,6 +108,12 @@ export const McpCommand = cmd({ yargs .command(McpAddCommand) .command(McpListCommand) + // altimate_change start — upstream_fix (#790): `status` is the name people reach for when a + // server will not connect, and it was the one name that did not exist. It shares the list + // handler rather than duplicating a view that already probes live and already prints the + // failure reason; a `list` alias would widen yargs' alias column and rewrap sibling rows. + .command({ ...McpListCommand, command: "status", aliases: [], describe: "show MCP server health" }) + // altimate_change end .command(McpAuthCommand) .command(McpLogoutCommand) // altimate_change start — restore `mcp remove` removed during v1.4.0 bridge merge @@ -118,6 +124,26 @@ export const McpCommand = cmd({ async handler() {}, }) +// altimate_change start — upstream_fix (#878/#701): config-level diagnostics, shared by every +// exit of `mcp list` / `mcp status` so a config with nothing listable still reports them. +function reportConfigDiagnostics() { + // Discovery is first-source-wins, so a server already in altimate-code.json is skipped and a + // changed .vscode/mcp.json is never mentioned. The configured value still wins; this only + // says the two disagree and which file to look at. + for (const { server, source, fields } of McpDiscover.configDrift()) { + prompts.log.warn(`${server} differs from ${source}: ${fields.join(", ")} (config wins)`) + } + + // 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 + export const McpListCommand = effectCmd({ command: "list", aliases: ["ls"], @@ -131,6 +157,11 @@ export const McpListCommand = effectCmd({ if (servers.length === 0) { prompts.log.warn("No MCP servers configured") + // altimate_change start — upstream_fix (#878): drift and blank-variable warnings are about + // the config, not about any one server, so they must survive the nothing-to-list exit. An + // enabled-only override for a discovered server leaves this list empty while drift exists. + reportConfigDiagnostics() + // altimate_change end // altimate_change start — branding regression prompts.outro("Add servers with: altimate mcp add") // altimate_change end @@ -179,20 +210,14 @@ export const McpListCommand = effectCmd({ 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 start — upstream_fix (#878/#701): config-level diagnostics. + reportConfigDiagnostics() // altimate_change end prompts.outro(`${servers.length} server(s)`) @@ -508,7 +533,8 @@ export const McpAddCommand = effectCmd({ // altimate_change start — non-interactive mode: upstream v1.17 form (--url or command after --) // plus the fork's explicit --type/--command form. const passthrough = Array.isArray(args["--"]) ? args["--"] : [] - const inferredType = args.type ?? (args.url ? "remote" : passthrough.length > 0 || args.command ? "local" : undefined) + const inferredType = + args.type ?? (args.url ? "remote" : passthrough.length > 0 || args.command ? "local" : undefined) if (args.name && inferredType) { if (!args.name.trim()) { console.error("MCP server name cannot be empty") diff --git a/packages/opencode/src/config/config.ts b/packages/opencode/src/config/config.ts index 33fdeaa26..4ee63401a 100644 --- a/packages/opencode/src/config/config.ts +++ b/packages/opencode/src/config/config.ts @@ -756,7 +756,10 @@ export const layer = Layer.effect( const autoMcpDiscovery = (result.experimental as { auto_mcp_discovery?: boolean } | undefined) ?.auto_mcp_discovery if (!Flag.OPENCODE_DISABLE_PROJECT_CONFIG && autoMcpDiscovery !== false) { - const { discoverExternalMcp, setDiscoveryResult } = yield* Effect.promise(() => import("../mcp/discover")) + const { discoverExternalMcp, setDiscoveryResult, driftFields, setConfigDrift, discoveredSource } = + yield* Effect.promise( + () => import("../mcp/discover"), + ) const { servers: externalMcp, sources } = yield* Effect.promise(() => discoverExternalMcp(ctx.directory)) if (Object.keys(externalMcp).length > 0) { result.mcp ??= {} @@ -765,6 +768,15 @@ export const layer = Layer.effect( if (!(name in result.mcp)) { ;(result.mcp as Record)[name] = server added.push(name) + } else { + // altimate_change — upstream_fix (#878): the user's config still wins, but the + // difference is recorded so a surface can report it rather than silently skipping. + const configured = (result.mcp as Record)[name] + setConfigDrift( + name, + discoveredSource(name) ?? sources.join(", "), + driftFields(server as Record, configured), + ) } } setDiscoveryResult(added, sources) diff --git a/packages/opencode/src/mcp/discover.ts b/packages/opencode/src/mcp/discover.ts index 522c7f157..e0d32dd9f 100644 --- a/packages/opencode/src/mcp/discover.ts +++ b/packages/opencode/src/mcp/discover.ts @@ -75,6 +75,96 @@ function resetUnresolvedEnv() { } // altimate_change end +// altimate_change start — upstream_fix (#878): report drift instead of silently skipping. +// Discovery is first-source-wins, so a server already present in altimate-code.json is skipped +// outright and a changed `.vscode/mcp.json` (a new ALTIMATE_EXTENSION_RPC port, a moved command) +// is never mentioned. Overwriting the user's own config would be worse than the silence, so the +// differing field names are recorded and a user surface reports them; the user decides. +const _drift = new Map() + +/** + * Fields whose difference is expected and not worth reporting. + * + * `updatedAt` is the datamate sync change-signal: `normalizeMcpConfig` preserves it on the + * configured entry and discovery never produces one, so every comparison saw a value against + * `undefined` and reported drift on every `mcp list` for any datamate-synced server. + */ +const DRIFT_IGNORED = new Set(["enabled", "updatedAt"]) + +/** Key order must not read as a difference — `{a,b}` and `{b,a}` are the same config. */ +function stableStringify(value: any): string { + if (value === null || typeof value !== "object") return JSON.stringify(value) + if (Array.isArray(value)) return "[" + value.map(stableStringify).join(",") + "]" + return ( + "{" + + Object.keys(value) + .sort() + .map((k) => JSON.stringify(k) + ":" + stableStringify(value[k])) + .join(",") + + "}" + ) +} + +const isPlainObject = (v: any): v is Record => !!v && typeof v === "object" && !Array.isArray(v) + +/** + * Field names that differ between a discovered server and the one already configured. + * Nested `environment`/`headers` differences are reported per key (`environment.FOO`) so the + * message names the thing to fix rather than just "environment". + */ +export function driftFields(discovered: Record, configured: Record): string[] { + const fields: string[] = [] + for (const key of new Set([...Object.keys(discovered), ...Object.keys(configured)])) { + if (DRIFT_IGNORED.has(key)) continue + const a = discovered[key] + const b = configured[key] + // Nested when EITHER side has the block: requiring both meant a server that gained or lost + // an `environment` wholesale reported the bare word "environment" and lost the key names, + // which is the one thing this function exists to provide. + if ((key === "environment" || key === "headers") && (isPlainObject(a) || isPlainObject(b))) { + const left = isPlainObject(a) ? a : {} + const right = isPlainObject(b) ? b : {} + const inner = new Set([...Object.keys(left), ...Object.keys(right)]) + // An empty block against a missing one has no key to name, so report it at the top level + // rather than saying nothing at all. + if (inner.size === 0) { + if (stableStringify(a) !== stableStringify(b)) fields.push(key) + continue + } + for (const name of inner) if (left[name] !== right[name]) fields.push(`${key}.${name}`) + continue + } + if (stableStringify(a) !== stableStringify(b)) fields.push(key) + } + return fields.sort() +} + +/** Record that `server` is configured differently from what discovery found in `source`. */ +export function setConfigDrift(server: string, source: string, fields: string[]) { + if (fields.length > 0) _drift.set(server, { source, fields }) + else _drift.delete(server) +} + +/** Servers whose configured definition differs from the discovered one. */ +export function configDrift(): { server: string; source: string; fields: string[] }[] { + return [..._drift.entries()] + .map(([server, info]) => ({ server, ...info })) + .sort((a, b) => a.server.localeCompare(b.server)) +} + +/** Server name -> the file that actually defined it, for drift attribution. */ +const _discoveredSource = new Map() + +/** The config file a discovered server came from, or undefined if it was not discovered. */ +export function discoveredSource(server: string): string | undefined { + return _discoveredSource.get(server) +} + +/** Test seam — drift accumulates at module level. */ +export function resetConfigDrift() { + _drift.clear() +} +// altimate_change end interface ExternalMcpSource { /** Relative path from base directory */ file: string @@ -240,6 +330,10 @@ function addServersFromFile( ;(transformed as any).enabled = false } result[name] = transformed + // altimate_change start — upstream_fix (#878): attribute drift to the file that defined + // this server, not to every file that contributed something to the run. + _discoveredSource.set(name, sourceLabel) + // altimate_change end added++ } } @@ -340,6 +434,11 @@ export async function discoverExternalMcp(projectDir: string): Promise<{ log.info("Discovering MCP servers from external AI tool configs...") // Start from a clean slate so a variable fixed since the last run stops being reported. resetUnresolvedEnv() + // Same for drift: a server removed from the external config, or a reload that resolved the + // difference, otherwise left a stale entry and `mcp status` reported a mismatch that no + // longer existed. The setConfigDrift calls after this run repopulate it. + resetConfigDrift() + _discoveredSource.clear() const result: Record = Object.create(null) const contributingSources: string[] = [] const homedir = os.homedir() diff --git a/packages/opencode/src/session/prompt.ts b/packages/opencode/src/session/prompt.ts index baa5ce549..1e757bb4f 100644 --- a/packages/opencode/src/session/prompt.ts +++ b/packages/opencode/src/session/prompt.ts @@ -2997,6 +2997,18 @@ 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. */ + // altimate_change start — upstream_fix (#878): `/mcps` reported neither drift nor file-scoped + // blanks, so the session view consistently showed less than `mcp list` for the same problem. + /** Config-drift lines for `/mcps`, empty string when nothing has drifted. */ + export function formatConfigDriftForDisplay(entries: { server: string; source: string; fields: string[] }[]): string { + return entries + .map( + ({ server, source, fields }) => + "- `" + server + "` differs from `" + source + "`: " + fields.join(", ") + " (config wins)", + ) + .join("\n") + } + // altimate_change end // altimate_change start — upstream_fix (#701): exported so the wording is testable without // standing up a session; `/mcps` is otherwise only reachable through the whole handler. /** File-scoped blank-variable lines for `/mcps`, empty string when there are none. */ @@ -3084,8 +3096,9 @@ NOTE: At any point in time through this workflow you should feel free to ask the // file rather than the server, so it appeared in the CLI and not here — in the session // view, which is where someone is when a server will not connect. const blanked = formatBlankedEnvForDisplay(ConfigVariable.blankedEnvVars()) + const drift = formatConfigDriftForDisplay(McpDiscover.configDrift()) const table = rows ? "MCP servers:\n\n| Server | Status |\n|---|---|\n" + rows : "No MCP servers configured." - const responseText = blanked ? table + "\n\n" + blanked : table + const responseText = [table, drift, blanked].filter(Boolean).join("\n\n") // altimate_change end return respond(userMsg.info.id, responseText, model) diff --git a/packages/opencode/test/cli/help/__snapshots__/help-snapshots.test.ts.snap b/packages/opencode/test/cli/help/__snapshots__/help-snapshots.test.ts.snap index e71b7cff4..54c6a28d0 100644 --- a/packages/opencode/test/cli/help/__snapshots__/help-snapshots.test.ts.snap +++ b/packages/opencode/test/cli/help/__snapshots__/help-snapshots.test.ts.snap @@ -31,6 +31,7 @@ manage MCP (Model Context Protocol) servers Commands: altimate-code mcp add add an MCP server altimate-code mcp list list MCP servers and their status [aliases: ls] + altimate-code mcp status show MCP server health altimate-code mcp auth [name] authenticate with an OAuth-enabled MCP server altimate-code mcp logout [name] remove OAuth credentials for an MCP server altimate-code mcp remove remove an MCP server [aliases: rm] diff --git a/packages/opencode/test/cli/mcp-status.test.ts b/packages/opencode/test/cli/mcp-status.test.ts new file mode 100644 index 000000000..037768964 --- /dev/null +++ b/packages/opencode/test/cli/mcp-status.test.ts @@ -0,0 +1,82 @@ +// altimate_change start — upstream_fix (#878): `mcp status` must exist and must report drift. +// Drives the real binary in an isolated HOME. The subprocess harness lives in +// ./fixtures/isolated-cli so this file and mcp-env-diagnostics.test.ts cannot drift apart. +import { describe, expect, test } from "bun:test" +import { SUBPROCESS_TIMEOUT_MS, withIsolatedCli } from "./fixtures/isolated-cli" +// altimate_change end + +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 status", () => { + test( + "`status` reaches the server listing", + () => + withIsolatedCli(brokenServer, (output) => { + const out = output(["mcp", "status"]) + expect(out, out).toContain("broken") + expect(out, out).not.toContain("Unknown argument") + }), + SUBPROCESS_TIMEOUT_MS, + ) +}) +// altimate_change end + +// altimate_change start — upstream_fix (#878): drift must reach the user, not just the record. +describe("altimate-code mcp status — discovered config drift", () => { + const configured = { + datamate: { + type: "local", + command: ["/nonexistent-binary-for-mcp-status-test"], + environment: { ALTIMATE_EXTENSION_RPC: "127.0.0.1:9000" }, + enabled: true, + }, + } + const vscode = (rpc: string) => + JSON.stringify({ + servers: { + datamate: { + type: "stdio", + command: "/nonexistent-binary-for-mcp-status-test", + env: { ALTIMATE_EXTENSION_RPC: rpc }, + }, + }, + }) + + test( + "reports the field that drifted from the discovered config", + () => + withIsolatedCli( + configured, + (output) => { + const out = output(["mcp", "status"]) + expect(out, out).toContain("datamate") + expect(out, out).toContain("environment.ALTIMATE_EXTENSION_RPC") + }, + { ".vscode/mcp.json": vscode("127.0.0.1:9999") }, + ), + SUBPROCESS_TIMEOUT_MS, + ) + + test( + "says nothing when the discovered config agrees", + () => + withIsolatedCli( + configured, + (output) => { + const out = output(["mcp", "status"]) + expect(out, out).toContain("datamate") + expect(out, out).not.toContain("environment.ALTIMATE_EXTENSION_RPC") + }, + { ".vscode/mcp.json": vscode("127.0.0.1:9000") }, + ), + SUBPROCESS_TIMEOUT_MS, + ) +}) +// altimate_change end diff --git a/packages/opencode/test/mcp/config-drift.test.ts b/packages/opencode/test/mcp/config-drift.test.ts new file mode 100644 index 000000000..a5821a32b --- /dev/null +++ b/packages/opencode/test/mcp/config-drift.test.ts @@ -0,0 +1,88 @@ +// altimate_change start — upstream_fix (#878): discovery skipped already-configured servers +// without a word, so a changed .vscode/mcp.json never surfaced. These pin what counts as drift. +import { describe, expect, test, beforeEach } from "bun:test" +import { driftFields, setConfigDrift, configDrift, resetConfigDrift } from "../../src/mcp/discover" + +describe("driftFields", () => { + test("identical definitions report no drift", () => { + const server = { type: "local", command: ["node", "server.js"], environment: { PORT: "1" } } + expect(driftFields({ ...server }, { ...server })).toEqual([]) + }) + + test("names the environment key that changed, not just `environment`", () => { + const discovered = { type: "local", environment: { ALTIMATE_EXTENSION_RPC: "127.0.0.1:9001", KEEP: "same" } } + const configured = { type: "local", environment: { ALTIMATE_EXTENSION_RPC: "127.0.0.1:9000", KEEP: "same" } } + expect(driftFields(discovered, configured)).toEqual(["environment.ALTIMATE_EXTENSION_RPC"]) + }) + + test("reports a key present on only one side", () => { + expect(driftFields({ environment: { A: "1", B: "2" } }, { environment: { A: "1" } })).toEqual(["environment.B"]) + }) + + test("compares command arrays by value, not identity", () => { + expect(driftFields({ command: ["node", "a.js"] }, { command: ["node", "a.js"] })).toEqual([]) + expect(driftFields({ command: ["node", "a.js"] }, { command: ["node", "b.js"] })).toEqual(["command"]) + }) + + test("ignores `enabled`, which discovery sets for its own reasons", () => { + expect(driftFields({ type: "local", enabled: false }, { type: "local", enabled: true })).toEqual([]) + }) + + test("reports a changed url", () => { + expect(driftFields({ url: "https://a" }, { url: "https://b" })).toEqual(["url"]) + }) +}) + +describe("configDrift record", () => { + beforeEach(() => resetConfigDrift()) + + test("records only servers that actually differ", () => { + setConfigDrift("datamate", ".vscode/mcp.json", ["environment.ALTIMATE_EXTENSION_RPC"]) + setConfigDrift("clean", ".vscode/mcp.json", []) + expect(configDrift()).toEqual([ + { server: "datamate", source: ".vscode/mcp.json", fields: ["environment.ALTIMATE_EXTENSION_RPC"] }, + ]) + }) + + test("a server that stops drifting is dropped from the report", () => { + setConfigDrift("datamate", ".vscode/mcp.json", ["url"]) + setConfigDrift("datamate", ".vscode/mcp.json", []) + expect(configDrift()).toEqual([]) + }) +}) +// altimate_change end + +// altimate_change start — upstream_fix (#878): findings from PR review. +describe("driftFields — false positives and lost detail", () => { + test("ignores updatedAt, the datamate sync bookkeeping field", () => { + // normalizeMcpConfig preserves updatedAt on the configured entry and discovery never + // produces one, so this compared a string against undefined and reported drift on every + // `mcp list` for any datamate-synced server. + expect(driftFields({ command: ["a"] }, { command: ["a"], updatedAt: "2026-08-28T00:00:00Z" })).toEqual([]) + }) + + test("does not report key order as a difference", () => { + const discovered = { oauth: { clientId: "x", scope: "y" } } + const configured = { oauth: { scope: "y", clientId: "x" } } + expect(driftFields(discovered, configured)).toEqual([]) + }) + + test("names the inner key when only one side has the block", () => { + // Previously required both sides to be objects, so a server that gained an environment + // wholesale reported the bare word "environment" and lost the key that actually differs. + expect(driftFields({ environment: { PORT: "1" } }, {})).toEqual(["environment.PORT"]) + expect(driftFields({}, { environment: { PORT: "1" } })).toEqual(["environment.PORT"]) + }) + + test("still reports an empty block against a missing one", () => { + // No inner key exists to name, so the top-level field is the only honest answer. + expect(driftFields({ environment: {} }, {})).toEqual(["environment"]) + }) + + test("still reports a genuine difference", () => { + // The guard against false positives must not silence real drift. + expect(driftFields({ environment: { PORT: "1" } }, { environment: { PORT: "2" } })).toEqual(["environment.PORT"]) + expect(driftFields({ command: ["a"] }, { command: ["b"] })).toEqual(["command"]) + }) +}) +// altimate_change end diff --git a/packages/opencode/test/session/mcps-command.test.ts b/packages/opencode/test/session/mcps-command.test.ts index 01396511e..da3c3fe5d 100644 --- a/packages/opencode/test/session/mcps-command.test.ts +++ b/packages/opencode/test/session/mcps-command.test.ts @@ -84,3 +84,30 @@ describe("/mcps file-scoped blank variables", () => { }) }) // altimate_change end + +// altimate_change start — upstream_fix (#878): drift belongs in the session view too. +describe("/mcps config drift", () => { + test("names the server, the file, and the fields", () => { + const out = SessionPrompt.formatConfigDriftForDisplay([ + { server: "datamate", source: ".vscode/mcp.json", fields: ["environment.ALTIMATE_EXTENSION_RPC"] }, + ]) + expect(out).toContain("datamate") + expect(out).toContain(".vscode/mcp.json") + expect(out).toContain("environment.ALTIMATE_EXTENSION_RPC") + // The user's config still wins; the message says so rather than implying an action was taken. + expect(out).toContain("config wins") + }) + + test("one line per drifted server", () => { + const out = SessionPrompt.formatConfigDriftForDisplay([ + { server: "a", source: "x.json", fields: ["command"] }, + { server: "b", source: "y.json", fields: ["command"] }, + ]) + expect(out.split("\n")).toHaveLength(2) + }) + + test("empty when nothing drifted", () => { + expect(SessionPrompt.formatConfigDriftForDisplay([])).toBe("") + }) +}) +// altimate_change end