fix: nine defects in the reference implementation, found by a package wide sweep by lywinged · Pull Request #236 · agentrust-io/trace-spec

lywinged · 2026-08-28T20:06:50Z

What this changes

Nine defects in the reference implementation, found by sweeping the package rather
than by reading any one function. Each is one commit, so the series can be split,
reordered or partly taken. main goes from 828 passed to 1015 passed, 1 skipped.

#	commit	what was wrong
1	`fix(sign)`	`jwk_thumbprint` and `verify_record` read a member off their argument before establishing its shape, so a string, a number, `None`, a list or a bool raised `AttributeError`. `verify_record`'s docstring says every rejection other than a bad signature is a `ValueError`, and a caller written against that contract does not catch it.
2	`fix`	The same shape in `provenance.verify_record`, `provenance.check_tool_catalog` and `content_marking.verify_assertion`. The third was worse than a crash: it never checked that `record_bytes` were bytes, and `bytes(5)` is five zero bytes, so an int was hashed, failed to match, and the caller was told the record at the URL had changed. That is a specific accusation about somebody else's server, and it was false.
3	`fix(sign)`	The revocation check failed open on a non bool answer. `RevocationStore` is `Callable[[str], bool]` and the return value was read by truthiness, so `None`, `""`, `0` and `[]` all read as "not revoked". `None` is what a CRL, status or SCITT lookup returns when its author handled the 200 and forgot the rest, which is exactly the outage the existing `except` clause was written to survive.
4	`fix`	`docs/quickstart.md` writes an unencrypted Ed25519 private key to `trace-key.pem` under a comment reading "keep secure, never commit or log". The comment was the whole of the enforcement. A reader following the quickstart inside a clone was one `git add -A` from committing their signing key.
5	`fix`	Six public functions raised exceptions no module documents. `key_to_jwk` leaked `AttributeError` on all seventeen probe inputs including the public key, which is the plausible mistake for a function whose name reads as "turn a key into a JWK" and whose result is the public JWK. The sweep that finds them is now committed rather than described.
6	`fix(models)`	The models accepted booleans where JSON says integer and coerced them to 1. `isinstance(True, int)` is a Python fact and not a JSON one: `schema/trace-claim.json` rejects `{"slsa_level": true}` and the models accepted it, so the record was a claim of SLSA build level 1 assembled out of a boolean.
7	`fix(models)`	`TrustRecord.model_validate(record).model_dump()` returned a record `validate_json` rejects and whose signature no longer verifies, because pydantic writes every unset optional as an explicit `null` and the added members change the RFC 8785 canonical bytes. That round trip is what `sign_record`'s own docstring points a caller at.
8	`fix(validate)`	All eight `"format": "uri"` declarations in the schema were inert. `jsonschema` treats `format` as an annotation unless a checker for that format is installed, so `validate_json` built a `FormatChecker` and `"not a uri"` validated.
9	`fix(validate)`	The exported `SCHEMA` was the live object the validator reads. `_schema()` is `lru_cache`d, so mutating the dict handed to downstream tooling silently reconfigured `validate_json`, `iter_errors` and the structural gate inside `verify_record`, process wide, for every later call.

Two things worth knowing before merging

One new runtime dependency, in commit 8. rfc3986-validator, which is what makes
the eight format: uri declarations do anything. jsonschema[format] was not used
on purpose: it pulls rfc3987, which is GPLv3, into an Apache 2.0 dependency tree.

Commits 6 and 8 tighten what validates. Every JSON file in the repository was
walked for embedded records, 77 of them, and each was run through validate_json and
TrustRecord.model_validate on main and on this branch. There are zero acceptance
differences, so nothing in the tree is rejected that was accepted before. The control
for that check: on main measured in its own environment, "not a uri" validates and
slsa_level: true is accepted by the model; on this branch both are refused.

Verification

Every commit is load bearing. Reverting a commit's source change while keeping its
tests turns the suite red: 20, 32, 10, 3, 12, 7, 10 and 3 failures respectively.
Commit 8 changes a dependency declaration rather than code, so reverting the file
proves nothing while the package is still installed. Uninstalling it is the real
mutation: 25 failures.
Suites run on 3.11 and 3.12, matching CI. ruff check src tests scripts,
python tools/check_dashes.py, mypy src/agentrust_trace and
pytest --cov are all clean; coverage is 96%.
Every test file also passes alone, and the per file totals sum to the whole.
Built as sdist and wheel, installed into an empty environment, and commit 9's
property re checked from the installed artifact rather than the source tree.
Merges into sign.verify_record: harden max_age_seconds/max_future_skew_seconds validation to match provenance.py #233 with no code conflict. sign.py auto merges and the merged tree
passes. Only the CHANGELOG.md entry position conflicts, which is a one line resolve.

