test: sweep every keyword argument of every public function, and close what it found

[lywinged]

What this changes

tests/test_public_functions_raise_what_they_document.py swept the first positional argument of every public function and declared provenance.build_record and intent_bridge.verify_bridge unsweepable, for a true reason: their arguments are keyword-only. #320 then arrived on build_record's issued_at, a keyword argument that int() coerced before the guard saw it. An exemption whose reason is true is still an exemption, and the leaks were on the other kind of argument.

The sweep now varies every parameter of every public function, one at a time, from a call that is valid in full, against the same junk matrix. Three things hold it honest. Every parameter is accounted for by name, so a new one fails the coverage test until it is swept or declared with a reason. Every baseline is asserted to succeed before anything is varied, so a leak is from the varied parameter and not from a baseline that was already broken. One witness per function proves the call arrives. Two further checks come from the shape of #320: what a producer accepts, its own verifier has to accept; and a bare string or a single object where an iterable of them is expected is refused rather than iterated.

What it found on main, and what changed

Eight parameter sites raised exceptions their modules do not document, and five behaviours produced a wrong result with no exception at all: the emitted false, the dropped anchor, the string read as identifiers, the JWK read as a list, and the empty value read as no keys.

• build_record(publisher=123) raised TypeError from the regex. Now ProvenanceError.
• build_record(attestation=False) emitted false where spec/server-provenance-v1.md says null, because truthiness stood where is None was meant. Now refused, on both sides: the shape check is shared, so a record already carrying false, 0 or "" there is refused by verify_record too. That is the one verification-side change here, and a test pins it with the null control beside it.
• provenance.sign_record let rfc8785's errors and key_to_jwk's ValueError escape. Now ProvenanceError, with the wrap intent_bridge._jcs already carries. provenance.verify_record carried the other half and now has the same wrap: the record a verifier reads is the untrusted document, and an out-of-range integer under a field the structural checks do not type, a tools entry's version for instance, left that function as rfc8785's IntegerDomainError. Neither escape is one of the eight: the junk matrix never reaches canonicalisation, a record carrying such an integer does.
• intent_bridge.sign_bridge(key="a-string") raised AttributeError. Now IntentBridgeError. The guard is isinstance(key, Ed25519PrivateKey), which the package's two other signers already require through key_to_jwk, so a duck-typed key object that used to sign a bridge no longer does; that is the one signing-side behaviour change here.
• content_marking.build_assertion raised TypeError for a non-string alg, and a non-string or empty anchor was dropped silently, so the caller got an assertion with no anchor and no error. Both now ContentMarkingError.
• revocation.check_bundle(trusted_key_identifiers="sha256:...") iterated the string as characters, matched no statement, and reported verified. A single JWK passed where a list was meant iterated as its field names. Both now ValueError, and the elements are typed.
• sign.verify_record(trusted_bundle_keys="") passed trusted_bundle_keys or () down, so an empty string, an empty object or False meant no keys. Only None means that now; the rest reach the check and are refused.

The eighth site is issued_at itself, which is #320's, and it is now closed here rather than filed. #334 merged the reordering half of #320 at 04:31Z on 09-12 and closed the issue. On the rebase onto c04d938 the marker did what it is for: test_no_keyword_argument_leaks_an_undocumented_exception[provenance.build_record.issued_at] went XPASS(strict) and this file failed rather than going quietly green.

The second case under the same entry did not flip. test_what_a_producer_accepts_its_own_verifier_accepts asks whether a producer emits anything its own verifier refuses, and for issued_at it still reported one value, 10000000000000000000. That is the other half of #320: _check_structure had no upper bound, so build_record accepted a timestamp sign_record cannot canonicalise and the caller met rfc8785's IntegerDomainError rather than the ProvenanceError the module documents. #219 bounded every integer a schema declares, and the provenance record has no schema, which is why this field was the one left. The bound is in _check_structure now, reading JCS_SAFE_INTEGER from sign.py, and LEAKS_FILED is empty.

_check_structure is shared, so the bound reaches verify_record too. Under the default freshness policy such a record was already refused as dated in the future, so nothing that verified before is refused now; the case that changes is a caller who widens max_future_skew_seconds past the gap, which returned rfc8785's error out of a function documented to raise ProvenanceError and now returns ProvenanceError. Reverting only that clause fails four tests, one of them the producer-and-verifier case above, so the instrument that reported the gap is what confirms it is shut.

One arithmetic note, because the count below does not move and the reason for it did. With all five modules reverted the report still shows 7 unfiled leaks even though LEAKS_FILED is now empty: main already carries #334's reordering, so reverting provenance.py to main does not bring the exception leak back, only the missing bound, which is a wrong result rather than a leak the report counts.

