Skip to content

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

Merged
m1ngshum merged 18 commits into
mainfrom
fix/onskip-callers-59
Sep 4, 2026
Merged

fix(#59): wire read()'s onSkip into every caller — incl. a v0.34.0 env-loss regression in mcpm update#199
m1ngshum merged 18 commits into
mainfrom
fix/onskip-callers-59

Conversation

@m1ngshum

@m1ngshum m1ngshum commented Sep 4, 2026

Copy link
Copy Markdown
Member

Closes TODOS #59 — BaseAdapter.read()'s onSkip seam (#23, v0.34.0) was not wired into every caller.

Reading the call sites first turned up something the backlog had not: one of them is a data-loss regression, not a reporting gap. #59 was filed as "fail-safe as-is — a coverage gap, not a live bug."

1. mcpm update destroyed a malformed entry's env block

readExistingEnv reads through read(), which since #23 drops an entry failing shape validation. So for a user whose entry is malformed in one field (args: "-y pkg" instead of an array) but whose env holds real API keys, it returned undefined — and the force: true re-write discarded them.

Verified against the pre-fix binary in an isolated $HOME:

  ✓ Updated io.github.getmcpm/cli to 0.34.0 [caution]

...while the entry went from {command, args, env:{MY_API_KEY:"super-secret-value"}} to {command, args}. It reports success while deleting the secret. Shipped in v0.34.0, live through v0.36.0.

The existing env-preservation test ("carries the user's existing env values into the new entry", added as "Fix #1 — a regression would silently wipe them") could not see it: it mocks read() to return the entry, which is the one thing the real read() stopped doing.

readExistingEnv now distinguishes "has no env" from "read() dropped it"; the caller refuses the destructive write and reports through the existing clientErrors channel.

2. A malformed entry was reported as a missing server

Five sites, each making a claim the dropped entry falsified:

site the false claim
sync --check told the user a client was missing a server it has; when only one client held it, the server vanished from the model and the gate exited 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 need to fix
export silently omitted from a stack file the user keeps as their declared state
import silently absent from the pick-list, then "No existing MCP servers found"
up --strict left behind while reporting a clean reconciliation

up --strict still does not delete it — the fail-safe direction #23 chose is kept. Silence was the bug, not the refusal to delete.

guard/cli.ts's two sites deliberately stay on the default. planForClient already routes the entry into skipped with a reason and printClientReport prints name + reason on both enable and disable, so wiring them would print the same name twice in one command. Recorded in the code rather than left looking unfinished.

Three of the fixes introduced false statements of their own

All found by dogfooding the built binary, none by the suite:

  • export/import warned a name was omitted when another client had supplied a good copy — the export was complete and the warning said otherwise.
  • sync summarised 1 drifted (0 missing in ≥1 client, 0 shape conflicts) — a drift count with no stated cause.
  • doctor rendered ⚠ broken-srv — in ; missing in cursor: empty present list, and it never named the client actually holding the server.

Verification

  • 2843 tests green, tsc --noEmit clean.
  • 17 mutations, all caught. Two survived first and were fixes to the tests: toContain("?") was satisfied by the detail line under the table rather than the matrix cell, and doctor's wiring had nothing pinning it at all. One doctor assertion also got a positive control — it checked an absent-list exclusion that would have passed vacuously had the entry dropped out of the drift list entirely.
  • Dogfooded end to end in an isolated $HOME across sync, diff, export, import, doctor, list, up --strict, and update (including the pre-fix counterfactual above), for both the single-client and cross-client cases.

Contract notes

sync --json is the frozen shape and gains only an additive malformed field. diff --json gains an unreadable status value and doctor --json an unreadable drift kind; both are UNSTABLE per docs/CONTRACTS.md.

Also removes six byte-identical duplicate read keys in adapter test helpers (esbuild warned on every run; the second silently won, so changing only the first would have had no effect).

@m1ngshum
fix(update): refuse to overwrite a client entry read() could not vali…
1c154f3 
…date (#59)

Since #23 (v0.34.0) BaseAdapter.read() drops an entry that fails shape
validation, so readExistingEnv()'s `servers[name]?.env` returned undefined
for a malformed entry and the `force: true` re-write discarded its env
block — a user's API keys, lost silently, for an entry malformed in some
other field.

readExistingEnv now distinguishes "no env" from "read() dropped it" and
the caller refuses the destructive write, surfacing it through the existing
clientErrors channel (human output + --json).

The pre-existing env-preservation test could not see this: it mocks read()
to return the entry, which is the one thing the real read() stopped doing.
@m1ngshum
test: remove duplicate read keys in six adapter test helpers
1cc847d 
Byte-identical duplicates (esbuild warned on every run). The second silently
won, so changing only the first would have had no effect.
@m1ngshum
fix(sync): a malformed entry is no longer reported as a missing server (
26166e4 
#59)

collectClientStates discarded read()'s onSkip signal, so an entry dropped for
failing shape validation was indistinguishable from one that was never there.
sync --check therefore told the user a client was MISSING a server it
demonstrably has (sending them to re-add it), and when only one client held
the server it vanished from the model entirely — a clean exit 0 over a config
mcpm could not read.

ClientState/ServerDrift gain `malformed`; a malformed holder is excluded from
both `absent` and the shape comparison, and counts as drifted (exit 2 already
means "not clean, read the output" — no new exit code). Rendered as ? with its
own legend entry and detail line. doctor feeds its existing skippedEntries in
so the cross-client view stops contradicting the DoctorIssue it already
raises. `malformed` on ClientState is optional, so callers predating this
compile unchanged.
@m1ngshum
test(#59): pin the sync matrix cell and doctor's cross-client agreement
a3a22b1 
Both assertions were too weak and survived their mutants: toContain("?") was
satisfied by the detail line under the table rather than the matrix cell, and
doctor had nothing pinning the malformed-entry wiring at all.
@m1ngshum
fix(#59): wire read()'s onSkip into diff, export, import and up --strict
479dbbc 
The last five call sites left on the stderr-only default, each of which made a
claim the dropped entry falsified:

- diff reported a declared-but-unreadable server as "missing" — sending the
  user to `mcpm up` to re-install over an entry they only need to fix. New
  "unreadable" DiffStatus; an UNDECLARED unreadable entry was invisible to
  both loops and now surfaces too.
- export silently omitted it from a stack file the user keeps as their
  declared state. Named on stderr (not `output`: with no --output the YAML
  itself goes to stdout).
- import silently dropped it from the pick-list, and could then print "No
  existing MCP servers found" when unreadable entries were the only ones
  present. Reported before that line.
- up --strict left it behind while reporting a clean reconciliation. Still
  NOT deleted — the fail-safe direction #23 chose is kept — but reported as
  skipped. Silence was the bug, not the refusal to delete.

diff --json gains a status value; it is UNSTABLE per CONTRACTS. sync --json is
the frozen one and gains only an additive field.
@m1ngshum
fix(#59): three false statements the fixes themselves introduced, fou…
f74e62f 
…nd by dogfooding

- export/import warned that a name was omitted when ANOTHER client had
  supplied a well-formed copy of it — the export was complete and the warning
  said otherwise. Only names no client could supply are reported now.
- sync's summary read "1 drifted (0 missing, 0 shape conflicts)" — a drift
  count with no stated cause. Unreadable entries are counted in it.
- doctor's cross-client line rendered "in ; missing in cursor" when the only
  holder's entry was unreadable: an empty present list, and no mention of the
  client that actually holds the server. New "unreadable" DoctorDriftEntry
  kind; the absent branch names unreadable holders too.

Also strengthened the first doctor test with a positive control — it asserted
an absent-list exclusion that would have passed vacuously had the entry
dropped out of the drift list entirely.
@m1ngshum
docs: CHANGELOG for the #59 fixes
cd1b58d
@m1ngshum
fix(#59): address 19 adversarial-review findings
d551267 
Two independent Opus reviews (correctness/security, test-coverage) with
opposed mandates. Four of the findings were regressions the fixes themselves
introduced, and two reproduced verbatim the bugs the CHANGELOG claimed to
have closed.

Correctness:
- diff went COMPLETELY SILENT when a name was malformed in one client and
  valid in another — 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, worse than the behaviour it replaced.
- update's refusal was the wrong fix. It printed `✓ Updated`, advanced the
  store and wrote zero configs, and its own remediation was unreachable (the
  warning fires once, then "All servers are up to date"). Worse, overwriting
  a mis-shaped entry is the user's SELF-REPAIR path, so refusing turned a
  self-healing case into a stuck one. onSkip now carries the raw entry and
  update recovers env from it with a narrow parse: repairs AND preserves.
- sync still printed `in ; missing in cursor` — fixed in doctor, missed in
  the command the same CHANGELOG bullet holds up as the CI gate — and
  double-counted the server under "missing in >=1 client".
- export's `name in servers` walked the prototype chain, so entries named
  toString/constructor/valueOf vanished from the warning and the file.
- Names reached the terminal unsanitized in three new render sites.
- Both guard/cli.ts rationales were INVERTED: keeping the default is what
  duplicated the line; a no-op is what removes it.
- list was neither wired nor listed as a deliberate exception.
- import's "No existing MCP servers found" contradicted the warning above it.
- doctor's conflict branch dropped malformed, asymmetric with absent.

Test coverage — the headline finding is mine, in the class I sent the
reviewer hunting for. The central drift test built a state read() can never
produce (a name in both `servers` and `malformed`; base.ts is an if/else),
so its assertion was true by construction and deleting the fix left the file
green. A second test passed against unmodified origin/main. Also unpinned:
the drifted predicate's malformed clause, update's name filter, up --strict's
recordResult channel, import's good-copy filter, diff's guard, both doctor
render branches, and every sanitize call.

20 mutations, all caught. Dogfooded end to end per finding.
@m1ngshum
fix(#59): third-review findings — a hole in the headline fix, and a s…
0167c15 
…ilent CI failure

The third review re-derived from the diff rather than checking the fix list,
which is how this project's history says its worst bugs get found. Both HIGH
findings reproduced.

H2 — the env-recovery fix had a hole in exactly the population it targets.
`env` is frequently the field that makes an entry invalid (a numeric port is
the archetypal hand-edit), and parsing the whole record then rejected EVERY
key: measured, `{API_KEY:"s3cret", PORT:8080}` came back as no env at all,
still under `✓ Updated`. Recovery is now PER KEY, and a key that cannot be
carried is named rather than dropped in silence. Same site: replacing the
default onSkip had silenced the warning for unrelated malformed neighbours;
re-stated.

H1 — `sync --check` exited 2 while printing only "nothing to compare across
clients". renderDashboard returns early for 0/1 clients and for no servers,
but `drifted` comes from the model, so on the common single-client desktop
shape a CI gate failed with the entry named on neither stream. Unreadable
entries and clients are now reported before every early return.

M3 — an entirely unparseable config passed --check silently while one bad
entry inside it failed. Both fail now.

M1 — up --strict's two render sites took the name raw, while the CHANGELOG
already claimed sanitization at "every new render site". Fixed both.

M2 — list --json flipped from a bare array to an object only when something
was skipped, breaking JSON.parse(out).map() exactly when things are wrong.
Shape is unconditional again; the notice goes to stderr, which the CLI has
and the MCP surface does not (that is why handlers.ts differs).

L1/L3 — CONTRACTS.md records the sync --json and exit-code change instead of
still calling it frozen-and-additive; sync's "N missing in >=1 client"
sub-count comes from the model again rather than from a filtered section.

Not acted on: L6 was stale (the reviewer read a pre-fix fixture; the shipped
one already uses the reachable shape) and L5 is a pre-existing __proto__ hazard
in valid-entry handling, filed rather than ridden in.
@m1ngshum
fix(update): a note about a client that WAS updated no longer reads a…
77fb695 
…s 'could not update' it

Routing the #59 recovery notes through clientErrors made the success line say
"warning: could not update claude-desktop" about a client it had just written.
@m1ngshum
fix(#59): round-4 findings — and untrack a local settings file
0c79ea9 
0 HIGH; three MEDIUM, two of them the class the previous two commits were
written to remove.

M1 — `update` said a server was "not updated" one line before "✓ Updated"
it. The neighbour warning fired from inside the per-server read, so it named
a server this run was about to update, and repeated once per updated server.
Collected across the run and reported once, excluding names this run updated.

M2 — the CHANGELOG described two implementations the diff REJECTED: a
`z.record(z.string(), z.string())` parse (there is none; the code argues
against it two lines above) and a `list --json` `skipped` field (routed to
stderr on purpose). Both checkable against files in the same PR. Every
remaining claim re-verified against the code by grep.

M3 — `sync` printed "No client configs found. Install a server first"
immediately after naming the configs it found: false, and the wrong
remediation for broken JSON. The same contradiction this PR already fixed in
import.ts and list.ts, left unfixed in the third place.

Also: read()'s `raw` argument — the entire mechanism the env recovery depends
on — was pinned by NO test; neutering it left all 2868 green while update
silently wiped env blocks again. Pinned in base-read-validation. An env key
named `__proto__` with a string value was dropped by the object-literal
setter, contradicting the block's own promise to name what it cannot carry
(Object.create(null) now; same class as v0.36.0's pin-hash fix). A non-object
`env` returned without a note. Unused zod import removed. sync's
parenthetical no longer adds a client count into a server count.

.claude/settings.local.json was committed by accident in 77fb695 — a
per-machine permissions file, untracked on main. Removed and gitignored.
@m1ngshum
test(#59): the neighbour-suppression assertion could never match
dbca73e 
/srv-b.*not updated/ required the name BEFORE the phrase; the report puts the
phrase first, so it passed with the bug live (mutation W2 survived). Asserts
on the exact rendering now, with a positive control.
@m1ngshum
fix(#59): round-5 — update --json emitted the malformed-entry notice …
e3355dd 
…nowhere

MEDIUM: the custom onSkip replaces read()'s stderr default for EVERY entry in
the config, and the neighbour report was gated on !isJson — so under --json
there was neither a field nor a stderr fallback, and a malformed entry became
LESS visible than on main. That is this PR's own thesis inverted. list.ts had
already solved the identical problem the right way; update --json was the one
new call site that did not follow it.

Also: `neighbours` was keyed by name alone, so the same malformed name in two
clients reported one of them and which one depended on iteration order — now
keyed by client+name. Stale JSDoc on SyncResult.drift (it is true for more
than absence/conflict now). CONTRACTS' exit-code table — the row the doc tells
scripts to depend on — did not mention that 2 now covers "could not verify";
the --json section 20 lines below did.

Two dead assertions removed: one 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; the other asserted a prototype-pollution
needle nothing in the test writes, replaced with a real prototype check.
@m1ngshum
refactor(update): key the neighbour map with JSON.stringify([client, …
8b7e9dc 
…name])

A NUL separator worked but made every tooling round-trip over this file
awkward. Same uniqueness guarantee, no control character.
@m1ngshum
fix(update): suppress the neighbour notice by (client, name), not by …
856ab0f 
…name

Round 5 changed the neighbours map KEY to (client, name) because the same
malformed name in two clients is two facts. The FILTER was left name-scoped,
so a malformed copy in a client the server is NOT installed in — never
written, never reported — was suppressed by the name's own success elsewhere.
Suppression is now keyed by the (client, name) pairs actually re-written.

The fix passed all 42 existing tests, so it was unpinned; added the exact
cross-client case.
@m1ngshum
refactor(drift): drop collectClientStates, orphaned by this PR
9bbd460 
sync was its only production caller and moved to collectClientStatesWithErrors;
folded its tests onto the surviving function. Also two indentation regressions
introduced earlier in the branch.
@m1ngshum
test(#59): pin the last three unpinned lines
0ee7ef1 
Round 7 converged with no defect, but three lines survived the whole suite:
`writtenPairs.add` sitting AFTER the await (moving it before lets a failed
write suppress the report for an entry still malformed on disk), and the
sanitize calls on update's neighbour notice, its dropped-env-key note, list's
warning and export's warning — all four render values a config file controls,
while every other new render site already had an escape test.

Also corrects the CHANGELOG's sanitized-site list, which under-enumerated for
the third time.
@m1ngshum
refactor(list): one skip-notice builder, not two
33e3878 
The duplicate copy actively hid a gap: a sanitization mutation applied to the
--json path survived, because the only test covering that sentence exercised
the text path. One builder, two sinks.
@m1ngshum
m1ngshum merged commit 947e77e into main Sep 4, 2026
8 checks passed
@m1ngshum
m1ngshum deleted the fix/onskip-callers-59 branch September 4, 2026 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@m1ngshum