diff --git a/packages/opencode/src/cli/cmd/mcp.ts b/packages/opencode/src/cli/cmd/mcp.ts index 6739ca22a6..a6681dde61 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 @@ -179,17 +185,23 @@ 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. + // altimate_change start — upstream_fix (#878): 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 where to look. + for (const { server, source, fields } of McpDiscover.configDrift()) { + prompts.log.warn(`${server} differs from ${source}: ${fields.join(", ")} (config wins)`) + } + + // 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)`) } diff --git a/packages/opencode/src/config/config.ts b/packages/opencode/src/config/config.ts index 28b5cb0ade..e5d08faefb 100644 --- a/packages/opencode/src/config/config.ts +++ b/packages/opencode/src/config/config.ts @@ -724,7 +724,9 @@ 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 } = yield* Effect.promise( + () => import("../mcp/discover"), + ) const { servers: externalMcp, sources } = yield* Effect.promise(() => discoverExternalMcp(ctx.directory)) if (Object.keys(externalMcp).length > 0) { result.mcp ??= {} @@ -733,6 +735,11 @@ 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, 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 86314bf278..a3fb8f50a2 100644 --- a/packages/opencode/src/mcp/discover.ts +++ b/packages/opencode/src/mcp/discover.ts @@ -46,7 +46,6 @@ function resolveServerEnvVars( } 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. */ @@ -56,6 +55,59 @@ const _unresolvedEnv = new Map>() export function unresolvedEnvVars(server: string): string[] { return [...(_unresolvedEnv.get(server) ?? [])].sort() } + +// 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. */ +const DRIFT_IGNORED = new Set(["enabled"]) + +/** + * 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] + const nested = key === "environment" || key === "headers" + if (nested && a && b && typeof a === "object" && typeof b === "object") { + for (const inner of new Set([...Object.keys(a), ...Object.keys(b)])) { + if (a[inner] !== b[inner]) fields.push(`${key}.${inner}`) + } + continue + } + if (JSON.stringify(a) !== JSON.stringify(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)) +} + +/** Test seam — drift accumulates at module level. */ +export function resetConfigDrift() { + _drift.clear() +} +// altimate_change end +// altimate_change end // altimate_change end interface ExternalMcpSource { 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 e71b7cff4e..54c6a28d07 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 0000000000..65274eef61 --- /dev/null +++ b/packages/opencode/test/cli/mcp-status.test.ts @@ -0,0 +1,149 @@ +// altimate_change start — upstream_fix (#790, #878): `mcp status` must exist, and a configured +// server that has drifted from the discovered config must be reported. Both are user-facing CLI +// behaviour, so this drives the real binary in an isolated HOME rather than the handler. +// The env-variable reporting these views share is covered against `mcp list` in +// test/cli/mcp-env-diagnostics.test.ts and is not duplicated here. +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 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 0000000000..ce55d329a4 --- /dev/null +++ b/packages/opencode/test/mcp/config-drift.test.ts @@ -0,0 +1,53 @@ +// 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