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

[harshnair75567-cloud]

What this changes

provenance.verify_record() validates max_age_seconds and max_future_skew_seconds
via _check_seconds() (added per the review on #164): a negative value is refused with
a named error rather than treated as a stricter bound, and bool is excluded because it
is an int subclass, so True would otherwise silently pass as one second.

sign.verify_record() — the original Trust Record verifier — never received the same
hardening. It checks only max_future_skew_seconds < 0 and compares max_age_seconds
against the record's age with no validation at all. Passing max_age_seconds=-1, a value
a caller might use meaning "no bound" (the documented way to disable the check is None),
rejects every record — including one issued the same second — as record is stale, with
an error naming the record rather than the misconfigured argument.

_check_seconds() moves to sign.py and is shared: sign.verify_record() calls it
directly, and provenance.verify_record() imports it, passing its own ProvenanceError
through a new exc parameter so each keeps its existing public error type.

No behavior changes for any previously-valid input. Reference implementation only —
spec/trace-v0.2.md and spec/server-provenance-v1.md are untouched.

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 above — this is a reference-library fix (src/agentrust_trace/sign.py,
src/agentrust_trace/provenance.py), not a change to normative spec text. Per
CONTRIBUTING.md, tooling changes are in the no-sponsor-required set.

Spec section

None — implementation only.

Checklist

• DCO sign-off on all commits (git commit -s)
• CHANGELOG.md updated (see entry below — not required for a non-normative change, added anyway since the reference library documents its own bug fixes there)
• Breaking changes marked with <!-- CHANGED: #NNN: description --> in spec text — N/A, not breaking
• Backward compatibility statement included — non-breaking: tightens rejection of already-invalid configuration values only; no previously-valid call changes behavior

[lywinged]

CI is red, and separately the change looks half applied. The code that is here behaves exactly as documented, so both are additions rather than rewrites.

The problem it describes reproduces exactly. On main, a record signed a moment earlier, with max_age_seconds=-1:

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

0s old is the changelog's "including one issued the same second", and the message does name the record rather than the argument that was wrong.

1. The failing step

House style (no em dashes), which runs tools/check_dashes.py:

CHANGELOG.md:16: em dash (use a colon, a comma, or two sentences)
  ...Passing `max_age_seconds=-1` — a value a caller might use meaning "no boun...
CHANGELOG.md:16: em dash (use a colon, a comma, or two sentences)
  ...is the documented way to disable the check — rejected every record, includ...

2 occurrence(s).

Both are in the changelog entry this adds, and the same script exits 0 on main. EXEMPT_PREFIXES is ("examples/", "spec/trace-v0.1.md"), so CHANGELOG.md is in scope. This is easy to miss: the rule and the script both arrived with #230.

I replaced those two characters with commas and the step exits 0. Nothing else is needed for it.

2. The provenance half is missing

The changelog says:

_check_seconds() is now defined once in sign.py and shared: sign.verify_record() calls it directly, and provenance.verify_record() imports it, passing its own ProvenanceError via a new exc parameter

The docstring on the new function says the same. provenance.py is not in this diff. It still carries its own _check_seconds at line 251 hardcoding ProvenanceError, so the package has two, and nothing anywhere passes exc.

So the exc parameter is right and the sentence describing it is ahead of the code. The missing half is small, and provenance.py line 23 already imports several private names from sign, so there is somewhere to put it:

• delete the local _check_seconds at provenance.py:251
• add _check_seconds to the existing from agentrust_trace.sign import (...)
• pass exc=ProvenanceError at the two call sites on lines 334 and 335

Line numbers are against e23e126. I applied that: 828 passed 1 skipped, ruff check src tests scripts clean, mypy src/agentrust_trace clean, and one _check_seconds in the package. The import runs one way, provenance to sign and not back, so there is no cycle. And exc then earns its place: with the local copy gone, provenance.verify_record still raises ProvenanceError for a string, a bool and a negative, which is the whole point of the parameter and is currently untestable because nothing passes it.

3. What I ran, and what passed

Against e23e126, the four steps in ci.yml verbatim:

step	result
ruff check src tests scripts	passed
python tools/check_dashes.py	exit 1, the two above
mypy src/agentrust_trace	passed
pytest --cov	828 passed, 1 skipped, 95%

And every branch the change adds, on a fresh clone with a signed record:

configuration	outcome
max_future_skew_seconds=-1	ValueError, must be non-negative
max_future_skew_seconds="x"	ValueError, must be an integer
max_future_skew_seconds=True	ValueError, must be an integer
max_age_seconds=-1	ValueError, must be non-negative
max_age_seconds="x"	ValueError, must be an integer
max_age_seconds=True	ValueError, must be an integer
max_age_seconds=None	accepted, the bound stays disabled

All seven behave as the docstring says. The bool exclusion is the part worth having: isinstance(True, int) is true in Python, so True would otherwise have passed as a one second bound.

4. One note, not blocking

The type check is the one branch with nothing behind it. I disabled each of the three branches separately and reran:

branch disabled	tests that fail
isinstance(value, bool) or not isinstance(value, int)	none, 828 still green
value < 0	1, test_verify_record_rejects_negative_future_skew_configuration
optional and value is None	1

The two that fail are caught by tests already on main: the first guards the three lines this replaces, and the second exists because something already relies on max_age_seconds=None disabling the bound. So the < 0 and optional behaviour is held, and the isinstance guard, which is the whole of the new type and boolean handling, is held by nothing. The suite count is unchanged at 828, which is the same reading from the other side.

A few parametrized cases rather than a redesign. The behaviour is already right; what is missing is the part that says so.

[imran-siddique]

The change is right and I want it. provenance.verify_record() got _check_seconds() on the #164 review and sign.verify_record() never did, so max_age_seconds=-1 classified every record as stale and raised an error naming the record rather than the misconfigured argument. Sharing one implementation with an exc parameter, rather than copying it, is the correct call: it keeps ValueError and ProvenanceError as the two public types while there is only one rule.

Two things to fix before it can land.

1. The red check is not a test failure. test (3.12) fails on its House style (no em dashes) step. There is exactly one em dash, in the CHANGELOG entry:

Passing max_age_seconds=-1 — a value a caller might use meaning "no bound," since None is the documented way to disable the check — rejected every record

Both need replacing. A comma, a colon, or parentheses all work; the repo uses spaced hyphens elsewhere in that file.

2. Missing blank lines before _check_seconds. At src/agentrust_trace/sign.py on your head commit e23e126, line 278 is the closing raise ValueError(...) from exc of _b64url_decode and line 279 is def _check_seconds( with nothing between them. The diff removed the blank line that was there. Module-level definitions want two blank lines. The linter is not catching it and it should not land that way.

One sequencing note that is not yours to fix. #236 rewrites verify_record in the same file and both of you append to CHANGELOG.md, so these two will conflict. Yours is the focused one at 28 lines against 1318, so I would like yours to land first and #236 to rebase onto it. Nothing for you to do beyond the two fixes above; I am recording it so neither of you is surprised.

Push those and I will re-review straight away.