feat(a2a-auth-callout): subscriber - #375
Merged
Merged
Conversation
Wires the dispatcher to a NATS subscription on \$SYS.REQ.USER.AUTH: decodes the wire envelope through AuthCalloutWireCodec, dispatches through the verifier chain, mints the response, and publishes it back. The final piece before the real bin entrypoint. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
PR SummaryHigh Risk Overview Denials on the wire use only opaque Reviewed by Cursor Bugbot for commit 72cb236. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Warning Review limit reached
More reviews will be available in 20 minutes and 5 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. 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, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
WalkthroughA new ChangesNATS auth-callout subscriber
Sequence Diagram(s)sequenceDiagram
participant NATSServer
participant Subscriber
participant AuthDispatcher
participant AuthCalloutWireCodec
NATSServer->>Subscriber: message on $SYS.REQ.USER.AUTH
Subscriber->>Subscriber: validate reply subject exists
Subscriber->>AuthCalloutWireCodec: decode_request(payload)
alt decode fails
Subscriber->>NATSServer: publish empty bytes to reply inbox (fast-fail)
else decode succeeds
Subscriber->>AuthDispatcher: dispatch(request) [tokio::spawn]
alt dispatch succeeds
AuthDispatcher->>AuthCalloutWireCodec: encode_success(response)
AuthCalloutWireCodec->>NATSServer: publish encoded success to reply subject
else dispatch fails
AuthDispatcher->>AuthCalloutWireCodec: encode_denial(error_string)
AuthCalloutWireCodec->>NATSServer: publish encoded denial to reply subject
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Code Coverage SummaryDetailsDiff against mainResults for commit: 72cb236 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Plain subscribe on \$SYS.REQ.USER.AUTH delivers each request to every replica, and they all publish to the same reply subject — multiple authorization replies race for one server request and client connects fail or behave unpredictably. Switch to queue_subscribe with a queue group name (operator-overridable via AUTH_CALLOUT_QUEUE_GROUP, default 'a2a-auth-callout') so NATS load-balances requests across replicas and only one reply lands. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…cts fail fast The subscriber was logging-and-continuing when wire decode failed, leaving the reply inbox open — the NATS server kept waiting and the client's connect stalled until server-side timeout instead of being denied immediately. Publish an empty payload on the reply subject so nats-server treats it as a malformed authorization response and denies the connect right away. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…/denial fails Symmetric with the decode-failure path: if encoding the success response or denial JWT fails after dispatch (e.g. signing key hiccup, JSON edge case), publish an empty payload on the reply inbox so nats-server treats it as malformed and denies the connect right away. Without that, the client connect just hangs until server-side timeout. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rsworkspace/crates/a2a-auth-callout/src/subscriber.rs`:
- Around line 155-158: In the client.publish() error handling block, you are
converting the PublishError to a string with e.to_string() in the map_err
closure. Instead, wrap the actual async_nats::PublishError directly in the
AuthCalloutError::Reply variant by passing the error object itself rather than
its string representation. This preserves the full error context instead of
losing it through string conversion.
- Line 65: The map_err call in the subscriber at the Subscribe error handling is
converting the error to a string using e.to_string(), which discards the
original error context. Instead of converting the error to a string, modify the
AuthCalloutError::Subscribe variant to accept and store the original
async_nats::SubscribeError directly (or wrap it in a boxed error if needed for
type compatibility). Update the map_err closure to pass the original error
object instead of its string representation, and ensure the AuthCalloutError
enum variant definition is updated accordingly to hold the typed or boxed error
rather than a String.
- Around line 141-144: In the `.map_err()` call for the `client.publish()`
operation, the error is being converted to a string with `.to_string()` before
wrapping it in `AuthCalloutError::Reply`. Instead of converting the error to
string, pass the actual `async_nats::PublishError` directly to
`AuthCalloutError::Reply` to preserve the full error context and source
information, which will help with debugging and error handling downstream.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d7ec2cef-54ce-429e-8406-2039b524219f
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
rsworkspace/crates/a2a-auth-callout/Cargo.tomlrsworkspace/crates/a2a-auth-callout/src/lib.rsrsworkspace/crates/a2a-auth-callout/src/subscriber.rs
…_nats errors - Subscriber was putting AuthCalloutError's full Display text into the denial response — exposes internal verification + configuration detail to the connecting client and diverges from the DenialCategory / DenialReason contract this crate already defines. Map the typed error through DenialCategory::from_auth_callout_error → DenialReason and send only the opaque category string on the wire; keep the full error in server-side warn logs for debugging. - AuthCalloutError::Subscribe / Reply now wrap typed async_nats::SubscribeError / PublishError so the source() chain is preserved instead of collapsing async_nats's internal kind/source into a flat string at the boundary. Display still renders an opaque message; the source chain surfaces the real cause. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…ch denial routing
DenialCategory::from_auth_callout_error was doing
msg.contains('not allowlisted'), msg.contains('not configured'),
msg.contains('scheme but') etc. on the inner CredentialVerification
string to derive the wire denial category. That coupled wire response
behavior to log message wording — any rename of an internal error
string would silently re-route the denial category.
Introduce a typed CredentialError enum (UnknownAccount,
VerifierUnavailable { scheme }, InvalidRequest, InvalidCredentials),
make AuthCalloutError::CredentialVerification wrap it, and dispatch
denial categories off the variant tag. All construction sites
(dispatcher, bridge_adapter, server_auth_request_claims, mtls, oidc,
account_resolver, api_key) updated to produce typed variants; the
From<JwtError> / From<ApiKeyError> / From<AccountResolverError>
impls route by variant, not by stringification.
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4d12805. Configure here.
…e literal The python regex that wrapped credential-error sites accidentally appended `.into()` inside the string literal before the closing quote in one site, so the rendered error message read 'authorization request missing tenant account hint (...).into()'. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
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
1 participant
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.
main, which is the final slice