feat(a2a-bridge): wire inbound HTTPS route, production main, and NATS transport harness - #382
Merged
Merged
Conversation
yordis
commented
Jun 21, 2026
yordis
commented
Member
- With identity, auth-mint, and outbound already merged, the inbound HTTPS surface is what the bridge needs to become deployable end-to-end; landing the harness alongside it so the new request paths get exercised in CI under the `stub` transport without standing up a real NATS server.
PR SummaryHigh Risk Overview Inbound surface: Runtime: Tests: In-process NATS transport harness exercises mint-per-request, subject routing, audit publishes, and SSE resubscribe without a live server; optional ignored compose smoke. Reviewed by Cursor Bugbot for commit 6007d3a. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Warning Review limit reached
More reviews will be available in 30 minutes and 11 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 (3)
WalkthroughA new ChangesInbound HTTP-to-NATS Gateway
Sequence Diagram(s)sequenceDiagram
participant Client
participant handle_jsonrpc
participant AuthCallout as Auth Callout (JWT Mint)
participant InboundGatewayPublish as NATS Unary Publisher
participant TaskJetStreamPort as JetStream SSE Port
Client->>handle_jsonrpc: POST /jsonrpc (HTTP headers + body)
handle_jsonrpc->>AuthCallout: mint per-request caller JWT
AuthCallout-->>handle_jsonrpc: BridgeUserJwt
handle_jsonrpc->>handle_jsonrpc: parse method, correlation id, build NATS subject
handle_jsonrpc->>InboundGatewayPublish: publish(subject, headers w/ JWT + correlation, body)
InboundGatewayPublish-->>handle_jsonrpc: JSON-RPC response bytes
alt is_sse_method (message/stream or tasks/resubscribe)
handle_jsonrpc->>TaskJetStreamPort: consume_stream(SseConsumePlan)
TaskJetStreamPort-->>handle_jsonrpc: async task event stream
handle_jsonrpc-->>Client: text/event-stream (bootstrap event + task events)
else unary method
handle_jsonrpc-->>Client: JSON response body
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 6007d3a Minimum allowed coverage is ♻️ This comment has been updated with latest results |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
rsworkspace/crates/a2a-bridge/src/nats_transport_harness.rs (1)
129-136: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueReuse the tenant variable to eliminate duplication.
BridgeTenantAccount::new(HARNESS_TENANT).expect("harness tenant")is created twice. Thetenantvariable from line 129 can be cloned and reused on line 135.♻️ Proposed refactor to reuse tenant
let tenant = BridgeTenantAccount::new(HARNESS_TENANT).expect("harness tenant"); let dispatcher = Arc::new(harness_callout_dispatcher(HARNESS_CALLER_ID)); let mint_wire = Arc::new(InProcessCalloutDispatcherMintWire::new(dispatcher, tenant.clone())); let auth = Arc::new(AuthCalloutJsonMintClient::with_tenant_account( mint_wire.clone(), AuthCalloutJsonMintClient::<InProcessCalloutDispatcherMintWire>::default_mint_subject(), - Some(BridgeTenantAccount::new(HARNESS_TENANT).expect("harness tenant")), + Some(tenant), ));🤖 Prompt for 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. In `@rsworkspace/crates/a2a-bridge/src/nats_transport_harness.rs` around lines 129 - 136, The BridgeTenantAccount::new(HARNESS_TENANT).expect("harness tenant") is being created twice - once on line 129 and stored in the tenant variable, and again inline in the AuthCalloutJsonMintClient::with_tenant_account call on line 135. Replace the inline creation in the AuthCalloutJsonMintClient::with_tenant_account method call with Some(tenant.clone()) to reuse the already-created tenant variable and eliminate the duplication.rsworkspace/crates/a2a-bridge/src/main.rs (1)
116-119: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winError context discarded by stringifying the connect error.
The typed
async_natsconnection error is converted to a string viaformat!(), losing the source error chain. Per coding guidelines, wrap source errors as fields/variants rather than stringifying them.This likely requires
BridgeError::NatsConnect(or similar) to hold a#[source]field, which may be outside this file's scope.🤖 Prompt for 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. In `@rsworkspace/crates/a2a-bridge/src/main.rs` around lines 116 - 119, The connect error from the connect_opts.connect() call is being stringified via format!(), which discards the error chain and source information. Replace the map_err block that uses BridgeError::NatsPublish with a new typed error variant (such as BridgeError::NatsConnect) that includes a #[source] field to hold the original async_nats connection error. This preserves the full error context for debugging purposes instead of converting it to a string.Source: Coding guidelines
🤖 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-bridge/src/nats_transport_harness.rs`:
- Around line 70-76: The code silently ignores lock failures when recording test
state for self.last_subject and self.last_caller_jwt_present by using if let Ok
patterns, which causes stale or empty data when mutexes are poisoned, leading to
confusing test assertion failures. Replace the if let Ok patterns with .expect()
or .unwrap() calls on both the self.last_subject.lock() and
self.last_caller_jwt_present.lock() operations to panic immediately when lock
acquisition fails, ensuring test failures are explicit and obvious rather than
silently corrupted.
- Around line 50-58: The last_caller_jwt_present and last_subject methods are
silently handling mutex lock failures by returning false and None respectively
using .ok(), which masks poisoned mutexes that indicate panics elsewhere.
Replace the .ok() calls with .expect() or .unwrap() in both methods to make lock
failures panic immediately, ensuring test harness code surfaces mutex poisoning
issues instead of silently treating them as successful "no data" states. This
applies to both the last_caller_jwt_present method and the last_subject method
where they acquire locks on self.last_caller_jwt_present and self.last_subject
respectively.
---
Nitpick comments:
In `@rsworkspace/crates/a2a-bridge/src/main.rs`:
- Around line 116-119: The connect error from the connect_opts.connect() call is
being stringified via format!(), which discards the error chain and source
information. Replace the map_err block that uses BridgeError::NatsPublish with a
new typed error variant (such as BridgeError::NatsConnect) that includes a
#[source] field to hold the original async_nats connection error. This preserves
the full error context for debugging purposes instead of converting it to a
string.
In `@rsworkspace/crates/a2a-bridge/src/nats_transport_harness.rs`:
- Around line 129-136: The
BridgeTenantAccount::new(HARNESS_TENANT).expect("harness tenant") is being
created twice - once on line 129 and stored in the tenant variable, and again
inline in the AuthCalloutJsonMintClient::with_tenant_account call on line 135.
Replace the inline creation in the
AuthCalloutJsonMintClient::with_tenant_account method call with
Some(tenant.clone()) to reuse the already-created tenant variable and eliminate
the duplication.
🪄 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: dc002f95-9b63-4691-b436-39c011f0a131
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
rsworkspace/crates/a2a-bridge/Cargo.tomlrsworkspace/crates/a2a-bridge/src/auth.rsrsworkspace/crates/a2a-bridge/src/inbound.rsrsworkspace/crates/a2a-bridge/src/lib.rsrsworkspace/crates/a2a-bridge/src/main.rsrsworkspace/crates/a2a-bridge/src/nats_transport_harness.rs
… transport harness With the typed identity, auth-mint client, and outbound forwarder in place, the inbound HTTPS surface is the last shape the bridge needs to become deployable end-to-end. Landing the harness alongside the inbound module so the new request paths get exercised in CI under the \`stub\` transport without standing up a real NATS server. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
For streaming requests with absent/null JSON-RPC id, the correlation id was being minted twice — once for the gateway publish headers and once for the JetStream consume plan — so the gateway unary call and the event consumer used different ReqIds and SSE silently delivered no task events. Separately, BRIDGE_GATEWAY_RPC_TIMEOUT_SECS only configured async_nats connection_timeout, leaving request_with_headers unbounded; a hung gateway responder could block the inbound HTTPS request indefinitely while the configured RPC timeout was ignored. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…-task cleanup For streaming JSON-RPC the gateway unary publish raced ahead of the JetStream consumer; events emitted before the consumer existed were silently dropped by the events stream's interest retention. The pull loop also acked each task event before enqueuing it, so a client disconnect lost the in-flight event with no chance of redelivery, and the pull task itself stayed alive after the SSE stream dropped, keeping its NATS connection and ephemeral consumer pinned. Test harness mutex accessors swallowed poison errors as 'no data', which hid the real cause when assertions failed. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…eway errors
The streaming path turned every gateway JSON-RPC error envelope into a
HTTP 502 with a bare {\"error\": \"...\"} body, while the unary path (and
a2a-nats-http) forwarded the gateway's JSON-RPC error as HTTP 200 with
the envelope intact. The asymmetry made the same failure mode look
like a bridge transport failure on streaming methods and like an
application-level JSON-RPC error on unary methods.
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
force-pushed
the
yordis/feat-a2a-bridge-inbound
branch
from
June 22, 2026 01:41
33a7458 to
b4add05
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
rsworkspace/crates/a2a-bridge/src/inbound.rs (3)
49-62: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider accepting domain types instead of primitive strings.
build_gateway_subjecttakesagent_id: &strandtask_events_wild_subjecttakestask_id: &str, but callers already hold validated domain types (BridgeAgentId,A2aTaskId). Accepting the domain types directly avoids re-exposing raw strings and keeps the type-safety boundary tighter.Suggested change
#[must_use] -pub fn build_gateway_subject(prefix: &A2aPrefix, agent_id: &str, method: &str) -> String { +pub fn build_gateway_subject(prefix: &A2aPrefix, agent_id: &BridgeAgentId, method: &str) -> String { format!( "{}.gateway.{}.{}", prefix.as_str(), - agent_id, + agent_id.as_str(), gateway_method_to_subject_dots(method) ) } #[must_use] -pub fn task_events_wild_subject(prefix: &A2aPrefix, task_id: &str) -> String { +pub fn task_events_wild_subject(prefix: &A2aPrefix, task_id: &A2aTaskId) -> String { format!("{}.task.{}.events.>", prefix.as_str(), task_id.as_str()) }🤖 Prompt for 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. In `@rsworkspace/crates/a2a-bridge/src/inbound.rs` around lines 49 - 62, The functions build_gateway_subject and task_events_wild_subject are accepting primitive string types for agent_id and task_id parameters respectively, but callers already hold validated domain types (BridgeAgentId and A2aTaskId). Update the parameter types in build_gateway_subject to accept agent_id: &BridgeAgentId instead of &str, and in task_events_wild_subject to accept task_id: &A2aTaskId instead of &str. Then update the format! calls within these functions to extract the string representation from these domain types (likely using .as_str() or similar accessor methods) to maintain the same formatting logic.Source: Coding guidelines
498-508: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRedundant re-validation of already-validated JWT.
BridgeUserJwt(fromauth.mint()) already validates compact-JWT shape at construction (seeidentity.rs:67-93). Re-validating viaMintedUserJwt::new(caller_jwt.as_str())on line 503 duplicates that check and forces an error conversion toString, losing typed context.Consider adding an infallible conversion (e.g.,
impl From<&BridgeUserJwt> for MintedUserJwt) or a shared validated representation so the second validation is unnecessary.Alternative: infallible conversion
If both types validate the same shape,
BridgeUserJwtcould expose:impl BridgeUserJwt { pub fn as_minted(&self) -> MintedUserJwt { // Safety: BridgeUserJwt construction already validated shape MintedUserJwt::new_unchecked(self.as_str()) } }Then in
gateway_publish_headers:- let minted = MintedUserJwt::new(caller_jwt.as_str()).map_err(|e| BridgeError::Mint(e.to_string()))?; + let minted = caller_jwt.as_minted();🤖 Prompt for 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. In `@rsworkspace/crates/a2a-bridge/src/inbound.rs` around lines 498 - 508, The gateway_publish_headers function is performing redundant validation by calling MintedUserJwt::new on a BridgeUserJwt that was already validated at construction. Add an infallible conversion method to BridgeUserJwt (such as as_minted) that returns a MintedUserJwt without re-validation, then replace the MintedUserJwt::new(caller_jwt.as_str()).map_err(...) call with this new method to eliminate the duplicate validation check and the error conversion to String that loses typed context.Source: Coding guidelines
213-231: 🧹 Nitpick | 🔵 Trivial | 💤 Low valuePer-request NATS connection has performance implications.
Each
unary_request_gatewaycall creates a new NATS connection with the caller's JWT. This is architecturally correct for per-user auth but expensive under load. Consider documenting this trade-off or adding a comment explaining why connection pooling isn't viable here (per-request JWT auth requires per-request connections in NATS's connection-level auth model).🤖 Prompt for 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. In `@rsworkspace/crates/a2a-bridge/src/inbound.rs` around lines 213 - 231, The NATS connection creation in the unary_request_gateway method lacks documentation about the performance trade-off. Add a comment before the async_nats::ConnectOptions::new() call explaining that a new connection is created per request to support per-user JWT authentication, and clarify that connection pooling is not viable here because NATS's connection-level auth model requires separate connections for different JWT tokens. This will help future maintainers understand the architectural decision and the performance implications trade-off.
🤖 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-bridge/src/inbound.rs`:
- Around line 654-672: The Response::builder() error handling is incorrectly
using BridgeError::NatsPublish instead of a more appropriate error type for HTTP
response construction failures. First, add a new error variant to the
BridgeError enum in error.rs such as ResponseBuild that describes HTTP response
building failures. Then, locate both occurrences in the inbound.rs file where
Response::builder() chains end with .map_err(|e|
BridgeError::NatsPublish(e.to_string())) and change them to use the new
ResponseBuild variant instead, ensuring the error attribution correctly reflects
that these are HTTP response construction failures, not NATS publishing
failures.
---
Nitpick comments:
In `@rsworkspace/crates/a2a-bridge/src/inbound.rs`:
- Around line 49-62: The functions build_gateway_subject and
task_events_wild_subject are accepting primitive string types for agent_id and
task_id parameters respectively, but callers already hold validated domain types
(BridgeAgentId and A2aTaskId). Update the parameter types in
build_gateway_subject to accept agent_id: &BridgeAgentId instead of &str, and in
task_events_wild_subject to accept task_id: &A2aTaskId instead of &str. Then
update the format! calls within these functions to extract the string
representation from these domain types (likely using .as_str() or similar
accessor methods) to maintain the same formatting logic.
- Around line 498-508: The gateway_publish_headers function is performing
redundant validation by calling MintedUserJwt::new on a BridgeUserJwt that was
already validated at construction. Add an infallible conversion method to
BridgeUserJwt (such as as_minted) that returns a MintedUserJwt without
re-validation, then replace the
MintedUserJwt::new(caller_jwt.as_str()).map_err(...) call with this new method
to eliminate the duplicate validation check and the error conversion to String
that loses typed context.
- Around line 213-231: The NATS connection creation in the unary_request_gateway
method lacks documentation about the performance trade-off. Add a comment before
the async_nats::ConnectOptions::new() call explaining that a new connection is
created per request to support per-user JWT authentication, and clarify that
connection pooling is not viable here because NATS's connection-level auth model
requires separate connections for different JWT tokens. This will help future
maintainers understand the architectural decision and the performance
implications trade-off.
🪄 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: 85cefb35-5b4e-4abc-8244-a4c0bc0c0e6f
⛔ Files ignored due to path filters (1)
rsworkspace/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
rsworkspace/crates/a2a-bridge/Cargo.tomlrsworkspace/crates/a2a-bridge/src/auth.rsrsworkspace/crates/a2a-bridge/src/inbound.rsrsworkspace/crates/a2a-bridge/src/lib.rsrsworkspace/crates/a2a-bridge/src/main.rsrsworkspace/crates/a2a-bridge/src/nats_transport_harness.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- rsworkspace/crates/a2a-bridge/src/auth.rs
- rsworkspace/crates/a2a-bridge/src/lib.rs
- rsworkspace/crates/a2a-bridge/src/nats_transport_harness.rs
- rsworkspace/crates/a2a-bridge/src/main.rs
- rsworkspace/crates/a2a-bridge/Cargo.toml
…l-error-impl policy The repo's dylint policy denies hand-rolled std::error::Error impls and requires #[derive(thiserror::Error)], which the binary's bootstrap error escaped from the original copy of main.rs. Replacing the manual Display + Error + From<BridgeError> with thiserror attributes keeps the same wire shape (source chain, From conversion) while clearing the policy lint. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
…ag ResponseBuild errors The inbound path was dropping the external caller identity on the floor — gateway spans, audits, and caller-scoped routing only saw the minted user JWT, not the caller advertised by the HTTP client. The tasks/resubscribe cursor extractor also ignored 'lastSeq' (the canonical name used by a2a-nats-http/stdio) and the older 'metadata.lastEventId' SSE form, so reconnecting clients silently resumed from sequence 0 and either replayed or skipped events. Lastly, Response::builder() failures were misattributed to NatsPublish, which read like a transport problem in logs/metrics; routing them through a new ResponseBuild variant keeps the source of failure honest. 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 c168158. Configure here.
Production bootstrap always built AppState with default_a2a_prefix
('a2a'), so a non-default deployment would publish gateway subjects
and JetStream stream names that did not match a2a-nats-http,
a2a-nats-stdio, or a2a-nats-server in the same stack — the rest of
the stack reads ENV_A2A_PREFIX, the bridge silently didn't.
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.