sign.verify_record: validate max_age_seconds/max_future_skew_seconds

[harshnair75567-cloud]

continues #233,which was closed after I accidentally deleted my fork. Same change,same review discussion — recovered the commits from the PR ref and pushed them to a new fork. Both review rounds are addressed:

• House style: removed the two em dashes in the CHANGELOG entry
  (tools/check_dashes.py now passes clean).
• Missing blank line before _check_seconds in sign.py (two blank lines
  now, matching module-level convention).
• The missing provenance.py half: local _check_seconds removed, imported
  from sign.py instead, both call sites now pass exc=ProvenanceError.
• Added the parametrized tests for the type/bool/negative branches
  (test_a_malformed_max_age_is_reported_not_applied,
  test_a_malformed_skew_is_reported_not_applied,
  test_a_well_formed_bound_still_verifies) — this covers the one branch
  lywinged flagged as untested (the isinstance bool/type guard).

ruff check src tests scripts, python tools/check_dashes.py,
mypy src/agentrust_trace, and pytest (843 passed, 1 skipped) all clean.

[rest of the original PR template — What this changes / Type of change /
Spec section / Checklist — same content as before]

[lywinged]

Ran this at def60e1 against e7e2eca, on a fresh clone, on both legs of the CI matrix. All four steps are green on 3.11 and on 3.12, with identical counts:

step	e7e2eca	def60e1
ruff check src tests scripts	pass	pass
python tools/check_dashes.py	pass	pass
mypy src/agentrust_trace	pass	pass
pytest	828 passed, 1 skipped	843 passed, 1 skipped

The delta is +15. Diffing tests/ between the two refs, this PR adds three test functions carrying 15 parametrized cases and touches nothing else: zero removed lines, no existing test modified, no parametrize list changed.

Rather than read for the #233 points I re-ran the checks that produced them.

The em dashes are gone. check_dashes.py reported 2 occurrences in CHANGELOG.md:16 at e23e126; it exits 0 here, and the two characters are now parentheses.

The provenance half is done. _check_seconds() is defined once, at sign.py:281, and a package-wide grep finds no second definition. provenance.py imports it and passes exc=ProvenanceError at both call sites. I checked that the exc plumbing is actually held rather than merely present: dropping exc=ProvenanceError from those two sites fails 14 cases in tests/test_provenance.py, across three test functions, all three of which are on main already. So the part of the refactor that could silently change a public error type is covered.

The branch that had nothing behind it now has something. The note on #233 was that of the three branches in _check_seconds, the isinstance/bool one was held by no test, and that a few parametrized cases would fix it. Re-running that same table here:

branch deleted	tests failing at e23e126	tests failing at def60e1
isinstance(value, bool) or not isinstance(value, int)	0	18
value < 0	1	2

The first row is the ask, answered. The type and boolean handling went from unheld to held on both sides of the shared helper. That is the substantive part of this PR and it is right.

The second row is why I am not saying this is ready. It moved by one, and that one is provenance's pre-existing skew test now running through the shared helper. No test this PR adds is in it.

The value < 0 branch is held by none of the 15 new tests

I deleted if value < 0: raise ... from the shared _check_seconds() and ran the suite. All 15 new cases pass:

test_a_malformed_max_age_is_reported_not_applied[-1]      PASSED
test_a_malformed_max_age_is_reported_not_applied[-86400]  PASSED
test_a_malformed_max_age_is_reported_not_applied[False]   PASSED
...  all 13 malformed cases and both controls: PASSED

The only two failures in the whole suite are test_verify_record_rejects_negative_future_skew_configuration and provenance's test_a_negative_skew_bound_is_refused_rather_than_applied, both of which were already on main, and both of which are about skew. 841 passed, 2 failed.

So the negative-bound behaviour, which is the PR title, the CHANGELOG entry and the test comment, is demonstrated by nothing that this PR adds.

The reason is one line. Reverting sign.verify_record's freshness block to e7e2eca byte for byte and leaving the tests untouched, four cases still pass:

test_a_malformed_max_age_is_reported_not_applied[-1]      PASSED
test_a_malformed_max_age_is_reported_not_applied[-86400]  PASSED
test_a_malformed_max_age_is_reported_not_applied[False]   PASSED
test_a_malformed_skew_is_reported_not_applied[-1]         PASSED

because the unfixed code says:

ValueError: record is stale: iat is 0s old, exceeds max_age_seconds=-1

