Repository navigation
fix(mcp): one server name in two files is one capability, reconciled - #445
Conversation
A `codex_config` row over a workspace where two packages each carry a `.mcp.json` naming `github` aborted the scan with `'pkg_b/.mcp.json' was read twice as one tool source [...] Remove the repeated shipgate.yaml entry naming 'pkg_b/.mcp.json'` — false on both counts, and naming an edit nobody could make: the manifest names `path: .` once, and that file was read once. Two deliberate rules collided. An MCP capability is identified by `(server, tool)` and by nothing else, so that moving a `.mcp.json` is not a capability change (`mcp audit` pins this); and one identity may be observed only once. Qualifying the minted id with the path resolves the collision and breaks the first rule, so it stays rejected — a named test keeps it rejected. The answer is that the two files declare one capability twice, and the reader reconciles them before the catalog sees them. Identical declarations are silent. Nothing was dropped, and a source warning is a gating input; the ordinary monorepo layout gains no evidence gap. Sameness is decided on the declaration as written, not on the fields that reach a catalog row, so two files agreeing on every tool and disagreeing on the command that serves them are not identical. Disagreeing declarations are merged conservatively, never by picking one. The merged server carries the union of the tools; a claim that raises risk survives from any one declaration (`destructiveHint`, an unenumerated `server.*` remainder, secret environment names, an external URL, an authority scope, `approval_mode: approve` — which reaches even a tool whose own file said nothing about approval); a reassuring claim survives only when every declaration makes it (`readOnlyHint`, a local-documentation server, a transport); and two files describing one tool with different schemas leave the interface unknown rather than publishing either. A declaration the auth parser refuses stays refused rather than being rebuilt as a valid one. Each merged tool names the file that declares it, and each declaration that did not enter the catalog on its own becomes a `SourceSurfaceOmission` — an `adapter_parse` row in the #403 exclusion ledger, accounted for by the source warning that reports it. Two defects found alongside it: - A `codex_config` row over *any* config naming an MCP server aborted the scan before reaching the above: the loader returned one file-level tool source holding tools stamped per server, so every tool was reported as belonging to a source other than the one it was read from. It returns one source per server. Broken since #191; no test scanned such a workspace end to end. - Two `tool_sources` entries reading one server from two files is the one duplicate no reader can reconcile — neither entry's declarations speak for both — and it was reported as a repeated manifest entry, which it is not. A third `details.cause`, `duplicate_across_artifacts`, names both files and the two repairs that exist. Documented in `docs/errors.json`. No verdict, count, or version moves: `report_schema_version`, `contract_version` and every published schema document are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pengfei-threemoonslab
left a comment
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
Blocking review findings from an independent correctness, test, and contract review of commit a4fdb75e.
I reproduced eight actionable gaps. The highest-impact cases are fail-open or no-result paths: the public mcp audit flow does not use the new reconciliation, structured plugin/server IDs can collide and silently collapse distinct capabilities, and the merge can turn absent or malformed security evidence into clean, pass-eligible semantics. The inline comments include exact reproductions and expected fixes.
One additional contract issue is outside the changed lines: STABILITY.md:66-75 still enumerates the duplicate-error details fields and routes consumers on details.cause, but it does not document the new duplicate_across_artifacts cause or other_source_file. Because STABILITY.md declares itself authoritative, please update that migration note alongside docs/errors.json.
GitHub currently reports the branch as conflicting with main (the textual conflict is in CHANGELOG.md).
Validation performed on a clean detached checkout: the focused MCP/source/ledger tests passed; Ruff, compileall, and generated-schema checks passed; GitHub CI is green. The local Agents Shipgate verifier also returned control.state=complete, merge_verdict=mergeable, and release_decision=passed; these review findings are uncovered edge paths that the existing static gate and tests do not exercise.
| servers: list[NormalizedMcpServer], | ||
| ) -> LoadedToolSource: | ||
| warnings = [ | ||
| def _loaded_sources_from_servers( |
There was a problem hiding this comment.
[P1] Apply reconciliation to the public mcp audit flow
This helper is used by the scan loader, but src/agents_shipgate/cli/mcp.py:282-350 still appends per-file tools directly. I reproduced both failure modes: two changed files declaring github.search raise ConfigError: Duplicate capability ids are not allowed before any mcp_audit_v1 result; with disjoint tools, a server-wide approve in one file never reaches a destructive purge from the other, so the audit returns no violations while this reconciler correctly produces MCP-AUTO-APPROVE-SIDE-EFFECT. Route both base and head audit servers through the same reconciliation before fact construction and risk scoring.
|
|
||
| grouped: dict[tuple[str, str], list[NormalizedMcpServer]] = {} | ||
| for server in servers: | ||
| grouped.setdefault((server.source_type, server.source_id), []).append(server) |
There was a problem hiding this comment.
[P1] Group on a collision-free structured server identity
The colon-concatenated source_id is not injective for plugin/server pairs. plugins."a:b".mcp_servers.c and plugins.a.mcp_servers."b:c" both mint codex_plugin_config_mcp:a:b:c. With identical raw blocks, the digest shortcut treats the distinct server names as identical duplicates and returns one source with no warning, silently dropping one (server, tool) capability. Include the structured components in the grouping key and use a collision-free encoding for the published ID; add a quoted-TOML-name regression.
| # not statically known — which is also what caps the tool's extraction | ||
| # confidence in ``_tool_from_normalized``. Keeping one of the two schemas | ||
| # and calling it the surface is the fail-open reading. | ||
| unknown_schema = ( |
There was a problem hiding this comment.
[P2] Downgrade output-schema conflicts too
output_agrees is computed and a disagreement later clears output_schema, but it is not part of unknown_schema. Equal input schemas plus conflicting output schemas therefore produce output_schema={}, mcp_unknown_schema=False, and extraction_confidence="high". That contradicts the stated rule that a schema disagreement leaves the interface unknown. Include not output_agrees in the confidence decision, or emit an equivalent explicit evidence gap, and add an output-only disagreement test.
| } | ||
| ) | ||
| ), | ||
| source_ref=primary_server.source_ref, |
There was a problem hiding this comment.
[P2] Preserve the donor provenance of merged security evidence
The merged annotations/auth may come solely from a secondary declaration, but the result is always stamped with the first declaration path. In a PR-head reproduction, approve, destructiveHint, external access, and a secret name existed only in b/.mcp.json, while the merged tool and its critical findings pointed to a/.mcp.json. Carry per-field or multi-source donor provenance through this merge and make the MCP permission checks cite it; otherwise findings and SARIF send reviewers to a file containing none of the judged evidence.
| ) -> dict[str, Any]: | ||
| """Only what every declaration says, plus every risk claim any of them makes.""" | ||
|
|
||
| first, *rest = annotation_sets |
There was a problem hiding this comment.
[P1] Do not erase parser-invalid semantic annotations
The intersection drops a non-unanimous malformed annotation, and the risk recovery below restores only exact True. Thus destructiveHint: "yes" in one declaration plus omission in another loses the expected invalid_semantic_annotation. With unanimous permission_class: write, valid input schema, and auth.mode: none, I reproduced the merged tool becoming pass_eligible=True; the invalid declaration should prevent that. Preserve invalid recognized values or carry their validation issue through reconciliation, and add malformed-boolean cases.
| ) | ||
| if unreadable is not _MISSING: | ||
| return unreadable, explicit | ||
| mappings = [raw for raw in raws if isinstance(raw, dict)] |
There was a problem hiding this comment.
[P1] Keep absent and parser-invalid auth as uncertainty
Filtering absent auth before unanimity lets the lone mapping win: no auth in one file plus {mode: none} in another becomes structural/high-confidence unauthenticated authority; scoped OAuth is promoted the same way. Lines 801-807 also discard non-string scope members, so ["repo:read"] plus [123] becomes a clean scoped declaration instead of retaining auth.scopes must contain non-blank strings. Since none and scoped are pass-eligible, validate every explicit raw auth and treat absence as disagreement before rebuilding/unioning it.
| ), | ||
| warning=warning, | ||
| ) | ||
| for server in group[1:] |
There was a problem hiding this comment.
[P2] Keep identical copies out of the omission ledger
For three files where A and B are identical and C differs, this loop emits omissions for both B and C. The detail for B falsely says it disagrees with A, and the report gains an adapter_parse exclusion even though the B declaration is fully represented. Filter secondary declarations by digest, or partition the group by digest, before constructing warning paths and omissions; add a mixed three-declaration regression so the advertised identical-declarations-are-silent rule remains true.
Only `CHANGELOG.md` conflicted: both sides prepend a bullet to `## Unreleased`. Both are kept, newest first — this branch's MCP duplicate-declaration entry above #438's `Root agent:` entry. `tests/test_adopter_vocabulary.py` auto-merged: #438 strengthened the sample markdown sweep to un-escape each line before checking it, and this branch adds one probe for `duplicate_mcp_server_declaration_warning`. The two touch different parts of the file, and the strengthened sweep covers the new source warning as written. Full CI-equivalent run green on the merge: `pytest -n auto -m "not perf"`, `tests/test_latency_budget.py -m perf`, `tests/test_adapter_static_only.py`, `ruff check .`, `compileall -q src tests`, and `scripts/generate_schemas.py --check`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Summary
codex_configrow over a workspace where two packages each carry a.mcp.jsonnaminggithubabortedrun_scanwith'pkg_b/.mcp.json' was read twice as one tool source [...] Remove the repeated shipgate.yaml entry naming 'pkg_b/.mcp.json'— false on both counts, and naming an edit nobody could make: the manifest namespath: .once, and that file was read once.(server, tool)and by nothing else, so that moving a.mcp.jsonis not a capability change (mcp auditpins this); and one identity may be observed only once. This PR answers what the collision means: the two files declare one capability twice, and the reader reconciles them before the catalog sees them.tests/test_mcp_manifest.py::test_the_minted_server_id_stays_free_of_the_path_it_was_read_fromkeeps it rejected, next to themcp auditrename test it would have broken.The rule
Identical declarations are silent. Nothing was dropped, and a source warning is a gating input (
evidence_below_ie_threshold); the ordinary monorepo layout where each package vendors the same.mcp.jsongains no evidence gap. Sameness is decided on the declaration as written (NormalizedMcpServer.declaration_digest), not on the fields that reach a catalog row — so two files agreeing on every tool and disagreeing only on thecommandthat serves them are not identical.commandnever reaches a catalog row, so comparing normalized fields would have dropped the second file in silence.Disagreeing declarations are merged conservatively, never by picking one. The merged server carries the union of the tools; a claim that raises risk survives from any one declaration, and a reassuring claim survives only when every declaration makes it.
destructiveHint,openWorldHint,long_running, an unenumeratedserver.*remainder,mcp_unknown_schema— kept if any declaration makes the claim (_RISK_MONOTONE_ANNOTATIONS)_RISK_UNION_ANNOTATIONS)approvewins, and reaches a tool whose own file said nothing about approval — the file that did was describing the same serverreadOnlyHint,mcp_local_documentation,mcp_transport— dropped unless unanimousmedium)What gets recorded. Each declaration that did not enter the catalog on its own becomes a
SourceSurfaceOmission→ anadapter_parserow in the #403 exclusion ledger,accounted_bythe source warning that reports it. Each merged tool names the file that declares it — the merged server keeps the first file's provenance, a tool only the second file declares does not.What is not laundered. An
auththat is not an object, or ascopesthat is not a list, is returned untouched rather than rebuilt through the union, soinvalid_annotationsstill reports it.Two defects found alongside it
codex_configrow over any config naming an MCP server aborted the scan, before the above was reachable: the loader returned one file-levelLoadedToolSourceholding tools stamped per server, so every tool was reported as belonging to a source other than the one it was read from. It now returns one source per minted server id. Broken since Add MCP governance checks and CLI support #191 — no test scanned such a workspace end to end.tool_sourcesentries reading one server from two files is the one duplicate no reader can reconcile (neither entry's declarations speak for both, and merging would drop one entry's declared authority). It was reported as a repeated manifest entry, which it is not. A thirddetails.cause,duplicate_across_artifacts, names both files and the two repairs that exist.Type
Verification
CI is authoritative for
python -m ruff check .,python -m compileall -q src tests, andpython -m pytest.Additional local checks run:
pytest -n auto -m "not perf" --ignore=tests/test_adapter_static_only.py, thentests/test_latency_budget.py -m perfandtests/test_adapter_static_only.pyseparately — all three exit 0.python -m ruff check src testsandpython scripts/generate_schemas.py --check— both clean../shipgate scan, before (aborts) and after (onesearchrow, no warnings), plus the disagreeing variant (union of tools, one source warning, one ledger row,readOnlyHintdropped).Release-readiness notes
docs/checks.md— n/a, no check IDs added or changedSTABILITY.md— no schema change:report_schema_version,contract_versionand every published schema document are unchanged. The two new values within existing surfaces (details.cause = duplicate_across_artifacts,SourceSurfaceOmission.reason = duplicate_server_declaration) are additive; the first is documented indocs/errors.json, the second renders throughEXCLUSION_REASON_PHRASES.Surface discipline
No new surface under
CONTRIBUTING.md§ Surface discipline: no CLI command or sub-app, no schema version, no report orverifier.jsonblock, no agent-discovery surface, no framework adapter. This is a defect fix on the existingcodex_configadapter. Headline metric: time-to-first-verdict — before this, acodex_configrow over any workspace declaring an MCP server produced no verdict at all.Note on
LoadedToolSource.source_idshapecodex_configresults are now minted per server (mcp_json:github) rather than per file (mcp_json:.mcp.json). This id is adapter-minted, matches no configured row either way, and is not a foreign key intotool_sources;configured_source_idis still recorded by the dispatcher from the row that produced the call. Stale comments naming the old shape are updated incore/domain.py,cli/scan/source_loading.py, andtests/test_source_authority.py.🤖 Generated with Claude Code