alkhttp/tasks/adapters/review-001-response-decode-fidelity.md

id, name, status, depends_on, scope, risk, impact, level, tags

id	name	status	depends_on	scope	risk	impact	level	tags
review-001-forward-response-fidelity	Upstream response decode — vendored JSON, size caps, loud auth errors, error bodies (FWD-07, FWD-08, FWD-10, FWD-12)	completed		moderate	medium	component	implementation	adapters review-001

Description

Review 001 findings on src/adapters/forward.rs's response-decode paths (forward, forward_stream), minus the SSE parser (its own task):

• FWD-07: content_type.contains("application/json") (forward.rs:236) misses application/vnd.api+json, application/problem+json, etc. → vendor JSON decoded as a Value::Array of one Number per byte. No response size limit on any read path (json()/text()/bytes()) — a hostile upstream controls caller memory. forward_stream never checks content-type (200 HTML → empty stream, no error). Fix: mime-essence matching (application/.*+json suffix semantics), response size caps on all read paths, and a loud error for non-SSE content on a Sub op.
• FWD-08 (forward.rs:104-126, :87-94): invalid credential values (control characters, typo'd header name) silently drop the header — the request goes out unauthenticated and the caller only sees the eventual upstream 401. Fail loudly at call time. (No leak risk today — nothing is logged; keep it that way.)
• FWD-10 (forward.rs:215-227, :333-346): upstream error bodies discarded — surfaced message is only "HTTP {status}: {reason}"; upstream diagnostics never reach the caller and unconsumed bodies hinder connection reuse. Surface a bounded error-body echo on the error path; keep the HTTP_<status> mapping as is (correct).
• FWD-12: byte-identical duplicates value_to_path_segment/ value_to_query (:133-151); forward's op_type parameter only toggles ACCEPT and its text/ branch would buffer an entire SSE stream if a Sub were ever routed here (currently unreachable, inviting misuse — remove or make safe); the unwrap_or_else(|_| "null") at :197 masks an unreachable serialization failure with a null body; SharedHttpClient::reload performs two separate ArcSwap::store calls.

Acceptance Criteria

• application/vnd.api+json (and application/problem+json) decode as JSON, not byte arrays (test)
• Response size cap enforced on JSON/text/binary read paths (test)
• Non-SSE content-type on a Sub op produces a loud error, not an empty stream (test)
• Invalid credential values fail loudly at call time (test), no credential material logged
• Upstream 4xx/5xx bodies surface bounded in the error envelope (test)
• Duplicate value_to_* helpers collapsed; text/ buffering branch removed or safe; reload atomic
• forward.rs COV-01 uncovered regions (binary branch, auth arms, non-2xx mapping, content-type branches) substantially covered
• cargo test and cargo clippy --all-targets -- -D warnings pass

References

• docs/reviews/001-initial-implementation-review.md (Part D, FWD-07, FWD-08, FWD-10, FWD-12; Part I, COV-01)

Notes

Agent fills during implementation. Shares forward.rs with review-001-forward-url-safety (request side) and review-001-sse-parser — sequence those first or coordinate.

FWD-07: JSON dispatch is now mime-essence based (is_json_content_type: type application, subtype json or +json suffix; hand-rolled — mime is in the lock file only as a transitive dep of reqwest, not a direct dependency, so no new crate). Read paths (JSON/text/binary) are byte-capped by read_body_capped at RESPONSE_BODY_CAP = 16 MiB; over-cap surfaces HTTP_413. forward_stream checks the Content-Type essence (text/event-stream) and a non-SSE 2xx yields one INVALID_RESPONSE_TYPE error envelope instead of an empty stream. FWD-08: every silently-dropped header — default headers (build_request), and the Bearer/ApiKey/Basic credential arms — now returns a loud CallError::internal ("refusing to send the request unauthenticated"); error text names the header or namespace but never echoes credential material (tested). FWD-10: non-2xx in both forward and forward_stream goes through error_envelope: HTTP_<status> mapping unchanged (ADR-023) plus a bounded echo of the upstream body — read once, capped at ERROR_BODY_ECHO_CAP = 4 KiB, control characters stripped (newline/tab kept), [truncated] marker when the read hit STATUS_BODY_DRAIN (64 KiB) without EOF, in which case the connection is dropped rather than drained further (bounded connection-reuse trade). FWD-12: value_to_path_segment/value_to_query collapsed onto one scalar_value_to_string extractor (the raw scalar is shared; path rendering applies PATH_VALUE_ENCODE_SET, query emission stays raw for query_pairs_mut — they were byte-identical pre-FWD-01 but the roles diverged, so a single raw-then-encode function was wrong for the query arm and the pre-existing +-encoding test proved it); forward lost its op_type parameter (the Sub branch only toggled ACCEPT and the text/ buffering branch is gone — Sub ops route through forward_stream); request-body serialization failure is a loud CallError::internal instead of a silent null body (both handlers); SharedHttpClient now holds client+config in one ArcSwap<SharedHttpInner> so reload swaps both in a single atomic store (minimal touch to src/client/http_client.rs, public API client()/config() unchanged). COV-01 regions covered: vendor/+json content-type branches, binary branch, all three auth arms (loud error paths + the pre-existing success tests), non-2xx mapping with error-body echo (2/404/429/500, truncated and exact), cap trips on all three read paths, non-SSE Sub stream, SSE happy path through forward_stream.

Summary

FWD-07/08/10/12 remediated in src/adapters/forward.rs (+ one-spot SharedHttpClient::reload atomicity fix in src/client/http_client.rs): vendor-JSON mime-essence decode, 16 MiB RESPONSE_BODY_CAP on all buffered reads (HTTP_413 on breach), loud INVALID_RESPONSE_TYPE on non-SSE Sub-op streams, loud call-time errors for invalid credential/default-header values (no secret echo), bounded 4 KiB error-body echo on non-2xx with a 64 KiB STATUS_BODY_DRAIN read cap, duplicate scalar helpers collapsed, forward's dead op_type Sub branch removed, serialization failure made loud, and the hot-reload swap made atomic (joint client+config holder). 23 tests in forward.rs::tests (was 9) covering the previously uncovered binary/auth/non-2xx/content-type regions (COV-01); full suite 265 passing. Commits 9bb8487 (core) and 7ac2dc5 (tests).