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/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/docs/how-to/retire-acp-notifications-stream.md b/docs/how-to/retire-acp-notifications-stream.md new file mode 100644 index 0000000000..a5e19a70f9 --- /dev/null +++ b/docs/how-to/retire-acp-notifications-stream.md @@ -0,0 +1,81 @@ +# 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. Check the +deployed process versions to establish that, then use this to see whether the +retired stream is still there: + +```shell +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: + + ```shell + 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: + + ```shell + nats consumer rm _NOTIFICATIONS + ``` + +3. Delete the stream: + + ```shell + 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/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..bfb9326da2 100644 --- a/rsworkspace/crates/a2a/a2a-bridge/src/inbound.rs +++ b/rsworkspace/crates/a2a/a2a-bridge/src/inbound.rs @@ -19,8 +19,10 @@ 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; 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; @@ -29,7 +31,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 +477,73 @@ 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. +/// +/// 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-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 +/// `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 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" + ); + 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_task_line(body: &Bytes) -> Event { - Event::default() - .event("task-event") - .data(String::from_utf8_lossy(body.as_ref())) +/// 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()) } -fn sse_error_line(err: &BridgeError) -> Event { - Event::default().event("error").data(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": message }, + }); + 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, &caller_id); 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(), &caller_id), + Err(ref err) => sse_error_line(&caller_id, err), }) }); @@ -715,6 +758,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 @@ -744,7 +794,7 @@ 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 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 4486d2dd48..29094e7367 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; @@ -298,6 +299,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": {}})] { @@ -373,3 +392,102 @@ 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 { + 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 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")), + 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"); + + 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"); + 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}" + ); +} + +#[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 + // 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-bridge/src/nats_transport_harness.rs b/rsworkspace/crates/a2a/a2a-bridge/src/nats_transport_harness.rs index 27419d209a..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,8 +121,16 @@ 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!({ "event": "task-status", "taskId": "task-sse-1" }).to_string(), + json!({ + "jsonrpc": "2.0", + "id": "transport-9", + "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..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 @@ -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,33 @@ 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!( + !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]["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}"); } - assert!(saw_bootstrap, "expected SSE gateway bootstrap line"); - assert!(saw_task_event, "expected SSE JetStream task event line"); 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..11febbba2b 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,17 @@ 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`], 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())); } - a2a_nats::jsonrpc::extract_request_id_from_body(payload).map(|id| ReqId::from_header(id.to_string())) + 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 3770834019..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 @@ -199,6 +199,37 @@ 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 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 req_id = req_id_from_headers_or_payload(&headers, br#"{"jsonrpc":"2.0","id":"corr-1","method":"x"}"#) + .expect("payload extract"); + + 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] +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..862f268075 100644 --- a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env.rs +++ b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env.rs @@ -138,17 +138,12 @@ 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, 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 { - match a2a_nats::jsonrpc::extract_request_id_from_body(payload)? { - a2a_nats::JsonRpcId::Null => None, - id => Some(id.to_string()), - } + 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 b8fb8bb954..c9dba5963f 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,18 @@ 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")); +} + +#[test] +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-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..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 @@ -1,34 +1,49 @@ 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); +} + +#[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/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..b69332b2c6 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,48 @@ 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"); +} + +#[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/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/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.rs b/rsworkspace/crates/a2a/a2a-nats/src/client/event_stream.rs index a041430289..bb5d8449bf 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,21 @@ 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) => crate::task_event::decode_legacy_event(payload).ok_or_else(|| 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..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 @@ -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); @@ -135,7 +145,44 @@ 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] +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); + + 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); + + let event = stream.next().await.expect("a pre-envelope event").unwrap(); + assert_eq!(event, make_status_event("task-1")); +} + +#[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] @@ -168,8 +215,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 +232,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 +257,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..27decb45c2 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; @@ -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, JsonRpcId::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 f7ee04a046..e5b7ad3dc5 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,8 +50,8 @@ where Req: Serialize, Res: DeserializeOwned, { - let encoded = encode_client_request(method, JsonRpcId::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)?, @@ -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/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 708e9c4c18..ff5cdd7993 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,19 @@ 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), + WireError::Codec(e) => ClientError::Codec(e), + WireError::UnexpectedMessage => ClientError::UnexpectedMessage, + } +} + #[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..c9fc382a84 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(); @@ -66,6 +65,31 @@ fn roundtrip_reconstructs_canonical_json_at_edge() { assert_eq!(value["method"], "tasks/get"); } +#[test] +fn map_wire_error_carries_every_variant_across_without_flattening() { + let source = serde_json::from_str::("{}").unwrap_err(); + assert!(matches!( + map_wire_error(WireError::Deserialize(source)), + ClientError::Deserialize(_) + )); + + 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] fn merge_headers_overlays_jsonrpc_fields() { let mut base = HeaderMap::new(); @@ -80,9 +104,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..d6a43c98b1 100644 --- a/rsworkspace/crates/a2a/a2a-nats/src/jsonrpc.rs +++ b/rsworkspace/crates/a2a/a2a-nats/src/jsonrpc.rs @@ -9,58 +9,47 @@ 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, } } +/// 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. +/// +/// 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 { + match extract_request_id_from_body(raw)? { + ResponseId::Null => None, + ResponseId::Number(n) => Some(n.to_string()), + ResponseId::String(s) => Some(s), + } +} + #[cfg(test)] mod tests; 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..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; @@ -38,7 +39,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))); } 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()); +} 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..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::*; @@ -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 { @@ -231,3 +218,24 @@ fn no_subject_overlaps_between_streams() { } } } + +#[test] +fn retired_stream_names_follow_the_same_naming_as_provisioned_ones() { + 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() { + 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/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/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/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/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/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..54db61d0be 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,11 +78,21 @@ 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)) } } +/// 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('.', "_"); + crate::constants::RETIRED_STREAM_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()) 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 {