Skip to content

fix(provenance): refuse conflicting tool schema aliases - #300

Merged
imran-siddique merged 2 commits into
agentrust-io:mainfrom
altrudev:fix/provenance-dual-input-schema-297
Sep 6, 2026
Merged

fix(provenance): refuse conflicting tool schema aliases#300
imran-siddique merged 2 commits into
agentrust-io:mainfrom
altrudev:fix/provenance-dual-input-schema-297

Conversation

@altrudev

@altrudev altrudev commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Closes #297.

What

tool_catalog_hash() accepts both the SDK spelling input_schema and the MCP wire spelling inputSchema. The compatibility fallback previously preferred input_schema silently when both were present, even when the two values disagreed.

That left the live-catalog check vulnerable to an ambiguous tool object: a server could carry a snake-case schema matching the signed provenance record while also exposing a different camel-case schema.

This change preserves the existing compatibility rule for either spelling and for equal dual-key values, but refuses a tool object when both keys are present with different values.

Regression coverage

Focused tests hold three cases:

Scope

No schema change, wire-format change, canonicalization change, signature change, or tool-count semantic change.

Credit: the issue and proof of concept were reported by @harshnair75567-cloud.

AI-assistance disclosure: ChatGPT assisted with source triage, duplicate/ownership review, implementation drafting, regression design, and exact-diff review. altrudev reviewed the bounded claim and remains responsible for the contribution.

Closes agentrust-io#297.

Signed-off-by: Altru.dev <altrudevelop@gmail.com>
@altrudev
altrudev requested a review from a team as a code owner September 6, 2026 15:38
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

🔴 Contributor Check: HIGH

Check Result
Profile HIGH
Credential LOW
Overall HIGH

Automated check by AgenTrust Contributor Check.

@github-actions github-actions Bot added the needs-review:HIGH Contributor check flagged HIGH risk label Sep 6, 2026

@lywinged lywinged left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes, for one line. The direction is right and the reported vector is closed; the comparison that closes it sits one layer above the bytes the hash covers, and that layer lets a pair through.

For anyone reading this later: tool_catalog is a field of the MCP Server Provenance Record in spec/server-provenance-v1.md, not of the Trust Record, and validate_json refuses a Trust Record that carries it. Section 4 of that specification names the preimage as name, description and input_schema, and says nothing about inputSchema; accepting the MCP wire spelling is this implementation's convenience, and the ambiguity this PR closes is the one that convenience opened. It is reachable in practice: cmcp's proxy (mcp/proxy.py) sends tools/list to the upstream server and hands the returned objects to check_tool_catalog unchanged, so both spellings are the server's to set, while cmcp's catalog loader (catalog/loader.py) resolves inputSchema first and this hash resolves input_schema first.

What I ran on f5f184b. The three new tests copied into a worktree of main: two fail, one passes, which is the right shape. Full suite on this head 1188 passed and 1 skipped against 1185 on main, with ruff, tools/check_dashes.py and mypy clean. Two spellings alone hash identically, so a single-spelling producer sees no change. cmcp itself emits only inputSchema, never both, and no file in the organisation's repositories carries both spellings on one object, so nothing existing trips the new refusal.

The gap. The check is t["input_schema"] != t["inputSchema"], Python equality, and True == 1 and False == 0 in Python while anchor_bytes writes true and 1. On this head, a signed side of {"additionalProperties": 1} with a live side of {"additionalProperties": true} passes the new check and check_tool_catalog accepts it; 0 against false and a nested 1 against true behave the same. Whether that widens anything depends on the validator: Python's jsonschema treats 1 as true and 0 as false, so nothing changes there; a validator that discards an invalid keyword value rather than failing falls back to the default, and for additionalProperties the default is permissive. Narrow, and the function exists for exactly the cross-implementation case.

The fix is one token: compare anchor_bytes(t["input_schema"]) != anchor_bytes(t["inputSchema"]). On a copy of this branch that change leaves the three new tests passing, refuses the 1 and true pair and the 0 and false pair, still accepts a pair that differs only in key order, and the full suite stays at 1188. A fourth test for the 1 and true pair would pin it.

Not this PR, for the maintainers. Section 4 names one spelling and the two repositories resolve the pair in opposite orders; a stated precedence in the specification, or in cmcp, is what closes that for good, which is the second remedy #297 names. And a simpler design is available: refuse any tool object that carries both keys, equal or not, since neither this specification nor MCP's own wire format gives a server a reason to send both.

Tool-assisted: the runs and this write-up.

Signed-off-by: Imran Siddique <imran.siddique@opaque.co>

@imran-siddique imran-siddique left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 427952e. The requested comparison now uses anchor_bytes, the same representation the catalog hashes. Three independent cases demonstrate that Python-equal boolean/integer values, including nested values, were accepted before the repair and are refused afterward. Equivalent object key order still produces the same hash. All 112 focused provenance tests pass; changed-file Ruff and hosted CI pass. No held workflows were found.

This addresses the comparison finding on the earlier head. I pushed the repair, so independent re-review is still required before merge.

@imran-siddique
imran-siddique enabled auto-merge (squash) September 6, 2026 17:01

@lywinged lywinged left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at 427952e. The comparison now runs on the bytes the hash covers, and the new file pins it.

Measured on this head. The three pairs that passed at f5f184b are refused: 1 against true, 0 against false, and a nested 1 against true. A pair that differs only in object key order is still accepted, and a genuinely different pair is still refused, so the guard did not get coarser in the process. Full suite 1192 passed and 1 skipped, with ruff, tools/check_dashes.py and mypy clean.

Both test files are load-bearing, checked by putting the defect back. Restoring != in place of the byte comparison turns all three parametrized cases in test_alias_canonical_comparison.py red and leaves the earlier file green, which is the split you would want: the new file exists for exactly what the old one could not see. Removing the guard entirely turns five red across the two files.

One behaviour did change, and it is a diagnostic rather than a decision. A tool object that carries both aliases, differing values, and a number the anchor profile refuses used to fail with the conflicting-aliases error and now fails with UnanchorableValue naming the number, because the comparison reaches the serializer first. The other three combinations of those inputs that I tried raise what they raised before, and both outcomes are refusals, so nothing is accepted that was refused before.

Against current main, 52687e6, which now carries #239's change to the same module: merges with no conflict, and the merged tree runs 1217 passed and 1 skipped with the three checks clean.

The two notes from the earlier review are unchanged by this and still for the maintainers, not for this PR: a stated precedence for the two spellings, and the simpler design of refusing any object that carries both keys at all.

Tool-assisted: the runs and this write-up.

@imran-siddique
imran-siddique merged commit f24e1c1 into agentrust-io:main Sep 6, 2026
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-review:HIGH Contributor check flagged HIGH risk

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tool_catalog_hash silently prefers input_schema over inputSchema when both are present, letting a decoy schema pass check_tool_catalog

3 participants