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
This commit is contained in:
@@ -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<String>`; use `Secret<String>` 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<String>`
|
||||
- [ ] `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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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_<status>` mapping as is (correct).
|
||||
- **FWD-12**: byte-identical duplicates `value_to_path_segment`/
|
||||
`value_to_query` (`:133-151`); `forward`'s `op_type` parameter only
|
||||
toggles `ACCEPT` and its `text/` branch would buffer an entire SSE
|
||||
stream if a Sub were ever routed here (currently unreachable, inviting
|
||||
misuse — remove or make safe); the `unwrap_or_else(|_| "null")` at
|
||||
`:197` masks an unreachable serialization failure with a null body;
|
||||
`SharedHttpClient::reload` performs two separate `ArcSwap::store` calls.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] `application/vnd.api+json` (and `application/problem+json`) decode as JSON, not byte arrays (test)
|
||||
- [ ] Response size cap enforced on JSON/text/binary read paths (test)
|
||||
- [ ] Non-SSE content-type on a Sub op produces a loud error, not an empty stream (test)
|
||||
- [ ] Invalid credential values fail loudly at call time (test), no credential material logged
|
||||
- [ ] Upstream 4xx/5xx bodies surface bounded in the error envelope (test)
|
||||
- [ ] Duplicate `value_to_*` helpers collapsed; `text/` buffering branch removed or safe; reload atomic
|
||||
- [ ] `forward.rs` COV-01 uncovered regions (binary branch, auth arms, non-2xx mapping, content-type branches) substantially covered
|
||||
- [ ] `cargo test` and `cargo clippy --all-targets -- -D warnings` pass
|
||||
|
||||
## References
|
||||
|
||||
- docs/reviews/001-initial-implementation-review.md (Part D, FWD-07, FWD-08, FWD-10, FWD-12; Part I, COV-01)
|
||||
|
||||
## Notes
|
||||
|
||||
> Agent fills during implementation. Shares forward.rs with
|
||||
> review-001-forward-url-safety (request side) and
|
||||
> review-001-sse-parser — sequence those first or coordinate.
|
||||
|
||||
## Summary
|
||||
|
||||
> Filled on completion.
|
||||
@@ -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.
|
||||
Reference in New Issue
Block a user