tools/sweep_public_surface.py prints, from the same tables, what each parameter accepts and which leaks are filed where, and --strict exits 1 on an unfiled leak. It is a report and not a test because the accepted column is a judgment: a now of 0 is accepted on purpose, an issued_at of True is accepted by accident. Two tests hold that it still runs over every function in both tables and that its exit goes red for a planted leak with nothing filed for it, a control that still fires the day the last real leak is fixed, so the sweep is read from a report rather than re-run by hand. Three of its accepted rows I looked at and left: revocation accepts any string because a str is a Container[str] and substring membership still finds an identifier, revocation_bundle accepts junk because the docstring promises it does not raise, and allow_embedded_key is read by truthiness like any flag.

Checked on the branch

Reverting each fixed module in turn fails the sweep: 5, 3, 12, 11 and 4 tests for content_marking, intent_bridge, provenance, revocation and sign, and --strict exits 1 for the first four; sign's fix refuses with a documented ValueError, so it is a wrong result the tests catch and not a leak the report counts. With all five reverted at once the report shows 7 unfiled leaks. CI on 5ec62d5 is green on all five checks: gate, test (3.11), test (3.12), Analyze (python) and CodeQL. Locally, on the same head, 1547 passed and 1 skipped with nothing xfailed, against 1494 passed, 1 skipped and 2 xfailed on the previous base; the two xfails were the issued_at cases and are now passing tests. ruff passes on tools/ too, which CI does not lint. No wire format, schema or signature semantics change. Every record that verified before verifies now, except the falsy attestation stated above; checked by building every producer output the base accepts, on the base, and verifying each on this head.

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

None of the five. Reference implementation and its tests only.

Spec section

None changed. spec/server-provenance-v1.md's null for attestation is what one of the guards now enforces.

Checklist

• DCO sign-off on all commits (git commit -s)
• CHANGELOG.md updated (for any normative change): hunk deliberately dropped on 09-11 at the maintainer's request, so the queue stops conflicting on one block. The entry text is above under "What it found", which is where he said he would take it from.
• Breaking changes marked with <!-- CHANGED: #NNN: description --> in spec text: not a breaking change
• Backward compatibility statement included (for breaking changes): not a breaking change

[github-actions]

❔ Contributor Check: UNKNOWN

Check	Result
Profile	UNKNOWN
Credential	LOW
Overall	UNKNOWN

Automated check by AgenTrust Contributor Check.

[imran-siddique]

Conflicted by my merge of #315, on CHANGELOG.md and nothing else.

Rebase onto df0120b9b7d435d4217ffe34020461c6dd7bd0bf.

Trial-merged before writing this: the only conflicted file is CHANGELOG.md, and content_marking.py, intent_bridge.py, provenance.py, revocation.py and sign.py all merge clean. I also compared your added lines against @altrudev's #318, #322 and #323, which touch intent_bridge.py and provenance.py as well: zero shared added lines in any pairing, so there is nothing to reconcile beyond the changelog.

#327 is unaffected and still applies cleanly.

[lywinged]

Rebased, 5faefe2, onto 760cc3f.

Confirmed: CHANGELOG.md is the only conflict, both entries kept with this one's first. The two files your list does not cover, tests/test_public_functions_raise_what_they_document.py and tools/sweep_public_surface.py, are new on this branch and untouched by either merge.

One coupling the added-line comparison cannot reach, because it shares no line: #323 is #320's fix, and #320 is what this branch's two strict=True xfails are filed against. Applying #323's src/ diff onto this head turns both into XPASS(strict), so whichever of the two lands second takes main red until those markers come out. #318 and #322 applied the same way leave the sweep at 148 passed and 2 xfailed.

[imran-siddique]

Merging #322 re-conflicted this on CHANGELOG.md again, an hour after you rebased for the same reason. That is my sequencing, not yours.

Please drop the CHANGELOG.md hunk from #325 and force-push without it. It becomes mergeable immediately and stops being able to conflict with the rest of the queue.

I will write the changelog entries for this batch in one maintainer commit after they land, from the PR bodies.

The cause is structural: every open PR here appends to one block, so each merge conflicts every other, and the rebase count scales with the queue rather than with any change. A fragment directory is the fix and I am taking it separately.

content_marking.py, intent_bridge.py, provenance.py, revocation.py and sign.py all merge clean against current main. The changelog is the only obstacle.

[lywinged]

Done, 9387137. The CHANGELOG.md hunk is out and the branch is rebased onto 10fcba4, so it is one commit of seven files against current main.

The entry text is still in the PR body under Why, which is where you said you would take it from. The checklist line above it now overstates, since the file is no longer touched.

#322 and this branch both change intent_bridge.py and the rebase was clean. The suite on the new base gives 1499 passed, 1 skipped and 2 xfailed, against 1494 before your merge, and the sweep is 148 passed and 2 xfailed either side of it, so nothing in #322 added a parameter it cannot see. ruff, mypy and check_dashes.py pass.

[lywinged]

head 5ec62d5: ready for review, nothing outstanding on my side.

Rebased onto c04d938, now two commits: the sweep as you last saw it, and one that closes
the last thing it was still reporting.

