Three merged PRs in an MCP security scanner: the review that found my bug, and two more

# mcp# security# supplychain# python
Three merged PRs in an MCP security scanner: the review that found my bug, and two moreEdison Flores

What actually landed in mcp-guard this week: an npm attestations verifier whose happy path could never fire (caught in review), keyword matching without false positives, and exit codes as API.

What happened

Three PRs merged into yunaremaia/mcp-guard — a supply-chain/security scanner for MCP servers — in roughly 36 hours:

PR What it did The interesting part
#87 scan --fail-on low no longer exits 1 on a clean empty scan the root cause was max(..., default=0) vs a threshold of 0
#88 keyword matching no longer flags read-only tools as CRITICAL substring matching + the false-positive/false-negative trade-off
#86 mcp-guard verify — npm supply-chain verification (attestations, provenance, strict policy) the maintainer's review found that my feature could never report "signed"

That last cell is the reason this post exists. The rest is what the merge taught.

The embarrassing one: a feature that could never say "yes"

verify resolves an npm package ref, hits the registry, and checks for npm's provenance attestations (sigstore bundles). I shipped v1 with the documented endpoint shape:

GET /-/npm/v1/packages/attestations/{name}/{version}
Enter fullscreen mode Exit fullscreen mode

316 tests green, 100% coverage on the new module, smoke-tested live against the registry. Merged? Not yet — the review came back with a reproduction: that URL 404s for every package, including @sigstore/sign — the reference implementation of the thing being verified. Every verdict was structurally "unsigned." A security tool whose happy path can never fire.

The real contract, verified against the live registry:

  • GET /{name}/{version} (the manifest) carries dist.attestations.url when — and only when — the version is signed
  • packages without attestations simply lack the field
  • the packument (GET /{name}) carries it per-version under versions[v].dist.attestations
  • only the pathname of the discovered URL is trusted; it's re-attached to the registry origin, never followed as an absolute host (normalization in the spirit of pacote)

So the manifest became the single source of truth: unpinned refs resolve dist-tags + versions[latest] from the packument; pinned refs fetch the manifest directly and a 404 there is a real not_found instead of a disguised unsigned. Two states that a policy gate (--policy strict, exit 1 on unsigned) must never confuse.

The takeaway: green tests mean the code matches your model of the world. The reviewer's repro checked the model. For anything that talks to a live registry, "verified in vivo" needs to include the negative control — and ideally someone who tries the endpoint you were sure about.

The subtle one: when a fix trades a false positive for a false negative

mcp-guard flags tools as write/destructive by keyword. It matched substrings: get_address contained "add"... which is a write verb fragment; worse, names like read_settings and search_update_records tripped the update rule. Read-only tools flagged CRITICAL (#84).

The fix design:

  • names are tokenized (snake/kebab/camelCase split with [\s_\-.]+ after camel splitting) and matched per whole segment
  • descriptions match on word boundaries
  • a leading read-only verb (get, read, list, search, fetch, …) suppresses only the name-based match — the description channel stays live

First version of the guard: the review caught it — get_and_delete_user stopped flagging. For a security tool, trading "read-only flagged as critical" for "destructive not flagged at all" is the wrong direction. The merged rule: suppression only while the keyword follows the verb directly through a conjunction (get_delete_and_remove_user still flags, get_and_delete_user doesn't), plus inflection handling (Deletes/dropping/modifies) for descriptions.

Same lesson as the fusion post from today, inverted: assert the shape of the whole class, not just the case you came to fix. The test suite parametrizes both directions — the five false positives from the issue and the true positives the naive guard would have swallowed.

The small one: exit codes are API

--fail-on low with a zero-findings scan exited 1, because max(severities, default=0) >= 0 is always true. An empty scan is "nothing found at the strictest gate" — that's success (#82, fixed by default=-1). Two-line fix, two regression tests, and worth filing anyway: CI pipelines drink exit codes, and a flaky 1 on clean scans trains people to ignore the gate.

What actually got three PRs merged

Not talent — shape:

  1. One mechanism per PR. The exit-code PR didn't touch the matcher. The matcher PR didn't touch exit codes. Reviewable in one sitting, revertable independently.
  2. Tests that fail first. Every PR's tests were verified red on main before the fix was claimed. Three of four in #88, both in #87.
  3. Repro before argument. Every issue got the mechanism verified live (the exact versions, the exact numbers) before a fix was proposed.
  4. Review comments answered with commits, not prose. Both reviews came back with technical objections; both were answered by push + a comment explaining the diff. The maintainer merged within hours of each.
  5. Scope honesty. drop_in_query still false-flags (adjective vs verb needs semantics) — documented in the PR as out of scope rather than silently shipped.

The repo is young, the maintainer is fast and rigorous, and the issues are real bugs with clean repros — if you want a place to practice security-tool contributions, the issue tracker is open.

Disclosure: the investigation, fixes and tests were AI-assisted (Claude/GLM), human-directed — same disclosure is on the linked PRs. The mcp-guard maintainer's reviews are what made all three PRs better than their first versions.