fix(catalog): make the schema the structural authority for approval records (#533) - #546
Conversation
…ecords schemas/catalog-approval.schema.json shipped in the repository and nowhere else. verify_catalog_change reimplemented structural validation by hand, the two drifted, and the file was absent from the wheel force-include, so an installed distribution did not carry it at all. agentrust-io#531 could only document one divergence, a negative approved_at that the schema rejects and the verifier accepted, as a strict xfail. The record is now validated against the schema before any other check, and verification refuses outright when the schema is missing from the installation, following loader.py rather than falling back to hand-written checks. Duplicated checks are deleted rather than left unreachable: field presence and unknown members on the record, the policy and each approval, digest shapes, integer and boolean types, string emptiness, and the signature alphabet. _require_str and _require_int go with them. What stays is what JSON Schema cannot express: * the runtime catalog hash binding and the caller's chain checkpoints * the policy pin, including that policy_hash covers its own body * reviewer identity, role, revocation and key reuse * the validity interval ordering * the signatures The xfail is now a plain assertion, since the case fails closed. previous_record_hash on the first record in a chain is the all-zero digest, sha256: followed by 64 zeros, written into the schema and the spec doc. It validated against the pattern before and meant nothing. Packaging: the schema is force-included beside catalog-entry.schema.json, and verify_python_distribution.py checks both resolve to a file inside the installed distribution, so a wheel that drops one fails the release smoke test rather than a verifier at runtime. A unit test asserts the force-include mapping, since that line is the only thing putting these files next to the code. loader.py's _CATALOG_ENTRY_SCHEMA_PATH is now CATALOG_ENTRY_SCHEMA_PATH, since the distribution check imports it. Verified by mutation: dropping the schema call fails 7 tests, accepting a missing schema fails 1, dropping the 64-byte signature check fails 1. Coverage of approval.py is 149 statements with none missing. Signed-off-by: Mohammed Zoheb Shaik <zoheb.shaik7@gmail.com>
|
🟡 Contributor Check: MEDIUM
Automated check by AgenTrust Contributor Check. |
github-actions
Bot
added
the
needs-review:MEDIUM
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Two things I described in the PR as reasoned rather than tested. Both are now built and run locally, so here is the evidence. The schema is in the built wheel
The sdist carries the whole directory, so a wheel built from the sdist force-includes the same two. Built that path as well, since the release job installs both artifacts: Verification then works from the packaged copy rather than a checkout, which is the property that matters: The guard fires when force-include regressesRebuilt the wheel with the one So the release job fails on the artifact, and a verifier that somehow ran against it refuses rather than falling back to hand-written checks. No CI steps are added by any of this: the mapping assertion is a unit test in the existing matrix, and the artifact assertion is nine lines inside |
imran-siddique
left a comment
imran-siddique
left a comment
There was a problem hiding this comment.
Verified before approving.
The schema is now load-bearing rather than decorative. verify_catalog_change validates against schemas/catalog-approval.schema.json before any other check and refuses to verify when it is missing from the installation, which matches how loader.py already treats catalog-entry.schema.json. That is the pattern #533 asked for, and it means the two validators can no longer drift because there is only one.
The deletions are the part that makes it real. Field presence, unknown members, digest shapes, integer and boolean types, string emptiness and the signature alphabet are gone from code rather than left unreachable behind the schema check. What stays is what JSON Schema cannot express: the runtime hash binding, the policy pin, reviewer identity and key rules, the interval ordering, and the signatures. That split is the right one.
The negative approved_at xfail is closed properly. #531 could only document that divergence because the verifier accepted -1 while the schema rejected it. It now fails closed, and the strict xfail is removed rather than relaxed.
The packaging half is the bit most easily missed. The force-include plus the verify_python_distribution.py check means a wheel that drops either schema fails the release smoke test instead of failing a verifier at a customer's runtime. The unit test asserting the force-include mapping is worth having, since that line is the only thing keeping the schemas next to the code. Thanks for building and checking the wheel rather than leaving it as reasoning.
Non-blocking: this and #547 overlap completely. Handling that separately on #547.
4 participants
Closes half of #533, the schema half. The clock half is not here; see Not covered.
Why
schemas/catalog-approval.schema.jsonwas added by #519 and never loaded.verify_catalog_changereimplemented structural validation by hand, the two drifted, and the file was absent from[tool.hatch.build.targets.wheel.force-include], so an installed wheel did not carry it at all. #531 could only document one of the divergences it found, a negativeapproved_atthat the schema rejects and the verifier accepted, as a strict xfail.What
The schema is the structural authority. The record is validated against it before any other check, and verification refuses outright when the schema is missing from the installation, following
loader.pyrather than quietly falling back to hand-written checks. Validation errors carry the failing path:schema violation at approvals/0/expires_at: True is not of type 'integer'.The duplicated checks are deleted, not left unreachable. Field presence and unknown members on the record, the policy and each approval; digest shapes; integer and boolean types; string emptiness; the signature alphabet.
_require_strand_require_intgo with them. What stays in code is what JSON Schema cannot express:policy_hashcovers its own bodyThe xfail is now a plain assertion. The negative
approved_atcase fails closed, which is what the xfail was written for.Genesis is specified.
previous_record_hashon the first record in a chain is the all-zero digest,sha256:followed by 64 zeros, in both the schema and the spec doc. It validated against the digest pattern before and meant nothing, which is the combination worth fixing: representable and unspecified.Packaging. The schema is force-included beside
catalog-entry.schema.json.scripts/verify_python_distribution.pynow checks that both resolve to a file inside the installed distribution, so a wheel that drops one fails the release smoke test rather than a verifier at runtime, and a unit test asserts the force-include mapping, since that line is the only thing putting these files next to the code.loader.py's_CATALOG_ENTRY_SCHEMA_PATHbecomesCATALOG_ENTRY_SCHEMA_PATH, since the distribution check imports it.Tests
New: a genesis record validated against the shipped schema, verification refusing when the schema is absent, a corrupt schema failing closed, a malformed
expected_policy_hashfrom the caller, and the force-include assertion. Message assertions in the existing tests now name the schema path, since the schema owns those rejections.Verified by mutation:
Decisions worth reviewing
Deleting rather than keeping the hand-written checks. Keeping both means two sources of truth and a set of branches no test can reach, which is how the drift in #531 happened. The cost is that the errors for malformed records now come from
jsonschemaand read differently, which is why the test assertions changed.audit-entry.schema.jsonandtrace-claim.schema.jsonare in the same position and are deliberately not touched here, per your note on #533. The distribution check is written so adding them later is one line each.Not covered
The validity clock, the other half of #533. It needs one question answered first:
TrustedReviewercarries no validity window, so "check key validity at an instant inside the approval window" has nothing to check against unless the dataclass growsnot_beforeandnot_after. Asked on #533; the PR follows the answer.