The branch is out of the conflict loop. GitHub reports it blocked rather than dirty, and
all five checks are green on this exact head: gate, test (3.11), test (3.12),
Analyze (python) and CodeQL, finished 06:38 to 06:39Z. No conflicts, no red, no
CHANGELOG.md hunk to conflict with anything else in the queue. The only thing between it
and merge is a review, so it is approvable as it stands.

Two things are yours to decide and neither needs an answer before you read the rest. The
second commit adds a src/ change to a branch you have not reviewed yet, and if you would
rather it went as its own pull request against a fresh issue it is separable, being the
whole of the second commit. And #334 and this branch now sweep provenance.build_record
from two tables; I have left both rather than delete an entry that landed hours ago.

The strict marker is what turned up the rest, and not by firing. One of the two cases it
covered fired and the other did not, and the one that did not is the informative half.

What the marker did

LEAKS_FILED held one entry, ("provenance.build_record", "issued_at") against #320, and
it covered two cases. #334 merged the reordering half of #320 at 04:31Z.

On the rebase,
test_no_keyword_argument_leaks_an_undocumented_exception[provenance.build_record.issued_at]
went XPASS(strict) and the file failed, so the entry had to come out rather than going
quietly green.

test_what_a_producer_accepts_its_own_verifier_accepts[provenance.build_record.issued_at]
did not flip. It asks whether the producer emits anything its own verifier refuses, and for
issued_at it still reported exactly one value: 10000000000000000000.

The case that did not flip is the useful one. On 09-11 I told you that applying #323's
src/ diff to this head turns both markers into XPASS(strict); re-running it now,
that still holds. #334 carried one half of #320 and flips one. The only difference between
the two diffs is the safe-integer bound, so the case that did not flip names the half still
open, and nobody had to go looking for it.

The half that was open

On c04d938:

build_record(issued_at=2**60)            -> built, issued_at=1152921504606846976
sign_record(that record)                 -> rfc8785.IntegerDomainError

_check_structure had no upper bound, so the producer accepted a timestamp it cannot
canonicalise, and the class the caller met was not the ProvenanceError the module
documents. #219 bounded every integer a schema declares, across five schemas plus the
undeclared cnf.jwk members, and the provenance record has no schema: its structural rules
are _check_structure. So #219's mechanism could not reach this field, which is why it is
the one that survived. I proposed the bound on #320 on 09-10 and @altrudev carried it on
#323; #334 landed without it and #320 is closed, so until this push the only place the case was
still written down was #323, which is superseded on everything else it carries.

The guard now carries it, reading JCS_SAFE_INTEGER from sign.py rather than writing the
number a third time. LEAKS_FILED is empty, and the comment above it records what the entry
held and what each of the two firings established, so the history does not leave with the
line.

The verifier side, stated because _check_structure is shared

Under the default freshness policy a record carrying such a timestamp was already refused,
as dated in the future, so no record that verified before is refused now. That is #219's own
argument about iat reaching the same field by the other route: a timestamp at the ceiling
dates a record to the year 285 million, and a verifier enforcing freshness rejects it before
canonicalization, so the collision never decides anything. What changes is a caller who
widens max_future_skew_seconds past the gap: that path returned
rfc8785.IntegerDomainError out of verify_record, which is documented to raise
ProvenanceError, and now returns ProvenanceError. A test pins it.

This is the same leak class the branch already closes on provenance.sign_record and
provenance.verify_record, where both now catch rfc8785.CanonicalizationError in the
shape intent_bridge._jcs already carried. issued_at is the one route into it that a
structural rule can close at the producer instead, which is better, because the caller is
told which field is wrong rather than that something somewhere in the record had no
canonical form.

Verified in both directions

• With the clause: 1547 passed, 1 skipped, and this file at 152 passed with nothing
  xfailed, against 148 passed and 2 xfailed before. Two more cases than before, both from
  #334's own CALLS and REACHES entries for build_record; the two that were xfailed are
  the same two cases, now passing. ruff, mypy and tools/check_dashes.py pass.
• With only the clause reverted, everything else kept: 4 fail. The two out-of-range boundary
  cases, the verifier case, and
  test_what_a_producer_accepts_its_own_verifier_accepts[provenance.build_record.issued_at],
  which is the instrument that reported the gap now confirming it is shut.

The boundary is asserted against the canonicalizer rather than against a literal. Each of
the four cases checks that build_record and rfc8785.dumps agree about whether the value
exists, so the guard cannot drift away from the thing it is guarding, which is the failure
that produced this in the first place.

The overlap with #334, in full

#334 put provenance.build_record into CALLS with issued_at as the varied argument.
This branch sweeps the same function over every parameter through KEYWORD_CALLS. Both
survive the rebase, so issued_at is swept twice. That costs a duplicated sweep and costs
nothing in coverage, and I left your entry rather than delete it: removing a test that
landed hours ago, inside a pull request about something else, is not this branch's call. A
note at the site says the same. Collapse it or keep it as you prefer.

One line of the PR body is stale, its account of the issued_at cases as strict expected
failures against #320, and I will correct it. My comment on 09-11 saying the branch is one
commit is stale too and stays, since it was true when it was written.