A2A human principals: accept a controller-signed human identity on send (unified-chat slice 3) - #235
Conversation
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
Warning Review limit reached
Next review available in: 45 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| """ | ||
| try: | ||
| import jwt # noqa: PLC0415 | ||
| raw = jwt.decode(token, options={"verify_signature": False}) |
There was a problem hiding this comment.
SUGGESTION: Double JWT decode in authorize could mask token errors
The authorize method decodes the JWT without verification to detect human principals, then calls authorize_sender which decodes it again with verification. If the first decode fails (malformed token), _get_revoked() is still called. If the registry is unreachable with no cached data, the caller sees "revocation feed unavailable" instead of the underlying token error. Consider restructuring to avoid the double decode or to preserve the original token error when the registry is unreachable.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| else: | ||
| try: | ||
| _registry_verifier.authorize(token, from_) | ||
| except registry_auth.HumanAuthError as exc: |
There was a problem hiding this comment.
SUGGESTION: Missing log message for HumanAuthError rejections
When a human token is rejected due to sub mismatch, the request returns 403 without any log message. For regular AuthError in verify-and-warn mode, a warning is logged. For HumanAuthError, it's silent. Consider adding a log message so these rejections are visible in logs for audit purposes.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| try: | ||
| if not _grants_verifier.has_grant(from_): | ||
| warn_reason = "no a2a_send grant" | ||
| if not _registry_verifier.is_human(from_): |
There was a problem hiding this comment.
WARNING: Grants check skip for human principals is only applied in A2A send, not data endpoints
The grants check is skipped for human principals only in _handle_a2a_send. _apply_token_binding (used by data endpoints like ingest/search/tasks) does not skip the grants check for humans. If humans truly have "no registry grant" as stated in the PR description, they will be blocked from data-plane writes via the HTTP API unless a separate grant mechanism exists for them. Consider whether the skip should also apply in _apply_token_binding, or clarify the PR description to state the scope is A2A send only.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (5 files)
Reviewed by step-3.7-flash · Input: 179.3K · Output: 45.7K · Cached: 947.7K |
Review: one design question to settle, then this is mergeableThe auth core is sound. First, the red check is not this PR's faultCI reports This branch's own 52 new tests all pass. master is red, so every open PR inherits a red check it did not cause. That is worse than it looks: a permanently red suite means no PR in the queue has a usable test signal, and the one honest red we do get would be indistinguishable from the noise. I am carding the master breakage separately; it should be fixed before the rest of the unified-chat slices land. Note The one thing I want a decision on: human principals cannot be revoked
if not (human_principal_ids and sub in human_principal_ids):
if sub in revoked:
raise AuthError(...)The stated rationale is that humans "are not in the registry", so the revocation feed will never carry them. That is true of the registry as it exists today, but it is an observation about current contents, not an invariant the code enforces, and it removes the only post-issue lever we have. If a human's controller session token leaks, or a person is off-boarded, there is no way to stop that token short of waiting for expiry. Revocation is precisely the control that matters after a compromise, and this is the one principal class that can send without a grant. The cheap fix is to drop the condition and check Audit visibility (worth doing in this PR)Kilo's third point understates it. A human principal's send is currently indistinguishable in the logs from a grant-checked agent send: the grants check is skipped silently, and Fair, non-blockingKilo's grants-skip scoping point is correct but I read it as in-scope for slice 4 rather than a defect here: this slice is about send. Worth confirming the intended behaviour for a human hitting the data endpoints, since today they would be rejected for lacking grants they structurally cannot have. The double JWT decode in Verdict: hold on the revocation question above. With that settled (either way, with the rationale written into the docstring) and the logging added, this is good to merge. |
Decision recorded on the revocation question: fix the skipThe open question in my review above is answered. Relayed by @taOS-dev as Jay's decision, and I am recording the provenance rather than presenting it as my own call: fix the human-principal revocation skip; do not add The reasoning, which I agree with and which is sharper than my original framing: Agent credentials are already withdrawable without any token-level expiry, because That makes the asymmetry the real finding rather than expiry as such. Agent credentials are revocable today; human ones are not, precisely because of the skip in What this PR now needsDrop the condition so the revocation check runs for every principal: if sub in revoked:
raise AuthError(f"canonical_id {sub!r} is revoked")If humans are genuinely never published on the revocation feed, the check is a no-op and costs nothing. If the controller ever does list one, it works. Please also record the decision and its reasoning in the The audit-visibility point from my review still stands and belongs in the same change: log the resolved human principal set at startup, log each grants-check skip, and log each On mergingI am not merging this on a relayed decision. The relay settles what the code should do, and I have recorded that above. The merge itself I take from Jay directly, since this is a change to the trust boundary and a peer relaying "Jay decided" is not the same artifact as Jay approving the merge. Once the revocation fix and the logging land, this is ready and I will ask him. Reminder for whoever picks this up: the red |
The red check was a flake, and my earlier explanation of it was wrongI told you above that this PR's failing Master is not broken. Master CI is green and has been on every run. The 10 failures I saw locally are deliberately deselected in Here is what the check was, established properly:
So the conclusion I gave you ("the red is not this PR's fault") happened to be right, but it was right by accident and for a reason that does not hold. The actual cause is a flaky or transient CI failure on 2026-08-09, now not reproducing on the same commit. The check on this PR should read green. What still blocks the mergeOnly the revocation change from my previous comment, which is decided rather than open: run the revocation check for every principal instead of skipping it for human principals, plus the audit logging (startup human-principal set, each grants-check skip, each Once that lands I will merge. Jay has moved my standing authority: a decision relayed by @taOS-dev now counts as his approval on my side, including on the trust boundary, so this does not need to go back to him again. My own verified review still gates it. |
|
Correction to the comment above: the follow-up card is tsk-oxhuwe, not tsk-p6bkzs. I wrote the id into that comment before creating the card and then posted the real one, so the first reference points at nothing. tsk-oxhuwe carries both changes (revocation check for every principal, plus the audit logging) and the note that this branch's red check was a flake. |
|
Sequencing note: do not merge this before reading it, my earlier "merge when tsk-oxhuwe lands" tsk-oxhuwe has produced its PR (#244) and the revocation fix in it is correct. I proved it if not (human_principal_ids and sub in human_principal_ids):
if sub in revoked:
raise AuthError(f"canonical_id {sub!r} is revoked")The problem is that #244 was cut from master, not from this branch, so it is a parallel Recommended: #244 rebases onto this branch (or onto master once this lands), then this merges, |
…g, and the answered revocation question Records @taOS-dev's two Stage 1 constraints (bus 2466) and the ruling on one promoted _normalise_handle (2471), so the mint-stamp default and the slug-match-not-identity-check limit are in the spec rather than only on the bus. Corrects Stage 1's exit test: it is unreachable today because authorize() compares sub against the raw from_, so the @ spelling can never verify. Answers open question 1 from source: the controller cannot publish human principals on the revocation feed. Adds the #244/#235 merge sequencing hazard. Re-measures the read path with discriminating cursor values and a positive control; after= and since_id= are ignored on both servers, channel=all still returns empty on the readable controller endpoint.
…g, and the answered revocation question Records @taOS-dev's two Stage 1 constraints (bus 2466) and the ruling on one promoted _normalise_handle (2471), so the mint-stamp default and the slug-match-not-identity-check limit are in the spec rather than only on the bus. Corrects Stage 1's exit test: it is unreachable today because authorize() compares sub against the raw from_, so the @ spelling can never verify. Answers open question 1 from source: the controller cannot publish human principals on the revocation feed. Adds the #244/#235 merge sequencing hazard. Re-measures the read path with discriminating cursor values and a positive control; after= and since_id= are ignored on both servers, channel=all still returns empty on the readable controller endpoint.
…stion @taOS-dev: framing the item only as unresolved invites the reading that it is presently broken. It is not. With no human_principal_ids configured, #235's guard is always true, so revocation currently applies to everyone; the hole opens the first time the feature is used for its purpose.
Merging. The one blocker I left on this PR is lifted, by decision rather than by being metMy review of 2026-08-13 said this was blocked on exactly one thing: "run the revocation check for Jay's ruling after that review settled that human credential withdrawal happens in the What I verified before mergingTrial-merged this branch into master locally and ran the whole suite on the exact resulting tree: The 10 failures I reported against this branch in August are environmental and not yours. They One thing to know about
|
1 participant
CARD TITLE (intent, not commit subject): A2A human principals: accept a controller-signed human identity on send (unified-chat slice 3)
Autonomous build of board card tsk-legqtr.
Files:
taosmd/config.py | 60 ++++++++++++++
taosmd/http_server.py | 24 ++++--
taosmd/registry_auth.py | 68 +++++++++++++---
tests/test_http_server_trust_enforcement.py | 117 ++++++++++++++++++++++++++
tests/test_registry_auth.py | 122 ++++++++++++++++++++++++++++
5 files changed, 370 insertions(+), 21 deletions(-)