fix(#59): wire read()'s onSkip into every caller — incl. a v0.34.0 env-loss regression in mcpm update

.gitignore

@@ -37,3 +37,6 @@ tmp/
 # MCP Registry publisher
 server.json
 research/
+
+# Local Claude Code permissions (per-machine, never shared)
+.claude/settings.local.json

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

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.

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<string, McpServerEntry> = {}) {
+  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<typeof vi.fn>).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<typeof vi.fn>).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<typeof vi.fn>).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<typeof vi.fn>).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<ClientId[]>>()
+        .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<typeof vi.fn>).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<typeof vi.fn>).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<typeof vi.fn>).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
+  });
+});