Type of change

None of the four boxes fit: this is implementation only. No spec text, no schema file
and no wire format changes. pyproject.toml gains the dependency named above.

Spec section

None modified. The authority for each fix is named in its own CHANGELOG entry: commit 3
holds the revocation check to the fail closed rule the function's own docstring states,
and commit 6 restores agreement with schema/trace-claim.json, which already rejects a
boolean where it says integer.

Checklist

DCO sign off on all commits
CHANGELOG.md updated
Breaking changes marked in spec text: not applicable, no spec text changed
Backward compatibility statement: not applicable, no breaking change. The
acceptance comparison above is the evidence.

…the documented error Both read a member off their argument before establishing its shape, so a string, a number, None, a list or a bool raised AttributeError. That is not the ValueError verify_record's docstring names for every rejection other than a bad signature, and a caller written against that contract does not catch it. Neither argument is one the caller has already established. A JWK reaches jwk_thumbprint from a peer, a key document or a record's own cnf. The record handed to verify_record is by definition not yet known to be an object. 828 to 849 passed, 1 skipped. Removing the two guards fails 20 of the 21 added; the twenty-first is the control asserting a valid key and record still pass. Ruff, mypy and check_dashes.py clean. Signed-off-by: Louielunz <48041247+lywinged@users.noreply.github.com>

provenance.verify_record and provenance.check_tool_catalog each leaked AttributeError on 11 of a 12-value junk matrix, which is not the ProvenanceError verify_record documents and is not caught by a caller written against it. The twelfth value is {}: an object, so it reached the documented refusal, which is what the other eleven should have done. content_marking.verify_assertion never checked that record_bytes were bytes, and bytes(5) is five zero bytes, so an int was hashed, failed to match, and the caller was told the record at the URL had changed: a specific and false accusation about somebody else's server. Each now raises the error its own module documents, naming the type received. Signed-off-by: Louielunz <48041247+lywinged@users.noreply.github.com>

`RevocationStore` is `Container[str] | Callable[[str], bool]`, and the callable's return value was read by truthiness. `None`, `""`, `0` and `[]` all read as "not revoked" and let the key through; the string `"no"` read as revoked. Truthiness is not a reading of revocation status in either direction. `None` is the case that matters and it is not hypothetical. It is what a CRL, status or SCITT lookup returns when its author handled the 200 and forgot every other response, which is exactly the outage the existing `except` clause was written to survive. That clause already treats a store that raises as a rejection, on the stated grounds that an unavailable source is not evidence a key is unrevoked. A store answering `None` supplied no more evidence than one that raises, and was being believed. `provenance.verify_record` imports the same function and makes the same claim in its own docstring, so one fix closes both entry points. A callable returning anything other than `True` or `False` is treated as unable to answer and fails closed through the same path and message. The membership branch is untouched: `in` yields a real bool whatever `__contains__` returns, and a test pins that so it does not acquire a guard by accident. Nine revocation tests existed and none returned a non-bool, so the gap sat between a covered raise and a covered `False`. Signed-off-by: Louielunz <48041247+lywinged@users.noreply.github.com>

docs/quickstart.md writes an unencrypted Ed25519 private key to trace-key.pem, under a comment reading "keep secure, never commit or log". That comment was the whole of the enforcement. A reader following the quickstart inside a clone, which is what a quickstart invites, was one `git add -A` away from committing their signing key. The three paths the documentation writes are ignored now, along with *.pem, *.key, *.p8 and *.pfx. The repository tracks no key material today and there is no case for the first one arriving unnoticed. The test recovers the written paths from the documentation rather than listing them, so a doc that starts writing somewhere new fails rather than widening the gap quietly, and asserts no key material is tracked. Its breadth guard needed correcting before it guarded anything. The first version ran `git check-ignore` against tracked files and passed with `*` appended to .gitignore, because ignore rules do not apply to tracked files and check-ignore reports them as not ignored whatever the rules say. It uses --no-index now. Signed-off-by: Louielunz <48041247+lywinged@users.noreply.github.com>

…is committed key_to_jwk leaked AttributeError on all seventeen probe inputs, including the public key, which is the plausible mistake for a function whose name reads as turn a key into a JWK and whose result is the public JWK. load_key leaked fourteen AttributeErrors and a UnicodeEncodeError. sign.sign_record and provenance.sign_record unpacked {**record} before checking it was a mapping. anchor_bytes refused the two classes registry-anchor-v1 section 1 excludes and passed everything else to json.dumps, so bytes came back as a message about a serializer from the function whose stated purpose is to refuse the value by name. intent_bridge is the one worth reading twice. digest_jcs and sign_bridge let rfc8785 errors out as themselves. Those are ValueError subclasses, which satisfies sign's contract but not this module's: a CanonicalizationError is not an IntentBridgeError, so except IntentBridgeError does not catch it. Four of the five tripping values are ordinary JSON. verify_bridge did not leak but misattributed, canonicalizing inside the try that reports the signature invalid. The sweep is committed as a test. It walks the package rather than listing functions, and a coverage test fails until every discovered function is either swept or declared unsweepable. Each entry carries an explicit witness value and the exception it must produce, because a ratio over the junk matrix is a property of the function rather than evidence the call is wired up. Signed-off-by: Louielunz <48041247+lywinged@users.noreply.github.com>

