From 12b35e2c5f8bb16217d937296ca6df9135318843 Mon Sep 17 00:00:00 2001 From: "glm-5.3-flash" Date: Sat, 29 Aug 2026 07:10:15 +0000 Subject: [PATCH] docs(tasks): decompose review 001 remediation Units 1-5 into taskgraph tasks 21 review-001 tasks across server/adapters/client/gateway/websocket/infra, chunked from the 7-unit remediation plan in docs/reviews/001-initial-implementation-review.md. - Scope split by mechanism, not one-per-finding: 15 tasks in generation 1 (parallelizable), 6 sequenced after their file-sharing precursors - Deliberately deferred until dependent fixes land: projection/doc fidelity partial (Unit 6 beyond dependency hygiene), coverage backfills (COV-01..07 via in-task acceptance for forward.rs), and per-finding minors (OAI-06/07, HY-02/04/06/10/11, CON-08) - Cross-crate WS-12 (alkcall demux 4 GiB discard alloc) noted for filing in alkcall, not here taskgraph: validate clean, no cycles, 6 generations --- .../review-001-consumer-adapter-hygiene.md | 85 +++++++++++++++++++ .../adapters/review-001-forward-url-safety.md | 64 ++++++++++++++ .../review-001-input-schema-enforcement.md | 63 ++++++++++++++ .../adapters/review-001-mcp-tool-fidelity.md | 61 +++++++++++++ .../review-001-openapi-import-integrity.md | 56 ++++++++++++ .../review-001-openapi-projection-fidelity.md | 75 ++++++++++++++++ tasks/adapters/review-001-ref-cycle-guard.md | 56 ++++++++++++ .../review-001-response-decode-fidelity.md | 68 +++++++++++++++ tasks/adapters/review-001-sse-parser.md | 55 ++++++++++++ .../client/review-001-client-timeout-retry.md | 67 +++++++++++++++ .../review-001-gateway-publish-semantics.md | 74 ++++++++++++++++ .../review-001-gateway-stream-errors.md | 75 ++++++++++++++++ tasks/infra/review-001-dependency-hygiene.md | 61 +++++++++++++ tasks/server/review-001-extra-routes-auth.md | 52 ++++++++++++ tasks/server/review-001-hyper-server-knobs.md | 67 +++++++++++++++ tasks/server/review-001-mcp-body-limit.md | 48 +++++++++++ .../review-001-schema-internal-visibility.md | 60 +++++++++++++ .../review-001-ws-data-channel-decision.md | 50 +++++++++++ tasks/websocket/review-001-ws-eof-signal.md | 65 ++++++++++++++ .../review-001-ws-pump-consolidation.md | 58 +++++++++++++ .../websocket/review-001-ws-session-limits.md | 64 ++++++++++++++ 21 files changed, 1324 insertions(+) create mode 100644 tasks/adapters/review-001-consumer-adapter-hygiene.md create mode 100644 tasks/adapters/review-001-forward-url-safety.md create mode 100644 tasks/adapters/review-001-input-schema-enforcement.md create mode 100644 tasks/adapters/review-001-mcp-tool-fidelity.md create mode 100644 tasks/adapters/review-001-openapi-import-integrity.md create mode 100644 tasks/adapters/review-001-openapi-projection-fidelity.md create mode 100644 tasks/adapters/review-001-ref-cycle-guard.md create mode 100644 tasks/adapters/review-001-response-decode-fidelity.md create mode 100644 tasks/adapters/review-001-sse-parser.md create mode 100644 tasks/client/review-001-client-timeout-retry.md create mode 100644 tasks/gateway/review-001-gateway-publish-semantics.md create mode 100644 tasks/gateway/review-001-gateway-stream-errors.md create mode 100644 tasks/infra/review-001-dependency-hygiene.md create mode 100644 tasks/server/review-001-extra-routes-auth.md create mode 100644 tasks/server/review-001-hyper-server-knobs.md create mode 100644 tasks/server/review-001-mcp-body-limit.md create mode 100644 tasks/server/review-001-schema-internal-visibility.md create mode 100644 tasks/websocket/review-001-ws-data-channel-decision.md create mode 100644 tasks/websocket/review-001-ws-eof-signal.md create mode 100644 tasks/websocket/review-001-ws-pump-consolidation.md create mode 100644 tasks/websocket/review-001-ws-session-limits.md diff --git a/tasks/adapters/review-001-consumer-adapter-hygiene.md b/tasks/adapters/review-001-consumer-adapter-hygiene.md new file mode 100644 index 0000000..5e65b9e --- /dev/null +++ b/tasks/adapters/review-001-consumer-adapter-hygiene.md @@ -0,0 +1,85 @@ +--- +id: review-001-consumer-adapter-hygiene +name: from_wss/from_mcp consumer fixes — plaintext ws://, 401 classification, token hygiene (CON-03, CON-05..CON-13) +status: pending +depends_on: [] +scope: moderate +risk: low +impact: component +level: implementation +tags: [adapters, review-001, mcp, from-wss] +--- + +## Description + +Review 001 consumer-adapter findings, minus the hang fix (its own task — +review-001-ws-robustness carries WS-02/CON-02), grouped as the small CON +cleanup list: + +- **CON-01 (one-liner)**: `from_mcp/mod.rs:86` imports only the first + `tools/list` page; `next_cursor` is never followed. rmcp 1.8 provides + `list_all_tools()` for exactly this. Servers that paginate silently + truncate the import — no error, no log. Fix and add a paginating-server + test (COV gap 11). +- **CON-03 (security)**: `from_wss.rs:114-128` accepts plaintext `ws://` + and attaches the Bearer token unconditionally. A config typo silently + ships a long-lived bearer credential over plaintext. Refuse `ws://` + when a token is present — or unconditionally unless explicitly allowed + — with a clear error. +- **CON-04** (`from_mcp/mod.rs:240-248`): the audio variant of + `content_block_union_schema` requires non-existent `"audio"` (property + is `data`) — valid audio blocks never satisfy the published + `output_schema`. Fix and validate a real audio block. +- **CON-05/CON-07**: docs claim per-call credential reads; reality is + import-time credentials (`from_mcp` transport-pinned token, `from_wss` + bundles carry no capabilities at all). Correct docs, remove the dead + capability read (`mod.rs:139-143`). +- **CON-06** (`mod.rs:103-116`): 401 classified via + `format!("{error:?}").contains("401")` — a `:4010/` port in a URL + misclassifies as Unauthorized and real auth failures can be missed. + Match typed variants; substring only as last resort. +- **CON-11**: transport-level `tools/call` failures → undeclared + `CallError::internal`; declare the failure mode (and preserve JSON-RPC + error fidelity where cheap). +- **CON-12**: remote tool names interpolated into op names unsanitized — + a `/` in a tool name breaks the two-segment `ns/op` convention. + Sanitize/validate at import. +- **CON-13** (`from_wss.rs:48`, `from_mcp/mod.rs:37`): held tokens are + plain `Option`; use `Secret` to match the crate's + posture (no `Debug` derives exists — keep it that way). +- **CON-10**: `[[test]] full_surface` is missing `test-support` in + `required-features` → `cargo test --features mcp` fails to compile + (empirically verified; masked by CI's `--all-features`). +- **CON-08/CON-09**: no teardown path on `FromMCP` (each `import()` + leaks an open server-side session + GET SSE stream) and `from_wss` + import stacks duplicate sessions on reconnect by design (ADR-070 v1). + Either add explicit close/teardown handles or write the ADR-070 note + + module docs that make the limitation explicit — do not silently expand + scope into a reconnect layer. + +## Acceptance Criteria + +- [ ] Paginated `tools/list` fully imported (paginating-server test) +- [ ] `ws://` + token refused (or explicit opt-out) with a clear error (test) +- [ ] Audio variant of `content_block_union_schema` satisfied by a valid block (test) +- [ ] No substring-based 401 classification on typed paths; a URL containing `:401x` no longer misclassifies (test) +- [ ] CON-05/07 docs corrected; dead capability read removed +- [ ] CON-11/12: transport-failure error declared; remote tool names sanitized (test) +- [ ] Tokens held as `Secret` +- [ ] `cargo test --features mcp` compiles and passes (CON-10), as does `cargo test --all-features` +- [ ] CON-08/09: close handles exist or docs/ADR-070 note state the limitation + +## References + +- docs/reviews/001-initial-implementation-review.md (Part G, CON-03..CON-13) +- docs/architecture/decisions/070-from-wss-consumer-adapter.md + +## Notes + +> Agent fills during implementation. Independent of the WS tasks. +> The from_wss notify/pending fixes are tracked separately in +> review-001-ws-robustness (WS-02/CON-02 share one mechanism). + +## Summary + +> Filled on completion. \ No newline at end of file diff --git a/tasks/adapters/review-001-forward-url-safety.md b/tasks/adapters/review-001-forward-url-safety.md new file mode 100644 index 0000000..13e8da1 --- /dev/null +++ b/tasks/adapters/review-001-forward-url-safety.md @@ -0,0 +1,64 @@ +--- +id: review-001-forward-url-safety +name: Safe outbound URL construction — encoding, base-path, host validation (FWD-01, FWD-02) +status: pending +depends_on: [] +scope: narrow +risk: high +impact: component +level: implementation +tags: [adapters, review-001, security] +--- + +## Description + +Review 001 findings FWD-01 + FWD-02, both empirically verified against +`src/adapters/forward.rs` and both security-bearing **with the +operation's injected credentials attached**: + +- **FWD-01**: path-parameter values are substituted raw + (`forward.rs:56-71, 133-141`). `Url::join` normalizes `..`, so + `{owner} = "../../admin"` escapes a path-scoped prefix (cross-tenant + IDOR); `?` and `#` in a value split/inject into the URL; values + containing a later placeholder get re-expanded by the iterative + substitution. Fix: percent-encode each segment + (`utf8_percent_encode` with a path-segment set), reject/encode + `?`/`#`, and render the template in a single pass (not iterative + replace). The query path is already correctly encoded via + `query_pairs_mut` — keep that shape. +- **FWD-02**: `Url::join` resolves against the base *directory*, so + `base_url = "https://api.openai.com/v1"` + `/chat/completions` silently + drops `/v1` (`forward.rs:74-78`); every test uses origin-only base + URLs so the suite can't see it. Worse: a path key that is an absolute + URL replaces scheme+host entirely (verified — reqwest only rejects + non-http(s) *schemes*), and credential injection happens after URL + construction, so a spec-controlled absolute path sends the namespace's + credentials to an arbitrary host (SSRF). Specs are + assembly-layer-supplied (trusted per ADR-066) but nothing enforces that + boundary. Fix: append to the base *path* (not origin), require the + joined URL to keep the base host (fail loudly on host change), and + require https by default (explicit opt-out for http). + +## Acceptance Criteria + +- [ ] Traversal test: `{owner} = "../../admin"` cannot escape the template path (segment encoded or rejected) +- [ ] `?`/`#`/later-placeholder-in-value tested (encoded or rejected, never URL-structural) +- [ ] Base URL with a path prefix keeps the prefix (`…/v1` + `/chat/completions` → `…/v1/chat/completions`, test) +- [ ] Absolute-URL path template is rejected loudly; joined host ≠ base host is rejected; http base refused unless opted out (tests) +- [ ] Rendering is single-pass (a rendered value is never re-substituted) +- [ ] Both `from_openapi` and `from_jsonschema` paths covered (they share `forward.rs`) +- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass + +## References + +- docs/reviews/001-initial-implementation-review.md (Part D, FWD-01, FWD-02) +- docs/architecture/decisions/066-from-jsonschema-as-http-adapter.md + +## Notes + +> Agent fills during implementation. One of the two gate tasks for +> any deployment-facing milestone (with review-001-client-timeout-retry). + +## Summary + +> Filled on completion. \ No newline at end of file diff --git a/tasks/adapters/review-001-input-schema-enforcement.md b/tasks/adapters/review-001-input-schema-enforcement.md new file mode 100644 index 0000000..af1ca1b --- /dev/null +++ b/tasks/adapters/review-001-input-schema-enforcement.md @@ -0,0 +1,63 @@ +--- +id: review-001-input-schema-enforcement +name: Send exactly what the input schema advertises (OAI-02, OAI-03, OAI-07, OAI-09) +status: pending +depends_on: [review-001-openapi-import-integrity] +scope: narrow +risk: medium +impact: component +level: implementation +tags: [adapters, review-001, from-openapi, from-jsonschema] +--- + +## Description + +Review 001 findings OAI-02 + OAI-03 + OAI-07 + OAI-09 — the request a +forwarder sends must match the contract `/schema` advertises: + +- **OAI-02** (`forward.rs:60-72`): no input-schema enforcement — every + input key that isn't a path placeholder (and isn't literally `"body"`) + becomes an upstream query parameter regardless of the registered + `input_schema`. Peer input like `"debug": "true"` or + `"impersonate_id": "…"` reaches the upstream. ADR-066's + "input schema validation before send" is implemented nowhere. Fix: + enforce `input_schema` at call time — reject undeclared keys (or + document pass-through as an explicit, per-adapter choice). +- **OAI-03** (`openapi_spec.rs:35-40`): `in: header` parameters are + silently sent as query parameters (`in_` is a dead field). Upstream + auth/trace headers never arrive, and header-designated values land in + upstream *access logs* instead. Honor `in: header`; fix `in: cookie` + (reject with a clear unsupported error). +- **OAI-07**: the magic `"body"` key collides with a spec parameter + actually named `body` — the declared parameter is silently diverted. + Namespace the body input or reject the collision loudly. +- **OAI-09** (`from_jsonschema.rs:36-51`): malformed `method`/ + `path_template`/`base_url` validate only at first invoke; and the + module doc says "`Internal` by default" while the adapter passes the + caller's spec through verbatim (`from_openapi` hardcodes + `Visibility::Internal`). Validate at construction; align the doc or + default the visibility. + +## Acceptance Criteria + +- [ ] Undeclared input keys are rejected (or the pass-through is explicit config, tested) — peer input cannot add upstream query params (test) +- [ ] `in: header` parameters send as headers; `in: cookie` errors clearly at import (tests) +- [ ] `body`-named parameter collision handled loudly (test) +- [ ] `from_jsonschema` validates method/path_template/base_url at construction; `Internal`-by-default matches `from_openapi` or the doc is corrected +- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass + +## References + +- docs/reviews/001-initial-implementation-review.md (Part E, OAI-02, OAI-03, OAI-07, OAI-09) +- docs/architecture/decisions/066-from-jsonschema-as-http-adapter.md + +## Notes + +> Agent fills during implementation. Depends on ref-resolution only +> for shared fixtures/types in `openapi_spec.rs`; start after it lands +> to avoid churn. OAI-06 (silent feature degradation) is deliberately +> deferred — revisit after this task. + +## Summary + +> Filled on completion. \ No newline at end of file diff --git a/tasks/adapters/review-001-mcp-tool-fidelity.md b/tasks/adapters/review-001-mcp-tool-fidelity.md new file mode 100644 index 0000000..c844aba --- /dev/null +++ b/tasks/adapters/review-001-mcp-tool-fidelity.md @@ -0,0 +1,61 @@ +--- +id: review-001-mcp-tool-fidelity +name: MCP tool-gateway runtime fidelity (PRJ-07, PRJ-08, PRJ-09, PRJ-10, PRJ-13) +status: pending +depends_on: [review-001-schema-internal-visibility] +scope: narrow +risk: low +impact: component +level: implementation +tags: [adapters, review-001, mcp] +--- + +## Description + +Review 001 MCP projection findings in `src/adapters/to_mcp.rs` (the +schema-ACL half of the MCP story is review-001-schema-internal-visibility; +this task covers the rest of the tool surface): + +- **PRJ-07**: the `search` tool advertises an optional substring `query` + filter but `call_tool` drops `arguments` entirely (`:382`) — an LLM + passing `{"query": "fs"}` gets the full unfiltered listing (context + waste/overflow in big registries). Honor the filter. +- **PRJ-08**: `search` excludes only `Sub`, not `Pub` (`:268-274`), + contradicting ADR-068 ("excludes both"). A discovered Pub op can never + be invoked via the `call` tool — the advertised discovery surface is a + lie for the whole Pub class. (The `"subscription"`/`"Sub"` match arms + are dead — `op_type_str` only emits lowercase.) +- **PRJ-09**: batch item shape `{"isError", "output"|"error"}` contradicts + the tool description ("each shaped like a `call` result") — fix the + description or the shape. +- **PRJ-10**: `structuredContent` passes output through verbatim, so + string/array/null outputs produce non-object `structuredContent` + (strict MCP clients may reject; batch returns a top-level array). Wrap + or document. +- **PRJ-13**: hand-rolled argument errors omit `retryable` (required by + the OpenAPI error schemas, always present on `CallError`), and a + non-string `operation` reports the misleading "missing required field". + Build argument errors in the `CallError` wire shape. + +## Acceptance Criteria + +- [ ] `search` respects a `query` argument (test); `Pub` ops excluded from results (test) +- [ ] Batch item doc matches the emitted shape (or shape changed to match — one way or the other, tested) +- [ ] Non-object outputs wrapped into objects or documented as an exception; batch shape consistent +- [ ] MCP argument errors carry `retryable` and truthful messages +- [ ] `cargo test --all-features` passes + +## References + +- docs/reviews/001-initial-implementation-review.md (Part F, PRJ-07..PRJ-10, PRJ-13) +- docs/architecture/decisions/041-mcp-tool-gateway-pattern.md +- docs/architecture/decisions/068-gateway-publish-endpoint.md + +## Notes + +> Agent fills during implementation. Depends on the visibility task +> because both rework the `schema` tool in `to_mcp.rs`. + +## Summary + +> Filled on completion. \ No newline at end of file diff --git a/tasks/adapters/review-001-openapi-import-integrity.md b/tasks/adapters/review-001-openapi-import-integrity.md new file mode 100644 index 0000000..b17cf4e --- /dev/null +++ b/tasks/adapters/review-001-openapi-import-integrity.md @@ -0,0 +1,56 @@ +--- +id: review-001-openapi-import-integrity +name: Resolve parameter/requestBody $refs; detect import collisions (OAI-04, OAI-05) +status: pending +depends_on: [review-001-ref-cycle-guard] +scope: narrow +risk: medium +impact: component +level: implementation +tags: [adapters, review-001, from-openapi] +--- + +## Description + +Review 001 findings OAI-04 + OAI-05 — import produces ops that misbehave +silently at call time: + +- **OAI-04** (`openapi_spec.rs:236-262`): parameter entries of the form + `{"$ref": "#/components/parameters/Id"}` (extremely common in real + specs) have no `name`, are silently skipped, and never reach + `resolve_refs_recursive`; same for `requestBody: {"$ref": …}`. Only + `components/schemas` is indexed (`:162-174`). Result: the op registers + with `{id}` in the template but `id` absent from the schema; at call + time the placeholder is substituted with the literal `{owner}` text and + percent-encoded (`%7Bowner%7D`) — a well-formed request to a nonsense + path, with credentials attached. No error at import or call time. + Fix: index `components/parameters` + `requestBodies`, resolve the ref + forms, and fail loudly on unresolved path placeholders (import and/or + call time). +- **OAI-05** (`from_openapi.rs:51-65,160`): generated operation IDs + collide by construction (`/x/{id}/y` and `/x/y` both → `get_x_y`); + the registry silently replaces, so one op shadows another and + `/search` under-reports. Detect duplicates within the import batch and + fail loudly (or deterministically disambiguate + warn — pick one and + document). + +## Acceptance Criteria + +- [ ] `{"$ref": "#/components/parameters/…"}` params and `requestBody` refs resolve into the op's schema (test) +- [ ] Unresolved path placeholders fail loudly (import-time, or call-time with a loud error — tested, not silently `%7Bowner%7D`) +- [ ] Duplicate operation IDs in one import batch are detected and rejected (or deterministically disambiguated + warned — tested) +- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass + +## References + +- docs/reviews/001-initial-implementation-review.md (Part E, OAI-04, OAI-05) + +## Notes + +> Agent fills during implementation. Follows the cycle-guard task so +> the resolver is already hardened when the new ref kinds are wired +> through it. + +## Summary + +> Filled on completion. \ No newline at end of file diff --git a/tasks/adapters/review-001-openapi-projection-fidelity.md b/tasks/adapters/review-001-openapi-projection-fidelity.md new file mode 100644 index 0000000..e9c9e24 --- /dev/null +++ b/tasks/adapters/review-001-openapi-projection-fidelity.md @@ -0,0 +1,75 @@ +--- +id: review-001-openapi-projection-fidelity +name: to_openapi golden fidelity — runtime-truthful document (PRJ-01..PRJ-05, PRJ-11, PRJ-12, PRJ-14, PRJ-15) +status: pending +depends_on: [review-001-gateway-stream-errors, review-001-gateway-publish-semantics] +scope: moderate +risk: medium +impact: component +level: implementation +tags: [adapters, review-001, to-openapi] +--- + +## Description + +Review 001 Part F: the `to_openapi` document describes responses the +gateway does not emit. The runtime contract must be settled **before** +this doc work — hence the gateway dependencies (per the review's +sequencing note). Systematic diff of the projection against the runtime +(routes tests are the oracle): + +- **PRJ-01/PRJ-02**: `/search` and `/schema` 200 responses are documented + without the envelope wrapper (`{request_id, result, output}`) and with + wrong item fields (`description` that doesn't exist; missing + `namespace`/`op_type`/`visibility`/`access_control`/`channel_open`/ + `publish_schema`). Clients generated from the doc are broken day one. +- **PRJ-03**: documented 400 `INVALID_INPUT` vs runtime 422 + (`error.rs:51`); axum extractor rejections are plain-text bodies with + none of the documented `{code, message, retryable}` shape; no 422 in + the doc at all. +- **PRJ-04**: op errors declared at non-`HTTP_*` codes are projected + under statuses the runtime never produces (`ErrorDefinition.http_status` + is never consulted; `RATE_LIMITED` surfaces as 500). Either project + honestly under 500 or honor `http_status` at runtime — with the review's + note that `operation_errors_projected_onto_call` currently enshrines the + wrong behavior. +- **PRJ-05**: `/subscribe` documented statuses are structurally + unreachable (always 200 + SSE per GW-12); in-band `event:error` frames + undocumented. Document the 200+in-band-error contract. +- **PRJ-11/HY-03**: remaining guarded `expect`s in `to_openapi` + (`:440, :505, :512`) — replace with `if let` (convention). +- **PRJ-12**: error dedupe iterates a `HashMap` → nondeterministic doc + regeneration for identical registry state. Dedupe by `(code, status)` + or sort. +- **PRJ-14**: `schema_call_request()` inlined 4× and unused-in-practice + `components.schemas` — use `$ref` or drop components so shape changes + can't drift. +- **PRJ-15**: `/search` documents 401/403 that cannot occur, omits the + 404 that can; no `securitySchemes` anywhere despite Bearer being the + contract (ADR-004). + +## Acceptance Criteria + +- [ ] Generated-doc golden test asserted against hand-written expectations matching the routes tests' actual bodies (the review's gate for this unit) +- [ ] `/subscribe` doc documents the 200 + `event:error` in-band contract (GW-12's asymmetry called out); `/search` statuses match reality +- [ ] PRJ-04 decision landed (project-honest vs honor-`http_status`) and the enshrining test aligned +- [ ] Deterministic doc generation (same registry → byte-identical doc, test) +- [ ] `$ref`s in components or components removed; `expect`s replaced +- [ ] `components.securitySchemes` declares the Bearer scheme +- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass + +## References + +- docs/reviews/001-initial-implementation-review.md (Part F, PRJ-01..PRJ-05, PRJ-11, PRJ-12, PRJ-14, PRJ-15) +- docs/architecture/decisions/042-openapi-gateway-pattern.md, 045-to-openapi-gateway-spec-versioning.md + +## Notes + +> Agent fills during implementation. Sequenced after the gateway +> tasks so the documented contract matches the settled runtime (review's +> Unit 6 ordering). Bump `info.version` per ADR-045 if the gateway +> description changes. + +## Summary + +> Filled on completion. \ No newline at end of file diff --git a/tasks/adapters/review-001-ref-cycle-guard.md b/tasks/adapters/review-001-ref-cycle-guard.md new file mode 100644 index 0000000..c2b7131 --- /dev/null +++ b/tasks/adapters/review-001-ref-cycle-guard.md @@ -0,0 +1,56 @@ +--- +id: review-001-ref-cycle-guard +name: Bounded, cycle-safe $ref resolution (OAI-01, OAI-08) +status: pending +depends_on: [] +scope: narrow +risk: high +impact: component +level: implementation +tags: [adapters, review-001, from-openapi] +--- + +## Description + +Review 001 finding OAI-01 — the single highest-severity finding in the +review (borderline critical, empirically verified): +`resolve_refs_recursive` (`src/adapters/openapi_spec.rs:199-221`) recurses +with no cycle detection and no depth budget. A self-referential component +(`{"$ref":"#/components/schemas/Node"}` inside Node — trees, linked +lists, cursor pagination: common and *valid* OpenAPI) recurses until the +stack is exhausted: `thread has overflowed its stack; fatal runtime +error` → **process abort**. Not a catchable panic; `import()` kills the +whole process — startup crash-loop, or remote DoS if specs are ever +runtime-refreshed/peer-supplied. + +Fix: depth budget + visited set keyed on the JSON-pointer path, returning +a clean `CallError`/import error on cycles and over-deep specs. **Do not** +try to preserve full recursive expansion of recursive schemas — the goal +is a clean, loud error (or a depth-limited expansion where the adapter can +safely represent it), not unbounded expansion. + +Ride-along: **OAI-08** — guarded `expect`s in `openapi_spec.rs:144,170` +(the "no `expect` outside tests" convention; `if let` costs nothing). + +## Acceptance Criteria + +- [ ] Import of a self-referential spec returns an error — the process does not abort (the review's named acceptance gate: a test importing a self-referential spec would have caught the abort immediately) +- [ ] Deeply-nested non-circular spec beyond the budget errors cleanly at import (test) +- [ ] A non-recursive spec with `$ref` sharing (refs to a common schema) still imports identically — no false cycle positives (existing suite green) +- [ ] OAI-08 `expect`s replaced +- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass + +## References + +- docs/reviews/001-initial-implementation-review.md (Part E, OAI-01, OAI-08) + +## Notes + +> Agent fills during implementation. Do before anything +> deployment-facing. Deliberately split from the parameter/requestBody +> work (review-001-openapi-import-integrity) so the delicate +> cycle-detection change lands alone. + +## Summary + +> Filled on completion. \ No newline at end of file diff --git a/tasks/adapters/review-001-response-decode-fidelity.md b/tasks/adapters/review-001-response-decode-fidelity.md new file mode 100644 index 0000000..14b616a --- /dev/null +++ b/tasks/adapters/review-001-response-decode-fidelity.md @@ -0,0 +1,68 @@ +--- +id: review-001-forward-response-fidelity +name: Upstream response decode — vendored JSON, size caps, loud auth errors, error bodies (FWD-07, FWD-08, FWD-10, FWD-12) +status: pending +depends_on: [] +scope: moderate +risk: medium +impact: component +level: implementation +tags: [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_` 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. + +## Summary + +> Filled on completion. \ No newline at end of file diff --git a/tasks/adapters/review-001-sse-parser.md b/tasks/adapters/review-001-sse-parser.md new file mode 100644 index 0000000..13acae3 --- /dev/null +++ b/tasks/adapters/review-001-sse-parser.md @@ -0,0 +1,55 @@ +--- +id: review-001-sse-parser +name: Incremental byte-level SSE parser (FWD-06) +status: pending +depends_on: [] +scope: narrow +risk: high +impact: component +level: implementation +tags: [adapters, review-001] +--- + +## Description + +Review 001 finding FWD-06 — silent subscription data loss, empirically +verified: `parse_sse_frames` (`src/adapters/forward.rs:409-417`) keeps +only the *last* line of each TCP chunk and discards any pending +multi-line `data_buffer`, so a chunk ending exactly at `data: …\n` +(blank line not yet arrived) silently loses the event. Verified: chunks +`"data: {\"n\":1}\n"` + `"\ndata: {\"n\":2}\n\n"` yield only event 2. +Single-chunk delivery (as in the tests) works, which is why the suite +passes. + +Additional defects in the same parser: per-chunk `String::from_utf8_lossy` +(`:357`) corrupts multi-byte characters split at a chunk boundary (JSON +parse failure → event degraded to raw string); the trailing partial line +has no length cap (unbounded buffering); an event pending at EOF is +dropped (SSE says dispatch at EOF). For a subscription forwarder this is +silent data loss with no error signal. + +Fix: an incremental **byte-level** parser carrying buffer state across +chunks (decode UTF-8 once over the reassembled buffer, not per chunk). + +## Acceptance Criteria + +- [ ] Multi-chunk test: event split across two TCP chunks is delivered (the review's empirically-verified case — the acceptance gate) +- [ ] Split multi-byte UTF-8 across chunks parses (test) +- [ ] Pending event dispatched at EOF; trailing partial line length-capped (tests) +- [ ] Existing single-chunk SSE tests unchanged and green +- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass + +## References + +- docs/reviews/001-initial-implementation-review.md (Part D, FWD-06; Part I, COV-01) + +## Notes + +> Agent fills during implementation. Parser rewrite is isolated +> from the response-decode fixes (content-type, size caps, error bodies) +> in review-001-response-decoding so the delicate stateful rewrite lands +> alone. Same file — sequence or coordinate. + +## Summary + +> Filled on completion. \ No newline at end of file diff --git a/tasks/client/review-001-client-timeout-retry.md b/tasks/client/review-001-client-timeout-retry.md new file mode 100644 index 0000000..933ca63 --- /dev/null +++ b/tasks/client/review-001-client-timeout-retry.md @@ -0,0 +1,67 @@ +--- +id: review-001-client-timeout-retry +name: Client redirect/Retry-After policy — idempotency, timeouts, caps (FWD-03, FWD-04, FWD-05, FWD-11, FWD-09) +status: pending +depends_on: [] +scope: moderate +risk: medium +impact: component +level: implementation +tags: [client, review-001] +--- + +## Description + +Review 001 findings on the outbound client host +(`src/client/http_client.rs`, `retry_after.rs`), grouped because they all +shape the shared client's request policy: + +- **FWD-03**: no explicit redirect policy → reqwest's default + cross-host redirect scrub removes only `Authorization`/`Cookie`/etc; + `HttpAuthScheme::ApiKey { header_name }` credentials and + `default_headers` follow a 302 to an attacker host intact (verified + against reqwest 0.13 source). Fix: explicit policy — none, or limited + same-host. +- **FWD-04**: `RetryTransientMiddleware` retries POSTs (5xx/408/429/ + timeout classified retryable regardless of method — verified in + reqwest-retry 0.9.1) → duplicate upstream side effects; backoff has no + total-duration cap. Fix: skip retries for non-idempotent methods (or + make idempotency a per-adapter policy) and cap total retry wall time. +- **FWD-05**: `HttpClientConfig::default()` sets `request_timeout: None` + and no connect timeout anywhere; `retry_after.rs:27-37` accepts any u64 + `Retry-After` with no maximum — a hostile backend's 10-year deadline + stalls calls to that URL indefinitely. Fix: default request + connect + timeouts (the gateway's 30 s deadline is the natural anchor) and a + `Retry-After` ceiling (e.g. 300 s, configurable). +- **FWD-11**: `Retry-After` is keyed on the pre-redirect URL, eviction + drops the *earliest* deadline (keeping year-long entries), and all + waiters wake together with no jitter. Key on the effective URL, evict + sensibly, add jitter. +- **FWD-09**: blocking `std::fs::read` in `build_client`, reachable via + the public documented hot-reload path `SharedHttpClient::reload` — + `tokio::fs`/`spawn_blocking` the reads. + +## Acceptance Criteria + +- [ ] Cross-host redirect test with an API-key credential header — key must not reach the redirect target +- [ ] Non-idempotent method is never retried (test); total retry duration bounded (test) +- [ ] Default request + connect timeouts exist in `HttpClientConfig::default()`; `Retry-After` capped (tests) +- [ ] Retry-After keyed post-redirect; eviction and wake behavior fixed or documented (tests) +- [ ] No blocking fs reads on the async path (FWD-09) +- [ ] `HttpClientConfig` defaults documented; review-001-forward-url-safety and this task together close the deployment-facing gate +- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass + +## References + +- docs/reviews/001-initial-implementation-review.md (Part D, FWD-03, FWD-04, FWD-05, FWD-09, FWD-11) +- docs/architecture/decisions/039-http-server-and-client-host-colocated.md + +## Notes + +> Agent fills during implementation. Independent of +> review-001-forward-url-safety (different file); they form the +> deployment gate together. + +## Summary + +> Filled on completion. \ No newline at end of file diff --git a/tasks/gateway/review-001-gateway-publish-semantics.md b/tasks/gateway/review-001-gateway-publish-semantics.md new file mode 100644 index 0000000..8c78b49 --- /dev/null +++ b/tasks/gateway/review-001-gateway-publish-semantics.md @@ -0,0 +1,74 @@ +--- +id: review-001-gateway-publish-semantics +name: /publish + /batch gateway fixes (GW-01, GW-06, GW-08..GW-11, HY-13) +status: pending +depends_on: [] +scope: narrow +risk: medium +impact: component +level: implementation +tags: [gateway, review-001] +--- + +## Description + +Review 001 findings on `src/gateway/routes.rs`'s `/publish` and `/batch` +routes: + +- **GW-01 (major)**: `/publish` (`routes.rs:257-276`) feeds parsed NDJSON + straight to `invoke_sink`; `publish_schema` validation lives only in + alkcall's wire `Dispatcher` — so a Pub op registered with + `publish_schema` receives arbitrary attacker-controlled JSON over HTTP + while the same op over the call protocol aborts invalid chunks. + Handlers written against the validated-wire guarantee get a + transport-dependent invariant. Fix: validate in the route (or move + validation into the shared `invoke_sink` spine so both transports + enforce it — prefer the spine if alkcall's surface allows, else the + route). +- **GW-10**: a first line missing `chunk` silently publishes + `Value::Null` (`routes.rs:213`) — indistinguishable from intent since + null is a legitimate payload. Reject with `INVALID_INPUT` (the route + already rejects a missing `operation` this way). +- **GW-06**: `/publish` buffers the whole NDJSON body (2 MiB-capped) + before dispatch, contradicting ADR-068 step 4. Either stream the body + (axum `Body` → framed stream) — the real fix — or amend ADR-068 to + document the buffered 2 MiB semantics. Decide, then implement or + amend. +- **GW-08**: no cap on batch operation count; the 2 MiB body is the only + bound and the 30 s deadline is unenforced (GW-05). Cap batch size + (constant, e.g. 100). +- **GW-09**: internal-op batch entries emit `request_id: null` while + dispatched entries carry a UUID — one response body, two envelope + shapes. Generate request ids for internal-op entries. +- **GW-11**: `/publish` runs four registry lookups + ACL checks that + `invoke_sink` then repeats — drop the redundant pre-checks (mirror + `/call`/`/batch`, which deliberately rely on the registry). +- **HY-13**: the vacuous test at `routes.rs:1572-1583` + (`publish_body_is_fully_consumed_before_dispatch_not_required`) cites a + socket-level test that doesn't exist — wire the early-disconnect test + or delete the stub. + +## Acceptance Criteria + +- [ ] `/publish` test with a `publish_schema`-registered Pub op rejects an invalid chunk (review's gate for this unit) +- [ ] First line without `chunk` → `INVALID_INPUT`, not a null publish (test) +- [ ] GW-06 decision landed: true streaming or ADR-068 amended; consistent tests + docs +- [ ] Batch size capped (test); mixed-shape batch envelopes fixed +- [ ] Redundant `/publish` pre-checks removed (dispatch still enforces) +- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass + +## References + +- docs/reviews/001-initial-implementation-review.md (Part C, GW-01, GW-06, GW-08..GW-11; HY-13) +- docs/architecture/decisions/068-gateway-publish-endpoint.md +- docs/architecture/decisions/023-operation-error-schemas.md + +## Notes + +> Agent fills during implementation. If the GW-01 fix goes into +> alkcall's `invoke_sink` spine, coordinate the alkcall change (small, +> additive) and note it in the summary. + +## Summary + +> Filled on completion. \ No newline at end of file diff --git a/tasks/gateway/review-001-gateway-stream-errors.md b/tasks/gateway/review-001-gateway-stream-errors.md new file mode 100644 index 0000000..bbbe86e --- /dev/null +++ b/tasks/gateway/review-001-gateway-stream-errors.md @@ -0,0 +1,75 @@ +--- +id: review-001-gateway-stream-errors +name: Gateway error-fidelity — SSE terminality, mappings, Retry-After, deadline (GW-03..GW-07, GW-12..GW-14) +status: pending +depends_on: [] +scope: narrow +risk: low +impact: component +level: implementation +tags: [gateway, review-001] +--- + +## Description + +Review 001 gateway error-fidelity findings +(`src/gateway/routes.rs`, `error.rs`, `dispatch.rs`) — the response a +caller sees must match the documented contract and match across +transports: + +- **GW-04**: SSE error events are not terminal — the stream continues + after `Err` (`routes.rs:299-313`) while the wire dispatcher treats + `Err` as terminal; two transports disagree about stream semantics + (http-server.md:219-223 documents terminal). Emit the error frame and + end the stream (`take_while`). +- **GW-03**: `INVALID_OPERATION_TYPE` → 500 on `/call`//`/batch` (client + fault reported as server fault; pollutes alerting) but 400 on + `/publish`. Map consistently (400 or 422) and document in + http-server.md. +- **GW-05**: the 30 s `DEFAULT_TIMEOUT` deadline is recorded + (`dispatch.rs:34,165`) but never enforced — a hung handler holds the + request open indefinitely. Either enforce (`tokio::time::timeout` + around Once-op invokes) or remove the dead metadata. Prefer enforcing; + it is the documented contract. +- **GW-07**: `Retry-After` machinery exists in + `call_error_to_http_response` (`error.rs:61-75`) but the main gateway + error path builds responses by hand (`routes.rs:319-333`) — retryable + `HTTP_429`/`HTTP_503` reach callers with no `Retry-After` despite the + documented mapping. Route gateway error responses through the shared + mapper. +- **GW-12**: ACL denial on `/subscribe` surfaces as HTTP 200 + + `event:error` while `/call` returns 401/403. ADR-049 makes + 200-on-stream defensible, but the doc must call out the asymmetry — + coordinate with review-001-output-projection (PRJ-05) which documents + it. +- **GW-13**: no SSE keep-alive/heartbeat (`routes.rs:172`) — quiet-but- + alive streams (the normal state for subscriptions) die at LB/proxy + idle timeouts. Add keep-alive + `retry:` field. +- **GW-14**: stale module doc (`routes.rs:1-9` claims `/publish` is "a + separate module"; AGENTS.md §7's "5 gateway endpoints" framing is + similarly stale — ADR-068 made it 6). + +## Acceptance Criteria + +- [ ] SSE stream ends after an error event (test); keep-alive present +- [ ] `INVALID_OPERATION_TYPE` maps to the same status on `/call`, `/batch`, `/publish`; http-server.md table updated +- [ ] Once-op invokes enforce the 30 s deadline (test with a hung handler) +- [ ] Retryable errors carry `Retry-After` on all live error paths (test) +- [ ] GW-14 docs fixed (module doc; AGENTS.md gateway-endpoint count if touched) +- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass + +## References + +- docs/reviews/001-initial-implementation-review.md (Part C, GW-03..GW-07, GW-12..GW-14) +- docs/architecture/decisions/049-streaming-handler-for-subscriptions.md +- docs/architecture/decisions/023-operation-error-schemas.md + +## Notes + +> Agent fills during implementation. Independent of +> review-001-gateway-publish-semantics (both touch routes.rs — +> sequence or coordinate to avoid churn). + +## Summary + +> Filled on completion. \ No newline at end of file diff --git a/tasks/infra/review-001-dependency-hygiene.md b/tasks/infra/review-001-dependency-hygiene.md new file mode 100644 index 0000000..554f154 --- /dev/null +++ b/tasks/infra/review-001-dependency-hygiene.md @@ -0,0 +1,61 @@ +--- +id: review-001-dependency-hygiene +name: Dependency and feature hygiene (HY-01, HY-05, HY-08, HY-12, CON-10) +status: pending +depends_on: [] +scope: narrow +risk: trivial +impact: project +level: implementation +tags: [infra, review-001, cargo] +--- + +## Description + +Review 001 hygiene findings that are pure Cargo.toml / dependency work — +mechanical, parallelizable, worth landing any time: + +- **HY-01 [major]**: `openapiv3` is a production dependency used only + inside `#[cfg(test)]` (`to_openapi.rs:1103`). Move to + `[dev-dependencies]`. +- **HY-05**: `bytes` declared, zero direct use in src/ (only axum's + re-export); `parking_lot` declared, zero direct use; `tokio` features = + `["full"]` drags in `process`/`signal` extras. Prune to what's used. +- **HY-08**: `tokio-tungstenite` declared three times with different + feature sets (`wss`, `test-support`, dev-deps); make `test-support` + extend `wss` (also resolves CON-10's class of fragility) — note + `required-features` fix itself is tracked in + review-001-consumer-adapter-hygiene (CON-10). +- **HY-12**: duplicate dependency roots (`getrandom 0.3/0.4`, + `cpufeatures 0.2/0.3`) driven by `tokio-tungstenite 0.28` here vs + `tungstenite 0.29` via axum's ws — align versions to collapse. +- Related slack: HY-07 (`READ_SLOTS` pub constant leak in a public + module — privatize or document) can ride along. + +Deliberately deferred to a later pass: HY-02 (the 110-warning +`missing_docs` sweep — better done once the remediation stabilizes the +API), HY-04 (test-support `unwrap`, documented as intentional), HY-06 +(`ExponentialBackoff` in public API — revisit when the client config +settles), HY-10 (stale docs — several overlap fixes landing in other +tasks), HY-11 (`docs/` in the package — needs a publishing decision). + +## Acceptance Criteria + +- [ ] `openapiv3` in dev-dependencies only +- [ ] `cargo tree -d` duplicate roots reduced (getrandom/cpufeatures collapse) +- [ ] Unused deps pruned; tokio features tightened (build passes in default, `mcp`-only, `wss`, `--all-features`, `--no-default-features`) +- [ ] HY-07 resolved (privatized or documented) +- [ ] Full feature matrix still green: default, `--all-features`, `mcp`-only, `wss`-only, `--no-default-features` + +## References + +- docs/reviews/001-initial-implementation-review.md (Part H, HY-01, HY-05, HY-07, HY-08, HY-12) + +## Notes + +> Agent fills during implementation. Safe to land any time; keep it +> separate from behavioral tasks so reverts are clean. + +## Summary + +> Filled on completion. \ No newline at end of file diff --git a/tasks/server/review-001-extra-routes-auth.md b/tasks/server/review-001-extra-routes-auth.md new file mode 100644 index 0000000..9f57bf3 --- /dev/null +++ b/tasks/server/review-001-extra-routes-auth.md @@ -0,0 +1,52 @@ +--- +id: review-001-extra-routes-auth +name: Mount extra_routes under the bearer-auth middleware (SRV-01) +status: pending +depends_on: [] +scope: narrow +risk: medium +impact: component +level: implementation +tags: [server, review-001, security] +--- + +## Description + +Review 001 finding SRV-01 (`docs/reviews/001-initial-implementation-review.md`, +empirically verified): `build_router` +(`src/server/adapter.rs:170-183`) applies `bearer_auth_middleware` via +`route_layer` **before** merging `extra_routes`, so assembly-layer custom +routes are mounted without auth — contradicting ADR-046 §4 ("custom routes +carry the same auth middleware by default"). Amplified by +`ResolvedIdentity` extraction being infallible: a custom handler silently +receives `None` on every request. + +Fix: apply the auth layer **after** merging extra routes so the documented +default holds. Per-route opt-out remains the deployment's explicit choice +(extras may carry their own inner layers). Also decide and document the +SRV-06 interaction: a same-method collision on a reserved path panics in +axum's `merge` (sanctioned), but a different-method merge (e.g. custom +`POST /search`) silently serves on a reserved path — enforce +`RESERVED_PATHS` or document the behavior; the exported constant currently +has no reader. + +## Acceptance Criteria + +- [ ] A test mounts an extra route and asserts `ResolvedIdentity` is resolved from the bearer token (auth applies) +- [ ] A test shows an extra route carrying its own layer can still opt out (documented escape hatch) +- [ ] SRV-06 decision landed: per-method reserved-path merges rejected/documented; `RESERVED_PATHS` enforced or un-exported +- [ ] ADR-046 §4 language matches the implemented default after the fix +- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass + +## References + +- docs/reviews/001-initial-implementation-review.md (Part A, SRV-01, SRV-06) +- docs/architecture/decisions/046-assembly-layer-custom-http-routes.md + +## Notes + +> Agent fills during implementation. + +## Summary + +> Filled on completion. \ No newline at end of file diff --git a/tasks/server/review-001-hyper-server-knobs.md b/tasks/server/review-001-hyper-server-knobs.md new file mode 100644 index 0000000..04d107d --- /dev/null +++ b/tasks/server/review-001-hyper-server-knobs.md @@ -0,0 +1,67 @@ +--- +id: review-001-hyper-server-knobs +name: Configure hyper timeouts + decoy/405/proxy-path fixes (SRV-04, SRV-07, SRV-08, SRV-09, SRV-10) +status: pending +depends_on: [] +scope: moderate +risk: low +impact: component +level: implementation +tags: [server, review-001, stealth] +--- + +## Description + +Review 001 server-core minors, grouped because they are small, disjoint, +and all land in `src/server/`: + +- **SRV-04**: `HyperBuilder` (`src/server/adapter.rs:217-224`) sets no + timer — hyper 1.11 *silently ignores* `header_read_timeout` without one + (verified against hyper source), so the slow-loris surface is open and + every keep-alive knob is off. Set a timer (tokio) + `header_read_timeout` + + h1/h2 keep-alive knobs. No concurrency cap exists in this crate (the + accept loop is the consumer's) — document that boundary in the module + doc. +- **SRV-05**: `with_decoy` consumes `extra_routes` via `.take()` + (`adapter.rs:104`); a second builder call silently rebuilds without the + custom routes. Clone (or error on double-build). +- **SRV-07**: the decoy is only the router `fallback` — method-mismatch + requests return axum's bare 405 with no `Server: nginx` header, which + single-handedly distinguishes alkhttp from nginx in decoy deployments + (empirically verified). Wrap 405s with the decoy response. +- **SRV-08**: `percent_decode` maps `+`→space (wrong in a URI path) and + per-byte→char (mojibake for non-ASCII filenames) — `decoy.rs:106-127`. + Fix with proper UTF-8 percent-decoding; also move the blocking + `is_dir()`/`is_file()` syscalls off the async path. +- **SRV-09**: `/openapi.json` error path echoes raw serde internals to + unauthenticated callers and re-serializes the whole projection per + request (`adapter.rs:250-254`); companion unguarded + `.expect("to_openapi always emits…")` in `to_openapi.rs:67`. Cache the + serialized doc, return a generic 500 body, remove the `expect`. +- **SRV-10**: WS upgrade hardcodes `NoCap` while the doc claims a + stricter policy is passable — add the injection point or fix the doc; + drop the double token resolution (router middleware + `ws_bearer_auth`). + +## Acceptance Criteria + +- [ ] Hyper configured with timer + header read timeout; knob values documented +- [ ] 405 responses carry the decoy `Server` header (test with `OPTIONS /search`) +- [ ] `%C3%A9` and `a+b.html` resolve correctly in the decoy static server (tests); async-path syscalls gone +- [ ] `/openapi.json` 500 body is generic; doc cached; `expect` removed +- [ ] Second `.with_decoy` no longer silently drops extra routes (test) +- [ ] SRV-10: policy injection point exists or doc corrected; single token resolution +- [ ] `cargo test`, `cargo test --all-features`, `cargo clippy --all-targets -- -D warnings` pass + +## References + +- docs/reviews/001-initial-implementation-review.md (Part A, SRV-04..SRV-10) +- docs/architecture/decisions/010-alpn-router-and-endpoint.md + +## Notes + +> Agent fills during implementation. Independent of the other +> review-001 tasks; can proceed in parallel. + +## Summary + +> Filled on completion. \ No newline at end of file diff --git a/tasks/server/review-001-mcp-body-limit.md b/tasks/server/review-001-mcp-body-limit.md new file mode 100644 index 0000000..219e1c0 --- /dev/null +++ b/tasks/server/review-001-mcp-body-limit.md @@ -0,0 +1,48 @@ +--- +id: review-001-mcp-body-limit +name: Cap the /mcp nest body size (SRV-03, gated on mcp feature) +status: pending +depends_on: [] +scope: narrow +risk: low +impact: component +level: implementation +tags: [server, review-001, security, mcp] +--- + +## Description + +Review 001 finding SRV-03 (mcp feature only): `src/server/adapter.rs:141-150` +nests rmcp's `StreamableHttpService`; the bearer middleware only stashes +identity. axum's 2 MiB `DefaultBodyLimit` applies to axum *extractors*, +but the nested rmcp service collects the raw body itself +(`body.collect().await`, verified against rmcp 1.8.0) with no cap — a +single multi-GB chunked `POST /mcp` is buffered entirely in memory; a few +concurrent requests OOM the process. Gateway routes are correctly capped +at 2 MiB by the extractors; `/mcp` is the one uncapped surface. + +Fix: wrap the `/mcp` nest with an explicit `DefaultBodyLimit` (or an +equivalent body-limit layer) sized for MCP traffic. Pick and document the +limit (a JSON-RPC batch is the largest legitimate body; something in the +2–8 MiB range is defensible) rather than leaving it at hyper's unlimited +default. + +## Acceptance Criteria + +- [ ] Explicit body limit applied to the `/mcp` nest; limit value documented in code and ADR-039 or http-server.md if touched +- [ ] Test: oversized `POST /mcp` body → 413 (Content-Length and streaming/chunked variants) +- [ ] Normal-size MCP initialize + tools/call round-trip still passes +- [ ] `cargo test --all-features` passes (feature-gated code) + +## References + +- docs/reviews/001-initial-implementation-review.md (Part A, SRV-03) +- docs/architecture/decisions/039-http-server-and-client-host-colocated.md + +## Notes + +> Agent fills during implementation. + +## Summary + +> Filled on completion. \ No newline at end of file diff --git a/tasks/server/review-001-schema-internal-visibility.md b/tasks/server/review-001-schema-internal-visibility.md new file mode 100644 index 0000000..43f1d04 --- /dev/null +++ b/tasks/server/review-001-schema-internal-visibility.md @@ -0,0 +1,60 @@ +--- +id: review-001-schema-internal-visibility +name: Enforce Internal-op invisibility on GET /schema (SRV-02, PRJ-06, GW-02) +status: pending +depends_on: [] +scope: narrow +risk: medium +impact: component +level: implementation +tags: [server, gateway, review-001, security] +--- + +## Description + +Review 001 findings SRV-02 + PRJ-06 + GW-02 — three faces of one +visibility gap on the discovery axis: + +- **SRV-02** (`src/gateway/routes.rs:120-137`): `schema_handler` runs only + `access_check_for_op`; it skips the `is_internal_op` pre-check that + `/call`, `/batch`, `/subscribe`, `/publish` all apply. Internal ops carry + `AccessControl::default()`, so an unauthenticated caller can fetch the + full spec (visibility, schemas, `access_control`) of an op it cannot + call. `POST /call` on the same op correctly 404s (tested); `GET /schema` + does not — the invisibility invariant (ADR-015 §2) is defeated on the + discovery axis. +- **PRJ-06** (`src/adapters/to_mcp.rs:148-174`): the MCP `schema` tool has + the identical gap — no per-op pre-check, full spec including + `required_scopes` returned for forbidden ops. The test + `schema_returns_full_operation_spec` enshrines the leak. +- **GW-02**: `/search` and `/schema` are per-identity GETs with no + `Cache-Control: no-store` / `Vary: Authorization`, so shared caches may + serve caller A's filtered response to caller B. + +Fix all three in one pass since they share the pre-check logic and test +fixtures: add the 404 guard to `schema_handler`; run the same +`access_check_for_op` pre-check in the MCP `schema` tool (or filter the +returned spec per identity) and fix the enshrining test; add cache headers +to the two per-identity GETs. + +## Acceptance Criteria + +- [ ] `GET /schema?name=` → 404 unauthenticated, for an unauthorized identity, and for an anonymous identity (test) +- [ ] MCP `schema` tool denies/404s unauthorized ops symmetrically with HTTP `/schema` (test); enshrining test fixed +- [ ] `/search` + `/schema` responses carry `Cache-Control: no-store` (and `Vary: Authorization` where a token can change the body) +- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass +- [ ] `cargo test --all-features` passes (the MCP half is feature-gated) + +## References + +- docs/reviews/001-initial-implementation-review.md (SRV-02, PRJ-06, GW-02) +- docs/architecture/decisions/015-privilege-model-and-authority-context.md +- docs/architecture/decisions/041-mcp-tool-gateway-pattern.md + +## Notes + +> Agent fills during implementation. + +## Summary + +> Filled on completion. \ No newline at end of file diff --git a/tasks/websocket/review-001-ws-data-channel-decision.md b/tasks/websocket/review-001-ws-data-channel-decision.md new file mode 100644 index 0000000..dd18910 --- /dev/null +++ b/tasks/websocket/review-001-ws-data-channel-decision.md @@ -0,0 +1,50 @@ +--- +id: review-001-ws-data-channel-decision +name: Reconcile ADR-067 browser data channels with the v1 implementation (WS-03, planning) +status: pending +depends_on: [] +scope: narrow +risk: low +impact: project +level: planning +tags: [websocket, review-001, planning] +--- + +## Description + +Review 001 finding WS-03: ADR-067 (websocket.md §"Data channels for +browsers") promises browser-opened data channels and ADR-048 promises bidirectionality +via the connection-local overlay, but the implementation hands the base +registry to `install_channel_zero` and runs +`Dispatcher::run_loop_single_stream` (`src/websocket/upgrade.rs:40,62-98`); +no `ChannelCore`/`register_openable`/`ChannelOperations` wiring exists +anywhere (grep-verified). A browser can never open a data channel — the +capability ADR-067 says the channels design exists to provide. + +This may have been a deliberate v1 cut (the WS tasks scoped channel-0 +dispatch only), but the spec promise and the implementation have not been +reconciled in writing. This is a **decision task**, not an implementation +task: either (a) wire the data-channel path, or (b) file the OQ / amend +ADR-067 (+ ADR-048's overlay contract) to defer with a rationale. + +## Acceptance Criteria + +- [ ] Decision made and recorded (implementation task + ADR/OQ update, or ADR-067 amendment) +- [ ] If v1-cut: ADR-067 and ADR-048 carry the reconciliation note; the gap is not silently silent +- [ ] If in-scope: a follow-up task file exists with scoped acceptance criteria (incl. a browser-opened-channel test) + +## References + +- docs/reviews/001-initial-implementation-review.md (Part B, WS-03) +- docs/architecture/decisions/067-websocket-carries-channels.md +- docs/architecture/decisions/048-websocket-native-session-not-gateway.md + +## Notes + +> Agent fills during implementation. Deliberately sequenced as +> planning so the WS-01/02 robustness work is not blocked on a design +> discussion. + +## Summary + +> Filled on completion. \ No newline at end of file diff --git a/tasks/websocket/review-001-ws-eof-signal.md b/tasks/websocket/review-001-ws-eof-signal.md new file mode 100644 index 0000000..8f3252e --- /dev/null +++ b/tasks/websocket/review-001-ws-eof-signal.md @@ -0,0 +1,65 @@ +--- +id: review-001-ws-eof-signal +name: Lossless EOF signal + pending-map sweep for from_wss (WS-02, CON-02) +status: pending +depends_on: [] +scope: narrow +risk: high +impact: component +level: implementation +tags: [websocket, adapters, review-001, from-wss] +--- + +## Description + +Review 001 findings WS-02 + CON-02 — one mechanism, verified end-to-end: + +`src/websocket/byte_adapter.rs:138-139` (and the tungstenite twin at +`:319`) fires `read_eof.notify_waiters()`, which wakes only +*already-registered* waiters and stores no permit. `from_wss` spawns its +drop-monitor *after* session setup (`from_wss.rs:156-166`); if the read +task hits EOF before the monitor first polls `Notified`, the signal is +lost. `import()` does `std::mem::forget(session)` (`:193`), so the +`close_rx` fallback never fires either — the monitor never runs +`fail_all`, and in-flight imported-op calls hang (Once-calls recover only +at the 30 s sweeper *if* a sweeper runs; CON-02 establishes it doesn't on +this path — `Dispatcher::run_loop`'s sweeper is never taken; `Sub`/`Pub` +pendings hang forever). The module doc at `from_wss.rs:111-113` promises +the opposite of the behavior. + +Fix both halves: + +- Replace `notify_waiters` with a permit-storing signal: `tokio::sync::watch`, + `CancellationToken`, or a checked `AtomicBool` — anything a late + subscriber observes. Apply to both the axum and tungstenite pump paths. +- Extend the from_wss monitor to sweep the pending map periodically while + the session lives (or otherwise ensure post-`fail_all` registrations + still resolve), since calls registered after the one-shot `fail_all` + are currently never resolved. + +Acceptance gates from the review: (1) a from_wss test that drops the +connection **while a call is being registered** — the CON-02 race — with +no hang; (2) the module doc's promise ("no hang") becomes true. + +## Acceptance Criteria + +- [ ] EOF-notify is stored (late subscriber observes it) — race test: drop during session setup/first call registration resolves all in-flight calls as retryable +- [ ] Post-`fail_all`-registered pendings also resolve (sweep or equivalent), not hang forever +- [ ] The `connection_drop_fails_in_flight_calls_retryable_no_hang` test remains green; add the racing-drop variant (COV gap 10) +- [ ] Module doc at `from_wss.rs:111-113` matches implemented behavior +- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass + +## References + +- docs/reviews/001-initial-implementation-review.md (Part B, WS-02; Part G, CON-02; COV-03) +- docs/architecture/decisions/070-from-wss-consumer-adapter.md + +## Notes + +> Agent fills during implementation. Highest-priority WS fix — lossy +> notification hangs calls; everything else in the WS subsystem can +> follow. + +## Summary + +> Filled on completion. \ No newline at end of file diff --git a/tasks/websocket/review-001-ws-pump-consolidation.md b/tasks/websocket/review-001-ws-pump-consolidation.md new file mode 100644 index 0000000..2483478 --- /dev/null +++ b/tasks/websocket/review-001-ws-pump-consolidation.md @@ -0,0 +1,58 @@ +--- +id: review-001-ws-pump-consolidation +name: Dedupe pumps + byte-based caps + write-side validation (WS-11, WS-04, WS-05, WS-06, HY-09) +status: pending +depends_on: [] +scope: moderate +risk: medium +impact: component +level: implementation +tags: [websocket, review-001] +--- + +## Description + +Review 001 findings where the correct fix is restructuring the duplicated +pump code in `src/websocket/byte_adapter.rs`, so they are done once +instead of twice (axum path `:119-180`, tungstenite twin `:299-364`): + +- **WS-11**: ~60 lines of pump logic (read task, write task, close + handling) are copy-pasted between the axum and tungstenite paths while + the module doc claims "one implementation, both directions". Factor + over a generic sink/stream of messages. This also closes COV-03 (the + tungstenite twins are untested). +- **WS-04 + HY-09**: the write-side chunk parser does no length + validation (`:161-163`, twin `:343-345`) — non-chunk-framed bytes make + the parser silently wait to accumulate `8 + len` (up to ~4 GiB) from + misaligned offsets. Validate `len > MAX_CHUNK_LEN` → fail the stream + loudly; saturating add for the 32-bit overflow. +- **WS-05**: the write-side `pending` buffer is bounded in slots (64) but + not in bytes (~1 GiB worst case per connection with a slow sink). + Cap `pending` growth in bytes. +- **WS-06**: inbound per-connection memory bound is 64 slots × 64 MiB + (axum/tungstenite defaults) ≈ 4 GiB — `max_message_size`/ + `max_frame_size` are never configured. Set explicit caps consistent + with the plan's ~1 MiB write-side intent. + +## Acceptance Criteria + +- [ ] Pump bodies factored into one generic implementation (both paths share it) +- [ ] Write-side rejects `len > MAX_CHUNK_LEN` with a stream error (test); no `8 + len` overflow path +- [ ] Write `pending` byte-capped; inbound message/frame size explicitly configured (tests) +- [ ] Tungstenite path exercised by the shared test suite (COV-03 backfilled) +- [ ] 16 MiB round-trip and disconnect-cleanup tests still pass +- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass + +## References + +- docs/reviews/001-initial-implementation-review.md (Part B, WS-04, WS-05, WS-06, WS-11; HY-09; COV-03) + +## Notes + +> Agent fills during implementation. Sequenced with (or after) +> review-001-ws-eof-signal so the dedup doesn't churn under it — both +> touch byte_adapter.rs. + +## Summary + +> Filled on completion. \ No newline at end of file diff --git a/tasks/websocket/review-001-ws-session-limits.md b/tasks/websocket/review-001-ws-session-limits.md new file mode 100644 index 0000000..89262ff --- /dev/null +++ b/tasks/websocket/review-001-ws-session-limits.md @@ -0,0 +1,64 @@ +--- +id: review-001-ws-session-limits +name: WS session lifecycle — idle timeout, shutdown, teardown, caps (WS-01, WS-07..WS-10) +status: pending +depends_on: [review-001-ws-eof-signal] +scope: narrow +risk: medium +impact: component +level: implementation +tags: [websocket, review-001] +--- + +## Description + +Review 001 WS subsystem lifecycle/staleness findings over +`src/websocket/upgrade.rs` and `byte_adapter.rs`: + +- **WS-01 (major)**: the single demux loop means one dribbled chunk + stalls all channels indefinitely — alkcall's demux has no read timeout + and a peer header `[ch=0][len=16 MiB]` + one byte/ minute parks the + allocation and hangs every outstanding channel-0 call (Sub/Pub pendings + until the socket dies). Add a configurable idle timeout on the WS read + that closes with 1001 on staleness (deployment knob; default bounds the + stall). +- **WS-07**: `poll_shutdown` drops a *fresh clone* of `write_tx` + (`byte_adapter.rs:269-276`) so the channel never closes and the + documented `ws_sink.close()` never runs at shutdown — the `from_wss` + path never emits a WS Close frame at all. Drop the held sender. +- **WS-08**: `_pumps` is dropped immediately (`upgrade.rs:36`); the + documented forced-teardown lever `WsPumps::abort()` is never callable + on the server path. Retain the handle so a stuck session is evictable + in-crate. +- **WS-09**: no cap on WS sessions (post-auth DoS); the assembly layer + cannot add one because the route is built inside `HttpAdapter`. Add a + semaphore in `ws_upgrade_handler` (configurable; document the default). +- **WS-10**: dispatcher/mux tasks outlive a failed session task + (`upgrade.rs:65-97`) — self-healing but a peer-behavior-tied leak + window. Document the semantics (the cheap fix) or tie task lifetimes + to the session. +- Fold-in from review-001-hyper-server-knobs: SRV-10's policy-injection + point (channel cap) belongs here if that task doesn't land it. + +## Acceptance Criteria + +- [ ] Idle timeout bounds a dribble stall (test: dribbling peer's channels fail/bound rather than hang forever) +- [ ] `shutdown()` causes a WS Close frame to the peer (test) +- [ ] `_pumps` retained; `abort()` reachable on the server path +- [ ] Session concurrency cap configurable in `HttpAdapter`; enforced (test) +- [ ] Detached-task lifetime semantics documented (WS-10) +- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass + +## References + +- docs/reviews/001-initial-implementation-review.md (Part B, WS-01, WS-07..WS-10) +- docs/architecture/decisions/048-websocket-native-session-not-gateway.md + +## Notes + +> Agent fills during implementation. Takes the EOF-signal task first +> (same files, and the lifecycle knobs build on the lossless signal). + +## Summary + +> Filled on completion. \ No newline at end of file