--- id: review-001-gateway-publish-semantics name: /publish + /batch gateway fixes (GW-01, GW-06, GW-08..GW-11, HY-13) status: completed depends_on: [] scope: narrow risk: medium impact: component level: implementation tags: [gateway, review-001] --- ## Description Review 001 findings on `src/gateway/routes.rs`'s `/publish` and `/batch` routes: - **GW-01 (major)**: `/publish` (`routes.rs:257-276`) feeds parsed NDJSON straight to `invoke_sink`; `publish_schema` validation lives only in alkcall's wire `Dispatcher` — so a Pub op registered with `publish_schema` receives arbitrary attacker-controlled JSON over HTTP while the same op over the call protocol aborts invalid chunks. Handlers written against the validated-wire guarantee get a transport-dependent invariant. Fix: validate in the route (or move validation into the shared `invoke_sink` spine so both transports enforce it — prefer the spine if alkcall's surface allows, else the route). - **GW-10**: a first line missing `chunk` silently publishes `Value::Null` (`routes.rs:213`) — indistinguishable from intent since null is a legitimate payload. Reject with `INVALID_INPUT` (the route already rejects a missing `operation` this way). - **GW-06**: `/publish` buffers the whole NDJSON body (2 MiB-capped) before dispatch, contradicting ADR-068 step 4. Either stream the body (axum `Body` → framed stream) — the real fix — or amend ADR-068 to document the buffered 2 MiB semantics. Decide, then implement or amend. - **GW-08**: no cap on batch operation count; the 2 MiB body is the only bound and the 30 s deadline is unenforced (GW-05). Cap batch size (constant, e.g. 100). - **GW-09**: internal-op batch entries emit `request_id: null` while dispatched entries carry a UUID — one response body, two envelope shapes. Generate request ids for internal-op entries. - **GW-11**: `/publish` runs four registry lookups + ACL checks that `invoke_sink` then repeats — drop the redundant pre-checks (mirror `/call`/`/batch`, which deliberately rely on the registry). - **HY-13**: the vacuous test at `routes.rs:1572-1583` (`publish_body_is_fully_consumed_before_dispatch_not_required`) cites a socket-level test that doesn't exist — wire the early-disconnect test or delete the stub. ## Acceptance Criteria - [x] `/publish` test with a `publish_schema`-registered Pub op rejects an invalid chunk (review's gate for this unit) - [x] First line without `chunk` → `INVALID_INPUT`, not a null publish (test) - [x] GW-06 decision landed: true streaming (implemented — not the ADR amendment); consistent tests + docs - [x] Batch size capped (test); mixed-shape batch envelopes fixed - [x] Redundant `/publish` pre-checks removed (dispatch still enforces) - [x] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass ## References - docs/reviews/001-initial-implementation-review.md (Part C, GW-01, GW-06, GW-08..GW-11; HY-13) - docs/architecture/decisions/068-gateway-publish-endpoint.md - docs/architecture/decisions/023-operation-error-schemas.md ## Notes - **GW-01 — route-level fix (spine route explored, rejected):** the spine option would put validation inside alkcall's `OperationRegistry::invoke_sink`; but the wire dispatcher's per-chunk validation is intentionally *outside* `invoke_sink` (it lives in `Dispatcher::dispatch`'s `SinkDispatch`/`pump_sink`, which owns the abort channel and init-side error injection). Moving it into `invoke_sink` would have meant a wrapping-stream refactor of that spine plus an alkcall release (alkhttp's dep is the crates.io `0.1.1`). The route now compiles the op's `publish_schema` (jsonschema 0.46, same version alkcall uses) and feeds `NdjsonChunkStream` into `invoke_sink`: every chunk — including the first-line chunk — is validated exactly like the wire dispatcher's `EVENT_PUBLISHED` branch; a violation (or malformed JSON) yields the terminal `Err(INVALID_INPUT)` item the wire's initiator-side `call.error` produces. Both transports now enforce the identical per-chunk contract; no alkcall change was needed. - **GW-06 — true streaming implemented:** `publish_handler` now takes `axum::body::Body` → `into_data_stream()` → `BufferedLines` (newline framer) → `NdjsonChunkStream`; nothing buffers the whole body. The only bound is the per-line cap (2 MiB, mirroring axum's default body limit) plus axum's own whole-body default on transport reads. ADR-068 gained a "Body handling (streamed, not buffered)" section + a consequence line instead of an amendment — the original step-4 wording was already "stream each NDJSON line"; the docs now state the implemented mechanics. - **GW-08:** `MAX_BATCH_OPERATIONS = 100`; over-cap → 400 `INVALID_INPUT` (test covers over-cap rejection and at-cap dispatch). - **GW-09:** `not_found_envelope_json` now generates a `uuid::Uuid::new_v4()` request id, so every entry in one `/batch` response body carries the same envelope shape. - **GW-11:** all four `/publish` pre-checks (internal-op probe, existence probe, ACL probe, op-type probe) removed; visibility/ACL/ handler-kind/type enforcement rides on `invoke_sink` via the shared dispatch spine, exactly like `/call`//`/batch`. - **HY-13:** the vacuous stub was replaced by `publish_client_disconnect_before_dispatch_signals_error_item` (body cut short mid-stream → the handler's `PublishStream` observes the `Err` item) plus the malformed-later-line test now asserts the real 422 `INVALID_INPUT` mapping. - **Adjacent (in-scope necessity):** `error.rs` gained the `INVALID_OPERATION_TYPE` mapping (422 with identity / 401 without) — the dropped pre-checks mean `/publish` now relies on the shared mapper for the non-Pub-op case, and the old fall-through mapped it to 500 (GW-03's finding; the route previously special-cased it to 400). Coordinates with review-001-gateway-stream-errors (GW-03) — that task should verify/doc the mapping rather than re-implement it. ## Summary Implemented all seven findings in `src/gateway/routes.rs` (+ the `error.rs` mapping addition + an ADR-068 doc note): - **GW-01:** `/publish` validates every NDJSON chunk against the op's compiled `publish_schema` (incl. the first-line chunk) before it reaches the sink — a terminal `Err(INVALID_INPUT)`/422 on violation, message + details matching the wire dispatcher. No alkcall spine change (rejected: the wire validation is pump-side by design; a route-level fix achieves both-transport enforcement without touching alkcall's spine or taking a new release dependency). - **GW-06:** true streaming — `Body::into_data_stream()` → newline-framed `BufferedLines` → lazily parsed, schema-validated `NdjsonChunkStream` → `invoke_sink`. ADR-068 documents the streamed semantics + 2 MiB per-line cap. - **GW-08:** `/batch` capped at 100 operations (`INVALID_INPUT` 400). - **GW-09:** internal-op batch entries get generated UUID request ids (single envelope shape per response). - **GW-10:** first publish line missing `chunk` → `INVALID_INPUT` 400. - **GW-11:** `/publish` pre-checks removed; enforcement via `invoke_sink`. - **HY-13:** vacuous test deleted; replaced by an early-terminate body test (client-cut body → handler sees the `Err` item, 422). Tests: 24 gateway-route tests added/updated (schema reject/accept, first-chunk validation, no-schema passthrough, first-line-missing-chunk, oversized-line cap, cap+disconnect semantics, batch cap, batch request-id shape, `INVALID_OPERATION_TYPE` 401/422 matrix + `error.rs` mapping tests). `cargo test` 199 passed; `cargo clippy --all-targets -- -D warnings` clean; `cargo fmt --check` clean.