… as a number `isinstance(True, int)` is a Python fact and not a JSON one. JSON Schema's `"type": "integer"` does not match `true`, so schema/trace-claim.json rejects `{"slsa_level": true}`. models.BuildProvenance accepted it and coerced it to 1, so the record became a claim of SLSA build level 1 assembled out of a boolean: valid to this library, invalid to every implementation validating against the published schema. tool_transcript.call_count did the same. appraisal.timestamp read true as 1 January 1970. iat and origin.ingested_at did not have the hole, and were safe by accident rather than by design: their lower bound sits above 1, so the coerced value failed the range check after the coercion had already happened. All five carry the guard now, so the safety does not depend on a bound nobody is thinking about when they change it. models.py already stated the principle this violates, above JCS_SAFE_INTEGER: a model that accepts what the schema rejects sends the failure downstream to whichever canonicalizer the producer happens to be using. Found by mutating every field of a valid record across a 24-value matrix and comparing the two validators, which had never been compared against each other. The differential is committed. Disagreements it does not fix are declared with the reason, and a declared one that has been resolved fails too, so the set cannot go stale in either direction. Signed-off-by: Louielunz <48041247+lywinged@users.noreply.github.com>

…erify_record fails Pydantic serializes every unset optional as an explicit null. The schema types no named field as nullable, and the added members change the RFC 8785 canonical bytes the signature is taken over. So the round trip produced a ValueError from validate_json and an InvalidSignature from verify_record, on a record that was valid and verified a moment earlier. This is the round trip sign_record's own docstring points a caller at: pass the returned dict to TrustRecord.model_validate() to confirm structural validity before writing. A caller who then wrote the model out wrote a broken record, and neither check runs at the moment the damage is done. Absent optionals are omitted now and the round trip is exact identity. Only declared fields are dropped. JWK sets extra=allow and the schema's canonicalizableValue permits a null among those members, so a null inside cnf.jwk is data rather than an unset field. A first version filtered the whole serialized dict and removed it, which is the same defect one level down, and a test pins that half on its own. Signed-off-by: Louielunz <48041247+lywinged@users.noreply.github.com>

…t a uri" validated jsonschema treats format as an annotation rather than an assertion unless a checker for that format is installed. validate.py builds its validator with format_checker=jsonschema.FormatChecker(), which reads exactly as though URIs are checked. FormatChecker().checkers ships with date, email, idn-email, ipv4, ipv6, regex, time and uuid. uri is not among them without an optional dependency the project did not declare. So appraisal.verifier, transparency, model.aibom_uri, runtime.rim_uri, policy.policy_uri, tool_transcript.transcript_uri, build_provenance.provenance_uri and appraisal.policy_ref all accepted "not a uri" and the empty string. The wiring was correct and the behaviour was a no-op, which is the shape that survives review: there is nothing wrong to see in validate.py. rfc3986-validator is the dependency rather than jsonschema[format], which pulls rfc3987 into an Apache-2.0 package's dependency tree under GPLv3. Turning the constraint on changes nothing about the existing corpus. The suite passed unchanged before any new test was added. It does create one gap the models do not mirror: the schema now refuses a non-URI in appraisal.verifier and transparency, and the models type both a bare str. That is declared by field in the schema/model differential rather than value by value, because it is a whole class of value. Mirroring format: uri in the models is its own change. Signed-off-by: Louielunz <48041247+lywinged@users.noreply.github.com>

…reads validate.py exposes it with the comment "exposed for downstream tooling that needs the raw dict". `_schema()` is lru_cache(maxsize=1) and `_validator()` is built over whatever it returns, so the exported name and the validator's schema were one object. SCHEMA["properties"]["iat"]["minimum"] = 0 made a record dated 1970 valid to validate_json, to iter_errors, and to the structural gate inside sign.verify_record, for every later call in the process. Nothing about the call site looks wrong. `s = SCHEMA` followed by a mutation is what a caller building a variant writes, and adapting the raw dict is the use the comment invites. SCHEMA is a deep copy now. A shallow copy would not do: the nested properties dicts would still be shared and the same edit would still land. A test distinguishes the two rather than only asserting the objects are not identical. The copy is taken once, so two callers that both mutate it still see each other. That is an ordinary shared-object surprise and it is left alone. Reaching into the verifier from outside it is not, and that is the half closed here. Signed-off-by: Louielunz <48041247+lywinged@users.noreply.github.com>

