fix(intent-bridge): preserve canonicalization error classification - #318
fix(intent-bridge): preserve canonicalization error classification#318altrudev wants to merge 2 commits into
Conversation
|
🔴 Contributor Check: HIGH
Automated check by AgenTrust Contributor Check. |
lywinged
left a comment
There was a problem hiding this comment.
This is an approval waiting on one addition. Reviewed at 63ae280 on a247244. It is posted as a comment rather than an approval because the PR is a draft and the addition below will move the head, and the maintainer-approval check only counts an approval on the commit it was given on; I will approve the head that carries the entry once you mark it ready.
The defect is real and the fix is the right shape. On main, a signed and digested call of {"approved": 1} with a transcript before.tool_call of {"approved": true} passes the transcript check; on this head it is refused. It is the identity relation the #300 review asked for in the tool-schema check, applied here where the transcript is compared: the signature covers RFC 8785 bytes, so comparing the two calls as _jcs bytes puts the transcript under the same relation as the digest it sits beside.
What I measured rather than read:
- Five lines in and one out in
intent_bridge.py, reusing the module's own_jcswrapper and thecompare_digestit already imports. - The five new tests pass. With the old comparison restored, four fail and the unchanged-call control passes.
- The four CI steps with the pinned install, on 3.11 and on 3.12: green, 1407 passed and 1 skipped.
- The new canonicalisation step cannot leak. A transcript call carrying 2^60 or NaN raises
IntentBridgeErrorfrom_jcs, becauseIntegerDomainErrorandFloatDomainErrorsubclassCanonicalizationErrorin the pinnedrfc87850.1.4.
One thing the body does not say, and the entry below should. "The existing AuthorizationMismatch result and diagnostic are preserved" holds for a call that differs. For a transcript call JCS cannot serialise, the exception moves: main raised AuthorizationMismatch from the inequality, this head raises IntentBridgeError from _jcs before the comparison is reached. Same documented base type, and the treatment the authorization, the declaration and the executed call already get, so nothing to change in the code.
The one thing to add: a CHANGELOG.md entry under Unreleased, Fixed. Both code fixes already there carry one, and it is the record a reader of the next release will have. The shape the others use: what was accepted, what is now refused, that the signed digest was never affected, and the exception note above.
80dbb2e to
ac1a08f
Compare
|
Second conflict in an hour, same file, and this one is mine again: merging #322 re-conflicted #318 and #323 on the Please drop the I will write the changelog entries for all of them in one maintainer commit once they land, using the wording from your PR bodies. You lose nothing: the entries still say what your changes did, and the attribution is in the commits and the PRs. Why this rather than another rebase. Every open PR in this repository appends to the same block, so each merge conflicts every other one, and the number of rebases grows with the size of the queue rather than with the size of anyone's change. Two of your PRs have now paid for that twice. The durable fix is a fragment directory, one file per change assembled at release, and I am looking at it separately.
|
|
You opened this three minutes after filing #317 and it says "Closes #317". I wrote the same fix from scratch today and merged it as #333 without checking whether one was already open. That is my error, not a race: this PR has been sitting here since the 10th, and I had its number in front of me. On the difference between the two, you are right and I was wrong. Your changelog entry argues that a transcript call the pinned canonicalizer has no form for should raise The class docstrings settle it against me. So there is still unique content here and I would rather it landed as yours. The whole of it is now that classification: drop the Your byte comparison is also the better construction. |
8bd9bb6 to
6737005
Compare
Signed-off-by: altrudev <266135212+altrudev@users.noreply.github.com>
Signed-off-by: altrudev <266135212+altrudev@users.noreply.github.com>
|
Rebuilt cleanly on current main after #325/#333. The PR now contains only the remaining classification correction plus its regression coverage: non-object transcript calls stay |
Follow-up to #317 / #333.
What
#333 correctly moved the required transcript-call comparison off Python container equality, but it wraps an RFC 8785 canonicalization failure from
transcript.before.tool_callback intoAuthorizationMismatch.That classification is inconsistent with this module's own contracts:
IntentBridgeError: malformed, untrusted, stale, or cannot be evaluated.AuthorizationMismatch: execution evidence does not match the signed authorization.A transcript call with no RFC 8785 canonical form cannot be evaluated; it has not been shown to mismatch.
This PR removes that wrap and lets the existing
_jcs()boundary raiseIntentBridgeError, while preserving the explicit object-type guard so a non-object transcript call remainsAuthorizationMismatch.The comparison is performed directly over canonical bytes with
compare_digest(_jcs(a), _jcs(b)). The signedtool_call_digestsemantics are unchanged.Regression coverage
Existing coverage from #333 continues to pin:
AuthorizationMismatch;AuthorizationMismatch.This PR adds explicit cases for transcript calls containing values the pinned canonicalizer cannot represent (
2**60andNaN) and requires:IntentBridgeError;AuthorizationMismatch;transcript.before.tool_call.Scope
No schema, wire-format, authorization, scope, digest, or signature semantics change.
This is only an exception-classification correction at the canonicalization boundary, plus the direct canonical-byte comparison already discussed on this PR.
The branch is rebuilt directly on current
mainafter #325 and #333. NoCHANGELOG.mdhunk, per maintainer request.AI-assistance disclosure: ChatGPT assisted with repository triage, diff reconstruction, test drafting, and branch cleanup.
altrudevreviewed the bounded claim and remains responsible for the contribution.