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

> Source: <https://dev.to/edison_flores_6d2cd381b13/three-merged-prs-in-an-mcp-security-scanner-the-review-that-found-my-bug-and-two-more-3887>
> Published: 2026-10-03 21:21:41+00:00

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.*