github-actions · 2026-08-28T20:07:18Z

❔ Contributor Check: UNKNOWN

Check	Result
Profile	UNKNOWN
Credential	LOW
Overall	UNKNOWN

Automated check by AgenTrust Contributor Check.

imran-siddique

Sweeping the package rather than reading one function is the right way to find this class of defect, and one commit per defect with an explicit offer to split the series is what makes 1318 lines reviewable.

What I verified independently. Three of the nine, and I would rather say which than imply I checked all of them:

The exported SCHEMA was the live validator object. _schema() is lru_cached, so SCHEMA = _schema() handed downstream tooling the exact dict _validator() reads. Mutating it, which is the ordinary thing to do with a schema handed over to adapt, silently reconfigured validate_json, iter_errors and the structural gate inside sign.verify_record, process-wide, for every later call. The copy.deepcopy is right and the comment explaining why the copy is deliberate is what stops someone optimising it back out.
All eight format: uri declarations were inert. The validator was constructed with no format_checker, and JSON Schema treats format as an annotation unless one is registered. Eight declarations that read as constraints and enforced nothing.
bytes(5) is b'\x00\x00\x00\x00\x00', confirmed. So verify_assertion hashed five zero bytes, got a mismatch, and reported that the record at the URL had changed. As you put it, that is a specific accusation about somebody else's server, and it was false. That is the worst of the nine and it is worth that it is stated that way in the commit rather than as "type check missing".

I did not independently verify defects 1, 3, 4, 5, 6 and 7. CI is green on all six checks and the ratio is roughly 269 lines of source against 1000 of tests, which is why I am comfortable merging the series rather than holding it for a line-by-line pass.

Two of them converge with decisions made elsewhere today, which is worth recording: the boolean-accepted-as-SLSA-level defect is the same Python fact that trace-tests#85 caught in the conformance suite, and the revocation check reading truthiness is the same fail-open shape as a widened except I flagged on cmcp#590. isinstance(True, int) has now produced three separate defects across three repositories in one day.

The quickstart one deserves a note too. A comment reading "keep secure, never commit or log" above code that writes an unencrypted Ed25519 private key into the working tree is enforcement by politeness, and covering it in .gitignore with a test asserting the docs do not leave a key behind is the correct fix rather than editing the comment.

Merging. #234 and #233 both touch files this changes, so both will need a rebase; I am telling each of them.

lywinged added 9 commits August 28, 2026 19:48

lywinged requested a review from a team as a code owner August 28, 2026 20:06

github-actions Bot added the needs-review:UNKNOWN Contributor check flagged UNKNOWN risk label Aug 28, 2026

This was referenced Aug 28, 2026

fix(schema): runtime.platform description now matches §3.1.1 #234

Open

sign.verify_record: harden max_age_seconds/max_future_skew_seconds validation to match provenance.py #233

Closed

This was referenced Aug 29, 2026

Vendor platform annex: software-only deterministic multi-agent orchestrator (Bernstein) #231

Open

sign.verify_record: validate max_age_seconds/max_future_skew_seconds #239

Open

imran-siddique approved these changes Aug 29, 2026

View reviewed changes

imran-siddique merged commit 25013c5 into agentrust-io:main Aug 29, 2026
6 checks passed

imran-siddique mentioned this pull request Aug 29, 2026

verify_record reports a pass when no revocation check was performed, where §3.2.3 requires it to report that none was performed #246

Open

lywinged mentioned this pull request Aug 30, 2026

fix(schema): the subject pattern was a prefix test where the model requires an identity #248

Merged

9 tasks

imran-siddique mentioned this pull request Aug 30, 2026

0.9.0 schema rejects §3.1.2 references (additionalProperties: false) #241

Open

altrudev mentioned this pull request Aug 30, 2026

provenance.verify_record still leaks AttributeError on malformed cnf #251

Open

Navigation Menu

Uh oh!

fix: nine defects in the reference implementation, found by a package wide sweep - #236

fix: nine defects in the reference implementation, found by a package wide sweep#236
imran-siddique merged 9 commits into
agentrust-io:mainfrom
lywinged:fix/nine-defects-from-a-package-wide-sweep

lywinged commented Aug 28, 2026

Uh oh!

github-actions Bot commented Aug 28, 2026 •

edited

Loading

Uh oh!

imran-siddique left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

lywinged commented Aug 28, 2026

What this changes

Two things worth knowing before merging

Verification

Type of change

Spec section

Checklist

Uh oh!

github-actions Bot commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

imran-siddique left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Aug 28, 2026 •

edited

Loading