Merge branch 'wt/review-002-fwd17-19-contract-decisions'

docs/architecture/decisions/066-from-jsonschema-as-http-adapter.md

@@ -149,6 +149,56 @@ error projection and the success sweep (SSE detection +
 output-schema selection), where the precedence order is: concrete 2XX
 statuses, then `2XX`, then `default`.
 
+### Forwarding contract decisions (review 002 FWD-17/18/19)
+
+The shared forwarding core (`src/adapters/forward.rs`) used by both
+`from_openapi` and `from_jsonschema` pins three contracts. Each is a
+decision the code half-implied; the module doc carries the full normative
+text, this section records the reasoning at the assembly-trust boundary
+that ADR-066 establishes: an assembly's spec/template/base-URL inputs
+are trusted configuration, while peer call-time input never is.
+
+**FWD-17 — SSE payload contract (decided: carry raw payload + event
+name for non-JSON frames).** A subscription frame's `data:` payload
+that is itself valid JSON surfaces as the decoded value (`123` stays a
+number, `"123"` stays a string). Any other payload surfaces as the
+`{"data": <raw>, "event": <name|null>}` wrapper: a legitimately
+non-JSON stream stays field-addressable and an upstream's named-event
+conventions (`event: error`) are visible instead of indistinguishable
+from data. The `event` value is the frame's `event:` field under
+WHATWG last-wins semantics — `null` when absent, never the implicit
+`message` default, so "named" and "default" stay distinguishable. JSON
+frames surface as themselves even under a named event; consequence:
+the event name is only visible on non-JSON frames. Chosen over the
+alternative (document the old JSON-or-string contract) because the old
+behavior silently erased payload shape and made an upstream's error
+convention unobservable — a fidelity loss on the same axis ADR-023
+already rejects for status mapping.
+
+**FWD-18 — placeholder routing rule (decided: fix + structural-value
+error).** A key matching a path-template placeholder is consumed by the
+path and never also emits as a query parameter, whatever its value's
+shape (the renderer's placeholder check precedes query routing — fixed
+behavior, pinned by test). A placeholder renders exactly one literal
+path segment, so a structural value (object/array) under a placeholder
+key fails with `INVALID_INPUT` rather than splicing minified JSON into
+the path. Scalar values (string/number/boolean/null) render
+percent-encoded as before.
+
+**FWD-19 — literal `%` in template text (decided: documented
+trade-off, not rejection).** A `%` inside a *value* is always encoded
+(`%` → `%25`), so a value can never inject or fake percent escapes. A
+`%` in *template or base text* survives verbatim: an assembly writing
+`%2F` into a template is presumed to intend a pre-encoded segment for
+upstreams that route `%2F` differently from `/`. Inputs are
+assembly-supplied (this ADR's trust boundary), so the strict shape
+(call-time rejection of raw `%` in template text) is rejected — it
+would break legitimate pre-encoded templates without adding safety:
+the surviving `%2F` still renders as a single literal segment, cannot
+change the origin, and cannot be forged from peer input. The trade-off
+(upstream-dependent routing of bare percent escapes) is the
+assembly's choice, not this crate's.
+
 ## Consequences
 
 **Positive**:

src/adapters/forward.rs

@@ -38,6 +38,29 @@
 //! the buffer grows, so the reassembly buffer can never exceed the
 //! line cap.
 //!
+//! # Streaming payload contract (FWD-17)
+//!
+//! Each parsed SSE frame becomes one success envelope:
+//!
+//! - A `data:` payload that is valid JSON surfaces as the decoded value
+//!   itself — `123` as a number, `"123"` as a string (the raw-text
+//!   fallback this replaces erased that distinction). This holds even
+//!   when the frame carries an `event:` name.
+//! - Any other payload surfaces as the
+//!   `{"data": <raw payload>, "event": <name|null>}` wrapper: the raw
+//!   text stays field-addressable and an upstream's named-event
+//!   conventions (`event: error`, `event: ping`, …) are visible on
+//!   non-JSON frames. The name is the frame's `event:` field (WHATWG
+//!   last-wins); `null` when the upstream sent none — the spec's
+//!   implicit `message` default is *not* substituted, so "upstream
+//!   named it" and "default name" remain distinguishable.
+//! - An empty payload is `Null`.
+//!
+//! Known limitation: a JSON body under a named event surfaces as the
+//! decoded value only — the event name is not carried on JSON frames;
+//! a non-JSON body under a named event is the only combination where
+//! both are visible.
+//!
 //! # Credentials and input routing
 //!
 //! The forwarding handler is the no-env-vars credential injection point
@@ -56,6 +79,36 @@
 //! request header, the declared `GATEWAY_BODY_KEY` property becomes the
 //! request body, and every other declared key becomes an upstream query
 //! parameter.
+//!
+//! # Input routing and path placeholders (FWD-18)
+//!
+//! A key that matches a `{placeholder}` in the path template is consumed
+//! by the path and never also emits as a query parameter, regardless of
+//! its value's shape — the placeholder check precedes query routing in
+//! the request builder. A placeholder renders exactly one literal path
+//! segment, so its value must be a scalar: object/array values fail with
+//! `INVALID_INPUT` (structural values have no faithful single-segment
+//! rendering; splicing the minified JSON into the path was the
+//! pre-decision behavior and is rejected now).
+//!
+//! # Percent handling in the rendered path (FWD-19)
+//!
+//! Two different contracts apply, by design:
+//!
+//! - A `%` arriving inside a *value* is always encoded (`%` → `%25`),
+//!   so values carrying `%2F` cannot be mistaken for this crate's own
+//!   escapes and a value can never inject URL structure.
+//! - A `%` in *template/base text* survives verbatim. Templates and
+//!   base URLs are assembly-supplied (ADR-066 trust boundary), so an
+//!   assembly that writes `/s3%2Fkeys` is presumed to mean a
+//!   pre-encoded segment for upstreams that route `%2F` differently
+//!   from `/` — that upstream-semantics choice belongs to the assembly,
+//!   not this crate. No injection results: the surviving `%2F` still
+//!   forms a single segment (the origin check plus the two-pass
+//!   percent-encoding over template text see to that), and the
+//!   value-side rule above means every bare `%` in a rendered path
+//!   traces to template text the assembly author wrote.
+//!
 
 use std::collections::HashMap;
 use std::sync::Arc;
@@ -420,11 +473,26 @@ fn scalar_value_to_string(value: &Value) -> String {
 /// Percent-encoding scheme used by [`render_path_template`]: a value
 /// containing `/`, `?`, `#`, or `\` — and any raw `%` it carries — is
 /// encoded so that a rendered value stays one literal path segment
-/// (FWD-01 traversal/ smuggled-segments gate). Path parameters are never
+/// (FWD-01 traversal/ smuggled-segments gate). Scalars are never
-/// rejected: they are rendered safely instead.
+/// rejected: they are rendered safely instead. Object/array values
-pub(crate) fn value_to_path_segment(value: &Value) -> String {
+/// under a placeholder key are rejected with `INVALID_INPUT` (FWD-18):
-    let raw = scalar_value_to_string(value);
+/// a path placeholder designates exactly one literal segment, so a
-    utf8_percent_encode(&raw, PATH_VALUE_ENCODE_SET).to_string()
+/// structural value has no faithful rendering — the pre-decision
+/// behavior spliced the minified JSON into the path *and* emitted the
+/// key as a query parameter (double-routed, neither faithful).
+pub(crate) fn value_to_path_segment(value: &Value) -> Result<String, CallError> {
+    let raw = match value {
+        Value::String(_) | Value::Number(_) | Value::Bool(_) | Value::Null => {
+            scalar_value_to_string(value)
+        }
+        other => {
+            return Err(CallError::invalid_input(format!(
+                "path placeholder value must be a scalar (string, number, boolean, or null); got {}: structural values have no faithful single-segment rendering",
+                type_name_of(other)
+            )))
+        }
+    };
+    Ok(utf8_percent_encode(&raw, PATH_VALUE_ENCODE_SET).to_string())
 }
 
 /// Raw form of a scalar input value for query emission: `&`/`=`
@@ -469,7 +537,7 @@ pub(crate) fn render_path_template(
                 continue;
             }
         };
-        out.push_str(&value_to_path_segment(raw_value));
+        out.push_str(&value_to_path_segment(raw_value)?);
         rest = &tail[end + 1..];
     }
     out.push_str(rest);
@@ -804,13 +872,29 @@ async fn error_envelope(
     ResponseEnvelope::error(request_id, CallError::new(code, message, false))
 }
 
-/// Converts a parsed SSE event into a response envelope, JSON-decoding
+/// Maps a parsed SSE frame to a response envelope under the FWD-17
-/// the data payload when possible.
+/// payload contract: a payload that is valid JSON surfaces as the
+/// decoded value itself (a number payload stays a number, a
+/// quoted-string payload stays a string — the distinction a raw-text
+/// fallback would erase); any other payload surfaces as the
+/// `{"data": <raw payload>, "event": <event-name|null>}` wrapper, so
+/// a non-JSON stream is field-addressable and an upstream's
+/// named-event conventions are visible on it. An empty payload is
+/// `Null`. JSON payloads intentionally carry no `event` binding: a
+/// JSON frame surfaces as itself (shape stability for JSON-first
+/// upstreams, and the spec's implicit `event: message` default never
+/// wraps a payload).
 fn sse_event_envelope(event: SseEvent, request_id: &str) -> ResponseEnvelope {
     let parsed = if event.data.trim().is_empty() {
         Value::Null
     } else {
-        serde_json::from_str(&event.data).unwrap_or(Value::String(event.data.clone()))
+        match serde_json::from_str::<Value>(&event.data) {
+            Ok(value) => value,
+            Err(_) => serde_json::json!({
+                "data": event.data,
+                "event": event.event,
+            }),
+        }
     };
     ResponseEnvelope::ok(request_id, parsed)
 }
@@ -1022,9 +1106,14 @@ fn is_sse_content_type(content_type: &str) -> bool {
     content_type.split(';').next().unwrap_or_default().trim() == "text/event-stream"
 }
 
-/// A parsed SSE event: the `data:` lines joined with `\n`.
+/// A parsed SSE event: the `data:` lines joined with `\n`, plus the
+/// frame's `event:` field name when the upstream sent one (WHATWG
+/// event-stream semantics: the last `event:` line before the blank
+/// line wins; absent → `None`, never the implicit `message` default —
+/// consumers distinguish "upstream named it" from "default name").
 pub(crate) struct SseEvent {
     pub(crate) data: String,
+    pub(crate) event: Option<String>,
 }
 
 #[derive(Debug, thiserror::Error)]
@@ -1053,13 +1142,16 @@ pub(crate) const SSE_EVENT_BUFFER_CAP: usize = 1024 * 1024;
 /// a chunk boundary is therefore not corrupted. Framing follows the
 /// WHATWG `text/event-stream` draft semantics for what the call
 /// protocol needs: lines split on `\n` with optional trailing `\r`;
-/// `data:` fields accumulate and join with `\n`; `event:`, `id:`, and
+/// `data:` fields accumulate and join with `\n`; `event:` is captured
+/// per frame (last one before the blank line wins) and surfaced on
+/// [`SseEvent::event`] for non-JSON payloads (FWD-17); `id:` and
 /// `retry:` fields are accepted and ignored; a blank line dispatches
 /// the pending event; a pending event with data is dispatched at EOF.
 pub(crate) struct SseParser {
     buf: Vec<u8>,
     data_lines: Vec<String>,
     data_seen: bool,
+    event_name: Option<String>,
     bom_stripped: bool,
 }
 
@@ -1069,6 +1161,7 @@ impl SseParser {
             buf: Vec::new(),
             data_lines: Vec::new(),
             data_seen: false,
+            event_name: None,
             bom_stripped: false,
         }
     }
