From 4acbde6916308d75bfdd580705fe542c93c161be Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Thu, 13 Aug 2026 02:34:19 -0400 Subject: [PATCH 01/12] fix(acp): deliver session updates through one path only `session/update` already reaches the client through the client-op proxy for every operation, so the prompt-scoped notification consumer was a second delivery path that existed only for the duration of a prompt. Signed-off-by: Yordis Prieto --- .../acp/acp-nats-server/src/component.rs | 25 +- .../acp/acp-nats-server/src/constants.rs | 3 - .../crates/acp/acp-nats-stdio/src/main.rs | 10 +- .../crates/acp/acp-nats/src/agent/bridge.rs | 17 +- .../acp/acp-nats/src/agent/cancel/tests.rs | 1 - .../acp-nats/src/agent/initialize/tests.rs | 1 - .../crates/acp/acp-nats/src/agent/prompt.rs | 76 +----- .../acp/acp-nats/src/agent/prompt/tests.rs | 244 +----------------- .../acp/acp-nats/src/agent/test_support.rs | 2 - .../crates/acp/acp-nats/src/agent/tests.rs | 11 +- .../crates/acp/acp-nats/src/client/tests.rs | 2 - .../acp/acp-nats/src/jetstream/consumers.rs | 12 - .../acp-nats/src/jetstream/consumers/tests.rs | 34 +-- .../acp-nats/src/jetstream/provision/tests.rs | 7 +- .../acp/acp-nats/src/jetstream/streams.rs | 6 +- .../acp-nats/src/jetstream/streams/tests.rs | 17 +- rsworkspace/crates/acp/acp-nats/src/lib.rs | 15 +- .../src/nats/subjects/conformance_tests.rs | 1 - .../src/nats/subjects/responses/mod.rs | 2 - .../src/nats/subjects/responses/update.rs | 45 ---- .../nats/subjects/responses/update/tests.rs | 10 - .../acp/acp-nats/src/nats/subjects/stream.rs | 8 +- .../acp/acp-nats/src/nats/subjects/tests.rs | 10 - rsworkspace/crates/acp/acp-nats/src/tests.rs | 55 +--- .../channel-bridge-telegram/src/main.rs | 19 +- .../tests/subject_literal_policy.rs | 2 +- 26 files changed, 40 insertions(+), 595 deletions(-) delete mode 100644 rsworkspace/crates/acp/acp-nats/src/nats/subjects/responses/update.rs delete mode 100644 rsworkspace/crates/acp/acp-nats/src/nats/subjects/responses/update/tests.rs diff --git a/rsworkspace/crates/acp/acp-nats-server/src/component.rs b/rsworkspace/crates/acp/acp-nats-server/src/component.rs index a4bf579b3f..b75636a312 100644 --- a/rsworkspace/crates/acp/acp-nats-server/src/component.rs +++ b/rsworkspace/crates/acp/acp-nats-server/src/component.rs @@ -9,17 +9,14 @@ //! becomes upstream's problem. use acp_nats::boundary::{AbortOnDrop, BoundaryExit, ConnectionClient, connect_agent_boundary_with}; -use acp_nats::{agent::Bridge, client, spawn_notification_forwarder}; -use agent_client_protocol::schema::v1::SessionNotification; +use acp_nats::{agent::Bridge, client}; use agent_client_protocol::{Agent, Client, ConnectTo, Result}; use opentelemetry::metrics::Meter; use std::sync::Arc; -use tokio::sync::{mpsc, watch}; +use tokio::sync::watch; use tracing::{error, info, warn}; use trogon_std::time::SystemClock; -use crate::constants::NOTIFICATION_CHANNEL_CAPACITY; - /// Maps the client proxy's join result onto the connection's outcome. /// /// A `JoinError` means the proxy panicked or was cancelled out from under us, so @@ -92,18 +89,7 @@ where mut shutdown_rx, } = self; - // Per-connection by necessity: this channel feeds notifications to *this* - // connection's SDK handle, and the bridge owns per-connection state - // (pending prompt waiters, background tasks) keyed to that sender. - let (notification_tx, notification_rx) = mpsc::channel::(NOTIFICATION_CHANNEL_CAPACITY); - let bridge = Arc::new(Bridge::new( - nats.clone(), - js, - SystemClock, - &meter, - config, - notification_tx, - )); + let bridge = Arc::new(Bridge::new(nats.clone(), js, SystemClock, &meter, config)); info!("ACP connection established"); @@ -112,11 +98,6 @@ where async move |cx| { // Agent-to-client traffic reaches the peer through the SDK // connection handle; the NATS side never addresses it directly. - let _forwarder_guard = AbortOnDrop::new(spawn_notification_forwarder( - ConnectionClient::new(cx.clone()), - notification_rx, - )); - let mut client_task = AbortOnDrop::new(tokio::spawn(client::run( nats, Arc::new(ConnectionClient::new(cx)), diff --git a/rsworkspace/crates/acp/acp-nats-server/src/constants.rs b/rsworkspace/crates/acp/acp-nats-server/src/constants.rs index 923c0e4d49..4cff8b9609 100644 --- a/rsworkspace/crates/acp/acp-nats-server/src/constants.rs +++ b/rsworkspace/crates/acp/acp-nats-server/src/constants.rs @@ -32,6 +32,3 @@ pub(crate) const MAX_INSPECTED_BODY: usize = 1024 * 1024; /// connection, which is already how an id this layer never saw initialize is /// treated. The cost of eviction is a missed check, not a rejected request. pub(crate) const MAX_TRACKED_CONNECTIONS: usize = 4096; - -/// Matches the capacity the hand-rolled transport used for the same channel. -pub(crate) const NOTIFICATION_CHANNEL_CAPACITY: usize = 64; diff --git a/rsworkspace/crates/acp/acp-nats-stdio/src/main.rs b/rsworkspace/crates/acp/acp-nats-stdio/src/main.rs index a9e566d1b9..8e32a228e4 100644 --- a/rsworkspace/crates/acp/acp-nats-stdio/src/main.rs +++ b/rsworkspace/crates/acp/acp-nats-stdio/src/main.rs @@ -3,8 +3,7 @@ mod config; use acp_nats::boundary::{AbortOnDrop, BoundaryExit, ConnectionClient, connect_agent_boundary}; -use acp_nats::{agent::Bridge, client, spawn_notification_forwarder}; -use agent_client_protocol::schema::v1::SessionNotification; +use acp_nats::{agent::Bridge, client}; use std::sync::Arc; use tracing::{error, info}; use trogon_std::time::SystemClock; @@ -73,22 +72,15 @@ where R: futures::AsyncRead + Send + Unpin + 'static, { let meter = trogon_telemetry::meter("acp-io-bridge-nats"); - let (notification_tx, notification_rx) = tokio::sync::mpsc::channel::(64); let bridge = Arc::new(Bridge::new( nats_client.clone(), js_client, SystemClock, &meter, config.clone(), - notification_tx, )); let boundary_result = connect_agent_boundary(bridge.clone(), stdout, stdin, async move |cx| { - let _forwarder_guard = AbortOnDrop::new(spawn_notification_forwarder( - ConnectionClient::new(cx.clone()), - notification_rx, - )); - let mut client_task = AbortOnDrop::new(tokio::spawn(client::run( nats_client, Arc::new(ConnectionClient::new(cx)), diff --git a/rsworkspace/crates/acp/acp-nats/src/agent/bridge.rs b/rsworkspace/crates/acp/acp-nats/src/agent/bridge.rs index 12abb6ec87..462984fba4 100644 --- a/rsworkspace/crates/acp/acp-nats/src/agent/bridge.rs +++ b/rsworkspace/crates/acp/acp-nats/src/agent/bridge.rs @@ -14,12 +14,10 @@ use agent_client_protocol::schema::v1::{ ExtRequest, ExtResponse, ForkSessionRequest, ForkSessionResponse, InitializeRequest, InitializeResponse, ListProvidersRequest, ListProvidersResponse, ListSessionsRequest, ListSessionsResponse, LoadSessionRequest, LoadSessionResponse, LogoutRequest, LogoutResponse, NewSessionRequest, NewSessionResponse, PromptRequest, - PromptResponse, ResumeSessionRequest, ResumeSessionResponse, SessionId, SessionNotification, SetProviderRequest, - SetProviderResponse, SetSessionConfigOptionRequest, SetSessionConfigOptionResponse, SetSessionModeRequest, - SetSessionModeResponse, + PromptResponse, ResumeSessionRequest, ResumeSessionResponse, SessionId, SetProviderRequest, SetProviderResponse, + SetSessionConfigOptionRequest, SetSessionConfigOptionResponse, SetSessionModeRequest, SetSessionModeResponse, }; use opentelemetry::metrics::Meter; -use tokio::sync::mpsc; use tokio::task::JoinHandle; use tracing::{info, warn}; use trogon_nats::jetstream::{JetStreamGetStream, JetStreamPublisher, JsRequestMessage}; @@ -39,26 +37,17 @@ pub struct Bridge { pub(crate) clock: C, pub(crate) config: Config, pub(crate) metrics: Metrics, - pub(crate) notification_sender: mpsc::Sender, pub(crate) background_tasks: Mutex>>, } impl Bridge { - pub fn new( - nats: N, - js: J, - clock: C, - meter: &Meter, - config: Config, - notification_sender: mpsc::Sender, - ) -> Self { + pub fn new(nats: N, js: J, clock: C, meter: &Meter, config: Config) -> Self { Self { nats, js, clock, config, metrics: Metrics::new(meter), - notification_sender, background_tasks: Mutex::new(Vec::new()), } } diff --git a/rsworkspace/crates/acp/acp-nats/src/agent/cancel/tests.rs b/rsworkspace/crates/acp/acp-nats/src/agent/cancel/tests.rs index 8735792457..c565af7212 100644 --- a/rsworkspace/crates/acp/acp-nats/src/agent/cancel/tests.rs +++ b/rsworkspace/crates/acp/acp-nats/src/agent/cancel/tests.rs @@ -20,7 +20,6 @@ fn mock_bridge_with_clock() -> ( clock.clone(), &opentelemetry::global::meter("acp-nats-test"), Config::for_test("acp"), - tokio::sync::mpsc::channel(1).0, ); (mock, clock, bridge) } diff --git a/rsworkspace/crates/acp/acp-nats/src/agent/initialize/tests.rs b/rsworkspace/crates/acp/acp-nats/src/agent/initialize/tests.rs index 20ca8bf12c..4d8290ca32 100644 --- a/rsworkspace/crates/acp/acp-nats/src/agent/initialize/tests.rs +++ b/rsworkspace/crates/acp/acp-nats/src/agent/initialize/tests.rs @@ -105,7 +105,6 @@ async fn handlers_use_custom_prefix() { trogon_std::time::SystemClock, &opentelemetry::global::meter("acp-nats-test"), Config::for_test("myorg.prod"), - tokio::sync::mpsc::channel(1).0, ); let expected = InitializeResponse::new(ProtocolVersion::LATEST); set_json_response(&mock, "myorg.prod.v1.global.agent.initialize", &expected); diff --git a/rsworkspace/crates/acp/acp-nats/src/agent/prompt.rs b/rsworkspace/crates/acp/acp-nats/src/agent/prompt.rs index 03966a2122..8303121c86 100644 --- a/rsworkspace/crates/acp/acp-nats/src/agent/prompt.rs +++ b/rsworkspace/crates/acp/acp-nats/src/agent/prompt.rs @@ -1,10 +1,10 @@ -use agent_client_protocol::schema::v1::{PromptRequest, PromptResponse, SessionNotification, StopReason}; +use agent_client_protocol::schema::v1::{PromptRequest, PromptResponse, StopReason}; use agent_client_protocol::{Error, ErrorCode}; use async_nats::jetstream::AckKind; use futures::StreamExt; use jsonrpc_nats::RequestId; use tokio::time::timeout; -use tracing::{instrument, warn}; +use tracing::instrument; use trogon_nats::jetstream::{ JetStreamConsumer as _, JetStreamCreateConsumer as _, JetStreamGetStream, JetStreamPublisher, JsAck as _, JsAckWith as _, JsMessageRef as _, JsRequestMessage, @@ -18,9 +18,7 @@ use crate::nats::parsing::SessionAgentMethod; use crate::nats::{FlushClient, PublishClient, RequestClient, SubscribeClient, commands, responses}; use crate::req_id::ReqId; use crate::session_id::AcpSessionId; -use crate::wire::{ - WireError, decode_notification_params, decode_response, encode_request, req_id_from_request_headers, -}; +use crate::wire::{WireError, decode_response, encode_request, req_id_from_request_headers}; #[instrument( name = ACP_SESSION_PROMPT, @@ -70,29 +68,13 @@ where J: JetStreamPublisher + JetStreamGetStream, trogon_nats::jetstream::JsMessageOf: JsRequestMessage, { - // Create consumers BEFORE publishing — same principle as subscribe-before-publish. - // Both consumers are session-scoped and deliver only new messages, so creating - // them first is what guarantees the response is seen even if the runner answers - // before we start consuming. - let notifications_stream = streams::notifications_stream_name(prefix); - let notif_config = consumers::prompt_notifications_consumer(prefix, session_id); - let notif_stream = js.get_stream(¬ifications_stream).await.map_err(|e| { - Error::new( - ErrorCode::InternalError.into(), - format!("get notifications stream: {e}"), - ) - })?; - let notif_consumer = notif_stream.create_consumer(notif_config).await.map_err(|e| { - Error::new( - ErrorCode::InternalError.into(), - format!("create notification consumer: {e}"), - ) - })?; - let mut notif_messages = notif_consumer - .messages() - .await - .map_err(|e| Error::new(ErrorCode::InternalError.into(), format!("notification messages: {e}")))?; - + // Create the consumer BEFORE publishing — same principle as subscribe-before-publish. + // It is session-scoped and delivers only new messages, so creating it first is what + // guarantees the response is seen even if the runner answers before we start consuming. + // + // Mid-prompt `session/update` progress is not consumed here: it is a client-directed + // notification and reaches the local client through the client-op proxy + // (`...client.session.update`), which covers every operation rather than just prompts. let responses_stream = streams::responses_stream_name(prefix); let resp_config = consumers::response_consumer(prefix, session_id); let resp_stream = js @@ -139,44 +121,6 @@ where loop { tokio::select! { - notif = notif_messages.next() => { - match notif { - None => { - bridge.metrics.record_error("prompt", "notification_stream_closed"); - break Err(Error::new( - ErrorCode::InternalError.into(), - "notification stream closed unexpectedly", - )); - } - Some(Err(e)) => { - bridge.metrics.record_error("prompt", "notification_consumer_error"); - break Err(Error::new( - ErrorCode::InternalError.into(), - format!("notification consumer: {e}"), - )); - } - Some(Ok(js_msg)) => { - let message = js_msg.message(); - let notification_headers = message.headers.clone().unwrap_or_default(); - let notification: SessionNotification = match decode_notification_params( - "session/update", - ¬ification_headers, - message.payload.as_ref(), - ) { - Ok(n) => n, - Err(e) => { - warn!(error = %e, "bad notification payload; skipping"); - let _ = js_msg.ack().await; - continue; - } - }; - let _ = js_msg.ack().await; - let _ = bridge.notification_sender.send(notification).await.inspect_err(|_| { - warn!("notification receiver dropped; continuing prompt"); - }); - } - } - } resp = timeout(op_timeout, resp_messages.next()) => { match resp { Ok(Some(Ok(js_msg))) => { diff --git a/rsworkspace/crates/acp/acp-nats/src/agent/prompt/tests.rs b/rsworkspace/crates/acp/acp-nats/src/agent/prompt/tests.rs index 81be3c9dac..c71a3ac594 100644 --- a/rsworkspace/crates/acp/acp-nats/src/agent/prompt/tests.rs +++ b/rsworkspace/crates/acp/acp-nats/src/agent/prompt/tests.rs @@ -1,7 +1,6 @@ use super::*; use crate::config::Config; use jsonrpc_nats::{Message, encode}; -use tracing_subscriber::util::SubscriberInitExt; use trogon_nats::AdvancedMockNatsClient; use trogon_nats::jetstream::mocks::*; @@ -17,11 +16,6 @@ fn make_nats_msg(payload: &[u8], headers: Option) -> asyn } } -fn make_wire_notification_msg(notification: &Req) -> async_nats::Message { - let encoded = crate::wire::encode_notification("session/update", notification).unwrap(); - make_nats_msg(&encoded.body, Some(encoded.headers)) -} - use crate::agent::test_support::{MockJs, reply_when_published}; type RespTx = futures::channel::mpsc::UnboundedSender>; @@ -69,14 +63,12 @@ fn mock_bridge() -> ( ) { let mock = AdvancedMockNatsClient::new(); let js = MockJs::new(); - let (notification_tx, _notification_rx) = tokio::sync::mpsc::channel::(64); let bridge = Bridge::new( mock.clone(), js.clone(), trogon_std::time::SystemClock, &opentelemetry::global::meter("prompt-js-test"), Config::for_test("acp").with_prompt_timeout(std::time::Duration::from_secs(5)), - notification_tx, ); (mock, js, bridge) } @@ -97,10 +89,6 @@ async fn prompt_js_success() { // cancel sub for core NATS let _cancel_tx = mock.inject_messages(); - // notification consumer - let (notif_consumer, notif_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); - js.consumer_factory.add_consumer(notif_consumer); - // response consumer let (resp_consumer, resp_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); js.consumer_factory.add_consumer(resp_consumer); @@ -109,7 +97,6 @@ async fn prompt_js_success() { let result = handle(&bridge, PromptRequest::new("s1", vec![])).await; - drop(notif_tx); let response = result.expect("expected Ok prompt response"); assert_eq!(response.stop_reason, StopReason::EndTurn); } @@ -120,9 +107,6 @@ async fn prompt_js_cancel() { let cancel_tx = mock.inject_messages(); - let (notif_consumer, _notif_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); - js.consumer_factory.add_consumer(notif_consumer); - let (resp_consumer, _resp_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); js.consumer_factory.add_consumer(resp_consumer); @@ -137,21 +121,16 @@ async fn prompt_js_cancel() { async fn prompt_js_timeout() { let mock = AdvancedMockNatsClient::new(); let js = MockJs::new(); - let (notification_tx, _notification_rx) = tokio::sync::mpsc::channel::(64); let bridge = Bridge::new( mock.clone(), js.clone(), trogon_std::time::SystemClock, &opentelemetry::global::meter("prompt-js-timeout-test"), Config::for_test("acp").with_prompt_timeout(std::time::Duration::from_millis(50)), - notification_tx, ); let _cancel_tx = mock.inject_messages(); - let (notif_consumer, _notif_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); - js.consumer_factory.add_consumer(notif_consumer); - let (resp_consumer, _resp_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); js.consumer_factory.add_consumer(resp_consumer); @@ -160,57 +139,11 @@ async fn prompt_js_timeout() { assert!(result.unwrap_err().message.contains("timed out")); } -#[tokio::test] -async fn prompt_js_notification_forwarding() { - let mock = AdvancedMockNatsClient::new(); - let js = MockJs::new(); - let (notification_tx, _notification_rx) = tokio::sync::mpsc::channel::(64); - let bridge = Bridge::new( - mock.clone(), - js.clone(), - trogon_std::time::SystemClock, - &opentelemetry::global::meter("prompt-js-notif-test"), - Config::for_test("acp").with_prompt_timeout(std::time::Duration::from_secs(5)), - notification_tx, - ); - - let _cancel_tx = mock.inject_messages(); - - let (notif_consumer, notif_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); - js.consumer_factory.add_consumer(notif_consumer); - - let (resp_consumer, resp_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); - js.consumer_factory.add_consumer(resp_consumer); - - let notification = SessionNotification::new( - "s1", - agent_client_protocol::schema::v1::SessionUpdate::AgentThoughtChunk( - agent_client_protocol::schema::v1::ContentChunk::new( - agent_client_protocol::schema::v1::ContentBlock::Text( - agent_client_protocol::schema::v1::TextContent::new("thinking..."), - ), - ), - ), - ); - let notif_msg = MockJsMessage::new(make_wire_notification_msg(¬ification)); - notif_tx.unbounded_send(Ok(notif_msg)).unwrap(); - - reply_success_when_published(&js, resp_tx, &PromptResponse::new(StopReason::EndTurn)); - let _notif_keeper = notif_tx; - - let result = handle(&bridge, PromptRequest::new("s1", vec![])).await; - - let response = result.expect("expected Ok prompt response"); - assert_eq!(response.stop_reason, StopReason::EndTurn); -} - #[tokio::test] async fn prompt_js_publish_failure() { let (mock, js, bridge) = mock_bridge(); let _cancel_tx = mock.inject_messages(); - let (notif_consumer, _notif_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); - js.consumer_factory.add_consumer(notif_consumer); let (resp_consumer, _resp_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); js.consumer_factory.add_consumer(resp_consumer); @@ -226,9 +159,6 @@ async fn prompt_js_bad_response_payload() { let (mock, js, bridge) = mock_bridge(); let _cancel_tx = mock.inject_messages(); - let (notif_consumer, _notif_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); - js.consumer_factory.add_consumer(notif_consumer); - let (resp_consumer, resp_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); js.consumer_factory.add_consumer(resp_consumer); @@ -244,9 +174,6 @@ async fn prompt_js_agent_error_response() { let (mock, js, bridge) = mock_bridge(); let _cancel_tx = mock.inject_messages(); - let (notif_consumer, _notif_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); - js.consumer_factory.add_consumer(notif_consumer); - let (resp_consumer, resp_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); js.consumer_factory.add_consumer(resp_consumer); @@ -265,9 +192,6 @@ async fn prompt_js_response_stream_closed() { let (mock, js, bridge) = mock_bridge(); let _cancel_tx = mock.inject_messages(); - let (notif_consumer, _notif_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); - js.consumer_factory.add_consumer(notif_consumer); - let (resp_consumer, resp_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); js.consumer_factory.add_consumer(resp_consumer); @@ -277,45 +201,21 @@ async fn prompt_js_response_stream_closed() { assert!(result.is_err()); } -#[tokio::test] -async fn prompt_js_get_notif_stream_failure() { - let (mock, js, bridge) = mock_bridge(); - let _cancel_tx = mock.inject_messages(); - js.consumer_factory.fail_get_stream_at(1); - let result = handle(&bridge, PromptRequest::new("s1", vec![])).await; - assert!(result.is_err()); - assert!(result.unwrap_err().message.contains("get notifications stream")); -} - #[tokio::test] async fn prompt_js_get_resp_stream_failure() { let (mock, js, bridge) = mock_bridge(); let _cancel_tx = mock.inject_messages(); - let (notif_consumer, _notif_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); - js.consumer_factory.add_consumer(notif_consumer); - js.consumer_factory.fail_get_stream_at(2); + js.consumer_factory.fail_get_stream_at(1); let result = handle(&bridge, PromptRequest::new("s1", vec![])).await; assert!(result.is_err()); assert!(result.unwrap_err().message.contains("get responses stream")); } -#[tokio::test] -async fn prompt_js_notif_consumer_creation_failure() { - let (mock, _js, bridge) = mock_bridge(); - let _cancel_tx = mock.inject_messages(); - // Don't add any consumers — first create_consumer call will fail - let result = handle(&bridge, PromptRequest::new("s1", vec![])).await; - assert!(result.is_err()); - assert!(result.unwrap_err().message.contains("create notification consumer")); -} - #[tokio::test] async fn prompt_js_resp_consumer_creation_failure() { - let (mock, js, bridge) = mock_bridge(); + let (mock, _js, bridge) = mock_bridge(); let _cancel_tx = mock.inject_messages(); - // Add notif consumer but not response consumer - let (notif_consumer, _notif_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); - js.consumer_factory.add_consumer(notif_consumer); + // Add no consumers — create_consumer fails let result = handle(&bridge, PromptRequest::new("s1", vec![])).await; assert!(result.is_err()); assert!(result.unwrap_err().message.contains("create response consumer")); @@ -325,8 +225,6 @@ async fn prompt_js_resp_consumer_creation_failure() { async fn prompt_js_cancel_subscribe_failure() { let (_mock, js, bridge) = mock_bridge(); // Don't inject cancel_tx — subscribe will fail (no streams in mock) - let (notif_consumer, _notif_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); - js.consumer_factory.add_consumer(notif_consumer); let (resp_consumer, _resp_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); js.consumer_factory.add_consumer(resp_consumer); let result = handle(&bridge, PromptRequest::new("s1", vec![])).await; @@ -334,27 +232,11 @@ async fn prompt_js_cancel_subscribe_failure() { assert!(result.unwrap_err().message.contains("subscribe cancelled")); } -#[tokio::test] -async fn prompt_js_notif_messages_failure() { - let (mock, js, bridge) = mock_bridge(); - let _cancel_tx = mock.inject_messages(); - - let failing_consumer = trogon_nats::jetstream::MockJetStreamConsumer::failing(); - js.consumer_factory.add_consumer(failing_consumer); - - let result = handle(&bridge, PromptRequest::new("s1", vec![])).await; - assert!(result.is_err()); - assert!(result.unwrap_err().message.contains("notification messages")); -} - #[tokio::test] async fn prompt_js_resp_messages_failure() { let (mock, js, bridge) = mock_bridge(); let _cancel_tx = mock.inject_messages(); - let (notif_consumer, _notif_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); - js.consumer_factory.add_consumer(notif_consumer); - let failing_consumer = trogon_nats::jetstream::MockJetStreamConsumer::failing(); js.consumer_factory.add_consumer(failing_consumer); @@ -363,34 +245,11 @@ async fn prompt_js_resp_messages_failure() { assert!(result.unwrap_err().message.contains("response messages")); } -#[tokio::test] -async fn prompt_js_notification_consumer_error() { - let (mock, js, bridge) = mock_bridge(); - let _cancel_tx = mock.inject_messages(); - - let (notif_consumer, notif_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); - js.consumer_factory.add_consumer(notif_consumer); - - let (resp_consumer, _resp_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); - js.consumer_factory.add_consumer(resp_consumer); - - notif_tx - .unbounded_send(Err(trogon_nats::mocks::MockError("consumer error".to_string()))) - .unwrap(); - - let result = handle(&bridge, PromptRequest::new("s1", vec![])).await; - assert!(result.is_err()); - assert!(result.unwrap_err().message.contains("notification consumer")); -} - #[tokio::test] async fn prompt_js_response_consumer_error() { let (mock, js, bridge) = mock_bridge(); let _cancel_tx = mock.inject_messages(); - let (notif_consumer, _notif_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); - js.consumer_factory.add_consumer(notif_consumer); - let (resp_consumer, resp_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); js.consumer_factory.add_consumer(resp_consumer); @@ -403,99 +262,6 @@ async fn prompt_js_response_consumer_error() { assert!(result.unwrap_err().message.contains("response consumer")); } -#[tokio::test] -async fn prompt_js_bad_notification_payload_skipped() { - let mock = AdvancedMockNatsClient::new(); - let js = MockJs::new(); - let (notification_tx, _notification_rx) = tokio::sync::mpsc::channel::(64); - let bridge = Bridge::new( - mock.clone(), - js.clone(), - trogon_std::time::SystemClock, - &opentelemetry::global::meter("prompt-js-bad-notif-test"), - Config::for_test("acp").with_prompt_timeout(std::time::Duration::from_millis(100)), - notification_tx, - ); - let _cancel_tx = mock.inject_messages(); - - let (notif_consumer, notif_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); - js.consumer_factory.add_consumer(notif_consumer); - - let (resp_consumer, _resp_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); - js.consumer_factory.add_consumer(resp_consumer); - - // Send only bad notification, no response. Handler processes bad notif - // (warn, ack, continue), then times out waiting for response. - let bad_notif = MockJsMessage::new(make_nats_msg(b"not json", None)); - notif_tx.unbounded_send(Ok(bad_notif)).unwrap(); - let _notif_keeper = notif_tx; - - let result = handle(&bridge, PromptRequest::new("s1", vec![])).await; - // Times out after processing bad notification - assert!(result.is_err()); - assert!(result.unwrap_err().message.contains("timed out")); -} - -#[tokio::test] -async fn prompt_js_notification_receiver_dropped() { - let _guard = tracing_subscriber::fmt().with_test_writer().set_default(); - - let mock = AdvancedMockNatsClient::new(); - let js = MockJs::new(); - let (notification_tx, notification_rx) = tokio::sync::mpsc::channel::(64); - drop(notification_rx); - let bridge = Bridge::new( - mock.clone(), - js.clone(), - trogon_std::time::SystemClock, - &opentelemetry::global::meter("prompt-js-rx-dropped-test"), - Config::for_test("acp").with_prompt_timeout(std::time::Duration::from_millis(100)), - notification_tx, - ); - - let _cancel_tx = mock.inject_messages(); - - let (notif_consumer, notif_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); - js.consumer_factory.add_consumer(notif_consumer); - let (resp_consumer, _resp_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); - js.consumer_factory.add_consumer(resp_consumer); - - let notification = SessionNotification::new( - "s1", - agent_client_protocol::schema::v1::SessionUpdate::AgentThoughtChunk( - agent_client_protocol::schema::v1::ContentChunk::new( - agent_client_protocol::schema::v1::ContentBlock::Text( - agent_client_protocol::schema::v1::TextContent::new("thinking..."), - ), - ), - ), - ); - let notif_msg = MockJsMessage::new(make_wire_notification_msg(¬ification)); - notif_tx.unbounded_send(Ok(notif_msg)).unwrap(); - let _notif_keeper = notif_tx; - - let result = handle(&bridge, PromptRequest::new("s1", vec![])).await; - assert!(result.is_err()); - assert!(result.unwrap_err().message.contains("timed out")); -} - -#[tokio::test] -async fn prompt_js_notification_stream_closed() { - let (mock, js, bridge) = mock_bridge(); - let _cancel_tx = mock.inject_messages(); - - let (notif_consumer, notif_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); - js.consumer_factory.add_consumer(notif_consumer); - let (resp_consumer, _resp_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); - js.consumer_factory.add_consumer(resp_consumer); - - drop(notif_tx); - - let result = handle(&bridge, PromptRequest::new("s1", vec![])).await; - assert!(result.is_err()); - assert!(result.unwrap_err().message.contains("notification stream closed")); -} - #[tokio::test] async fn prompt_js_skips_a_reply_belonging_to_another_request_on_the_session() { // The response consumer is session-scoped, so a second prompt in flight on @@ -505,9 +271,6 @@ async fn prompt_js_skips_a_reply_belonging_to_another_request_on_the_session() { let _cancel_tx = mock.inject_messages(); - let (notif_consumer, notif_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); - js.consumer_factory.add_consumer(notif_consumer); - let (resp_consumer, resp_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); js.consumer_factory.add_consumer(resp_consumer); @@ -527,7 +290,6 @@ async fn prompt_js_skips_a_reply_belonging_to_another_request_on_the_session() { let result = handle(&bridge, PromptRequest::new("s1", vec![])).await; - drop(notif_tx); assert_eq!( result.expect("expected Ok prompt response").stop_reason, StopReason::EndTurn diff --git a/rsworkspace/crates/acp/acp-nats/src/agent/test_support.rs b/rsworkspace/crates/acp/acp-nats/src/agent/test_support.rs index 3de9e4dbd3..f6b30db877 100644 --- a/rsworkspace/crates/acp/acp-nats/src/agent/test_support.rs +++ b/rsworkspace/crates/acp/acp-nats/src/agent/test_support.rs @@ -21,7 +21,6 @@ pub fn mock_bridge() -> ( trogon_std::time::SystemClock, &opentelemetry::global::meter("acp-nats-test"), Config::for_test("acp"), - tokio::sync::mpsc::channel(1).0, ); (mock, js, bridge) } @@ -89,7 +88,6 @@ pub fn mock_bridge_with_metrics() -> ( trogon_std::time::SystemClock, &meter, Config::for_test("acp"), - tokio::sync::mpsc::channel(1).0, ); (mock, js, bridge, exporter, provider) } diff --git a/rsworkspace/crates/acp/acp-nats/src/agent/tests.rs b/rsworkspace/crates/acp/acp-nats/src/agent/tests.rs index 67e41bb26f..f8e6095734 100644 --- a/rsworkspace/crates/acp/acp-nats/src/agent/tests.rs +++ b/rsworkspace/crates/acp/acp-nats/src/agent/tests.rs @@ -5,10 +5,7 @@ use crate::AgentHandler; use crate::agent::test_support::MockJs; use crate::config::Config; use agent_client_protocol::ErrorCode; -use agent_client_protocol::schema::v1::{ - ExtNotification, ExtRequest, PromptRequest, PromptResponse, SessionNotification, StopReason, -}; -use tokio::sync::mpsc; +use agent_client_protocol::schema::v1::{ExtNotification, ExtRequest, PromptRequest, PromptResponse, StopReason}; use trogon_nats::AdvancedMockNatsClient; fn mock_bridge() -> ( @@ -18,14 +15,12 @@ fn mock_bridge() -> ( ) { let mock = AdvancedMockNatsClient::new(); let js = MockJs::new(); - let (tx, _rx) = mpsc::channel::(64); let bridge = Bridge::new( mock.clone(), js.clone(), trogon_std::time::SystemClock, &opentelemetry::global::meter("acp-nats-test"), Config::for_test("acp"), - tx, ); (mock, js, bridge) } @@ -55,10 +50,6 @@ async fn prompt_via_agent_trait_returns_done() { // cancel sub for core NATS let _cancel_tx = mock.inject_messages(); - // notification consumer - let (notif_consumer, _notif_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); - js.consumer_factory.add_consumer(notif_consumer); - // response consumer let (resp_consumer, resp_tx) = trogon_nats::jetstream::MockJetStreamConsumer::new(); js.consumer_factory.add_consumer(resp_consumer); diff --git a/rsworkspace/crates/acp/acp-nats/src/client/tests.rs b/rsworkspace/crates/acp/acp-nats/src/client/tests.rs index 7a5fbdfc52..8ce02b5bb7 100644 --- a/rsworkspace/crates/acp/acp-nats/src/client/tests.rs +++ b/rsworkspace/crates/acp/acp-nats/src/client/tests.rs @@ -123,7 +123,6 @@ fn make_bridge(nats: MockNatsClient) -> Arc Config { - let pfx = prefix.as_str(); - let sid = session_id.as_str(); - Config { - filter_subject: format!("{pfx}.v1.session.{sid}.agent.update"), - deliver_policy: DeliverPolicy::New, - ack_policy: AckPolicy::Explicit, - replay_policy: ReplayPolicy::Instant, - ..Default::default() - } -} - pub fn response_consumer(prefix: &AcpPrefix, session_id: &AcpSessionId) -> Config { let pfx = prefix.as_str(); let sid = session_id.as_str(); diff --git a/rsworkspace/crates/acp/acp-nats/src/jetstream/consumers/tests.rs b/rsworkspace/crates/acp/acp-nats/src/jetstream/consumers/tests.rs index 6d44f26d88..346fa2a58a 100644 --- a/rsworkspace/crates/acp/acp-nats/src/jetstream/consumers/tests.rs +++ b/rsworkspace/crates/acp/acp-nats/src/jetstream/consumers/tests.rs @@ -8,20 +8,6 @@ fn sid(s: &str) -> AcpSessionId { AcpSessionId::new(s).expect("test session id") } -#[test] -fn prompt_notifications_consumer_filter() { - let config = prompt_notifications_consumer(&p("acp"), &sid("sess-1")); - assert_eq!(config.filter_subject, "acp.v1.session.sess-1.agent.update"); -} - -#[test] -fn prompt_notifications_consumer_delivers_new() { - let config = prompt_notifications_consumer(&p("acp"), &sid("s1")); - assert_eq!(config.deliver_policy, DeliverPolicy::New); - assert_eq!(config.ack_policy, AckPolicy::Explicit); - assert_eq!(config.replay_policy, ReplayPolicy::Instant); -} - #[test] fn commands_observer_delivers_all() { let config = commands_observer(); @@ -57,17 +43,11 @@ fn response_consumer_custom_prefix() { #[test] fn consumer_filters_carry_no_request_id() { - let p = p("acp"); - let s = sid("sess-1"); - for filter in [ - response_consumer(&p, &s).filter_subject, - prompt_notifications_consumer(&p, &s).filter_subject, - ] { - assert!( - !filter - .split('.') - .any(trogon_nats::subject_conformance::looks_like_request_id), - "{filter}" - ); - } + let filter = response_consumer(&p("acp"), &sid("sess-1")).filter_subject; + assert!( + !filter + .split('.') + .any(trogon_nats::subject_conformance::looks_like_request_id), + "{filter}" + ); } diff --git a/rsworkspace/crates/acp/acp-nats/src/jetstream/provision/tests.rs b/rsworkspace/crates/acp/acp-nats/src/jetstream/provision/tests.rs index 0722be0304..fd50a6eb08 100644 --- a/rsworkspace/crates/acp/acp-nats/src/jetstream/provision/tests.rs +++ b/rsworkspace/crates/acp/acp-nats/src/jetstream/provision/tests.rs @@ -8,10 +8,10 @@ fn p(s: &str) -> AcpPrefix { } #[tokio::test] -async fn provision_creates_six_streams() { +async fn provision_creates_five_streams() { let ctx = MockJetStreamContext::new(); provision_streams(&ctx, &p("acp")).await.unwrap(); - assert_eq!(ctx.created_streams().len(), 6); + assert_eq!(ctx.created_streams().len(), 5); } #[tokio::test] @@ -22,7 +22,6 @@ async fn provision_creates_correct_stream_names() { assert!(names.contains(&"ACP_COMMANDS".to_string())); assert!(names.contains(&"ACP_RESPONSES".to_string())); assert!(names.contains(&"ACP_CLIENT_OPS".to_string())); - assert!(names.contains(&"ACP_NOTIFICATIONS".to_string())); assert!(names.contains(&"ACP_GLOBAL".to_string())); assert!(names.contains(&"ACP_GLOBAL_EXT".to_string())); } @@ -50,5 +49,5 @@ async fn provision_is_idempotent() { let ctx = MockJetStreamContext::new(); provision_streams(&ctx, &p("acp")).await.unwrap(); provision_streams(&ctx, &p("acp")).await.unwrap(); - assert_eq!(ctx.created_streams().len(), 12); + assert_eq!(ctx.created_streams().len(), 10); } diff --git a/rsworkspace/crates/acp/acp-nats/src/jetstream/streams.rs b/rsworkspace/crates/acp/acp-nats/src/jetstream/streams.rs index fce34b4147..adbb46ec66 100644 --- a/rsworkspace/crates/acp/acp-nats/src/jetstream/streams.rs +++ b/rsworkspace/crates/acp/acp-nats/src/jetstream/streams.rs @@ -3,10 +3,6 @@ use async_nats::jetstream::stream::Config; use crate::acp_prefix::AcpPrefix; use crate::nats::AcpStream; -pub fn notifications_stream_name(prefix: &AcpPrefix) -> String { - AcpStream::Notifications.stream_name(prefix) -} - pub fn responses_stream_name(prefix: &AcpPrefix) -> String { AcpStream::Responses.stream_name(prefix) } @@ -23,7 +19,7 @@ pub fn global_ext_stream_name(prefix: &AcpPrefix) -> String { AcpStream::GlobalExt.stream_name(prefix) } -pub fn all_configs(prefix: &AcpPrefix) -> [Config; 6] { +pub fn all_configs(prefix: &AcpPrefix) -> [Config; 5] { AcpStream::all_configs(prefix) } diff --git a/rsworkspace/crates/acp/acp-nats/src/jetstream/streams/tests.rs b/rsworkspace/crates/acp/acp-nats/src/jetstream/streams/tests.rs index 8cda2c3521..1f50004aca 100644 --- a/rsworkspace/crates/acp/acp-nats/src/jetstream/streams/tests.rs +++ b/rsworkspace/crates/acp/acp-nats/src/jetstream/streams/tests.rs @@ -16,7 +16,6 @@ fn stream_names_use_uppercase_prefix() { assert_eq!(AcpStream::Commands.stream_name(&prefix), "ACP_COMMANDS"); assert_eq!(AcpStream::Responses.stream_name(&prefix), "ACP_RESPONSES"); assert_eq!(AcpStream::ClientOps.stream_name(&prefix), "ACP_CLIENT_OPS"); - assert_eq!(AcpStream::Notifications.stream_name(&prefix), "ACP_NOTIFICATIONS"); assert_eq!(AcpStream::Global.stream_name(&prefix), "ACP_GLOBAL"); assert_eq!(AcpStream::GlobalExt.stream_name(&prefix), "ACP_GLOBAL_EXT"); } @@ -76,12 +75,6 @@ fn client_ops_subjects() { assert_eq!(config.subjects, vec!["acp.v1.session.*.client.>"]); } -#[test] -fn notifications_subjects() { - let config = AcpStream::Notifications.config(&p("acp")); - assert_eq!(config.subjects, vec!["acp.v1.session.*.agent.update"]); -} - #[test] fn all_streams_use_file_storage() { let prefix = p("acp"); @@ -112,12 +105,6 @@ fn all_streams_use_limits_retention() { } } -#[test] -fn notifications_stream_name_formats_correctly() { - assert_eq!(notifications_stream_name(&p("acp")), "ACP_NOTIFICATIONS"); - assert_eq!(notifications_stream_name(&p("myapp")), "MYAPP_NOTIFICATIONS"); -} - #[test] fn responses_stream_name_formats_correctly() { assert_eq!(responses_stream_name(&p("acp")), "ACP_RESPONSES"); @@ -174,8 +161,8 @@ fn global_ext_stream_name_formats_correctly() { } #[test] -fn all_configs_returns_six_streams() { - assert_eq!(all_configs(&p("acp")).len(), 6); +fn all_configs_returns_five_streams() { + assert_eq!(all_configs(&p("acp")).len(), 5); } fn nats_pattern_matches(pattern: &str, subject: &str) -> bool { diff --git a/rsworkspace/crates/acp/acp-nats/src/lib.rs b/rsworkspace/crates/acp/acp-nats/src/lib.rs index fcc0624166..5f108281f5 100644 --- a/rsworkspace/crates/acp/acp-nats/src/lib.rs +++ b/rsworkspace/crates/acp/acp-nats/src/lib.rs @@ -27,7 +27,7 @@ pub use client_proxy::NatsClientProxy; pub use config::{Config, DEFAULT_ACP_PREFIX, ENV_ACP_PREFIX, apply_timeout_overrides, nats_connect_timeout}; pub use error::AGENT_UNAVAILABLE; pub use ext_method_name::ExtMethodName; -pub use nats::responses::{ResponseSubject, UpdateSubject}; +pub use nats::responses::ResponseSubject; pub use nats::{FlushClient, PublishClient, RequestClient, SubscribeClient}; pub use req_id::ReqId; pub use session_id::AcpSessionId; @@ -37,18 +37,5 @@ pub use trogon_nats::jetstream::{JetStreamGetStream, JetStreamPublisher}; pub use trogon_nats::{NatsAuth, NatsConfig}; pub use trogon_std::StdJsonSerialize; -pub fn spawn_notification_forwarder( - client: impl crate::ClientHandler + Send + 'static, - mut rx: tokio::sync::mpsc::Receiver, -) -> tokio::task::JoinHandle<()> { - tokio::spawn(async move { - while let Some(notif) = rx.recv().await { - if client.session_notification(notif).await.is_err() { - break; - } - } - }) -} - #[cfg(test)] mod tests; diff --git a/rsworkspace/crates/acp/acp-nats/src/nats/subjects/conformance_tests.rs b/rsworkspace/crates/acp/acp-nats/src/nats/subjects/conformance_tests.rs index 75b5f4a083..a29f47fe10 100644 --- a/rsworkspace/crates/acp/acp-nats/src/nats/subjects/conformance_tests.rs +++ b/rsworkspace/crates/acp/acp-nats/src/nats/subjects/conformance_tests.rs @@ -65,7 +65,6 @@ fn published(p: &AcpPrefix) -> Vec<(&'static str, String)> { ("CancelledSubject", responses::CancelledSubject::new(p, &s).to_string()), ("ExtReadySubject", responses::ExtReadySubject::new(p, &s).to_string()), ("ResponseSubject", responses::ResponseSubject::new(p, &s).to_string()), - ("UpdateSubject", responses::UpdateSubject::new(p, &s).to_string()), ( "SessionUpdateSubject", client_ops::SessionUpdateSubject::new(p, &s).to_string(), diff --git a/rsworkspace/crates/acp/acp-nats/src/nats/subjects/responses/mod.rs b/rsworkspace/crates/acp/acp-nats/src/nats/subjects/responses/mod.rs index ec01fbf105..b502513674 100644 --- a/rsworkspace/crates/acp/acp-nats/src/nats/subjects/responses/mod.rs +++ b/rsworkspace/crates/acp/acp-nats/src/nats/subjects/responses/mod.rs @@ -1,9 +1,7 @@ mod cancelled; mod ext_ready; mod response; -mod update; pub use cancelled::CancelledSubject; pub use ext_ready::ExtReadySubject; pub use response::ResponseSubject; -pub use update::UpdateSubject; diff --git a/rsworkspace/crates/acp/acp-nats/src/nats/subjects/responses/update.rs b/rsworkspace/crates/acp/acp-nats/src/nats/subjects/responses/update.rs deleted file mode 100644 index b86268ac9c..0000000000 --- a/rsworkspace/crates/acp/acp-nats/src/nats/subjects/responses/update.rs +++ /dev/null @@ -1,45 +0,0 @@ -/// Agent -> bridge async notification. -/// -/// Scoped to the session, not the request (ADR#0055). Requests within a session -/// are told apart by the JSON-RPC `id`, projected to `Jsonrpc-Id`, which the -/// bridge mints per request. -#[derive(Debug)] -pub struct UpdateSubject { - prefix: crate::acp_prefix::AcpPrefix, - session_id: crate::session_id::AcpSessionId, -} - -impl UpdateSubject { - pub fn new(prefix: &crate::acp_prefix::AcpPrefix, session_id: &crate::session_id::AcpSessionId) -> Self { - Self { - prefix: prefix.clone(), - session_id: session_id.clone(), - } - } -} - -impl std::fmt::Display for UpdateSubject { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!( - f, - "{}.v1.session.{}.agent.update", - self.prefix.as_str(), - self.session_id.as_str() - ) - } -} - -impl async_nats::subject::ToSubject for UpdateSubject { - fn to_subject(&self) -> async_nats::subject::Subject { - async_nats::subject::Subject::from(self.to_string().as_str()) - } -} - -impl super::super::markers::Subscribable for UpdateSubject {} - -impl super::super::stream::StreamAssignment for UpdateSubject { - const STREAM: Option = Some(super::super::stream::AcpStream::Notifications); -} - -#[cfg(test)] -mod tests; diff --git a/rsworkspace/crates/acp/acp-nats/src/nats/subjects/responses/update/tests.rs b/rsworkspace/crates/acp/acp-nats/src/nats/subjects/responses/update/tests.rs deleted file mode 100644 index f35de41f7a..0000000000 --- a/rsworkspace/crates/acp/acp-nats/src/nats/subjects/responses/update/tests.rs +++ /dev/null @@ -1,10 +0,0 @@ -use super::*; -use async_nats::subject::ToSubject as _; - -#[test] -fn to_subject_matches_display() { - let prefix = crate::acp_prefix::AcpPrefix::new("acp").expect("prefix"); - let session_id = crate::session_id::AcpSessionId::new("s1").expect("session_id"); - let subject = UpdateSubject::new(&prefix, &session_id); - assert_eq!(subject.to_subject().as_str(), subject.to_string()); -} diff --git a/rsworkspace/crates/acp/acp-nats/src/nats/subjects/stream.rs b/rsworkspace/crates/acp/acp-nats/src/nats/subjects/stream.rs index 10a4823cd7..fd66d56ae7 100644 --- a/rsworkspace/crates/acp/acp-nats/src/nats/subjects/stream.rs +++ b/rsworkspace/crates/acp/acp-nats/src/nats/subjects/stream.rs @@ -9,17 +9,15 @@ pub enum AcpStream { Commands, Responses, ClientOps, - Notifications, Global, GlobalExt, } impl AcpStream { - pub const ALL: [AcpStream; 6] = [ + pub const ALL: [AcpStream; 5] = [ Self::Commands, Self::Responses, Self::ClientOps, - Self::Notifications, Self::Global, Self::GlobalExt, ]; @@ -29,7 +27,6 @@ impl AcpStream { Self::Commands => "COMMANDS", Self::Responses => "RESPONSES", Self::ClientOps => "CLIENT_OPS", - Self::Notifications => "NOTIFICATIONS", Self::Global => "GLOBAL", Self::GlobalExt => "GLOBAL_EXT", } @@ -59,7 +56,6 @@ impl AcpStream { format!("{p}.v1.session.*.agent.cancelled"), ], Self::ClientOps => vec![format!("{p}.v1.session.*.client.>")], - Self::Notifications => vec![format!("{p}.v1.session.*.agent.update")], Self::Global => vec![ format!("{p}.v1.global.agent.initialize"), format!("{p}.v1.global.agent.authenticate"), @@ -82,7 +78,7 @@ impl AcpStream { } } - pub fn all_configs(prefix: &AcpPrefix) -> [Config; 6] { + pub fn all_configs(prefix: &AcpPrefix) -> [Config; 5] { Self::ALL.map(|s| s.config(prefix)) } } diff --git a/rsworkspace/crates/acp/acp-nats/src/nats/subjects/tests.rs b/rsworkspace/crates/acp/acp-nats/src/nats/subjects/tests.rs index b3b0412968..75fc47afb2 100644 --- a/rsworkspace/crates/acp/acp-nats/src/nats/subjects/tests.rs +++ b/rsworkspace/crates/acp/acp-nats/src/nats/subjects/tests.rs @@ -200,14 +200,6 @@ fn session_agent_ext_ready() { ); } -#[test] -fn session_agent_update() { - assert_eq!( - responses::UpdateSubject::new(&p("acp"), &sid("s1")).to_string(), - "acp.v1.session.s1.agent.update" - ); -} - #[test] fn session_agent_response() { assert_eq!( @@ -402,7 +394,6 @@ fn stream_assignments() { assert_eq!(responses::CancelledSubject::STREAM, Some(AcpStream::Responses)); assert_eq!(responses::ExtReadySubject::STREAM, Some(AcpStream::Responses)); assert_eq!(responses::ResponseSubject::STREAM, Some(AcpStream::Responses)); - assert_eq!(responses::UpdateSubject::STREAM, Some(AcpStream::Notifications)); assert_eq!(client_ops::FsReadTextFileSubject::STREAM, Some(AcpStream::ClientOps)); assert_eq!(client_ops::FsWriteTextFileSubject::STREAM, Some(AcpStream::ClientOps)); @@ -441,7 +432,6 @@ fn acp_stream_display() { assert_eq!(AcpStream::Commands.to_string(), "COMMANDS"); assert_eq!(AcpStream::Responses.to_string(), "RESPONSES"); assert_eq!(AcpStream::ClientOps.to_string(), "CLIENT_OPS"); - assert_eq!(AcpStream::Notifications.to_string(), "NOTIFICATIONS"); assert_eq!(AcpStream::Global.to_string(), "GLOBAL"); assert_eq!(AcpStream::GlobalExt.to_string(), "GLOBAL_EXT"); } diff --git a/rsworkspace/crates/acp/acp-nats/src/tests.rs b/rsworkspace/crates/acp/acp-nats/src/tests.rs index 99b202f353..a40f9b51b4 100644 --- a/rsworkspace/crates/acp/acp-nats/src/tests.rs +++ b/rsworkspace/crates/acp/acp-nats/src/tests.rs @@ -1,7 +1,6 @@ use super::*; use agent_client_protocol::schema::v1::{ - ContentBlock, ContentChunk, RequestPermissionRequest, RequestPermissionResponse, SessionNotification, - SessionUpdate, TextContent, ToolCallUpdate, ToolCallUpdateFields, + RequestPermissionRequest, RequestPermissionResponse, SessionNotification, ToolCallUpdate, ToolCallUpdateFields, }; use std::sync::{Arc, Mutex}; @@ -41,32 +40,6 @@ impl crate::ClientHandler for MockClient { } } -#[tokio::test] -async fn spawn_notification_forwarder_delivers_notifications() { - let client = MockClient::new(None); - let received = client.received.clone(); - let (tx, rx) = tokio::sync::mpsc::channel(16); - - let local = tokio::task::LocalSet::new(); - local - .run_until(async { - spawn_notification_forwarder(client, rx); - - let notif = SessionNotification::new( - "s1", - SessionUpdate::AgentMessageChunk(ContentChunk::new(ContentBlock::Text(TextContent::new("hello")))), - ); - tx.send(notif).await.unwrap(); - drop(tx); - - tokio::task::yield_now().await; - tokio::task::yield_now().await; - }) - .await; - - assert_eq!(received.lock().unwrap().len(), 1); -} - #[tokio::test] async fn mock_client_request_permission_returns_error() { let client = MockClient::new(None); @@ -76,29 +49,3 @@ async fn mock_client_request_permission_returns_error() { .await; assert!(result.is_err()); } - -#[tokio::test] -async fn spawn_notification_forwarder_stops_on_client_error() { - let client = MockClient::new(Some(0)); - let received = client.received.clone(); - let (tx, rx) = tokio::sync::mpsc::channel(16); - - let local = tokio::task::LocalSet::new(); - local - .run_until(async { - spawn_notification_forwarder(client, rx); - - let notif = SessionNotification::new( - "s1", - SessionUpdate::AgentMessageChunk(ContentChunk::new(ContentBlock::Text(TextContent::new("hello")))), - ); - let _ = tx.send(notif).await; - drop(tx); - - tokio::task::yield_now().await; - tokio::task::yield_now().await; - }) - .await; - - assert_eq!(received.lock().unwrap().len(), 0); -} diff --git a/rsworkspace/crates/channel/channel-bridge-telegram/src/main.rs b/rsworkspace/crates/channel/channel-bridge-telegram/src/main.rs index 0bf841b1b6..2af617d148 100644 --- a/rsworkspace/crates/channel/channel-bridge-telegram/src/main.rs +++ b/rsworkspace/crates/channel/channel-bridge-telegram/src/main.rs @@ -28,7 +28,7 @@ mod render; // coverage build and is exercised by the module tests. #[cfg(not(coverage))] use { - acp_nats::{AgentHandler, ClientHandler}, + acp_nats::AgentHandler, acp_port::{AcpBridge, AcpPort, SessionMethods}, agent_client_protocol::schema::ProtocolVersion, agent_client_protocol::schema::v1::InitializeRequest, @@ -149,7 +149,6 @@ async fn run( config: BridgeConfig, ) -> anyhow::Result<()> { let meter = trogon_telemetry::meter("channel-bridge-telegram"); - let (notification_tx, mut notification_rx) = tokio::sync::mpsc::channel(64); let js_client = trogon_nats::jetstream::NatsJetStreamClient::new(async_nats::jetstream::new(nats_client.clone())); let bridge: Arc = Arc::new(acp_nats::Bridge::new( nats_client.clone(), @@ -157,7 +156,6 @@ async fn run( trogon_std::time::SystemClock, &meter, config.acp.clone(), - notification_tx, )); let renderer = Arc::new(TelegramRenderClient::new()); @@ -166,16 +164,6 @@ async fn run( renderer.clone(), bridge.clone(), )); - let renderer_for_rx = renderer.clone(); - let mut notification_task = tokio::task::spawn_local(async move { - while let Some(notification) = notification_rx.recv().await { - if let Err(e) = renderer_for_rx.session_notification(notification).await { - error!(error = ?e, "Render client rejected a session notification"); - break; - } - } - }); - let initialized = bridge .initialize(InitializeRequest::new(ProtocolVersion::LATEST)) .await @@ -213,10 +201,6 @@ async fn run( error!(?result, "ACP client task ended; agent responses can no longer be rendered"); break; } - result = &mut notification_task => { - error!(?result, "Notification task ended; agent responses can no longer be rendered"); - break; - } next = messages.next() => { let Some(next) = next else { warn!("Inbound consumer stream ended"); @@ -237,7 +221,6 @@ async fn run( } client_task.abort(); - notification_task.abort(); Ok(()) } diff --git a/rsworkspace/crates/platform/trogon-nats/tests/subject_literal_policy.rs b/rsworkspace/crates/platform/trogon-nats/tests/subject_literal_policy.rs index 16a5f7e2ff..9f1252391f 100644 --- a/rsworkspace/crates/platform/trogon-nats/tests/subject_literal_policy.rs +++ b/rsworkspace/crates/platform/trogon-nats/tests/subject_literal_policy.rs @@ -46,7 +46,7 @@ const KNOWN_SITES: [(&str, usize); 13] = [ ("crates/a2a/a2a-nats/src/gateway_ingress.rs", 8), ("crates/a2a/a2a-nats/src/push/dlq.rs", 1), ("crates/a2a/a2a-nats/src/server/bridge.rs", 2), - ("crates/acp/acp-nats/src/jetstream/consumers.rs", 2), + ("crates/acp/acp-nats/src/jetstream/consumers.rs", 1), ]; fn workspace_root() -> PathBuf { From 2e01f2bb997d203a9e9ccc960536182c15a16aae Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Thu, 13 Aug 2026 02:34:23 -0400 Subject: [PATCH 02/12] fix(a2a): let one JSON-RPC envelope define the wire at every edge Streaming clients could not correlate SSE frames to the call that started them, because chunks left as named SSE events carrying no JSON-RPC id. Four separate types spelled the single JSON-RPC id, so each edge decided for itself what the wire looked like and the shared codec was bypassed. Audit traffic could not be filtered per agent: the emitter was handed an agent id and dropped it, spending the subject terminal on the method instead. Signed-off-by: Yordis Prieto --- ...55-nats-subject-design-jsonrpc-bindings.md | 52 ++++-- ...0056-canonical-jsonrpc-bodies-over-nats.md | 37 +++- .../crates/a2a/a2a-bridge/src/constants.rs | 3 + .../crates/a2a/a2a-bridge/src/inbound.rs | 39 +++-- .../a2a-bridge/src/nats_transport_harness.rs | 7 +- .../src/nats_transport_harness/tests.rs | 32 ++-- .../a2a/a2a-gateway/src/gw_ingress_stream.rs | 11 +- .../src/gw_ingress_stream/tests.rs | 9 + .../src/runtime/audit_publish/tests.rs | 6 +- .../crates/a2a/a2a-gateway/src/runtime/env.rs | 5 +- .../src/runtime/tier1_denial/tests.rs | 2 +- .../a2a/a2a-nats-http/src/handlers/mod.rs | 4 +- .../crates/a2a/a2a-nats-http/src/rest.rs | 4 +- .../crates/a2a/a2a-nats-http/src/sse.rs | 14 +- .../crates/a2a/a2a-nats-stdio/Cargo.toml | 2 +- .../crates/a2a/a2a-nats-stdio/src/dispatch.rs | 60 +++---- .../a2a/a2a-nats-stdio/src/dispatch/tests.rs | 161 ++++++++---------- .../crates/a2a/a2a-nats-stdio/src/io_loop.rs | 45 ++--- .../a2a-nats-stdio/src/io_loop/parse_tests.rs | 38 +++-- .../a2a/a2a-nats-stdio/src/io_loop/tests.rs | 2 +- .../crates/a2a/a2a-nats-stdio/src/wire.rs | 128 ++++++-------- .../a2a/a2a-nats-stdio/src/wire/tests.rs | 47 +++-- .../crates/a2a/a2a-nats/src/audit/emitter.rs | 18 +- .../a2a/a2a-nats/src/audit/emitter/tests.rs | 23 ++- .../a2a/a2a-nats/src/client/event_stream.rs | 20 ++- .../a2a-nats/src/client/event_stream/tests.rs | 58 ++++++- .../a2a/a2a-nats/src/client/streaming.rs | 4 +- .../crates/a2a/a2a-nats/src/client/unary.rs | 13 +- .../crates/a2a/a2a-nats/src/client/wire.rs | 20 +-- .../a2a/a2a-nats/src/client/wire/tests.rs | 11 +- .../a2a/a2a-nats/src/gateway_ingress.rs | 17 +- .../crates/a2a/a2a-nats/src/jsonrpc.rs | 45 +---- .../crates/a2a/a2a-nats/src/jsonrpc/tests.rs | 38 ++--- rsworkspace/crates/a2a/a2a-nats/src/lib.rs | 3 +- .../a2a/a2a-nats/src/server/message_stream.rs | 38 +++-- .../src/server/message_stream/tests.rs | 24 +++ .../crates/a2a/a2a-nats/src/server/wire.rs | 5 +- .../a2a/a2a-nats/src/server/wire/tests.rs | 7 +- 38 files changed, 554 insertions(+), 498 deletions(-) diff --git a/docs/adr/0055-nats-subject-design-jsonrpc-bindings.md b/docs/adr/0055-nats-subject-design-jsonrpc-bindings.md index e227d13c28..3e0c1d3464 100644 --- a/docs/adr/0055-nats-subject-design-jsonrpc-bindings.md +++ b/docs/adr/0055-nats-subject-design-jsonrpc-bindings.md @@ -282,7 +282,7 @@ the configured prefix, and no ACP or A2A code reads a protocol version at the transport layer today. Record the rule now; skip the implementation. **Revisit trigger.** The first time a durable stream must survive a protocol -version bump. ACP's `COMMANDS`, `RESPONSES`, and `NOTIFICATIONS` streams are +version bump. ACP's `COMMANDS`, `RESPONSES`, and `CLIENT_OPS` streams are `Limits` retention with `max_age`, so stored messages outlive their connection. If a payload shape changes across an ACP version, a replaying consumer has no way to pick the right schema, and `v{major}` cannot help because it versions @@ -309,9 +309,16 @@ MUST NOT mint a per-request subject such as `...response.{req_id}`. ACP terminal durable results use one subject shape: `{prefix}.v{major}.session.{session_id}.agent.response`. The historical -`...agent.prompt.response.{req_id}` path collapses into that subject; mid-flight -prompt progress remains on `...agent.update`. Partitioning terminal results by -method is unnecessary once demux is on the correlation token. +`...agent.prompt.response.{req_id}` path collapses into that subject. Partitioning +terminal results by method is unnecessary once demux is on the correlation token. + +Mid-flight progress is *not* a durable agent response. `session/update` is a +client-directed notification under the ACP method surface, so it takes the +client-op terminal `{prefix}.v{major}.session.{session_id}.client.session.update` +alongside every other agent-to-client call, and rides the `CLIENT_OPS` stream. +Routing it under `...agent.update` instead would fork one method across two role +segments and scope progress to prompts only, which drops the `session/load` +replay that emits the same notification. ### Streams @@ -369,21 +376,27 @@ this ADR's cardinality, `v{major}`, limits, and left-prefix rules. They use an - Entity tokens stay long-lived (agent, caller, task). Per-request identifiers remain forbidden. -#### Audit subject defects (current code) +#### Audit subject defects -Today's audit emitters are non-conformant and MUST be corrected as part of -baseline alignment: +Audit subjects MUST be entity-scoped with fixed terminals, for example +`a2a.v1.audit.{agent_id}.{outcome}` (method in the payload or a header), not +method-led growth without an entity token. Two defects motivated the rule: - `a2a.audit.{outcome}.{method}` grows with the method set and embeds the method as a subject token, defeating the fixed-terminal rule. -- The emitter accepts `agent_id` and then drops it (`let _ = agent_id`), so audit - traffic cannot be filtered per agent. +- An emitter that accepts `agent_id` and then drops it (`let _ = agent_id`) + leaves audit traffic unfilterable per agent. + +Exact terminals are an implementation detail under this profile; the two defects +above are the conformance requirement. -Target shape is entity-scoped, for example -`a2a.v1.audit.{agent_id}.{outcome}` (fixed outcome terminals; method in the -payload or a header), not method-led growth without an entity token. Exact -terminals are an implementation detail under this profile; the defects above are -the conformance requirement. +The A2A emitter (`a2a-nats::audit::emitter`) now conforms, publishing +`{prefix}.v1.audit.{agent_id}.{ok|err}` and +`{prefix}.v1.audit.{agent_id}.lifecycle`. The gateway's ingress audit builder +(`a2a-gateway::audit_ingress::ingress_audit_subject`) still emits +`{prefix}.a2a.audit.{outcome}.ingress.{skill}` — duplicated root, no `v{major}`, +skill as a growing terminal, no entity token. It has no production caller today, +so it is corrected when the ingress audit path is wired rather than ahead of it. ### Limits @@ -437,10 +450,13 @@ non-conformant with this standard; they are not the standard: prefix. - **Request id in ACP response and update subjects** (`...agent.response.{req_id}`, `...agent.update.{req_id}`) and the prompt- - specific `...agent.prompt.response.{req_id}`. Replace with entity-scoped - `...agent.response` and `...agent.update`, correlating on ACP's - transport-minted JSON-RPC `id`. Collapse the prompt-specific terminal into - `...agent.response`. + specific `...agent.prompt.response.{req_id}`. Replace with the entity-scoped + `...agent.response`, correlating on ACP's transport-minted JSON-RPC `id`, and + collapse the prompt-specific terminal into it. +- **Duplicate ACP progress subject.** `...agent.update` and + `...client.session.update` both claimed `session/update`, and only the latter + had a publisher. Remove `...agent.update` and its `NOTIFICATIONS` stream; + progress rides the client-op subtree. - **Request id in A2A task event subjects** (`...tasks.{task_id}.events.{req_id}`). Replace with `...tasks.{task_id}.events` plus `Trogon-Req-Id` (A2A ids are peer-supplied). This diff --git a/docs/adr/0056-canonical-jsonrpc-bodies-over-nats.md b/docs/adr/0056-canonical-jsonrpc-bodies-over-nats.md index 53cdc2b764..e4cefaabbe 100644 --- a/docs/adr/0056-canonical-jsonrpc-bodies-over-nats.md +++ b/docs/adr/0056-canonical-jsonrpc-bodies-over-nats.md @@ -34,7 +34,7 @@ now needs: message. This is *envelope* reconstruction and is unrelated to [ADR#0021](./0021-typed-decode-over-passthrough-forwarding.md), which rejected *payload* passthrough (forwarding `params` as an untyped `Value` instead of - decoding it). [ADR#0021](0021-typed-decode-over-passthrough-forwarding.md) is unaffected by this ADR; see §7. + decoding it). [ADR#0021](0021-typed-decode-over-passthrough-forwarding.md) is unaffected by this ADR; see §8. [ADR#0041](./0041-canonical-mcp-jsonrpc-bodies-over-nats.md) (draft) already moves MCP to canonical full-envelope bodies with non-authoritative header @@ -148,7 +148,34 @@ This ADR adds only that the `id` is application-level, travels authoritatively i the body, and is projected to `Jsonrpc-Id`. `Nats-Msg-Id` stays reserved for JetStream deduplication. -### 5. MCP-specific rules carried from [ADR#0041](0041-canonical-mcp-jsonrpc-bodies-over-nats.md) +### 5. A streaming chunk is a response, not a notification + +A2A streams `message/stream` and `tasks/resubscribe` as a sequence of JSON-RPC +**success responses that repeat the request id**, terminated by the one whose +result is final. Each chunk on the NATS leg carries that complete object: + +```json +{"jsonrpc": "2.0", "id": "", "result": { "statusUpdate": { } }} +``` + +This is not a stylistic choice. A server push carrying both an `id` and a +`method` is a *request* under JSON-RPC, and one carrying a `method` without an +`id` is a notification the caller cannot correlate to the subscription it opened. +Only the response shape says "this belongs to the call you made" without +inventing a member the specification does not define. + +Because the chunk is complete at the point of publish, every hop after it +forwards the bytes: the gateway egress pump, the HTTP bridge, the SSE facade, +and the stdio bridge each emit the body verbatim rather than each inventing an +envelope. An edge that owns the caller's id (a bridge whose caller minted its own +`id`) rewrites only that member. + +ACP is the opposite case and stays so: its mid-turn `session/update` really is a +notification (id-less, method-bearing) and the terminal `PromptResponse` is the +only message answering the request id. The difference is in the upstream +protocols, not in this binding. + +### 6. MCP-specific rules carried from [ADR#0041](0041-canonical-mcp-jsonrpc-bodies-over-nats.md) These remain in force for MCP and are unchanged by the ACP/A2A migration: @@ -159,14 +186,14 @@ These remain in force for MCP and are unchanged by the ACP/A2A migration: `Mcp-Name`, `Mcp-Param-*`. `Mcp-Session-Id` and unrelated HTTP headers are not forwarded. Those headers remain derived metadata validated against the body. -### 6. Shared package; legacy content mode removed after migration +### 7. Shared package; legacy content mode removed after migration The shared `jsonrpc-nats` package owns encode/decode. After ACP and A2A migrate to the canonical APIs, the legacy content-mode `encode`/`decode` are removed (or deprecated with a removal milestone). A bridge that hand-builds an envelope forwards the body unmodified instead, where that assembly is redundant. -### 7. What this ADR does not change +### 8. What this ADR does not change - **Typed payload decode stays.** [ADR#0021](./0021-typed-decode-over-passthrough-forwarding.md) (accepted) keeps @@ -198,6 +225,8 @@ forwards the body unmodified instead, where that assembly is redundant. subject terminal that is not the body method's projection under the binding's method-to-terminal mapping. - `"jsonrpc":"2.0"` travels in the body; it is not duplicated into a header. +- Every A2A stream chunk deserializes as a JSON-RPC success response bearing the + request id, on the NATS leg and on every edge that forwards it. - MCP `params._meta` and allowlisted `Mcp-*` / `MCP-Protocol-Version` headers survive an HTTP proxy to NATS round trip. diff --git a/rsworkspace/crates/a2a/a2a-bridge/src/constants.rs b/rsworkspace/crates/a2a/a2a-bridge/src/constants.rs index 8b790154a4..4f1c947616 100644 --- a/rsworkspace/crates/a2a/a2a-bridge/src/constants.rs +++ b/rsworkspace/crates/a2a/a2a-bridge/src/constants.rs @@ -1 +1,4 @@ pub(crate) const AGENT_ID_HEADER: &str = "x-a2a-agent-id"; + +/// JSON-RPC 2.0 reserved code for a server-side failure. +pub(crate) const INTERNAL_ERROR: i32 = -32603; diff --git a/rsworkspace/crates/a2a/a2a-bridge/src/inbound.rs b/rsworkspace/crates/a2a/a2a-bridge/src/inbound.rs index 395ea53933..f57a1a8322 100644 --- a/rsworkspace/crates/a2a/a2a-bridge/src/inbound.rs +++ b/rsworkspace/crates/a2a/a2a-bridge/src/inbound.rs @@ -29,7 +29,7 @@ use a2a_nats::{A2aPrefix, A2aTaskId, ReqId}; use a2a_auth_callout::{CALLER_JWT_HEADER_NAME, CallerJwtHeaderValue, MintedUserJwt}; use crate::auth::AuthCalloutClient; -use crate::constants::AGENT_ID_HEADER; +use crate::constants::{AGENT_ID_HEADER, INTERNAL_ERROR}; use crate::error::BridgeError; use crate::identity::{BridgeAgentId, BridgeUserJwt, CallerHttpsAuth}; @@ -475,32 +475,34 @@ impl TaskJetStreamPort for ScriptedTaskJetstream { } } -fn sse_gateway_line(body: &[u8]) -> Event { - Event::default() - .event("gateway-bootstrap") - .data(String::from_utf8_lossy(body)) +/// Every A2A SSE frame is an unnamed `data:` line carrying a JSON-RPC response +/// that repeats the caller's request id. Naming the events instead would put the +/// bootstrap and the task chunks on distinct SSE event types, which a spec client +/// never subscribes to, and the bodies already say which is which. +fn sse_data_line(body: &[u8]) -> Event { + Event::default().data(String::from_utf8_lossy(body)) } -fn sse_task_line(body: &Bytes) -> Event { - Event::default() - .event("task-event") - .data(String::from_utf8_lossy(body.as_ref())) -} - -fn sse_error_line(err: &BridgeError) -> Event { - Event::default().event("error").data(err.to_string()) +fn sse_error_line(caller_id: &Value, err: &BridgeError) -> Event { + let envelope = serde_json::json!({ + "jsonrpc": "2.0", + "id": caller_id, + "error": { "code": INTERNAL_ERROR, "message": err.to_string() }, + }); + Event::default().data(envelope.to_string()) } fn sse_from_bootstrap_and_payloads( bootstrap_owned: Vec, tail: Pin> + Send>>, + caller_id: Value, ) -> BoxStream<'static, Result> { - let head_event = sse_gateway_line(&bootstrap_owned); + let head_event = sse_data_line(&bootstrap_owned); let head = futures_util::stream::once(futures_util::future::ready(Ok::(head_event))); - let tail_mapped = tail.map(|item| { + let tail_mapped = tail.map(move |item| { Ok::(match item { - Ok(chunk) => sse_task_line(&chunk), - Err(ref err) => sse_error_line(err), + Ok(chunk) => sse_data_line(chunk.as_ref()), + Err(ref err) => sse_error_line(&caller_id, err), }) }); @@ -744,7 +746,8 @@ pub async fn handle_jsonrpc(headers: HeaderMap, body: bytes::Bytes, state: &AppS } None => Box::pin(futures_util::stream::empty()), }; - let merged = sse_from_bootstrap_and_payloads(unary_reply.to_vec(), payloads); + let caller_id = v.get("id").cloned().unwrap_or(Value::Null); + let merged = sse_from_bootstrap_and_payloads(unary_reply.to_vec(), payloads, caller_id); return Ok(Sse::new(merged).keep_alive(KeepAlive::default()).into_response()); } diff --git a/rsworkspace/crates/a2a/a2a-bridge/src/nats_transport_harness.rs b/rsworkspace/crates/a2a/a2a-bridge/src/nats_transport_harness.rs index 27419d209a..45d342626e 100644 --- a/rsworkspace/crates/a2a/a2a-bridge/src/nats_transport_harness.rs +++ b/rsworkspace/crates/a2a/a2a-bridge/src/nats_transport_harness.rs @@ -122,7 +122,12 @@ pub fn build_nats_transport_app_state( let harness = Arc::new(HarnessGatewayUnary::new(nats, prefix.clone(), agent)); let publisher = GatewayInboundPublisher::new(harness.clone()); let jetstream: Arc = Arc::new(ScriptedTaskJetstream::single_ok( - json!({ "event": "task-status", "taskId": "task-sse-1" }).to_string(), + json!({ + "jsonrpc": "2.0", + "id": "corr-1", + "result": { "statusUpdate": { "taskId": "task-sse-1" } } + }) + .to_string(), )); let tenant = BridgeTenantAccount::new(HARNESS_TENANT).expect("harness tenant"); diff --git a/rsworkspace/crates/a2a/a2a-bridge/src/nats_transport_harness/tests.rs b/rsworkspace/crates/a2a/a2a-bridge/src/nats_transport_harness/tests.rs index 851a2aba87..97301b6144 100644 --- a/rsworkspace/crates/a2a/a2a-bridge/src/nats_transport_harness/tests.rs +++ b/rsworkspace/crates/a2a/a2a-bridge/src/nats_transport_harness/tests.rs @@ -90,7 +90,7 @@ async fn nats_transport_message_send_round_trips_caller_jwt_and_audit() { let audit_subject = nats .published_messages() .into_iter() - .find(|subject| subject.contains(".audit.ok.message.send")); + .find(|subject| subject.ends_with(".audit.planner.ok")); assert!(audit_subject.is_some(), "expected gateway audit publish"); } @@ -125,25 +125,29 @@ async fn nats_transport_tasks_resubscribe_bootstraps_sse_stream() { ); let mut stream = response.into_body().into_data_stream(); - let mut saw_bootstrap = false; - let mut saw_task_event = false; + let mut wire = String::new(); while let Some(chunk) = stream.next().await { - let chunk = chunk.unwrap(); - let text = String::from_utf8_lossy(&chunk); - if text.contains("gateway-bootstrap") { - saw_bootstrap = true; - } - if text.contains("task-event") { - saw_task_event = true; - } + wire.push_str(&String::from_utf8_lossy(&chunk.unwrap())); } - assert!(saw_bootstrap, "expected SSE gateway bootstrap line"); - assert!(saw_task_event, "expected SSE JetStream task event line"); + assert!( + !wire.contains("event:"), + "A2A SSE frames are unnamed data lines, got: {wire}" + ); + let frames: Vec = wire + .lines() + .filter_map(|line| line.strip_prefix("data:")) + .map(|data| serde_json::from_str(data.trim()).expect("each frame is a JSON-RPC body")) + .collect(); + assert_eq!(frames.len(), 2, "bootstrap plus one task event, got: {wire}"); + assert_eq!(frames[0]["result"]["taskId"], "task-sse-1"); + assert_eq!(frames[1]["jsonrpc"], "2.0"); + assert_eq!(frames[1]["id"], "corr-1"); + assert_eq!(frames[1]["result"]["statusUpdate"]["taskId"], "task-sse-1"); let audit_subject = nats .published_messages() .into_iter() - .find(|subject| subject.contains(".audit.ok.tasks.resubscribe")); + .find(|subject| subject.ends_with(".audit.planner.ok")); assert!(audit_subject.is_some(), "expected resubscribe audit publish"); } diff --git a/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream.rs b/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream.rs index 3f0989ed4b..888975d056 100644 --- a/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream.rs +++ b/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream.rs @@ -579,11 +579,20 @@ fn parse_last_event_id_as_u64(value: serde_json::Value) -> Option { } } +/// Correlation id for a streaming ingress envelope. A JSON-RPC `null` id +/// yields `None` for the same reason as [`crate::runtime::env::json_rpc_audit_req_id`]: +/// no caller correlation is possible, and a synthesized `"null"` token would +/// alias unrelated envelopes onto one stream pump. pub fn req_id_from_headers_or_payload(headers: &async_nats::HeaderMap, payload: &[u8]) -> Option { if let Some(value) = headers.get(a2a_nats::constants::REQ_ID_HEADER) { return Some(ReqId::from_header(value.as_str())); } - a2a_nats::jsonrpc::extract_request_id_from_body(payload).map(|id| ReqId::from_header(id.to_string())) + let text = match a2a_nats::jsonrpc::extract_request_id_from_body(payload)? { + a2a_nats::ResponseId::Null => return None, + a2a_nats::ResponseId::Number(n) => n.to_string(), + a2a_nats::ResponseId::String(s) => s, + }; + Some(ReqId::from_header(text)) } #[cfg(test)] diff --git a/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream/tests.rs b/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream/tests.rs index 3770834019..8c589d84a6 100644 --- a/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream/tests.rs +++ b/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream/tests.rs @@ -199,6 +199,15 @@ fn req_id_from_payload_when_header_absent() { assert_eq!(req_id.as_str(), "req-from-payload"); } +#[test] +fn req_id_is_none_for_a_null_json_rpc_id() { + // A `"null"` string would collapse every null-id envelope onto one + // correlation key, so the pump is not spawned at all. + let headers = async_nats::HeaderMap::new(); + let payload = br#"{"jsonrpc":"2.0","id":null,"method":"x"}"#; + assert!(req_id_from_headers_or_payload(&headers, payload).is_none()); +} + #[test] fn streaming_ingress_kind_resubscribe_requires_task_id_by_type() { // Compile-only check: the variant has `task_id` as a non-optional diff --git a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/audit_publish/tests.rs b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/audit_publish/tests.rs index 8e53d0c7ca..6529787920 100644 --- a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/audit_publish/tests.rs +++ b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/audit_publish/tests.rs @@ -74,18 +74,18 @@ async fn enabled_publish_targets_ok_audit_subject() { spawn_gateway_audit_publish(true, nats.clone(), prefix(), agent(), ok_envelope()); wait_for_publish(&nats, 1).await; let subjects = nats.published_messages(); - assert_eq!(subjects, vec!["a2a.v1.audit.ok.message.send".to_owned()]); + assert_eq!(subjects, vec!["a2a.v1.audit.planner.ok".to_owned()]); } #[tokio::test] async fn enabled_publish_targets_err_audit_subject() { - // Outcome::Err must route to the `.err.` subject so consumers + // Outcome::Err must route to the `err` terminal so consumers // can subscribe to denials independently of allow-throughs. let nats = MockNatsClient::new(); spawn_gateway_audit_publish(true, nats.clone(), prefix(), agent(), err_envelope()); wait_for_publish(&nats, 1).await; let subjects = nats.published_messages(); - assert_eq!(subjects, vec!["a2a.v1.audit.err.tasks.get".to_owned()]); + assert_eq!(subjects, vec!["a2a.v1.audit.planner.err".to_owned()]); } #[tokio::test] diff --git a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env.rs b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env.rs index 6d607347a3..267b84b811 100644 --- a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env.rs +++ b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env.rs @@ -146,8 +146,9 @@ pub fn json_rpc_params(payload: &[u8]) -> serde_json::Value { /// unrelated envelopes onto the same audit row. pub fn json_rpc_audit_req_id(payload: &[u8]) -> Option { match a2a_nats::jsonrpc::extract_request_id_from_body(payload)? { - a2a_nats::JsonRpcId::Null => None, - id => Some(id.to_string()), + a2a_nats::ResponseId::Null => None, + a2a_nats::ResponseId::Number(n) => Some(n.to_string()), + a2a_nats::ResponseId::String(s) => Some(s), } } diff --git a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/tier1_denial/tests.rs b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/tier1_denial/tests.rs index 3da10b6b31..3e6c83fdf0 100644 --- a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/tier1_denial/tests.rs +++ b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/tier1_denial/tests.rs @@ -96,7 +96,7 @@ async fn deny_tier1_writes_reply_and_audit_when_publishing_enabled() { wait_for(&nats, 2).await; let subjects = nats.published_messages(); assert!(subjects.contains(&"_INBOX.reply".to_owned())); - assert!(subjects.iter().any(|s| s == "a2a.v1.audit.err.message.send")); + assert!(subjects.iter().any(|s| s == "a2a.v1.audit.planner.err")); } #[tokio::test] diff --git a/rsworkspace/crates/a2a/a2a-nats-http/src/handlers/mod.rs b/rsworkspace/crates/a2a/a2a-nats-http/src/handlers/mod.rs index 5f3af42bc9..6a38a2b460 100644 --- a/rsworkspace/crates/a2a/a2a-nats-http/src/handlers/mod.rs +++ b/rsworkspace/crates/a2a/a2a-nats-http/src/handlers/mod.rs @@ -85,7 +85,7 @@ where let bootstrap_sse = futures::stream::once(async move { Ok::(Event::default().data(String::from_utf8_lossy(&bootstrap_bytes))) }); - let sse_stream = typed_event_stream_to_sse(stream, id, "message/stream"); + let sse_stream = typed_event_stream_to_sse(stream, id); sse_response(bootstrap_sse.chain(sse_stream)) } Err(e) => jsonrpc_error_response(&id, &e), @@ -163,7 +163,7 @@ where let snapshot_sse = futures::stream::once(async move { Ok::(Event::default().data(String::from_utf8_lossy(&snapshot_bytes))) }); - let sse_stream = typed_event_stream_to_sse(stream, id, "tasks/resubscribe"); + let sse_stream = typed_event_stream_to_sse(stream, id); sse_response(snapshot_sse.chain(sse_stream)) } Err(e) => jsonrpc_error_response(&id, &e), diff --git a/rsworkspace/crates/a2a/a2a-nats-http/src/rest.rs b/rsworkspace/crates/a2a/a2a-nats-http/src/rest.rs index 5c825f1809..7c66d574e5 100644 --- a/rsworkspace/crates/a2a/a2a-nats-http/src/rest.rs +++ b/rsworkspace/crates/a2a/a2a-nats-http/src/rest.rs @@ -124,7 +124,7 @@ where Event::default().data(serde_json::to_string(&bootstrap_event).unwrap_or_default()), ) }); - sse_response(bootstrap_sse.chain(typed_event_stream_to_sse(stream, Value::Null, "message/stream"))) + sse_response(bootstrap_sse.chain(typed_event_stream_to_sse(stream, Value::Null))) } Err(e) => rest_error_response(&e), } @@ -283,7 +283,7 @@ where Event::default().data(serde_json::to_string(&snapshot_event).unwrap_or_default()), ) }); - sse_response(snapshot_sse.chain(typed_event_stream_to_sse(stream, Value::Null, "tasks/resubscribe"))) + sse_response(snapshot_sse.chain(typed_event_stream_to_sse(stream, Value::Null))) } Err(e) => rest_error_response(&e), } diff --git a/rsworkspace/crates/a2a/a2a-nats-http/src/sse.rs b/rsworkspace/crates/a2a/a2a-nats-http/src/sse.rs index 556492b2f1..f9322b8d84 100644 --- a/rsworkspace/crates/a2a/a2a-nats-http/src/sse.rs +++ b/rsworkspace/crates/a2a/a2a-nats-http/src/sse.rs @@ -9,20 +9,18 @@ use futures::StreamExt; pub fn typed_event_stream_to_sse( stream: TypedEventStream, jsonrpc_id: serde_json::Value, - method: &'static str, ) -> impl Stream> { stream.map(move |item| { let data = match item { Ok(response) => { - // Stream events ride as JSON-RPC notifications tagged with the - // originating method so callers can route incremental events, - // matching the a2a-nats-stdio wire shape. Errors keep the - // result-or-error envelope shape with the request id for - // correlation. + // The A2A spec streams every chunk as a JSON-RPC success response + // repeating the request id, terminated by the one whose result is + // final. The id is the caller's, not the transport's, because the + // caller correlates against the id they sent. let envelope = serde_json::json!({ "jsonrpc": "2.0", - "method": method, - "params": response, + "id": jsonrpc_id, + "result": response, }); serde_json::to_string(&envelope).unwrap_or_else(|e| { // Server-side serialization failure is `-32603` Internal, diff --git a/rsworkspace/crates/a2a/a2a-nats-stdio/Cargo.toml b/rsworkspace/crates/a2a/a2a-nats-stdio/Cargo.toml index 7afa8912c2..ef4868f692 100644 --- a/rsworkspace/crates/a2a/a2a-nats-stdio/Cargo.toml +++ b/rsworkspace/crates/a2a/a2a-nats-stdio/Cargo.toml @@ -20,6 +20,7 @@ a2a-nats = { workspace = true } async-nats = { workspace = true } bytes = { workspace = true } futures = { workspace = true } +jsonrpc-nats = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } tokio = { workspace = true, features = ["rt-multi-thread", "macros", "io-std", "signal"] } @@ -28,7 +29,6 @@ trogon-nats = { workspace = true } trogon-std = { workspace = true, features = ["signal"] } [dev-dependencies] -jsonrpc-nats = { workspace = true } tokio = { workspace = true, features = ["test-util"] } trogon-nats = { workspace = true, features = ["test-support"] } trogon-std = { workspace = true, features = ["test-support"] } diff --git a/rsworkspace/crates/a2a/a2a-nats-stdio/src/dispatch.rs b/rsworkspace/crates/a2a/a2a-nats-stdio/src/dispatch.rs index 61b5ce9091..9e5005778a 100644 --- a/rsworkspace/crates/a2a/a2a-nats-stdio/src/dispatch.rs +++ b/rsworkspace/crates/a2a/a2a-nats-stdio/src/dispatch.rs @@ -6,15 +6,16 @@ use a2a::types::{ use a2a_nats::client::{A2aClient, ClientError, ValidatedRpc}; use a2a_nats::task_id::A2aTaskId; use futures::StreamExt; +use jsonrpc_nats::{RequestId, ResponseId}; use serde_json::Value; use tokio::sync::mpsc; use trogon_nats::RequestClient; use trogon_nats::jetstream::{JetStreamCreateConsumer, JetStreamGetStream, JsAck, JsMessageOf, JsMessageRef}; use crate::constants::{INVALID_PARAMS, METHOD_NOT_FOUND}; -use crate::wire::{OutboundError, OutboundFrame, OutboundNotification, RpcId}; +use crate::wire::OutboundFrame; -fn client_err_to_frame(id: RpcId, err: ClientError) -> OutboundFrame { +fn client_err_to_frame(id: RequestId, err: ClientError) -> OutboundFrame { let (code, message) = match &err { ClientError::TaskNotFound => (a2a_nats::error::TASK_NOT_FOUND, err.to_string()), ClientError::TaskNotCancelable => (a2a_nats::error::TASK_NOT_CANCELABLE, err.to_string()), @@ -33,38 +34,29 @@ fn client_err_to_frame(id: RpcId, err: ClientError) -> OutboundFrame { ClientError::JsonRpc { code, message } => (*code, message.clone()), _ => (-32603, err.to_string()), }; - OutboundFrame::Error(OutboundError::new(id, code, message)) + OutboundFrame::error(ResponseId::from(id), code, message) } fn parse_params(params: Value) -> Result> { - serde_json::from_value(params).map_err(|e| { - Box::new(OutboundFrame::Error(OutboundError::new( - RpcId::Null, - INVALID_PARAMS, - e.to_string(), - ))) - }) + serde_json::from_value(params) + .map_err(|e| Box::new(OutboundFrame::error(ResponseId::Null, INVALID_PARAMS, e.to_string()))) } -fn forward_validated(id: &RpcId, validated: ValidatedRpc) -> OutboundFrame { - match validated.body_with_client_id(&id.to_json_value()) { +fn forward_validated(id: &RequestId, validated: ValidatedRpc) -> OutboundFrame { + match validated.body_with_client_id(&id.to_json()) { Ok(body) => OutboundFrame::RawBody(body), - Err(e) => OutboundFrame::Error(OutboundError::new(id.clone(), -32603, e.to_string())), + Err(e) => OutboundFrame::error(ResponseId::from(id.clone()), -32603, e.to_string()), } } -/// `method` is the JSON-RPC method this notification is associated with — -/// `message/stream` for the streaming send path, `tasks/resubscribe` for the -/// resubscribe path. Clients route notifications by method, so emitting the -/// wrong one makes the resubscribe stream invisible to compliant callers. -fn stream_event_to_frame(id: &RpcId, method: &'static str, event: &StreamResponse) -> OutboundFrame { - let params = serde_json::to_value(event).unwrap_or(Value::Null); - OutboundFrame::Notification(OutboundNotification::new(id.clone(), method, params)) +fn stream_event_to_frame(id: &RequestId, event: &StreamResponse) -> OutboundFrame { + let result = serde_json::to_value(event).unwrap_or(Value::Null); + OutboundFrame::success(ResponseId::from(id.clone()), result) } pub async fn dispatch_request( client: &A2aClient, - id: RpcId, + id: RequestId, method: &str, params: Value, tx: &mpsc::Sender, @@ -117,7 +109,7 @@ pub async fn dispatch_request( while let Some(item) = stream.next().await { match item { Ok(event) => { - let frame = stream_event_to_frame(&id, "message/stream", &event); + let frame = stream_event_to_frame(&id, &event); if tx.send(frame).await.is_err() { return; } @@ -197,11 +189,11 @@ pub async fn dispatch_request( Ok(t) => t, Err(e) => { let _ = tx - .send(OutboundFrame::Error(OutboundError::new( - id, + .send(OutboundFrame::error( + ResponseId::from(id), INVALID_PARAMS, e.to_string(), - ))) + )) .await; return; } @@ -220,7 +212,7 @@ pub async fn dispatch_request( while let Some(item) = stream.next().await { match item { Ok(event) => { - let frame = stream_event_to_frame(&id, "tasks/resubscribe", &event); + let frame = stream_event_to_frame(&id, &event); if tx.send(frame).await.is_err() { return; } @@ -297,24 +289,18 @@ pub async fn dispatch_request( Err(e) => client_err_to_frame(id, e), }, - unknown => OutboundFrame::Error(OutboundError::new( - id, + unknown => OutboundFrame::error( + ResponseId::from(id), METHOD_NOT_FOUND, format!("method not found: {unknown}"), - )), + ), }; let _ = tx.send(frame).await; } -fn make_with_id(frame: OutboundFrame, id: &RpcId) -> OutboundFrame { - match frame { - OutboundFrame::Error(mut err) => { - err.id = id.clone(); - OutboundFrame::Error(err) - } - other => other, - } +fn make_with_id(frame: OutboundFrame, id: &RequestId) -> OutboundFrame { + frame.with_error_id(ResponseId::from(id.clone())) } #[cfg(test)] diff --git a/rsworkspace/crates/a2a/a2a-nats-stdio/src/dispatch/tests.rs b/rsworkspace/crates/a2a/a2a-nats-stdio/src/dispatch/tests.rs index a7bf3997ad..130897dce0 100644 --- a/rsworkspace/crates/a2a/a2a-nats-stdio/src/dispatch/tests.rs +++ b/rsworkspace/crates/a2a/a2a-nats-stdio/src/dispatch/tests.rs @@ -2,13 +2,11 @@ use super::*; use a2a_nats::client::A2aClient; use a2a_nats::{A2aAgentId, A2aPrefix}; use bytes::Bytes; -use jsonrpc_nats::{Message as JrpcMessage, ResponseId, encode}; +use jsonrpc_nats::{Message as JrpcMessage, RequestId, ResponseId, encode}; use serde_json::json; use trogon_nats::AdvancedMockNatsClient; use trogon_nats::jetstream::mocks::{MockJetStreamConsumer, MockJetStreamConsumerFactory}; -use crate::wire::RpcError; - fn make_client( nats: AdvancedMockNatsClient, js: MockJetStreamConsumerFactory, @@ -63,7 +61,7 @@ fn send_message_response(task_id: &str) -> (async_nats::HeaderMap, Bytes) { async fn dispatch( client: &A2aClient, - id: RpcId, + id: RequestId, method: &str, params: Value, ) -> OutboundFrame { @@ -81,7 +79,7 @@ async fn tasks_get_success() { let client = make_client(nats, MockJetStreamConsumerFactory::new()); let frame = dispatch( &client, - RpcId::Number(1), + RequestId::Number(1), "tasks/get", json!({"id": "t1", "tenant": ""}), ) @@ -96,12 +94,12 @@ async fn tasks_get_error_maps_to_rpc_error() { let client = make_client(nats, MockJetStreamConsumerFactory::new()); let frame = dispatch( &client, - RpcId::Number(2), + RequestId::Number(2), "tasks/get", json!({"id": "t1", "tenant": ""}), ) .await; - assert!(matches!(frame, OutboundFrame::Error(_))); + assert!(frame.error_code().is_some()); } #[tokio::test] @@ -112,7 +110,7 @@ async fn tasks_cancel_success() { let client = make_client(nats, MockJetStreamConsumerFactory::new()); let frame = dispatch( &client, - RpcId::String("x".into()), + RequestId::String("x".into()), "tasks/cancel", json!({"id": "tc", "tenant": ""}), ) @@ -128,7 +126,7 @@ async fn message_send_success() { let client = make_client(nats, MockJetStreamConsumerFactory::new()); let frame = dispatch( &client, - RpcId::Number(3), + RequestId::Number(3), "message/send", json!({"message": {"messageId": "m1", "role": "ROLE_USER", "parts": []}}), ) @@ -163,7 +161,7 @@ async fn message_stream_returns_when_bootstrap_send_fails() { std::time::Duration::from_secs(2), dispatch_request( &client, - RpcId::Number(4), + RequestId::Number(4), "message/stream", json!({"message": {"messageId": "m-drop", "role": "ROLE_USER", "parts": []}}), &chan_tx, @@ -191,7 +189,7 @@ async fn tasks_resubscribe_returns_when_bootstrap_send_fails() { std::time::Duration::from_secs(2), dispatch_request( &client, - RpcId::Number(5), + RequestId::Number(5), "tasks/resubscribe", json!({"id": "rsub-drop", "lastSeq": 0}), &chan_tx, @@ -216,7 +214,7 @@ async fn message_stream_emits_bootstrap_then_events() { let (chan_tx, mut chan_rx) = mpsc::channel(16); dispatch_request( &client, - RpcId::Number(4), + RequestId::Number(4), "message/stream", json!({"message": {"messageId": "m2", "role": "ROLE_USER", "parts": []}}), &chan_tx, @@ -243,7 +241,7 @@ async fn tasks_resubscribe_emits_snapshot_then_empty_stream() { let (chan_tx, mut chan_rx) = mpsc::channel(8); dispatch_request( &client, - RpcId::Number(5), + RequestId::Number(5), "tasks/resubscribe", json!({"id": "task1", "lastSeq": 0}), &chan_tx, @@ -284,7 +282,7 @@ async fn agent_card_success() { let client = make_client(nats, MockJetStreamConsumerFactory::new()); let frame = dispatch( &client, - RpcId::Number(6), + RequestId::Number(6), "agent/getAuthenticatedExtendedCard", json!({}), ) @@ -296,28 +294,16 @@ async fn agent_card_success() { async fn unknown_method_returns_method_not_found() { let nats = AdvancedMockNatsClient::new(); let client = make_client(nats, MockJetStreamConsumerFactory::new()); - let frame = dispatch(&client, RpcId::Number(7), "bogus/method", json!({})).await; - assert!(matches!( - frame, - OutboundFrame::Error(OutboundError { - error: RpcError { code: -32601, .. }, - .. - }) - )); + let frame = dispatch(&client, RequestId::Number(7), "bogus/method", json!({})).await; + assert_eq!(frame.error_code(), Some(-32601)); } #[tokio::test] async fn invalid_params_returns_error() { let nats = AdvancedMockNatsClient::new(); let client = make_client(nats, MockJetStreamConsumerFactory::new()); - let frame = dispatch(&client, RpcId::Number(8), "tasks/get", json!("not an object")).await; - assert!(matches!( - frame, - OutboundFrame::Error(OutboundError { - error: RpcError { code: -32602, .. }, - .. - }) - )); + let frame = dispatch(&client, RequestId::Number(8), "tasks/get", json!("not an object")).await; + assert_eq!(frame.error_code(), Some(-32602)); } fn err_response(code: i32, msg: &str) -> (async_nats::HeaderMap, Bytes) { @@ -333,11 +319,7 @@ fn err_response(code: i32, msg: &str) -> (async_nats::HeaderMap, Bytes) { #[track_caller] fn assert_err_code(frame: OutboundFrame, expected: i32) { - let OutboundFrame::Error(OutboundError { - error: RpcError { code, .. }, - .. - }) = frame - else { + let Some(code) = frame.error_code() else { panic!("expected error frame, got non-error variant"); }; assert_eq!(code, expected); @@ -359,7 +341,7 @@ async fn tasks_list_success() { .unwrap(); nats.set_response_wire("a2a.v1.agents.bot.tasks.list", encoded.headers, encoded.body); let client = make_client(nats, MockJetStreamConsumerFactory::new()); - let frame = dispatch(&client, RpcId::Number(1), "tasks/list", json!({})).await; + let frame = dispatch(&client, RequestId::Number(1), "tasks/list", json!({})).await; assert!(matches!(frame, OutboundFrame::RawBody(_))); } @@ -383,7 +365,7 @@ async fn push_set_success() { let client = make_client(nats, MockJetStreamConsumerFactory::new()); let frame = dispatch( &client, - RpcId::Number(2), + RequestId::Number(2), "tasks/pushNotificationConfig/set", json!({"url":"https://example.com","id":"c","taskId":"t1"}), ) @@ -411,7 +393,7 @@ async fn push_get_success() { let client = make_client(nats, MockJetStreamConsumerFactory::new()); let frame = dispatch( &client, - RpcId::Number(3), + RequestId::Number(3), "tasks/pushNotificationConfig/get", json!({"taskId":"t1","id":"c"}), ) @@ -435,7 +417,7 @@ async fn push_list_success() { let client = make_client(nats, MockJetStreamConsumerFactory::new()); let frame = dispatch( &client, - RpcId::Number(4), + RequestId::Number(4), "tasks/pushNotificationConfig/list", json!({"taskId":"t1"}), ) @@ -455,7 +437,7 @@ async fn push_delete_success() { let client = make_client(nats, MockJetStreamConsumerFactory::new()); let frame = dispatch( &client, - RpcId::Number(5), + RequestId::Number(5), "tasks/pushNotificationConfig/delete", json!({"taskId":"t1","id":"c"}), ) @@ -511,7 +493,7 @@ async fn client_err_to_frame_maps_every_typed_variant() { let client = make_client(nats, MockJetStreamConsumerFactory::new()); let frame = dispatch( &client, - RpcId::Number(input as i64), + RequestId::Number(input as i64), "tasks/get", json!({"id":"t","tenant":""}), ) @@ -525,14 +507,20 @@ async fn transport_error_falls_back_to_internal_code() { let nats = AdvancedMockNatsClient::new(); nats.fail_next_request(); let client = make_client(nats, MockJetStreamConsumerFactory::new()); - let frame = dispatch(&client, RpcId::Number(99), "tasks/get", json!({"id":"t","tenant":""})).await; + let frame = dispatch( + &client, + RequestId::Number(99), + "tasks/get", + json!({"id":"t","tenant":""}), + ) + .await; assert_err_code(frame, -32603); } #[tokio::test] async fn agent_error_routes_to_outbound_error_for_every_typed_method() { // For each method, configure the agent to reply with a typed JSON-RPC - // error and confirm the dispatcher forwards it as an OutboundError + // error and confirm the dispatcher forwards it as a JSON-RPC error frame // through that method's Err arm. let cases = [ ( @@ -577,7 +565,7 @@ async fn agent_error_routes_to_outbound_error_for_every_typed_method() { let (headers, body) = err_response(a2a_nats::error::TASK_NOT_FOUND, "missing"); nats.set_response_wire(subject, headers, body); let client = make_client(nats, MockJetStreamConsumerFactory::new()); - let frame = dispatch(&client, RpcId::Number(1), method, params).await; + let frame = dispatch(&client, RequestId::Number(1), method, params).await; assert_err_code(frame, a2a_nats::error::TASK_NOT_FOUND); } } @@ -593,7 +581,7 @@ async fn message_stream_error_at_bootstrap_routes_to_outbound_error() { let client = make_client(nats, js); let frame = dispatch( &client, - RpcId::Number(1), + RequestId::Number(1), "message/stream", json!({"message": {"messageId": "m", "role": "ROLE_USER", "parts": []}}), ) @@ -612,7 +600,7 @@ async fn tasks_resubscribe_error_at_snapshot_routes_to_outbound_error() { let client = make_client(nats, js); let frame = dispatch( &client, - RpcId::Number(1), + RequestId::Number(1), "tasks/resubscribe", json!({"id": "missing", "lastSeq": 0}), ) @@ -635,7 +623,7 @@ async fn invalid_params_returned_for_every_typed_method() { "tasks/pushNotificationConfig/list", "tasks/pushNotificationConfig/delete", ] { - let frame = dispatch(&client, RpcId::Number(1), method, json!("not an object")).await; + let frame = dispatch(&client, RequestId::Number(1), method, json!("not an object")).await; assert_err_code(frame, -32602); } } @@ -677,6 +665,17 @@ async fn minted_req_id(nats: &AdvancedMockNatsClient) -> String { panic!("the client never sent a request carrying a correlation id") } +/// A task event as it lands on the wire: a JSON-RPC success response repeating +/// the request id, with the event as its result. +fn event_body(req_id: &str, event: &a2a::event::StreamResponse) -> Vec { + serde_json::to_vec(&json!({ + "jsonrpc": "2.0", + "id": req_id, + "result": event, + })) + .unwrap() +} + fn status_event(task_id: &str) -> a2a::event::StreamResponse { a2a::event::StreamResponse::StatusUpdate(a2a::event::TaskStatusUpdateEvent { task_id: task_id.to_string(), @@ -691,7 +690,7 @@ fn status_event(task_id: &str) -> a2a::event::StreamResponse { } #[tokio::test] -async fn message_stream_forwards_status_events_as_notifications() { +async fn message_stream_forwards_status_events_as_responses() { let nats = AdvancedMockNatsClient::new(); let (headers, body) = send_message_response("ms3"); nats.set_response_wire("a2a.v1.agents.bot.message.stream", headers, body); @@ -701,14 +700,14 @@ async fn message_stream_forwards_status_events_as_notifications() { let wire = nats.clone(); tokio::spawn(async move { let req_id = minted_req_id(&wire).await; - let event_payload = serde_json::to_vec(&status_event("task-stream")).unwrap(); + let event_payload = event_body(&req_id, &status_event("task-stream")); evt_tx.unbounded_send(Ok(js_msg(event_payload, &req_id))).unwrap(); }); let client = make_client(nats, js); let (chan_tx, mut chan_rx) = mpsc::channel(16); dispatch_request( &client, - RpcId::Number(10), + RequestId::Number(10), "message/stream", json!({"message": {"messageId": "m3", "role": "ROLE_USER", "parts": []}}), &chan_tx, @@ -717,22 +716,22 @@ async fn message_stream_forwards_status_events_as_notifications() { drop(chan_tx); let first = chan_rx.recv().await.expect("bootstrap"); assert!(matches!(first, OutboundFrame::RawBody(_))); - let second = chan_rx.recv().await.expect("event notification"); - match second { - OutboundFrame::Notification(n) => assert_eq!(n.method, "message/stream"), - other => panic!("expected notification, got {other:?}"), - } + let second = chan_rx.recv().await.expect("event response"); + // The caller's own id, not the transport's correlation id. + let v = serde_json::to_value(&second).unwrap(); + assert_eq!(v["id"], 10); + assert!(v.get("result").is_some(), "a stream chunk is a success response"); } #[tokio::test] -async fn tasks_resubscribe_forwards_status_events_under_resubscribe_method() { +async fn tasks_resubscribe_forwards_status_events_as_responses() { let nats = AdvancedMockNatsClient::new(); let (headers, body) = task_response("rsub"); nats.set_response_wire("a2a.v1.agents.bot.tasks.resubscribe", headers, body); let js = MockJetStreamConsumerFactory::new(); let (consumer, evt_tx) = MockJetStreamConsumer::new(); js.add_consumer(consumer); - let event_payload = serde_json::to_vec(&status_event("rsub")).unwrap(); + let event_payload = event_body("req-of-the-original-subscription", &status_event("rsub")); // A replay carries the correlation id of the subscription that first asked for // these events, and a resume is entitled to them all the same. evt_tx @@ -743,7 +742,7 @@ async fn tasks_resubscribe_forwards_status_events_under_resubscribe_method() { let (chan_tx, mut chan_rx) = mpsc::channel(16); dispatch_request( &client, - RpcId::Number(11), + RequestId::Number(11), "tasks/resubscribe", json!({"id": "rsub", "lastSeq": 0}), &chan_tx, @@ -752,12 +751,10 @@ async fn tasks_resubscribe_forwards_status_events_under_resubscribe_method() { drop(chan_tx); let first = chan_rx.recv().await.expect("snapshot"); assert!(matches!(first, OutboundFrame::RawBody(_))); - let second = chan_rx.recv().await.expect("event notification"); - match second { - // Notification method MUST be tasks/resubscribe, not message/stream. - OutboundFrame::Notification(n) => assert_eq!(n.method, "tasks/resubscribe"), - other => panic!("expected notification, got {other:?}"), - } + let second = chan_rx.recv().await.expect("event response"); + let v = serde_json::to_value(&second).unwrap(); + assert_eq!(v["id"], 11); + assert!(v.get("result").is_some(), "a stream chunk is a success response"); } #[tokio::test] @@ -766,7 +763,7 @@ async fn tasks_resubscribe_rejects_blank_id() { let client = make_client(nats, MockJetStreamConsumerFactory::new()); let frame = dispatch( &client, - RpcId::Number(11), + RequestId::Number(11), "tasks/resubscribe", json!({"id": "", "lastSeq": 0}), ) @@ -776,22 +773,15 @@ async fn tasks_resubscribe_rejects_blank_id() { #[test] fn make_with_id_overwrites_error_id_and_passes_through_non_error_frames() { - let from_parse_helper = OutboundFrame::Error(OutboundError::new(RpcId::Null, INVALID_PARAMS, "x".into())); - let target_id = RpcId::Number(42); + let from_parse_helper = OutboundFrame::error(ResponseId::Null, INVALID_PARAMS, "x"); + let target_id = RequestId::Number(42); let rewritten = super::make_with_id(from_parse_helper, &target_id); - if let OutboundFrame::Error(e) = rewritten { - assert_eq!(e.id, RpcId::Number(42)); - } - // Non-Error variants pass through unchanged — there's nothing to rewrite. - let notif = OutboundFrame::Notification(OutboundNotification::new( - RpcId::Number(1), - "message/stream", - Value::Null, - )); - let passed = super::make_with_id(notif, &target_id); - if let OutboundFrame::Notification(n) = passed { - assert_eq!(n.id, RpcId::Number(1)); - } + assert_eq!(serde_json::to_value(&rewritten).unwrap()["id"], 42); + + // Non-error frames pass through unchanged, there is nothing to rewrite. + let event = OutboundFrame::success(ResponseId::Number(1), Value::Null); + let passed = super::make_with_id(event, &target_id); + assert_eq!(serde_json::to_value(&passed).unwrap()["id"], 1); } #[test] @@ -800,12 +790,7 @@ fn a_body_that_cannot_be_rewritten_becomes_an_internal_error_frame() { // body that will not parse has to surface as a JSON-RPC error rather than // reach the client as a malformed envelope. let validated = ValidatedRpc::new((), Bytes::from_static(b"not json")); - let frame = forward_validated(&RpcId::Number(1), validated); - match frame { - OutboundFrame::Error(err) => { - assert_eq!(err.id, RpcId::Number(1)); - assert_eq!(err.error.code, -32603); - } - other => panic!("expected an error frame, got {other:?}"), - } + let frame = forward_validated(&RequestId::Number(1), validated); + assert_eq!(frame.error_code(), Some(-32603)); + assert_eq!(serde_json::to_value(&frame).unwrap()["id"], 1); } diff --git a/rsworkspace/crates/a2a/a2a-nats-stdio/src/io_loop.rs b/rsworkspace/crates/a2a/a2a-nats-stdio/src/io_loop.rs index e7db7d880b..edae650817 100644 --- a/rsworkspace/crates/a2a/a2a-nats-stdio/src/io_loop.rs +++ b/rsworkspace/crates/a2a/a2a-nats-stdio/src/io_loop.rs @@ -6,6 +6,7 @@ use std::sync::Arc; use a2a_nats::client::A2aClient; +use jsonrpc_nats::{CodecError, Message, RequestId, ResponseId, from_json_value}; use tokio::io::{AsyncBufReadExt, AsyncWriteExt, BufReader}; use tokio::sync::{Semaphore, mpsc}; use tokio::task::JoinSet; @@ -15,7 +16,7 @@ use trogon_nats::jetstream::{JetStreamCreateConsumer, JetStreamGetStream, JsAck, use crate::constants::{CHANNEL_CAP, MAX_INFLIGHT_DISPATCH}; use crate::dispatch::dispatch_request; -use crate::wire::{InboundRequest, OutboundError, OutboundFrame, RpcId}; +use crate::wire::OutboundFrame; /// Returns `Err` when the stdout writer task failed (broken pipe, write/flush /// error). Callers should propagate so the process exits non-zero — a stdio @@ -153,7 +154,7 @@ where shutdown_requested = true; break 'outer; } - _ = frame_tx.send(OutboundFrame::Error(err)) => {} + _ = frame_tx.send(*err) => {} } continue; } @@ -277,34 +278,38 @@ fn writer_task_err(res: Result, tokio::task::JoinError>) -> /// Split JSON-syntax failures (`-32700` Parse error) from envelope-shape /// failures (`-32600` Invalid Request). JSON-RPC reserves `-32700` for actual /// invalid JSON; structurally invalid requests are a different class. -fn parse_inbound(raw: &str) -> Result<(RpcId, String, serde_json::Value), OutboundError> { +fn parse_inbound(raw: &str) -> Result<(RequestId, String, serde_json::Value), Box> { let value: serde_json::Value = serde_json::from_str(raw).map_err(|e| { warn!(error = %e, "stdin line is not valid JSON"); - OutboundError::new(RpcId::Null, -32700, format!("parse error: {e}")) + Box::new(OutboundFrame::error( + ResponseId::Null, + -32700, + format!("parse error: {e}"), + )) })?; // Salvage the request id from the raw JSON before the envelope check so a // malformed-shape `-32600` reply still correlates with the originating // call. JSON-RPC requires echoing the id when it can be determined. let salvaged_id = value .get("id") - .and_then(|v| serde_json::from_value::(v.clone()).ok()) - .unwrap_or(RpcId::Null); - // InboundRequest deserializes only `id`/`method`/`params`, so a missing or - // wrong `jsonrpc` would otherwise dispatch as a real call. JSON-RPC 2.0 - // requires the version field exactly equal to "2.0". - if !matches!(value.get("jsonrpc"), Some(serde_json::Value::String(v)) if v == "2.0") { - warn!("JSON-RPC envelope has missing or wrong version"); - return Err(OutboundError::new( - salvaged_id, - -32600, + .and_then(|v| serde_json::from_value::(v.clone()).ok()) + .unwrap_or(ResponseId::Null); + let invalid_request = |message: String| { + warn!(message, "JSON-RPC envelope is invalid"); + Box::new(OutboundFrame::error(salvaged_id.clone(), -32600, message)) + }; + match from_json_value(&value) { + Ok(Message::Request { id, method, params }) => Ok((id, method, params)), + // A notification carries no id, so its reply could never be correlated; + // the stdio bridge answers requests only. + Ok(_) => Err(invalid_request( + "invalid request: expected a JSON-RPC request".to_string(), + )), + Err(CodecError::UnsupportedVersion { .. }) => Err(invalid_request( "invalid request: missing or unsupported jsonrpc version".to_string(), - )); + )), + Err(e) => Err(invalid_request(format!("invalid request: {e}"))), } - let req: InboundRequest = serde_json::from_value(value).map_err(|e| { - warn!(error = %e, "JSON-RPC envelope is invalid"); - OutboundError::new(salvaged_id, -32600, format!("invalid request: {e}")) - })?; - Ok((req.id, req.method, req.params)) } // Loop-exercising tests are gated to `cfg(not(coverage))` — see the io_loop diff --git a/rsworkspace/crates/a2a/a2a-nats-stdio/src/io_loop/parse_tests.rs b/rsworkspace/crates/a2a/a2a-nats-stdio/src/io_loop/parse_tests.rs index 3391b15f28..f21d2d2aa9 100644 --- a/rsworkspace/crates/a2a/a2a-nats-stdio/src/io_loop/parse_tests.rs +++ b/rsworkspace/crates/a2a/a2a-nats-stdio/src/io_loop/parse_tests.rs @@ -1,34 +1,40 @@ use super::*; +use serde_json::{Value, json}; + +/// The id the bridge echoes back on a rejected line, as it lands on stdout. +#[track_caller] +fn err_id(raw: &str) -> Value { + serde_json::to_value(*parse_inbound(raw).unwrap_err()).unwrap()["id"].clone() +} #[test] fn parse_inbound_routes_syntax_to_parse_error_and_shape_to_invalid_request() { - assert_eq!(parse_inbound("not json").unwrap_err().error.code, -32700); - assert_eq!(parse_inbound(r#"{"id":1}"#).unwrap_err().error.code, -32600); + assert_eq!(parse_inbound("not json").unwrap_err().error_code().unwrap(), -32700); + assert_eq!(parse_inbound(r#"{"id":1}"#).unwrap_err().error_code().unwrap(), -32600); let (id, method, _) = parse_inbound(r#"{"jsonrpc":"2.0","id":7,"method":"tasks/get","params":{}}"#).unwrap(); - assert_eq!(id, RpcId::Number(7)); + assert_eq!(id, RequestId::Number(7)); assert_eq!(method, "tasks/get"); } #[test] fn parse_inbound_preserves_id_on_envelope_failure() { - let err = parse_inbound(r#"{"jsonrpc":"2.0","id":42}"#).unwrap_err(); - assert_eq!(err.id, RpcId::Number(42)); - let err = parse_inbound(r#"{"jsonrpc":"2.0","id":"corr-7"}"#).unwrap_err(); - assert_eq!(err.id, RpcId::String("corr-7".into())); - let err = parse_inbound(r#"{"jsonrpc":"2.0"}"#).unwrap_err(); - assert_eq!(err.id, RpcId::Null); - let err = parse_inbound(r#"{"jsonrpc":"2.0","id":[1,2,3]}"#).unwrap_err(); - assert_eq!(err.id, RpcId::Null); + assert_eq!(err_id(r#"{"jsonrpc":"2.0","id":42}"#), json!(42)); + assert_eq!(err_id(r#"{"jsonrpc":"2.0","id":"corr-7"}"#), json!("corr-7")); + assert_eq!(err_id(r#"{"jsonrpc":"2.0"}"#), Value::Null); + assert_eq!(err_id(r#"{"jsonrpc":"2.0","id":[1,2,3]}"#), Value::Null); } #[test] fn parse_inbound_rejects_missing_or_wrong_jsonrpc_version() { let err = parse_inbound(r#"{"id":1,"method":"tasks/get","params":{}}"#).unwrap_err(); - assert_eq!(err.error.code, -32600); - assert_eq!(err.id, RpcId::Number(1)); + assert_eq!(err.error_code().unwrap(), -32600); + assert_eq!(err_id(r#"{"id":1,"method":"tasks/get","params":{}}"#), json!(1)); let err = parse_inbound(r#"{"jsonrpc":"1.0","id":2,"method":"tasks/get","params":{}}"#).unwrap_err(); - assert_eq!(err.error.code, -32600); - assert_eq!(err.id, RpcId::Number(2)); + assert_eq!(err.error_code().unwrap(), -32600); + assert_eq!( + err_id(r#"{"jsonrpc":"1.0","id":2,"method":"tasks/get","params":{}}"#), + json!(2) + ); let err = parse_inbound(r#"{"jsonrpc":2.0,"id":3}"#).unwrap_err(); - assert_eq!(err.error.code, -32600); + assert_eq!(err.error_code().unwrap(), -32600); } diff --git a/rsworkspace/crates/a2a/a2a-nats-stdio/src/io_loop/tests.rs b/rsworkspace/crates/a2a/a2a-nats-stdio/src/io_loop/tests.rs index f91caf5f61..3663bea135 100644 --- a/rsworkspace/crates/a2a/a2a-nats-stdio/src/io_loop/tests.rs +++ b/rsworkspace/crates/a2a/a2a-nats-stdio/src/io_loop/tests.rs @@ -148,7 +148,7 @@ async fn io_loop_handles_invalid_request_envelope() { let (stdin_reader, mut stdin_writer) = tokio::io::duplex(4096); let (mut stdout_reader, stdout_writer) = tokio::io::duplex(4096); - // Valid JSON but missing the required `method` field on InboundRequest. + // Valid JSON but missing the `method` field a JSON-RPC request requires. stdin_writer.write_all(b"{\"id\":1}\n").await.unwrap(); drop(stdin_writer); diff --git a/rsworkspace/crates/a2a/a2a-nats-stdio/src/wire.rs b/rsworkspace/crates/a2a/a2a-nats-stdio/src/wire.rs index 3083b387fd..d8067389fd 100644 --- a/rsworkspace/crates/a2a/a2a-nats-stdio/src/wire.rs +++ b/rsworkspace/crates/a2a/a2a-nats-stdio/src/wire.rs @@ -1,94 +1,70 @@ use bytes::Bytes; -use serde::{Deserialize, Serialize}; +use jsonrpc_nats::{Message, ResponseId, to_json_value}; +use serde::Serialize; use serde_json::Value; -#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] -#[serde(untagged)] -pub enum RpcId { - Number(i64), - String(String), - Null, +/// Stdio outbound frame. +/// +/// Success responses prefer the validated NATS body (id already rewritten to the +/// edge client id) so the bytes the agent produced reach stdout unaltered. +/// Everything the bridge builds locally goes out as a canonical JSON-RPC message. +#[derive(Debug)] +pub enum OutboundFrame { + /// Canonical JSON-RPC body bytes after typed validate + id rewrite. + RawBody(Bytes), + Message(Message), } -impl RpcId { - pub fn to_json_value(&self) -> Value { - match self { - Self::Number(n) => Value::Number((*n).into()), - Self::String(s) => Value::String(s.clone()), - Self::Null => Value::Null, - } +impl OutboundFrame { + /// A2A streams every chunk as a JSON-RPC success response repeating the + /// request id, so a stream event and the terminal response share one shape. + pub fn success(id: ResponseId, result: Value) -> Self { + Self::Message(Message::Success { id, result }) } -} - -#[derive(Debug, Deserialize)] -pub struct InboundRequest { - pub id: RpcId, - pub method: String, - #[serde(default)] - pub params: Value, -} - -#[derive(Debug, Serialize)] -pub struct OutboundNotification { - pub jsonrpc: &'static str, - pub id: RpcId, - pub method: &'static str, - pub params: Value, -} -impl OutboundNotification { - pub fn new(id: RpcId, method: &'static str, params: Value) -> Self { - Self { - jsonrpc: "2.0", + pub fn error(id: ResponseId, code: i32, message: impl Into) -> Self { + Self::Message(Message::Error { id, - method, - params, - } + code, + message: message.into(), + data: None, + }) } -} - -#[derive(Debug, Serialize)] -pub struct RpcError { - pub code: i32, - pub message: String, -} -#[derive(Debug, Serialize)] -pub struct OutboundError { - pub jsonrpc: &'static str, - pub id: RpcId, - pub error: RpcError, -} - -impl OutboundError { - pub fn new(id: RpcId, code: i32, message: String) -> Self { - Self { - jsonrpc: "2.0", - id, - error: RpcError { code, message }, + /// Stamp the caller's id onto a locally-built error whose id was not known + /// at construction time. + pub fn with_error_id(self, id: ResponseId) -> Self { + match self { + Self::Message(Message::Error { + code, message, data, .. + }) => Self::Message(Message::Error { + id, + code, + message, + data, + }), + other => other, } } -} -/// Stdio outbound frame. Success responses prefer a validated NATS body -/// (id rewritten to the edge client id); local errors still use a typed -/// envelope built via serde. -#[derive(Debug, Serialize)] -#[serde(untagged)] -pub enum OutboundFrame { - /// Canonical JSON-RPC body bytes after typed validate + id rewrite. - #[serde(serialize_with = "serialize_raw_body")] - RawBody(Bytes), - Notification(OutboundNotification), - Error(OutboundError), + pub fn error_code(&self) -> Option { + match self { + Self::Message(Message::Error { code, .. }) => Some(*code), + _ => None, + } + } } -fn serialize_raw_body(body: &Bytes, serializer: S) -> Result -where - S: serde::Serializer, -{ - let value: Value = serde_json::from_slice(body).map_err(serde::ser::Error::custom)?; - value.serialize(serializer) +impl Serialize for OutboundFrame { + fn serialize(&self, serializer: S) -> Result { + match self { + Self::RawBody(body) => { + let value: Value = serde_json::from_slice(body).map_err(serde::ser::Error::custom)?; + value.serialize(serializer) + } + Self::Message(message) => to_json_value(message).serialize(serializer), + } + } } #[cfg(test)] diff --git a/rsworkspace/crates/a2a/a2a-nats-stdio/src/wire/tests.rs b/rsworkspace/crates/a2a/a2a-nats-stdio/src/wire/tests.rs index ea83d29e97..a03da03e7c 100644 --- a/rsworkspace/crates/a2a/a2a-nats-stdio/src/wire/tests.rs +++ b/rsworkspace/crates/a2a/a2a-nats-stdio/src/wire/tests.rs @@ -2,21 +2,6 @@ use super::*; use bytes::Bytes; use serde_json::json; -#[test] -fn inbound_request_deserializes_numeric_id() { - let raw = r#"{"jsonrpc":"2.0","id":42,"method":"tasks/get","params":{"id":"t1","tenant":""}}"#; - let req: InboundRequest = serde_json::from_str(raw).unwrap(); - assert_eq!(req.id, RpcId::Number(42)); - assert_eq!(req.method, "tasks/get"); -} - -#[test] -fn inbound_request_deserializes_string_id() { - let raw = r#"{"jsonrpc":"2.0","id":"abc","method":"agent/getAuthenticatedExtendedCard","params":{}}"#; - let req: InboundRequest = serde_json::from_str(raw).unwrap(); - assert_eq!(req.id, RpcId::String("abc".into())); -} - #[test] fn outbound_raw_body_rewrites_via_serde() { let body = @@ -30,30 +15,38 @@ fn outbound_raw_body_rewrites_via_serde() { #[test] fn outbound_error_serializes() { - let err = OutboundError::new(RpcId::Number(2), -32001, "not found".into()); - let v = serde_json::to_value(&err).unwrap(); + let frame = OutboundFrame::error(ResponseId::Number(2), -32001, "not found"); + let v = serde_json::to_value(&frame).unwrap(); + assert_eq!(v["jsonrpc"], "2.0"); + assert_eq!(v["id"], 2); assert_eq!(v["error"]["code"], -32001); assert_eq!(v["error"]["message"], "not found"); } #[test] -fn outbound_notification_serializes() { - let notif = OutboundNotification::new(RpcId::Number(3), "message/stream", json!({"event": "x"})); - let v = serde_json::to_value(¬if).unwrap(); - assert_eq!(v["method"], "message/stream"); +fn outbound_success_serializes() { + let frame = OutboundFrame::success(ResponseId::Number(3), json!({"event": "x"})); + let v = serde_json::to_value(&frame).unwrap(); + assert_eq!(v["jsonrpc"], "2.0"); assert_eq!(v["id"], 3); + assert_eq!(v["result"]["event"], "x"); + assert!(v.get("method").is_none(), "a response carries no method"); } #[test] -fn outbound_frame_error_variant_serializes() { - let frame = OutboundFrame::Error(OutboundError::new(RpcId::Null, -32600, "invalid".into())); +fn null_id_still_serializes_as_a_response() { + let frame = OutboundFrame::error(ResponseId::Null, -32600, "invalid"); let v = serde_json::to_value(&frame).unwrap(); + assert_eq!(v["id"], Value::Null); assert_eq!(v["error"]["code"], -32600); } #[test] -fn rpc_id_projects_every_json_rpc_id_shape() { - assert_eq!(RpcId::Number(7).to_json_value(), json!(7)); - assert_eq!(RpcId::String("abc".into()).to_json_value(), json!("abc")); - assert_eq!(RpcId::Null.to_json_value(), Value::Null); +fn with_error_id_stamps_errors_and_leaves_other_frames_alone() { + let stamped = OutboundFrame::error(ResponseId::Null, -32602, "bad params").with_error_id(ResponseId::Number(9)); + assert_eq!(serde_json::to_value(&stamped).unwrap()["id"], 9); + + let raw = Bytes::from(serde_json::to_vec(&json!({"jsonrpc":"2.0","id":"keep","result":{}})).unwrap()); + let untouched = OutboundFrame::RawBody(raw).with_error_id(ResponseId::Number(9)); + assert_eq!(serde_json::to_value(&untouched).unwrap()["id"], "keep"); } diff --git a/rsworkspace/crates/a2a/a2a-nats/src/audit/emitter.rs b/rsworkspace/crates/a2a/a2a-nats/src/audit/emitter.rs index 313edf1596..052817152c 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/audit/emitter.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/audit/emitter.rs @@ -11,6 +11,10 @@ use crate::audit::task_lifecycle::TaskLifecycleEnvelope; type BoxFuture<'a, T> = Pin + Send + 'a>>; pub trait AuditEmitter: Send + Sync { + /// Published on `{prefix}.v1.audit.{agent_id}.{ok|err}` per ADR#0055's + /// operational profile: entity-scoped, fixed terminals. The JSON-RPC method + /// travels in the payload rather than as a subject token so the subject + /// vocabulary does not grow with the method set. fn publish<'a>( &'a self, prefix: &'a A2aPrefix, @@ -18,7 +22,8 @@ pub trait AuditEmitter: Send + Sync { envelope: AuditEnvelope, ) -> BoxFuture<'a, ()>; - /// Published on each [`TaskLifecycleEnvelope`] emitted from the streaming task pump (`message/stream`). + /// Published on `{prefix}.v1.audit.{agent_id}.lifecycle` for each + /// [`TaskLifecycleEnvelope`] emitted from the streaming task pump (`message/stream`). fn publish_task_lifecycle<'a>( &'a self, prefix: &'a A2aPrefix, @@ -52,12 +57,7 @@ where crate::audit::envelope::AuditOutcome::Ok => "ok", crate::audit::envelope::AuditOutcome::Err { .. } => "err", }; - let subject = format!( - "{}.v1.audit.{}.{}", - prefix.as_str(), - outcome_token, - envelope.method.replace('/', ".") - ); + let subject = format!("{}.v1.audit.{}.{}", prefix.as_str(), agent_id.as_str(), outcome_token); let payload = Bytes::from(serde_json::to_vec(&envelope).unwrap_or_default()); if let Err(e) = self .nats @@ -66,7 +66,6 @@ where { tracing::warn!(error = %e, "failed to publish audit envelope"); } - let _ = agent_id; }) } @@ -77,7 +76,7 @@ where envelope: TaskLifecycleEnvelope, ) -> BoxFuture<'a, ()> { Box::pin(async move { - let subject = format!("{}.v1.audit.lifecycle", prefix.as_str()); + let subject = format!("{}.v1.audit.{}.lifecycle", prefix.as_str(), agent_id.as_str()); let payload = Bytes::from(serde_json::to_vec(&envelope).unwrap_or_default()); if let Err(e) = self .nats @@ -86,7 +85,6 @@ where { tracing::warn!(error = %e, "failed to publish task lifecycle audit envelope"); } - let _ = agent_id; }) } } diff --git a/rsworkspace/crates/a2a/a2a-nats/src/audit/emitter/tests.rs b/rsworkspace/crates/a2a/a2a-nats/src/audit/emitter/tests.rs index 02347598b0..21388d1865 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/audit/emitter/tests.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/audit/emitter/tests.rs @@ -30,7 +30,7 @@ async fn nats_emitter_publishes_ok_subject() { emitter .publish(&prefix(), &agent(), make_envelope(AuditOutcome::Ok)) .await; - assert_eq!(nats.published_messages(), vec!["a2a.v1.audit.ok.message.send"]); + assert_eq!(nats.published_messages(), vec!["a2a.v1.audit.bot.ok"]); } #[tokio::test] @@ -47,7 +47,24 @@ async fn nats_emitter_publishes_err_subject() { }), ) .await; - assert_eq!(nats.published_messages(), vec!["a2a.v1.audit.err.message.send"]); + assert_eq!(nats.published_messages(), vec!["a2a.v1.audit.bot.err"]); +} + +#[tokio::test] +async fn nats_emitter_scopes_the_subject_to_the_agent_and_keeps_the_method_off_it() { + // Guards the ADR#0055 operational profile: the emitter used to drop + // `agent_id` and spend the terminal on the method instead, so audit + // traffic could not be filtered per agent. + let nats = AdvancedMockNatsClient::new(); + let emitter = NatsAuditEmitter::new(nats.clone()); + let other = A2aAgentId::new("planner").unwrap(); + emitter + .publish(&prefix(), &other, make_envelope(AuditOutcome::Ok)) + .await; + assert_eq!(nats.published_messages(), vec!["a2a.v1.audit.planner.ok"]); + let payloads = nats.published_payloads(); + let v: serde_json::Value = serde_json::from_slice(&payloads[0]).unwrap(); + assert_eq!(v["method"], "message/send"); } #[tokio::test] @@ -76,7 +93,7 @@ async fn nats_emitter_task_lifecycle_publishes_lifecycle_subject() { 4000, ); emitter.publish_task_lifecycle(&prefix(), &agent(), env).await; - assert_eq!(nats.published_messages(), vec!["a2a.v1.audit.lifecycle"]); + assert_eq!(nats.published_messages(), vec!["a2a.v1.audit.bot.lifecycle"]); let payloads = nats.published_payloads(); let v: serde_json::Value = serde_json::from_slice(&payloads[0]).unwrap(); assert_eq!(v["task_id"], "task-xyz"); diff --git a/rsworkspace/crates/a2a/a2a-nats/src/client/event_stream.rs b/rsworkspace/crates/a2a/a2a-nats/src/client/event_stream.rs index a041430289..34c7e415bd 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/client/event_stream.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/client/event_stream.rs @@ -11,6 +11,7 @@ use trogon_nats::jetstream::{JetStreamConsumer, JsAck, JsMessageRef}; use crate::req_id::ReqId; use super::error::ClientError; +use super::wire::{decode_response, map_wire_error}; pub struct TypedEventStream { receiver: mpsc::UnboundedReceiver>, @@ -127,11 +128,9 @@ async fn pull_loop( } let stream_seq = stream_sequence_from_reply(js_msg.message().reply.as_deref()); - let payload = js_msg.message().payload.as_ref(); - let send_result = match serde_json::from_slice::(payload) { - Ok(event) => tx.unbounded_send(Ok(event)), - Err(e) => tx.unbounded_send(Err(ClientError::Deserialize(e))), - }; + let message = js_msg.message(); + let event_headers = message.headers.clone().unwrap_or_default(); + let send_result = tx.unbounded_send(decode_event(&event_headers, message.payload.as_ref())); if send_result.is_err() { return; @@ -152,6 +151,17 @@ async fn pull_loop( } } +/// Each task event on the wire is a full JSON-RPC success response repeating the +/// request id, which is the shape the A2A spec puts on `message/stream` and +/// `tasks/resubscribe`. The result member is the `StreamResponse` the reader wants. +fn decode_event(headers: &async_nats::header::HeaderMap, payload: &[u8]) -> Result { + match decode_response::(headers, payload) { + Ok(Ok(event)) => Ok(event), + Ok(Err((code, message))) => Err(ClientError::from_jsonrpc_code(code, message)), + Err(e) => Err(map_wire_error(e)), + } +} + /// Parse the JetStream stream sequence from the message reply subject. /// /// JetStream encodes delivery metadata in the reply subject: diff --git a/rsworkspace/crates/a2a/a2a-nats/src/client/event_stream/tests.rs b/rsworkspace/crates/a2a/a2a-nats/src/client/event_stream/tests.rs index fc8e33a5d1..3590a39b17 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/client/event_stream/tests.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/client/event_stream/tests.rs @@ -18,6 +18,17 @@ fn make_status_event(task_id: &str) -> StreamResponse { }) } +/// The wire body of a task event: a JSON-RPC success response repeating the +/// request id, with the `StreamResponse` as its result. +fn event_body(id: &str, event: &StreamResponse) -> Vec { + serde_json::to_vec(&serde_json::json!({ + "jsonrpc": "2.0", + "id": id, + "result": event, + })) + .unwrap() +} + fn nats_msg_with_reply(payload: Vec, reply: Option<&str>) -> async_nats::Message { async_nats::Message { subject: "a2a.v1.tasks.t1.events".into(), @@ -40,8 +51,7 @@ async fn stream_yields_deserialized_events() { let last_seq = Arc::new(Mutex::new(0u64)); let mut stream = build_event_stream(consumer, last_seq.clone(), None); - let event = make_status_event("task-1"); - let payload = serde_json::to_vec(&event).unwrap(); + let payload = event_body("req-1", &make_status_event("task-1")); tx.unbounded_send(Ok(MockJsMessage::new(nats_msg_with_reply(payload, None)))) .unwrap(); drop(tx); @@ -52,7 +62,7 @@ async fn stream_yields_deserialized_events() { } fn event_stamped_for(req_id: &str) -> async_nats::Message { - let payload = serde_json::to_vec(&make_status_event(req_id)).unwrap(); + let payload = event_body(req_id, &make_status_event(req_id)); let mut message = nats_msg_with_reply(payload, Some(&ack_reply(1))); let mut headers = async_nats::HeaderMap::new(); headers.insert(crate::constants::REQ_ID_HEADER, req_id); @@ -138,6 +148,39 @@ async fn stream_yields_error_on_bad_payload() { assert!(matches!(item, Some(Err(ClientError::Deserialize(_))))); } +#[tokio::test] +async fn stream_yields_error_on_bare_result_payload() { + let (consumer, tx) = MockJetStreamConsumer::new(); + let last_seq = Arc::new(Mutex::new(0u64)); + let mut stream = build_event_stream(consumer, last_seq, None); + + let payload = serde_json::to_vec(&make_status_event("task-1")).unwrap(); + tx.unbounded_send(Ok(MockJsMessage::new(nats_msg_with_reply(payload, None)))) + .unwrap(); + drop(tx); + + assert!(matches!(stream.next().await, Some(Err(ClientError::Deserialize(_))))); +} + +#[tokio::test] +async fn stream_maps_a_jsonrpc_error_event_to_a_typed_error() { + let (consumer, tx) = MockJetStreamConsumer::new(); + let last_seq = Arc::new(Mutex::new(0u64)); + let mut stream = build_event_stream(consumer, last_seq, None); + + let payload = serde_json::to_vec(&serde_json::json!({ + "jsonrpc": "2.0", + "id": "req-1", + "error": { "code": crate::constants::TASK_NOT_FOUND, "message": "Task not found" }, + })) + .unwrap(); + tx.unbounded_send(Ok(MockJsMessage::new(nats_msg_with_reply(payload, None)))) + .unwrap(); + drop(tx); + + assert!(matches!(stream.next().await, Some(Err(ClientError::TaskNotFound)))); +} + #[tokio::test] async fn stream_yields_error_on_consumer_stream_error() { let (consumer, tx) = MockJetStreamConsumer::new(); @@ -168,8 +211,7 @@ async fn last_seq_advances_only_after_successful_downstream_send() { let last_seq = Arc::new(Mutex::new(0u64)); let mut stream = build_event_stream(consumer, last_seq.clone(), None); - let event = make_status_event("task-1"); - let payload = serde_json::to_vec(&event).unwrap(); + let payload = event_body("req-1", &make_status_event("task-1")); let reply = ack_reply(7); tx.unbounded_send(Ok(MockJsMessage::new(nats_msg_with_reply(payload, Some(&reply))))) .unwrap(); @@ -186,8 +228,7 @@ async fn ack_failure_does_not_stop_delivery() { let last_seq = Arc::new(Mutex::new(0u64)); let mut stream = build_event_stream(consumer, last_seq.clone(), None); - let event = make_status_event("task-1"); - let payload = serde_json::to_vec(&event).unwrap(); + let payload = event_body("req-1", &make_status_event("task-1")); let reply = ack_reply(3); tx.unbounded_send(Ok(MockJsMessage::with_failing_signals(nats_msg_with_reply( payload, @@ -212,8 +253,7 @@ async fn pull_loop_returns_early_when_receiver_dropped() { let (tx, receiver) = mpsc::unbounded::>(); drop(receiver); // Channel closed before pull_loop sees the message. - let event = make_status_event("task-1"); - let payload = serde_json::to_vec(&event).unwrap(); + let payload = event_body("req-1", &make_status_event("task-1")); let reply = ack_reply(42); msg_tx .unbounded_send(Ok(MockJsMessage::new(nats_msg_with_reply(payload, Some(&reply))))) diff --git a/rsworkspace/crates/a2a/a2a-nats/src/client/streaming.rs b/rsworkspace/crates/a2a/a2a-nats/src/client/streaming.rs index 67db45f1d0..f80f92d45c 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/client/streaming.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/client/streaming.rs @@ -1,6 +1,7 @@ use std::sync::{Arc, Mutex}; use a2a::types::SendMessageResponse; +use jsonrpc_nats::RequestId; use serde::Serialize; use tokio::time::timeout; use trogon_nats::RequestClient; @@ -11,7 +12,6 @@ use a2a_identity_types::MintedUserJwt; use crate::a2a_prefix::A2aPrefix; use crate::jetstream::consumers::stream_events_consumer; use crate::jetstream::streams::events_stream_name; -use crate::jsonrpc::JsonRpcId; use crate::req_id::ReqId; use crate::task_id::A2aTaskId; @@ -77,7 +77,7 @@ where op_timeout, gateway_caller_jwt, } = ctx; - let encoded = encode_client_request(method, JsonRpcId::String(req_id.as_str().to_owned()), params) + let encoded = encode_client_request(method, RequestId::String(req_id.as_str().to_owned()), params) .map_err(|e| ClientError::Serialize(::custom(format!("{e}"))))?; let headers = match gateway_caller_jwt { diff --git a/rsworkspace/crates/a2a/a2a-nats/src/client/unary.rs b/rsworkspace/crates/a2a/a2a-nats/src/client/unary.rs index f7ee04a046..cc78b3d113 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/client/unary.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/client/unary.rs @@ -1,17 +1,17 @@ use std::time::Duration; +use jsonrpc_nats::RequestId; use serde::{Serialize, de::DeserializeOwned}; use trogon_nats::RequestClient; use a2a_identity_types::MintedUserJwt; -use crate::jsonrpc::JsonRpcId; use crate::req_id::ReqId; use super::error::ClientError; use super::gateway_headers::{agent_rpc_headers, gateway_ingress_rpc_headers}; use super::validated::ValidatedRpc; -use super::wire::{decode_client_response, encode_client_request, merge_jsonrpc_headers}; +use super::wire::{decode_client_response, encode_client_request, map_wire_error, merge_jsonrpc_headers}; pub async fn send_unary( nats: &N, @@ -50,7 +50,7 @@ where Req: Serialize, Res: DeserializeOwned, { - let encoded = encode_client_request(method, JsonRpcId::String(req_id.as_str().to_owned()), params) + let encoded = encode_client_request(method, RequestId::String(req_id.as_str().to_owned()), params) .map_err(|e| ClientError::Serialize(::custom(format!("{e}"))))?; let headers = match gateway_caller_jwt { @@ -77,12 +77,5 @@ where } } -fn map_wire_error(error: crate::wire::WireError) -> ClientError { - match error { - crate::wire::WireError::Deserialize(e) => ClientError::Deserialize(e), - other => ClientError::Deserialize(::custom(format!("{other}"))), - } -} - #[cfg(test)] mod tests; diff --git a/rsworkspace/crates/a2a/a2a-nats/src/client/wire.rs b/rsworkspace/crates/a2a/a2a-nats/src/client/wire.rs index 708e9c4c18..9e04569562 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/client/wire.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/client/wire.rs @@ -6,17 +6,10 @@ use async_nats::header::HeaderMap; use jsonrpc_nats::{Encoded, RequestId}; use serde::{Serialize, de::DeserializeOwned}; -use crate::jsonrpc::JsonRpcId; +use crate::client::error::ClientError; -pub fn encode_client_request(method: &str, id: JsonRpcId, params: &Req) -> Result { - let request_id = match id { - JsonRpcId::Number(n) => RequestId::Number(n), - JsonRpcId::String(s) => RequestId::String(s), - JsonRpcId::Null => { - return Err(WireError::Codec(jsonrpc_nats::CodecError::RequestWithoutId)); - } - }; - encode_request(method, request_id, params) +pub fn encode_client_request(method: &str, id: RequestId, params: &Req) -> Result { + encode_request(method, id, params) } pub fn decode_client_response( @@ -26,5 +19,12 @@ pub fn decode_client_response( decode_response(headers, body) } +pub fn map_wire_error(error: WireError) -> ClientError { + match error { + WireError::Deserialize(e) => ClientError::Deserialize(e), + other => ClientError::Deserialize(::custom(format!("{other}"))), + } +} + #[cfg(test)] mod tests; diff --git a/rsworkspace/crates/a2a/a2a-nats/src/client/wire/tests.rs b/rsworkspace/crates/a2a/a2a-nats/src/client/wire/tests.rs index 3dc52d485d..fa3e5454e7 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/client/wire/tests.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/client/wire/tests.rs @@ -3,7 +3,6 @@ use jsonrpc_nats::{Direction, Message, RequestId, ResponseId, decode, encode, to use serde::{Deserialize, Serialize}; use super::*; -use crate::jsonrpc::JsonRpcId; #[derive(Serialize, Deserialize, Debug, PartialEq)] struct DummyParams { @@ -17,7 +16,7 @@ struct DummyResult { #[test] fn encode_client_request_puts_id_in_header_and_params_in_body() { - let encoded = encode_client_request("tasks/get", JsonRpcId::Number(1), &DummyParams { value: "x".into() }).unwrap(); + let encoded = encode_client_request("tasks/get", RequestId::Number(1), &DummyParams { value: "x".into() }).unwrap(); assert!(encoded.headers.get(jsonrpc_nats::HEADER_ID).is_some()); let body: serde_json::Value = serde_json::from_slice(&encoded.body).unwrap(); let params: DummyParams = serde_json::from_value(body["params"].clone()).unwrap(); @@ -56,7 +55,7 @@ fn decode_client_error_response() { fn roundtrip_reconstructs_canonical_json_at_edge() { let encoded = encode_client_request( "tasks/get", - JsonRpcId::String("abc".into()), + RequestId::String("abc".into()), &DummyParams { value: "x".into() }, ) .unwrap(); @@ -80,9 +79,3 @@ fn merge_headers_overlays_jsonrpc_fields() { assert_eq!(merged.get("Trogon-Req-Id").unwrap().as_str(), "transport"); assert!(merged.get(jsonrpc_nats::HEADER_ID).is_some()); } - -#[test] -fn encode_client_request_rejects_null_id() { - let err = encode_client_request("tasks/get", JsonRpcId::Null, &DummyParams { value: "x".into() }).unwrap_err(); - assert!(matches!(err, WireError::Codec(_))); -} diff --git a/rsworkspace/crates/a2a/a2a-nats/src/gateway_ingress.rs b/rsworkspace/crates/a2a/a2a-nats/src/gateway_ingress.rs index 3764e2e6ab..842e8b468e 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/gateway_ingress.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/gateway_ingress.rs @@ -12,7 +12,7 @@ use jsonrpc_nats::Encoded; use crate::a2a_prefix::A2aPrefix; use crate::agent_id::A2aAgentId; pub use crate::constants::GATEWAY_INGRESS_METHOD_SUFFIXES; -use crate::jsonrpc::{JsonRpcId, extract_request_id, extract_request_id_from_body}; +use crate::jsonrpc::{extract_request_id, extract_request_id_from_body}; use crate::wire::{WireError, encode_error, response_id_from_request_headers}; /// Failure resolving a `{prefix}.v1.gateway.` subject to an agent RPC subject. @@ -149,18 +149,9 @@ fn validate_agent_id(segment: &str) -> Result<(), GatewayIngressError> { } fn response_id_for_ingress(request_headers: &HeaderMap, request_payload_hint: &[u8]) -> jsonrpc_nats::ResponseId { - if let Some(id) = extract_request_id(request_headers) { - return match id { - JsonRpcId::Number(n) => jsonrpc_nats::ResponseId::Number(n), - JsonRpcId::String(s) => jsonrpc_nats::ResponseId::String(s), - JsonRpcId::Null => jsonrpc_nats::ResponseId::Null, - }; - } - match extract_request_id_from_body(request_payload_hint) { - Some(JsonRpcId::Number(n)) => jsonrpc_nats::ResponseId::Number(n), - Some(JsonRpcId::String(s)) => jsonrpc_nats::ResponseId::String(s), - Some(JsonRpcId::Null) | None => jsonrpc_nats::ResponseId::Null, - } + extract_request_id(request_headers) + .or_else(|| extract_request_id_from_body(request_payload_hint)) + .unwrap_or(jsonrpc_nats::ResponseId::Null) } fn ingress_error_wire( diff --git a/rsworkspace/crates/a2a/a2a-nats/src/jsonrpc.rs b/rsworkspace/crates/a2a/a2a-nats/src/jsonrpc.rs index a8f8478e53..04af36e258 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/jsonrpc.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/jsonrpc.rs @@ -9,55 +9,24 @@ use async_nats::header::HeaderMap; use jsonrpc_nats::ResponseId; use serde_json::Value; -/// Minimal JSON-RPC id, mirroring the subset A2A uses. -#[derive(Debug, Clone, PartialEq, Eq, serde::Deserialize, serde::Serialize)] -#[serde(untagged)] -pub enum JsonRpcId { - Number(i64), - String(String), - Null, -} - -impl std::fmt::Display for JsonRpcId { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match self { - Self::Number(n) => write!(f, "{n}"), - Self::String(s) => f.write_str(s), - Self::Null => f.write_str("null"), - } - } -} - -impl From for JsonRpcId { - fn from(id: ResponseId) -> Self { - match id { - ResponseId::Number(n) => Self::Number(n), - ResponseId::String(s) => Self::String(s), - ResponseId::Null => Self::Null, - } - } -} - /// Extracts the JSON-RPC id from `Jsonrpc-Id` request headers. /// -/// Returns `None` when the header is absent (notification). Returns `Some(JsonRpcId::Null)` +/// Returns `None` when the header is absent (notification). Returns `Some(ResponseId::Null)` /// when the header carries the JSON literal `null`. -pub fn extract_request_id(headers: &HeaderMap) -> Option { +pub fn extract_request_id(headers: &HeaderMap) -> Option { let value = headers.get(jsonrpc_nats::HEADER_ID)?.as_str(); - jsonrpc_nats::decode_response_id_literal(value) - .ok() - .map(JsonRpcId::from) + jsonrpc_nats::decode_response_id_literal(value).ok() } /// Legacy body-based id extraction retained for transitional call sites that only /// have a payload hint (e.g. gateway ingress error helpers before headers arrive). -pub fn extract_request_id_from_body(raw: &[u8]) -> Option { +pub fn extract_request_id_from_body(raw: &[u8]) -> Option { let value: Value = serde_json::from_slice(raw).ok()?; let id = value.as_object()?.get("id")?; match id { - Value::Number(n) => n.as_i64().map(JsonRpcId::Number), - Value::String(s) => Some(JsonRpcId::String(s.clone())), - Value::Null => Some(JsonRpcId::Null), + Value::Number(n) => n.as_i64().map(ResponseId::Number), + Value::String(s) => Some(ResponseId::String(s.clone())), + Value::Null => Some(ResponseId::Null), _ => None, } } diff --git a/rsworkspace/crates/a2a/a2a-nats/src/jsonrpc/tests.rs b/rsworkspace/crates/a2a/a2a-nats/src/jsonrpc/tests.rs index d5c0ca199a..894eea0506 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/jsonrpc/tests.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/jsonrpc/tests.rs @@ -13,14 +13,14 @@ fn headers_with_id(id: &str) -> HeaderMap { fn extract_numeric_id_from_header() { let mut headers = HeaderMap::new(); headers.insert(jsonrpc_nats::HEADER_ID, "42"); - assert_eq!(extract_request_id(&headers), Some(JsonRpcId::Number(42))); + assert_eq!(extract_request_id(&headers), Some(ResponseId::Number(42))); } #[test] fn extract_string_id_from_header() { assert_eq!( extract_request_id(&headers_with_id("\"abc-123\"")), - Some(JsonRpcId::String("abc-123".into())) + Some(ResponseId::String("abc-123".into())) ); } @@ -28,7 +28,7 @@ fn extract_string_id_from_header() { fn extract_null_id_from_header() { let mut headers = HeaderMap::new(); headers.insert(jsonrpc_nats::HEADER_ID, "null"); - assert_eq!(extract_request_id(&headers), Some(JsonRpcId::Null)); + assert_eq!(extract_request_id(&headers), Some(ResponseId::Null)); } #[test] @@ -39,7 +39,7 @@ fn missing_header_returns_none() { #[test] fn extract_request_id_from_body_still_works() { let raw = br#"{"jsonrpc":"2.0","id":42,"method":"message/send","params":{}}"#; - assert_eq!(extract_request_id_from_body(raw), Some(JsonRpcId::Number(42))); + assert_eq!(extract_request_id_from_body(raw), Some(ResponseId::Number(42))); } #[test] @@ -50,33 +50,19 @@ fn boolean_id_in_body_returns_none() { #[test] fn id_roundtrips_through_serde() { - let id = JsonRpcId::String("x".into()); + let id = ResponseId::String("x".into()); let bytes = serde_json::to_vec(&id).unwrap(); - let back: JsonRpcId = serde_json::from_slice(&bytes).unwrap(); + let back: ResponseId = serde_json::from_slice(&bytes).unwrap(); assert_eq!(id, back); } +/// A request id may not be null, so the request path takes `RequestId`, which has +/// no null variant, and crossing from request to response is infallible. #[test] -fn display_covers_every_variant() { - assert_eq!(format!("{}", JsonRpcId::Number(42)), "42"); - assert_eq!(format!("{}", JsonRpcId::String("abc".into())), "abc"); - assert_eq!(format!("{}", JsonRpcId::Null), "null"); -} - -#[test] -fn response_id_converts_to_jsonrpc_id() { +fn a_request_id_is_always_a_valid_response_id() { assert_eq!( - JsonRpcId::from(ResponseId::String("req".into())), - JsonRpcId::String("req".into()) + ResponseId::from(RequestId::String("req".into())), + ResponseId::String("req".into()) ); -} - -#[test] -fn request_id_converts_for_client_encoding() { - let id = JsonRpcId::String("req".into()); - let req = match id { - JsonRpcId::String(s) => RequestId::String(s), - _ => panic!("expected string"), - }; - assert!(matches!(req, RequestId::String(_))); + assert!(RequestId::try_from(ResponseId::Null).is_err()); } diff --git a/rsworkspace/crates/a2a/a2a-nats/src/lib.rs b/rsworkspace/crates/a2a/a2a-nats/src/lib.rs index 5ad20b1c2d..303225b351 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/lib.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/lib.rs @@ -38,7 +38,8 @@ pub use gateway_ingress::{ ingress_gateway_tier3_refused_response_bytes, ingress_invalid_request_response_bytes, resolve_gateway_ingress_subject, }; -pub use jsonrpc::{JsonRpcId, extract_request_id}; +pub use jsonrpc::extract_request_id; +pub use jsonrpc_nats::{RequestId, ResponseId}; pub use req_id::ReqId; pub use server::A2aMethod; pub use task_id::{A2aTaskId, TaskIdError}; diff --git a/rsworkspace/crates/a2a/a2a-nats/src/server/message_stream.rs b/rsworkspace/crates/a2a/a2a-nats/src/server/message_stream.rs index 5d1e3bb575..3c567c7aea 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/server/message_stream.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/server/message_stream.rs @@ -1,4 +1,5 @@ use futures::StreamExt; +use jsonrpc_nats::ResponseId; use tracing::{instrument, warn}; use trogon_nats::jetstream::JetStreamPublisher; use trogon_semconv::span::A2A_SERVER_MESSAGE_STREAM; @@ -7,7 +8,6 @@ use crate::a2a_prefix::A2aPrefix; use crate::constants::{ GATEWAY_CALLER_ID_HEADER, GATEWAY_PRINCIPAL_HEADER, MESSAGE_STREAM_METHOD as METHOD, REQ_ID_HEADER, }; -use crate::jsonrpc::JsonRpcId; use crate::nats::subjects::tasks::TaskEventsSubject; use crate::req_id::ReqId; use crate::server::handler::{A2aError, A2aExecutor}; @@ -15,6 +15,7 @@ use crate::server::wire::{ encode_error_reply, encode_success_reply, is_notification, parse_request_params, request_id, }; use crate::task_id::A2aTaskId; +use crate::wire::merge_jsonrpc_headers; /// Handles `message/stream`. /// @@ -95,21 +96,28 @@ pub async fn handle( let events_subject = TaskEventsSubject::new(prefix, &task_id).to_string(); let event_headers = event_headers(headers, &req_id); while let Some(item) = events.next().await { - let payload = match item { - Ok(event) => match serde_json::to_vec(&event) { - Ok(b) => bytes::Bytes::from(b), - Err(e) => { - warn!(error = %e, "failed to serialize task event; ending stream"); - return; - } - }, + let event = match item { + Ok(event) => event, Err(e) => { warn!(error = %e, "task event stream yielded error; ending stream"); return; } }; + // Each event is a full JSON-RPC success response repeating the request id, + // which is the shape the A2A spec puts on the wire for `message/stream` and + // `tasks/resubscribe`. Every hop downstream (gateway pump, bridge, stdio, + // SSE) forwards these bytes verbatim, so encoding once here is what keeps + // the edges from each inventing their own envelope. + let encoded = match encode_success_reply(headers, &event) { + Ok(encoded) => encoded, + Err(e) => { + warn!(error = %e, "failed to encode task event; ending stream"); + return; + } + }; let subject = async_nats::Subject::from(events_subject.as_str()); - if let Err(e) = js.publish_with_headers(subject, event_headers.clone(), payload).await { + let publish_headers = merge_jsonrpc_headers(event_headers.clone(), encoded.headers); + if let Err(e) = js.publish_with_headers(subject, publish_headers, encoded.body).await { warn!(error = %e, "failed to publish task event to JetStream; ending stream"); return; } @@ -119,9 +127,9 @@ pub async fn handle( /// Correlation and caller identity for every event of one subscription. /// /// The subject only names the task, so `Trogon-Req-Id` is what tells concurrent -/// subscribers of that task apart (ADR#0055). Caller identity rides along because -/// the gateway's egress pump gates per caller and can no longer recover it from -/// the subject either. +/// subscribers of that task apart (ADR#0055) without every hop having to parse the +/// body. Caller identity rides along because the gateway's egress pump gates per +/// caller and can no longer recover it from the subject either. fn event_headers(request_headers: &async_nats::header::HeaderMap, req_id: &ReqId) -> async_nats::header::HeaderMap { let mut headers = async_nats::header::HeaderMap::new(); headers.insert(REQ_ID_HEADER, req_id.as_str()); @@ -137,12 +145,12 @@ async fn prepare_bootstrap( handler: &H, headers: &async_nats::header::HeaderMap, payload: &[u8], - id: &Option, + id: &Option, ) -> Result<(a2a::types::Task, crate::server::handler::TaskEventStream, ReqId), A2aError> { let raw = parse_request_params::(METHOD, headers, payload) .map_err(|_| A2aError::new(-32700, "Parse error"))?; let req_id = match id { - Some(JsonRpcId::String(s)) => ReqId::from_header(s.clone()), + Some(ResponseId::String(s)) => ReqId::from_header(s.clone()), _ => { return Err(A2aError::new( -32602, diff --git a/rsworkspace/crates/a2a/a2a-nats/src/server/message_stream/tests.rs b/rsworkspace/crates/a2a/a2a-nats/src/server/message_stream/tests.rs index 1dfe40b79e..7c283eddc6 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/server/message_stream/tests.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/server/message_stream/tests.rs @@ -73,6 +73,30 @@ async fn bootstrap_publishes_task_and_then_events() { assert_eq!(js.published_subjects(), vec!["a2a.v1.tasks.task-1.events".to_string()]); } +#[tokio::test] +async fn every_event_is_a_jsonrpc_response_repeating_the_request_id() { + let nats = AdvancedMockNatsClient::new(); + let js = MockJetStreamPublisher::new(); + let handler = stub(); + let events: crate::server::handler::TaskEventStream = + Box::pin(stream::iter(vec![Ok(working_status_event("task-1"))])); + handler.lock().unwrap().message_stream_result = Some(Ok((task("task-1"), events))); + + let (headers, payload) = stream_payload("call-1"); + handle(&handler, &headers, &payload, Some("r".into()), &nats, &js, &prefix()).await; + + let published = js.published_messages(); + let event = published.first().expect("one event"); + let body: serde_json::Value = serde_json::from_slice(&event.payload).expect("json body"); + assert_eq!(body["jsonrpc"], "2.0"); + assert_eq!(body["id"], "call-1"); + assert_eq!(body["result"]["statusUpdate"]["taskId"], "task-1"); + assert_eq!( + event.headers.get(jsonrpc_nats::HEADER_ID).map(|v| v.as_str()), + Some("\"call-1\"") + ); +} + #[tokio::test] async fn every_event_carries_the_request_id_header() { // The subject names only the task, so `Trogon-Req-Id` is what tells two diff --git a/rsworkspace/crates/a2a/a2a-nats/src/server/wire.rs b/rsworkspace/crates/a2a/a2a-nats/src/server/wire.rs index 56fa31318b..610a5800ff 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/server/wire.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/server/wire.rs @@ -1,10 +1,9 @@ //! Server-side JSON-RPC content-mode wire helpers for A2A over NATS. use async_nats::header::HeaderMap; -use jsonrpc_nats::Encoded; +use jsonrpc_nats::{Encoded, ResponseId}; use trogon_nats::PublishClient; -use crate::jsonrpc::JsonRpcId; use crate::wire::{WireError, encode_error, encode_success, response_id_from_request_headers}; pub use crate::wire::{WireError as ServerWireError, decode_request_params as parse_request_params, is_notification}; @@ -47,7 +46,7 @@ pub fn encode_error_reply( encode_error(response_id_from_request_headers(request_headers), code, message, data) } -pub fn request_id(headers: &HeaderMap) -> Option { +pub fn request_id(headers: &HeaderMap) -> Option { crate::jsonrpc::extract_request_id(headers) } diff --git a/rsworkspace/crates/a2a/a2a-nats/src/server/wire/tests.rs b/rsworkspace/crates/a2a/a2a-nats/src/server/wire/tests.rs index f6f42a2489..d8a4ac4663 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/server/wire/tests.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/server/wire/tests.rs @@ -1,8 +1,7 @@ use async_nats::header::HeaderMap; -use jsonrpc_nats::encode; +use jsonrpc_nats::{ResponseId, encode}; use super::*; -use crate::jsonrpc::JsonRpcId; use crate::wire::{ decode_request_params, encode_error, encode_success, is_notification, response_id_from_request_headers, }; @@ -58,7 +57,7 @@ fn encode_error_reply_sets_error_code_header() { fn optional_id_states_decode_from_headers() { let mut headers = HeaderMap::new(); headers.insert(jsonrpc_nats::HEADER_ID, "null"); - assert_eq!(crate::jsonrpc::extract_request_id(&headers), Some(JsonRpcId::Null)); + assert_eq!(crate::jsonrpc::extract_request_id(&headers), Some(ResponseId::Null)); } #[test] @@ -97,5 +96,5 @@ fn request_id_returns_none_when_header_absent() { fn request_id_returns_some_when_header_present() { let mut headers = HeaderMap::new(); headers.insert(jsonrpc_nats::HEADER_ID, "5"); - assert_eq!(request_id(&headers), Some(JsonRpcId::Number(5))); + assert_eq!(request_id(&headers), Some(ResponseId::Number(5))); } From 2251af766de65a9f0515dedfc06e5bfc7f4c3693 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Fri, 14 Aug 2026 18:50:41 -0400 Subject: [PATCH 03/12] fix(a2a): let the SSE caller correlate on the id it sent The hops behind the bridge correlate on their own transport id, which the caller never saw and cannot match against its own request. Signed-off-by: Yordis Prieto --- .../crates/a2a/a2a-bridge/src/inbound.rs | 23 ++++++++-- .../a2a/a2a-bridge/src/inbound/tests.rs | 42 +++++++++++++++++++ .../a2a-bridge/src/nats_transport_harness.rs | 5 ++- .../src/nats_transport_harness/tests.rs | 6 ++- 4 files changed, 70 insertions(+), 6 deletions(-) diff --git a/rsworkspace/crates/a2a/a2a-bridge/src/inbound.rs b/rsworkspace/crates/a2a/a2a-bridge/src/inbound.rs index f57a1a8322..23e19fc106 100644 --- a/rsworkspace/crates/a2a/a2a-bridge/src/inbound.rs +++ b/rsworkspace/crates/a2a/a2a-bridge/src/inbound.rs @@ -20,7 +20,9 @@ use bytes::Bytes; use futures_util::StreamExt; use futures_util::stream::{self, BoxStream, Stream}; use serde_json::{Value, json}; +use tracing::warn; +use a2a_nats::client::rewrite_response_id; use a2a_nats::constants::{GATEWAY_CALLER_ID_HEADER, GATEWAY_CALLER_ID_HTTP, REQ_ID_HEADER}; use a2a_nats::jetstream::consumers::{PullConfig, resubscribe_consumer, stream_events_consumer}; use a2a_nats::jetstream::streams::events_stream_name; @@ -479,8 +481,21 @@ impl TaskJetStreamPort for ScriptedTaskJetstream { /// that repeats the caller's request id. Naming the events instead would put the /// bootstrap and the task chunks on distinct SSE event types, which a spec client /// never subscribes to, and the bodies already say which is which. -fn sse_data_line(body: &[u8]) -> Event { - Event::default().data(String::from_utf8_lossy(body)) +/// +/// The id is restamped rather than forwarded, because the hops behind this edge +/// correlate on their own transport id (`Trogon-Req-Id`, or a minted one when the +/// caller sent no id at all). Only the caller's own id is meaningful to the +/// caller. A body that is not a JSON object goes out untouched: there is no +/// envelope to stamp, and swallowing it would lose the only diagnostic the caller +/// would get. +fn sse_data_line(body: &[u8], caller_id: &Value) -> Event { + match rewrite_response_id(body, caller_id) { + Ok(stamped) => Event::default().data(String::from_utf8_lossy(stamped.as_ref())), + Err(e) => { + warn!(error = %e, "SSE frame is not a JSON-RPC envelope; forwarding without a caller id"); + Event::default().data(String::from_utf8_lossy(body)) + } + } } fn sse_error_line(caller_id: &Value, err: &BridgeError) -> Event { @@ -497,11 +512,11 @@ fn sse_from_bootstrap_and_payloads( tail: Pin> + Send>>, caller_id: Value, ) -> BoxStream<'static, Result> { - let head_event = sse_data_line(&bootstrap_owned); + let head_event = sse_data_line(&bootstrap_owned, &caller_id); let head = futures_util::stream::once(futures_util::future::ready(Ok::(head_event))); let tail_mapped = tail.map(move |item| { Ok::(match item { - Ok(chunk) => sse_data_line(chunk.as_ref()), + Ok(chunk) => sse_data_line(chunk.as_ref(), &caller_id), Err(ref err) => sse_error_line(&caller_id, err), }) }); diff --git a/rsworkspace/crates/a2a/a2a-bridge/src/inbound/tests.rs b/rsworkspace/crates/a2a/a2a-bridge/src/inbound/tests.rs index 4486d2dd48..bff0c61459 100644 --- a/rsworkspace/crates/a2a/a2a-bridge/src/inbound/tests.rs +++ b/rsworkspace/crates/a2a/a2a-bridge/src/inbound/tests.rs @@ -373,3 +373,45 @@ async fn handle_jsonrpc_invalid_agent_header_errors() { let err = handle_jsonrpc(headers, Bytes::new(), &state).await.unwrap_err(); assert!(matches!(err, BridgeError::InvalidAgent(_))); } + +/// Reads an SSE stream the way a caller does: every frame is an unnamed +/// `data:` line, so the JSON bodies are what the assertions look at. +async fn sse_frames(stream: BoxStream<'static, Result>) -> Vec { + use axum::response::IntoResponse; + let response = Sse::new(stream).into_response(); + let bytes = axum::body::to_bytes(response.into_body(), usize::MAX).await.unwrap(); + String::from_utf8_lossy(&bytes) + .lines() + .filter_map(|line| line.strip_prefix("data:").map(|d| d.trim().to_owned())) + .collect() +} + +#[tokio::test] +async fn sse_frames_carry_the_caller_id_and_survive_a_body_that_is_not_an_envelope() { + let bootstrap = serde_json::to_vec(&json!({"jsonrpc":"2.0","id":"transport-9","result":{"taskId":"t-1"}})).unwrap(); + let tail = stream::iter(vec![ + Ok(Bytes::from_static(b"not-an-envelope")), + Err(BridgeError::JetStreamConsume("consumer gone".into())), + ]); + let frames = sse_frames(sse_from_bootstrap_and_payloads( + bootstrap, + Box::pin(tail), + json!("corr-1"), + )) + .await; + + assert_eq!(frames.len(), 3); + let bootstrap: serde_json::Value = serde_json::from_str(&frames[0]).unwrap(); + assert_eq!(bootstrap["id"], "corr-1"); + assert_eq!(bootstrap["result"]["taskId"], "t-1"); + + assert_eq!(frames[1], "not-an-envelope"); + + let failure: serde_json::Value = serde_json::from_str(&frames[2]).unwrap(); + assert_eq!(failure["id"], "corr-1"); + assert_eq!(failure["error"]["code"], INTERNAL_ERROR); + assert!( + failure["error"]["message"].as_str().unwrap().contains("consumer gone"), + "the caller keeps the only diagnostic the stream produced: {failure}" + ); +} diff --git a/rsworkspace/crates/a2a/a2a-bridge/src/nats_transport_harness.rs b/rsworkspace/crates/a2a/a2a-bridge/src/nats_transport_harness.rs index 45d342626e..1a48aabb04 100644 --- a/rsworkspace/crates/a2a/a2a-bridge/src/nats_transport_harness.rs +++ b/rsworkspace/crates/a2a/a2a-bridge/src/nats_transport_harness.rs @@ -121,10 +121,13 @@ pub fn build_nats_transport_app_state( let agent = A2aAgentId::new(agent_id).expect("fixture agent id"); let harness = Arc::new(HarnessGatewayUnary::new(nats, prefix.clone(), agent)); let publisher = GatewayInboundPublisher::new(harness.clone()); + // The scripted event carries a transport id the caller never sent, which is + // what the hops behind the bridge correlate on. The SSE edge is expected to + // restamp it with the caller's own id. let jetstream: Arc = Arc::new(ScriptedTaskJetstream::single_ok( json!({ "jsonrpc": "2.0", - "id": "corr-1", + "id": "transport-9", "result": { "statusUpdate": { "taskId": "task-sse-1" } } }) .to_string(), diff --git a/rsworkspace/crates/a2a/a2a-bridge/src/nats_transport_harness/tests.rs b/rsworkspace/crates/a2a/a2a-bridge/src/nats_transport_harness/tests.rs index 97301b6144..44816ecc79 100644 --- a/rsworkspace/crates/a2a/a2a-bridge/src/nats_transport_harness/tests.rs +++ b/rsworkspace/crates/a2a/a2a-bridge/src/nats_transport_harness/tests.rs @@ -141,8 +141,12 @@ async fn nats_transport_tasks_resubscribe_bootstraps_sse_stream() { assert_eq!(frames.len(), 2, "bootstrap plus one task event, got: {wire}"); assert_eq!(frames[0]["result"]["taskId"], "task-sse-1"); assert_eq!(frames[1]["jsonrpc"], "2.0"); - assert_eq!(frames[1]["id"], "corr-1"); assert_eq!(frames[1]["result"]["statusUpdate"]["taskId"], "task-sse-1"); + // The bootstrap arrived with a null id and the task event with the transport + // id `transport-9`; the caller correlates on neither. + for frame in &frames { + assert_eq!(frame["id"], "corr-1", "every SSE frame echoes the caller id: {wire}"); + } let audit_subject = nats .published_messages() From 2d9e38d505a8c52d56516fb51fdff349e4dc15fa Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Fri, 14 Aug 2026 18:50:44 -0400 Subject: [PATCH 04/12] chore(a2a): cover the branches this branch introduced CI gates on uncovered lines per file, and the reworked JSON-RPC edges left new arms unexercised. Signed-off-by: Yordis Prieto --- .../a2a/a2a-gateway/src/gw_ingress_stream/tests.rs | 8 ++++++++ .../crates/a2a/a2a-gateway/src/runtime/env/tests.rs | 6 ++++++ .../a2a/a2a-nats-stdio/src/io_loop/parse_tests.rs | 9 +++++++++ .../crates/a2a/a2a-nats-stdio/src/wire/tests.rs | 10 ++++++++++ .../crates/a2a/a2a-nats/src/client/wire/tests.rs | 13 +++++++++++++ 5 files changed, 46 insertions(+) diff --git a/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream/tests.rs b/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream/tests.rs index 8c589d84a6..f550ac7d26 100644 --- a/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream/tests.rs +++ b/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream/tests.rs @@ -199,6 +199,14 @@ fn req_id_from_payload_when_header_absent() { assert_eq!(req_id.as_str(), "req-from-payload"); } +#[test] +fn req_id_from_a_numeric_payload_id_uses_its_decimal_text() { + let headers = async_nats::HeaderMap::new(); + let payload = br#"{"jsonrpc":"2.0","id":7,"method":"x"}"#; + let req_id = req_id_from_headers_or_payload(&headers, payload).expect("payload extract"); + assert_eq!(req_id.as_str(), "7"); +} + #[test] fn req_id_is_none_for_a_null_json_rpc_id() { // A `"null"` string would collapse every null-id envelope onto one diff --git a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env/tests.rs b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env/tests.rs index b8fb8bb954..23582d6b48 100644 --- a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env/tests.rs +++ b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env/tests.rs @@ -194,3 +194,9 @@ fn json_rpc_audit_req_id_returns_request_id_when_present() { let payload = br#"{"jsonrpc":"2.0","id":"req-42","method":"tasks/get","params":{}}"#; assert_eq!(json_rpc_audit_req_id(payload).as_deref(), Some("req-42")); } + +#[test] +fn json_rpc_audit_req_id_renders_a_numeric_id_as_its_decimal_text() { + let payload = br#"{"jsonrpc":"2.0","id":42,"method":"tasks/get","params":{}}"#; + assert_eq!(json_rpc_audit_req_id(payload).as_deref(), Some("42")); +} diff --git a/rsworkspace/crates/a2a/a2a-nats-stdio/src/io_loop/parse_tests.rs b/rsworkspace/crates/a2a/a2a-nats-stdio/src/io_loop/parse_tests.rs index f21d2d2aa9..dc79b56b7c 100644 --- a/rsworkspace/crates/a2a/a2a-nats-stdio/src/io_loop/parse_tests.rs +++ b/rsworkspace/crates/a2a/a2a-nats-stdio/src/io_loop/parse_tests.rs @@ -38,3 +38,12 @@ fn parse_inbound_rejects_missing_or_wrong_jsonrpc_version() { let err = parse_inbound(r#"{"jsonrpc":2.0,"id":3}"#).unwrap_err(); assert_eq!(err.error_code().unwrap(), -32600); } + +#[test] +fn parse_inbound_rejects_a_notification() { + // A notification carries no id, so the stdio bridge has nothing to + // correlate a reply with; it answers requests only. + let err = parse_inbound(r#"{"jsonrpc":"2.0","method":"tasks/get","params":{}}"#).unwrap_err(); + assert_eq!(err.error_code().unwrap(), -32600); + assert_eq!(serde_json::to_value(*err).unwrap()["id"], Value::Null); +} diff --git a/rsworkspace/crates/a2a/a2a-nats-stdio/src/wire/tests.rs b/rsworkspace/crates/a2a/a2a-nats-stdio/src/wire/tests.rs index a03da03e7c..b69332b2c6 100644 --- a/rsworkspace/crates/a2a/a2a-nats-stdio/src/wire/tests.rs +++ b/rsworkspace/crates/a2a/a2a-nats-stdio/src/wire/tests.rs @@ -50,3 +50,13 @@ fn with_error_id_stamps_errors_and_leaves_other_frames_alone() { let untouched = OutboundFrame::RawBody(raw).with_error_id(ResponseId::Number(9)); assert_eq!(serde_json::to_value(&untouched).unwrap()["id"], "keep"); } + +#[test] +fn error_code_is_none_for_frames_that_are_not_errors() { + assert!( + OutboundFrame::success(ResponseId::Number(1), json!({})) + .error_code() + .is_none() + ); + assert!(OutboundFrame::RawBody(Bytes::from_static(b"{}")).error_code().is_none()); +} diff --git a/rsworkspace/crates/a2a/a2a-nats/src/client/wire/tests.rs b/rsworkspace/crates/a2a/a2a-nats/src/client/wire/tests.rs index fa3e5454e7..3b5290415b 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/client/wire/tests.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/client/wire/tests.rs @@ -65,6 +65,19 @@ fn roundtrip_reconstructs_canonical_json_at_edge() { assert_eq!(value["method"], "tasks/get"); } +#[test] +fn map_wire_error_keeps_the_deserialize_source_and_flattens_the_rest() { + let source = serde_json::from_str::("{}").unwrap_err(); + let mapped = map_wire_error(WireError::Deserialize(source)); + assert!(matches!(mapped, ClientError::Deserialize(_))); + + let mapped = map_wire_error(WireError::UnexpectedMessage); + let ClientError::Deserialize(e) = mapped else { + panic!("every wire error reaches the caller as a deserialize failure"); + }; + assert!(e.to_string().contains("unexpected JSON-RPC message variant")); +} + #[test] fn merge_headers_overlays_jsonrpc_fields() { let mut base = HeaderMap::new(); From 76cce9fc03dd80e48e1bf13b6552ac8762f18058 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Fri, 14 Aug 2026 20:07:46 -0400 Subject: [PATCH 05/12] fix(a2a): give a streaming caller only frames it can correlate A client parses every SSE `data:` line as a JSON-RPC response, so a raw body forwarded there is a parse failure rather than the diagnostic it looks like, and a streaming request with no usable id has no reply anyone can read. Signed-off-by: Yordis Prieto --- .../crates/a2a/a2a-bridge/src/inbound.rs | 40 ++++++++++++------- .../a2a/a2a-bridge/src/inbound/tests.rs | 24 ++++++++++- 2 files changed, 48 insertions(+), 16 deletions(-) diff --git a/rsworkspace/crates/a2a/a2a-bridge/src/inbound.rs b/rsworkspace/crates/a2a/a2a-bridge/src/inbound.rs index 23e19fc106..689fbe7fbe 100644 --- a/rsworkspace/crates/a2a/a2a-bridge/src/inbound.rs +++ b/rsworkspace/crates/a2a/a2a-bridge/src/inbound.rs @@ -22,7 +22,7 @@ use futures_util::stream::{self, BoxStream, Stream}; use serde_json::{Value, json}; use tracing::warn; -use a2a_nats::client::rewrite_response_id; +use a2a_nats::RequestId; use a2a_nats::constants::{GATEWAY_CALLER_ID_HEADER, GATEWAY_CALLER_ID_HTTP, REQ_ID_HEADER}; use a2a_nats::jetstream::consumers::{PullConfig, resubscribe_consumer, stream_events_consumer}; use a2a_nats::jetstream::streams::events_stream_name; @@ -485,24 +485,30 @@ impl TaskJetStreamPort for ScriptedTaskJetstream { /// The id is restamped rather than forwarded, because the hops behind this edge /// correlate on their own transport id (`Trogon-Req-Id`, or a minted one when the /// caller sent no id at all). Only the caller's own id is meaningful to the -/// caller. A body that is not a JSON object goes out untouched: there is no -/// envelope to stamp, and swallowing it would lose the only diagnostic the caller -/// would get. +/// caller. A payload that is not a JSON object leaves as a correlated error +/// frame: a client parses every `data:` line as a JSON-RPC response, so raw +/// bytes there are a parse failure rather than the diagnostic they look like. fn sse_data_line(body: &[u8], caller_id: &Value) -> Event { - match rewrite_response_id(body, caller_id) { - Ok(stamped) => Event::default().data(String::from_utf8_lossy(stamped.as_ref())), - Err(e) => { - warn!(error = %e, "SSE frame is not a JSON-RPC envelope; forwarding without a caller id"); - Event::default().data(String::from_utf8_lossy(body)) - } - } + let Ok(Value::Object(mut envelope)) = serde_json::from_slice::(body) else { + warn!( + payload = %String::from_utf8_lossy(body), + "stream payload is not a JSON-RPC envelope" + ); + return sse_error_frame(caller_id, "stream payload is not a JSON-RPC envelope".to_owned()); + }; + envelope.insert("id".to_owned(), caller_id.clone()); + Event::default().data(Value::Object(envelope).to_string()) } fn sse_error_line(caller_id: &Value, err: &BridgeError) -> Event { + sse_error_frame(caller_id, err.to_string()) +} + +fn sse_error_frame(caller_id: &Value, message: String) -> Event { let envelope = serde_json::json!({ "jsonrpc": "2.0", "id": caller_id, - "error": { "code": INTERNAL_ERROR, "message": err.to_string() }, + "error": { "code": INTERNAL_ERROR, "message": message }, }); Event::default().data(envelope.to_string()) } @@ -732,6 +738,13 @@ pub async fn handle_jsonrpc(headers: HeaderMap, body: bytes::Bytes, state: &AppS let req_id = json_rpc_corr_id(&v); if is_sse_jsonrpc_method(method) { + // Every SSE frame repeats this id, so a streaming request that carries + // none has no reply the caller can correlate. Rejecting here keeps the + // gateway from doing the work of a stream nobody can read. + let stream_caller_id = v + .get("id") + .and_then(|id| serde_json::from_value::(id.clone()).ok()) + .ok_or(BridgeError::MissingJsonRpcId)?; // The unary publish comes BEFORE the JetStream consumer. Task event // subjects are scoped to the task (ADR#0055), and the bootstrap reply is // where the task id comes from. Nothing is lost in the gap, because @@ -761,8 +774,7 @@ pub async fn handle_jsonrpc(headers: HeaderMap, body: bytes::Bytes, state: &AppS } None => Box::pin(futures_util::stream::empty()), }; - let caller_id = v.get("id").cloned().unwrap_or(Value::Null); - let merged = sse_from_bootstrap_and_payloads(unary_reply.to_vec(), payloads, caller_id); + let merged = sse_from_bootstrap_and_payloads(unary_reply.to_vec(), payloads, stream_caller_id.to_json()); return Ok(Sse::new(merged).keep_alive(KeepAlive::default()).into_response()); } diff --git a/rsworkspace/crates/a2a/a2a-bridge/src/inbound/tests.rs b/rsworkspace/crates/a2a/a2a-bridge/src/inbound/tests.rs index bff0c61459..a8bb7ea463 100644 --- a/rsworkspace/crates/a2a/a2a-bridge/src/inbound/tests.rs +++ b/rsworkspace/crates/a2a/a2a-bridge/src/inbound/tests.rs @@ -298,6 +298,24 @@ async fn handle_jsonrpc_unary_publish_records_gateway_subject() { ); } +#[tokio::test] +async fn handle_jsonrpc_rejects_a_streaming_request_without_a_usable_id() { + for id in [json!(null), json!({})] { + let publisher = Arc::new(RecordingInboundPublisher::new()); + let state = test_state(publisher.clone()); + let body = + Bytes::from(json!({ "jsonrpc": "2.0", "id": id, "method": "message/stream", "params": {} }).to_string()); + let err = handle_jsonrpc(caller_headers("planner", None), body, &state) + .await + .unwrap_err(); + assert!(matches!(err, BridgeError::MissingJsonRpcId), "id {id} was accepted"); + assert!( + publisher.peek_subject().is_none(), + "the gateway must not open a stream nobody can correlate" + ); + } +} + #[test] fn json_rpc_corr_id_null_and_complex_ids_mint_fresh() { for body in [json!({"id": null}), json!({"id": []}), json!({"id": {}})] { @@ -387,7 +405,7 @@ async fn sse_frames(stream: BoxStream<'static, Result>) -> Ve } #[tokio::test] -async fn sse_frames_carry_the_caller_id_and_survive_a_body_that_is_not_an_envelope() { +async fn every_sse_frame_is_a_json_rpc_response_carrying_the_caller_id() { let bootstrap = serde_json::to_vec(&json!({"jsonrpc":"2.0","id":"transport-9","result":{"taskId":"t-1"}})).unwrap(); let tail = stream::iter(vec![ Ok(Bytes::from_static(b"not-an-envelope")), @@ -405,7 +423,9 @@ async fn sse_frames_carry_the_caller_id_and_survive_a_body_that_is_not_an_envelo assert_eq!(bootstrap["id"], "corr-1"); assert_eq!(bootstrap["result"]["taskId"], "t-1"); - assert_eq!(frames[1], "not-an-envelope"); + let undecodable: serde_json::Value = serde_json::from_str(&frames[1]).unwrap(); + assert_eq!(undecodable["id"], "corr-1"); + assert_eq!(undecodable["error"]["code"], INTERNAL_ERROR); let failure: serde_json::Value = serde_json::from_str(&frames[2]).unwrap(); assert_eq!(failure["id"], "corr-1"); From 923d2b36101daf7350a42a778c3d538ffcd15698 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Fri, 14 Aug 2026 20:07:51 -0400 Subject: [PATCH 06/12] fix(a2a): keep a JSON-RPC id's type in its correlation key A key built from the id's text alone lets the request `7` and the request "7" land on one stream pump and one audit row, which are two different callers reading each other's events. Signed-off-by: Yordis Prieto --- .../a2a/a2a-gateway/src/gw_ingress_stream.rs | 16 ++++++---------- .../a2a-gateway/src/gw_ingress_stream/tests.rs | 12 +++++++++++- .../crates/a2a/a2a-gateway/src/runtime/env.rs | 15 ++++----------- .../a2a/a2a-gateway/src/runtime/env/tests.rs | 9 ++++++++- rsworkspace/crates/a2a/a2a-nats/src/jsonrpc.rs | 11 +++++++++++ 5 files changed, 40 insertions(+), 23 deletions(-) diff --git a/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream.rs b/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream.rs index 888975d056..ede9219492 100644 --- a/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream.rs +++ b/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream.rs @@ -579,20 +579,16 @@ fn parse_last_event_id_as_u64(value: serde_json::Value) -> Option { } } -/// Correlation id for a streaming ingress envelope. A JSON-RPC `null` id -/// yields `None` for the same reason as [`crate::runtime::env::json_rpc_audit_req_id`]: -/// no caller correlation is possible, and a synthesized `"null"` token would -/// alias unrelated envelopes onto one stream pump. +/// Correlation id for a streaming ingress envelope. Falls back to +/// [`a2a_nats::jsonrpc::correlation_key_from_body`] so a payload-derived id +/// keeps its JSON type and a `null` or absent id spawns no pump at all. pub fn req_id_from_headers_or_payload(headers: &async_nats::HeaderMap, payload: &[u8]) -> Option { if let Some(value) = headers.get(a2a_nats::constants::REQ_ID_HEADER) { return Some(ReqId::from_header(value.as_str())); } - let text = match a2a_nats::jsonrpc::extract_request_id_from_body(payload)? { - a2a_nats::ResponseId::Null => return None, - a2a_nats::ResponseId::Number(n) => n.to_string(), - a2a_nats::ResponseId::String(s) => s, - }; - Some(ReqId::from_header(text)) + Some(ReqId::from_header(a2a_nats::jsonrpc::correlation_key_from_body( + payload, + )?)) } #[cfg(test)] diff --git a/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream/tests.rs b/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream/tests.rs index f550ac7d26..5863d032e8 100644 --- a/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream/tests.rs +++ b/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream/tests.rs @@ -196,7 +196,7 @@ fn req_id_from_payload_when_header_absent() { let headers = async_nats::HeaderMap::new(); let payload = br#"{"jsonrpc":"2.0","id":"req-from-payload","method":"x"}"#; let req_id = req_id_from_headers_or_payload(&headers, payload).expect("payload extract"); - assert_eq!(req_id.as_str(), "req-from-payload"); + assert_eq!(req_id.as_str(), r#""req-from-payload""#); } #[test] @@ -207,6 +207,16 @@ fn req_id_from_a_numeric_payload_id_uses_its_decimal_text() { assert_eq!(req_id.as_str(), "7"); } +#[test] +fn a_numeric_and_a_string_payload_id_pump_separate_streams() { + let headers = async_nats::HeaderMap::new(); + let numeric = + req_id_from_headers_or_payload(&headers, br#"{"jsonrpc":"2.0","id":7,"method":"x"}"#).expect("payload extract"); + let text = req_id_from_headers_or_payload(&headers, br#"{"jsonrpc":"2.0","id":"7","method":"x"}"#) + .expect("payload extract"); + assert_ne!(numeric.as_str(), text.as_str()); +} + #[test] fn req_id_is_none_for_a_null_json_rpc_id() { // A `"null"` string would collapse every null-id envelope onto one diff --git a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env.rs b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env.rs index 267b84b811..123a3f4697 100644 --- a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env.rs +++ b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env.rs @@ -138,18 +138,11 @@ pub fn json_rpc_params(payload: &[u8]) -> serde_json::Value { } } -/// Audit-side correlation id derived from the JSON-RPC request id. -/// Returns `None` when the payload doesn't carry an id (a -/// notification or malformed envelope) AND when the id is the -/// JSON-RPC `null` variant -- both shapes mean "no caller -/// correlation possible", so synthesizing a string would alias -/// unrelated envelopes onto the same audit row. +/// Audit-side correlation id derived from the JSON-RPC request id, sharing +/// [`a2a_nats::jsonrpc::correlation_key_from_body`] with the streaming pump so +/// an audit row and the stream it describes join on the same key. pub fn json_rpc_audit_req_id(payload: &[u8]) -> Option { - match a2a_nats::jsonrpc::extract_request_id_from_body(payload)? { - a2a_nats::ResponseId::Null => None, - a2a_nats::ResponseId::Number(n) => Some(n.to_string()), - a2a_nats::ResponseId::String(s) => Some(s), - } + a2a_nats::jsonrpc::correlation_key_from_body(payload) } fn parse_bool_flag(env: &E, key: &str) -> bool { diff --git a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env/tests.rs b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env/tests.rs index 23582d6b48..88a75fb58e 100644 --- a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env/tests.rs +++ b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env/tests.rs @@ -192,7 +192,7 @@ fn json_rpc_audit_req_id_returns_none_for_notification() { #[test] fn json_rpc_audit_req_id_returns_request_id_when_present() { let payload = br#"{"jsonrpc":"2.0","id":"req-42","method":"tasks/get","params":{}}"#; - assert_eq!(json_rpc_audit_req_id(payload).as_deref(), Some("req-42")); + assert_eq!(json_rpc_audit_req_id(payload).as_deref(), Some(r#""req-42""#)); } #[test] @@ -200,3 +200,10 @@ fn json_rpc_audit_req_id_renders_a_numeric_id_as_its_decimal_text() { let payload = br#"{"jsonrpc":"2.0","id":42,"method":"tasks/get","params":{}}"#; assert_eq!(json_rpc_audit_req_id(payload).as_deref(), Some("42")); } + +#[test] +fn json_rpc_audit_req_id_keeps_a_numeric_and_a_string_id_on_separate_rows() { + let numeric = json_rpc_audit_req_id(br#"{"jsonrpc":"2.0","id":7,"method":"tasks/get"}"#); + let text = json_rpc_audit_req_id(br#"{"jsonrpc":"2.0","id":"7","method":"tasks/get"}"#); + assert_ne!(numeric, text); +} diff --git a/rsworkspace/crates/a2a/a2a-nats/src/jsonrpc.rs b/rsworkspace/crates/a2a/a2a-nats/src/jsonrpc.rs index 04af36e258..a0dad6237b 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/jsonrpc.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/jsonrpc.rs @@ -31,5 +31,16 @@ pub fn extract_request_id_from_body(raw: &[u8]) -> Option { } } +/// Canonical correlation key for the id in a request body: the id's JSON +/// literal, the same form the `Jsonrpc-Id` header carries. +/// +/// The literal keeps the id's type in the key, so a numeric `7` and a string +/// `"7"` cannot collapse onto one stream pump or one audit row. A missing id +/// and a `null` id both yield `None`: neither can correlate anything, and a +/// synthesized token would alias unrelated envelopes together. +pub fn correlation_key_from_body(raw: &[u8]) -> Option { + jsonrpc_nats::encode_response_id_literal(&extract_request_id_from_body(raw)?) +} + #[cfg(test)] mod tests; From 148df0d549a7bd4a716b809f7639b73c8c56b8d7 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Fri, 14 Aug 2026 20:07:54 -0400 Subject: [PATCH 07/12] fix(a2a): let a wire error keep its own identity at the client edge A peer that breaks the JSON-RPC framing and a caller whose result type did not match are different failures with different owners, and a client that receives both as one variant cannot tell which one it is looking at. Signed-off-by: Yordis Prieto --- .../crates/a2a/a2a-nats/src/client/error.rs | 9 ++++++ .../a2a-nats/src/client/event_stream/tests.rs | 4 +-- .../a2a/a2a-nats/src/client/streaming.rs | 17 ++++++----- .../crates/a2a/a2a-nats/src/client/unary.rs | 4 +-- .../a2a/a2a-nats/src/client/unary/tests.rs | 4 +-- .../crates/a2a/a2a-nats/src/client/wire.rs | 9 +++++- .../a2a/a2a-nats/src/client/wire/tests.rs | 28 +++++++++++++------ 7 files changed, 51 insertions(+), 24 deletions(-) diff --git a/rsworkspace/crates/a2a/a2a-nats/src/client/error.rs b/rsworkspace/crates/a2a/a2a-nats/src/client/error.rs index f9b747bb0c..a4760ffab6 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/client/error.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/client/error.rs @@ -10,6 +10,15 @@ pub enum ClientError { Serialize(#[source] serde_json::Error), #[error("failed to deserialize response: {0}")] Deserialize(#[source] serde_json::Error), + /// The body violated the JSON-RPC envelope grammar itself. Distinct from + /// [`Self::Deserialize`], which is a well-formed envelope whose payload did + /// not match the caller's type: only this one indicts the peer's framing. + #[error("JSON-RPC codec error: {0}")] + Codec(#[source] jsonrpc_nats::CodecError), + /// A valid envelope of the wrong kind for this position, e.g. a request + /// where a response was expected. + #[error("unexpected JSON-RPC message variant")] + UnexpectedMessage, #[error("transport error: {0}")] Transport(String), #[error("request to '{subject}' timed out")] diff --git a/rsworkspace/crates/a2a/a2a-nats/src/client/event_stream/tests.rs b/rsworkspace/crates/a2a/a2a-nats/src/client/event_stream/tests.rs index 3590a39b17..b056964c22 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/client/event_stream/tests.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/client/event_stream/tests.rs @@ -145,7 +145,7 @@ async fn stream_yields_error_on_bad_payload() { drop(tx); let item = stream.next().await; - assert!(matches!(item, Some(Err(ClientError::Deserialize(_))))); + assert!(matches!(item, Some(Err(ClientError::Codec(_))))); } #[tokio::test] @@ -159,7 +159,7 @@ async fn stream_yields_error_on_bare_result_payload() { .unwrap(); drop(tx); - assert!(matches!(stream.next().await, Some(Err(ClientError::Deserialize(_))))); + assert!(matches!(stream.next().await, Some(Err(ClientError::Codec(_))))); } #[tokio::test] diff --git a/rsworkspace/crates/a2a/a2a-nats/src/client/streaming.rs b/rsworkspace/crates/a2a/a2a-nats/src/client/streaming.rs index f80f92d45c..27decb45c2 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/client/streaming.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/client/streaming.rs @@ -20,7 +20,7 @@ use super::error::ClientError; use super::event_stream::{TypedEventStream, build_event_stream, empty_event_stream}; use super::gateway_headers::{agent_rpc_headers, gateway_ingress_rpc_headers}; use super::validated::ValidatedRpc; -use super::wire::{decode_client_response, encode_client_request, merge_jsonrpc_headers}; +use super::wire::{decode_client_response, encode_client_request, map_wire_error, merge_jsonrpc_headers}; pub struct StreamingRequest<'a, N, J> { pub nats: &'a N, @@ -77,8 +77,8 @@ where op_timeout, gateway_caller_jwt, } = ctx; - let encoded = encode_client_request(method, RequestId::String(req_id.as_str().to_owned()), params) - .map_err(|e| ClientError::Serialize(::custom(format!("{e}"))))?; + let encoded = + encode_client_request(method, RequestId::String(req_id.as_str().to_owned()), params).map_err(map_wire_error)?; let headers = match gateway_caller_jwt { Some(jwt) => gateway_ingress_rpc_headers(req_id, jwt)?, @@ -98,12 +98,11 @@ where let response_headers = msg.headers.unwrap_or_default(); let body = msg.payload.clone(); - let result = match decode_client_response::(&response_headers, &body) - .map_err(|e| ClientError::Deserialize(::custom(format!("{e}"))))? - { - Ok(result) => result, - Err((code, message)) => return Err(ClientError::from_jsonrpc_code(code, message)), - }; + let result = + match decode_client_response::(&response_headers, &body).map_err(map_wire_error)? { + Ok(result) => result, + Err((code, message)) => return Err(ClientError::from_jsonrpc_code(code, message)), + }; // The consumer is opened after the reply, not before it: task event subjects // are scoped to the task (ADR#0055), and the bootstrap reply is where the task diff --git a/rsworkspace/crates/a2a/a2a-nats/src/client/unary.rs b/rsworkspace/crates/a2a/a2a-nats/src/client/unary.rs index cc78b3d113..e5b7ad3dc5 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/client/unary.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/client/unary.rs @@ -50,8 +50,8 @@ where Req: Serialize, Res: DeserializeOwned, { - let encoded = encode_client_request(method, RequestId::String(req_id.as_str().to_owned()), params) - .map_err(|e| ClientError::Serialize(::custom(format!("{e}"))))?; + let encoded = + encode_client_request(method, RequestId::String(req_id.as_str().to_owned()), params).map_err(map_wire_error)?; let headers = match gateway_caller_jwt { Some(jwt) => gateway_ingress_rpc_headers(req_id, jwt)?, diff --git a/rsworkspace/crates/a2a/a2a-nats/src/client/unary/tests.rs b/rsworkspace/crates/a2a/a2a-nats/src/client/unary/tests.rs index 25e6f0b9ce..39e3f0eaa7 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/client/unary/tests.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/client/unary/tests.rs @@ -142,7 +142,7 @@ async fn hang_returns_timeout_error() { } #[tokio::test] -async fn malformed_response_returns_deserialize_error() { +async fn malformed_response_returns_a_codec_error() { let mock = AdvancedMockNatsClient::new(); mock.set_response_wire( "a2a.v1.agents.bot.tasks.get", @@ -161,7 +161,7 @@ async fn malformed_response_returns_deserialize_error() { ) .await; - assert!(matches!(result, Err(ClientError::Deserialize(_)))); + assert!(matches!(result, Err(ClientError::Codec(_)))); } #[tokio::test] diff --git a/rsworkspace/crates/a2a/a2a-nats/src/client/wire.rs b/rsworkspace/crates/a2a/a2a-nats/src/client/wire.rs index 9e04569562..ff5cdd7993 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/client/wire.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/client/wire.rs @@ -19,10 +19,17 @@ pub fn decode_client_response( decode_response(headers, body) } +/// The single wire-to-client error mapping. Every variant keeps its own +/// identity: flattening `Codec` and `UnexpectedMessage` into a synthesized +/// `Deserialize` would report a peer that broke the JSON-RPC framing as a +/// caller whose result type did not match, and callers cannot tell those apart +/// from a string. pub fn map_wire_error(error: WireError) -> ClientError { match error { + WireError::Serialize(e) => ClientError::Serialize(e), WireError::Deserialize(e) => ClientError::Deserialize(e), - other => ClientError::Deserialize(::custom(format!("{other}"))), + WireError::Codec(e) => ClientError::Codec(e), + WireError::UnexpectedMessage => ClientError::UnexpectedMessage, } } diff --git a/rsworkspace/crates/a2a/a2a-nats/src/client/wire/tests.rs b/rsworkspace/crates/a2a/a2a-nats/src/client/wire/tests.rs index 3b5290415b..c9fc382a84 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/client/wire/tests.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/client/wire/tests.rs @@ -66,16 +66,28 @@ fn roundtrip_reconstructs_canonical_json_at_edge() { } #[test] -fn map_wire_error_keeps_the_deserialize_source_and_flattens_the_rest() { +fn map_wire_error_carries_every_variant_across_without_flattening() { let source = serde_json::from_str::("{}").unwrap_err(); - let mapped = map_wire_error(WireError::Deserialize(source)); - assert!(matches!(mapped, ClientError::Deserialize(_))); + assert!(matches!( + map_wire_error(WireError::Deserialize(source)), + ClientError::Deserialize(_) + )); - let mapped = map_wire_error(WireError::UnexpectedMessage); - let ClientError::Deserialize(e) = mapped else { - panic!("every wire error reaches the caller as a deserialize failure"); - }; - assert!(e.to_string().contains("unexpected JSON-RPC message variant")); + let source = serde_json::from_str::("{}").unwrap_err(); + assert!(matches!( + map_wire_error(WireError::Serialize(source)), + ClientError::Serialize(_) + )); + + assert!(matches!( + map_wire_error(WireError::Codec(jsonrpc_nats::CodecError::RequestWithoutId)), + ClientError::Codec(_) + )); + + assert!(matches!( + map_wire_error(WireError::UnexpectedMessage), + ClientError::UnexpectedMessage + )); } #[test] From ffb455fa94045848a6e2aacc89e0f5ca133e150b Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Fri, 14 Aug 2026 20:07:58 -0400 Subject: [PATCH 08/12] chore(acp): name the stream an upgrade leaves behind Dropping a stream from the registry stops the provisioner from creating it, but a deployment that ran an earlier release still carries it, its messages, and its storage cost, with nothing to tell an operator it is now orphaned. Signed-off-by: Yordis Prieto --- docs/.vitepress/config.mts | 4 + .../how-to/retire-acp-notifications-stream.md | 80 +++++++++++++++++++ .../acp-nats/src/jetstream/streams/tests.rs | 25 ++++++ .../crates/acp/acp-nats/src/nats/mod.rs | 4 +- .../acp/acp-nats/src/nats/subjects/mod.rs | 2 +- .../acp/acp-nats/src/nats/subjects/stream.rs | 20 +++++ 6 files changed, 133 insertions(+), 2 deletions(-) create mode 100644 docs/how-to/retire-acp-notifications-stream.md diff --git a/docs/.vitepress/config.mts b/docs/.vitepress/config.mts index bfa6acad10..3bed66bd05 100644 --- a/docs/.vitepress/config.mts +++ b/docs/.vitepress/config.mts @@ -62,6 +62,10 @@ export default async () => { { text: "Bring Your Own OpenBao", link: "/how-to/bring-your-own-openbao" }, { text: "Migrate Between Key Backends", link: "/how-to/migrate-key-backends" }, { text: "Handle Unusable Keys", link: "/how-to/handle-unusable-keys" }, + { + text: "Retire the ACP Notifications Stream", + link: "/how-to/retire-acp-notifications-stream", + }, ], }, { diff --git a/docs/how-to/retire-acp-notifications-stream.md b/docs/how-to/retire-acp-notifications-stream.md new file mode 100644 index 0000000000..926bbbcef4 --- /dev/null +++ b/docs/how-to/retire-acp-notifications-stream.md @@ -0,0 +1,80 @@ +# Retire the ACP Notifications Stream + +`session/update` reaches the client through the client-op proxy for every +operation. The prompt-scoped notification path that used the +`_NOTIFICATIONS` stream was a second delivery path for the same +updates, so it was removed, and the ACP provisioner no longer creates that +stream. + +Removing a stream from the provisioner does not remove it from a deployment +that already ran an earlier release. That deployment still has the stream, its +stored messages, and its storage bill. This page is how you retire it. + +## When to use this + +Use this procedure once per ACP deployment, after every process has been +upgraded to a release whose provisioner no longer lists the stream. Confirm +that with: + +``` +nats stream ls +``` + +You are retiring a stream whose name matches `retired_stream_names` for your +prefix. For the default `acp` prefix that is `ACP_NOTIFICATIONS`; for a prefix +of `my.multi.part` it is `MY_MULTI_PART_NOTIFICATIONS`. + +Do not use this procedure while any process is still running the previous +release. Those processes publish to `.v1.session.*.agent.update`, and +deleting the stream underneath them drops those messages. + +## Preconditions + +- Every ACP agent and client process runs a release that does not provision + the stream. A mixed fleet is the one case where this procedure loses data. +- The stream has no consumers with unacknowledged messages you still need. + `nats consumer ls _NOTIFICATIONS` lists them; an empty list is the + state you want before deleting. +- You have a JetStream account credential with delete authority on the + stream. + +## Steps + +1. Confirm the stream is idle. Its message count should stop growing: + + ``` + nats stream info _NOTIFICATIONS + ``` + + A message count that still climbs means something is publishing to + `.v1.session.*.agent.update`. Find it and upgrade it before + continuing. + +2. Delete the remaining consumers. Deleting the stream removes them anyway, + but doing it first makes a still-attached reader fail visibly here rather + than silently later: + + ``` + nats consumer rm _NOTIFICATIONS + ``` + +3. Delete the stream: + + ``` + nats stream rm _NOTIFICATIONS + ``` + +## What this does not do + +The provisioner does not perform any of the above. It creates and reconciles +the streams it declares and touches nothing else, so a stream delete is never +a side effect of a boot. That is deliberate: a stream delete is +unrecoverable, it races an operator who may still be draining the stream, and +a rollback to the previous release would re-create the stream empty and hide +the fact that its history is gone. + +## Rollback + +There is none. A deleted stream and its messages do not come back. If you +roll back to a release that still provisions the stream, the provisioner +creates it again with no history. diff --git a/rsworkspace/crates/acp/acp-nats/src/jetstream/streams/tests.rs b/rsworkspace/crates/acp/acp-nats/src/jetstream/streams/tests.rs index 1f50004aca..9b1bddbd5f 100644 --- a/rsworkspace/crates/acp/acp-nats/src/jetstream/streams/tests.rs +++ b/rsworkspace/crates/acp/acp-nats/src/jetstream/streams/tests.rs @@ -218,3 +218,28 @@ fn no_subject_overlaps_between_streams() { } } } + +#[test] +fn retired_stream_names_follow_the_same_naming_as_provisioned_ones() { + use crate::nats::retired_stream_names; + + assert_eq!(retired_stream_names(&p("acp")), vec!["ACP_NOTIFICATIONS".to_owned()]); + assert_eq!( + retired_stream_names(&p("my.multi.part")), + vec!["MY_MULTI_PART_NOTIFICATIONS".to_owned()] + ); +} + +#[test] +fn no_retired_stream_is_still_provisioned() { + use crate::nats::retired_stream_names; + + let prefix = p("acp"); + let live: Vec = AcpStream::ALL.iter().map(|s| s.stream_name(&prefix)).collect(); + for retired in retired_stream_names(&prefix) { + assert!( + !live.contains(&retired), + "{retired} is listed as retired but the provisioner still creates it" + ); + } +} diff --git a/rsworkspace/crates/acp/acp-nats/src/nats/mod.rs b/rsworkspace/crates/acp/acp-nats/src/nats/mod.rs index dad5227c6a..ddfe97671f 100644 --- a/rsworkspace/crates/acp/acp-nats/src/nats/mod.rs +++ b/rsworkspace/crates/acp/acp-nats/src/nats/mod.rs @@ -12,7 +12,9 @@ pub use parsing::{ ClientMethod, GlobalAgentMethod, ParsedAgentSubject, ParsedClientSubject, SessionAgentMethod, parse_agent_subject, parse_client_subject, }; -pub use subjects::{AcpStream, StreamAssignment, client_ops, commands, global, markers, responses, subscriptions}; +pub use subjects::{ + AcpStream, StreamAssignment, client_ops, commands, global, markers, responses, retired_stream_names, subscriptions, +}; pub use trogon_nats::{ FlushClient, FlushPolicy, NatsError, PublishClient, PublishOptions, RequestClient, RetryPolicy, SubscribeClient, client, connect, headers_with_trace_context, inject_trace_context, diff --git a/rsworkspace/crates/acp/acp-nats/src/nats/subjects/mod.rs b/rsworkspace/crates/acp/acp-nats/src/nats/subjects/mod.rs index 4afb5fa27c..f40b698efe 100644 --- a/rsworkspace/crates/acp/acp-nats/src/nats/subjects/mod.rs +++ b/rsworkspace/crates/acp/acp-nats/src/nats/subjects/mod.rs @@ -6,7 +6,7 @@ pub mod responses; pub mod stream; pub mod subscriptions; -pub use stream::{AcpStream, StreamAssignment}; +pub use stream::{AcpStream, StreamAssignment, retired_stream_names}; #[cfg(test)] mod conformance_tests; diff --git a/rsworkspace/crates/acp/acp-nats/src/nats/subjects/stream.rs b/rsworkspace/crates/acp/acp-nats/src/nats/subjects/stream.rs index fd66d56ae7..7954faa673 100644 --- a/rsworkspace/crates/acp/acp-nats/src/nats/subjects/stream.rs +++ b/rsworkspace/crates/acp/acp-nats/src/nats/subjects/stream.rs @@ -83,6 +83,26 @@ impl AcpStream { } } +/// Suffixes this crate used to provision and no longer does. +/// +/// Dropping a variant from [`AcpStream`] stops the provisioner from creating +/// the stream, but a deployment that ran an earlier release still has it, +/// still has its stored messages, and still bills for its storage. Nothing +/// here deletes anything: a stream delete is unrecoverable, it races an +/// operator who may still be draining the old stream, and a rollback to the +/// prior release would silently re-create it empty. Retirement is an operator +/// step, and this list is what names the streams that step applies to. +/// +/// See `docs/how-to/retire-acp-notifications-stream.md`. +const RETIRED_SUFFIXES: [&str; 1] = ["NOTIFICATIONS"]; + +/// Stream names an upgraded deployment may still be carrying, for the prefix +/// it was provisioned under. +pub fn retired_stream_names(prefix: &AcpPrefix) -> Vec { + let root = prefix.as_str().to_uppercase().replace('.', "_"); + RETIRED_SUFFIXES.iter().map(|s| format!("{root}_{s}")).collect() +} + impl std::fmt::Display for AcpStream { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.write_str(self.suffix()) From 3354986e4d58cdecb3e909dbf3cb3937e9de77a1 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Fri, 14 Aug 2026 21:03:55 -0400 Subject: [PATCH 09/12] fix(a2a): correlate on the id text the rest of the transport writes The bridge and the agent both stamp a string id as its bare text, so a key in any other form joins no audit row and matches no event. Signed-off-by: Yordis Prieto --- .../a2a/a2a-gateway/src/gw_ingress_stream.rs | 5 ++-- .../src/gw_ingress_stream/tests.rs | 16 ++++++++----- .../crates/a2a/a2a-gateway/src/runtime/env.rs | 3 ++- .../a2a/a2a-gateway/src/runtime/env/tests.rs | 12 ++++++---- .../crates/a2a/a2a-nats/src/jsonrpc.rs | 23 +++++++++++++------ 5 files changed, 38 insertions(+), 21 deletions(-) diff --git a/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream.rs b/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream.rs index ede9219492..11febbba2b 100644 --- a/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream.rs +++ b/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream.rs @@ -580,8 +580,9 @@ fn parse_last_event_id_as_u64(value: serde_json::Value) -> Option { } /// Correlation id for a streaming ingress envelope. Falls back to -/// [`a2a_nats::jsonrpc::correlation_key_from_body`] so a payload-derived id -/// keeps its JSON type and a `null` or absent id spawns no pump at all. +/// [`a2a_nats::jsonrpc::correlation_key_from_body`], which yields the same text +/// the agent stamps its task events with, so the pump's filter can match; a +/// `null` or absent id spawns no pump at all. pub fn req_id_from_headers_or_payload(headers: &async_nats::HeaderMap, payload: &[u8]) -> Option { if let Some(value) = headers.get(a2a_nats::constants::REQ_ID_HEADER) { return Some(ReqId::from_header(value.as_str())); diff --git a/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream/tests.rs b/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream/tests.rs index 5863d032e8..526568df3d 100644 --- a/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream/tests.rs +++ b/rsworkspace/crates/a2a/a2a-gateway/src/gw_ingress_stream/tests.rs @@ -196,7 +196,7 @@ fn req_id_from_payload_when_header_absent() { let headers = async_nats::HeaderMap::new(); let payload = br#"{"jsonrpc":"2.0","id":"req-from-payload","method":"x"}"#; let req_id = req_id_from_headers_or_payload(&headers, payload).expect("payload extract"); - assert_eq!(req_id.as_str(), r#""req-from-payload""#); + assert_eq!(req_id.as_str(), "req-from-payload"); } #[test] @@ -208,13 +208,17 @@ fn req_id_from_a_numeric_payload_id_uses_its_decimal_text() { } #[test] -fn a_numeric_and_a_string_payload_id_pump_separate_streams() { +fn a_payload_derived_req_id_can_still_match_an_event_the_agent_stamped() { + // The agent stamps task events with the bare string id, so a pump that + // derived its filter from the body has to produce that same text or it + // discards every event as belonging to another request. let headers = async_nats::HeaderMap::new(); - let numeric = - req_id_from_headers_or_payload(&headers, br#"{"jsonrpc":"2.0","id":7,"method":"x"}"#).expect("payload extract"); - let text = req_id_from_headers_or_payload(&headers, br#"{"jsonrpc":"2.0","id":"7","method":"x"}"#) + let req_id = req_id_from_headers_or_payload(&headers, br#"{"jsonrpc":"2.0","id":"corr-1","method":"x"}"#) .expect("payload extract"); - assert_ne!(numeric.as_str(), text.as_str()); + + let mut event = async_nats::HeaderMap::new(); + event.insert(a2a_nats::constants::REQ_ID_HEADER, "corr-1"); + assert!(req_id.matches_event_headers(Some(&event))); } #[test] diff --git a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env.rs b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env.rs index 123a3f4697..862f268075 100644 --- a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env.rs +++ b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env.rs @@ -140,7 +140,8 @@ pub fn json_rpc_params(payload: &[u8]) -> serde_json::Value { /// Audit-side correlation id derived from the JSON-RPC request id, sharing /// [`a2a_nats::jsonrpc::correlation_key_from_body`] with the streaming pump so -/// an audit row and the stream it describes join on the same key. +/// an audit row and the stream it describes join on the same key, and so both +/// agree with the `Trogon-Req-Id` the bridge minted for the same request. pub fn json_rpc_audit_req_id(payload: &[u8]) -> Option { a2a_nats::jsonrpc::correlation_key_from_body(payload) } diff --git a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env/tests.rs b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env/tests.rs index 88a75fb58e..c9dba5963f 100644 --- a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env/tests.rs +++ b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env/tests.rs @@ -192,7 +192,7 @@ fn json_rpc_audit_req_id_returns_none_for_notification() { #[test] fn json_rpc_audit_req_id_returns_request_id_when_present() { let payload = br#"{"jsonrpc":"2.0","id":"req-42","method":"tasks/get","params":{}}"#; - assert_eq!(json_rpc_audit_req_id(payload).as_deref(), Some(r#""req-42""#)); + assert_eq!(json_rpc_audit_req_id(payload).as_deref(), Some("req-42")); } #[test] @@ -202,8 +202,10 @@ fn json_rpc_audit_req_id_renders_a_numeric_id_as_its_decimal_text() { } #[test] -fn json_rpc_audit_req_id_keeps_a_numeric_and_a_string_id_on_separate_rows() { - let numeric = json_rpc_audit_req_id(br#"{"jsonrpc":"2.0","id":7,"method":"tasks/get"}"#); - let text = json_rpc_audit_req_id(br#"{"jsonrpc":"2.0","id":"7","method":"tasks/get"}"#); - assert_ne!(numeric, text); +fn json_rpc_audit_req_id_matches_the_req_id_header_the_bridge_would_mint() { + // The audit row joins the request through `Trogon-Req-Id`, and the bridge + // mints that from a string id as its bare text. Quoting it here would + // produce a row that joins nothing. + let payload = br#"{"jsonrpc":"2.0","id":"corr-1","method":"tasks/get","params":{}}"#; + assert_eq!(json_rpc_audit_req_id(payload).as_deref(), Some("corr-1")); } diff --git a/rsworkspace/crates/a2a/a2a-nats/src/jsonrpc.rs b/rsworkspace/crates/a2a/a2a-nats/src/jsonrpc.rs index a0dad6237b..d6a43c98b1 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/jsonrpc.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/jsonrpc.rs @@ -31,15 +31,24 @@ pub fn extract_request_id_from_body(raw: &[u8]) -> Option { } } -/// Canonical correlation key for the id in a request body: the id's JSON -/// literal, the same form the `Jsonrpc-Id` header carries. +/// Correlation key for the id in a request body, in the one form the rest of +/// the transport already agrees on: the id's text, unquoted. /// -/// The literal keeps the id's type in the key, so a numeric `7` and a string -/// `"7"` cannot collapse onto one stream pump or one audit row. A missing id -/// and a `null` id both yield `None`: neither can correlate anything, and a -/// synthesized token would alias unrelated envelopes together. +/// This is deliberately *not* the `Jsonrpc-Id` literal. `Trogon-Req-Id` is what +/// a stream pump filters on and what an audit row joins against, and the two +/// places that mint it (the bridge's caller-id derivation and the agent's event +/// stamp) both write a string id as its bare text. A key that quoted the id +/// would match neither, so every event would look like another request's and +/// every audit row would join nothing. +/// +/// A missing id and a `null` id both yield `None`: neither can correlate +/// anything, and a synthesized token would alias unrelated envelopes together. pub fn correlation_key_from_body(raw: &[u8]) -> Option { - jsonrpc_nats::encode_response_id_literal(&extract_request_id_from_body(raw)?) + match extract_request_id_from_body(raw)? { + ResponseId::Null => None, + ResponseId::Number(n) => Some(n.to_string()), + ResponseId::String(s) => Some(s), + } } #[cfg(test)] From 0b49b16e55c2d94c42863789ca32f4f784ac4df2 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Fri, 14 Aug 2026 22:09:43 -0400 Subject: [PATCH 10/12] fix(a2a): keep reading task events an older agent published The events stream retains by limits and a rolling upgrade runs both agent releases against it, so refusing the pre-envelope shape would end a stream mid-task on every deploy and every replay. Signed-off-by: Yordis Prieto --- .../crates/a2a/a2a-bridge/src/inbound.rs | 8 +++ .../a2a/a2a-bridge/src/inbound/tests.rs | 31 +++++++++ .../a2a/a2a-nats/src/client/event_stream.rs | 6 +- .../a2a-nats/src/client/event_stream/tests.rs | 8 ++- rsworkspace/crates/a2a/a2a-nats/src/lib.rs | 1 + .../crates/a2a/a2a-nats/src/task_event.rs | 38 +++++++++++ .../a2a/a2a-nats/src/task_event/tests.rs | 66 +++++++++++++++++++ 7 files changed, 155 insertions(+), 3 deletions(-) create mode 100644 rsworkspace/crates/a2a/a2a-nats/src/task_event.rs create mode 100644 rsworkspace/crates/a2a/a2a-nats/src/task_event/tests.rs diff --git a/rsworkspace/crates/a2a/a2a-bridge/src/inbound.rs b/rsworkspace/crates/a2a/a2a-bridge/src/inbound.rs index 689fbe7fbe..de5c129485 100644 --- a/rsworkspace/crates/a2a/a2a-bridge/src/inbound.rs +++ b/rsworkspace/crates/a2a/a2a-bridge/src/inbound.rs @@ -488,7 +488,15 @@ impl TaskJetStreamPort for ScriptedTaskJetstream { /// caller. A payload that is not a JSON object leaves as a correlated error /// frame: a client parses every `data:` line as a JSON-RPC response, so raw /// bytes there are a parse failure rather than the diagnostic they look like. +/// +/// An event an older agent published carries no envelope of its own, so it is +/// given one here instead of being restamped: forwarding it as-is would put a +/// `data:` line on the wire that no spec client can parse as a response. fn sse_data_line(body: &[u8], caller_id: &Value) -> Event { + if let Some(response) = a2a_nats::task_event::legacy_event_as_response(body, caller_id) { + return Event::default().data(response.to_string()); + } + let Ok(Value::Object(mut envelope)) = serde_json::from_slice::(body) else { warn!( payload = %String::from_utf8_lossy(body), diff --git a/rsworkspace/crates/a2a/a2a-bridge/src/inbound/tests.rs b/rsworkspace/crates/a2a/a2a-bridge/src/inbound/tests.rs index a8bb7ea463..5931b98056 100644 --- a/rsworkspace/crates/a2a/a2a-bridge/src/inbound/tests.rs +++ b/rsworkspace/crates/a2a/a2a-bridge/src/inbound/tests.rs @@ -435,3 +435,34 @@ async fn every_sse_frame_is_a_json_rpc_response_carrying_the_caller_id() { "the caller keeps the only diagnostic the stream produced: {failure}" ); } + +#[tokio::test] +async fn an_event_an_older_agent_published_reaches_the_caller_as_a_response() { + // The events stream retains by limits and a rolling upgrade runs both agent + // releases at once, so this edge still meets bare events. Stamping an id onto + // one and forwarding it would emit a `data:` line that is no JSON-RPC response. + let legacy = serde_json::to_vec(&a2a::event::StreamResponse::StatusUpdate( + a2a::event::TaskStatusUpdateEvent { + task_id: "t-1".to_owned(), + context_id: "ctx".to_owned(), + status: a2a::types::TaskStatus { + state: a2a::types::TaskState::Working, + message: None, + timestamp: None, + }, + metadata: None, + }, + )) + .unwrap(); + let frames = sse_frames(sse_from_bootstrap_and_payloads( + serde_json::to_vec(&json!({"jsonrpc":"2.0","id":"transport-9","result":{"taskId":"t-1"}})).unwrap(), + Box::pin(stream::iter(vec![Ok(Bytes::from(legacy))])), + json!("corr-1"), + )) + .await; + + let event: serde_json::Value = serde_json::from_str(&frames[1]).unwrap(); + assert_eq!(event["jsonrpc"], "2.0"); + assert_eq!(event["id"], "corr-1"); + assert_eq!(event["result"]["statusUpdate"]["taskId"], "t-1"); +} diff --git a/rsworkspace/crates/a2a/a2a-nats/src/client/event_stream.rs b/rsworkspace/crates/a2a/a2a-nats/src/client/event_stream.rs index 34c7e415bd..bb5d8449bf 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/client/event_stream.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/client/event_stream.rs @@ -154,11 +154,15 @@ async fn pull_loop( /// Each task event on the wire is a full JSON-RPC success response repeating the /// request id, which is the shape the A2A spec puts on `message/stream` and /// `tasks/resubscribe`. The result member is the `StreamResponse` the reader wants. +/// +/// A body that is not an envelope gets one more chance as a +/// [`crate::task_event`] an older agent published, because refusing those would +/// end a stream mid-task on every replay and every rolling upgrade. fn decode_event(headers: &async_nats::header::HeaderMap, payload: &[u8]) -> Result { match decode_response::(headers, payload) { Ok(Ok(event)) => Ok(event), Ok(Err((code, message))) => Err(ClientError::from_jsonrpc_code(code, message)), - Err(e) => Err(map_wire_error(e)), + Err(e) => crate::task_event::decode_legacy_event(payload).ok_or_else(|| map_wire_error(e)), } } diff --git a/rsworkspace/crates/a2a/a2a-nats/src/client/event_stream/tests.rs b/rsworkspace/crates/a2a/a2a-nats/src/client/event_stream/tests.rs index b056964c22..31580c1efb 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/client/event_stream/tests.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/client/event_stream/tests.rs @@ -149,7 +149,10 @@ async fn stream_yields_error_on_bad_payload() { } #[tokio::test] -async fn stream_yields_error_on_bare_result_payload() { +async fn stream_still_reads_an_event_an_older_agent_published() { + // `A2A_EVENTS` retains by limits and a rolling upgrade runs both agent + // releases at once, so a bare `StreamResponse` is a body this reader meets + // rather than a malformed one. Refusing it would end the stream mid-task. let (consumer, tx) = MockJetStreamConsumer::new(); let last_seq = Arc::new(Mutex::new(0u64)); let mut stream = build_event_stream(consumer, last_seq, None); @@ -159,7 +162,8 @@ async fn stream_yields_error_on_bare_result_payload() { .unwrap(); drop(tx); - assert!(matches!(stream.next().await, Some(Err(ClientError::Codec(_))))); + let event = stream.next().await.expect("a pre-envelope event").unwrap(); + assert_eq!(event, make_status_event("task-1")); } #[tokio::test] diff --git a/rsworkspace/crates/a2a/a2a-nats/src/lib.rs b/rsworkspace/crates/a2a/a2a-nats/src/lib.rs index 303225b351..e625abb99e 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/lib.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/lib.rs @@ -22,6 +22,7 @@ pub mod nats; pub mod push; pub mod req_id; pub mod server; +pub mod task_event; pub mod task_id; pub mod wire; diff --git a/rsworkspace/crates/a2a/a2a-nats/src/task_event.rs b/rsworkspace/crates/a2a/a2a-nats/src/task_event.rs new file mode 100644 index 0000000000..f3f0a7c500 --- /dev/null +++ b/rsworkspace/crates/a2a/a2a-nats/src/task_event.rs @@ -0,0 +1,38 @@ +//! Reading task events an older release wrote. +//! +//! Task events on `A2A_EVENTS` are JSON-RPC success responses. They used to be +//! bare [`StreamResponse`] bodies, and a reader on this release still meets those: +//! the stream retains by limits for a whole `max_age` window, so `tasks/resubscribe` +//! replays them, and a rolling upgrade runs both agent releases at once, so a live +//! subscription sees them too. Refusing them would end a stream mid-task on every +//! deploy. +//! +//! This module can go once no deployment can still hold pre-envelope events: one +//! retention window after the last agent is upgraded. + +use a2a::event::StreamResponse; +use serde_json::Value; + +/// Decodes a task event that predates the JSON-RPC envelope, or `None` if the body +/// is not one. +/// +/// The decision is a full parse rather than a shape guess, so a body that is merely +/// malformed stays the decode error it is instead of arriving as a plausible event. +/// A current envelope carries none of the four variant keys, so it never lands here. +pub fn decode_legacy_event(body: &[u8]) -> Option { + serde_json::from_slice::(body).ok() +} + +/// The same event lifted into the envelope this release's readers expect, for the +/// hops that forward event bytes without typing them. +pub fn legacy_event_as_response(body: &[u8], id: &Value) -> Option { + let event = decode_legacy_event(body)?; + Some(serde_json::json!({ + "jsonrpc": "2.0", + "id": id, + "result": event, + })) +} + +#[cfg(test)] +mod tests; diff --git a/rsworkspace/crates/a2a/a2a-nats/src/task_event/tests.rs b/rsworkspace/crates/a2a/a2a-nats/src/task_event/tests.rs new file mode 100644 index 0000000000..87c52a0909 --- /dev/null +++ b/rsworkspace/crates/a2a/a2a-nats/src/task_event/tests.rs @@ -0,0 +1,66 @@ +use a2a::event::TaskStatusUpdateEvent; +use a2a::types::{TaskState, TaskStatus}; + +use super::*; + +fn status_event(task_id: &str) -> StreamResponse { + StreamResponse::StatusUpdate(TaskStatusUpdateEvent { + task_id: task_id.to_string(), + context_id: "ctx".to_string(), + status: TaskStatus { + state: TaskState::Working, + message: None, + timestamp: None, + }, + metadata: None, + }) +} + +/// What the previous release published: the event itself, no envelope. +fn legacy_body(task_id: &str) -> Vec { + serde_json::to_vec(&status_event(task_id)).unwrap() +} + +#[test] +fn a_pre_envelope_event_still_decodes() { + let decoded = decode_legacy_event(&legacy_body("task-1")).expect("an older agent's event is still readable"); + assert_eq!(decoded, status_event("task-1")); +} + +#[test] +fn a_current_envelope_is_not_mistaken_for_a_legacy_event() { + // Both shapes reach a reader during a rolling upgrade, so the two must stay + // distinguishable: an envelope decoded as a legacy event would lose its id and + // hide a JSON-RPC error as a success. + let body = serde_json::to_vec(&serde_json::json!({ + "jsonrpc": "2.0", + "id": "req-1", + "result": status_event("task-1"), + })) + .unwrap(); + assert!(decode_legacy_event(&body).is_none()); +} + +#[test] +fn a_malformed_body_is_not_dressed_up_as_an_event() { + assert!(decode_legacy_event(b"not json").is_none()); + assert!(decode_legacy_event(br#"{"unknownVariant":{}}"#).is_none()); +} + +#[test] +fn a_legacy_event_lifts_into_a_response_carrying_the_callers_id() { + let response = legacy_event_as_response(&legacy_body("task-1"), &Value::String("corr-1".to_owned())) + .expect("a legacy event is liftable"); + + assert_eq!(response["jsonrpc"], "2.0"); + assert_eq!(response["id"], "corr-1"); + assert_eq!( + serde_json::from_value::(response["result"].clone()).unwrap(), + status_event("task-1") + ); +} + +#[test] +fn nothing_else_lifts() { + assert!(legacy_event_as_response(b"not json", &Value::Null).is_none()); +} From 7db032d8d7c1207c60a7715f5ee670683bccb1fc Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Fri, 14 Aug 2026 23:34:07 -0400 Subject: [PATCH 11/12] chore(lints): place declarations where the repo policy lints expect them Signed-off-by: Yordis Prieto --- .../crates/a2a/a2a-bridge/src/inbound/tests.rs | 2 +- .../crates/acp/acp-nats/src/constants.rs | 13 +++++++++++++ .../acp-nats/src/jetstream/streams/tests.rs | 6 +----- .../acp/acp-nats/src/nats/subjects/stream.rs | 18 ++++-------------- 4 files changed, 19 insertions(+), 20 deletions(-) diff --git a/rsworkspace/crates/a2a/a2a-bridge/src/inbound/tests.rs b/rsworkspace/crates/a2a/a2a-bridge/src/inbound/tests.rs index 5931b98056..2a76579e33 100644 --- a/rsworkspace/crates/a2a/a2a-bridge/src/inbound/tests.rs +++ b/rsworkspace/crates/a2a/a2a-bridge/src/inbound/tests.rs @@ -3,6 +3,7 @@ use std::sync::Arc; use async_trait::async_trait; use axum::http::{HeaderMap, HeaderValue, StatusCode}; +use axum::response::IntoResponse; use bytes::Bytes; use futures_util::stream::{self, Stream}; use serde_json::json; @@ -395,7 +396,6 @@ async fn handle_jsonrpc_invalid_agent_header_errors() { /// Reads an SSE stream the way a caller does: every frame is an unnamed /// `data:` line, so the JSON bodies are what the assertions look at. async fn sse_frames(stream: BoxStream<'static, Result>) -> Vec { - use axum::response::IntoResponse; let response = Sse::new(stream).into_response(); let bytes = axum::body::to_bytes(response.into_body(), usize::MAX).await.unwrap(); String::from_utf8_lossy(&bytes) diff --git a/rsworkspace/crates/acp/acp-nats/src/constants.rs b/rsworkspace/crates/acp/acp-nats/src/constants.rs index ba03b2d63b..d564ff60e3 100644 --- a/rsworkspace/crates/acp/acp-nats/src/constants.rs +++ b/rsworkspace/crates/acp/acp-nats/src/constants.rs @@ -30,3 +30,16 @@ pub const CONTENT_TYPE_PLAIN: &str = "text/plain"; pub const SESSION_ID_HEADER: &str = "X-Session-Id"; pub const CAUSATION_ID_HEADER: &str = "X-Causation-Id"; + +/// Stream suffixes this crate used to provision and no longer does. +/// +/// Dropping a variant from `AcpStream` stops the provisioner from creating the +/// stream, but a deployment that ran an earlier release still has it, still has +/// its stored messages, and still bills for its storage. Nothing here deletes +/// anything: a stream delete is unrecoverable, it races an operator who may still +/// be draining the old stream, and a rollback to the prior release would silently +/// re-create it empty. Retirement is an operator step, and this list is what names +/// the streams that step applies to. +/// +/// See `docs/how-to/retire-acp-notifications-stream.md`. +pub const RETIRED_STREAM_SUFFIXES: [&str; 1] = ["NOTIFICATIONS"]; diff --git a/rsworkspace/crates/acp/acp-nats/src/jetstream/streams/tests.rs b/rsworkspace/crates/acp/acp-nats/src/jetstream/streams/tests.rs index 9b1bddbd5f..b9d1efcce3 100644 --- a/rsworkspace/crates/acp/acp-nats/src/jetstream/streams/tests.rs +++ b/rsworkspace/crates/acp/acp-nats/src/jetstream/streams/tests.rs @@ -2,7 +2,7 @@ use async_nats::jetstream::stream::{DiscardPolicy, RetentionPolicy, StorageType} use crate::acp_prefix::AcpPrefix; use crate::constants::DEFAULT_STREAM_MAX_AGE; -use crate::nats::AcpStream; +use crate::nats::{AcpStream, retired_stream_names}; use super::*; @@ -221,8 +221,6 @@ fn no_subject_overlaps_between_streams() { #[test] fn retired_stream_names_follow_the_same_naming_as_provisioned_ones() { - use crate::nats::retired_stream_names; - assert_eq!(retired_stream_names(&p("acp")), vec!["ACP_NOTIFICATIONS".to_owned()]); assert_eq!( retired_stream_names(&p("my.multi.part")), @@ -232,8 +230,6 @@ fn retired_stream_names_follow_the_same_naming_as_provisioned_ones() { #[test] fn no_retired_stream_is_still_provisioned() { - use crate::nats::retired_stream_names; - let prefix = p("acp"); let live: Vec = AcpStream::ALL.iter().map(|s| s.stream_name(&prefix)).collect(); for retired in retired_stream_names(&prefix) { diff --git a/rsworkspace/crates/acp/acp-nats/src/nats/subjects/stream.rs b/rsworkspace/crates/acp/acp-nats/src/nats/subjects/stream.rs index 7954faa673..54db61d0be 100644 --- a/rsworkspace/crates/acp/acp-nats/src/nats/subjects/stream.rs +++ b/rsworkspace/crates/acp/acp-nats/src/nats/subjects/stream.rs @@ -83,24 +83,14 @@ impl AcpStream { } } -/// Suffixes this crate used to provision and no longer does. -/// -/// Dropping a variant from [`AcpStream`] stops the provisioner from creating -/// the stream, but a deployment that ran an earlier release still has it, -/// still has its stored messages, and still bills for its storage. Nothing -/// here deletes anything: a stream delete is unrecoverable, it races an -/// operator who may still be draining the old stream, and a rollback to the -/// prior release would silently re-create it empty. Retirement is an operator -/// step, and this list is what names the streams that step applies to. -/// -/// See `docs/how-to/retire-acp-notifications-stream.md`. -const RETIRED_SUFFIXES: [&str; 1] = ["NOTIFICATIONS"]; - /// Stream names an upgraded deployment may still be carrying, for the prefix /// it was provisioned under. pub fn retired_stream_names(prefix: &AcpPrefix) -> Vec { let root = prefix.as_str().to_uppercase().replace('.', "_"); - RETIRED_SUFFIXES.iter().map(|s| format!("{root}_{s}")).collect() + crate::constants::RETIRED_STREAM_SUFFIXES + .iter() + .map(|s| format!("{root}_{s}")) + .collect() } impl std::fmt::Display for AcpStream { From 9f3fdded7599d7d997eac1fb86bf8b61a5035962 Mon Sep 17 00:00:00 2001 From: Yordis Prieto Date: Fri, 14 Aug 2026 23:49:48 -0400 Subject: [PATCH 12/12] fix(a2a): keep every SSE data line something the caller can answer to A JSON object is not a response, and a caller parses each data line as one. Signed-off-by: Yordis Prieto --- .../how-to/retire-acp-notifications-stream.md | 13 +++++----- .../crates/a2a/a2a-bridge/src/inbound.rs | 22 ++++++++++++---- .../a2a/a2a-bridge/src/inbound/tests.rs | 25 +++++++++++++++++++ 3 files changed, 49 insertions(+), 11 deletions(-) diff --git a/docs/how-to/retire-acp-notifications-stream.md b/docs/how-to/retire-acp-notifications-stream.md index 926bbbcef4..a5e19a70f9 100644 --- a/docs/how-to/retire-acp-notifications-stream.md +++ b/docs/how-to/retire-acp-notifications-stream.md @@ -13,10 +13,11 @@ stored messages, and its storage bill. This page is how you retire it. ## When to use this Use this procedure once per ACP deployment, after every process has been -upgraded to a release whose provisioner no longer lists the stream. Confirm -that with: +upgraded to a release whose provisioner no longer lists the stream. Check the +deployed process versions to establish that, then use this to see whether the +retired stream is still there: -``` +```shell nats stream ls ``` @@ -42,7 +43,7 @@ deleting the stream underneath them drops those messages. 1. Confirm the stream is idle. Its message count should stop growing: - ``` + ```shell nats stream info _NOTIFICATIONS ``` @@ -54,13 +55,13 @@ deleting the stream underneath them drops those messages. but doing it first makes a still-attached reader fail visibly here rather than silently later: - ``` + ```shell nats consumer rm _NOTIFICATIONS ``` 3. Delete the stream: - ``` + ```shell nats stream rm _NOTIFICATIONS ``` diff --git a/rsworkspace/crates/a2a/a2a-bridge/src/inbound.rs b/rsworkspace/crates/a2a/a2a-bridge/src/inbound.rs index de5c129485..bfb9326da2 100644 --- a/rsworkspace/crates/a2a/a2a-bridge/src/inbound.rs +++ b/rsworkspace/crates/a2a/a2a-bridge/src/inbound.rs @@ -19,7 +19,7 @@ use axum::{ use bytes::Bytes; use futures_util::StreamExt; use futures_util::stream::{self, BoxStream, Stream}; -use serde_json::{Value, json}; +use serde_json::{Map, Value, json}; use tracing::warn; use a2a_nats::RequestId; @@ -485,9 +485,10 @@ impl TaskJetStreamPort for ScriptedTaskJetstream { /// The id is restamped rather than forwarded, because the hops behind this edge /// correlate on their own transport id (`Trogon-Req-Id`, or a minted one when the /// caller sent no id at all). Only the caller's own id is meaningful to the -/// caller. A payload that is not a JSON object leaves as a correlated error -/// frame: a client parses every `data:` line as a JSON-RPC response, so raw -/// bytes there are a parse failure rather than the diagnostic they look like. +/// caller. A payload that is not a JSON-RPC response leaves as a correlated +/// error frame: a client parses every `data:` line as a JSON-RPC response, so +/// anything else there is a parse failure rather than the diagnostic it looks +/// like. /// /// An event an older agent published carries no envelope of its own, so it is /// given one here instead of being restamped: forwarding it as-is would put a @@ -497,7 +498,7 @@ fn sse_data_line(body: &[u8], caller_id: &Value) -> Event { return Event::default().data(response.to_string()); } - let Ok(Value::Object(mut envelope)) = serde_json::from_slice::(body) else { + let Some(mut envelope) = serde_json::from_slice::(body).ok().and_then(response_envelope) else { warn!( payload = %String::from_utf8_lossy(body), "stream payload is not a JSON-RPC envelope" @@ -508,6 +509,17 @@ fn sse_data_line(body: &[u8], caller_id: &Value) -> Event { Event::default().data(Value::Object(envelope).to_string()) } +/// A JSON-RPC response carries exactly one of `result` and `error`. An object +/// holding neither is a request, an empty body, or something else entirely, and +/// stamping the caller's id onto it would emit a `data:` line that answers +/// nothing. +fn response_envelope(body: Value) -> Option> { + let Value::Object(envelope) = body else { + return None; + }; + (envelope.contains_key("result") != envelope.contains_key("error")).then_some(envelope) +} + fn sse_error_line(caller_id: &Value, err: &BridgeError) -> Event { sse_error_frame(caller_id, err.to_string()) } diff --git a/rsworkspace/crates/a2a/a2a-bridge/src/inbound/tests.rs b/rsworkspace/crates/a2a/a2a-bridge/src/inbound/tests.rs index 2a76579e33..29094e7367 100644 --- a/rsworkspace/crates/a2a/a2a-bridge/src/inbound/tests.rs +++ b/rsworkspace/crates/a2a/a2a-bridge/src/inbound/tests.rs @@ -436,6 +436,31 @@ async fn every_sse_frame_is_a_json_rpc_response_carrying_the_caller_id() { ); } +#[tokio::test] +async fn a_json_object_that_answers_nothing_leaves_as_an_error_frame() { + // Being a JSON object is not being a response. An empty body or a + // request-shaped one carries no result and no error, so stamping the caller's + // id onto it would put a `data:` line on the wire that answers nothing. + let frames = sse_frames(sse_from_bootstrap_and_payloads( + serde_json::to_vec(&json!({"jsonrpc":"2.0","id":"transport-9","result":{"taskId":"t-1"}})).unwrap(), + Box::pin(stream::iter(vec![ + Ok(Bytes::from(serde_json::to_vec(&json!({})).unwrap())), + Ok(Bytes::from( + serde_json::to_vec(&json!({"jsonrpc":"2.0","id":1,"method":"message/stream","params":{}})).unwrap(), + )), + ])), + json!("corr-1"), + )) + .await; + + for frame in &frames[1..] { + let value: serde_json::Value = serde_json::from_str(frame).unwrap(); + assert_eq!(value["id"], "corr-1"); + assert_eq!(value["error"]["code"], INTERNAL_ERROR, "{value}"); + assert!(value["result"].is_null(), "{value}"); + } +} + #[tokio::test] async fn an_event_an_older_agent_published_reaches_the_caller_as_a_response() { // The events stream retains by limits and a rolling upgrade runs both agent