fix(provenance): enforce tool_catalog.tool_count

[altrudev]

Current restack

Restacked onto current main at 98aae176aa9ae4053d944624dad7629c76789ff7.

Exact head: 251c8b6efe1cd722d166ed734c83c47a9a77d583.

The branch is one commit ahead / zero behind and changes exactly the same two files. Fresh exact-head CI and CodeQL both pass. The previous @lywinged approval was on pre-restack head 7b4ee0ba103ec369fb3d1bd8056b29964190f6e0 and was correctly dismissed by GitHub after the rewrite; no exact-head approval is claimed.

What

Closes #256.

The server-provenance format marks tool_catalog.tool_count as required, but the reference consumer never made it load-bearing. A correctly signed record could carry the right catalog hash for two tools and tool_count: 999; both verify_record() and check_tool_catalog() still succeeded.

This change adds one shared _tool_count() boundary and uses it from both consumer entry points:

• verify_record() now requires a non-negative JSON integer and excludes bool explicitly;
• check_tool_catalog() validates the same field even when called independently;
• after the live hash matches, check_tool_catalog() checks that the signed count equals the number of offered tools.

Error semantics

The existing distinction is preserved.

• If the live catalog hash differs, ToolCatalogMismatch still means the signed paper may be fine and the server in front of the verifier is different.
• If the hash matches but the signed count disagrees with the matching live list, the record contradicts its own catalog metadata. That is a plain ProvenanceError, not ToolCatalogMismatch.

Regression coverage

Focused tests cover:

• missing count;
• negative count;
• boolean count;
• string count;
• null count;
• floating-point count, pinning the JSON-integer boundary;
• independent check_tool_catalog() validation of malformed counts;
• hash mismatch outranking malformed-count validation;
• correct hash with wrong positive count;
• correct hash and count control;
• ordinary live hash mismatch preserving ToolCatalogMismatch.

The wrong-positive-count test deliberately verifies the paper first, then proves the live catalog check is the step that makes the semantic count disagreement load-bearing.

Scope

No wire-format or schema change is made here. This PR does change the reference consumer's live-catalog semantics by making the signed count load-bearing after a matching hash. The current spec/server-provenance-v1.md §5 steps do not explicitly require that live-count comparison, so whether the normative text should be aligned is left to the maintainers/spec process rather than claimed as settled by this implementation PR.

AI-assistance disclosure: ChatGPT assisted with source triage, adversarial-case design, implementation drafting, restack preparation, and exact-diff review. altrudev reviewed the bounded claim and remains responsible for the contribution.

[github-actions]

🟡 Contributor Check: MEDIUM

Check	Result
Profile	MEDIUM
Credential	LOW
Overall	MEDIUM

Automated check by AgenTrust Contributor Check.

[lywinged]

Reproduced at 7cda000. Four gates green: ruff check src tests scripts,
tools/check_dashes.py, mypy src/agentrust_trace (Success: no issues found in 10 source files), pytest at 1023 passed, 1 skipped.

The reorder in 7cda000 is right, and it is cheaper than it looks. I ran both orderings over
twenty tool_count values crossed with three offer sets and diffed the sixty cells. Thirty
differ, and they are exactly the fifteen malformed counts crossed with the two offer sets whose
hash does not match. All thirty are refusals under both orderings; only the class moves,
ToolCatalogMismatch after 7cda000 against ProvenanceError before it. Nothing in the grid
moves between accepted and refused, and on every well-formed count, including a wrong one like
999, the two orderings are identical down to the message text. Your Error semantics section does
what it says.

Three coverage gaps. All three came from running a fix a reviewer would plausibly have written
instead of running the revert.

The reorder your second commit makes is not pinned by the new file. Reverting 7cda000 and
changing nothing else leaves tests/test_provenance_tool_count.py entirely green, 8 passed. The
whole suite catches it with exactly one failure,
test_check_tool_catalog_null_tool_catalog_still_behaves_like_absent in tests/test_provenance.py,
which reaches the ordering only because a null catalog makes the count absent as a side effect.
test_hash_mismatch_remains_a_tool_catalog_mismatch builds with _signed_with_count(len(TOOLS)),
so _tool_count returns cleanly and it passes under either ordering.

