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 aclean empty scan |
the root cause was max(..., default=0) vs a threshold of0 |
| #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.
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}
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 signedGET /{name}) carries it per-version under versions[v].dist.attestations
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.
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:
[\s_\-.]+ after camel splitting) and matched per whole segmentget, read, list, search, fetch, …) suppresses 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.
--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.
Not talent — shape:
main before the fix was claimed. Three of four in #88, both in #87.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.