diff --git a/.gitignore b/.gitignore index be82dde..1af8e4c 100644 --- a/.gitignore +++ b/.gitignore @@ -37,3 +37,6 @@ tmp/ # MCP Registry publisher server.json research/ + +# Local Claude Code permissions (per-machine, never shared) +.claude/settings.local.json diff --git a/CHANGELOG.md b/CHANGELOG.md index 1431678..2c39f64 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,101 @@ _Add entries here, never under a stamped version_ — a release commit renames t heading, and a branch that wrote beneath it merges without conflict straight into a published section (it happened to #170). +### Fixed + +- **`mcpm update` silently destroyed a malformed entry's `env` block — a + regression shipped in v0.34.0 (TODOS #59).** `readExistingEnv` reads through + `BaseAdapter.read()`, which since #23 DROPS an entry that fails shape + validation. It therefore returned `undefined` for a user whose entry was + malformed in one field (say `args: "-y pkg"` instead of an array) but whose + `env` held real API keys — and the `force: true` re-write discarded them + while printing `✓ Updated`. Verified against a binary built from the pre-fix + commit: an entry carrying `MY_API_KEY` came back as `{command, args}` with no + `env` at all, reported as a success. + + `read()`'s `onSkip` now receives the **raw entry** alongside the name, and + `update` recovers `env` from it **per key** — each string-valued key is + carried, and any key that cannot be (plus a non-object `env`, plus unrelated + malformed neighbours) is NAMED rather than dropped in silence. Per key and + not a whole-record parse, because `env` is frequently the field that makes + the entry invalid — a numeric port is the archetypal hand-edit — and + rejecting the whole record then destroys the API key sitting beside the bad + one. Nothing else from an unvalidated entry is read, and it is never spread; + the accumulator is `Object.create(null)` so a key named `__proto__` is + carried rather than swallowed by the prototype setter. **The first cut of this fix refused the write instead, and + review was right to reject it**: overwriting a mis-shaped entry with a + freshly resolved one is the user's self-repair path, so refusing converted a + self-healing case (a malformed entry with no env to lose) into a permanently + stuck one whose only signal was a warning that fired once and never again — + the printed remediation, "fix the entry and re-run", produced "All servers + are up to date". Recovering the value repairs the entry *and* keeps the + secret; measured end to end, `args` comes back a proper array with + `MY_API_KEY` intact. + +- **A malformed entry is no longer reported as a *missing* server (TODOS + #59).** Six call sites read through the stderr-only default, and each made a + claim the dropped entry falsified. `sync --check` — a CI gate — told the user + a client was MISSING a server it demonstrably has, and when only one client + held it the server vanished from the model entirely, exiting 0 over a config + mcpm could not read. `diff` reported it "missing", sending the user to `mcpm + up` to re-install over an entry they only needed to fix. `export` omitted it + from a stack file the user keeps as their declared state. `import` — the + first-run path — dropped it from the pick-list and then printed "No existing + MCP servers found". `list` omitted it from the inventory, including `--json`, + which has no stderr channel a consumer reads. `up --strict` left it behind + while reporting a clean reconciliation; it is still NOT deleted (the + fail-safe direction #23 chose), but it is now reported on both the human and + the `recordResult` channel — the latter being the `mcpm_up` MCP surface's + only signal. + + `ClientState`/`ServerDrift` gain `malformed`, `DiffStatus` gains + `unreadable`, `DoctorDriftEntry` gains an `unreadable` kind, `list --json` keeps its + bare-array shape and puts the skip notice on **stderr** — flipping the + payload to an object only in the malformed case would break + `JSON.parse(out).map(...)` exactly when something is already wrong. (The MCP + `mcpm_list` tool does put `skipped` in its result, because that surface has + no stderr an agent can see; the CLI does.) **`guard/cli.ts`'s two sites pass a + NO-OP**: the orchestrator already names the same entry in the same + invocation, so the default's stderr line was printing it a second time. + +- **Config-supplied server names now reach the terminal sanitized in every new + render site** (`sync`'s table, detail, conflict and missing lines; `diff`'s + unreadable line; `doctor`'s three cross-client branches; `list`'s warning; + `up --strict`'s not-removed line; `export`'s and `import`'s warnings; and + `update`'s neighbour notice and dropped-env-key note). The `up --strict` + site was missed on the first pass while this bullet already claimed "every + new render site", and the list itself has since been corrected twice for + under-enumeration — each new render site has an escape test now. + These names are arbitrary JSON keys from a file mcpm does not control; + `base.ts` states the rule for exactly this value, and routing malformed names + into these renderers is what newly exposed them. `--json` stays byte-faithful. + +- **`export` dropped `Object.prototype`-named entries from its own warning.** + The omission check used `name in servers` on an object literal, so a + malformed entry named `toString`, `constructor` or `valueOf` read as + already-exported and vanished from the warning that exists to say it was + dropped. It now uses the `Set` already in scope. + +- **`sync --check` reported a CI failure whose own output said there was + nothing to look at.** `renderDashboard` returns early for zero clients, one + client, or no servers, but `drifted` is computed from the model — so on the + common single-client desktop shape, `--check` exited 2 while printing only + "nothing to compare across clients", and the entry was named on neither + stream (the collector had replaced the stderr default). Unreadable entries + and unreadable client configs are now reported before every early return. + Relatedly, an entirely unparseable config used to pass `--check` silently + while one mis-typed entry inside it failed — the larger failure was the + quieter one; both now fail. + +### Notes + +`mcpm sync --json` is a **frozen** contract (`docs/CONTRACTS.md`) and this +change is more than additive: `malformed` is a new field, but membership of +`servers[]` and the `drifted`/`inSync` counts also change, and `sync --check` +flips **0 → 2** for a config that previously passed by being unreadable. The +change is deliberate — the old exit 0 was the bug — but it is a contract +change, not a field addition. + ## [0.36.0] - 2026-09-04 ### Fixed diff --git a/docs/CONTRACTS.md b/docs/CONTRACTS.md index 717c9bc..193c747 100644 --- a/docs/CONTRACTS.md +++ b/docs/CONTRACTS.md @@ -16,7 +16,7 @@ do-not-proceed. | `mcpm up --frozen` | lockfile verified, applied | `1` | fail-closed pre-install verify: blocks on integrity drift, an unverifiable record, a format mismatch, or a missing stack/lock | | `mcpm verify` | lockfile integrity verified | `1` | repo-only, **client-free** CI gate: the same fail-closed integrity pass as `up --frozen` (drift / unverifiable / format mismatch / suspicious missing baseline), plus `1` when no lock file is found. `--json` emits the verify model | | `mcpm up --ci` | applied, no prompts | `1` | non-interactive; also non-zero on shadow collisions when combined with `--check-shadowing` | -| `mcpm sync --check` | all clients in sync | **`2`** on drift/conflict; `1` on error | **`2` is the drift signal** — the value CI consumes. `--json` emits the drift model | +| `mcpm sync --check` | all clients in sync **and every config readable** | **`2`** on drift/conflict, on an entry that failed shape validation, or on a config that could not be read at all; `1` on error | **`2` is the drift signal** — the value CI consumes. `--json` emits the drift model. Since 0.37.0 (#59), `2` also covers "could not verify": previously an unreadable entry or config exited `0`, reporting in-sync over input never compared | | `mcpm audit` | scan complete | `1` when overall trust level is **risky**; **`2`** when the invocation cannot be satisfied | advisory findings (e.g. a delisted/deprecated server) lower the score but do not by themselves flip the exit. `2` is scoped to four invocations mcpm refuses outright: `--min-trust` above the highest score audit could produce for *every* scanned server, `--fix --json` without `--yes`, `--min-trust` without `--fix`, and `--sarif` with `--fix`. It is **not** a general "usage errors exit 2" promise — Commander's own argument-parse failures (e.g. `--min-trust 150`) still exit `1` | | `mcpm doctor` | no blocking issues | `1` | health check; the cross-client advisory section never changes the exit code | | `mcpm install` | installed | `1` | non-zero on a policy/trust block (`--min-trust`, `--min-release-age`, a registry-**deleted** server) or any failure | @@ -41,6 +41,14 @@ added or renamed — with one exception: - **`mcpm sync --json`** (the drift model) is **frozen** because CI consumes it alongside the exit-`2` contract above. + **Changed in 0.37.0 (#59), deliberately and not additively:** `ServerDrift` + gains `malformed` and `DriftModel` gains `unreadableClients`, but membership + of `servers[]` and the `drifted`/`inSync` counts also change — a server whose + entry failed shape validation used to be absent from the model entirely, and + a client whose config could not be parsed contributed nothing. `sync --check` + therefore now exits **2** where it previously exited **0** for a config mcpm + could not read. The old exit `0` was the bug: the gate reported "in sync" + over input it had never compared. The remaining `--json` shapes stabilize per-command as they are schema-typed and documented; until then, pin to the exit codes, not the field names. diff --git a/src/__tests__/commands/diff.test.ts b/src/__tests__/commands/diff.test.ts index c692119..23dda59 100644 --- a/src/__tests__/commands/diff.test.ts +++ b/src/__tests__/commands/diff.test.ts @@ -334,3 +334,126 @@ servers: ); }); }); + +// --------------------------------------------------------------------------- +// #59: a declared server whose client entry read() dropped for failing shape +// validation was reported "missing" — a false statement that sends the user to +// `mcpm up` to re-install over an entry they only need to fix. +// --------------------------------------------------------------------------- + +function makeSkippingAdapter(skip: string[], servers: Record = {}) { + return { + read: vi.fn().mockImplementation(async (_p: string, onSkip?: (n: string) => void) => { + for (const n of skip) onSkip?.(n); + return { ...servers }; + }), + }; +} + +describe("handleDiff — malformed entries", () => { + it("reports a declared-but-unreadable entry as unreadable, not missing", async () => { + const stackPath = await writeStackAndLock(basicStack, basicLock); + const deps = makeDeps({ + getAdapter: vi.fn().mockReturnValue(makeSkippingAdapter(["io.github.test/server-a"])), + }); + + await handleDiff({ stackFile: stackPath }, deps); + + const text = (deps.output as ReturnType).mock.calls.map((c) => c[0]).join("\n"); + expect(text).toContain("Unreadable"); + expect(text).toMatch(/1 unreadable/); + // The false claim: it must not be listed under Missing. + expect(text).not.toMatch(/Missing \(in mcpm\.yaml but not installed\):/); + }); + + it("surfaces an UNDECLARED unreadable entry (in neither loop otherwise)", async () => { + const stackPath = await writeStackAndLock(basicStack, basicLock); + const deps = makeDeps({ + getAdapter: vi.fn().mockReturnValue( + makeSkippingAdapter(["some-other-server"], { + "io.github.test/server-a": { command: "npx", args: ["-y", "@test/server-a@1.2.0"] }, + }) + ), + }); + + await handleDiff({ stackFile: stackPath }, deps); + + const text = (deps.output as ReturnType).mock.calls.map((c) => c[0]).join("\n"); + expect(text).toContain("some-other-server"); + expect(text).toMatch(/1 unreadable/); + }); + + it("emits status \"unreadable\" under --json", async () => { + const stackPath = await writeStackAndLock(basicStack, basicLock); + const deps = makeDeps({ + getAdapter: vi.fn().mockReturnValue(makeSkippingAdapter(["io.github.test/server-a"])), + }); + + await handleDiff({ stackFile: stackPath, json: true }, deps); + + const parsed = JSON.parse((deps.output as ReturnType).mock.calls[0][0]); + const row = parsed.find((e: { name: string }) => e.name === "io.github.test/server-a"); + expect(row.status).toBe("unreadable"); + expect(row.clients).toEqual(["claude-desktop"]); + }); + + it("still reports a genuinely absent server as missing (negative control)", async () => { + const stackPath = await writeStackAndLock(basicStack, basicLock); + const deps = makeDeps({ getAdapter: vi.fn().mockReturnValue(makeSkippingAdapter([])) }); + + await handleDiff({ stackFile: stackPath }, deps); + + const text = (deps.output as ReturnType).mock.calls.map((c) => c[0]).join("\n"); + expect(text).toContain("Missing"); + expect(text).not.toContain("Unreadable"); + }); +}); + +describe("handleDiff — malformed in one client, VALID in another", () => { + it("reports the unreadable copy alongside the readable one", async () => { + // Regression this closes: gating on `installed.has(name)` meant NEITHER + // loop fired, and because diff passes its own onSkip the default stderr + // warning was gone too — "1 in sync" over a config it could not read, + // with nothing on either stream. Worse than the behaviour it replaced. + const stackPath = await writeStackAndLock(basicStack, basicLock); + const deps = makeDeps({ + detectClients: vi + .fn<() => Promise>() + .mockResolvedValue(["claude-desktop", "cursor"]), + getAdapter: vi.fn().mockImplementation((id: ClientId) => + id === "claude-desktop" + ? makeSkippingAdapter(["io.github.test/server-a"]) + : makeSkippingAdapter([], { + "io.github.test/server-a": { command: "npx", args: ["-y", "@test/server-a@1.2.0"] }, + }) + ), + }); + + await handleDiff({ stackFile: stackPath }, deps); + + const text = (deps.output as ReturnType).mock.calls.map((c) => c[0]).join("\n"); + expect(text).toContain("Unreadable"); + expect(text).toMatch(/1 unreadable/); + // and the readable copy is still reported as in sync + expect(text).toContain("In sync:"); + }); + + it("does not append \", 0 unreadable\" when there are none", async () => { + const stackPath = await writeStackAndLock(basicStack, basicLock); + const deps = makeDeps({ getAdapter: vi.fn().mockReturnValue(makeSkippingAdapter([])) }); + await handleDiff({ stackFile: stackPath }, deps); + const text = (deps.output as ReturnType).mock.calls.map((c) => c[0]).join("\n"); + expect(text).not.toContain("unreadable"); + }); + + it("sanitizes a config-supplied name before it reaches the terminal", async () => { + const stackPath = await writeStackAndLock(basicStack, basicLock); + const deps = makeDeps({ + getAdapter: vi.fn().mockReturnValue(makeSkippingAdapter(["ev\u001b]0;PWNED\u0007il"])), + }); + await handleDiff({ stackFile: stackPath }, deps); + const text = (deps.output as ReturnType).mock.calls.map((c) => c[0]).join("\n"); + expect(text).toContain("PWNED"); // the name is still shown... + expect(text).not.toContain("\u001b"); // ...but the escape is gone + }); +}); diff --git a/src/__tests__/commands/doctor.test.ts b/src/__tests__/commands/doctor.test.ts index 35d9c1a..a2ff828 100644 --- a/src/__tests__/commands/doctor.test.ts +++ b/src/__tests__/commands/doctor.test.ts @@ -37,9 +37,6 @@ function makeAdapter( read: vi.fn().mockImplementation(() => shouldThrow ? Promise.reject(new SyntaxError("Unexpected token")) : Promise.resolve(servers) ), - read: vi.fn().mockImplementation(() => - shouldThrow ? Promise.reject(new SyntaxError("Unexpected token")) : Promise.resolve(servers) - ), addServer: vi.fn().mockResolvedValue(undefined), removeServer: vi.fn().mockResolvedValue(undefined), }; @@ -508,3 +505,138 @@ describe("doctorHandler — plaintext-secret scan (F9)", () => { expect(cap.text()).not.toContain("mcpm secrets set"); // env-only advice suppressed }); }); + +// --------------------------------------------------------------------------- +// #59: doctor already raises a DoctorIssue for an entry read() dropped, but its +// cross-client section built the drift model WITHOUT those names — so the same +// report said "this server is absent from claude-desktop" about a server +// claude-desktop has. The two halves must not contradict each other. +// --------------------------------------------------------------------------- + +describe("buildDoctorModel — cross-client view agrees with the malformed-entry issue", () => { + it("does not report a malformed entry's client as absent", async () => { + const good: McpServerEntry = { command: "npx", args: ["-y", "fs"] }; + const deps = makeHealthyDeps({ + detectClients: vi.fn().mockResolvedValue(["claude-desktop", "cursor"] as ClientId[]), + getAdapter: vi.fn().mockImplementation((id: ClientId) => ({ + clientId: id, + read: vi.fn().mockImplementation(async (_p: string, onSkip?: (n: string) => void) => { + if (id === "claude-desktop") { + onSkip?.("fs"); + return {}; + } + return { fs: good }; + }), + addServer: vi.fn().mockResolvedValue(undefined), + removeServer: vi.fn().mockResolvedValue(undefined), + })), + }); + + const model = await buildDoctorModel(deps); + + // The issue is raised (pre-existing #23 behavior)... + expect(model.issues.some((i) => i.kind === "malformed-config")).toBe(true); + // ...and the cross-client section must not contradict it by calling the + // same server absent from the client that holds it. + const absentEntries = (model.crossClient?.drift ?? []).filter( + (d) => d.name === "fs" && d.absent.includes("claude-desktop") + ); + expect(absentEntries).toEqual([]); + // Positive control: the entry must actually BE in the drift list, or the + // assertion above passes vacuously. + const fs = model.crossClient!.drift.find((d) => d.name === "fs"); + expect(fs?.malformed).toEqual(["claude-desktop"]); + }); + + it("names the holding client when the ONLY holder's entry is unreadable", async () => { + // Found by dogfooding: this rendered as "in ; missing in cursor" — an empty + // present list, and no mention of the client that actually has the server. + const deps = makeHealthyDeps({ + detectClients: vi.fn().mockResolvedValue(["claude-desktop", "cursor"] as ClientId[]), + getAdapter: vi.fn().mockImplementation((id: ClientId) => ({ + clientId: id, + read: vi.fn().mockImplementation(async (_p: string, onSkip?: (n: string) => void) => { + if (id === "claude-desktop") onSkip?.("fs"); + return {}; + }), + addServer: vi.fn().mockResolvedValue(undefined), + removeServer: vi.fn().mockResolvedValue(undefined), + })), + }); + + const model = await buildDoctorModel(deps); + const fs = model.crossClient!.drift.find((d) => d.name === "fs")!; + expect(fs.kind).toBe("unreadable"); + expect(fs.malformed).toEqual(["claude-desktop"]); + expect(fs.present).toEqual([]); + }); +}); + +// --------------------------------------------------------------------------- +// #59: the model-level assertions above do not cover the RENDER, which is what +// the user actually reads. A mutant that never matched the "unreadable" branch +// regenerated `in ; missing in cursor` verbatim — the bug dogfooding found — +// with the whole suite green. +// --------------------------------------------------------------------------- + +describe("doctorHandler — cross-client render of unreadable entries", () => { + const twoClientsOnly = (id: ClientId) => id === "claude-desktop" || id === "cursor"; + + function skippingAdapter(id: ClientId, skip: string[], servers: Record) { + return { + clientId: id, + read: vi.fn().mockImplementation(async (_p: string, onSkip?: (n: string) => void) => { + for (const n of skip) onSkip?.(n); + return servers; + }), + addServer: vi.fn().mockResolvedValue(undefined), + removeServer: vi.fn().mockResolvedValue(undefined), + } as unknown as ConfigAdapter; + } + + it("names the holding client when the ONLY holder's entry is unreadable", async () => { + const cap = captureOutput(); + const deps = makeHealthyDeps({ + checkConfigExists: vi.fn((id: ClientId) => Promise.resolve(twoClientsOnly(id))), + getAdapter: vi.fn((id: ClientId) => + id === "claude-desktop" ? skippingAdapter(id, ["fs"], {}) : skippingAdapter(id, [], {}) + ), + output: cap.fn, + }); + await doctorHandler(deps); + expect(cap.text()).toMatch(/⚠ fs — entry in claude-desktop does not match the expected shape/); + // The bug this replaces rendered an empty present list: + expect(cap.text()).not.toMatch(/⚠ fs — in ;/); + }); + + it("names the unreadable client alongside the readable one", async () => { + const cap = captureOutput(); + const deps = makeHealthyDeps({ + checkConfigExists: vi.fn((id: ClientId) => Promise.resolve(twoClientsOnly(id))), + getAdapter: vi.fn((id: ClientId) => + id === "claude-desktop" + ? skippingAdapter(id, ["fs"], {}) + : skippingAdapter(id, [], { fs: { command: "npx", args: ["fs"] } }) + ), + output: cap.fn, + }); + await doctorHandler(deps); + expect(cap.text()).toMatch(/⚠ fs — in cursor; unreadable in claude-desktop/); + }); + + it("sanitizes a config-supplied name before it reaches the terminal", async () => { + const cap = captureOutput(); + const deps = makeHealthyDeps({ + checkConfigExists: vi.fn((id: ClientId) => Promise.resolve(twoClientsOnly(id))), + getAdapter: vi.fn((id: ClientId) => + id === "claude-desktop" + ? skippingAdapter(id, ["ev\u001b]0;PWNED\u0007il"], {}) + : skippingAdapter(id, [], {}) + ), + output: cap.fn, + }); + await doctorHandler(deps); + expect(cap.text()).toContain("PWNED"); + expect(cap.text()).not.toContain("\u001b"); + }); +}); diff --git a/src/__tests__/commands/export.test.ts b/src/__tests__/commands/export.test.ts index bc06a5a..77c8cc4 100644 --- a/src/__tests__/commands/export.test.ts +++ b/src/__tests__/commands/export.test.ts @@ -252,3 +252,151 @@ describe("handleExport", () => { expect(Object.keys(stack.servers)).toHaveLength(1); }); }); + +// --------------------------------------------------------------------------- +// #59: an entry read() dropped is silently ABSENT from the export, and the user +// keeps the result as their declared stack. It must never look complete. +// --------------------------------------------------------------------------- + +describe("handleExport — unreadable entries", () => { + it("names the entries it could not read on stderr", async () => { + const errs: string[] = []; + const spy = vi + .spyOn(process.stderr, "write") + .mockImplementation((chunk: string | Uint8Array) => { + errs.push(String(chunk)); + return true; + }); + try { + const deps = makeDeps({ + detectClients: vi.fn<() => Promise>().mockResolvedValue(["claude-desktop"]), + getAdapter: vi.fn().mockReturnValue({ + read: vi.fn().mockImplementation(async (_p: string, onSkip?: (n: string) => void) => { + onSkip?.("broken-server"); + return { ok: { command: "npx", args: ["-y", "ok"] } }; + }), + }), + }); + await handleExport({} as ExportOptions, deps); + expect(errs.join("")).toContain("broken-server"); + expect(errs.join("")).toMatch(/NOT in this export/); + } finally { + spy.mockRestore(); + } + }); + + it("stays quiet when ANOTHER client supplied a readable copy of the same name", async () => { + // Found by dogfooding: warning "NOT in this export" about a server the + // export DOES contain (via the other client) is its own false statement. + const errs: string[] = []; + const spy = vi + .spyOn(process.stderr, "write") + .mockImplementation((chunk: string | Uint8Array) => { + errs.push(String(chunk)); + return true; + }); + try { + const deps = makeDeps({ + detectClients: vi + .fn<() => Promise>() + .mockResolvedValue(["claude-desktop", "cursor"]), + getAdapter: vi.fn().mockImplementation((id: ClientId) => + id === "claude-desktop" + ? { + read: vi + .fn() + .mockImplementation(async (_p: string, onSkip?: (n: string) => void) => { + onSkip?.("shared"); + return {}; + }), + } + : makeAdapter({ shared: { command: "npx", args: ["-y", "shared"] } }) + ), + }); + await handleExport({} as ExportOptions, deps); + expect(errs.join("")).not.toMatch(/NOT in this export/); + } finally { + spy.mockRestore(); + } + }); + + it("sanitizes a config-supplied name before it reaches the terminal", async () => { + const errs: string[] = []; + const spy = vi + .spyOn(process.stderr, "write") + .mockImplementation((chunk: string | Uint8Array) => { + errs.push(String(chunk)); + return true; + }); + try { + const deps = makeDeps({ + detectClients: vi.fn<() => Promise>().mockResolvedValue(["claude-desktop"]), + getAdapter: vi.fn().mockReturnValue({ + read: vi.fn().mockImplementation(async (_p: string, onSkip?: (n: string) => void) => { + onSkip?.("ev\u001b]0;PWNED\u0007il"); + return {}; + }), + }), + }); + await handleExport({} as ExportOptions, deps); + expect(errs.join("")).toContain("PWNED"); + expect(errs.join("")).not.toContain("\u001b"); + } finally { + spy.mockRestore(); + } + }); + + it("writes nothing to stderr for a fully readable config (negative control)", async () => { + const errs: string[] = []; + const spy = vi + .spyOn(process.stderr, "write") + .mockImplementation((chunk: string | Uint8Array) => { + errs.push(String(chunk)); + return true; + }); + try { + const deps = makeDeps({ + detectClients: vi.fn<() => Promise>().mockResolvedValue(["claude-desktop"]), + getAdapter: vi + .fn() + .mockReturnValue(makeAdapter({ ok: { command: "npx", args: ["-y", "ok"] } })), + }); + await handleExport({} as ExportOptions, deps); + expect(errs.join("")).not.toMatch(/NOT in this export/); + } finally { + spy.mockRestore(); + } + }); +}); + +describe("handleExport — Object.prototype-named entries", () => { + it("counts a malformed entry named `toString` (a `name in servers` hazard)", async () => { + // `name in servers` walks the prototype chain, so "toString" read as + // already-exported and vanished from both the warning and the count. + const errs: string[] = []; + const spy = vi + .spyOn(process.stderr, "write") + .mockImplementation((chunk: string | Uint8Array) => { + errs.push(String(chunk)); + return true; + }); + try { + const deps = makeDeps({ + detectClients: vi.fn<() => Promise>().mockResolvedValue(["claude-desktop"]), + getAdapter: vi.fn().mockReturnValue({ + read: vi.fn().mockImplementation(async (_p: string, onSkip?: (n: string) => void) => { + onSkip?.("toString"); + onSkip?.("real"); + return {}; + }), + }), + }); + await handleExport({} as ExportOptions, deps); + const text = errs.join(""); + expect(text).toContain("toString"); + expect(text).toMatch(/2 server entries/); + } finally { + spy.mockRestore(); + } + }); +}); diff --git a/src/__tests__/commands/import.test.ts b/src/__tests__/commands/import.test.ts index 9d80a14..26fe1d7 100644 --- a/src/__tests__/commands/import.test.ts +++ b/src/__tests__/commands/import.test.ts @@ -46,7 +46,6 @@ function makeAdapter( read: vi.fn().mockResolvedValue(servers), addServer: vi.fn(), removeServer: vi.fn(), - read: vi.fn().mockResolvedValue(servers), }; } @@ -499,7 +498,6 @@ describe("handleImport — adapter errors", () => { read: vi.fn().mockRejectedValue(new Error("Permission denied")), addServer: vi.fn(), removeServer: vi.fn(), - read: vi.fn().mockRejectedValue(new Error("Permission denied")), }; const deps = makeDeps({ detectClients: vi.fn().mockResolvedValue(["claude-desktop", "cursor"]), @@ -752,3 +750,66 @@ describe("handleImport — immutability", () => { expect(originalName).toBe("filesystem"); }); }); + +// --------------------------------------------------------------------------- +// #59: import is the "bring my existing setup in" path, so an entry that never +// appears in the pick-list is the worst place to stay quiet — and reporting it +// AFTER the empty-result return would leave "No existing MCP servers found" +// standing as a false statement. +// --------------------------------------------------------------------------- + +describe("handleImport — unreadable entries", () => { + it("names them, even when they were the ONLY entries present", async () => { + const lines: string[] = []; + const deps = makeDeps({ + detectClients: vi.fn().mockResolvedValue(["claude-desktop"]), + getAdapter: vi.fn().mockReturnValue({ + clientId: "claude-desktop", + read: vi.fn().mockImplementation(async (_p: string, onSkip?: (n: string) => void) => { + onSkip?.("broken-server"); + return {}; + }), + addServer: vi.fn(), + removeServer: vi.fn(), + }), + output: (t: string) => lines.push(t), + }); + + await handleImport({}, deps); + + const text = lines.join("\n"); + expect(text).toContain("broken-server"); + expect(text).toMatch(/cannot be imported/); + // ...and the follow-on line must not contradict it. "No existing MCP + // servers found" is false when servers WERE found and merely unreadable. + expect(text).not.toContain("No existing MCP servers found"); + expect(text).toContain("No importable MCP servers found"); + expect(text.indexOf("broken-server")).toBeLessThan(text.indexOf("No importable")); + }); +}); + +describe("handleImport — another client supplied a good copy", () => { + it("does not claim a server is un-importable when it IS in the pick-list", async () => { + const lines: string[] = []; + const deps = makeDeps({ + detectClients: vi.fn().mockResolvedValue(["claude-desktop", "cursor"]), + getAdapter: vi.fn().mockImplementation((id: ClientId) => ({ + clientId: id, + read: vi.fn().mockImplementation(async (_p: string, onSkip?: (n: string) => void) => { + if (id === "claude-desktop") { + onSkip?.("shared"); + return {}; + } + return { shared: { command: "npx", args: ["-y", "shared"] } }; + }), + addServer: vi.fn(), + removeServer: vi.fn(), + })), + output: (t: string) => lines.push(t), + confirm: vi.fn().mockResolvedValue(false), + }); + + await handleImport({}, deps); + expect(lines.join("\n")).not.toMatch(/cannot be imported/); + }); +}); diff --git a/src/__tests__/commands/install.test.ts b/src/__tests__/commands/install.test.ts index 0b1db6f..991b626 100644 --- a/src/__tests__/commands/install.test.ts +++ b/src/__tests__/commands/install.test.ts @@ -113,7 +113,6 @@ function makeAdapter( return { clientId, read: vi.fn().mockResolvedValue(servers), - read: vi.fn().mockResolvedValue(servers), addServer: vi.fn().mockResolvedValue(undefined), removeServer: vi.fn().mockResolvedValue(undefined), }; diff --git a/src/__tests__/commands/list.test.ts b/src/__tests__/commands/list.test.ts index 07efafe..af500e4 100644 --- a/src/__tests__/commands/list.test.ts +++ b/src/__tests__/commands/list.test.ts @@ -31,7 +31,6 @@ const VSCODE_SERVERS: Record = { function makeMockAdapter(servers: Record = {}) { return { - read: vi.fn().mockResolvedValue(servers), read: vi.fn().mockResolvedValue(servers), addServer: vi.fn(), removeServer: vi.fn(), @@ -64,8 +63,10 @@ describe("handleList — multiple clients", () => { const deps: ListDeps = { detectClients, getAdapter, getPath, output }; await handleList({}, deps); - expect(claudeAdapter.read).toHaveBeenCalledWith("/fake/path/config.json"); - expect(cursorAdapter.read).toHaveBeenCalledWith("/fake/path/config.json"); + // #59: read() now takes an onSkip callback; the assertion is about WHICH + // client configs were read, not the call's arity. + expect(claudeAdapter.read).toHaveBeenCalledWith("/fake/path/config.json", expect.any(Function)); + expect(cursorAdapter.read).toHaveBeenCalledWith("/fake/path/config.json", expect.any(Function)); }); it("displays Client and Server Name columns", async () => { @@ -434,3 +435,85 @@ describe("handleList — Command/URL column", () => { expect(lines.join("\n")).toContain("https://tools.example.com/mcp"); }); }); + +describe("handleList — unreadable entries (#59)", () => { + function skipping(skip: string[], servers: Record = {}) { + return { + read: vi.fn().mockImplementation(async (_p: string, onSkip?: (n: string) => void) => { + for (const n of skip) onSkip?.(n); + return servers; + }), + addServer: vi.fn(), + removeServer: vi.fn(), + }; + } + + it("names them, and does not claim nothing is installed", async () => { + const lines: string[] = []; + const deps = { + detectClients: vi.fn().mockResolvedValue(["claude-desktop"]), + getAdapter: vi.fn().mockReturnValue(skipping(["broken"])), + getPath: vi.fn().mockReturnValue("/fake/path/config.json"), + output: (t: string) => lines.push(t), + } as unknown as ListDeps; + + await handleList({}, deps); + const text = lines.join("\n"); + expect(text).toContain("broken"); + expect(text).not.toContain("No MCP servers installed"); + }); + + it("sanitizes a config-supplied name before it reaches the terminal", async () => { + const lines: string[] = []; + const deps = { + detectClients: vi.fn().mockResolvedValue(["claude-desktop"]), + getAdapter: vi.fn().mockReturnValue(skipping(["ev\u001b]0;PWNED\u0007il"])), + getPath: vi.fn().mockReturnValue("/fake/path/config.json"), + output: (t: string) => lines.push(t), + } as unknown as ListDeps; + + await handleList({}, deps); + expect(lines.join("\n")).toContain("PWNED"); + expect(lines.join("\n")).not.toContain("\u001b"); + }); + + it("keeps --json a bare array and puts the notice on stderr", async () => { + // Flipping array->object only in the malformed case would break + // `JSON.parse(out).map(...)` exactly when things are already wrong. + const lines: string[] = []; + const errs: string[] = []; + const spy = vi + .spyOn(process.stderr, "write") + .mockImplementation((chunk: string | Uint8Array) => { + errs.push(String(chunk)); + return true; + }); + try { + const deps = { + detectClients: vi.fn().mockResolvedValue(["claude-desktop"]), + getAdapter: vi.fn().mockReturnValue(skipping(["broken"])), + getPath: vi.fn().mockReturnValue("/fake/path/config.json"), + output: (t: string) => lines.push(t), + } as unknown as ListDeps; + + await handleList({ json: true }, deps); + expect(Array.isArray(JSON.parse(lines[0]!))).toBe(true); + expect(errs.join("")).toContain("broken"); + } finally { + spy.mockRestore(); + } + }); + + it("keeps --json a bare array when nothing was skipped (shape unchanged)", async () => { + const lines: string[] = []; + const deps = { + detectClients: vi.fn().mockResolvedValue(["claude-desktop"]), + getAdapter: vi.fn().mockReturnValue(skipping([], { ok: { command: "npx" } })), + getPath: vi.fn().mockReturnValue("/fake/path/config.json"), + output: (t: string) => lines.push(t), + } as unknown as ListDeps; + + await handleList({ json: true }, deps); + expect(Array.isArray(JSON.parse(lines[0]!))).toBe(true); + }); +}); diff --git a/src/__tests__/commands/remove.test.ts b/src/__tests__/commands/remove.test.ts index 4cf92c0..1cd326b 100644 --- a/src/__tests__/commands/remove.test.ts +++ b/src/__tests__/commands/remove.test.ts @@ -38,7 +38,6 @@ function makeAdapter( return { clientId, read: vi.fn().mockResolvedValue(servers), - read: vi.fn().mockResolvedValue(servers), addServer: vi.fn().mockResolvedValue(undefined), removeServer: vi.fn().mockResolvedValue(undefined), }; diff --git a/src/__tests__/commands/sync.test.ts b/src/__tests__/commands/sync.test.ts index cd49760..d5916c4 100644 --- a/src/__tests__/commands/sync.test.ts +++ b/src/__tests__/commands/sync.test.ts @@ -115,3 +115,205 @@ describe("exitCodeFor (the --check CI gate)", () => { expect(exitCodeFor(result(false), true)).toBe(0); }); }); + +// #59: drive read()'s onSkip the way the real BaseAdapter does for an entry +// that fails shape validation, so the whole `sync` path (collect -> model -> +// render) is exercised rather than a hand-built model. +function makeDepsWithMalformed( + configs: Partial>>, + malformed: Partial>, + output: (t: string) => void, +): SyncDeps { + const ids = Object.keys(configs) as ClientId[]; + return { + detectClients: vi.fn<() => Promise>().mockResolvedValue(ids), + getAdapter: vi.fn((id: ClientId) => ({ + read: vi.fn().mockImplementation(async (_p: string, onSkip?: (n: string) => void) => { + for (const name of malformed[id] ?? []) onSkip?.(name); + return configs[id] ?? {}; + }), + })), + getPath: vi.fn().mockReturnValue("/mock/config.json"), + output, + }; +} + +describe("handleSync — unreadable entries are not reported as missing", () => { + it("renders ? (not ·) for a client whose entry read() dropped", async () => { + const cap = capture(); + const deps = makeDepsWithMalformed( + { "claude-desktop": {}, cursor: { fs: { command: "npx", args: ["fs"] } } }, + { "claude-desktop": ["fs"] }, + cap.output, + ); + const result = await handleSync({}, deps); + const fs = result.model.servers.find((s) => s.name === "fs")!; + expect(fs.absent).toEqual([]); + expect(fs.malformed).toEqual(["claude-desktop"]); + // Assert on the MATRIX ROW, not the whole output: the detail line below the + // table also contains "?", so a bare toContain("?") passes even when the + // cell is wrong (verified — that assertion survived the cell-order mutant). + const row = cap + .text() + .split("\n") + .find((l) => l.includes("fs") && l.includes("\u2502"))!; + expect(row).toContain("?"); + expect(row).not.toContain("\u00b7"); // not rendered as absent + // The false claim this fixes: it must NOT say the server is missing there. + expect(cap.text()).not.toMatch(/missing in claude-desktop/); + expect(cap.text()).toMatch(/does not match the expected shape/); + }); + + it("sets drift=true (exit 2) when the only holder's entry is unreadable", async () => { + const cap = capture(); + const deps = makeDepsWithMalformed( + { "claude-desktop": {}, cursor: {} }, + { "claude-desktop": ["fs"] }, + cap.output, + ); + const result = await handleSync({}, deps); + expect(result.drift).toBe(true); + expect(exitCodeFor(result, true)).toBe(2); + // The summary must state a cause: "1 drifted (0 missing, 0 conflicts)" is + // a drift count with nothing behind it. + expect(cap.text()).toMatch(/1 drifted \(.*1 unreadable\)/); + }); +}); + +describe("handleSync — unreadable rendering details", () => { + it("does not ALSO list a malformed-only server under missing", async () => { + // present is empty for a malformed-only name, which rendered as + // "· orphan: in ; missing in cursor" beside the ? line, and counted the + // server under "missing in >=1 client" as well. + const cap = capture(); + const deps = makeDepsWithMalformed( + { "claude-desktop": {}, cursor: {} }, + { "claude-desktop": ["orphan"] }, + cap.output, + ); + await handleSync({}, deps); + expect(cap.text()).not.toMatch(/· orphan: in ;/); + expect(cap.text()).toMatch(/also missing in cursor/); + // The fact is stated ONCE in the body (folded into the ? line), but the + // summary sub-count must stay true to its own label: orphan IS missing + // from cursor, so "missing in >=1 client" is 1, not 0. + expect(cap.text()).toMatch(/1 missing in ≥1 client/); + }); + + it("prints a legend entry for the ? cell", async () => { + const cap = capture(); + const deps = makeDepsWithMalformed( + { "claude-desktop": {}, cursor: { fs: { command: "npx" } } }, + { "claude-desktop": ["fs"] }, + cap.output, + ); + await handleSync({}, deps); + expect(cap.text()).toMatch(/legend:.*\? unreadable entry/); + }); + + it("does not append \", 0 unreadable\" when there are none", async () => { + const cap = capture(); + const entry: McpServerEntry = { command: "npx", args: ["fs"] }; + const deps = makeDeps( + { "claude-desktop": { fs: entry }, cursor: { fs: { ...entry } } }, + cap.output, + ); + await handleSync({}, deps); + // The LEGEND always names the ? cell, so assert on the summary line only. + expect(cap.text()).toMatch(/0 shape conflicts\)/); + expect(cap.text()).not.toMatch(/, \d+ unreadable\)/); + }); + + it("sanitizes a config-supplied name before it reaches the terminal", async () => { + const cap = capture(); + const deps = makeDepsWithMalformed( + { "claude-desktop": {}, cursor: {} }, + { "claude-desktop": ["ev\u001b]0;PWNED\u0007il"] }, + cap.output, + ); + await handleSync({}, deps); + expect(cap.text()).toContain("PWNED"); + expect(cap.text()).not.toContain("\u001b"); + }); +}); + +describe("handleSync — reporting must not be gated on the matrix (#59/H1)", () => { + it("names the entry when there is only ONE client, and still exits 2", async () => { + // `drifted` comes from the model, so --check exited 2 while the ONLY line + // printed was "nothing to compare across clients" — a CI failure whose own + // output said there was nothing to look at. Single client is the common + // desktop shape. + const cap = capture(); + const deps = makeDepsWithMalformed({ "claude-desktop": {} }, { "claude-desktop": ["bad"] }, cap.output); + const result = await handleSync({}, deps); + expect(cap.text()).toContain("bad"); + expect(cap.text()).toMatch(/does not match the expected shape/); + expect(exitCodeFor(result, true)).toBe(2); + }); + + it("does not say 'No client configs found' when it just named them", async () => { + // "install a server" is also the wrong remediation for broken JSON. + const cap = capture(); + const deps: SyncDeps = { + detectClients: vi + .fn<() => Promise>() + .mockResolvedValue(["claude-desktop", "cursor"]), + getAdapter: vi.fn(() => ({ + read: vi.fn().mockRejectedValue(new SyntaxError("Unexpected token")), + })), + getPath: vi.fn().mockReturnValue("/mock/config.json"), + output: cap.output, + }; + await handleSync({}, deps); + expect(cap.text()).toMatch(/claude-desktop: config could not be read at all/); + expect(cap.text()).not.toContain("No client configs found"); + expect(cap.text()).toMatch(/No READABLE client configs found/); + }); + + it("counts unreadable SERVERS and unreadable CLIENTS separately", async () => { + // The parenthetical's other terms are server counts; folding a client + // count into them makes one number mean two things. + const cap = capture(); + const deps: SyncDeps = { + detectClients: vi + .fn<() => Promise>() + .mockResolvedValue(["claude-desktop", "cursor", "vscode"]), + getAdapter: vi.fn((id: ClientId) => ({ + read: + id === "vscode" + ? vi.fn().mockRejectedValue(new SyntaxError("Unexpected token")) + : vi.fn().mockImplementation(async (_p: string, onSkip?: (n: string) => void) => { + if (id === "claude-desktop") onSkip?.("alpha"); + return { beta: { command: "npx" } }; + }), + })), + getPath: vi.fn().mockReturnValue("/mock/config.json"), + output: cap.output, + }; + await handleSync({}, deps); + expect(cap.text()).toMatch(/1 unreadable\)/); + expect(cap.text()).toMatch(/1 client config\(s\) unreadable/); + }); + + it("names a client whose whole config is unparseable, and fails --check", async () => { + // Otherwise the LARGER failure is the quieter one: one mis-typed entry + // failed CI while an entirely broken config passed silently. + const cap = capture(); + const deps: SyncDeps = { + detectClients: vi + .fn<() => Promise>() + .mockResolvedValue(["claude-desktop", "cursor"]), + getAdapter: vi.fn((id: ClientId) => ({ + read: + id === "claude-desktop" + ? vi.fn().mockRejectedValue(new SyntaxError("Unexpected token")) + : vi.fn().mockResolvedValue({ fs: { command: "npx" } }), + })), + getPath: vi.fn().mockReturnValue("/mock/config.json"), + output: cap.output, + }; + const result = await handleSync({}, deps); + expect(cap.text()).toMatch(/claude-desktop: config could not be read at all/); + expect(exitCodeFor(result, true)).toBe(2); + }); +}); diff --git a/src/__tests__/commands/up.test.ts b/src/__tests__/commands/up.test.ts index 36cd397..7432306 100644 --- a/src/__tests__/commands/up.test.ts +++ b/src/__tests__/commands/up.test.ts @@ -1073,3 +1073,104 @@ servers: expect(findingsPassedToScore(deps).some((f) => f.type === "release-cooldown")).toBe(false); }); }); + +// --------------------------------------------------------------------------- +// #59 (was #23's deferred sub-gap): --strict must not silently leave a +// malformed, undeclared entry behind while reporting a clean reconciliation. +// The fail-safe direction is kept — it is NOT deleted — but it is reported. +// --------------------------------------------------------------------------- + +describe("handleUp --strict — malformed undeclared entry", () => { + it("reports it instead of silently leaving it, and does NOT delete it", async () => { + const stackPath = await writeStackAndLock(basicStack, basicLock); + const adapter = makeAdapter(); + (adapter.read as ReturnType).mockImplementation( + async (_p: string, onSkip?: (n: string) => void) => { + onSkip?.("broken-extra"); + return {}; + } + ); + const lines: string[] = []; + const recorded: Array<{ name: string; status: string }> = []; + const deps = makeDeps({ + getAdapter: vi.fn().mockReturnValue(adapter), + output: (t: string) => lines.push(t), + recordResult: (r: { name: string; status: string }) => recorded.push(r), + }); + + await handleUp({ stackFile: stackPath, strict: true, yes: true }, deps); + + // Fail-safe: never delete an entry mcpm could not read. + expect(adapter.removeServer).not.toHaveBeenCalledWith("/mock/config.json", "broken-extra"); + // But say so — silence was the bug, not the refusal to delete. + expect(lines.join("\n")).toContain("broken-extra"); + expect(lines.join("\n")).toMatch(/does not match the expected shape/); + // And say so on the MACHINE-READABLE channel too: `recordResult` is the + // mcpm_up MCP surface's only signal — that surface has no stderr an agent + // can see, which is the whole reason v0.34.0 added `skipped` fields. + expect(recorded).toContainEqual({ name: "broken-extra", status: "skipped" }); + }); + + it("sanitizes a config-supplied name before it reaches the terminal", async () => { + const stackPath = await writeStackAndLock(basicStack, basicLock); + const adapter = makeAdapter(); + (adapter.read as ReturnType).mockImplementation( + async (_p: string, onSkip?: (n: string) => void) => { + onSkip?.("ev\u001b]0;PWNED\u0007il"); + return {}; + } + ); + const lines: string[] = []; + const deps = makeDeps({ + getAdapter: vi.fn().mockReturnValue(adapter), + output: (t: string) => lines.push(t), + }); + + await handleUp({ stackFile: stackPath, strict: true, yes: true }, deps); + + expect(lines.join("\n")).toContain("PWNED"); + expect(lines.join("\n")).not.toContain("\u001b"); + }); + + it("says nothing about a DECLARED server whose entry is malformed", async () => { + // --strict only reconciles servers absent from mcpm.yaml. Reporting a + // declared one as "not in mcpm.yaml" would be a false statement. + const stackPath = await writeStackAndLock(basicStack, basicLock); + const adapter = makeAdapter(); + (adapter.read as ReturnType).mockImplementation( + async (_p: string, onSkip?: (n: string) => void) => { + onSkip?.("io.github.test/server-a"); // the DECLARED name + return {}; + } + ); + const lines: string[] = []; + const recorded: Array<{ name: string; status: string }> = []; + const deps = makeDeps({ + getAdapter: vi.fn().mockReturnValue(adapter), + output: (t: string) => lines.push(t), + recordResult: (r: { name: string; status: string }) => recorded.push(r), + }); + + await handleUp({ stackFile: stackPath, strict: true, yes: true }, deps); + + // NB: assert on `recorded` and on the rendered line separately. An earlier + // assertion here matched "not in mcpm.yaml", which lives only in + // `results[].message` and is never rendered on the strict-removal path — + // so it could not fail, whatever the code did. + expect(lines.join("\n")).not.toContain("io.github.test/server-a: not removed"); + expect(recorded).not.toContainEqual({ + name: "io.github.test/server-a", + status: "skipped", + }); + }); + + it("still removes a well-formed undeclared entry (negative control)", async () => { + const stackPath = await writeStackAndLock(basicStack, basicLock); + const adapter = makeAdapter({ "extra-server": { command: "npx", args: ["-y", "extra"] } }); + const deps = makeDeps({ getAdapter: vi.fn().mockReturnValue(adapter) }); + + await handleUp({ stackFile: stackPath, strict: true, yes: true }, deps); + + expect(adapter.removeServer).toHaveBeenCalledWith("/mock/config.json", "extra-server"); + }); +}); diff --git a/src/__tests__/commands/update.test.ts b/src/__tests__/commands/update.test.ts index fe6d568..7b063d1 100644 --- a/src/__tests__/commands/update.test.ts +++ b/src/__tests__/commands/update.test.ts @@ -80,7 +80,6 @@ function makeAdapter(clientId: ClientId): ConfigAdapter { return { clientId, read: vi.fn().mockResolvedValue({}), - read: vi.fn().mockResolvedValue({}), addServer: vi.fn().mockResolvedValue(undefined), removeServer: vi.fn().mockResolvedValue(undefined), }; @@ -590,3 +589,482 @@ describe("handleUpdate — multiple servers mixed state", () => { expect(out).toContain("server-a"); }); }); + +// --------------------------------------------------------------------------- +// #59 / #23 regression: `readExistingEnv` reads through `BaseAdapter.read()`, +// which since #23 (v0.34.0) DROPS an entry failing shape validation. A user +// whose entry was malformed in one field (e.g. `args: "bad"` from a hand-edit) +// but whose `env` held real API keys therefore got `undefined` back, and the +// `force: true` re-write wiped those keys while printing "✓ Updated". +// The existing "preserves existing client-config env" test above cannot see +// this: it mocks read() to RETURN the entry, the one thing the real read() +// stopped doing. +// --------------------------------------------------------------------------- + +/** Mimic the real read(): a malformed entry goes to onSkip, never to the map. */ +function readDropping(malformed: Record, valid: Record = {}) { + return vi + .fn() + .mockImplementation(async (_p: string, onSkip?: (n: string, raw: unknown) => void) => { + for (const [name, raw] of Object.entries(malformed)) onSkip?.(name, raw); + return { ...valid }; + }); +} + +describe("handleUpdate — malformed client entry must not silently wipe env", () => { + it("recovers the env block from the raw entry and REPAIRS the entry", async () => { + const adapter = makeAdapter("claude-desktop"); + (adapter.read as ReturnType).mockImplementation( + readDropping({ + "io.github.test/server-a": { + command: "npx", + args: "-y @test/server", // the malformation + env: { MY_KEY: "user-value" }, + }, + }) + ); + const deps = makeDeps({ + getInstalledServers: vi.fn().mockResolvedValue([ + makeInstalledServer({ name: "io.github.test/server-a", version: "1.0.0", clients: ["claude-desktop"] }), + ]), + getServer: vi.fn().mockResolvedValue(makeServerEntry("io.github.test/server-a", "1.1.0")), + getAdapter: vi.fn().mockReturnValue(adapter), + }); + + await handleUpdate({ yes: true }, deps); + + // The write MUST happen — overwriting a mis-shaped entry with a freshly + // resolved one is the user's self-repair path. Refusing it would convert a + // self-healing case into a permanently stuck one. + expect(adapter.addServer).toHaveBeenCalledTimes(1); + const call = (adapter.addServer as ReturnType).mock.calls[0]; + // ...and it must carry the secrets forward. + expect(call[2].env.MY_KEY).toBe("user-value"); + // ...and the repaired entry must have a well-formed args array. + expect(Array.isArray(call[2].args)).toBe(true); + }); + + it("still repairs a malformed entry that has NO env to recover", async () => { + const adapter = makeAdapter("claude-desktop"); + (adapter.read as ReturnType).mockImplementation( + readDropping({ "io.github.test/server-a": { command: "npx", args: "-y @test/server" } }) + ); + const deps = makeDeps({ + getInstalledServers: vi.fn().mockResolvedValue([ + makeInstalledServer({ name: "io.github.test/server-a", version: "1.0.0", clients: ["claude-desktop"] }), + ]), + getServer: vi.fn().mockResolvedValue(makeServerEntry("io.github.test/server-a", "1.1.0")), + getAdapter: vi.fn().mockReturnValue(adapter), + }); + + await handleUpdate({ yes: true }, deps); + expect(adapter.addServer).toHaveBeenCalledTimes(1); + }); + + it("ignores an env recovered from a DIFFERENT malformed entry", async () => { + // The onSkip callback fires for every malformed entry in the config, not + // just the one being updated. Without the name filter, an unrelated broken + // neighbour's env would be grafted onto this server. + const adapter = makeAdapter("claude-desktop"); + (adapter.read as ReturnType).mockImplementation( + readDropping( + { "some-other-server": { command: "npx", args: "bad", env: { LEAKED: "from-neighbour" } } }, + { "io.github.test/server-a": { command: "npx", args: ["-y", "@test/server"] } } + ) + ); + const deps = makeDeps({ + getInstalledServers: vi.fn().mockResolvedValue([ + makeInstalledServer({ name: "io.github.test/server-a", version: "1.0.0", clients: ["claude-desktop"] }), + ]), + getServer: vi.fn().mockResolvedValue(makeServerEntry("io.github.test/server-a", "1.1.0")), + getAdapter: vi.fn().mockReturnValue(adapter), + }); + + await handleUpdate({ yes: true }, deps); + + expect(adapter.addServer).toHaveBeenCalledTimes(1); + const call = (adapter.addServer as ReturnType).mock.calls[0]; + expect(call[2].env?.LEAKED).toBeUndefined(); + }); + + it("keeps the string keys when env ITSELF is the malformed field", async () => { + // A numeric port is the archetypal hand-edit, and it is what makes the + // entry invalid. Parsing the whole env record then rejected EVERY key and + // destroyed the API key beside the bad one — the exact loss this fix + // exists to prevent, in the population it targets. + const adapter = makeAdapter("claude-desktop"); + (adapter.read as ReturnType).mockImplementation( + readDropping({ + "io.github.test/server-a": { + command: "npx", + args: ["-y", "@test/server"], + env: { API_KEY: "s3cret", PORT: 8080 }, + }, + }) + ); + const lines: string[] = []; + const deps = makeDeps({ + getInstalledServers: vi.fn().mockResolvedValue([ + makeInstalledServer({ name: "io.github.test/server-a", version: "1.0.0", clients: ["claude-desktop"] }), + ]), + getServer: vi.fn().mockResolvedValue(makeServerEntry("io.github.test/server-a", "1.1.0")), + getAdapter: vi.fn().mockReturnValue(adapter), + output: (t: string) => lines.push(t), + }); + + await handleUpdate({ yes: true }, deps); + + const call = (adapter.addServer as ReturnType).mock.calls[0]; + expect(call[2].env.API_KEY).toBe("s3cret"); + expect(call[2].env.PORT).toBeUndefined(); + // and the key that could not be carried is NAMED, not dropped in silence + expect(lines.join("\n")).toContain("PORT"); + // ...as a NOTE. "could not update claude-desktop" would be false: it did. + expect(lines.join("\n")).toContain("(note:"); + expect(lines.join("\n")).not.toContain("could not update"); + }); + + it("does not disparage a neighbour this run is ALSO updating", async () => { + // Reporting from inside the per-server read said `srv-b ... (not updated)` + // one line before `✓ Updated srv-b`, and repeated it once per server. + const adapter = makeAdapter("claude-desktop"); + (adapter.read as ReturnType).mockImplementation( + readDropping( + { "srv-b": { command: "npx", args: "BAD" } }, + { "srv-a": { command: "npx", args: ["-y", "a"] } } + ) + ); + const lines: string[] = []; + const deps = makeDeps({ + getInstalledServers: vi.fn().mockResolvedValue([ + makeInstalledServer({ name: "srv-a", version: "1.0.0", clients: ["claude-desktop"] }), + makeInstalledServer({ name: "srv-b", version: "1.0.0", clients: ["claude-desktop"] }), + ]), + getServer: vi.fn().mockImplementation((n: string) => Promise.resolve(makeServerEntry(n, "1.1.0"))), + getAdapter: vi.fn().mockReturnValue(adapter), + output: (t: string) => lines.push(t), + }); + + await handleUpdate({ yes: true }, deps); + const text = lines.join("\n"); + // Assert on the EXACT rendering of an entry in the skipped report — the + // earlier /srv-b.*not updated/ could never match, because the report puts + // "not updated" BEFORE the names, so it passed with the bug live. + expect(text).not.toContain("srv-b (claude-desktop)"); + expect(text).not.toMatch(/malformed entr(y was|ies were) skipped/); + // positive control: srv-b really was processed by this run + expect(text).toMatch(/Updated srv-b/); + }); + + it("reports an unrelated malformed neighbour ONCE, not once per server", async () => { + const adapter = makeAdapter("claude-desktop"); + (adapter.read as ReturnType).mockImplementation( + readDropping( + { "never-installed": { command: "npx", args: "BAD" } }, + { "srv-a": { command: "npx", args: ["-y", "a"] } } + ) + ); + const lines: string[] = []; + const deps = makeDeps({ + getInstalledServers: vi.fn().mockResolvedValue([ + makeInstalledServer({ name: "srv-a", version: "1.0.0", clients: ["claude-desktop"] }), + makeInstalledServer({ name: "srv-c", version: "1.0.0", clients: ["claude-desktop"] }), + ]), + getServer: vi.fn().mockImplementation((n: string) => Promise.resolve(makeServerEntry(n, "1.1.0"))), + getAdapter: vi.fn().mockReturnValue(adapter), + output: (t: string) => lines.push(t), + }); + + await handleUpdate({ yes: true }, deps); + const hits = lines.join("\n").split("never-installed").length - 1; + expect(hits).toBe(1); + }); + + it("keeps a string env key named __proto__ instead of dropping it silently", async () => { + // A plain object literal routes an own `__proto__` key to Object.prototype's + // setter, dropping it — which would break this code's own promise to NAME + // anything it cannot carry. Same class v0.36.0 closed in the pin hash. + const adapter = makeAdapter("claude-desktop"); + (adapter.read as ReturnType).mockImplementation( + readDropping({ + "io.github.test/server-a": { + command: "npx", + args: "bad", + env: JSON.parse('{"__proto__":"secret-value","OK":"keep"}'), + }, + }) + ); + const deps = makeDeps({ + getInstalledServers: vi.fn().mockResolvedValue([ + makeInstalledServer({ name: "io.github.test/server-a", version: "1.0.0", clients: ["claude-desktop"] }), + ]), + getServer: vi.fn().mockResolvedValue(makeServerEntry("io.github.test/server-a", "1.1.0")), + getAdapter: vi.fn().mockReturnValue(adapter), + }); + + await handleUpdate({ yes: true }, deps); + const call = (adapter.addServer as ReturnType).mock.calls[0]; + expect(Object.prototype.hasOwnProperty.call(call[2].env, "__proto__")).toBe(true); + expect(call[2].env.OK).toBe("keep"); + // The real hazard is the prototype being REPLACED by the recovered value. + expect(Object.getPrototypeOf({})).toBe(Object.prototype); + expect(typeof ({} as Record).toString).toBe("function"); + }); + + it("names a non-object env instead of returning in silence", async () => { + const adapter = makeAdapter("claude-desktop"); + (adapter.read as ReturnType).mockImplementation( + readDropping({ "io.github.test/server-a": { command: "npx", args: "bad", env: ["A=1"] } }) + ); + const lines: string[] = []; + const deps = makeDeps({ + getInstalledServers: vi.fn().mockResolvedValue([ + makeInstalledServer({ name: "io.github.test/server-a", version: "1.0.0", clients: ["claude-desktop"] }), + ]), + getServer: vi.fn().mockResolvedValue(makeServerEntry("io.github.test/server-a", "1.1.0")), + getAdapter: vi.fn().mockReturnValue(adapter), + output: (t: string) => lines.push(t), + }); + + await handleUpdate({ yes: true }, deps); + expect(lines.join("\n")).toMatch(/env is not an object/); + // and an array env is never written out as {"0": ...} + const call = (adapter.addServer as ReturnType).mock.calls[0]; + expect(call[2].env?.["0"]).toBeUndefined(); + }); + + it("still reports an unrelated malformed entry under --json (via stderr)", async () => { + // --json has no field for it and stdout must stay parseable, so replacing + // read()'s stderr default emitted it NOWHERE — LESS visible than before + // this change, which is the class this whole PR exists to close. + const errs: string[] = []; + const spy = vi + .spyOn(process.stderr, "write") + .mockImplementation((chunk: string | Uint8Array) => { + errs.push(String(chunk)); + return true; + }); + try { + const adapter = makeAdapter("claude-desktop"); + (adapter.read as ReturnType).mockImplementation( + readDropping( + { "never-installed": { command: "npx", args: "BAD" } }, + { "srv-a": { command: "npx", args: ["-y", "a"] } } + ) + ); + const lines: string[] = []; + const deps = makeDeps({ + getInstalledServers: vi.fn().mockResolvedValue([ + makeInstalledServer({ name: "srv-a", version: "1.0.0", clients: ["claude-desktop"] }), + ]), + getServer: vi.fn().mockResolvedValue(makeServerEntry("srv-a", "1.1.0")), + getAdapter: vi.fn().mockReturnValue(adapter), + output: (t: string) => lines.push(t), + }); + + await handleUpdate({ yes: true, json: true }, deps); + + expect(errs.join("")).toContain("never-installed"); + // stdout must remain parseable JSON + expect(() => JSON.parse(lines.join(""))).not.toThrow(); + } finally { + spy.mockRestore(); + } + }); + + it("names EVERY client holding the same malformed entry, not just one", async () => { + // A name-only key dropped one of two facts, and which client got named + // depended on iteration order. + const adapter = makeAdapter("claude-desktop"); + (adapter.read as ReturnType).mockImplementation( + readDropping( + { "never-installed": { command: "npx", args: "BAD" } }, + { "srv-a": { command: "npx", args: ["-y", "a"] } } + ) + ); + const lines: string[] = []; + const deps = makeDeps({ + getInstalledServers: vi.fn().mockResolvedValue([ + makeInstalledServer({ + name: "srv-a", + version: "1.0.0", + clients: ["claude-desktop", "cursor"], + }), + ]), + getServer: vi.fn().mockResolvedValue(makeServerEntry("srv-a", "1.1.0")), + getAdapter: vi.fn().mockReturnValue(adapter), + output: (t: string) => lines.push(t), + }); + + await handleUpdate({ yes: true }, deps); + const text = lines.join("\n"); + expect(text).toContain("never-installed (claude-desktop)"); + expect(text).toContain("never-installed (cursor)"); + expect(text).toMatch(/2 other malformed entries/); + }); + + it("reports a malformed copy in a client the server is NOT installed in", async () => { + // `update` writes only to a server's own `originalClients`. A malformed + // copy of that name in a DIFFERENT client is therefore never updated — + // but a name-scoped suppression filter hid it anyway, silently, because + // the name succeeded elsewhere. Suppression must be keyed by (client, + // name), the same way the fact is. + const cd = makeAdapter("claude-desktop"); + const cur = makeAdapter("cursor"); + (cd.read as ReturnType).mockImplementation( + readDropping( + { "srv-b": { command: "npx", args: "BAD" } }, // malformed HERE + { "srv-a": { command: "npx", args: ["-y", "a"] } } + ) + ); + (cur.read as ReturnType).mockImplementation( + readDropping({}, { "srv-b": { command: "npx", args: ["-y", "b"] } }) // valid THERE + ); + const lines: string[] = []; + const deps = makeDeps({ + getInstalledServers: vi.fn().mockResolvedValue([ + makeInstalledServer({ name: "srv-a", version: "1.0.0", clients: ["claude-desktop"] }), + makeInstalledServer({ name: "srv-b", version: "1.0.0", clients: ["cursor"] }), + ]), + getServer: vi.fn().mockImplementation((n: string) => Promise.resolve(makeServerEntry(n, "1.1.0"))), + getAdapter: vi.fn((id: ClientId) => (id === "cursor" ? cur : cd)), + output: (t: string) => lines.push(t), + }); + + await handleUpdate({ yes: true }, deps); + + const text = lines.join("\n"); + // srv-b WAS updated — in cursor. The claude-desktop copy was not, and is + // the one that must still be named. + expect(text).toMatch(/Updated srv-b/); + expect(text).toContain("srv-b (claude-desktop)"); + }); + + it("sanitizes config-supplied names and env keys before the terminal", async () => { + // Both the neighbour notice and the dropped-env-key note render values a + // config file controls. Every other new render site in this change has an + // escape test; these two did not. + const adapter = makeAdapter("claude-desktop"); + (adapter.read as ReturnType).mockImplementation( + readDropping({ + "ev\u001b]0;PWNED\u0007il": { command: "npx", args: "BAD" }, + "srv-a": { command: "npx", args: "BAD", env: { "K\u001b[31mEY": 7 } }, + }) + ); + const lines: string[] = []; + const deps = makeDeps({ + getInstalledServers: vi.fn().mockResolvedValue([ + makeInstalledServer({ name: "srv-a", version: "1.0.0", clients: ["claude-desktop"] }), + ]), + getServer: vi.fn().mockResolvedValue(makeServerEntry("srv-a", "1.1.0")), + getAdapter: vi.fn().mockReturnValue(adapter), + output: (t: string) => lines.push(t), + }); + + await handleUpdate({ yes: true }, deps); + + const text = lines.join("\n"); + expect(text).toContain("PWNED"); // the neighbour name is still shown... + expect(text).toContain("KEY"); // ...and so is the dropped env key... + expect(text).not.toContain("\u001b"); // ...but no escape survives + }); + + it("records a written pair only AFTER the write succeeds", async () => { + // Recording before `await addServer` would let a FAILED write suppress the + // report for an entry that is therefore STILL malformed on disk. Needs two + // servers: each is the other's malformed "neighbour" in the same config, + // and both writes fail. + const adapter = makeAdapter("claude-desktop"); + (adapter.read as ReturnType).mockImplementation( + readDropping({ + "srv-a": { command: "npx", args: "BAD" }, + "srv-b": { command: "npx", args: "BAD" }, + }) + ); + (adapter.addServer as ReturnType).mockRejectedValue(new Error("read-only")); + const lines: string[] = []; + const deps = makeDeps({ + getInstalledServers: vi.fn().mockResolvedValue([ + makeInstalledServer({ name: "srv-a", version: "1.0.0", clients: ["claude-desktop"] }), + makeInstalledServer({ name: "srv-b", version: "1.0.0", clients: ["claude-desktop"] }), + ]), + getServer: vi.fn().mockImplementation((n: string) => Promise.resolve(makeServerEntry(n, "1.1.0"))), + getAdapter: vi.fn().mockReturnValue(adapter), + output: (t: string) => lines.push(t), + }); + + await handleUpdate({ yes: true }, deps); + + // Neither write landed, so both entries are still malformed and both must + // still be named. Recording the pair before the await would hide them. + const text = lines.join("\n"); + expect(text).toContain("srv-a (claude-desktop)"); + expect(text).toContain("srv-b (claude-desktop)"); + }); + + it("carries clientNotes into --json", async () => { + const adapter = makeAdapter("claude-desktop"); + (adapter.read as ReturnType).mockImplementation( + readDropping({ + "io.github.test/server-a": { command: "npx", args: "bad", env: { A: "1", N: 2 } }, + }) + ); + const lines: string[] = []; + const deps = makeDeps({ + getInstalledServers: vi.fn().mockResolvedValue([ + makeInstalledServer({ name: "io.github.test/server-a", version: "1.0.0", clients: ["claude-desktop"] }), + ]), + getServer: vi.fn().mockResolvedValue(makeServerEntry("io.github.test/server-a", "1.1.0")), + getAdapter: vi.fn().mockReturnValue(adapter), + output: (t: string) => lines.push(t), + }); + + await handleUpdate({ yes: true, json: true }, deps); + const parsed = JSON.parse(lines.join("")); + expect(parsed[0].clientNotes.join(" ")).toContain("N"); + }); + + it("re-states the warning for an unrelated malformed neighbour", async () => { + // Replacing the default onSkip suppressed its stderr line, so `update` + // went silent about every OTHER broken entry in the same config. + const adapter = makeAdapter("claude-desktop"); + (adapter.read as ReturnType).mockImplementation( + readDropping( + { "unrelated-bad": { command: "npx", args: "bad" } }, + { "io.github.test/server-a": { command: "npx", args: ["-y", "@test/server"] } } + ) + ); + const lines: string[] = []; + const deps = makeDeps({ + getInstalledServers: vi.fn().mockResolvedValue([ + makeInstalledServer({ name: "io.github.test/server-a", version: "1.0.0", clients: ["claude-desktop"] }), + ]), + getServer: vi.fn().mockResolvedValue(makeServerEntry("io.github.test/server-a", "1.1.0")), + getAdapter: vi.fn().mockReturnValue(adapter), + output: (t: string) => lines.push(t), + }); + + await handleUpdate({ yes: true }, deps); + expect(lines.join("\n")).toContain("unrelated-bad"); + }); + + it("ignores a non-string-valued env on the raw entry (narrow parse)", async () => { + const adapter = makeAdapter("claude-desktop"); + (adapter.read as ReturnType).mockImplementation( + readDropping({ + "io.github.test/server-a": { command: "npx", args: "bad", env: { NESTED: { deep: 1 } } }, + }) + ); + const deps = makeDeps({ + getInstalledServers: vi.fn().mockResolvedValue([ + makeInstalledServer({ name: "io.github.test/server-a", version: "1.0.0", clients: ["claude-desktop"] }), + ]), + getServer: vi.fn().mockResolvedValue(makeServerEntry("io.github.test/server-a", "1.1.0")), + getAdapter: vi.fn().mockReturnValue(adapter), + }); + + await handleUpdate({ yes: true }, deps); + const call = (adapter.addServer as ReturnType).mock.calls[0]; + expect(call[2].env?.NESTED).toBeUndefined(); + }); +}); diff --git a/src/__tests__/config/adapters/base-read-validation.test.ts b/src/__tests__/config/adapters/base-read-validation.test.ts index e3d7a42..ab6f97c 100644 --- a/src/__tests__/config/adapters/base-read-validation.test.ts +++ b/src/__tests__/config/adapters/base-read-validation.test.ts @@ -156,3 +156,47 @@ describe("BaseAdapter.setServerDisabled() — rejects a non-object raw entry (#2 ); }); }); + +/** + * #59 — `onSkip` also receives the RAW entry. That second argument is the + * entire mechanism `mcpm update`'s env recovery depends on: with it neutered, + * update silently wipes a malformed entry's env block again, which is the + * original bug. Every update test supplies `raw` itself from a mocked + * adapter, so nothing pinned that the REAL read() passes it. + */ +describe("BaseAdapter.read() — onSkip receives the raw entry (#59)", () => { + const adapter = new ClaudeDesktopAdapter(); + + beforeEach(() => { + vi.resetAllMocks(); + mockLstat.mockResolvedValue({ isSymbolicLink: () => false }); + vi.spyOn(process.stderr, "write").mockImplementation(() => true); + }); + + it("passes the unvalidated entry alongside the name", async () => { + mockReadFile.mockResolvedValue( + JSON.stringify({ + mcpServers: { + bad: { command: "npx", args: "not-an-array", env: { K: "v" } }, + good: { command: "npx", args: ["-y", "ok"] }, + }, + }) + ); + + const seen: Array<[string, unknown]> = []; + const out = await adapter.read(CONFIG_PATH, (name, raw) => seen.push([name, raw])); + + expect(Object.keys(out)).toEqual(["good"]); + expect(seen).toHaveLength(1); + expect(seen[0]![0]).toBe("bad"); + // The raw entry must arrive INTACT — this is what update reads env from. + expect(seen[0]![1]).toEqual({ command: "npx", args: "not-an-array", env: { K: "v" } }); + }); + + it("passes a non-object raw entry through as-is", async () => { + mockReadFile.mockResolvedValue(JSON.stringify({ mcpServers: { bad: "a bare string" } })); + const seen: unknown[] = []; + await adapter.read(CONFIG_PATH, (_n, raw) => seen.push(raw)); + expect(seen).toEqual(["a bare string"]); + }); +}); diff --git a/src/__tests__/config/drift.test.ts b/src/__tests__/config/drift.test.ts index c3c24d1..11d7de8 100644 --- a/src/__tests__/config/drift.test.ts +++ b/src/__tests__/config/drift.test.ts @@ -1,7 +1,7 @@ import { describe, it, expect, vi } from "vitest"; import { buildDriftModel, - collectClientStates, + collectClientStatesWithErrors, type ClientState, type DriftDeps, } from "../../config/drift.js"; @@ -136,7 +136,7 @@ describe("buildDriftModel", () => { }); // --------------------------------------------------------------------------- -// collectClientStates — I/O, injected +// collectClientStatesWithErrors — I/O, injected // --------------------------------------------------------------------------- function makeDeps(overrides: Partial = {}): DriftDeps { @@ -148,14 +148,14 @@ function makeDeps(overrides: Partial = {}): DriftDeps { }; } -describe("collectClientStates", () => { +describe("collectClientStatesWithErrors", () => { it("returns one state per readable client", async () => { const deps = makeDeps({ getAdapter: vi.fn((id: ClientId) => ({ read: vi.fn().mockResolvedValue(id === "cursor" ? { fs: { command: "npx" } } : {}), })), }); - const states = await collectClientStates(deps); + const states = (await collectClientStatesWithErrors(deps)).states; expect(states.map((s) => s.clientId)).toEqual(["claude-desktop", "cursor"]); expect(states[1]!.servers).toEqual({ fs: { command: "npx" } }); }); @@ -169,7 +169,97 @@ describe("collectClientStates", () => { : vi.fn().mockResolvedValue({ ok: { command: "npx" } }), })), }); - const states = await collectClientStates(deps); + const states = (await collectClientStatesWithErrors(deps)).states; expect(states.map((s) => s.clientId)).toEqual(["claude-desktop"]); }); }); + +// --------------------------------------------------------------------------- +// #59: an entry dropped by read()'s shape validation (#23, v0.34.0) used to be +// indistinguishable from an entry that was never there — so `sync --check` +// reported a client that HAS the server as "missing" it (a false drift claim +// on a CI gate), or, when only one client had it, dropped the server from the +// model entirely and counted the run clean. +// --------------------------------------------------------------------------- + +describe("buildDriftModel — malformed entries are not reported as absent", () => { + // NOTE ON FIXTURE SHAPE: `read()` puts a name in the returned map OR passes + // it to onSkip, never both (base.ts, if/else). A state with the same name in + // `servers` AND `malformed` is therefore unreachable — and a test built on + // one asserts `absent === []` that is already true via `present`, pinning + // nothing. Every fixture below uses the reachable shape. + it("reports a malformed entry as malformed, NOT missing, in that client", () => { + const model = buildDriftModel([ + { clientId: "claude-desktop", servers: {}, malformed: ["fs"] }, + state("cursor", { fs: { command: "npx", args: ["fs"] } }), + ]); + const fs = findServer(model, "fs"); + // claude-desktop HAS this server — saying it is "missing in claude-desktop" + // is a false statement that sends the user to re-add a server they have. + expect(fs.absent).toEqual([]); + expect(fs.malformed).toEqual(["claude-desktop"]); + expect(fs.present).toEqual(["cursor"]); + }); + + it("counts a server as drifted when its ONLY client's entry is unreadable", () => { + // Single client: `absent` is necessarily empty, so this is the only shape + // in which the malformed clause of the drifted predicate decides anything. + const model = buildDriftModel([{ clientId: "claude-desktop", servers: {}, malformed: ["fs"] }]); + expect(model.drifted).toBe(1); + expect(model.inSync).toBe(0); + }); + + it("counts a server as drifted when EVERY client's copy is unreadable", () => { + const model = buildDriftModel([ + { clientId: "claude-desktop", servers: {}, malformed: ["fs"] }, + { clientId: "cursor", servers: {}, malformed: ["fs"] }, + ]); + const fs = findServer(model, "fs"); + expect(fs.absent).toEqual([]); // present in both, readable in neither + expect(model.drifted).toBe(1); + expect(model.inSync).toBe(0); + }); + + it("surfaces a server that is malformed in the ONLY client holding it", () => { + // Previously this vanished from the model: drifted 0, inSync 0, exit 0 — + // a clean CI pass over a config mcpm could not actually read. + const model = buildDriftModel([ + { clientId: "claude-desktop", servers: {}, malformed: ["fs"] }, + state("cursor", {}), + ]); + const fs = findServer(model, "fs"); + expect(fs.malformed).toEqual(["claude-desktop"]); + expect(fs.present).toEqual([]); + expect(model.inSync).toBe(0); + expect(model.drifted).toBe(1); + }); + + it("does not count a fully-verified stack as drifted (negative control)", () => { + const entry: McpServerEntry = { command: "npx", args: ["fs"] }; + const model = buildDriftModel([ + state("claude-desktop", { fs: entry }), + state("cursor", { fs: { ...entry } }), + ]); + expect(findServer(model, "fs").malformed).toEqual([]); + expect(model.drifted).toBe(0); + }); +}); + +describe("collectClientStatesWithErrors — records malformed entry names", () => { + it("captures the names read() dropped, per client", async () => { + const deps = makeDeps({ + getAdapter: vi.fn((id: ClientId) => ({ + read: vi.fn().mockImplementation(async (_p: string, onSkip?: (n: string) => void) => { + if (id === "cursor") { + onSkip?.("broken"); + return { ok: { command: "npx" } }; + } + return { ok: { command: "npx" } }; + }), + })), + }); + const states = (await collectClientStatesWithErrors(deps)).states; + expect(states.find((s) => s.clientId === "cursor")!.malformed).toEqual(["broken"]); + expect(states.find((s) => s.clientId === "claude-desktop")!.malformed).toEqual([]); + }); +}); diff --git a/src/commands/diff.ts b/src/commands/diff.ts index eb9e722..5b8d750 100644 --- a/src/commands/diff.ts +++ b/src/commands/diff.ts @@ -21,6 +21,7 @@ import type { LockedServer, } from "../stack/schema.js"; import { lockPathFor } from "../stack/paths.js"; +import { sanitizeForTerminal } from "../guard/sanitize.js"; import { parseStackFile, parseLockFile, @@ -43,7 +44,11 @@ export interface DiffDeps { output: (text: string) => void; } -export type DiffStatus = "missing" | "extra" | "match" | "mismatch"; +// #59: "unreadable" is distinct from "missing" on purpose. A declared server +// whose client entry failed read()'s shape validation IS installed — reporting +// it missing sends the user to `mcpm up` to re-install over an entry that is +// merely mis-shaped, and hides the thing they actually need to fix. +export type DiffStatus = "missing" | "extra" | "match" | "mismatch" | "unreadable"; export interface DiffEntry { readonly name: string; @@ -73,12 +78,15 @@ export async function handleDiff( // Collect installed servers across all clients const clients = await deps.detectClients(); const installed = new Map(); + const unreadable = new Map(); for (const clientId of clients) { try { const adapter = deps.getAdapter(clientId); const configPath = deps.getPath(clientId); - const servers = await adapter.read(configPath); + const servers = await adapter.read(configPath, (name) => { + unreadable.set(name, [...(unreadable.get(name) ?? []), clientId]); + }); for (const [name, entry] of Object.entries(servers)) { const existing = installed.get(name); @@ -103,12 +111,22 @@ export async function handleDiff( const inst = installed.get(name); if (!inst) { - entries.push({ - name, - status: "missing", - detail: locked ? formatLocked(locked) : "not locked", - clients: [], - }); + const badClients = unreadable.get(name); + entries.push( + badClients + ? { + name, + status: "unreadable", + detail: "entry does not match the expected shape - cannot compare", + clients: badClients, + } + : { + name, + status: "missing", + detail: locked ? formatLocked(locked) : "not locked", + clients: [], + } + ); } else if (locked && isLockedRegistryServer(locked)) { const installedVersion = extractInstalledVersion(inst.entry, locked.identifier); if (installedVersion !== null && installedVersion !== locked.version) { @@ -137,6 +155,26 @@ export async function handleDiff( } } + // #59: every unreadable entry gets a row, EXCEPT one already emitted as the + // declared branch's "unreadable" verdict above (that name has no readable + // copy anywhere). Gating this on `installed.has(name)` was a regression: + // when a name is malformed in client A and VALID in client B, neither loop + // fired AND the default stderr warning was gone, so `diff` printed + // "1 in sync" with nothing on either stream about the config it could not + // read — worse than the behaviour it replaced. + const alreadyReported = new Set( + entries.filter((e) => e.status === "unreadable").map((e) => e.name) + ); + for (const [name, badClients] of unreadable) { + if (alreadyReported.has(name)) continue; + entries.push({ + name, + status: "unreadable", + detail: "entry does not match the expected shape - cannot compare", + clients: badClients, + }); + } + // Check for extra servers (installed but not in yaml) for (const [name, inst] of installed) { if (!declaredNames.has(name)) { @@ -161,6 +199,7 @@ export async function handleDiff( } const missing = entries.filter((e) => e.status === "missing"); + const badShape = entries.filter((e) => e.status === "unreadable"); const extra = entries.filter((e) => e.status === "extra"); const mismatched = entries.filter((e) => e.status === "mismatch"); const matched = entries.filter((e) => e.status === "match"); @@ -173,6 +212,16 @@ export async function handleDiff( deps.output(""); } + if (badShape.length > 0) { + deps.output("Unreadable (installed but the entry does not match the expected shape):"); + for (const e of badShape) { + deps.output( + ` ? ${sanitizeForTerminal(e.name)} [${e.clients.join(", ")}] - fix the entry, then re-run` + ); + } + deps.output(""); + } + if (extra.length > 0) { deps.output("Extra (installed but not in mcpm.yaml):"); for (const e of extra) { @@ -198,7 +247,8 @@ export async function handleDiff( } deps.output( - `${matched.length} in sync, ${mismatched.length} mismatched, ${missing.length} missing, ${extra.length} extra` + `${matched.length} in sync, ${mismatched.length} mismatched, ${missing.length} missing, ${extra.length} extra` + + (badShape.length > 0 ? `, ${badShape.length} unreadable` : "") ); } diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index 61d0c05..753cf84 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -66,11 +66,17 @@ export interface DoctorRuntimeHealth { export interface DoctorDriftEntry { name: string; - kind: "conflict" | "absent"; + kind: "conflict" | "absent" | "unreadable"; present: string[]; absent: string[]; /** Present only for `kind: "conflict"`. */ fields?: string[]; + /** + * #59: clients holding this server in an entry read() could not validate. + * Kept out of both `present` and `absent` — the client is not missing the + * server, and the entry cannot be compared. + */ + malformed?: string[]; } export interface DoctorCrossClient { @@ -218,8 +224,20 @@ export async function buildDoctorModel(deps: DoctorModelDeps): Promise - read.servers ? [{ clientId, servers: read.servers }] : [] + read.servers + ? [ + { + clientId, + servers: read.servers, + malformed: skippedEntries.filter((e) => e.clientId === clientId).map((e) => e.name), + }, + ] + : [] ); const crossClient = driftStates.length >= 2 ? toCrossClient(driftStates) : null; @@ -251,13 +269,20 @@ function toCrossClient(states: ClientState[]): DoctorCrossClient { present: [...server.present], absent: [...server.absent], fields: server.conflictFields ? [...server.conflictFields] : undefined, + // #59: symmetric with the absent branch below — a third client holding + // an unreadable copy is part of the same picture. + ...(server.malformed.length > 0 ? { malformed: [...server.malformed] } : {}), }); - } else if (server.absent.length > 0) { + } else if (server.absent.length > 0 || server.malformed.length > 0) { + // #59: a server whose ONLY holder has an unreadable entry has an empty + // `present`, which rendered as "in ; missing in cursor" — a broken line + // that also never mentioned the client actually holding it. entries.push({ name: server.name, - kind: "absent", + kind: server.present.length === 0 && server.malformed.length > 0 ? "unreadable" : "absent", present: [...server.present], absent: [...server.absent], + ...(server.malformed.length > 0 ? { malformed: [...server.malformed] } : {}), }); } } @@ -309,9 +334,27 @@ export function renderDoctorText(model: DoctorModel, output: (text: string) => v } else { for (const d of cc.drift) { if (d.kind === "conflict") { - output(` ⚠ ${d.name} — config differs (${d.fields!.join(", ")}) across ${d.present.join(", ")}`); + output( + ` ⚠ ${sanitizeForTerminal(d.name)} — config differs (${d.fields!.join(", ")}) ` + + `across ${d.present.join(", ")}` + + (d.malformed && d.malformed.length > 0 + ? `; unreadable in ${d.malformed.join(", ")}` + : "") + ); + } else if (d.kind === "unreadable") { + output( + ` ⚠ ${sanitizeForTerminal(d.name)} — entry in ${d.malformed!.join(", ")} ` + + `does not match the expected shape` + + (d.absent.length > 0 ? `; missing in ${d.absent.join(", ")}` : "") + ); } else { - output(` ⚠ ${d.name} — in ${d.present.join(", ")}; missing in ${d.absent.join(", ")}`); + output( + ` ⚠ ${sanitizeForTerminal(d.name)} — in ${d.present.join(", ")}` + + (d.malformed && d.malformed.length > 0 + ? `; unreadable in ${d.malformed.join(", ")}` + : "") + + (d.absent.length > 0 ? `; missing in ${d.absent.join(", ")}` : "") + ); } } output(" Run `mcpm sync --check` for the full matrix (advisory, not a failure)."); diff --git a/src/commands/export.ts b/src/commands/export.ts index 5c58bcd..62e2330 100644 --- a/src/commands/export.ts +++ b/src/commands/export.ts @@ -15,6 +15,7 @@ import type { ClientId } from "../config/paths.js"; import type { ConfigAdapter, McpServerEntry } from "../config/adapters/index.js"; import type { StackFile, StackEnvVar } from "../stack/schema.js"; import { serializeYaml } from "../stack/schema.js"; +import { sanitizeForTerminal } from "../guard/sanitize.js"; import { DEFAULT_MIN_RELEASE_AGE_HOURS } from "../scanner/cooldown.js"; // --------------------------------------------------------------------------- @@ -69,12 +70,19 @@ export async function handleExport( const clients = await detectClients(); const seen = new Set(); const servers: Record = {}; + // #59: an entry read() dropped for failing shape validation is silently + // ABSENT from the export — and the user keeps the result as their declared + // stack. Name them on stderr so the file is never mistaken for complete + // (stderr, not `output`: with no --output the YAML itself goes to stdout). + const unreadable: string[] = []; for (const clientId of clients) { try { const adapter = getAdapter(clientId); const configPath = getPath(clientId); - const installed = await adapter.read(configPath); + const installed = await adapter.read(configPath, (name) => { + if (!unreadable.includes(name)) unreadable.push(name); + }); for (const [name, entry] of Object.entries(installed)) { if (seen.has(name)) continue; @@ -86,6 +94,24 @@ export async function handleExport( } } + // A name is only ABSENT from the export if no client contributed a readable + // entry for it — another client holding a well-formed copy makes the export + // complete, and saying otherwise is its own false statement (caught by + // dogfooding: cursor's good copy of a claude-desktop-malformed server). + // `name in servers` walks the PROTOTYPE CHAIN, so an entry named `toString` + // / `constructor` / `valueOf` read as already-exported and vanished from both + // the warning and the file. Server names are arbitrary JSON keys. `seen` is + // the Set of names actually contributed, and has no such members. + const omitted = unreadable.filter((name) => !seen.has(name)); + if (omitted.length > 0) { + process.stderr.write( + `mcpm: ${omitted.length} server ${omitted.length === 1 ? "entry" : "entries"} ` + + `could not be read and ${omitted.length === 1 ? "is" : "are"} NOT in this export: ` + + `${omitted.map((n) => sanitizeForTerminal(n)).join(", ")}. ` + + `Run \`mcpm doctor\` for details.\n` + ); + } + const stackFile = buildStackFile(servers); const yaml = serializeYaml(stackFile); diff --git a/src/commands/import.ts b/src/commands/import.ts index 55fcdba..d84db5a 100644 --- a/src/commands/import.ts +++ b/src/commands/import.ts @@ -23,6 +23,7 @@ import type { Finding } from "../scanner/tier1.js"; import type { TrustScore, TrustScoreInput } from "../scanner/trust-score.js"; import { formatMcpEntryCommand } from "../utils/format-entry.js"; import { stdoutOutput } from "../utils/output.js"; +import { sanitizeForTerminal } from "../guard/sanitize.js"; // --------------------------------------------------------------------------- // Types @@ -67,12 +68,18 @@ interface DiscoveredServer { */ async function readClientServers( clientId: ClientId, - deps: ImportDeps + deps: ImportDeps, + // #59: names read() dropped for failing shape validation. Import is the + // first-run "bring my existing setup in" path, so an entry that silently + // never appears in the pick-list is the worst place to stay quiet. + unreadable?: string[] ): Promise { try { const adapter = deps.getAdapter(clientId); const configPath = deps.getConfigPath(clientId); - const servers = await adapter.read(configPath); + const servers = await adapter.read(configPath, (name) => { + if (unreadable && !unreadable.includes(name)) unreadable.push(name); + }); return Object.entries(servers).map(([name, entry]) => ({ name, clientId, @@ -218,16 +225,43 @@ export async function handleImport( } // Read servers from each client — in parallel + const unreadable: string[] = []; const perClientResults = await Promise.all( - clientsToScan.map((clientId) => readClientServers(clientId, deps)) + clientsToScan.map((clientId) => readClientServers(clientId, deps, unreadable)) ); const discovered: DiscoveredServer[] = perClientResults.flat(); // De-duplicate by server name const uniqueServers = deduplicateServers(discovered); + // #59: report BEFORE the empty-result return below — "No existing MCP servers + // found" is a false statement when the only entries present were unreadable. + // Only names NO client could supply are actually un-importable: a well-formed + // copy in another client makes the server importable regardless. + const importable = new Set(uniqueServers.map((s) => s.name)); + const omitted = unreadable.filter((n) => !importable.has(n)); + if (omitted.length > 0) { + output( + chalk.yellow( + `${omitted.length} server ${omitted.length === 1 ? "entry does" : "entries do"} ` + + `not match the expected shape and cannot be imported: ` + + `${omitted.map((n) => sanitizeForTerminal(n)).join(", ")}. ` + + `Run \`mcpm doctor\` for details.` + ) + ); + } + if (uniqueServers.length === 0) { - output(chalk.yellow("No existing MCP servers found in any client config.")); + // #59: "No existing MCP servers found" contradicts the warning above when + // unreadable entries were the only ones present — servers WERE found, mcpm + // just could not read them. + output( + chalk.yellow( + omitted.length > 0 + ? "No importable MCP servers found — every entry present failed shape validation." + : "No existing MCP servers found in any client config." + ) + ); return; } diff --git a/src/commands/list.ts b/src/commands/list.ts index 6932bc3..0a3a8f0 100644 --- a/src/commands/list.ts +++ b/src/commands/list.ts @@ -13,6 +13,7 @@ import { Command } from "commander"; import chalk from "chalk"; +import { sanitizeForTerminal } from "../guard/sanitize.js"; import Table from "cli-table3"; import type { ClientId } from "../config/paths.js"; import type { ConfigAdapter, McpServerEntry } from "../config/adapters/index.js"; @@ -50,6 +51,16 @@ interface ServerRow { * All dependencies are injected so the function is hermetically testable. * This command is strictly READ-ONLY — it never writes to any config file. */ +/** The one skip sentence. Two sinks (stderr under --json, stdout otherwise). */ +function skipNotice(malformed: ReadonlyArray<{ name: string; client: ClientId }>): string { + return ( + `${malformed.length} entr${malformed.length === 1 ? "y" : "ies"} could not be read ` + + `and ${malformed.length === 1 ? "is" : "are"} not listed: ` + + `${malformed.map((m) => `${sanitizeForTerminal(m.name)} (${m.client})`).join(", ")}. ` + + `Run \`mcpm doctor\` for details.` + ); +} + export async function handleList( options: ListOptions, deps: ListDeps @@ -65,12 +76,19 @@ export async function handleList( // Collect rows from all applicable clients. const rows: ServerRow[] = []; + // #59: entries read() dropped for failing shape validation. `list` is the + // inventory command, and --json has no stderr channel a consumer reads. + const malformed: Array<{ name: string; client: ClientId }> = []; for (const clientId of clients) { try { const adapter = getAdapter(clientId); const configPath = getPath(clientId); - const servers = await adapter.read(configPath); + const servers = await adapter.read(configPath, (name) => { + if (!malformed.some((m) => m.name === name && m.client === clientId)) { + malformed.push({ name, client: clientId }); + } + }); for (const [serverName, entry] of Object.entries(servers)) { rows.push({ client: clientId, serverName, entry: { ...entry } }); @@ -87,13 +105,25 @@ export async function handleList( serverName, entry, })); + // #59: the skip notice goes to STDERR, not into the payload. Flipping the + // shape from a bare array to an object only in the malformed case would + // break `JSON.parse(out).map(...)` exactly when things are already wrong. + // (The MCP `mcpm_list` tool puts `skipped` in its result because that + // surface has no stderr an agent can see; the CLI does.) + if (malformed.length > 0) process.stderr.write(`mcpm: ${skipNotice(malformed)}\n`); output(JSON.stringify(jsonData, null, 2)); return; } + if (malformed.length > 0) output(chalk.yellow(skipNotice(malformed))); + // No servers found. if (rows.length === 0) { - output("No MCP servers installed. Try: mcpm search "); + output( + malformed.length > 0 + ? "No readable MCP servers installed." + : "No MCP servers installed. Try: mcpm search " + ); return; } diff --git a/src/commands/sync.ts b/src/commands/sync.ts index e340907..aac85f3 100644 --- a/src/commands/sync.ts +++ b/src/commands/sync.ts @@ -20,9 +20,10 @@ import Table from "cli-table3"; import type { ClientId } from "../config/paths.js"; +import { sanitizeForTerminal } from "../guard/sanitize.js"; import { buildDriftModel, - collectClientStates, + collectClientStatesWithErrors, type DriftDeps, type DriftModel, type ServerDrift, @@ -42,7 +43,11 @@ export interface SyncDeps extends DriftDeps { export interface SyncResult { readonly model: DriftModel; - /** True iff at least one server is absent somewhere or has a shape conflict. */ + /** + * True iff at least one server is absent somewhere, has a shape conflict, has + * an entry that failed shape validation, or a client's config could not be + * read at all — i.e. anything mcpm could not verify to be in sync. + */ readonly drift: boolean; } @@ -51,9 +56,11 @@ export interface SyncResult { // --------------------------------------------------------------------------- export async function handleSync(options: SyncOptions, deps: SyncDeps): Promise { - const states = await collectClientStates(deps); - const model = buildDriftModel(states); - const drift = model.drifted > 0; + const { states, unreadableClients } = await collectClientStatesWithErrors(deps); + const model = { ...buildDriftModel(states), unreadableClients }; + // An entire unparseable config is a bigger coverage gap than one mis-typed + // entry inside it; without this the larger failure was the quieter one. + const drift = model.drifted > 0 || unreadableClients.length > 0; if (options.json) { deps.output(JSON.stringify(model, null, 2)); @@ -79,14 +86,56 @@ export function exitCodeFor(result: SyncResult, check: boolean | undefined): num // --------------------------------------------------------------------------- function cell(server: ServerDrift, clientId: ClientId): string { + // #59: check malformed BEFORE absent — the client holds this server, mcpm + // just could not read the entry. Rendering it as "·" claimed it was missing. + if (server.malformed.includes(clientId)) return "?"; if (!server.present.includes(clientId)) return "·"; // absent if (server.conflict) return "≠"; // present but the clients disagree on shape return "✓"; } +/** Unreadable entries and unreadable client configs — never gated on the matrix. */ +function renderUnreadable(model: DriftModel, output: (text: string) => void): void { + for (const s of model.servers.filter((x) => x.malformed.length > 0)) { + output( + ` ? ${sanitizeForTerminal(s.name)}: entry in ${s.malformed.join(", ")} does not match ` + + `the expected shape — cannot compare` + + // A malformed-only name has an EMPTY `present`, which the missing + // section rendered as "in ; missing in cursor". Say it once, here. + (s.present.length === 0 && s.absent.length > 0 + ? `; also missing in ${s.absent.join(", ")}` + : "") + + ` (run \`mcpm doctor\` for details)` + ); + } + for (const c of model.unreadableClients ?? []) { + output(` ? ${c}: config could not be read at all — not compared`); + } + if ( + model.servers.some((x) => x.malformed.length > 0) || + (model.unreadableClients ?? []).length > 0 + ) { + output(""); + } +} + function renderDashboard(model: DriftModel, output: (text: string) => void): void { + // #59/H1: these must print BEFORE the early returns below. `drifted` is + // computed from the model, so `--check` exited 2 while the only line printed + // was "nothing to compare across clients" — a CI failure whose own output + // said there was nothing to look at, with the entry named on neither stream. + // Single-client is the common desktop shape, not a corner case. + renderUnreadable(model, output); + if (model.clients.length === 0) { - output("No client configs found. Install a server first (e.g. `mcpm install `)."); + // #59: "No client configs found" is false when configs WERE found and could + // not be parsed, and "install a server" is the wrong remediation for broken + // JSON. Same contradiction this PR fixed in import.ts and list.ts. + output( + (model.unreadableClients ?? []).length > 0 + ? "No READABLE client configs found — fix the configs named above, then re-run." + : "No client configs found. Install a server first (e.g. `mcpm install `)." + ); return; } if (model.clients.length === 1) { @@ -102,24 +151,42 @@ function renderDashboard(model: DriftModel, output: (text: string) => void): voi const table = new Table({ head: ["server", ...model.clients], style: { head: [], border: [] } }); for (const server of model.servers) { - table.push([server.name, ...model.clients.map((c) => cell(server, c))]); + table.push([sanitizeForTerminal(server.name), ...model.clients.map((c) => cell(server, c))]); } output(table.toString()); - output(" legend: ✓ present · absent ≠ shape conflict"); + output(" legend: ✓ present · absent ≠ shape conflict ? unreadable entry"); const conflicts = model.servers.filter((s) => s.conflict); if (conflicts.length > 0) { output(""); for (const s of conflicts) { - output(` ≠ ${s.name}: differs on ${s.conflictFields!.join(", ")} (across ${s.present.join(", ")})`); + output( + ` ≠ ${sanitizeForTerminal(s.name)}: differs on ${s.conflictFields!.join(", ")} ` + + `(across ${s.present.join(", ")})` + ); } } - const missing = model.servers.filter((s) => s.absent.length > 0); + // Exclude names already fully described above, or they are reported twice and + // counted under "missing in ≥1 client" as well. + // Listed separately from the count below: a malformed-only name is fully + // described in the unreadable section (with "also missing in …"), so listing + // it again here would report one server twice. + const missing = model.servers.filter( + (s) => s.absent.length > 0 && !(s.present.length === 0 && s.malformed.length > 0) + ); + // ...but the SUMMARY sub-count must stay true to its own label. + const missingCount = model.servers.filter((s) => s.absent.length > 0).length; + const unreadableServers = model.servers.filter((s) => s.malformed.length > 0).length; + const unreadableClientCount = (model.unreadableClients ?? []).length; if (missing.length > 0) { output(""); for (const s of missing) { - output(` · ${s.name}: in ${s.present.join(", ")}; missing in ${s.absent.join(", ")}`); + output( + ` · ${sanitizeForTerminal(s.name)}: in ${s.present.join(", ")}` + + (s.malformed.length > 0 ? `; unreadable in ${s.malformed.join(", ")}` : "") + + `; missing in ${s.absent.join(", ")}` + ); } } @@ -128,7 +195,18 @@ function renderDashboard(model: DriftModel, output: (text: string) => void): voi // `inSync + drifted` partitions the servers; the parenthetical sub-counts can // overlap (a server can be both missing-somewhere and conflicting-elsewhere). output( - `${model.inSync} in sync · ${model.drifted} drifted (${missing.length} missing in ≥1 client, ${conflicts.length} shape ${conflictWord})`, + `${model.inSync} in sync · ${model.drifted} drifted (${missingCount} missing in ≥1 client, ` + + `${conflicts.length} shape ${conflictWord}` + + // #59: without this the line reads "1 drifted (0 missing, 0 conflicts)" + // — a drift count with no stated cause. + // Reported separately: the parenthetical's other terms are SERVER counts, + // so folding an unreadable-CLIENT count into them makes the number mean + // two different things at once. + (unreadableServers > 0 ? `, ${unreadableServers} unreadable` : "") + + `)` + + (unreadableClientCount > 0 + ? ` · ${unreadableClientCount} client config(s) unreadable` + : ""), ); } diff --git a/src/commands/up.ts b/src/commands/up.ts index 15642a8..4f84920 100644 --- a/src/commands/up.ts +++ b/src/commands/up.ts @@ -20,6 +20,7 @@ */ import type { ClientId } from "../config/paths.js"; +import { sanitizeForTerminal } from "../guard/sanitize.js"; import type { ConfigAdapter, McpServerEntry } from "../config/adapters/index.js"; import type { ServerEntry } from "../registry/types.js"; import type { Finding } from "../scanner/tier1.js"; @@ -1160,16 +1161,30 @@ async function handleStrictRemoval( for (const clientId of clients) { const adapter = deps.getAdapter(clientId); const configPath = deps.getPath(clientId); - // #23 follow-up (adversarial review, not wired): an entry that fails - // read()'s shape validation is invisible here too, so a malformed, - // undeclared entry survives --strict with only a stderr line — the - // "installed state now matches mcpm.yaml" claim isn't fully honored. - // Deliberately left as the DEFAULT (stderr-only) onSkip rather than fully - // wired: --strict's job is DELETION, and the fail-safe direction (never - // delete something read() couldn't validate) is the one this project - // has consistently chosen elsewhere (v0.29.0's Math.min row) — worth a - // dedicated pass, not a rushed change to a destructive path. - const installed = await adapter.read(configPath); + // #59 (was #23's deferred sub-gap): an entry that fails read()'s shape + // validation is invisible here, so a malformed UNDECLARED entry survives + // --strict. The fail-safe direction is kept — mcpm still does NOT delete an + // entry it could not read, the same choice as v0.29.0's Math.min row — but + // it no longer claims the reconciliation succeeded. Silence was the bug, + // not the refusal to delete. + const unreadable: string[] = []; + const installed = await adapter.read(configPath, (name) => unreadable.push(name)); + + for (const name of unreadable) { + if (declaredNames.has(name)) continue; // declared: not --strict's business + results.push({ + name, + status: "skipped", + message: + `in ${clientId} but not in mcpm.yaml, and its entry does not match the ` + + `expected shape — NOT removed (fix the entry, then re-run)`, + }); + deps.recordResult?.({ name, status: "skipped" }); + deps.output( + ` • ${sanitizeForTerminal(name)}: not removed from ${clientId} — ` + + `entry does not match the expected shape` + ); + } for (const name of Object.keys(installed)) { if (declaredNames.has(name)) continue; diff --git a/src/commands/update.ts b/src/commands/update.ts index fe28884..6b37bdb 100644 --- a/src/commands/update.ts +++ b/src/commands/update.ts @@ -25,6 +25,7 @@ import type { ConfigAdapter, McpServerEntry } from "../config/adapters/index.js" import { levelColor, levelLabel, extractRegistryMeta } from "../utils/format-trust.js"; import { resolveInstallEntry } from "./install.js"; import { stdoutOutput } from "../utils/output.js"; +import { sanitizeForTerminal } from "../guard/sanitize.js"; // --------------------------------------------------------------------------- // Types @@ -70,13 +71,67 @@ async function readExistingEnv( getAdapter: UpdateDeps["getAdapter"], getConfigPath: UpdateDeps["getConfigPath"], clientId: ClientId, - name: string + name: string, + onNote: (message: string) => void, + onNeighbour: (clientId: ClientId, skipped: string) => void ): Promise | undefined> { try { const adapter = getAdapter(clientId); const configPath = getConfigPath(clientId); - const servers = await adapter.read(configPath); - return servers[name]?.env; + // #59: since #23 (v0.34.0) an entry failing shape validation is omitted + // from the returned map, so a plain `servers[name]?.env` returned undefined + // for it — and the caller's `force: true` re-write then DISCARDED a + // perfectly good env block (API keys) held by an entry malformed in some + // OTHER field, while printing "✓ Updated". + // + // The fix is to recover the env, NOT to refuse the write. Overwriting a + // mis-shaped entry with a freshly resolved one is the user's self-repair + // path; refusing it turns a self-healing case into a permanently stuck one. + // + // Recovery is PER KEY, not all-or-nothing. `env` is frequently the field + // that makes the entry invalid in the first place — a numeric port is the + // archetypal hand-edit — and parsing the whole record then rejects every + // key, destroying the API key beside the bad one. Only string-valued keys + // can be carried into a valid entry; any key that cannot is NAMED rather + // than dropped in silence. + let recovered: Record | undefined; + const servers = await adapter.read(configPath, (skipped, raw) => { + if (skipped !== name) { + // Another malformed entry in the same config. Replacing the default + // onSkip suppressed its warning, so it is collected — but reported ONCE + // at the end of the run, and only for names this run did not itself + // update. Reporting here said `srv-b … (not updated)` one line before + // `✓ Updated srv-b`, and repeated it once per updated server. + onNeighbour(clientId, skipped); + return; + } + const env = (raw as { env?: unknown } | null | undefined)?.env; + if (env !== undefined && (env === null || typeof env !== "object" || Array.isArray(env))) { + onNote(`${clientId}: env is not an object — nothing could be carried over`); + return; + } + if (env === undefined) return; + // Object.create(null): a plain literal routes an own `__proto__` key to + // Object.prototype's setter, which silently drops a string value — the + // same class v0.36.0 closed in the guard's pin hash, and it would break + // this block's own promise to NAME anything it cannot carry. + const kept = Object.create(null) as Record; + const dropped: string[] = []; + for (const [k, v] of Object.entries(env as Record)) { + if (typeof v === "string") kept[k] = v; + else dropped.push(k); + } + if (dropped.length > 0) { + onNote( + `${clientId}: env ${dropped.length === 1 ? "key" : "keys"} ` + + `${dropped.map((k) => `"${sanitizeForTerminal(k)}"`).join(", ")} ` + + `${dropped.length === 1 ? "is" : "are"} not a string and could not be carried over ` + + `— re-set ${dropped.length === 1 ? "it" : "them"} with the value quoted` + ); + } + if (Object.keys(kept).length > 0) recovered = kept; + }); + return servers[name]?.env ?? recovered; } catch { return undefined; } @@ -224,10 +279,23 @@ export async function handleUpdate( // Track update outcomes immutably (name → { updated, trustScore, clientErrors }) const updateOutcomes = new Map< string, - { updated: boolean; trustScore: TrustScore; clientErrors: string[] } + { updated: boolean; trustScore: TrustScore; clientErrors: string[]; clientNotes: string[] } >(); - // Perform updates + // Perform updates. + // + // #59: malformed entries seen in passing while reading configs, collected + // across the whole run and reported ONCE below rather than per updated + // server. Keyed by (client, name), not name: the same malformed name in two + // clients is two facts, and a name-only key silently dropped one of them. + const neighbours = new Map(); + // The (client, name) pairs this run actually re-wrote. Suppression must be + // keyed the same way the FACT is: `update` writes only to a server's own + // `originalClients`, so a malformed copy of that name in a DIFFERENT client + // is neither updated nor — under a name-scoped filter — reported, silently + // suppressed by its own success elsewhere. + const writtenPairs = new Set(); + for (const r of withUpdates) { const entry = entryMap.get(r.name); @@ -265,10 +333,23 @@ export async function handleUpdate( // so we can warn the user instead of silently leaving them on the old // version. The store record still advances (best-effort write semantics). const clientErrors: string[] = []; + // #59: kept separate from clientErrors. These are things the user should + // know about a client that WAS updated — routing them through the error + // list made the output say "could not update claude-desktop" about a + // client it had just updated. + const clientNotes: string[] = []; for (const clientId of originalClients) { try { const rawEntry = resolveInstallEntry(entry, clientId); - const existingEnv = await readExistingEnv(getAdapter, getConfigPath, clientId, r.name); + const existingEnv = await readExistingEnv( + getAdapter, + getConfigPath, + clientId, + r.name, + (note) => clientNotes.push(note), + (cid, skipped) => + neighbours.set(JSON.stringify([cid, skipped]), { name: skipped, clientId: cid }) + ); const newEntry: McpServerEntry = { ...rawEntry, ...(existingEnv && Object.keys(existingEnv).length > 0 @@ -278,6 +359,7 @@ export async function handleUpdate( const adapter = getAdapter(clientId); const configPath = getConfigPath(clientId); await adapter.addServer(configPath, r.name, newEntry, { force: true }); + writtenPairs.add(JSON.stringify([clientId, r.name])); } catch (err) { // Some clients may not support this server type, or the config may be // unwritable — collect the failure and warn (store record still advances). @@ -295,27 +377,44 @@ export async function handleUpdate( await addInstalledServer(finalRecord); // Record outcome immutably instead of mutating the result object - updateOutcomes.set(r.name, { updated: true, trustScore, clientErrors }); + updateOutcomes.set(r.name, { updated: true, trustScore, clientErrors, clientNotes }); if (!isJson) { // Surface partial config-write failures so a client silently left on the // old version is visible to the user (mirrors the up.ts warning suffix). const warning = - clientErrors.length > 0 + (clientErrors.length > 0 ? chalk.yellow(` (warning: could not update ${clientErrors.join("; ")})`) - : ""; + : "") + + (clientNotes.length > 0 ? chalk.yellow(` (note: ${clientNotes.join("; ")})`) : ""); output( ` ${chalk.green("✓")} Updated ${chalk.white(r.name)} to ${chalk.green(r.newVersion)} [${levelColor(levelLabel(trustScore))}]${warning}` ); } } + const unrelated = [...neighbours].filter(([key]) => !writtenPairs.has(key)); + if (unrelated.length > 0) { + const body = + `${unrelated.length} other malformed entr${unrelated.length === 1 ? "y was" : "ies were"} ` + + `skipped and not updated: ` + + `${unrelated.map(([, e]) => `${sanitizeForTerminal(e.name)} (${e.clientId})`).join(", ")}. ` + + `Run \`mcpm doctor\` for details.`; + // #59: --json has no field for this and stdout must stay parseable, so the + // notice goes to stderr — otherwise replacing read()'s stderr default + // emitted it NOWHERE, leaving a malformed entry LESS visible than before + // this PR. Same resolution list.ts uses for the same problem. + if (isJson) process.stderr.write(`mcpm: ${body}\n`); + else output(chalk.yellow(` ${body}`)); + } + if (isJson) { output( JSON.stringify( results.map((r) => { const outcome = updateOutcomes.get(r.name); const clientErrors = outcome?.clientErrors ?? []; + const clientNotes = outcome?.clientNotes ?? []; return { name: r.name, oldVersion: r.oldVersion, @@ -324,6 +423,7 @@ export async function handleUpdate( trustScore: outcome?.trustScore ?? null, error: r.error ?? null, clientErrors: clientErrors.length > 0 ? clientErrors : null, + clientNotes: clientNotes.length > 0 ? clientNotes : null, }; }), null, diff --git a/src/config/adapters/base.ts b/src/config/adapters/base.ts index 9637ce5..32629a2 100644 --- a/src/config/adapters/base.ts +++ b/src/config/adapters/base.ts @@ -159,11 +159,25 @@ export abstract class BaseAdapter implements ConfigAdapter { // the skip into its own reporting instead of (or in addition to) stderr. // Default preserves the original stderr-warning behavior for every caller // that doesn't opt in. + // #59 closed the remaining unwired callers: diff (a dropped entry was + // reported "missing"), export (silently absent from a stack file the user + // keeps), import (silently absent from the pick-list), list (silently + // absent from the inventory, incl. `--json`), sync/doctor's drift model (a + // client that HAS the server was reported as lacking it), update (the + // force re-write discarded the entry's env block), and up --strict (left + // behind while reporting a clean reconciliation). guard/cli.ts's two sites + // pass a NO-OP on purpose — the orchestrator names the same entry in the + // same invocation, so the default would print it twice; see there. // #23 follow-up (adversarial review): name is config-supplied (attacker- // controllable) and reaches the real terminal via this default — sanitize // it the same way doctor.ts/guard's cli.ts already do for this class of // value, so a name like `srv\x1b]0;evil\x07` can't inject terminal escapes. - onSkip: (name: string) => void = (name) => + // #59: the RAW entry is passed alongside the name. A caller that is about + // to OVERWRITE this entry needs to know what it would discard, and the + // validated map cannot tell it — the entry is not in there. Deliberately + // `unknown`: it failed validation, so a consumer must narrow whatever + // single field it needs and must not spread it. + onSkip: (name: string, raw: unknown) => void = (name) => process.stderr.write( `mcpm: skipping malformed server entry "${sanitizeForTerminal(name)}" in ${configPath} ` + `(${this.clientId}): does not match the expected shape.\n`, @@ -183,7 +197,7 @@ export abstract class BaseAdapter implements ConfigAdapter { // #23: skip rather than propagate a malformed entry — better an // absent server than one silently corrupting a downstream transform // (e.g. spreading a string `args` char-by-char in guard/wrap.ts). - onSkip(name); + onSkip(name, entry); } } return out; diff --git a/src/config/adapters/index.ts b/src/config/adapters/index.ts index 901b8ec..74e9520 100644 --- a/src/config/adapters/index.ts +++ b/src/config/adapters/index.ts @@ -35,12 +35,12 @@ export interface ConfigAdapter { /** * Read all MCP server entries from the config file. An entry that fails - * shape validation is dropped rather than propagated; `onSkip` (name) is + * shape validation is dropped rather than propagated; `onSkip` (name, raw) is * called for each drop — defaults to a stderr warning (see BaseAdapter). */ read( configPath: string, - onSkip?: (name: string) => void, + onSkip?: (name: string, raw: unknown) => void, ): Promise>; /** diff --git a/src/config/drift.ts b/src/config/drift.ts index 5e61ec1..f4efe68 100644 --- a/src/config/drift.ts +++ b/src/config/drift.ts @@ -16,8 +16,8 @@ * header KEY set. It NEVER compares env / header VALUES — those are secrets, and * two clients legitimately hold the same key with a per-machine value. * - * Exports: DriftDeps, ClientState, ServerDrift, DriftModel, collectClientStates, - * buildDriftModel. + * Exports: DriftDeps, ClientState, ServerDrift, DriftModel, + * collectClientStatesWithErrors, buildDriftModel. */ import type { ClientId } from "./paths.js"; @@ -37,6 +37,15 @@ export interface DriftDeps { export interface ClientState { readonly clientId: ClientId; readonly servers: Record; + /** + * #59: names this client's config DOES hold but which `read()` dropped for + * failing shape validation (#23). Kept separate from `servers` — the entry + * is unusable — but not discarded, because "we could not read it" and "it is + * not there" are different facts and only the second one is drift. + * Optional so a caller that has not collected them (older tests, callers + * predating #59) is simply treated as having none. + */ + readonly malformed?: readonly string[]; } export interface ServerDrift { @@ -45,6 +54,12 @@ export interface ServerDrift { readonly present: readonly ClientId[]; /** Clients (with readable configs) that lack this server. */ readonly absent: readonly ClientId[]; + /** + * Clients that hold this server in an entry `read()` could not validate. + * Deliberately disjoint from both `present` and `absent`: the client is not + * missing the server, and the entry cannot be compared against anything. + */ + readonly malformed: readonly ClientId[]; /** True when the `present` clients disagree on the server's shape. */ readonly conflict: boolean; /** Which fields diverge among the `present` clients (only when conflict). */ @@ -52,6 +67,12 @@ export interface ServerDrift { } export interface DriftModel { + /** + * #59: clients whose config could not be read AT ALL (missing or unparseable + * JSON). They contribute no servers and no `malformed` names, so without + * this a whole broken config is quieter than one mis-typed entry inside it. + */ + readonly unreadableClients?: readonly ClientId[]; /** Clients considered — those whose config was readable. Sorted. */ readonly clients: readonly ClientId[]; /** One entry per distinct server name, sorted by name. */ @@ -72,18 +93,33 @@ export interface DriftModel { * broken config can't blind the whole cross-client view (same posture as * `diff` / `export`). */ -export async function collectClientStates(deps: DriftDeps): Promise { +/** + * Read each detected client's config into a `ClientState`, and report which + * clients could not be read at all. Never throws — one broken config must not + * blind the whole cross-client view — but the caller is told, because a config + * that exists and cannot be parsed is a bigger coverage gap than one mis-typed + * entry inside it, and used to be the quieter of the two. + */ +export async function collectClientStatesWithErrors( + deps: DriftDeps +): Promise<{ states: ClientState[]; unreadableClients: ClientId[] }> { const clients = await deps.detectClients(); const states: ClientState[] = []; + const unreadableClients: ClientId[] = []; for (const clientId of clients) { try { - const servers = await deps.getAdapter(clientId).read(deps.getPath(clientId)); - states.push({ clientId, servers }); + const malformed: string[] = []; + const servers = await deps + .getAdapter(clientId) + .read(deps.getPath(clientId), (name) => malformed.push(name)); + states.push({ clientId, servers, malformed }); } catch { - // Skip unreadable clients (missing or malformed config). + // A config that is missing is ordinary; one that exists but cannot be + // parsed is a coverage gap the caller should be able to surface. + unreadableClients.push(clientId); } } - return states; + return { states, unreadableClients }; } // --------------------------------------------------------------------------- @@ -130,13 +166,29 @@ export function buildDriftModel(states: readonly ClientState[]): DriftModel { } } + // #59: same, for names whose entry read() dropped. A malformed name may or + // may not also appear in byName (another client can hold a valid entry), so + // the two maps are unioned below rather than treated as alternatives. + const malformedByName = new Map(); + for (const { clientId, malformed } of states) { + for (const name of malformed ?? []) { + malformedByName.set(name, [...(malformedByName.get(name) ?? []), clientId]); + } + } + const servers: ServerDrift[] = []; - for (const name of [...byName.keys()].sort()) { - const holders = byName.get(name)!; + for (const name of [...new Set([...byName.keys(), ...malformedByName.keys()])].sort()) { + const holders = byName.get(name) ?? []; const present = holders.map((h) => h.clientId).sort(); - const presentSet = new Set(present); - const absent = clients.filter((c) => !presentSet.has(c)); - + const malformed = (malformedByName.get(name) ?? []).slice().sort(); + // A client holding a malformed entry is NOT missing the server. Counting it + // as absent was a false statement that told the user to re-add a server + // they already have. + const accountedFor = new Set([...present, ...malformed]); + const absent = clients.filter((c) => !accountedFor.has(c)); + + // Only VALID entries are compared: the malformed one is precisely the shape + // we could not read, so it must not manufacture a conflict. const fields = holders.length > 1 ? divergingFields(holders.map((h) => h.entry)) : []; const conflict = fields.length > 0; @@ -144,11 +196,18 @@ export function buildDriftModel(states: readonly ClientState[]): DriftModel { name, present, absent, + malformed, conflict, ...(conflict ? { conflictFields: fields } : {}), }); } - const drifted = servers.filter((s) => s.absent.length > 0 || s.conflict).length; + // A server mcpm could not read is not a server it verified to be in sync, so + // it counts as drifted (`sync --check` exits 2). Deliberately NOT a new exit + // code: 2 already means "not clean, read the output", and the output now says + // which of the three reasons applies. + const drifted = servers.filter( + (s) => s.absent.length > 0 || s.conflict || s.malformed.length > 0 + ).length; return { clients, servers, inSync: servers.length - drifted, drifted }; } diff --git a/src/guard/cli.ts b/src/guard/cli.ts index c8e3246..596ae24 100644 --- a/src/guard/cli.ts +++ b/src/guard/cli.ts @@ -190,7 +190,15 @@ async function collectConfineTargets(opts: ConfineComputeOpts): Promise {}); } catch { continue; // missing/unreadable config — skip (mirrors enable's read handling) } @@ -491,12 +499,13 @@ async function warnUnresolvablePlaceholders(opts: DisableOpts): Promise { if (opts.client !== undefined && clientId !== opts.client) continue; let entries; try { - // #23 follow-up (adversarial review, not wired): a malformed entry is - // invisible to this advisory placeholder scan too — a real gap, but - // the reachable case is narrow (a keychain placeholder inside an - // otherwise-malformed entry) and this warning is best-effort by design - // ("cannot turn an otherwise-successful disable into a failure"). - entries = await getAdapter(clientId).read(getConfigPath(clientId)); + // #59: a malformed entry is invisible to this advisory placeholder scan. + // That is accepted — the residual loss is one second-order warning (an + // unresolvable keychain placeholder) about a server whose first-order + // problem `disable` already reports by name, via planForClient's + // `skipped` list. A NO-OP rather than the default, because the default's + // stderr line is what printed that name a second time in one command. + entries = await getAdapter(clientId).read(getConfigPath(clientId), () => {}); } catch (err) { // A missing config is expected; surface anything else (permissions, // malformed JSON) rather than silently skipping it.