Skip to content

fix(provenance,validate): remove str() coercion on digest/hash fields in provenance and enforce strict JSON in schema loaders (#627) - #632

Merged
lywinged merged 1 commit into
agentrust-io:mainfrom
drk-ram:drk-ram-fix-provenance-validate-627
Oct 30, 2026
Merged

lywinged merged 1 commit into
agentrust-io:mainfrom
drk-ram:drk-ram-fix-provenance-validate-627

Conversation

@drk-ram

@drk-ram drk-ram commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

What this changes

Closes #627.

  1. src/agentrust_trace/provenance.py (_check_structure, verify_record):
    • Replace str(...) coercion before _DIGEST_RE.match(...) on artifact.digest, endpoint.spki_sha256, and tool_catalog.hash with isinstance(value, str) and _DIGEST_RE.match(value).
  2. src/agentrust_trace/validate.py (_schema, profiles_with_schema):
    • Parse packaged JSON schemas via _loads_schema with object_pairs_hook (rejecting duplicate keys) and parse_constant (rejecting NaN/Infinity).

Type of change

  • Editorial (typo, link fix, clarification: no normative effect)
  • Non-breaking spec change (new optional field, new platform profile, informative addition)
  • Breaking spec change (requires 14-day comment period and Project Lead sign-off)
  • Schema change
  • Example addition

Spec section

spec/server-provenance-v1.md section 3 and spec/trace-v0.2.md section 3.

Tests for a fix

  • tests.test_provenance_locator_types::test_provenance_rejects_str_coercible_non_string_digests_and_hashes
  • tests.test_provenance_locator_types::test_validate_loads_schema_rejects_duplicate_keys_and_nonfinite_constants
  • Validation:
    • python -m pytest tests/test_provenance_locator_types.py tests/test_provenance.py tests/test_validate.py -q -> 263 passed
    • python tools/review_bar.py --base origin/main --head HEAD -> 2 fail on behaviour, 0 fail on a missing name or file

Changelog entry

Checklist

  • DCO sign-off on all commits (git commit -s)
  • CHANGELOG.md updated (for any normative change)
  • Breaking changes marked with <!-- CHANGED: #NNN: description --> in spec text
  • Backward compatibility statement included (for breaking changes)

@drk-ram
drk-ram requested review from a team, lywinged and rajnisht7 as code owners October 25, 2026 05:29
@github-actions

github-actions Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

🟡 Contributor Check: MEDIUM

Check Result
Profile MEDIUM
Credential LOW
Overall MEDIUM

Automated check by AgenTrust Contributor Check.

@rajnisht7 rajnisht7 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.

the overall changes looks good, there is one point needs to be addressed:
check_tool_catalog() still missing the strict tool_catalog.hash validation. issue #627 explicitly called this out, the proposed fix said the isinstance(..., str) + digest check should apply in both verify_record and check_tool_catalog, whereas this PR only did the first one
Current path still does:

expected = catalog.get("hash")
if actual != expected:
    raise ToolCatalogMismatch(...)

so a malformed/non-string hash ends up as ToolCatalogMismatch instead of ProvenanceError. which breaks the intended distinction (document malformed vs live catalog mismatch), and check_tool_catalog is documented as independently callable before verify_record, kindly add the same guard before the comparison (and ideally before computing actual), plus a regression test that hits check_tool_catalog with a non-string / malformed hash and asserts ProvenanceError.

@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. Reviewed at 75e2f87 on da69b31.

One thing is missing that CONTRIBUTING's "Before a pull request is reviewed" asks for: a test for each rejection the change adds, failing when that rejection is switched off. The schema must be a JSON object check in _loads_schema has none: deleting it leaves all 4788 tests passing. I will review the rest once it is in.

CI at 75e2f87 failed in the portability step for a reason this pull request did not cause: main failed the same step at da69b31, before #624, and passes at dcca4e0.


Generated by Claude Code

@drk-ram

drk-ram commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor Author

Thank you @rajnisht7 and @lywinged! Addressed both points in 64df3cd (rebased onto origin/main at e9ab302):

  1. check_tool_catalog strict tool_catalog.hash validation (isinstance(expected, str) and _DIGEST_RE.match(expected)):
    • Validated catalog.get("hash") when "hash" in catalog in check_tool_catalog before comparing actual != expected, so a present non-string or malformed tool_catalog.hash raises ProvenanceError (and not the ToolCatalogMismatch subclass) while preserving test_check_tool_catalog_null_tool_catalog_still_behaves_like_absent, with regression assertions (type(excinfo_cat.value) is ProvenanceError) in test_provenance_rejects_str_coercible_non_string_digests_and_hashes.
  2. Test for schema must be a JSON object in _loads_schema:
    • Added "[1, 2, 3]" to test_validate_loads_schema_rejects_duplicate_keys_and_nonfinite_constants, which fails when if not isinstance(parsed, dict): raise ValueError("schema must be a JSON object") is removed.

… and harden _loads_schema (agentrust-io#627)

Require isinstance(..., str) and _DIGEST_RE.match(...) on artifact.digest,
endpoint.spki_sha256, and tool_catalog.hash in verify_record and
check_tool_catalog in src/agentrust_trace/provenance.py, and reject
duplicate JSON keys, non-finite constants, and non-object roots in
_loads_schema in src/agentrust_trace/validate.py.

Closes agentrust-io#627

Signed-off-by: Raghuvar Karthik Durga <177662415+drk-ram@users.noreply.github.com>
@drk-ram
drk-ram force-pushed the drk-ram-fix-provenance-validate-627 branch 2 times, most recently from 11f2320 to 64df3cd Compare October 29, 2026 15:14

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

Approve. Reviewed at 64df3cd on e9ab302.

Both points from the first round are in. The schema must be a JSON object check now has the "[1, 2, 3]" case, and deleting the check fails the test. check_tool_catalog answers a tool_catalog.hash of "not-a-digest", 5, uppercase hex or null with ProvenanceError, and an absent hash still reads as ToolCatalogMismatch; the new test checks the class and the message for its own three inputs. With that, Closes #627 closes the issue with its check_tool_catalog point met.

What I checked rather than read:

  • CI's commands, run here on 3.11 and 3.12: ruff, the dash check and mypy clean; the suite at 5222 passed and 98 skipped on 3.11 and 5221 and 99 on 3.12; the replay experiment, the two LC_ALL=C PYTHONUTF8=0 runs, the PYTHONOPTIMIZE=2 command and the two --help calls pass. The head is one commit on e9ab302, which is main as I write, so that is also the merged result.
  • The description's two validation lines reproduce: 263 passed for the three test files it lists, and tools/review_bar.py reports 2 fail on behaviour, 0 on a missing name or file and 17 pass or skip.
  • Removed alone, each of the seven rejections the change adds fails a test: the object check fails 375, the duplicate-key hook 131 and the parse_constant hook 304, the schema test plus the tests its leaked cache takes with it, as below; the check_tool_catalog guard, and each of the three isinstance clauses reverted to str(), fail 1. The guard narrowed to isinstance alone, without the digest match, also fails 1.
  • On main, verify_record accepts three signed records whose artifact.digest, endpoint.spki_sha256 and tool_catalog.hash are each an int subclass whose __str__ is a digest. This head refuses all three. verify_record refuses the same JSON values on both commits: null, true, false, 5, 5.5, an array, an object, an uppercase digest and an empty string in each of the three fields raise ProvenanceError, and none of the 691 JSON files in the tree carries spki_sha256, tool_catalog or mcp-server-provenance.
  • Python callers see more than the int subclass. build_record refuses a collections.UserString digest that main accepted at build time and then could not sign. A (str, Enum) digest that main refused now verifies. main compared a (str, Enum) kind by its str(), which is the class and member name rather than the value, so a tee-attested record with attestation: null verified and one with attestation present was refused; here the member reads as its value and both go the other way.
  • The four packaged schemas parse to equal values under json.loads and _loads_schema, which refuses a repeated key at the top level, three objects deep and inside an array, NaN, Infinity, -Infinity, and a top-level array, string or null.

Things I would not hold the merge for:

  • When a case of the schema test stops raising, it takes other tests with it: 374 in 16 files for the object check, 130 in 9 for the duplicate-key hook and 303 in 16 for the parse_constant hook, because the closing cache_clear() calls run only when every case raises, so _schema() keeps the temporary schema. A try/finally around the loop would keep a future failure to one test.
  • kind=record["kind"] reverted to str(record.get("kind")) leaves every test passing.
  • The verify_record cases for tool_catalog.hash are _StrDigest, 123 and "not-a-digest", which main refuses too, the first on canonicalization and the other two on the digest pattern, so the test pins the clause through its message. The input main accepts, the int subclass above, is not among them.
  • The guard sits after tool_catalog_hash has been computed, not before it as today's comment says; nothing turns on the order.
  • The description's changelog bullet does not say that check_tool_catalog now raises ProvenanceError for a malformed or null hash where main raised ToolCatalogMismatch; the merging Maintainer writes the CHANGELOG entry from that bullet.

Generated by Claude Code

@lywinged
lywinged merged commit b541d66 into agentrust-io:main Oct 30, 2026
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-review:MEDIUM Contributor check flagged MEDIUM risk

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(provenance,validate): remove str() coercion on digest/hash fields in provenance and enforce strict JSON in schema loaders

3 participants