The second bullet in your What section is not pinned by anything. Substituting the raw read for
the guard in check_tool_catalog:

declared_count = catalog.get("tool_count")

leaves the whole suite green at 1023 passed, 1 skipped. Under it, tool_count: 2.0 against a
matching two-tool catalog is accepted in silence, because 2.0 != 2 is false in Python. The guard
is in the code; nothing tests that check_tool_catalog applies it when called on its own, which is
the property that bullet claims.

The int half of _tool_count is not pinned against a float. Widening the check to

if isinstance(value, bool) or not isinstance(value, (int, float)) or value < 0:

also leaves the whole suite green at 1023 passed, 1 skipped. Under it, tool_count: 2.0 passes
verify_record and then passes check_tool_catalog against a two-tool catalog, again because
2.0 != 2 is false. JSON has one number type, so a producer emitting 2.0 is not exotic, and
7cda000 as it stands refuses it correctly. Your bad_count list is [_MISSING, -1, True, "2", None], which pins bool, str, None and the negative, but nothing pins int against float.

Two tests against your existing helper close all three:

@pytest.mark.parametrize("bad_count", [2.0, "2", True, -1, None])
def test_check_tool_catalog_alone_refuses_malformed_count(bad_count) -> None:
    record, _ = _signed_with_count(bad_count)
    with pytest.raises(ProvenanceError, match="tool_catalog.tool_count must be"):
        check_tool_catalog(record, TOOLS)


def test_hash_mismatch_outranks_a_malformed_count() -> None:
    record, _ = _signed_with_count("2")
    with pytest.raises(ToolCatalogMismatch):
        check_tool_catalog(record, [TOOLS[0]])

All six pass on 7cda000 as it stands. Against the raw-read variant the parametrized one fails
five of five; against the revert the second one is the single failure; against the float-widened
check the 2.0 parameter is the single failure. The 2.0 entry is carrying two of those on its
own, so it is worth keeping even though it looks like the odd one in the list. Appended to your file as
written, they pass ruff, check_dashes and mypy, and take the suite to 1029 passed, 1 skipped.

One for a maintainer rather than for you. check_tool_catalog now refuses a record whose signed
count disagrees with a matching live catalog. spec/server-provenance-v1.md §5 has six steps, none
of which compares a declared count against the live catalog, and §7 defines consumer conformance as
steps 1, 2, 5 and 6. So the reference consumer refuses records a §5-conformant consumer accepts.
#256 argues for that and it may well be right, but "no normative specification change" does not
quite reach it, and it is not mine to settle.

Add the two tests and I am glad to approve.

Tool-assisted: the grid, the mutants and this write-up.

[altrudev]

Thanks — added the two mutation-pinning tests you suggested at 7b4ee0b:

• standalone check_tool_catalog() rejects malformed counts, including 2.0;
• live hash mismatch outranks malformed-count validation.

I also tightened the Scope section so it no longer claims the live-count comparison is already normatively settled by §5; the implementation/spec alignment is left explicitly to the maintainers/spec process.

The fork-triggered CI/CodeQL runs for the new head are currently waiting on workflow approval.

[imran-siddique]

Batch response for this cluster is here: agentrust-io/agent-manifest#357 (comment)

Short version: the finding class is real and welcome. Your CI had never run, held under first-time-contributor gating, until I released 36 runs across your PRs an hour ago, and five of your eight are now red. Please fix those, sequence trace-spec#258 against #252 which touch the same two files, and tell me the order you want them reviewed in.

[lywinged]

Verified at 7b4ee0b. Both tests landed as proposed and now hold what the review found: the
raw-read substitution in check_tool_catalog fails five of five parameters, the reorder revert
fails inside this file rather than by accident elsewhere, and the float widening dies on the 2.0
parameter. Four gates green, 1029 passed, 1 skipped. The Scope wording now matches what the
change does. Approving.

Tool-assisted: the re-run and this note.

[altrudev]

