fix: add an MCP status command and report discovered-config drift #1160
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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<string, { source: string; fields: string[] }>() | ||
|
|
||
| /** | ||
| * 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<string, any> => !!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<string, any>, configured: Record<string, any>): 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 }) | ||
|
Contributor
There was a problem hiding this comment. configDrift() is exported and read by reportConfigDiagnostics() in mcp list / mcp status, but prompt.ts's /mcps handler never calls it. A user debugging a silently-failing server via /mcps mid-session sees neither drift warnings here nor blankedEnvVars warnings (the latter flagged in PR #1159). Both diagnostic surfaces are supposed to solve the same 'why won't this server connect' problem, but the session command /mcps consistently receives only unresolvedEnvVars while the CLI surfaces the full picture. The pattern repeats with each new diagnostic type added, so the gap between CLI and session view will keep widening without an explicit design decision to sync them. |
||
| 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<string, string>() | ||
|
|
||
| /** 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() | ||
|
There was a problem hiding this comment. SUGGESTION:
Reply with |
||
| const result: Record<string, ConfigMCPV1.Info> = Object.create(null) | ||
| const contributingSources: string[] = [] | ||
| const homedir = os.homedir() | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
|
There was a problem hiding this comment. 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win Remove the redundant The marker at this location is already covered by an enclosing marked block. Remove the extra closing marker here and the nested marker in 📍 Affects 2 files
🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
| // 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 | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SUGGESTION: New
#878marker block is nested inside the#701and "per-field" blocks, leaving three stacked// altimate_change endmarkersThis diff removed the
// altimate_change endthat closed the "per-field env-var resolution" block (afterresolveServerEnvVars), so the#701block and this new#878block are now nested inside it and closed by the three consecutive// altimate_change endlines below. Marker Guard checks presence/balance, not scoping, so this passes CI, but the "per-field" block now over-scopes to include_unresolvedEnvand the drift helpers. Restore the closing// altimate_change endafterresolveServerEnvVarsand keep these blocks as siblings.Reply with
@kilocode-bot fix itto have Kilo Code address this issue.