diff --git a/docs/reviews/002-post-remediation-review.md b/docs/reviews/002-post-remediation-review.md index 79d306c..cf35f19 100644 --- a/docs/reviews/002-post-remediation-review.md +++ b/docs/reviews/002-post-remediation-review.md @@ -50,8 +50,8 @@ return of review-001's classes. - **The remediation held.** Every review-001 finding was re-verified against its fix; 76 of 82 land FIXED-VERIFIED. The residuals are - tracked below as new findings (SRV-12, WS-13, CON-14, OAI-11/12/13, - JS-01, PRJ-16..17, CLI-01..03, FWD-13..14, GW-15..17, WS-14..19) — + tracked below as new findings (SRV-11/12, WS-13..19, CON-14..18, + OAI-10..19, JS-01..03, PRJ-16..24, CLI-01..03, FWD-13..19, GW-15..17) — none is a reversion of the original defect; each is a gap one layer short of the fix's goal, or a new-mechanism bug. - **The one critical-class regression-adjacent defect:** GW-15 — @@ -72,9 +72,9 @@ return of review-001's classes. arrival (so the dribble stall it was built for defeats it — WS-13); the SSE subscription path inherits a 30 s total-request timeout that kills healthy long-lived subscriptions (FWD-15) while its terminal - arms are untested; `from_wss`'s new pending-sweep never exits (CON-14, + arms are untested; `from_wss`'s new pending-sweep never exits (CON-18, a leak per dead session); and the MCP pagination loop trusts the - remote to eventually stop (CON-14). + remote to eventually stop (CON-17). - **Projection fidelity is much improved** (runtime-truthful `/call`, `/publish`, `/batch` verified against the wire) but two disclosure bugs remain: Internal op specs are readable through the @@ -137,7 +137,7 @@ fixed there, `adapter.rs:1184-1215` test). **Fix:** re-apply # Part B' — WebSocket subsystem + from_wss **Regression check: WS-02/04/05/06/07/08/09/10/11 FIXED-VERIFIED.** -WS-01 and the CON-02 sweep are the two residuals (WS-13, CON-14). The +WS-01 and the CON-02 sweep are the two residuals (WS-13, CON-18). The pump consolidation preserved behavior parity (diff-verified `4a825d3..HEAD`) and the inbound memory bound dropped from ~4 GiB to 64 MiB/conn. No reversion found. @@ -177,7 +177,7 @@ cap (1011), and text rejection (1002); the reason string lies in two of three cases. Numeric codes (what clients key on) are correct. **Fix:** carry the reason in the enum variant. -## WS-16 [minor] — Withdrawn during verification (merged into CON-14) +## WS-16 [minor] — Withdrawn during verification (merged into CON-18) ## WS-17 [minor] — Bare-registry/custom WS routes get no idle-timeout knob surface @@ -212,7 +212,7 @@ only. The shared pump makes regression risk low; the flavor adapters are the untested seam. **Fix:** mirror one text-rejection and one cap-trip test over `AxumFraming`. -## CON-14 [minor] — `from_wss` drop monitor loops forever after EOF: one 1 Hz task per dead session, permanently +## CON-18 [minor] — `from_wss` drop monitor loops forever after EOF: one 1 Hz task per dead session, permanently **Verified:** YES. `src/adapters/from_wss.rs:254-272` — once `eof_observed` holds, the sweep branch runs `fail_all` every second @@ -586,6 +586,20 @@ the shared response (oneOf append) instead of overwriting. to the 30 s deadline each) while `/batch` rejects at 101. **Fix:** enforce `MAX_BATCH_OPERATIONS` in the MCP `batch` tool. +## CON-14 [major] — `from_mcp` import hangs forever (and grows memory unboundedly) on a malicious or buggy server that never clears `next_cursor` + +**Verified:** YES (against rmcp 1.8.0 source). `from_mcp/mod.rs:114-119` +delegates to rmcp's `Peer::list_all_tools`, which loops +`while cursor.is_some()` with **no page cap and no timeout**. A server +that always returns `next_cursor: Some(...)` (cycling or ignoring the +cursor) hangs `import()` indefinitely while appending every page's +tools → unbounded memory growth alongside the hang. Tools from +completed pages are not lost (extend per page), but nothing in alkhttp +bounds the loop. **Fix:** replace `list_all_tools` with a bounded loop +(max pages + overall deadline), surfacing `AdapterError::DiscoveryFailed` +when the budget trips. Acceptance gate: a cycling-cursor paging server +test terminates with a clean error. + ## CON-15 [minor] — `from_mcp` tool-name sanitization is not injective; collisions resolve by silent last-write **Verified:** YES. `from_mcp/mod.rs:231-252` + alkcall @@ -794,7 +808,7 @@ The five findings with real attack or availability impact. - OAI-11: memoize cycle-free `$ref` expansions (or node-count budget). Gate: 30-level chain imports in bounded time. -## Unit 2 — Timeout/terminality coherence (WS-13, FWD-14, FWD-15, CLI-01, GW-17, CON-14) +## Unit 2 — Timeout/terminality coherence (WS-13, FWD-14, FWD-15, CLI-01, GW-17, CON-17, CON-18) One coherent theme: the new policy knobs (idle, budget, ceiling, timeout) interact with progress and terminality incorrectly. @@ -804,7 +818,8 @@ timeout) interact with progress and terminality incorrectly. `request_timeout: None`. - CLI-01: budget-aware Retry-After sleep. - GW-17: wrap sink completion in the 30 s deadline (or fix the doc). -- CON-14: dead-connection fast-fail + bounded sweep exit. +- CON-17: bounded MCP pagination loop (max pages + deadline). +- CON-18: dead-connection fast-fail + bounded sweep exit. ## Unit 3 — Projection + doc truthfulness (PRJ-16b, PRJ-17..24, WS-15) + gateway status drift (GW-16) @@ -846,5 +861,5 @@ Also delete the COV-13 dead code. - Six subsystem passes ran in parallel over disjoint file sets; two agent findings were rejected at consolidation (coverage-overreported dark array-`$ref` arm — code verified correct; WS-16 — duplicate of - CON-14) and several "uncovered" flags were reclassified as + CON-18) and several "uncovered" flags were reclassified as cross-binary artifacts after HTML-line verification. \ No newline at end of file diff --git a/tasks/adapters/review-002-con17-mcp-pagination.md b/tasks/adapters/review-002-con17-mcp-pagination.md new file mode 100644 index 0000000..0368da4 --- /dev/null +++ b/tasks/adapters/review-002-con17-mcp-pagination.md @@ -0,0 +1,56 @@ +--- +id: review-002-con17-mcp-pagination +name: Bound MCP tools/list pagination (max pages + deadline) (CON-14) +status: pending +depends_on: [] +scope: narrow +risk: low +impact: component +level: implementation +tags: [adapters, review-002, mcp] +--- + +## Description + +Review 002 CON-14 [major]. `from_mcp/mod.rs:114-119` delegates to +rmcp 1.8.0's `Peer::list_all_tools`, which loops `while +cursor.is_some()` with **no page cap and no timeout** (verified against +rmcp source `service/client.rs:390-407`). A malicious or buggy server +that always returns `next_cursor: Some(...)` (cycling or ignoring the +cursor) hangs `import()` indefinitely while appending every page's +tools → unbounded memory growth alongside the hang. Tools from +completed pages are not lost (extend per page), but nothing in alkhttp +bounds the loop. The paging test that exists (from_mcp_integration.rs) +terminates after 3 pages — the cycling shape is untested. + +## Acceptance Criteria + +- [ ] Replace `list_all_tools` with a bounded loop over `list_tools`: + hard page-count cap + overall deadline (constants or config; + pick generous defaults — e.g. 100 pages / 60 s — and note them + in the module doc) +- [ ] Budget trips → `AdapterError::DiscoveryFailed`-family error + naming the budget (pages seen, tools accumulated), no partial + import left behind (the existing import-fails-closed behavior + holds) +- [ ] Integration test: a paging server that cycles its cursor + (always `next_cursor: Some("a")`) → import terminates with the + clean error, bounded time (no hang) +- [ ] Existing 3-page pagination test unchanged and passing + (well-formed pagination still imports all pages) +- [ ] `cargo test --features mcp`, `cargo clippy --all-features --all-targets -- -D warnings`, + `cargo fmt --check` pass + +## References + +- docs/reviews/002-post-remediation-review.md (Part F', CON-14) +- src/adapters/from_mcp/mod.rs:114-119 (the delegation), rmcp 1.8.0 src/service/client.rs:390-407 (the unbounded loop) +- tests/from_mcp_integration.rs:302-317 (the existing paging server) +- tasks/adapters/review-001-consumer-adapter-hygiene.md (CON-01's pagination work this bounds) + +## Notes + +Small, self-contained, feature-gated (`mcp`). Consider (optional) a +`max_tools` sanity cap as well — same budget family, one more guard +against a hostile server — but do not let scope creep: the page cap + +deadline closes the hang, which is the finding. \ No newline at end of file diff --git a/tasks/adapters/review-002-con18-wss-sweep-exit.md b/tasks/adapters/review-002-con18-wss-sweep-exit.md new file mode 100644 index 0000000..9d7dc19 --- /dev/null +++ b/tasks/adapters/review-002-con18-wss-sweep-exit.md @@ -0,0 +1,59 @@ +--- +id: review-002-con18-wss-sweep-exit +name: Dead-connection fast-fail + bounded from_wss sweep exit (CON-18) +status: pending +depends_on: [] +scope: narrow +risk: low +impact: component +level: implementation +tags: [adapters, review-002, from-wss] +--- + +## Description + +Review 002 CON-18 [minor]. The review-001 CON-02 fix added a 1 s +pending-map sweep to `from_wss`'s drop monitor — but it never exits: +once `eof_observed` holds, the sweep branch runs `fail_all` every +second **forever** (`from_wss.rs:254-272`). Each fire-and-forget +`import()` whose peer dies leaves a spawned task + pending map + watch +receiver alive for the process lifetime (a true task/allocator leak +scaling with import count). Note the naive fix is wrong: the sweep +test proves post-EOF registrations race `fail_all` and hang without +the sweep — so exit requires making post-EOF registration fail fast +*first*. + +## Acceptance Criteria + +- [ ] Post-EOF (dead connection) registration fails fast: calls + registered after the observed EOF resolve immediately with + `CONNECTION_CLOSED` (retryable, matching the established + mapping) instead of relying on the next sweep tick +- [ ] The sweep then exits after a bounded grace period following EOF + (long enough that the fast-fail path is exercised; not forever) + — no per-dead-session permanent task remains +- [ ] Re-verify the existing race tests still pass (pre-drop, + forgotten/held drop-during-registration, post-EOF sweep — + from_wss.rs:809-1001) and tighten assertions where the fast-fail + makes timing deterministic +- [ ] A task-lifecycle test (or task-count assertion) proving the + monitor ends after teardown +- [ ] `cargo test --features wss`, `cargo clippy --features wss --all-targets -- -D warnings`, + `cargo fmt --check` pass + +## References + +- docs/reviews/002-post-remediation-review.md (Part B', CON-18 / CON-16-withdrawn merge note) +- src/adapters/from_wss.rs:244-273 (the monitor loop), :75 (interval), :305 (fire-and-forget import) +- tasks/adapters/review-001-ws-eof-signal.md (the WS-02/CON-02 fix this bounds) +- alkcall/docs/reviews/consumer-findings-ledger.md CF-001 (the retryability classification — keep consistent) + +## Notes + +Implementation shape suggestion: an `AtomicBool`/watch-dead flag on +the call connection consulted in the registration path (fail-fast) +plus a sweep-generation counter (exit after N idle sweeps post-EOF). +The alkcall-side CF-001 note matters: if/when the dead-mux write +mapping changes, the fail-fast class here should match. Keep the +WS-02 losslessness invariant intact — fast-fail is an *additional* +resolution path, not a replacement for the watch signal. \ No newline at end of file diff --git a/tasks/adapters/review-002-fwd13-dot-segments.md b/tasks/adapters/review-002-fwd13-dot-segments.md new file mode 100644 index 0000000..1bce3be --- /dev/null +++ b/tasks/adapters/review-002-fwd13-dot-segments.md @@ -0,0 +1,67 @@ +--- +id: review-002-fwd13-dot-segments +name: Reject lone dot-dot path values that Url-set_path normalizes away (FWD-13) +status: pending +depends_on: [] +scope: narrow +risk: medium +impact: component +level: implementation +tags: [adapters, review-002, security, from-openapi] +--- + +## Description + +Review 002 FWD-13 [major, security]. Lone `.`/`..` path-parameter +values survive the PATH_VALUE_ENCODE_SET (which has no `.`) and are +**silently normalized away by `Url::set_path`** — empirically +reproduced against the locked `url 2.5.8`: + +- `set_path("/tenants/../admin")` → `/admin` +- `set_path("/files/..")` → `/` +- `set_path("/repos/%2e%2e/x")` → `/x` (so adding `%2e` to the encode + set would NOT fix it — the parser normalizes every case-insensitive + spelling) + +Scenario: template `/tenants/{tenant}/resources` with peer input +`tenant = ".."` → the upstream receives `/resources` — a *different* +(east commonly less-scoped, list-everything) endpoint than the template +describes, **with the namespace's injected credentials attached**. The +existing dot-segment test (`traversal_value_cannot_escape_template_path`, +forward.rs:1155) only covers the multi-segment `../../admin` form the +encoder *does* catch. The origin check in `assemble_request_url` +(:520-522) cannot catch this — the origin never changes. + +## Acceptance Criteria + +- [ ] Rendered path **values** that are exactly `.` or `..` + (case-insensitive, including `%2e` spellings before encoding) are + rejected with a loud `INVALID_INPUT`-family CallError — the + cleanest point is `value_to_path_segment` / `render_path_template` +- [ ] A post-`set_path` invariant assert (decoded path segments = + base_dir + rendered segments, byte-identical) protects against + future normalizer surprises (implementer's choice: debug_assert + + loud runtime check, or a property test with a dot/percent/binary + corpus) +- [ ] Tests: lone `..` value → error; lone `.` value → error; `%2e%2e` + spelling → error; a *legitimate* segment containing a dot + (`v1.2.3`, `.hidden-file` as a value) still works (the rejection + is exact-match, not substring) +- [ ] Existing traversal/encoding tests unchanged and passing +- [ ] `cargo test`, `cargo clippy --all-targets -- -D warnings`, + `cargo fmt --check` pass + +## References + +- docs/reviews/002-post-remediation-review.md (Part D', FWD-13) +- src/adapters/forward.rs:340-352 (encode set lacking `.`), :413-448 (renderer), :481-531 (set_path + origin check), :1155 (the test family) +- tasks/adapters/review-001-forward-url-safety.md (the FWD-01 fix this completes — one case short) + +## Notes + +The error message must not echo the raw value's neighbor inputs (no +spec/content leakage beyond the parameter name — follow the existing +message style in `render_path_template`'s unbound-placeholder error). +FWD-18 (object/array placeholder values double-routing to query) is +tracked separately in review-002-fwd17-19-contract-decisions — do not +fold it here, the mechanisms are adjacent but the decisions differ. \ No newline at end of file diff --git a/tasks/adapters/review-002-fwd16-missing-capability.md b/tasks/adapters/review-002-fwd16-missing-capability.md new file mode 100644 index 0000000..439caa6 --- /dev/null +++ b/tasks/adapters/review-002-fwd16-missing-capability.md @@ -0,0 +1,70 @@ +--- +id: review-002-fwd16-missing-capability +name: Loud error on missing capability instead of silent unauthenticated request (FWD-16) +status: pending +depends_on: [] +scope: narrow +risk: low +impact: component +level: implementation +tags: [adapters, review-002, security, from-openapi] +--- + +## Description + +Review 002 FWD-16 [major]. The FWD-08 remediation made every +*malformed* credential fail loudly, but the **missing-capability arm +stays silent**: `forward.rs:200-238` — + +```rust +if let Some(scheme) = auth_scheme { + if let Some(secret) = context.capabilities.get(namespace) { + // ... four loud arms for malformed names/values + } + // capability == None → falls through: the request is sent + // with NO credential and NO diagnostic +} +``` + +`Capabilities::get` keys on `api_key:`/`http_token:` prefixes +(alkcall `core/types.rs:97-107`), so an assembly layer that registered +the secret under a wrong key form (or forgot) produces corrupted +upstream 401s at call time with zero local diagnostics. This is the +*most probable* misconfiguration shape and the one inconsistent arm in +an otherwise-loud match — every other arm was made loud by review-001's +FWD-08 fix (tested at forward.rs:1646-1736). + +## Acceptance Criteria + +- [ ] When `auth_scheme.is_some()` and the capability is absent, the + request is NOT sent; the caller gets an `INTERNAL`-family error + naming the missing capability key (e.g. "capability + `http_token:{namespace}` is absent; refusing to send the request + unauthenticated") — message contains no secret material +- [ ] When `auth_scheme.is_none()` the behavior is unchanged + (unauthenticated ops stay unauthenticated) +- [ ] Wire test: authed op, empty capabilities → error envelope (and + the test asserts via the existing `spawn_responder`-style seam + that the upstream received zero requests) +- [ ] Existing FWD-08 loud-arm tests unchanged and passing +- [ ] Doc note in the module doc: the loud-missing matrix now covers + malformed name/value AND absent capability +- [ ] `cargo test`, `cargo clippy --all-targets -- -D warnings`, + `cargo fmt --check` pass + +## References + +- docs/reviews/002-post-remediation-review.md (Part D', FWD-16) +- src/adapters/forward.rs:200-238 (the silent arm), :1646-1736 (the FWD-08 test family to mirror) +- docs/architecture/decisions/014-secret-material-flow-and-capability-injection.md +- tasks/adapters/review-001-forward-url-safety.md (FWD-08's loud-error work this completes) + +## Notes + +Error class choice: `INTERNAL` (misconfiguration, not caller fault) — +matches the FWD-08 precedent. Non-retryable, since re-sending cannot +succeed without the assembly layer changing. If a deployment legitimately +runs authed-ops with *optionally*-present credentials (none known +today), that would be a new config flag — do not preemptively add one; +the invariant (ADR-014: credentials flow only via Capabilities) argues +loud-and-closed. \ No newline at end of file diff --git a/tasks/adapters/review-002-fwd17-19-contract-decisions.md b/tasks/adapters/review-002-fwd17-19-contract-decisions.md new file mode 100644 index 0000000..2f40f78 --- /dev/null +++ b/tasks/adapters/review-002-fwd17-19-contract-decisions.md @@ -0,0 +1,68 @@ +--- +id: review-002-fwd17-19-contract-decisions +name: Forwarding contract decisions — non-JSON SSE payloads, double-routed placeholders, percent preservation (FWD-17/18/19 decide+document or fix) +status: pending +depends_on: [review-002-fwd15-stream-timeout] +scope: narrow +risk: low +impact: component +level: implementation +tags: [adapters, review-002, from-openapi] +--- + +## Description + +Three review-002 forwarding findings that are design decisions more +than bugs — decide, document, and fix where the decision says so: + +- **FWD-17**: non-JSON SSE payloads silently degrade to JSON strings + (`forward.rs:774-781`, `unwrap_or(Value::String)`) and the parser's + `event:` field is discarded (`:977-979`). A consumer cannot extract + fields from a legitimately non-JSON stream, and an upstream's + SSE `event: error` convention is indistinguishable from data. + Decide: (a) document the JSON-or-string contract as-is, or (b) carry + the raw payload + the `event:` field name in the envelope for + non-JSON payloads (e.g. `{data, event}` object wrapper). +- **FWD-18**: non-scalar path-placeholder values double-route — + rendered into the path (as JSON text) *and* appended as a query + param (`forward.rs:144-157`). Fix the routing rule: a key that + matched a placeholder never also emits as query, regardless of value + shape (plus a spec-decision on whether object/array path values are + an error instead). +- **FWD-19 [info]**: preserved literal `%` in template/base text is + upstream-semantics-changing (`%2F` survives; most stacks route it + differently from `/`). Inputs are assembly-supplied (ADR-066 trust); + document the trade-off in the module doc (+ ADR-066 note) — or + reject raw `%` in *template text* while allowing it in values + (the strict shape). + +## Acceptance Criteria + +- [ ] A decision per finding, recorded (module doc + ADR-066 section + as appropriate); doc-only outcomes still need the doc change + + a doc-asserting test where applicable +- [ ] FWD-17: whichever shape is chosen, non-JSON payload behavior is + pinned by a test (string case, number-vs-string distinction, + event-field presence if option (b)) +- [ ] FWD-18: placeholder keys never double-emit (query test with an + object value under a placeholder key) — and the object-in-path + outcome (error vs JSON-segment) decided and tested +- [ ] FWD-19: documented (and if the reject-raw-% shape is chosen, its + import- or call-time error tested) +- [ ] `cargo test`, `cargo clippy --all-targets -- -D warnings`, + `cargo fmt --check` pass + +## References + +- docs/reviews/002-post-remediation-review.md (Part D', FWD-17/18/19) +- src/adapters/forward.rs:774-781, :977-979, :144-157, :353-370, :481-505 +- docs/architecture/decisions/066-from-jsonschema-as-http-adapter.md +- tasks/adapters/review-002-fwd13-dot-segments.md (adjacent template-renderer work — sequence or coordinate to avoid churn in the same helpers) + +## Notes + +Deliberately a decide-first task: none of the three is a crash or +leak; each is a contract the code half-implies. Keep the decisions +small and documented rather than building speculative machinery (e.g. +no new envelope schema unless (b) is actually chosen). Coordinate with +review-002-fwd13-dot-segments (same helpers, different concerns). \ No newline at end of file diff --git a/tasks/adapters/review-002-import-loudness-cluster.md b/tasks/adapters/review-002-import-loudness-cluster.md new file mode 100644 index 0000000..db3d990 --- /dev/null +++ b/tasks/adapters/review-002-import-loudness-cluster.md @@ -0,0 +1,79 @@ +--- +id: review-002-import-loudness-cluster +name: Import loudness cluster — oneOf body, self-ref requestBody, siblings, collisions, header precedence, error truncation (OAI-10/14/15/17/19, JS-02/03) +status: pending +depends_on: [review-002-oai11-ref-memoization] +scope: moderate +risk: low +impact: component +level: implementation +tags: [adapters, review-002, from-openapi, from-jsonschema] +--- + +## Description + +Six review-002 minor findings across the two import files, all in the +"unsupported-feature loudness + import robustness" family (the OAI-06 +matrix completion). Batched — each is small: + +- **OAI-10**: `$ref` sibling keys are silently discarded + (openapi_spec.rs:396-410). Under 3.1 semantics siblings apply + alongside the `$ref` (advertise/enforce drift); there is no + `openapi: 3.1` version gate anywhere. Fix: warn on `$ref` siblings, + and/or gate `openapi: "3.1*"` like other unsupported features + (at minimum document the 3.0-only stance). +- **OAI-14**: top-level `oneOf` requestBodies (unconstraining `body` + contract — import-time gate), `discriminator`, `xml`, `callbacks`, + `security` blocks silently ignored. Extend the OAI-06 loudness + matrix: reject-or-warn at import with the established message + template. +- **OAI-15**: a self-`$ref`'d requestBody resolves to content-less → + body-less op registered silently (openapi_spec.rs:486-498). Fail + import with the OAI-04 error shape when a resolved requestBody + still contains a top-level `$ref` or lacks `content`. +- **OAI-17**: import errors echo unbounded spec-derived strings + (openapi_spec.rs:264-272 servers locations join; from_openapi.rs + placeholder/path interpolation). Truncate (first N + count) and cap + interpolated path/ref strings in `SchemaParse` messages. +- **OAI-19**: declared `in: header` params silently lose to + `default_headers` and credential headers (forward.rs:169-238 insert + order). Reject or warn at import when a header param collides with + a configured `default_headers` key or the auth scheme's header; + also reject peer-visible header params named `Authorization` on + authed namespaces. +- **JS-02**: `from_openapi` templates are not validated for balanced + braces at import (`/x{open` imports, fails per-call INTERNAL) — + reuse/hoist `from_jsonschema`'s `validate_path_template` into the + shared path and run it in `build_registration`. +- **JS-03**: `assert_eq!` in library code (`reject_collisions`, + from_openapi.rs:74-75) — return `Err(AdapterError::internal(…))` or + zip-iterate and drop the asserts. + +## Acceptance Criteria + +- [ ] Each item landed with its import-time test (loud-error content + asserted; sibling-warning case; header-collision warning/reject; + unterminated-template import failure; no asserts in non-test code) +- [ ] OAI-17: a 100k-path servers-list import error message is + bounded (test with a large synthetic spec asserting message + length < a sane cap) +- [ ] The OAI-06 matrix (or its successor doc section) lists the newly + loud features +- [ ] `cargo test`, `cargo clippy --all-targets -- -D warnings`, + `cargo fmt --check` pass + +## References + +- docs/reviews/002-post-remediation-review.md (Part E', OAI-10/14/15/17/19, JS-02, JS-03) +- src/adapters/openapi_spec.rs:396-410, :486-498, :241-273, :314-320, :264-272, :539-573 +- src/adapters/from_openapi.rs:69-89 (JS-03), :162-171 (the "body" collision pattern to extend for OAI-19), from_jsonschema.rs (the template validator to hoist) +- tasks/adapters/review-001-openapi-loud-degradation.md (the OAI-06 matrix), tasks/adapters/review-001-openapi-import-integrity.md + +## Notes + +Sequence after review-002-oai11-ref-memoization (same resolver +function for OAI-10/15 — or rebase-verify). Slice per finding, commit +per finding. The OAI-19 import-time check may not see the assembly +layer's `default_headers` (config lives elsewhere) — if the +import-time surface cannot know, the loud point moves to +first-call-time (warn-once) — implementer documents which. \ No newline at end of file diff --git a/tasks/adapters/review-002-js01-placeholder-check.md b/tasks/adapters/review-002-js01-placeholder-check.md new file mode 100644 index 0000000..18fd42c --- /dev/null +++ b/tasks/adapters/review-002-js01-placeholder-check.md @@ -0,0 +1,56 @@ +--- +id: review-002-js01-placeholder-check +name: Fix placeholder check skipped when input_schema has no properties (JS-01) +status: pending +depends_on: [] +scope: single +risk: low +impact: component +level: implementation +tags: [adapters, review-002, from-jsonschema] +--- + +## Description + +Review 002 JS-01 [major]. `from_jsonschema`'s eager placeholder-binding +check is skipped when `input_schema` has no `properties` key: +`spec_name_references_undeclared` (`from_jsonschema.rs:166-188`) hits +`if properties.is_empty() { return false; }` at :173-175. So: + +```rust +FromJsonSchema::new(spec_with_input_schema_type_object_no_properties, + path_template = "/widgets/{id}") +``` + +**passes construction** — then every call fails (`{id}` unbound → +INTERNAL at forward.rs:441-446; a peer-supplied `id` key → +INVALID_INPUT). The from_openapi equivalent (`unbound_placeholders`, +from_openapi.rs:36-57) handles the empty case correctly: with no +declared properties, any placeholder is unbound. The existing test +(from_jsonschema.rs:699-724) covers only the properties-present case. + +## Acceptance Criteria + +- [ ] Drop the `is_empty` early return (or return `true` for + placeholders when properties are empty — same semantics as the + from_openapi path) +- [ ] Test: `FromJsonSchema::new` with a no-properties schema + + `{id}` template fails at construction with the placeholder + error; a no-properties schema with a placeholder-free template + still constructs fine +- [ ] `cargo test`, `cargo clippy --all-targets -- -D warnings`, + `cargo fmt --check` pass + +## References + +- docs/reviews/002-post-remediation-review.md (Part E', cluster item JS-01; Test-gap 11) +- src/adapters/from_jsonschema.rs:166-188 (the early return), :77-93 (the construction-time validation), :699-724 (the present-props test) +- src/adapters/from_openapi.rs:36-57 (the correct reference implementation) +- docs/architecture/decisions/066-from-jsonschema-as-http-adapter.md + +## Notes + +One-line fix + one test. Explicitly approved to fold into any commit +that is already touching from_jsonschema.rs (e.g. the wire-tests task) +if a session prefers — but if touched standalone, keep it its own +commit per the small-units convention. \ No newline at end of file diff --git a/tasks/adapters/review-002-mcp-batch-cap.md b/tasks/adapters/review-002-mcp-batch-cap.md new file mode 100644 index 0000000..4df4e01 --- /dev/null +++ b/tasks/adapters/review-002-mcp-batch-cap.md @@ -0,0 +1,46 @@ +--- +id: review-002-mcp-batch-cap +name: Cap MCP batch tool at MAX_BATCH_OPERATIONS (PRJ-22) +status: pending +depends_on: [] +scope: single +risk: low +impact: component +level: implementation +tags: [adapters, review-002, mcp] +--- + +## Description + +Review 002 PRJ-22 [minor]. The MCP `batch` tool has no cap on the +`calls` array (`to_mcp.rs:215-247` executes serially) while HTTP +`/batch` rejects at 101 (`routes.rs:173-184`, +`MAX_BATCH_OPERATIONS`). A 10,000-entry MCP batch occupies the +dispatch spine serially (each invoke up to the 30 s deadline) — the +doc (PRJ-09's fix) advertises no limit either. + +## Acceptance Criteria + +- [ ] The `batch` tool enforces the same `MAX_BATCH_OPERATIONS` cap + (share the constant; do not duplicate the literal) — over-cap → + structured `INVALID_INPUT`-family error consistent with the + tool's other argument errors (including `retryable`, per PRJ-13) +- [ ] Tool description states the limit (mirroring how `/batch` docs + it) — ADR-041 shape unchanged (4 fixed tools) +- [ ] Test: over-cap `calls` array → structured error, no dispatch + happened (spy/gateway-count assert via the existing invoke_tool + harness) +- [ ] `cargo test --features mcp`, `cargo clippy --all-features + --all-targets -- -D warnings`, `cargo fmt --check` pass + +## References + +- docs/reviews/002-post-remediation-review.md (Part F', PRJ-22) +- src/adapters/to_mcp.rs:215-247 (the loop), src/gateway/routes.rs:42,173-184 (the cap to share) +- tasks/adapters/review-001-mcp-tool-fidelity.md (the PRJ-07..10/13 work whose conventions to follow) + +## Notes + +One of the smallest tasks in the batch — a good first warm-up slice or +fold-in candidate if a session runs short (but keep the commit +separate; it is its own unit of work). \ No newline at end of file diff --git a/tasks/adapters/review-002-oai11-ref-memoization.md b/tasks/adapters/review-002-oai11-ref-memoization.md new file mode 100644 index 0000000..cf5f645 --- /dev/null +++ b/tasks/adapters/review-002-oai11-ref-memoization.md @@ -0,0 +1,70 @@ +--- +id: review-002-oai11-ref-memoization +name: Memoize $ref expansion to bound resolver work (OAI-11) +status: pending +depends_on: [] +scope: moderate +risk: medium +impact: component +level: implementation +tags: [adapters, review-002, from-openapi, openapi-spec] +--- + +## Description + +Review 002 OAI-11 [major, borderline critical]. The OAI-01 fix +(cycle guard + depth budget, `openapi_spec.rs:382-437`) bounds *stack*, +not *work*: every `$ref` hop re-expands its target from a fresh clone +with no memoization. A chain of shared but acyclic refs — + +``` +S_i = { a: { $ref: "S_{i+1}" }, b: { $ref: "S_{i+1}" } } +``` + +(the doubly-linked-list `prev`/`next` shape — valid, common) expands as +a full binary tree. Empirically (verbatim harness): 20 levels → 7.3M +node visits / 3.6 s; growth ≈ 2^levels → 30 levels ≈ ~1 hour, 40 ≈ days. +Neither guard fires (`err=false`); the import just wedges — no stack +overflow, no error. The same "valid document kills the process" class +as OAI-01, one mitigation shy. + +## Acceptance Criteria + +- [ ] `resolve_refs_bounded` memoizes resolved, cycle-free `$ref` + targets per document (cache keyed on the ref string; clone the + resolved `Value` on hit instead of re-resolving) — or an + equivalent total-node-count budget with a clean + `SchemaParse`-family error (implementer's choice; memoization + strongly preferred — it preserves full acyclic support) +- [ ] The cycle guard and both depth checks remain (memoization must + not alter cycle detection semantics — a cycle still errors) +- [ ] Acceptance gate: a 30+ level shared-chain spec imports in + bounded time (add the test with a wall-clock or node-count + assert; a 30-level chain must import in < 1 s) +- [ ] Tests: diamond (non-cyclic shared refs) unchanged-correct; + cycle-through-shared-node still errors; memoized result equals + the old expansion for a shared-ref schema (golden compare on one + case) +- [ ] The four missing cycle-shape tests from OAI-16 (array-items, + allOf, additionalProperties, `$ref`-sibling) land here too — + same function, same risk envelope +- [ ] `cargo test`, `cargo clippy --all-targets -- -D warnings`, + `cargo fmt --check` pass + +## References + +- docs/reviews/002-post-remediation-review.md (Part E', OAI-11 + OAI-16) +- src/adapters/openapi_spec.rs:382-437 (the resolver), :713-805 (existing cycle tests), from_openapi.rs:736 (the requestBody-chain test) +- tasks/adapters/review-001-ref-cycle-guard.md (the OAI-01 fix this completes) +- docs/architecture/decisions/066-from-jsonschema-as-http-adapter.md + +## Notes + +Clone cost per hit is fine (schemas are small; the win is avoiding +exponential re-resolution, not allocation). If memoizing, note the +`resolving`-set interaction: a memo entry must only be written for +*completed* (cycle-free) expansions — a partial expansion under an +active cycle attempt must never be cached (write-on-success at the +top of the recursion return path, or post-order). This is the one +subtle part; the existing cycle tests + the four new shape tests are +the safety net. \ No newline at end of file diff --git a/tasks/adapters/review-002-oai13-path-item-wildcards.md b/tasks/adapters/review-002-oai13-path-item-wildcards.md new file mode 100644 index 0000000..fa16b94 --- /dev/null +++ b/tasks/adapters/review-002-oai13-path-item-wildcards.md @@ -0,0 +1,70 @@ +--- +id: review-002-oai13-path-item-wildcards +name: Path-item parameters, 2XX/4XX/5XX wildcards, webhooks handling (OAI-13) +status: pending +depends_on: [review-002-oai11-ref-memoization] +scope: moderate +risk: medium +impact: component +level: implementation +tags: [adapters, review-002, from-openapi, openapi-spec] +--- + +## Description + +Review 002 OAI-13 [major]. Three common real-world OpenAPI shapes are +still silently mishandled (verified at `openapi_spec.rs:276-336` + +`from_openapi.rs:136-145`): + +1. **Path-item-level `parameters`** (shared params declared next to + the path key — extremely common in real specs) never merge into + operations → `{id}` unbound → the whole import fails with a + *misleading* diagnosis (the skip filter at :314-320 explicitly + whitelists `"parameters"` out of the unsupported-methods warning, + so nothing names the cause). +2. **Response wildcard keys**: `"2XX"` is missed by the SSE-detection + success sweep (an SSE stream under `2XX` imports as a giant-text + Mutation — the exact OAI-06 misbehavior, one spelling away); + `"4XX"/"5XX"` error keys drop silently with the generic warn (the + test at from_openapi.rs:840 enshrines silent `5XX` dropping). +3. **Top-level `webhooks`** silently vanish from mixed documents. + +## Acceptance Criteria + +- [ ] Path-item `parameters` merge into each operation under the path + (operation-level entries override per OpenAPI spec semantics); + the import no longer fails with "unbound placeholder" for + shared-param specs +- [ ] Success sweep accepts `"2XX"` (SSE detection + output schema) + and error sweep accepts `"4XX"/"5XX"` (`HTTP_`-class mapping per + the wildcard's implied status range — choose the mapping, e.g. + 4XX → the strongest available code, and document it) +- [ ] `webhooks`: import (as Mutations/Webhook-kind ops or a decided + visibility) or reject the key loudly — pick per ADR-066's + single-endpoint adapter philosophy and record the decision +- [ ] The misleading-diagnosis fix: when import fails after skipping + path-item parameters, the error names the actual cause (the + skip-filter whitelist must not hide the parameter key from the + unsupported-features warning) +- [ ] Tests: shared-path-params spec imports correctly (incl. + operation-override precedence); `2XX`-declared SSE imports as + Sub; `5XX`-declared error lands in error schemas; webhooks-only + and mixed docs behave per the decision +- [ ] `cargo test`, `cargo clippy --all-targets -- -D warnings`, + `cargo fmt --check` pass + +## References + +- docs/reviews/002-post-remediation-review.md (Part E', OAI-13; Test-gaps: Petstore-with-shared-params) +- src/adapters/openapi_spec.rs:276-336 (method sweep + skip filter), from_openapi.rs:131-151 (success sweep), :248-270 (error sweep), :840 (the enshrining test) +- docs/architecture/decisions/066-from-jsonschema-as-http-adapter.md, /051-yaml-input-for-from-openapi.md +- tasks/adapters/review-001-openapi-loud-degradation.md (the OAI-06 matrix this extends) + +## Notes + +Slice suggestion: (1) path-item parameters + diagnosis fix, (2) +wildcard sweeps, (3) webhooks decision. The wildcard-to-error-status +mapping is the one judgment call worth a line in ADR-066 (e.g. "4XX +wildcards project to their range's first legal HTTP_ code per error +dedup rules"). Sequence after review-002-oai11-ref-memoization (same +file, resolver shape changes first) — or rebase-verify. \ No newline at end of file diff --git a/tasks/adapters/review-002-projection-truthfulness.md b/tasks/adapters/review-002-projection-truthfulness.md new file mode 100644 index 0000000..bcc3b18 --- /dev/null +++ b/tasks/adapters/review-002-projection-truthfulness.md @@ -0,0 +1,88 @@ +--- +id: review-002-projection-truthfulness +name: Projection doc truthfulness — dangling CallError ref, status clobber/drifts, OAS-invalid extension (PRJ-16b/17/18/19/20/21/23/24) +status: pending +depends_on: [] +scope: moderate +risk: low +impact: component +level: implementation +tags: [adapters, review-002, to-openapi] +--- + +## Description + +Eight review-002 findings in `to_openapi.rs` (+ one in `to_mcp.rs`), +all "the generated document must be runtime-truthful and +OAS-valid" — the PRJ-01..15 remediation's remaining tail: + +- **PRJ-16b [major]**: `BatchResultEntry.error` refs + `#/components/schemas/CallError` which does not exist + (to_openapi.rs:602 vs components at :387-413 defines `CallFailure` + + per-code variants). The openapiv3 parse test runs on an **empty + registry** so nothing trips. Generated clients/validators fail to + resolve. +- **PRJ-17**: an op declaring `HTTP_404@404` clobbers the shared + NotFound response on `/call` (:694/:237-239/:295-300) — losing + `NOT_FOUND` which the runtime still emits; every real 404 then + violates the documented schema. Merge op codes into the shared + response (oneOf append) instead of overwriting. +- **PRJ-18**: `/publish` 400 over-declares `INVALID_OPERATION_TYPE`; + runtime maps that condition to 401 without a token (the 401 entry in + the same doc is the true one) (:268-277). +- **PRJ-19**: extractor plain-text 415 (missing content-type) and + data-error 422 (JsonDataError) undocumented on `/call`, `/batch`, + `/subscribe` (axum 0.8 emits them; the module doc admits + "plain-text 400/415/422") (:673-682). +- **PRJ-20**: `/call` 401 under-declares — unauthenticated Sub/Pub + call → 401 with `INVALID_OPERATION_TYPE`, absent from the enum + (:218-219 vs error.rs:67-73; `/publish` got this split right). +- **PRJ-21**: `/batch` documents a 500 the runtime never emits (all + dispatch failures are in-band entries) (:329-334). +- **PRJ-23**: `x-operation-error-statuses` is emitted *inside* + `components.schemas` (:414-417) — extension keys are legal on + `components`, not as a schema name; OAS-invalid on any registry with + op errors. Move to `components` level or delete (nothing consumes + it). +- **PRJ-24**: dead `search_filter` parameter in `to_mcp::handle_batch` + (computed then `let _ =` discarded) — remove. + +## Acceptance Criteria + +- [ ] PRJ-16b: `CallError` defined (generic code/message/retryable/ + details) OR `error` points at existing per-code components via + oneOf; the openapiv3 parse test runs on a **populated** registry + (with op errors — exercising PRJ-23's key too) +- [ ] PRJ-17: protocol-status op errors merge into the shared response + (oneOf append); test with `HTTP_404@404` asserting `NOT_FOUND` + survives alongside +- [ ] PRJ-18/20: 400 and 401 oneOf lists match error.rs's actual + mapping (drop IOT from publish-400; add IOT to call-401) — + conformance tests for both bodies +- [ ] PRJ-19: 415 + plain-text 422 slots documented (extend + `plain_text_extractor_rejection` or an equivalent shared + component) +- [ ] PRJ-21: unreachable /batch 500 removed or marked reserved +- [ ] PRJ-23: extension key legal placement (or deleted); parse test + covers the with-errors path +- [ ] PRJ-24: dead parameter removed from `handle_batch` +- [ ] Golden-doc tests updated (byte-identical assertion refreshed); + `cargo test --all-features`, `cargo clippy --all-features + --all-targets -- -D warnings`, `cargo fmt --check` pass + +## References + +- docs/reviews/002-post-remediation-review.md (Part F', PRJ-16b/17/18/19/20/21/23/24; Test-gaps 5/6) +- src/adapters/to_openapi.rs:387-433 (components), :602 (dangling ref), :218-300 (status oneOfs), :414-417 (extension), :673-682 (extractor text) +- src/gateway/error.rs:57-81 (the runtime mapping these must match), routes.rs (the runtime truth) +- tasks/adapters/review-001-openapi-projection-fidelity.md (the PRJ-01..15 work this completes) + +## Notes + +The review-001 lesson applies again: every fix here needs its test to +run against a populated registry (the empty-registry parse test is how +PRJ-16b and PRJ-23 escaped). Consider one shared +`golden_doc_with_ops()` fixture replacing per-test registry setup. +ADR-045's version bump: these are doc-contract corrections — bump +`info.version` per ADR-045's tracking rule (gateway endpoint contract +itself unchanged). \ No newline at end of file diff --git a/tasks/adapters/review-002-yaml-normalization.md b/tasks/adapters/review-002-yaml-normalization.md new file mode 100644 index 0000000..d8f05e1 --- /dev/null +++ b/tasks/adapters/review-002-yaml-normalization.md @@ -0,0 +1,69 @@ +--- +id: review-002-yaml-normalization +name: YAML input normalization — duplicates, .inf, merge keys, non-string keys (OAI-12) +status: pending +depends_on: [] +scope: narrow +risk: medium +impact: component +level: implementation +tags: [adapters, review-002, from-openapi, openapi-spec] +--- + +## Description + +Review 002 OAI-12 [major]. `OpenAPISpec::from_yaml` +(`openapi_spec.rs:153-158`) is a single `yaml_serde::from_str::` +with zero post-parse normalization; three verified corruptions flow +through unimpeded (empirically confirmed against yaml_serde 0.10.7): + +1. **Duplicate keys silently last-win** on the Value path + (`visit_map → values.insert`) while the JSON path *errors* on the + same document — the two input formats disagree, YAML's failure + mode is silent. +2. **`.inf`/`.nan` numbers become `Value::Null`** through + `Number::from_f64(∞) → None` — a declared constraint (`maximum: + .inf` is unusual, but any float inf) silently vanishes from the + advertised schema. +3. **Merge keys (`<<: *anchor`) are not applied** (yaml_serde's + `apply_merge()` is opt-in and unused) — `<<` survives as a literal + property name in the resolved/advertised schema. + +Non-string keys (`200:` response codes as numbers etc.) were checked — +plain statuses survive via string round-trips — but numeric inference +into any string-matched field is a live hazard (e.g. `style: 1`). + +## Acceptance Criteria + +- [ ] A post-parse normalization pass on the YAML path (before + `from_value`): reject duplicate keys (loud, naming the path or + at least the document context), reject `.inf/.nan`-derived + nulls-from-floats (either reject or reject-with-context), and + decide merge keys: either invoke `apply_merge()` (matching YAML + 1.1 user expectations; ADR-051 declares YAML 1.2 via yaml_serde — + verify which the crate claims) or reject `<<` keys loudly +- [ ] Non-string keys: reject or coerce-with-loudness (decide; JSON + path parity is the goal — the same document should mean the same + thing through both entry points or the difference must be loud) +- [ ] Tests at the `from_yaml` seam (not the parser): duplicate keys → + error; `.inf` → loud error (not silent null); merge key applied + or rejected; quoted-always spec (`"200":`) unchanged +- [ ] A doc note on the YAML/JSON parity contract (ADR-051 section or + the module doc) +- [ ] `cargo test`, `cargo clippy --all-targets -- -D warnings`, + `cargo fmt --check` pass + +## References + +- docs/reviews/002-post-remediation-review.md (Part E', OAI-12; Test-gap 9) +- src/adapters/openapi_spec.rs:153-158 (the seam), from_openapi.rs (the consumers) +- docs/architecture/decisions/051-yaml-input-for-from-openapi.md +- tasks/adapters/review-001-openapi-import-integrity.md (the import-integrity context) + +## Notes + +Keep the walk cheap (one pass, no allocation beyond error messages). +The alias-bomb/depth bounds are dependency-provided and verified +working (review 002 re-verified) — do not re-implement them; the task +is about *semantic* normalization, not resource limits. ADR-051 is the +decision record to update if the merge-key stance changes behavior. \ No newline at end of file diff --git a/tasks/client/review-002-cli01-retry-after-budget.md b/tasks/client/review-002-cli01-retry-after-budget.md new file mode 100644 index 0000000..07f6fbd --- /dev/null +++ b/tasks/client/review-002-cli01-retry-after-budget.md @@ -0,0 +1,70 @@ +--- +id: review-002-cli01-retry-after-budget +name: Budget-aware Retry-After sleep — stop re-arming a full ceiling per attempt (CLI-01) +status: pending +depends_on: [] +scope: narrow +risk: medium +impact: component +level: implementation +tags: [client, review-002] +--- + +## Description + +Review 002 CLI-01 [major]. The `Retry-After` middleware sleeps outside +the retry budget and re-arms a full ceiling on every throttled retry — +middleware order is `RetryGateMiddleware` **outside** +`RetryAfterMiddleware` (`http_client.rs:511-520`), so every retry +attempt re-enters `RetryAfterMiddleware::handle`: + +1. attempt 1 → `429` + `Retry-After: 300` → deadline recorded `t₀+300` +2. reqwest-retry sleeps ≤ 2 s, retries +3. attempt 2 re-sleeps until `t₀+300` (inside maybe_sleep_for), gets a + fresh `429` → deadline **re-armed to `t₀+302`** (a full new 300 s) +4. … for up to `max_retries` more → ~15 min wall time inside one + `forward()` call + +`TotalRetryBudget` gates only its own backoff sleeps; the 30 s request +timeout never covers this window (the sleep happens before the reqwest +request is constructed, so the timer never starts). + +## Acceptance Criteria + +- [ ] `maybe_sleep_for` is budget-aware: caps the sleep by the + remaining `max_total_retry_duration` (check inside the sleep — + the pending sleeps, clamped, or checks-and-skips when the budget + is spent; implementer's choice, but a deadline that would extend + past the budget must be truncated to it) +- [ ] Re-arm clamp: `record_if_throttled` keeps the *earliest* deadline + (or otherwise does not extend past the first-seen deadline for + the same URL within one logical request) — a retry storm cannot + push the wall clock out ceiling-per-attempt +- [ ] Wire test: counting responder that always answers + `429`/`Retry-After: N` with the test's ceiling set small → the + caller's total wall time is bounded by max_total_retry_duration + + one attempt's request time (assert the bound, not the exact + count) +- [ ] The per-URL deadline map semantics for *separate* logical + requests are unchanged (a fresh request still honors the + recorded throttle window — that feature stays) +- [ ] Fix the misspelled test name `malware_records_under_the_effective_url` while touching the file +- [ ] `cargo test`, `cargo clippy --all-targets -- -D warnings`, + `cargo fmt --check` pass + +## References + +- docs/reviews/002-post-remediation-review.md (Part D', CLI-01) +- src/client/http_client.rs:511-520 (stack order), 370-395 (TotalRetryBudget), src/client/retry_after.rs:44-62 (ceiling), :127-211 (record/maybe_sleep) +- tasks/client/review-001-client-timeout-retry.md (the Retry-After machinery) +- tasks/client/review-002-client-policy-wire-tests.md (the counting-responder seam this test family shares) + +## Notes + +Keep the semantics decision narrow: the goal is "one logical request's +Retry-After waits are bounded by the retry budget," NOT changing the +cross-request throttle map (FWD-11's eviction/jitter behavior is +already fixed and correct). The natural seam is threading the budget +handle into the middleware order so RetryAfter sees it — small, +testable, no public API change (the budget is already a +`HttpClientConfig` field). \ No newline at end of file diff --git a/tasks/client/review-002-client-policy-wire-tests.md b/tasks/client/review-002-client-policy-wire-tests.md new file mode 100644 index 0000000..2bc8d28 --- /dev/null +++ b/tasks/client/review-002-client-policy-wire-tests.md @@ -0,0 +1,81 @@ +--- +id: review-002-client-policy-wire-tests +name: Wire tests — redirect policy, retry gate/budget, streaming terminal arms, TLS error arms (CLI-02/03, COV-10/11) +status: pending +depends_on: [review-002-fwd15-stream-timeout, review-002-cli01-retry-after-budget] +scope: moderate +risk: low +impact: component +level: implementation +tags: [client, review-002, coverage, adapters] +--- + +## Description + +The crate's highest-value coverage gap: the outbound client's policy +stack — the code that protects credentials — has never run against a +redirect or a retry on the wire, and `forward_stream`'s terminal arms +are dark end-to-end (review 002 COV-11: `same_host_redirect_policy` +100% dark; COV-10: SSE terminal arms dark). Concretely: + +- **CLI-02 (redirect)**: no test in the crate sends any 3xx. Add: + (a) same-host 302 → followed, credential headers forwarded; + (b) cross-host 302 → response surfaced (HTTP_302-family), the second + endpoint receives **zero requests** (count at the attacker server) — + the FWD-03 property, pinned. Hop-cap (>10) → loud error, third test. + The `spawn_responder` seam at forward.rs:1415-1477 already speaks + raw HTTP and can emit `location:`. +- **CLI-03 (retry)**: counting-responder tests: POST + 500 ⇒ exactly + 1 upstream hit (method gate, :362-366); GET + 500×2 ⇒ 3 hits then + success; budget exhaustion (`max_total_retry_duration` small) stops + retries even under a generous attempt cap (:381); PEM + parse-failure arms (:487-507) via garbage-PEM files asserting + `CaBundleParse`/`ClientCertParse` + path (never key material). +- **COV-10 (streaming/forward terminal arms)**: (a) upstream SSE line + > 1 MiB without newline through the real `forward_stream` → single + INTERNAL terminal envelope, stream ends; (b) responder sends + `data: a\n\n` then aborts the socket → terminal error envelope; + (c) responder ends without trailing blank line → pending event + dispatched at EOF; (d) dead-port transport failure through both + `forward` (:719-722) and `forward_stream` (:851-853) → INTERNAL + envelope; (e) binary `application/octet-stream` 200 → byte-array + envelope (:638-643); (f) `application/json` 200 with malformed body + → INTERNAL decode envelope (:656-658). +- Plus the small dark arms from COV-12 that live in this seam: + ApiKey/Basic malformed-credential loud arms (extend the FWD-08 test + family), declared-header-param invalid name/value rejections, and + `SharedHttpClient::config()` reflecting a reloaded config + (FWD-12's atomicity assertion). + +## Acceptance Criteria + +- [ ] Redirect: 3 tests (same-host follow, cross-host refuse + + zero-contact assert, hop-cap error) passing +- [ ] Retry: 3 tests (POST single-hit, GET retry-to-success with + counted hits, budget-bounded) passing +- [ ] TLS: 2 parse-failure tests (CaBundleParse, ClientCertParse) + + config()-after-reload assertion +- [ ] Streaming: 6 terminal-arm tests (oversized line, socket abort, + EOF-flush, dead-port×2, binary decode, malformed-JSON decode) + passing +- [ ] Credential arms: ApiKey/Basic malformed-value tests land +- [ ] `cargo test --all-features`, `cargo clippy --all-features + --all-targets -- -D warnings`, `cargo fmt --check` pass + +## References + +- docs/reviews/002-post-remediation-review.md (Part D', CLI-02/03; Part G', COV-09/10/11/12) +- src/client/http_client.rs:300-316 (redirect policy), :354-395 (retry gate/budget), :487-507 (PEM arms) +- src/adapters/forward.rs:1415-1477 (spawn_responder seam), :887-925 (stream terminal arms) +- tests/client_tls.rs (the TLS harness pattern) +- tasks/client/review-002-fwd15-stream-timeout.md and review-002-cli01-retry-after-budget.md (land first — these tests pin their new behavior too) + +## Notes + +Pure test-addition task (plus any test-seam helpers). Land after the +two client behavior tasks so the tests assert final semantics, not +interim ones. Slice: (1) redirect tests, (2) retry tests, (3) +streaming terminal arms, (4) TLS + config arms — independent commits. +If `spawn_responder` needs a "count requests per host" surface, build +it once and reuse — the cross-host zero-contact assertion is the +load-bearing one. \ No newline at end of file diff --git a/tasks/client/review-002-fwd15-stream-timeout.md b/tasks/client/review-002-fwd15-stream-timeout.md new file mode 100644 index 0000000..97575ba --- /dev/null +++ b/tasks/client/review-002-fwd15-stream-timeout.md @@ -0,0 +1,73 @@ +--- +id: review-002-fwd15-stream-timeout +name: Subscriptions must not inherit the 30s total request timeout (FWD-15) + stream byte cap (FWD-14) +status: pending +depends_on: [] +scope: moderate +risk: medium +impact: component +level: implementation +tags: [client, review-002, adapters] +--- + +## Description + +Review 002 FWD-15 [major] + FWD-14. Two streaming-path defects, same +code region (`src/adapters/forward.rs:824-934`): + +1. **FWD-15**: `forward_stream` sends through the shared client whose + `DEFAULT_REQUEST_TIMEOUT` is 30 s — verified against reqwest 0.13.4 + that the total-timeout sleep rides into the response body stream. + A healthy subscription dies at 30 s with `SSE stream error: + operation timed out` — even keep-alive-emitting sources (the + gateway deliberately runs subscriptions unbounded per ADR-021; + dispatch.rs:28). The outbound half of the same subscription is + mortally capped. +2. **FWD-14**: the streaming branch has no size accounting — the 1 MiB + cap bounds a single SSE *line*, not the stream. A hostile upstream + emits well-formed 1-MiB-line events forever: 30 s of those is + ~GB/s into envelope allocation (mitigated only by the timeout — + which FWD-15's fix then removes, so **both must land together or + the fix makes FWD-14 worse**). + +## Acceptance Criteria + +- [ ] FWD-15: `HandlerKind::Stream` forwards send with + `request_timeout: None` while keeping connect + read timeouts + (implementer's choice: per-request extension override verified + against reqwest 0.13, or a second derived client built from the + same config minus the total timeout) — a >30 s healthy + subscription survives +- [ ] FWD-15 wire test: responder trickling `: keepalive` comments past + 30 s (scaled: use a configurable short client timeout in the + test) → stream still delivering events after the old deadline +- [ ] FWD-14: a total byte cap on the streaming path (accumulate + across `feed`; per-subscription total, default documented) and + the line-cap check moved to *before* `extend_from_slice` so the + overshoot cannot exceed cap + one chunk +- [ ] FWD-14 test: a stream exceeding the total cap terminates with a + single terminal error envelope (the stream-ends semantics) +- [ ] Cap value + no-timeout decision documented in the module doc and + http-server.md (ADR-049/021 note: unbounded *time* by design for + subscriptions, bounded *bytes* per subscription) +- [ ] `cargo test`, `cargo clippy --all-targets -- -D warnings`, + `cargo fmt --check` pass + +## References + +- docs/reviews/002-post-remediation-review.md (Part D', FWD-14, FWD-15; Unit 2 rationale) +- src/adapters/forward.rs:824-934 (send + streaming branch), :969-1082 (SSE parser), http_client.rs:69-95/:149-150/:164-178 (timeout config + read-timeout doc claim) +- docs/architecture/decisions/049-streaming-handler-for-subscriptions.md +- tasks/client/review-001-client-timeout-retry.md (the timeout defaults this revises) + +## Notes + +Order matters **within** this task: land FWD-15 with FWD-14 in the same +commit-series — removing the total timeout without the byte cap opens +an unbounded-memory window. The read-timeout (10/30 s stall guard) +stays: it bounds upstream *staleness*; the doc at http_client.rs:149-150 +already correctly describes read-timeout as the stall guard (the total +timeout's "stalled upstream" claim there is the thing being fixed). +Coordinate with review-002-cli01-retry-after-budget (same file) and +review-002-client-policy-wire-tests (which should follow this to test +the new behavior) — sequence those. \ No newline at end of file diff --git a/tasks/gateway/review-002-gw15-publish-body-cap.md b/tasks/gateway/review-002-gw15-publish-body-cap.md new file mode 100644 index 0000000..3eb5e2e --- /dev/null +++ b/tasks/gateway/review-002-gw15-publish-body-cap.md @@ -0,0 +1,77 @@ +--- +id: review-002-gw15-publish-body-cap +name: Cap /publish body buffering pre-newline + explicit body-limit layer (GW-15) +status: pending +depends_on: [] +scope: moderate +risk: medium +impact: component +level: implementation +tags: [gateway, review-002, security] +--- + +## Description + +Review 002 GW-15 [major] — `/publish` lost both claimed memory bounds +in the GW-06 streaming rewrite. Verified at tree `91483a7`: + +1. **No body limit on the route.** `publish_handler` takes raw + `axum::body::Body` (`src/gateway/routes.rs:249-253`). axum's + 2 MiB `DefaultBodyLimit` is a request *extension* consulted only by + extractors (axum-core `request_parts.rs` returns the raw body + untouched) — a raw-Body handler has **no** default limit. The + module doc's claim ("in addition to axum's own 2 MiB default body + limit") is false on this route. +2. **Unbounded line buffer.** `BufferedLines::next_line` + (`routes.rs:425-448`) enforces `MAX_PUBLISH_LINE_BYTES` only when a + `\n` is found; bytes accumulate in `self.buffer` unboundedly until + then, and the trailing-EOF `std::mem::take(&mut self.buffer)` path + has no cap check at all. + +Concrete failure: unauthenticated `POST /publish` with chunked +transfer-encoding streaming `'a'` forever (no newline) → heap grows +with the upload until OOM; each poll also re-scans the whole buffer +from index 0 → O(n²) CPU on top. This is the SRV-03 class one route +over (SRV-03 fixed `/mcp`; `/publish` regressed into the same class +via the streaming rewrite). + +## Acceptance Criteria + +- [ ] `BufferedLines` errors (terminal `LineError::LineCap` → the same + INVALID_INPUT family the line-cap error uses today) once + `self.buffer.len() > MAX_PUBLISH_LINE_BYTES`, checked *before* + extending, even with no newline seen — the trailing-EOF + `mem::take` path cannot exceed the cap +- [ ] The publish route (or the whole gateway router) carries an + explicit request-body-limit layer so it does not depend on + extractor-side defaults for raw-Body handlers (size = the 2 MiB + convention; note in module doc) +- [ ] Wire test: chunked upload with no `"\n"` for > cap bytes is + rejected with the documented status/body (not 200, not OOM) +- [ ] Wire test: EOF without newline after > cap buffered bytes also + rejected (the `mem::take` path) +- [ ] Existing publish tests still pass (first-line handling, NDJSON + streaming, schema validation, caps on terminated lines) +- [ ] `cargo test`, `cargo clippy --all-targets -- -D warnings`, + `cargo fmt --check` pass + +## References + +- docs/reviews/002-post-remediation-review.md (Part C', GW-15; Part G', COV-12 `BufferedLines` hot spot) +- docs/architecture/decisions/068-gateway-publish-endpoint.md +- src/gateway/routes.rs:249-253 (handler), 395-470 (BufferedLines) +- tasks/gateway/review-001-publish-schema-validation-robust.md (the cache this must not stale) + +## Notes + +The line-cap test that exists today (`publish_line_exceeding_cap…`) +short-circuits at the route level in single-chunk delivery — the new +pre-extend check must be exercised with a **streamed** (multi-chunk, +never-newline) body, which is the shape the old test cannot see. Keep +`MAX_PUBLISH_LINE_BYTES` as the single cap constant; a separate total +cap is optional (the layer limit bounds the total once landed). + +GW-16 (the 400-vs-422 drift on the cap error's status) is deliberately +NOT in scope here — see review-002-gw16-status-drift; land the cap with +today's status first, then normalize statuses in that task to avoid +churn in the same lines. \ No newline at end of file diff --git a/tasks/gateway/review-002-gw16-status-drift.md b/tasks/gateway/review-002-gw16-status-drift.md new file mode 100644 index 0000000..c4eb206 --- /dev/null +++ b/tasks/gateway/review-002-gw16-status-drift.md @@ -0,0 +1,74 @@ +--- +id: review-002-gw16-status-drift +name: Unify INVALID_INPUT status on hand-rolled publish/batch paths + sink-deadline decision (GW-16, GW-17) +status: pending +depends_on: [review-002-gw15-publish-body-cap] +scope: narrow +risk: low +impact: component +level: implementation +tags: [gateway, review-002] +--- + +## Description + +Two review-002 gateway status/deadline findings, same files +(`routes.rs`, `dispatch.rs`), sequenced after the GW-15 body-cap task +because GW-15 adds one more hand-rolled status site: + +- **GW-16**: the documented mapping is `INVALID_INPUT → 422` + (http-server.md:361), and mid-stream publish validation correctly + emits 422. But the new pre-dispatch paths hand-build + `CallError::invalid_input`-shaped responses with + `StatusCode::BAD_REQUEST`: empty body, missing `operation`, missing + `chunk`, invalid first-line JSON (:257-278), the line-cap error + (:459-464), and the `/batch` over-cap reject (:173-184). Same error + class, two statuses depending on where it fires — the GW-03 drift + class one table row down. +- **GW-17**: the documented 30 s deadline includes "the `/publish` + final envelope" (http-server.md:381-384) but `invoke_sink` has no + timeout (`dispatch.rs:132-145`) — the module doc justifies it as + "bounded by the client's upload," which bounds chunk arrival, not + the handler's final await. A hung sink handler parks the HTTP + connection forever. + +## Acceptance Criteria + +- [ ] Decide + implement the body-shape-fault status: either (a) route + all hand-rolled `INVALID_INPUT` responses through + `call_error_to_http_response_with_identity` (making them 422 + like the documented mapping), or (b) sanction 400-for-body-shape + in http-server.md's table explicitly (documenting which paths + are "request shape" vs "per-chunk shape"). Implementer's choice; + whichever it is, `/publish` and `/batch` agree and the doc table + names both statuses' trigger conditions. +- [ ] GW-17: decide + implement: (a) wrap the sink handler's + completion in the same 30 s `tokio::time::timeout` family the + Once-op invoke uses (new error envelope on trip), or (b) amend + http-server.md to exclude the final envelope from the deadline + contract. Prefer (a) — the documented contract is the older + decision and hung-forever requests are the failure mode review + 001 filed twice. +- [ ] Tests: each hand-rolled path's status asserted (publish + first-line/empty/line-cap/batch-cap); sink-deadline test with a + hung sink handler (mirroring dispatch.rs's hung-handler test) + if (a) +- [ ] http-server.md error/deadline tables updated to match the + implemented choice +- [ ] `cargo test`, `cargo clippy --all-targets -- -D warnings`, + `cargo fmt --check` pass + +## References + +- docs/reviews/002-post-remediation-review.md (Part C', GW-16, GW-17) +- src/gateway/routes.rs:173-184, 257-278, 459-464 (the hand-rolled 400s), dispatch.rs:132-145 (the sink await), error.rs:51 (the 422 mapping) +- docs/architecture/http-server.md:361 (the error table), :381-388 (the deadline contract) +- tasks/gateway/review-002-gw15-publish-body-cap.md (lands the line-cap error this task then normalizes) + +## Notes + +Do this after GW-15 so the line-cap path's final status is chosen once +(avoid two commits churning the same lines). ADR-023 governs the +error-code discipline — if (b) is chosen for GW-16, note the +body-shape-vs-chunk-shape distinction there or in http-server.md so +the next projection refresh captures it. \ No newline at end of file diff --git a/tasks/gateway/review-002-prj16-schema-via-call.md b/tasks/gateway/review-002-prj16-schema-via-call.md new file mode 100644 index 0000000..d1a6e72 --- /dev/null +++ b/tasks/gateway/review-002-prj16-schema-via-call.md @@ -0,0 +1,76 @@ +--- +id: review-002-prj16-schema-via-call +name: Block Internal-op schema disclosure via services/schema over /call (PRJ-16, cross-crate CF-004) +status: pending +depends_on: [] +scope: moderate +risk: medium +impact: project +level: implementation +tags: [gateway, security, review-002, cross-crate] +--- + +## Description + +Review 002 PRJ-16 [major, security]. Internal/ACL-restricted op specs +are readable through the *op* path even though the GET `/schema` route +was fixed in review-001 (SRV-02): + +- `call_handler` (`src/gateway/routes.rs:120-133`) pre-checks only the + **outer** op name — `services/schema` is External with default ACL, + so the pre-check passes. +- `registry.invoke` dispatches to alkcall's `services_schema_handler` + (`alkcall src/registry/discovery.rs:327-343`), which does a bare + `registry.registration(name)` → `spec_to_json` with **no visibility + and no AccessControl check**. + +`POST /call {"operation":"services/schema","input":{"name":"secret/op"}}` +→ the full spec (input/output/error schemas + required_scopes) of any +Internal op, unauthenticated. Same hole via the MCP `call` tool. The +regenerated to_openapi doc asserts "Internal (hidden from HTTP +discovery, GW-02)" — currently false at runtime. The alkcall-side +instance is filed as **CF-004** in +`alkcall/docs/reviews/consumer-findings-ledger.md` (the handler-level +fix there is the complete one; this task is the alkhttp-local layer). + +## Acceptance Criteria + +- [ ] alkhttp-side guard: before dispatch, when the requested + operation resolves to a `services/schema`-family handler, apply + the same is_internal_op + access_check_for_op pre-checks to the + **inner `name` input** as GET `/schema` does (404 for Internal, + 403 for ACL denial, mirroring routes.rs:151-156) +- [ ] Implementer's choice on mechanism (options: a dispatch-path hook + in `call_handler`/`batch_handler`/`subscribe_handler`; an + alkhttp-side wrapper around the registry that intercepts + `services/schema` inputs; a GatewayDispatch-level check) — the + invariant is transport-level: **no alkhttp transport may fetch a + spec the GET `/schema` route would deny for the same identity** +- [ ] Test matrix mirroring the GET /schema tests: `/call` on + services/schema{internal-op} → 404; unauthorized identity → + 403; MCP `call` tool with the same input → same result + (feature-gated `mcp` test) +- [ ] A comment/ADR-note recording that the complete fix is + alkcall-side (CF-004) and this task is the local layer — when + CF-004 lands, the alkhttp guard becomes defense-in-depth (do NOT + remove it; the per-transport check stays) +- [ ] `cargo test`, `cargo clippy --all-targets -- -D warnings`, + `cargo fmt --check` pass (plus `--all-features` if the mcp test lands) + +## References + +- docs/reviews/002-post-remediation-review.md (Part F', PRJ-16) +- alkcall/docs/reviews/consumer-findings-ledger.md (CF-004, filed 2026-08-30) +- alkcall/src/registry/discovery.rs:327-343 (the unchecked handler) +- src/gateway/routes.rs:120-133 (outer-name-only pre-check), 151-156 (the correct GET /schema checks to mirror) +- docs/architecture/decisions/015-privilege-model-and-authority-context.md + +## Notes + +The three gateway invoke routes (`/call`, `/batch`, `/subscribe`) all +dispatch through the same spine, so a single interception point (if +found) covers all three + MCP; if the mechanism is route-local, mirror +it in all three pre-check sites. Do not block on the alkcall fix — +this task is shippable independently and the alkcall lead time is +unknown. If CF-004 lands first, re-scope this task to defense-in-depth +assertions only. \ No newline at end of file diff --git a/tasks/infra/review-002-bracketed-followup.md b/tasks/infra/review-002-bracketed-followup.md new file mode 100644 index 0000000..e2b4dd3 --- /dev/null +++ b/tasks/infra/review-002-bracketed-followup.md @@ -0,0 +1,67 @@ +--- +id: review-002-bracketed-followup +name: Bracketed review-002 follow-up pass (post-bulk — stale-check, OAI-18 enforcement decision, CON-08/CON-09 close() lever, cross-crate re-check) +status: pending +depends_on: [] +scope: narrow +risk: low +impact: project +level: planning +tags: [infra, review-002, follow-up, planning] +--- + +## Description + +Tentatively planned follow-up pass (mirrors review-001's pattern: the +first four remediation tasks emerged from post-bulk drift). Do NOT +decompose until the review-002 bulk has landed — several items below +may go stale or change shape based on what the bulk decided. Scope +candidates, to be re-validated when picked up: + +1. **OAI-18 enforcement decision** — `required`/`enum`/`pattern`/ + value-types are advertised via `/schema` but never enforced + (key-allowlist only). The bulk tasks leave this as + allowlist-plus-docs; the "compile-and-enforce the full input schema + per op" option (jsonschema crate is already a dep via schema_cache) + is the deferred alternative. Re-evaluate once + review-002-yaml-normalization + review-002-oai13-path-item-wildcards + land (both change the advertised-schema surface). +2. **CON-08 / review-001 CON-09 close() lever** — `from_mcp`'s + documented session leak and `from_wss`'s no-teardown import are + both accepted-for-v1 with documented contracts. If crates.io + release timing or consumer feedback makes reconnect/teardown real, + the `close()`-handle API is the v1.1 lever for both. +3. **Cross-crate re-check** — CF-001 (dead-mux write retryability), + CF-002 (demux TooLarge allocation), CF-003 (wire publish_schema + fail-open), CF-004 (services_schema_handler disclosure) in + alkcall's ledger: re-check status when alkcall next gets attention; + alkhttp-side assertions (ws-eof retryability tightening, PRJ-16 + defense-in-depth re-scope) may follow. +4. **Stale-comment sweep** — review-002 changes will touch hot docs + (byte_adapter pump docs, adapter.rs router comments, forward.rs + URL pipeline). A short sweep verifying module docs still match + behavior after the bulk (the SRV-11 lesson: comments asserting + axum semantics are exactly what went stale). +5. **ADR-045 version bump audit** — if projection-truthfulness and + gateway-status tasks landed, verify `to_openapi`'s `info.version` + reflects the accumulated contract changes per ADR-045. + +## Acceptance Criteria + +- [ ] Re-scoped into concrete tasks (or explicitly dismissed per item) + once the review-002 bulk is complete +- [ ] Nothing acted on before the bulk lands (planning-level gate) + +## References + +- docs/reviews/002-post-remediation-review.md (Part E' OAI-18; Part A' CON-08 status; Unit 3-4 notes; ADR-045) +- docs/reviews/001-initial-implementation-review.md (the follow-up pattern this mirrors — 4 tasks emerged post-bulk) +- alkcall/docs/reviews/consumer-findings-ledger.md (CF-001..CF-004) +- tasks/infra/review-001-missing-docs-sweep.md (precedent: opportunistic follow-up closure) + +## Notes + +Left `depends_on: []` deliberately — taskgraph ordering for a +planning-level task should not serialize the bulk (it drains +independently and is picked up when the operator chooses). All four +precede-the-bulk items are marked in each task's Notes where relevant. \ No newline at end of file diff --git a/tasks/infra/review-002-cov-deployment-knobs.md b/tasks/infra/review-002-cov-deployment-knobs.md new file mode 100644 index 0000000..3fefeec --- /dev/null +++ b/tasks/infra/review-002-cov-deployment-knobs.md @@ -0,0 +1,69 @@ +--- +id: review-002-cov-deployment-knobs +name: Coverage — with_ws_idle_timeout integration test, call_tool production dispatch, small dark arms (COV-11b/12) +status: pending +depends_on: [review-002-ws13-idle-progress] +scope: moderate +risk: low +impact: component +level: implementation +tags: [infra, review-002, coverage, websocket, mcp] +--- + +## Description + +Review 002's deployment-knob coverage gaps — production code paths +dark in **all** binaries: + +- **`with_ws_idle_timeout`** (`server/adapter.rs:228-242`): the WS-13 + server-side knob is called by no test — a regression silently + disables the idle timer on the default surface while tungstenite + unit tests stay green. Add: integration test building via + `HttpAdapter::new(...).with_ws_idle_timeout(Some(small))` → connect + a WS client → assert server closes with 1001 ≈ at the knob; plus a + `None` variant (session survives a long silence). NOTE: the + progress-semantics change from review-002-ws13-idle-progress lands + first — the 1001 assert must reflect final semantics (progress-based). +- **`call_tool` production dispatch** (`to_mcp.rs:459-468`): all four + tools + unknown-name are tested via `invoke_tool`, which **bypasses** + `call_tool` — the rmcp-entered routing shell (what production runs) + has never executed. Test: in `from_mcp_integration.rs`'s live-server + harness, one client-side `peer.call_tool("search"|"batch"|"schema"| + "call"|unknown)` round-trip through the real rmcp protocol. +- **Small dark arms (COV-12)**, one test each: `to_mcp` + missing-argument validation for `schema` (None args) and `batch` + (empty/no calls) tools (:175, :221); `from_mcp` non-object tool + arguments wrapped as `{"value": …}` (:405-408); `routes.rs` first- + line-invalid-JSON `/publish` 400 (:263-266); `decoy.rs` + directory→index.html resolution (:133-138); + `openapi_spec.rs` array-of-`$ref` resolution (:428-433 — the arm the + coverage pass initially mis-flagged; the *code is correct*, add the + test) and `style: simple` + `explode: true` rejection (:561-566). + +## Acceptance Criteria + +- [ ] Idle-knob integration test (Some(small) → 1001 per final + semantics; None → survives) in ws_upgrade_session.rs +- [ ] One rmcp-protocol `call_tool` round-trip test per tool + unknown + (feature `mcp`) +- [ ] Each small arm test lands (list above; skip any proven + unreachable during implementation — document which and why) +- [ ] `cargo llvm-cov --all-features` re-run: http_client.rs function + %, adapter.rs 228-242, to_mcp.rs call_tool no longer dark +- [ ] `cargo test --all-features`, `cargo clippy --all-features + --all-targets -- -D warnings`, `cargo fmt --check` pass + +## References + +- docs/reviews/002-post-remediation-review.md (Part G', COV-11b, COV-12) +- src/server/adapter.rs:228-242, src/adapters/to_mcp.rs:459-468, tests/ws_upgrade_session.rs, tests/from_mcp_integration.rs +- tasks/websocket/review-002-ws13-idle-progress.md (lands the semantics this asserts) +- tasks/adapters/review-002-oai11-ref-memoization.md (owns the array-$ref behavior; this adds only its test if not already added there) + +## Notes + +Sequence after ws13 (the idle-knob assert must match final semantics). +The array-`$ref` test is coverage-only insurance — if +review-002-oai11-ref-memoization already added it, check the box and +skip. The from_mcp/`{"value": …}` wrap test goes in +from_mcp_integration.rs (real wire behavior). \ No newline at end of file diff --git a/tasks/infra/review-002-cov13-dead-code.md b/tasks/infra/review-002-cov13-dead-code.md new file mode 100644 index 0000000..80b5d45 --- /dev/null +++ b/tasks/infra/review-002-cov13-dead-code.md @@ -0,0 +1,55 @@ +--- +id: review-002-cov13-dead-code +name: Delete dead accessors and FromRef impls — resolve_bearer wire-or-delete decision (COV-13) +status: pending +depends_on: [] +scope: single +risk: low +impact: component +level: implementation +tags: [infra, review-002, coverage] +--- + +## Description + +Review 002 COV-13: code confirmed dead by the coverage pass (every +binary, zero hits, nothing extracts these types) — delete rather than +test: + +- `server/state.rs:75-83` — both `FromRef` impls + (`Arc`, `Arc`): no route + extracts these types (the middleware takes state directly) +- `gateway/dispatch.rs:74-76, 79-81` — `identity_provider()` accessor + (dead) and `resolve_bearer()` accessor — the latter's doc **promises + an auth hook the middleware never calls** (spec/code drift): decide + wire-or-delete (if the hook concept matters for a future assembly- + layer story, wire it into `bearer_auth_middleware`/`ws_bearer_auth`; + otherwise delete) +- `websocket/upgrade.rs:145-147` — `FromRef for + Arc` (dead extractor) +- `to_mcp.rs` — `PRJ-24`'s dead `search_filter` parameter is owned by + review-002-projection-truthfulness; only handle it here if that task + did not + +## Acceptance Criteria + +- [ ] All dead items deleted (or resolve_bearer wired, with the doc + claim made true, if the hook decision says wire) +- [ ] `cargo build` + full test matrix green — the deletions prove + nothing referenced them (coverage claim verified) +- [ ] `cargo test`, `cargo clippy --all-targets -- -D warnings`, + `cargo fmt --check` pass + +## References + +- docs/reviews/002-post-remediation-review.md (Part G', COV-13) +- src/server/state.rs:75-83, src/gateway/dispatch.rs:74-81, src/websocket/upgrade.rs:145-147 +- tasks/infra/review-002-cov-deployment-knobs.md (the coverage pass's live-code work — separate from this dead-code sweep) + +## Notes + +Small, zero-risk deletion sweep. Good pairing with any other session's +warm-up. If the `resolve_bearer` hook-decision feels non-obvious +(needs an ADR or design note), default to delete — dead code with an +aspirational doc claim is worse than no hook; a real hook can be added +when a concrete assembly-layer need exists. \ No newline at end of file diff --git a/tasks/server/review-002-srv11-srv12-router-ordering.md b/tasks/server/review-002-srv11-srv12-router-ordering.md new file mode 100644 index 0000000..17f4ca0 --- /dev/null +++ b/tasks/server/review-002-srv11-srv12-router-ordering.md @@ -0,0 +1,67 @@ +--- +id: review-002-srv11-srv12-router-ordering +name: Router-layering fixes — double token resolution on WS and mcp, decoy 405 on extra routes (SRV-11, SRV-12) +status: pending +depends_on: [] +scope: narrow +risk: medium +impact: component +level: implementation +tags: [server, review-002, stealth] +--- + +## Description + +Two review-002 findings in `server/adapter.rs`'s `build_router` — both +are axum `route_layer`/merge-ordering semantics (empirically verified +in review 002 with an axum 0.8.9 harness; `route_layer` wraps routes +registered *before* the call): + +- **SRV-11**: the router-wide `bearer_auth_middleware` layer (:328) + wraps the WS route (registered earlier at :307-317 with its own + `ws_bearer_auth`) and the `/mcp` nest (own layer at :288-292) — the + token resolves **twice** on both, and the code comment claims the + opposite ("resolves the token exactly once"). Benign today + (ag stashes agree), but a provider with token-use side effects + double-counts. +- **SRV-12**: `method_not_allowed_fallback(decoy_method_not_allowed)` + (:322) covers only routers registered before the extras merge — + wrong-method probes on **extra routes** return axum's bare 405 (no + body, no `Server: nginx`), the exact stealth probe SRV-07's fix + neutralized for the default surface. + +## Acceptance Criteria + +- [ ] SRV-11: the WS route resolves the token exactly once (drop the + router-wide layer's application to it — e.g. verify with an + axum-semantics-confirmed mechanism: register the WS route after + the route_layer call, or exclude via per-route wrapping) and + `/mcp` likewise (keep exactly one auth layer for it); the + corrected comment states axum's actual `route_layer` semantics +- [ ] SRV-11 test: a counting IdentityProvider — one WS upgrade + request resolves the token exactly once (and one `/mcp` request + likewise, feature-gated) +- [ ] SRV-12: `method_not_allowed_fallback(decoy_method_not_allowed)` + re-applied after the extras merge; test: wrong-method probe on + an extra route carries the decoy body + `Server: nginx` header +- [ ] Re-verify the existing router tests (extra-routes auth, decoy + 405 default-surface, reserved-path panics) still pass — the + merge-order changes must not regress SRV-01's guarantee +- [ ] `cargo test`, `cargo clippy --all-targets -- -D warnings`, + `cargo fmt --check` pass + +## References + +- docs/reviews/002-post-remediation-review.md (Part A', SRV-11, SRV-12; hot-spot 2 in the server pass) +- src/server/adapter.rs:274-345 (build_router), :296-320 (comment + WS route), :322 (405 fallback), :328 (router-wide layer) +- tasks/server/review-001-extra-routes-auth.md (the SRV-01 ordering this must preserve), tasks/server/review-001-hyper-server-knobs.md (SRV-07's decoy-405 work) + +## Notes + +Order-sensitivity warning (from the server pass): "any future route +addition must re-derive these semantics empirically" — after fixing, +add the counting test + the extra-route 405 test so the invariants are +pinned, not just commented. The SRV-01 opt-out shape (inner layers +win) must remain exactly as tested. Keep the SRV-10 fold-in fix +(historical) intact — this task is about the *comment* being false and +the double-resolve being real. \ No newline at end of file diff --git a/tasks/websocket/review-002-con18b-ws-polish.md b/tasks/websocket/review-002-con18b-ws-polish.md new file mode 100644 index 0000000..4214ab7 --- /dev/null +++ b/tasks/websocket/review-002-con18b-ws-polish.md @@ -0,0 +1,73 @@ +--- +id: review-002-con18b-ws-polish +name: WS small fixes — pre-send cap check, close reasons, custom-route knobs, axum-flavor tests (WS-14/15/17/18/19) +status: pending +depends_on: [review-002-ws13-idle-progress] +scope: moderate +risk: low +impact: component +level: implementation +tags: [websocket, review-002] +--- + +## Description + +Five review-002 minor WS findings that all live in the two WS files +(`byte_adapter.rs`, `upgrade.rs`) — batched because each is small and +they share the review-002-ws13-idle-progress refactor context +(sequence after it; do not parallelize on the same file): + +- **WS-14**: the byte-cap check can trip only *after* `poll_write` has + accepted into the channel (`byte_adapter.rs:326-334, 585-586`) — the + mux committed the chunk, then the pump 1011s and truncates. Move the + size check into `poll_write` **before** `try_send` so the mux sees + the write error instead of the wire. +- **WS-15**: `WriteMsg::CloseWith` hardcodes the reason "text messages + not supported" for idle-timeout (1001), inbound-size (1011), and text + (1002) closes (`:316-324`). Carry the reason in the variant; tests + assert numeric codes today, keep them and add reason asserts where + cheap. +- **WS-17**: bare-registry WS routes get no knob surface + (`upgrade.rs:104-111` hardcodes 60 s idle + private 64-session + semaphore). Add a `WsIdleTimeout`-style request extension mirroring + `ChannelsPolicy` (or document the fixed values in the module doc — + implementer's choice, but `upgrade.rs` docs must match reality). +- **WS-18**: write-side stall is unbounded (peer stops reading → write + pump parks in `ws_sink.send`, 64×16 MiB bounded but time-unbounded). + Add a write-progress timeout to the write pump (same knob family as + the read side — share the config surface WS-17 touches). +- **WS-19**: axum-flavor `AxumFraming` arms have no direct unit test + (cap-trip closes + idle-1001 are asserted on tungstenite only, via + shared generic code). Mirror one text→1002 and one cap-trip test + over `AxumFraming`. + +## Acceptance Criteria + +- [ ] WS-14: over-cap write rejected in `poll_write` pre-`try_send`; + test asserts the *stream* error, not a wire-level 1011 +- [ ] WS-15: close reasons distinct per cause (idle / oversized / + text); existing close-frame tests updated with reason asserts +- [ ] WS-17: knob surface exists (extension or documented-fixed) and + `upgrade.rs` module docs match the implementation +- [ ] WS-18: write-progress timeout lands with the same config family; + test: a peer that stops reading (clog the sink) is evicted + within the knob (scaled test, tungstenite path) +- [ ] WS-19: two `AxumFraming` unit tests (text→1002, cap-trip) pass +- [ ] `cargo test`, `cargo clippy --all-targets -- -D warnings`, + `cargo fmt --check` pass + +## References + +- docs/reviews/002-post-remediation-review.md (Part B', WS-14/15/17/18/19) +- src/websocket/byte_adapter.rs:326-334 (WS-14), :316-324 (WS-15), :584-597 (WS-18), :782-1206 (WS-19 test target) +- src/websocket/upgrade.rs:104-111 (WS-17), :308-317 (the ChannelsPolicy extension pattern to mirror) +- tasks/websocket/review-001-ws-pump-consolidation.md (the generic pump these extend) + +## Notes + +Slice suggestion (per the review-001 lesson on WS agents): one slice +per finding with its own commit, in the order above (WS-14 is the only +behavior-correctness one; WS-18 is the only new mechanism). If WS-13 +(chief-risk task) changed the read-loop shape, rebase-verify the cap +accounting comments before this task's slices — they cite +pre/post-extend order explicitly. \ No newline at end of file diff --git a/tasks/websocket/review-002-ws13-idle-progress.md b/tasks/websocket/review-002-ws13-idle-progress.md new file mode 100644 index 0000000..8dbf405 --- /dev/null +++ b/tasks/websocket/review-002-ws13-idle-progress.md @@ -0,0 +1,74 @@ +--- +id: review-002-ws13-idle-progress +name: Idle deadline off demux progress + slow-legitimate-stream decision (WS-13) +status: pending +depends_on: [] +scope: moderate +risk: high +impact: component +level: implementation +tags: [websocket, review-002] +--- + +## Description + +Review 002 WS-13 [major]. The WS-01 remediation's idle timer +(`byte_adapter.rs:229-244`) wraps only the next-message await, so it +fails in both directions: + +1. **The attack it exists for defeats it.** WS-01's dribble (declare a + 16 MiB chunk, deliver one byte per WS message per minute) keeps + arriving messages → the deadline resets on every dribble → the demux + still parks forever with the partial chunk. The doc (:117) claims + "a peer that drips bytes cannot park the single demux loop longer + than this without traffic" — under the actual attack the peer + supplies traffic forever. +2. **Legitimate slowness is killed.** A >60 s-silent-but-alive session + (long handler compute, quiet subscription) is disconnected with + 1001 mid-flight — no ping/pong keepalive exists, and chunk progress + does not reset the timer. + +The existing test (`idle_read_timeout_resets_on_traffic`) enshrines +reset-on-message as correct — it codifies the gap. + +## Acceptance Criteria + +- [ ] Decide + implement the progress semantics: the deadline resets + on **demux progress** (bytes actually forwarded into `read_tx`), + not message arrival — a peer that dribbles a chunk forever hits + the deadline; a peer delivering complete chunks (even slowly + per-message but productively) keeps the connection +- [ ] Decide + document the legitimate-silence stance (choose one and + record it in the module doc + http-server/websocket.md): + (a) add WS ping/pong keepalive so app-silence ≠ transport-silence, + or (b) document that 60 s of *no progress* is an intentional + eviction line even for silent subscriptions +- [ ] Acceptance test 1: chunk header `[ch][len=N]` dribbled in small + messages with per-message gaps < timeout → connection closed + (progress deadline fires) +- [ ] Acceptance test 2: scaled-down silent-but-alive session with a + pending response is NOT disconnected when the knob's spirit says + it should survive (e.g. messages flowing that make progress) +- [ ] The WS-01 knob surface (`with_ws_idle_timeout`) is unchanged; + the semantics change is doc'd against it +- [ ] `cargo test`, `cargo clippy --all-targets -- -D warnings`, + `cargo fmt --check` pass (tungstenite-path unit tests + one axum + integration mirror) + +## References + +- docs/reviews/002-post-remediation-review.md (Part B', WS-13; Part G', COV-11b — the knob is currently untested on the axum path too) +- src/websocket/byte_adapter.rs:117 (doc claim), :229-253 (the timer), :782-1206 (tests) +- tasks/websocket/review-001-ws-session-limits.md (the WS-01 knob work) +- docs/architecture/websocket.md + +## Notes + +Highest-risk task in the batch (touching the shared pump's read loop — +the same loop the cap logic lives in). Slice suggestion: (1) progress +tracking + tests on the tungstenite path, (2) doc/keepalive decision, +(3) axum integration mirror. Coordinate with +review-002-con18b-ws-polish (same file) — sequence, don't parallelize. +If (a)-keepalive is chosen, note it interacts with FWD-15's +server-side keep-alives on the *HTTP* SSE path — different layers, but +document both in the same doc pass. \ No newline at end of file