fix(tr-sig): report a record with no canonical form instead of raising on it - #86
Open
lywinged wants to merge 1 commit into
Open
fix(tr-sig): report a record with no canonical form instead of raising on it#86lywinged wants to merge 1 commit into
lywinged wants to merge 1 commit into
Conversation
…g on it `runner.run` calls every module with no `try`, and `tests/test_modules_never_raise.py` states the contract in its first line: a malformed record must produce a finding, never an exception. `tr_sig` breaks it while canonicalizing. `_canonical_json` is `rfc8785.dumps`, which raises on three classes of value JCS has no form for: an integer outside the safe range, a non-finite float, and a string carrying a lone surrogate. All three are ordinary JSON. `json.loads` accepts them and `load_record` has no reason to refuse them, so both CLI commands reached them as a traceback, exiting 1, which is also what an honest FAIL exits. A caller reading the status code could not tell a rejected record from a crashed tool. Both call sites were exposed, `check_cmcp_runtime` over the whole envelope and `check` over the trace fields. Across the eight vectors, 20 wrong-typed values and 240 nested paths, 87 paths raised this way; now none do. The 30 that remain are the `TypeError` class agentrust-io#85 addresses plus the library-caller path `test_modules_never_raise.py` discloses, and neither is touched here. `_canonical_json` still raises, because `test_canonicalization_boundary.py` compares its bytes directly and a wrapper that swallowed the error would hide a real serializer defect. `_canonical_body` is the boundary: callers that owe a verdict use it, and it returns the reason rather than only the failure. All five `raise` sites in `rfc8785._impl` are `CanonicalizationError` or a subclass, so one clause covers every way it can refuse. The finding says the record has no canonical form and does not say verification failed, because those are different facts and a consumer acting on the second would go looking for a key problem that is not there. This came in with agentrust-io#66, and that is measured rather than inferred: at `c725bbb^` all three records verify to `Result: FAIL`; at `c725bbb` they traceback. That change was correct and §3.2.2 requires it, but `json.dumps` emitted `NaN`, `Infinity` and large integers without complaint. Making the canonicalizer correct made it strict, inside a function whose callers may not raise. Nothing caught it and no number of runs would have. Every one of the nine values `JUNK` carried serializes through `rfc8785` cleanly, so that test could not see the class by construction. The three are added to it first, which is the general guard; `tests/test_canonicalization_refusals.py` is the specific one and asserts the verdict rather than only the absence of an exception. Each of its four vectors is checked for still provoking its own error before anything else is asserted about it, so a vector edited into serializability fails loudly instead of passing over a record that exercises nothing. With the fix reverted, 8 assertions in that file fail and the widened `JUNK` takes the general guard red alongside them. 384 to 397 passed, the 13 added here; ruff and mypy unchanged at 57 and 3, all pre-existing and none in the files touched. Signed-off-by: Louielunz <48041247+lywinged@users.noreply.github.com>
Contributor
|
❔ Contributor Check: UNKNOWN
Automated check by AgenTrust Contributor Check. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found while reviewing #85, which is about the same contract and does not touch this. It is a defect I shipped in #66.
What breaks
runner.runcalls every module with notry.tests/test_modules_never_raise.pystates the contract in its first line: a malformed record must produce a finding, never an exception.tr_sig._canonical_jsonisrfc8785.dumps, and RFC 8785 has no canonical form for three classes of value, so it raises on them:10**20IntegerDomainError1e400literalFloatDomainError"\ud800"CanonicalizationErrorAll three are ordinary JSON.
json.loadsaccepts them andload_recordhas no reason to refuse them, so this is reachable from a file rather than only from a library caller. On46c9c0e2:reportfails the same way, so it is both commands. All three exit 1, which is also what an honest FAIL exits, so a caller reading the status code cannot tell a rejected record from a crashed tool.A schema change would not close it.
jsonschemais imported nowhere undersrc/: the packaged schema is used by the tests and never by the tool, sorunner.runreaches these values whatever the schema says about them.invalid_canonical_plain_trace.jsonmakes the point on its own, since the packaged schema accepts it as written.Both call sites are exposed:
check_cmcp_runtime, over the whole envelope, andcheck, over the trace fields. At46c9c0e2those are lines 129 and 183; this branch moves them, so the function names are the durable reference. Across the eight vectors, 20 wrong-typed values and 240 nested paths, 87 paths raise this way; after this, none do. The 30 that remain are theTypeErrorclass #85 addresses, plus the library-caller pathtest_modules_never_raise.pydiscloses in its docstring, and this PR touches neither.It came in with #66, which is mine, and that is measured
At
c725bbb^all three records verify toResult: FAIL (7 checks, 2 failures, 0 skipped). Atc725bbbthey traceback.That commit replaced
json.dumps(sort_keys=True, separators=(",", ":"), ensure_ascii=True)withrfc8785.dumps, which was correct and which §3.2.2 requires in as many words. Butjson.dumpsserialises all three of these without complaint: it emitsNaN,Infinityand large integers as they are. Making the canonicalizer correct made it strict, and strict inside a function whose callers may not raise is a failure mode the correctness argument did not cover.The fix
_canonical_jsonstill raises.tests/test_canonicalization_boundary.pycompares its bytes directly againstrfc8785.dumps, and a wrapper that swallowed the error would hide a real serializer defect. A value JCS has no form for genuinely has no bytes to return._canonical_bodyis the boundary between the two contracts. Callers that owe a verdict use it, and it returns the reason rather than only the failure.IntegerDomainErrorandFloatDomainErrorboth derive fromCanonicalizationError, and all fiveraisesites inrfc8785._implare that class or a subclass, so one clause covers every way it can refuse. I checked the five rather than the three I had provoked.The finding says the record has no canonical form. It deliberately does not say verification failed:
Those are different facts, and a consumer acting on the second would go looking for a key problem that is not there. The codes follow
docs/error-codes.mdas written: a cmcp claim reports its signature outcome under TR-SIG-001, a plain TRACE record under TR-SIG-005.Why nothing caught it, and what now does
Every one of the nine values
JUNKcarried serialises throughrfc8785without complaint, checked one at a time.test_modules_never_raise.pycould not see this class by construction, so no number of runs of it would have found this. The lens had to change, not the number of passes through it.The three values are added to
JUNKfirst. That is the general guard, and it goes red before any fix, ontr_sigalone: the other six modules take all three without raising.tests/test_canonicalization_refusals.pyis the specific guard. It asserts the verdict rather than only the absence of an exception, because a module that returned nothing at all would satisfy the general one. Four vectors, one per class plus one on the plain-trace call site so both branches of the fix are covered, loaded from disk throughload_recordbecause reachability from a file is the whole point.Each vector is first checked for still provoking its own
rfc8785error, before anything is asserted about the verdict. A vector edited into serialisability would otherwise leave every later assertion passing over a record that exercises nothing. There is also a control onvalid_cmcp_runtime.json, so a change that made TR-SIG fail on everything would not pass this file.With the fix reverted, 8 assertions in this file fail, and the widened
JUNKtakestest_modules_never_raise.pyred alongside them.Measurements
46c9c0e2rfc8785error, 8 vectors x 20 values x 240 pathsruff check src/ tests/mypy src/The 13 added tests are the whole difference in the suite. Both lint counts are unchanged and every one of them is pre-existing;
ruffreports 6 intr_sig.pyon both trees and none in either file added here.Reproduced from a fresh clone of the branch into a new virtualenv, running the three lanes
ci.ymlruns, verbatim:133 passed, 269 deselected,132 passed,397 passed, 5 xpassed. The transcripts above are the installedtrace-testsconsole script rather than a module invocation, and that script exists at46c9c0e2too, so both halves of every before and after are runnable as printed.