That is a ValueError and it contains the string max_age_seconds, so assert "max_age_seconds" in str(exc.value) is satisfied by exactly the behaviour being removed. The stale message interpolates the parameter name, so the name cannot separate "reported" from "applied."

Running the 13 parameters against both trees directly, with no mutation at all, reaches the same four, which is worth having as a second route to the same place. It also shows a case the changelog entry does not mention: at e7e2eca, max_age_seconds=True and max_age_seconds=1.5 raise nothing at all. verify_record returns successfully, having silently used 1 and 1.5 as the bound in seconds.

The discriminating assertion is already in the file. test_verify_record_rejects_negative_future_skew_configuration at line 429, twenty lines above the first new test, uses match="must be non-negative", which is text only _check_seconds() produces. Two words each:

assert "max_age_seconds must be" in str(exc.value)
assert "max_future_skew_seconds must be" in str(exc.value)

I checked this is the right change and not just a plausible one. With it applied: deleting the value < 0 branch now fails 3 of the 15 instead of 0, those being max_age_seconds at -1 and -86400 and max_future_skew_seconds at -1, which are the only three parameters that reach that branch at all, since every other malformed case is caught by the type check first. Against the reverted freshness block, 12 of the 13 malformed cases fail. The thirteenth is max_future_skew_seconds=-1, which still passes, correctly, because e7e2eca genuinely guards that one and already raises max_future_skew_seconds must be non-negative. Both controls still pass and the suite against this PR as submitted is unchanged at 843 passed, 1 skipped.

The same gap is in tests/test_provenance.py

These tests mirror the provenance ones faithfully, assertion included, so this is inherited rather than introduced here. Worth saying only because it is the same edit and cheap to make in both places at once.

Removing both _check_seconds() calls from provenance.verify_record on e7e2eca leaves the same four cases green, for the same reason:

ProvenanceError: record is stale: issued_at is 0s old, exceeds max_age_seconds=-1
ProvenanceError: record is dated 0s in the future, exceeds max_future_skew_seconds=-1

There the gap is latent, because the guard is present, so nothing can currently pass on a missing one. Here it is live: these tests were written to demonstrate a guard that did not exist yet, and for the headline input they would have passed without it.

The blank line is half done

The blank line before _check_seconds is fixed. The reading of the cause was right, and it explains a second site that is still open. e7e2eca had two blank lines between _b64url_decode and def sign_record; 3945a77 removed one of them and inserted the helper into the gap:

     except (binascii.Error, ValueError) as exc:
         raise ValueError(f"{field} is not valid base64url: {exc}") from exc