@@ -1135,6 +1228,7 @@ impl SseParser {
             return if self.data_seen {
                 self.complete_event()
             } else {
+                self.discard_event();
                 None
             };
         }
@@ -1148,6 +1242,8 @@ impl SseParser {
         if field == "data" {
             self.data_lines.push(value.to_string());
             self.data_seen = true;
+        } else if field == "event" && !value.is_empty() {
+            self.event_name = Some(value.to_string());
         }
         None
     }
@@ -1156,8 +1252,15 @@ impl SseParser {
         self.data_seen = false;
         Some(SseEvent {
             data: std::mem::take(&mut self.data_lines).join("\n"),
+            event: self.event_name.take(),
         })
     }
+
+    fn discard_event(&mut self) {
+        self.data_seen = false;
+        self.event_name = None;
+        self.data_lines.clear();
+    }
 }
 
 #[cfg(test)]
@@ -1476,6 +1579,114 @@ mod tests {
         assert!(q.contains("q=rust") && q.contains("debug=true"), "{q}");
     }
 
+    /// FWD-18, routing half pin: a key that matched a placeholder never
+    /// also emits as a query parameter — the skip in `build_request`
+    /// precedes query routing and is value-shape-agnostic. The scalar
+    /// case renders into the path only; the object-value case is
+    /// rejected (see the structural-value test below), so here the
+    /// companion non-placeholder key proves query routing stays intact
+    /// around the placeholder skip.
+    #[test]
+    fn placeholder_keys_never_double_route_as_query_params() {
+        let url = request_url(
+            "https://api.example.com",
+            "/repos/{owner}",
+            json!({"owner": "octocat", "extra": "q-val"}),
+        )
+        .expect("scalar placeholder builds");
+        assert_eq!(url.path(), "/repos/octocat");
+        assert_eq!(
+            url.query(),
+            Some("extra=q-val"),
+            "only the non-placeholder key routes to query"
+        );
+
+        let ctx = noop_context();
+        let schema = json!({
+            "type": "object",
+            "properties": {"id": {"type": "string"}},
+        });
+        let err = build_request(
+            "https://api.example.com",
+            "/items/{id}",
+            "GET",
+            &None,
+            &TestHashMap::new(),
+            "svc",
+            &schema,
+            &json!({"id": "x", "undeclared": "v"}),
+            &ctx,
+        )
+        .expect_err("undeclared key still rejected with a placeholder present");
+        assert_eq!(err.code, "INVALID_INPUT");
+    }
+
+    /// FWD-18, structural-value decision: an object/array value under a
+    /// placeholder key is an `INVALID_INPUT` error, not a spliced JSON
+    /// segment. A path placeholder designates exactly one literal
+    /// segment; the pre-decision behavior embedded the minified JSON
+    /// into the path (and, under an additionalProperties schema, also
+    /// emitted the key as a query param — the double-route).
+    #[test]
+    fn object_or_array_path_values_error_instead_of_splicing_json_into_the_path() {
+        for value in [json!({"a": 1}), json!([1, 2])] {
+            let err = request_url(
+                "https://api.example.com",
+                "/things/{id}",
+                json!({"id": value}),
+            )
+            .expect_err("structural path values must be rejected");
+            assert_eq!(err.code, "INVALID_INPUT", "value was: {value}");
+            assert!(
+                err.message.contains("must be a scalar"),
+                "value was: {value}"
+            );
+            assert!(
+                !err.message.contains("{\"a\":1}") && !err.message.contains("[1,2]"),
+                "error must not echo the structural value: {value}"
+            );
+        }
+
+        for value in [json!("text"), json!(42), json!(true)] {
+            let url = request_url(
+                "https://api.example.com",
+                "/things/{id}",
+                json!({ "id": value }),
+            )
+            .expect("scalar path values still render");
+            assert!(
+                url.path().starts_with("/things/"),
+                "scalar {value} renders into the path"
+            );
+        }
+    }
+
+    /// FWD-19 pin (documented trade-off, not a rejection): literal `%`
+    /// in template text is preserved as-is, so an assembly-supplied
+    /// pre-encoded template like `/s3%2Fkeys` reaches the upstream
+    /// verbatim (most stacks route `%2F` differently from `/` — the
+    /// assembly layer owns that choice, ADR-066). A `%` arriving in a
+    /// *value* is always encoded to `%25`, so the only bare `%` in a
+    /// rendered path is one the template author placed.
+    #[test]
+    fn percent_in_template_text_survives_and_percent_in_values_are_always_encoded() {
+        let url = request_url(
+            "https://api.example.com",
+            "/s3%2Fkeys/{name}",
+            json!({"name": "x"}),
+        )
+        .expect("template text may carry literal percent escapes");
+        assert_eq!(url.path(), "/s3%2Fkeys/x");
+
+        let url = request_url(
+            "https://api.example.com",
+            "/files/{name}",
+            json!({"name": "a%2Fb"}),
+        )
+        .expect("value percent is encoded, not preserved");
+        assert_eq!(url.path(), "/files/a%252Fb");
+    }
+
     fn ctx_with_capability(namespace: &str, value: String) -> OperationContext {
         let mut ctx = noop_context();
         ctx.capabilities = Capabilities::new().with_http_token(namespace, value);
@@ -1781,6 +1992,114 @@ mod tests {
         assert!(envelopes[1].result.is_ok());
     }
 
+    /// FWD-17: a valid-JSON payload surfaces as the decoded value
+    /// itself; a non-JSON payload surfaces as the
+    /// `{"data", "event"}` wrapper. Numbers-vs-quoted-strings are both
+    /// valid JSON, so both decode (the raw-text fallback would have
+    /// erased that distinction).
+    #[tokio::test]
+    async fn sse_payload_contract_decodes_json_and_wraps_non_json() {
+        let base = spawn_responder(TestArc::new(|_parts| {
+            http_response(
+                200,
+                "text/event-stream",
+                b"data: 123\n\ndata: \"123\"\n\ndata: plain text\n\ndata: {\"n\":1}\n\n".to_vec(),
+            )
+        }))
+        .await;
+        let stream = forward_stream(
+            &minimal_client(),
+            &base,
+            "/x",
+            "GET",
+            &None,
+            &TestHashMap::new(),
+            "svc",
+            &serde_json::json!({"type": "object"}),
+            &[],
+            json!({}),
+            noop_context(),
+        );
+        let envelopes = collect_stream(stream).await;
+        assert_eq!(envelopes.len(), 4);
+        assert_eq!(envelopes[0].result.clone().unwrap(), json!(123));
+        assert_eq!(envelopes[1].result.clone().unwrap(), json!("123"));
+        assert_eq!(
+            envelopes[2].result.clone().unwrap(),
+            json!({"data": "plain text", "event": null})
+        );
+        assert_eq!(envelopes[3].result.clone().unwrap(), json!({"n": 1}));
+    }
+
+    /// FWD-17: an upstream `event:` field on a non-JSON payload is
+    /// carried in the wrapper; JSON payloads surface as themselves even
+    /// under a named event, and the name is reset after dispatch (the
+    /// second frame must not inherit the first frame's name).
+    #[tokio::test]
+    async fn sse_event_field_carrys_on_the_non_json_wrapper_and_resets() {
+        let base = spawn_responder(TestArc::new(|_parts| {
+            http_response(
+                200,
+                "text/event-stream",
+                b"event: error\ndata: upstream exploded\n\nevent: custom\ndata: {\"n\":1}\n\ndata: after\n\n"
+                    .to_vec(),
+            )
+        }))
+        .await;
+        let stream = forward_stream(
+            &minimal_client(),
+            &base,
+            "/x",
+            "GET",
+            &None,
+            &TestHashMap::new(),
+            "svc",
+            &serde_json::json!({"type": "object"}),
+            &[],
+            json!({}),
+            noop_context(),
+        );
+        let envelopes = collect_stream(stream).await;
+        assert_eq!(envelopes.len(), 3);
+        assert_eq!(
+            envelopes[0].result.clone().unwrap(),
+            json!({"data": "upstream exploded", "event": "error"})
+        );
+        assert_eq!(envelopes[1].result.clone().unwrap(), json!({"n": 1}));
+        assert_eq!(
+            envelopes[2].result.clone().unwrap(),
+            json!({"data": "after", "event": null}),
+            "the name must not leak across frames"
+        );
+    }
+
+    /// FWD-17 parser pin: WHATWG last-wins for repeated `event:` lines
+    /// in one frame; an `event:`-only frame (no data) dispatches
+    /// nothing but must not leak its name into a later frame.
+    #[test]
+    fn sse_parser_last_event_wins_and_name_does_not_leak_across_frames() {
+        let mut parser = SseParser::new();
+        let events = parser
+            .feed(b"event: a\nevent: b\ndata: x\n\n", false)
+            .expect("ascii only");
+        assert_eq!(events.len(), 1);
+        assert_eq!(events[0].event.as_deref(), Some("b"));
+        assert_eq!(events[0].data, "x");
+
+        let events = parser
+            .feed(b"event: orphan\n\n", false)
+            .expect("ascii only");
+        assert!(events.is_empty(), "no data field, nothing dispatched");
+
+        let events = parser.feed(b"data: next\n\n", false).expect("ascii only");
+        assert_eq!(events.len(), 1);
+        assert_eq!(
+            events[0].event.as_deref(),
+            None,
+            "an event:-only frame must not leak its name"
+        );
+    }
+
     #[tokio::test]
     async fn bearer_credential_with_control_character_fails_loudly() {
         let base = spawn_responder(TestArc::new(|_parts| {