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