-
+def _check_seconds(

So the helper arrived with zero blank lines above it and one below, and ruff check --preview --select E302 finds both, on ruff 0.15.8 and 0.16.5 alike:

e7e2eca:  none
e23e126:  sign.py:279:1  Expected 2 blank lines, found 0     <- fixed
          sign.py:302:1  Expected 2 blank lines, found 1
def60e1:  sign.py:304:1  Expected 2 blank lines, found 1     <- still open

Line 304 is def sign_record, immediately below the new helper, 25 lines under the one that was named. Module-level definitions want two blank lines applies to it identically. CI stays green because E302 is a preview rule, so neither site shows up in a plain ruff check. ruff check --preview --select E302 --fix closes it.

Smaller

• tests/test_sign.py:437: the comment says "mirroring provenance.py's _check_seconds", but this commit moves that helper into sign.py. After this PR it appears in provenance.py only as an import.
• max_future_skew_seconds=-1 in the new parametrize duplicates test_verify_record_rejects_negative_future_skew_configuration at line 429, and it is the one skew case e7e2eca already covered.
• DCO: def60e1 carries Signed-off-by:. 3945a77 and e23e126 do not; 3945a77 has a signed-harshnair75567@gmail.com line, which is not the Signed-off-by: trailer. git rebase --signoff e7e2eca covers all three.
• Optional: the provenance set includes test_zero_is_a_bound_and_not_a_falsy_stand_in_for_unset and the mirror stops one short of it. No defect here, sign.verify_record uses if max_age_seconds is not None: and I confirmed 0 is enforced as the strictest bound while None disables the check. That test is what would catch a later refactor to a truthiness check, so it is the one worth copying across.

Sequencing

On the note about this landing first and #236 rebasing onto it, I ran that rather than assume it. Rebasing #236's nine commits onto def60e1: one conflict, in CHANGELOG.md, resolved by keeping both sides, and the entry count comes out at 41 from main plus 1 from here plus 9 from #236, so nothing is dropped or duplicated. No conflict in sign.py despite both touching verify_record. On the result, all four CI steps pass and the suite is 1030 passed, 1 skipped, which is 828 plus 187 plus 15 exactly. The 15 cases added here all still pass, and _check_seconds is still the single definition with both provenance call sites still passing exc=ProvenanceError.

So the ordering works and costs nothing on the code side. The two-word assertion change below also holds on that combined tree, which is where it would actually live if this lands first: 1030 passed with it applied, and deleting the value < 0 branch there fails 8 cases instead of 2.

Test-merges against the rest: clean against #235 and #237, CHANGELOG-only conflict with #234, the same adjacent-insert kind.

[harshnair75567-cloud]

Pushed a squashed, signed commit addressing everything from the last review:

• Fixed the two em dashes in CHANGELOG.md
• Fixed both missing blank lines (before _check_seconds AND before sign_record — the second one wasn't caught by plain ruff, only --preview --select E302)
• Tightened both malformed-input assertions to check for "must be" instead of just the parameter name, since the old message also contained that substring
• Removed the max_future_skew_seconds=-1 duplicate
• Fixed the stale comment
• Added test_zero_is_a_bound_and_not_a_falsy_stand_in_for_unset, mirroring provenance.py's

843 passed, 1 skipped. ruff (including --preview --select E302), check_dashes.py, and mypy all clean.

History is now a single signed commit (e47c145) rather than the original three, two of which weren't signed.

[lywinged]

Approving. Re-ran this at e47c145. Everything raised earlier is addressed, and the part that mattered is now load-bearing rather than merely present.

Four CI steps, both matrix legs:

3.11   ruff pass   check_dashes pass   mypy pass   843 passed, 1 skipped
3.12                                   mypy pass   843 passed, 1 skipped

The earlier review's one substantive point was that the new tests passed against the defect they were written for, because the stale message interpolates the parameter name and the assertion only looked for that name. The tightened assertion closes it, checked by deleting each branch of _check_seconds rather than by reading:

delete `value < 0`     4 tests fail, 2 of them this PR's own cases   (previously 0 of its own)
delete the type check  19 tests fail, all 19 this PR's own cases

So both branches of the shared helper are held by tests this pull request adds, which is what the earlier round could not say. ruff check --preview --select E302 is also clean on src/agentrust_trace/.

On sequencing, since several branches touch the same block: this fast-forwards onto main, and merges clean against #235, #237 and #243. #234, #236 and #242 conflict on CHANGELOG.md only, all of them the ordinary adjacent-insert kind at the top of ### Fixed where keeping both entries resolves it. Nothing in src/ or tests/ conflicts with any of them.

[imran-siddique]

Read the diff at e47c145. Both review rounds are addressed and the shape is right.

The defect is worth stating plainly, because it is the interesting part: max_age_seconds=-1 did not tighten the bound, it classified every record ever issued as stale, and reported that as record is stale. The caller misconfigured the verifier and the verifier blamed the document. That is a false statement about somebody else's record, produced by a check that never ran correctly.

Sharing _check_seconds from sign.py with an exc parameter is the right way to do it. Each module keeps the public error type its docstring promises, and there is now one implementation rather than two that can drift apart, which is how sign.verify_record() came to be missing this in the first place.

Thanks for recovering the commits from the PR ref after the fork went. That is more work than reopening, and it kept the review history attached.

Approving to release the hold. Merging. Closes out what #233 started.

[imran-siddique]

Correction to my review above: it says "Merging" and the merge did not land. GitHub refused it as not cleanly mergeable. The approval stands and nothing about the change needs revisiting, but I should not have written the merge as done before it was done.

The conflict is my sequencing, not anything you did. I merged four trace-spec PRs in the last hour and two of them landed on your files. Current main is 39e05195a72fed155d6b7cc1df9b222fe45d7d09.

• CHANGELOG.md: fix(schema): runtime.platform description now matches §3.1.1 #234, feat: verify_record consumes the revocation bundle and reports its check #271 and docs(schema): read runtime.platform's two situations as examples #272 all appended to the Unreleased section. Keep both sides; nothing merged today edits your entry.
• src/agentrust_trace/sign.py: this is the real one. feat: verify_record consumes the revocation bundle and reports its check #271 rewrote verify_record(), which is the function you are hardening. It gained four keyword arguments: revocation_bundle, trusted_bundle_keys, max_bundle_age_seconds and now.

That second conflict is worth more than a mechanical resolution, because #271 made your change more valuable rather than less. verify_record() now validates its bounds like this, at lines 529 to 532:

if max_bundle_age_seconds < 0:
    raise ValueError("max_bundle_age_seconds must be non-negative")
if max_future_skew_seconds < 0:
    raise ValueError("max_future_skew_seconds must be non-negative")

So there are now three freshness bounds on that function, not two, and max_bundle_age_seconds arrived carrying the exact weakness your PR exists to remove. A bare < 0 test accepts True as one second, accepts "300" never, and accepts a float silently in some paths. When you rebase, please route all three through _check_seconds rather than only the two you started with. max_age_seconds keeps optional=True since None disables it; the other two are not optional.

If the resolution turns out to be more tangled than that, say so and I will carry it on a maintainer branch with credit to you rather than have you fight my merge order.

[harshnair75567-cloud]

Rebased onto current main (39e0519). Since #271 landed a third freshness bound (max_bundle_age_seconds) with its own naive < 0 check, routed it through _check_seconds alongside the other two, per your note. Added test_a_malformed_bundle_age_is_reported_not_applied (8 cases) and test_a_well_formed_bundle_age_still_verifies (2 cases), mirroring the existing pattern. Confirmed by mutation: deleting the bundle-age check now fails those 8 cases; deleting the type check would fail it too. Full suite: 1192 passed, 1 skipped. ruff (incl. --preview --select E302), check_dashes.py, and mypy all clean.

[lywinged]

The force-push to 4d961e4 left three conflict-marker lines committed in CHANGELOG.md: <<<<<< HEAD on line 26, ====== on line 50, and >>>>>> e47c145 ( plus the old commit subject on line 52. They are six characters wide rather than seven, so git diff --check does not see them and the CI workflow is green on GitHub; the changelog as rendered still shows them. The only red check is the approval gate, which is about the approvals rather than the code, see below. The three lines are also the only thing that conflicts with main: delete them and nothing else needs reconciling, the branch merges clean onto 4383b14 as it stands, and your entry and the #242 entry are both kept, so no rebase is needed. That is the one item between this and re-approval.

Two optional things from the same push. It now routes max_bundle_age_seconds through _check_seconds as well, with ten new cases; that is consistent with the signature (int = 86400, None was never accepted) and worth a line in the PR description. No action is needed on the description otherwise; if you do edit it, the 843 has become 1192 passed, 1 skipped on this head. And ruff check --preview --select E302 reports a missing blank line before test_zero_is_a_bound_and_not_a_falsy_stand_in_for_unset at tests/test_sign.py:492; E302 is preview-only under ruff 0.16.6, so CI does not run it.

The gate is red only because the approvals of e47c145 do not cover the new head. The change is what was approved there plus the bundle-age guard. With the three lines gone I will re-approve.

[harshnair75567-cloud]

I'll change it right away thank you for being so concise with your analysis

[harshnair75567-cloud]

Removed the three leftover conflict-marker lines from CHANGELOG.md (26/50/52) — nothing else in that file changed, both entries are kept as noted. Also fixed the E302 blank-line gap at tests/test_sign.py:491 flagged above. Head is now 610da6e, CHANGELOG/tests only — sign.py and provenance.py are unchanged from the head you reviewed. Full suite still 1192 passed, 1 skipped; ruff (incl. --preview --select E302), check_dashes.py, and mypy all clean.

[lywinged]

610da6e checks out: the three marker lines are gone, the blank line at tests/test_sign.py:491 is in, and sign.py and provenance.py are byte-identical to the head I reviewed. One signed commit, merges clean against main at 8aebaa0, and the four ci.yml steps pass for me locally at 1192 passed, 1 skipped. Approving.

[imran-siddique]

Re-reviewed 610da6e. The conflict markers and blank-line issue are fixed; the shared helper validates all three freshness bounds while preserving ProvenanceError on the provenance path. All 218 tests in test_sign.py and test_provenance.py pass locally. Hosted CI is green apart from the original stale maintainer gate, with no held workflows or unresolved inline threads. The current head also has lywinged's approval.

The full local Windows run produced 1187 passes, one skip, and five failures in unchanged fixture-generation and schema-path tests. The schema inventory compares Windows backslashes with slash-delimited paths, and the generators compare raw file bytes across platform line endings. This review does not claim the full local suite passed.