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

A developer merged three pull requests into yunaremaia/mcp-guard, an open-source supply-chain security scanner for MCP servers, after a maintainer's review found that the new `mcp-guard verify` feature could never report an npm package as signed. The documented attestations endpoint returned 404 for every package, so the tool's happy path was structurally unreachable; the fix switched to reading `dist.attestations.url` from the package manifest and packument, and separate PRs corrected a `max(..., default=0)` exit-code bug in `scan --fail-on low` and replaced substring keyword matching that flagged read-only tools as CRITICAL.

Three PRs merged into yunaremaia/mcp-guard https://github.com/yunaremaia/mcp-guard — a supply-chain/security scanner for MCP servers — in roughly 36 hours: | PR | What it did | The interesting part | |---|---|---| | 87 https://github.com/yunaremaia/mcp-guard/pull/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 https://github.com/yunaremaia/mcp-guard/pull/88 | keyword matching no longer flags read-only tools as CRITICAL | substring matching + the false-positive/false-negative trade-off | | 86 https://github.com/yunaremaia/mcp-guard/pull/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 https://github.com/npm/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 https://github.com/yunaremaia/mcp-guard/pull/86 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 GET /{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 https://github.com/yunaremaia/mcp-guard/pull/84 . The fix design: \s \-. + after camel splitting and matched per whole segment get , read , list , search , fetch , … suppresses First version of the guard: the review caught it https://github.com/yunaremaia/mcp-guard/pull/88 issuecomment-5972930919-ish — 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 https://github.com/yunaremaia/mcp-guard/pull/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 https://github.com/yunaremaia/mcp-guard/issues 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.