@imran-siddique #257 is the only lane I am advancing now.

It is cleanly restacked onto current main (98aae176aa9ae4053d944624dad7629c76789ff7) at exact head 251c8b6efe1cd722d166ed734c83c47a9a77d583: one commit ahead / zero behind, exactly two files, with the previously reviewed semantic diff preserved over the current provenance code including #252/#258. Fresh exact-head CI and CodeQL both pass.

@lywinged's prior approval was on pre-restack head 7b4ee0ba103ec369fb3d1bd8056b29964190f6e0 and GitHub correctly dismissed it after the rewrite, so I am not claiming exact-head approval from that review.

The remaining maintainer question is the one Louie identified: whether the live tool_count comparison belongs in the reference consumer given the current §5/§7 wording. The PR body now states that scope explicitly rather than treating it as normatively settled.

[lywinged]

Approved, on 251c8b6e.

The restack checks out in the stronger form. Against 98aae176 the branch is one ahead and none behind, and it touches exactly the two files it touched before. tests/test_provenance_tool_count.py is byte-identical to the previous head, 7b4ee0ba. provenance.py is not, but that is the base moving under it: with the context lines stripped, the pull request's own patch on the old base and on the new base is the same fifteen lines. The blob differs only because #258 landed in the same file, between the hunks.

The ordering is the one from my earlier comment. _tool_count runs after the hash comparison, so a record with tool_catalog: null still reports ToolCatalogMismatch rather than a type error, and the older control test that pins that still passes.

What I ran on this head: the suite is 1167 passed 1 skipped, and the new test file is 14 passed. Then three mutations, since a guard is only worth what fails when it is gone:

• remove _tool_count(catalog) from verify_record: five tests fail, one per malformed shape in the parametrize list;
• remove only the declared_count != len(tools) raise from check_tool_catalog, keeping the assignment: exactly one fails, test_wrong_positive_count_is_detected_when_live_catalog_is_checked, with DID NOT RAISE;
• drop the isinstance(value, bool) exclusion, the simplification a reviewer is most likely to propose since bool already passes isinstance(value, int): two fail, both the True case, one per entry point.

The first and third share the verify_record True case, which is expected: removing the call and weakening the check both let True through. What matters is that each of the three has at least one test only it turns red: four for the first, the wrong-positive-count test for the second, and test_check_tool_catalog_alone_refuses_malformed_count[True] for the third. None of the three checks is covered by a neighbour.

One thing for the maintainers rather than for this review, since it is the point the body now leaves open. Section 3 calls tool_count redundant with the hash by design; section 5 step 5 compares the hash against the tools the server actually offers, and section 7 defines the hash over those tools. Neither mentions the count. So the comparison this PR makes after a hash match, count against the offered list, is behaviour this implementation adds, neither required nor ruled out by the format. Whether the reference consumer should carry it is a spec question, and approving the code does not settle it either way. The body says so now, which is the right place for it.

On the dismissed approval: correct not to carry it across a rewrite, and the checks above are why this one stands on the new head.

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

[imran-siddique]

Read the whole diff at 251c8b6. This is right.

tool_count was required by the format and read by nothing, so a correctly signed record could carry the right catalog hash for two tools and declare 999, and both entry points passed it. Making the field load-bearing is the fix.

Three details I checked specifically, because they are the ones that usually go wrong:

• _tool_count rejects bool explicitly. isinstance(True, int) is true in Python, so without that clause tool_count: true would have counted as 1.
• 2.0 is rejected as well, since isinstance(2.0, int) is false. A JSON producer emitting a float where an integer is required is exactly the case a count check should catch.
• In check_tool_catalog the count comparison runs after the hash check, so a live hash mismatch outranks a malformed count. That ordering is not incidental and you pinned it with its own test. It is the same principle I ruled on today in cmcp#596 and trace-spec#242: establish the thing before interpreting it, and never report a downstream verdict about a structure you have not confirmed.

Thanks for calling the dismissed approval yourself rather than leaving it to look like it stood. A restack dismissing an approval is silent, and most people do not mention it.

Approving to release the hold. Merging.