docs(tasks): re-validate bracketed follow-up into 3 tasks (OAI-18 decision, streaming-coverage residue, doc-gate fix)
The bracketed planning task's five candidates, re-validated against
the post-bulk tree (0a932e5, all 23 bulk tasks done, 446/568 green):
1. OAI-18 -> review-002-fu-oai18-decision: still key-allowlist-only;
bulk blockers (yaml/path-item) both landed. Task records the
enforce-vs-scope-the-advert decision framework from review 002.
2. CON-08/CON-09 close() lever: deferred still (documented v1
contracts unchanged, no consumer pressure) — not tasked.
3. Cross-crate: CF-001..004 all open in alkcall, no alkhttp-side task;
PRJ-16 guard already documents its defense-in-depth re-scope
disposition (no work when CF-004 lands).
4. Stale-comment sweep: mostly absorbed by the bulk (SRV-11 comment
rewritten in-fix); residue = 3 cargo-doc warnings + WsTimeouts
redundant Default -> review-002-fu-doc-warnings.
5. ADR-045 version audit: clean (1.4.0 + annotations + test pin).
Post-bulk coverage re-pass (95.89% regions, bulk's new code fully
covered: PRJ-16 guard, body cap, router reorder, batch cap, WS
knobs, OAI-11 node budget all exercised) surfaced one unanticipated
residue -> review-002-fu-stream-error-coverage (forward_stream
build-error arm never exercised on a Sub op; PEM read-failure arms;
over-cap poll_write; SSE split-CRLF edge; from_value structural
rejects; WsTimeouts Default).
Bracketed pass itself marked completed (planning consumed).
taskgraph: 69 valid, no cycles; 3 pending.
This commit is contained in:
@@ -0,0 +1,111 @@
|
||||
---
|
||||
id: review-002-fu-oai18-decision
|
||||
name: OAI-18 advertise-vs-enforce decision — enforce leaf constraints or scope the advert (OAI-18)
|
||||
status: pending
|
||||
depends_on: []
|
||||
scope: moderate
|
||||
risk: medium
|
||||
impact: component
|
||||
level: implementation
|
||||
tags: [adapters, review-002, follow-up, from-openapi, from-jsonschema]
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Bracketed follow-up item 1 — the one OAI finding the review-002 bulk
|
||||
deliberately deferred (Unit 4 landed OAI-12/13 and the loudness
|
||||
cluster; OAI-18 was re-evaluated afterwards per the bracket plan). The
|
||||
decision was re-validated against the post-bulk tree (43 commits,
|
||||
`0a932e5`+): the deferred status still stands, and this task now
|
||||
carries it.
|
||||
|
||||
**The gap (review 002 OAI-18, verified at tree `91483a7`):**
|
||||
`enforce_input_schema` (`src/adapters/forward.rs:455-499`) enforces a
|
||||
**key allowlist** only. The `/schema` surface advertises full JSON
|
||||
Schema (`required`, `enum`, `pattern`, `minLength`, value types) via
|
||||
`OperationSpec.input_schema`, but at call time:
|
||||
|
||||
- `required: [id]` accepts `{}` (the missing key forwards → upstream
|
||||
422 instead of the gateway's own `INVALID_INPUT 422`)
|
||||
- a `{"type": "string"}` property sent as an object serializes as JSON
|
||||
text into the query string (or a header value)
|
||||
- `enum`/`minimum`/`pattern` are never consulted
|
||||
|
||||
Advertise/enforce drift: the contract the gateway publishes is broader
|
||||
than the one it defends. The review-001 OAI-02 *unknown-key* vector
|
||||
(undeclared keys shaping the request) is closed; this is the
|
||||
declared-but-unchecked remainder. Nothing changed this in the bulk —
|
||||
re-verified `forward.rs:455-499` is still membership-only at `0a932e5`.
|
||||
|
||||
**The decision this task must make** (implement with the decision, or
|
||||
record its explicit rejection):
|
||||
|
||||
- **(a) Enforce leaf constraints**: compile the full input schema per
|
||||
op with the `jsonschema` crate (already a dependency via
|
||||
`gateway/schema_cache.rs`) and validate peer input at call time,
|
||||
reusing the `PublishSchemaCache` compile-once pattern. This makes
|
||||
advertise == enforce exactly, at the cost of a per-op compile and
|
||||
schema-dialect semantics (2020-12; same dialect publish_schema
|
||||
already uses, so the crate is consistent).
|
||||
- **(b) Scope the advert**: instead of enforcing, project a
|
||||
reduced input schema into `/schema` — describe only what is enforced
|
||||
(keys + types, drop `required`/`enum`/`pattern` from what
|
||||
`spec_to_json` advertises) — so the contract and the enforcement
|
||||
match while the *spec* stays the configuration source. (Downside:
|
||||
information loss for well-behaved consumers; the spec-derived
|
||||
detail is genuinely useful documentation for humans.)
|
||||
|
||||
The review-002 assessment leaned (a) — the crate already pays the
|
||||
compile-cache complexity for `publish_schema`, and the advertise/enforce
|
||||
asymmetry is exactly the class of drift review 002 filed as
|
||||
[minor→major-class]. Option (b) is legitimate if the assembly-layer
|
||||
trust boundary (ADR-066's "specs are trusted configuration") is judged
|
||||
to make full-schema enforcement unnecessary belt — but note OAI-02's
|
||||
rationale cut the other way: peer *input* is never trusted, and the
|
||||
advertised schema is exactly what a peer reads before crafting input.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] A decision recorded (implement one option, or document a third)
|
||||
in ADR-066 with the FWD-17/18/19 decision-section style
|
||||
- [ ] If (a): input schemas compile once per registration (cache
|
||||
invalidation mirrors `schema_cache.rs`'s value-keyed scheme);
|
||||
peer input failing leaf constraints → `INVALID_INPUT` 422
|
||||
naming the violated keyword — NOT an upstream round-trip; a spec
|
||||
whose input schema fails to compile fails *import* loudly
|
||||
(fail-closed, matching the publish_schema precedent from
|
||||
review-001's follow-up task)
|
||||
- [ ] If (a): the advertised-vs-enforced round-trip test from the
|
||||
review-002 test-gap list lands: construct a doc, import, assert
|
||||
the `/schema`-exported `input_schema` accepts exactly what
|
||||
`build_request` accepts (the missing chain-test that let JS-01
|
||||
and OAI-18 survive two reviews)
|
||||
- [ ] If (b): the advert-stripping behavior is tested (required/enum/
|
||||
pattern absent from `/schema` output while the original spec
|
||||
retains them internally) and the module doc + ADR-066 say so
|
||||
- [ ] Either way: `cargo test`, `cargo clippy --all-targets --
|
||||
-D warnings`, `cargo fmt --check` pass (`--all-features` if
|
||||
mcp-touched)
|
||||
|
||||
## References
|
||||
|
||||
- docs/reviews/002-post-remediation-review.md (Part E', OAI-18)
|
||||
- src/adapters/forward.rs:455-499 (the key-allowlist), src/gateway/schema_cache.rs (the compile-once pattern to reuse)
|
||||
- docs/architecture/decisions/066-from-jsonschema-as-http-adapter.md (the section to extend), /decisions/023-operation-error-schemas.md
|
||||
- tasks/adapters/review-002-yaml-normalization.md and review-002-oai13-path-item-wildcards.md (the post-bulk advertised-schema surface this decision was deferred against)
|
||||
- docs/reviews/002-post-remediation-review.md Test-gap 10 (the round-trip test)
|
||||
|
||||
## Notes
|
||||
|
||||
This was bracketed explicitly because review-002-yaml-normalization
|
||||
and review-002-oai13-path-item-wildcards both change the
|
||||
advertised-schema surface (wildcards now project with payload schemas;
|
||||
path-item parameters merge in). Those landed; the decision is now
|
||||
unblocked and the surface is stable. The jsonschema dialect question
|
||||
is settled by precedent (`PublishSchemaCache` compiles against the
|
||||
same crate). If (a) is chosen, watch one interaction: the
|
||||
`HEADER_PARAM_IN_MARKER`-decorated and `body` properties are
|
||||
peer-visible schema extensions — the compiled validator must accept
|
||||
them as ordinary properties (they are), and `additionalProperties:
|
||||
true` semantics must keep working (opt-in catch-all — the compiled
|
||||
validator gives you this for free).
|
||||
@@ -1,7 +1,7 @@
|
||||
---
|
||||
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
|
||||
status: completed
|
||||
depends_on: []
|
||||
scope: narrow
|
||||
risk: low
|
||||
@@ -48,20 +48,61 @@ candidates, to be re-validated when picked up:
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] Re-scoped into concrete tasks (or explicitly dismissed per item)
|
||||
- [x] 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)
|
||||
- [x] 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)
|
||||
- tasks/infra/review-001-missing-docs-sweep.md (precedent: opportunistic 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.
|
||||
precede-the-bulk items are marked in each task's Notes where relevant.
|
||||
|
||||
## Summary
|
||||
|
||||
Re-validated against the post-bulk tree (43 commits from e2c255d to
|
||||
0a932e5, all 23 implementation tasks completed; default 446/0,
|
||||
all-features 568/0) — the five candidates resolved to:
|
||||
|
||||
1. **OAI-18 → task `review-002-fu-oai18-decision`.** Still open;
|
||||
`forward.rs:455-499` re-verified as key-allowlist-only at
|
||||
0a932e5. Both bulk dependencies (yaml normalization, path-item
|
||||
wildcards) landed, so the advertised surface is now stable and the
|
||||
decision (enforce via jsonschema vs scope the advert) is unblocked.
|
||||
2. **CON-08/CON-09 close() → deferred (still).** Both the from_mcp
|
||||
teardown note (from_mcp/mod.rs:16-22) and the from_wss OQ-03
|
||||
"v1 = none" disposition stand unchanged; no consumer pressure
|
||||
surfaced during the bulk. The `close()` lever stays a v1.1 item
|
||||
contingent on crates.io timing/consumer feedback — not tasked.
|
||||
3. **Cross-crate re-check → no new alkhttp task.** All four
|
||||
CF-001..004 remain `open` in alkcall's ledger (no alkcall commits
|
||||
since a2d72f9). PRJ-16's alkhttp guard is the designed
|
||||
defense-in-depth (dispatch.rs:36-52 documents the re-scope: when
|
||||
CF-004 lands, the per-transport check STAYS — no re-scope work
|
||||
needed). The ws-eof retryability tightening stays tolerated in
|
||||
from_wss.rs:981 pending CF-001 — no alkcall change, no task.
|
||||
Re-check continues to be a per-alkcall-session item.
|
||||
4. **Stale-comment sweep → residues became `review-002-fu-doc-warnings`**
|
||||
(3 cargo-doc warnings: 2 broken intra-doc links in the PRJ-16 guard
|
||||
doc + 1 redundant link; plus WsTimeouts redundant `Default`). The
|
||||
sweep's other load was already absorbed by the bulk: the SRV-11
|
||||
router comment was rewritten in-fix, ADR-051/066/http-adapters.md
|
||||
all record their bulk decisions.
|
||||
5. **ADR-045 version audit → clean, nothing to do.** `GATEWAY_VERSION`
|
||||
is 1.4.0 with the accumulated per-change annotations in the
|
||||
to_openapi module doc (+ test pin), and http-adapters.md's
|
||||
to_openapi section was aligned (60362c4).
|
||||
|
||||
Bracket discovery beyond the plan: the post-bulk coverage re-pass
|
||||
(95.89% regions; the bulk's own code fully covered) surfaced a small
|
||||
residue the plan hadn't anticipated — most notably the never-exercised
|
||||
`forward_stream` build-error arm → task
|
||||
`review-002-fu-stream-error-coverage`.
|
||||
@@ -0,0 +1,62 @@
|
||||
---
|
||||
id: review-002-fu-doc-warnings
|
||||
name: Fix cargo-doc warnings (dispatch intra-doc links, byte_adapter redundant link) — restore the -D warnings doc gate
|
||||
status: pending
|
||||
depends_on: []
|
||||
scope: single
|
||||
risk: low
|
||||
impact: project
|
||||
level: implementation
|
||||
tags: [infra, review-002, follow-up, docs]
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Bracketed follow-up item 4 (stale-doc sweep) resolved to its
|
||||
residue. Most of the sweep was absorbed by the bulk itself — SRV-11's
|
||||
router comment was rewritten as part of the fix (adapter.rs now
|
||||
records the correct axum 0.8 `route_layer` semantics), and the
|
||||
`deny(missing_docs)` gate held through all 43 commits. What remains is
|
||||
a small doc-gate regression the bulk introduced; the crate's own
|
||||
standard (from the review-001 HY-02/HY-10 sweep, commit 91483a7) is
|
||||
`RUSTDOCFLAGS="-D warnings" cargo doc` fully clean, and it currently
|
||||
isn't:
|
||||
|
||||
- `dispatch.rs:45` + `:46` — broken intra-doc links `[invoke]` and
|
||||
`[invoke_streaming]` (added by the PRJ-16 guard doc, 207bca4): the
|
||||
methods exist but the link target doesn't resolve from this scope
|
||||
(module-level `//!` docs need fully-qualified paths, e.g.
|
||||
`[`GatewayDispatch::invoke`](crate::gateway::GatewayDispatch::invoke)`
|
||||
— or plain backticks)
|
||||
- `byte_adapter.rs:671` — `redundant explicit link target` on
|
||||
`[`DEFAULT_WS_WRITE_TIMEOUT`](crate::websocket::DEFAULT_WS_WRITE_TIMEOUT)`
|
||||
(the label already resolves to the same destination; the coordinator
|
||||
flagged this one pre-existing/cosmetic)
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] `RUSTDOCFLAGS="-D warnings" cargo doc --no-deps` exits clean
|
||||
(the review-001 HY-02/HY-10 gate restored)
|
||||
- [ ] The dispatch module doc still reads correctly (the reference to
|
||||
`invoke`/`invoke_streaming` readable whether linked or backticked)
|
||||
- [ ] While in the file: spot-check the WS pump/timeout module docs
|
||||
against the WS-13/WS-18/WS-17 implementations (the sweep intent —
|
||||
progress-reset semantics, write-timeout, bare-registry defaults)
|
||||
and fix any statement the bulk's code has overtaken
|
||||
- [ ] `cargo test`, `cargo clippy --all-targets -- -D warnings`,
|
||||
`cargo fmt --check` pass
|
||||
|
||||
## References
|
||||
|
||||
- src/gateway/dispatch.rs:36-52 (the PRJ-16 guard module doc — link source)
|
||||
- src/websocket/byte_adapter.rs:665-675 (the redundant link), src/websocket/upgrade.rs (bare-registry defaults doc to spot-check)
|
||||
- tasks/infra/review-001-missing-docs-sweep.md (the doc gate this restores; HY-10 precedent)
|
||||
- docs/reviews/002-post-remediation-review.md (Unit 4's stale-comment sweep note; bracketed follow-up item 4)
|
||||
|
||||
## Notes
|
||||
|
||||
Trivial, commit-alone. If the sweep's spot-check (third checklist box)
|
||||
finds more than a line or two of drift, split it out rather than
|
||||
widening this commit — the bulk's doc surface was large (ADR-051 §5,
|
||||
ADR-066 §OAI-13, http-adapters.md's matrix) and deserves its own pass
|
||||
if anything significant moved.
|
||||
@@ -0,0 +1,97 @@
|
||||
---
|
||||
id: review-002-fu-stream-error-coverage
|
||||
name: Streaming error-arm coverage — forward_stream invalid input, PEM read-failure, over-cap poll_write mirror (post-bulk coverage gap)
|
||||
status: pending
|
||||
depends_on: []
|
||||
scope: narrow
|
||||
risk: low
|
||||
impact: component
|
||||
level: implementation
|
||||
tags: [infra, review-002, follow-up, coverage]
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Bracketed follow-up discovery from the post-bulk coverage re-pass
|
||||
(95.89% regions overall; the review-002 bulk's own new code verified
|
||||
fully covered — PRJ-16 guard, body cap, router reorder, batch cap,
|
||||
WS-13/18/19, the OAI-11 memo node-budget all exercised). The residue
|
||||
is small but one item matters more than its line count:
|
||||
|
||||
- **`forward_stream` build-request error arm** (forward.rs:1085-1090):
|
||||
a Sub op invoked with an invalid input (undeclared key, non-scalar
|
||||
placeholder value) must produce exactly one error envelope from the
|
||||
stream — the streaming analog of the Once-path error handling. No
|
||||
test drives it: every SSE test (from_jsonschema `integration_sse_subscription…`,
|
||||
forward.rs's stream family) sends valid input. A regression returning
|
||||
an empty stream (silently swallowing the error) would be invisible —
|
||||
and an empty-200-subscribe is the worst failure shape for a
|
||||
subscriptions consumer.
|
||||
- **PEM read-failure arms** (http_client.rs CaBundleRead/ClientCertRead,
|
||||
sync + async): the *parse*-failure arms are covered
|
||||
(`corrupt_ca_bundle_fails_ca_bundle_parse_with_path`,
|
||||
`garbage_client_cert_fails_…` in client_tls.rs, landed in 6490d15),
|
||||
but the **unreadable-file** arms (nonexistent path, unreadable
|
||||
permissions) have no test on either build path. The error taxonomy
|
||||
half-documented at `HttpClientBuildError`'s doc block (:214) is
|
||||
half-specified.
|
||||
- **Over-cap `poll_write` rejection** (byte_adapter.rs:789-798): WS-14's
|
||||
moved check rejects writes *above* `PENDING_BUFFER_CAP` with
|
||||
`InvalidData` before entry to the queue — the "at cap" boundary is
|
||||
tested (`tungstenite_write_at_the_cap_is_accepted`, 1158) and the
|
||||
framing-level cap-trip is tested both flavors
|
||||
(`axum_framing_cap_trip…`), but the single-call over-cap rejection
|
||||
itself (the WS-14 acceptance test's own criterion) never runs: no
|
||||
test writes `PENDING_BUFFER_CAP + 1`.
|
||||
- **`forward_stream` SSE-parser residual edges**: split-CRLF-across-
|
||||
chunks (the `\r` + `"\n…"` case), invalid-UTF8-line drop — the
|
||||
parser edges the bulk's stream arms work next to.
|
||||
- **`from_value` structural rejections** (openapi_spec.rs:391-398,
|
||||
416-418): a non-object document, a missing-`info`, a non-object
|
||||
`paths` — none ever exercised (all other structural rejects are).
|
||||
- **Connection-failure arms**: `protocol_handler` accept-path
|
||||
`stream_error_to_handler` (adapter.rs:597-599) and the read-pump
|
||||
demux-gone `break` (byte_adapter.rs:374-381) — both are
|
||||
connection-failure semantics with zero direct coverage.
|
||||
- **Dead-ish**: `impl Default for WsTimeouts` (upgrade.rs:344-351)
|
||||
has zero callers (the extension is constructed explicitly in the
|
||||
one test that uses it) — delete or `#[allow(dead_code)]` with the
|
||||
reason.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] Wire test: Sub op + undeclared key (or non-scalar placeholder
|
||||
value) → the subscriber receives exactly one `INVALID_INPUT`
|
||||
envelope and the stream ends (upstream receives zero requests)
|
||||
- [ ] Two PEM read-failure tests: `SharedHttpClient::new` with a
|
||||
nonexistent CA path → `CaBundleRead` carrying the path;
|
||||
`reload` with a nonexistent client-cert path → `ClientCertRead`
|
||||
- [ ] One over-cap `poll_write` test: `write_all(PENDING_BUFFER_CAP+1)`
|
||||
→ `InvalidData` error naming the cap (asserts the WS-14 moved
|
||||
check's rejection leg; the = cap test exists)
|
||||
- [ ] SSE parser edge tests: CRLF split across chunks (`…\r` ends
|
||||
chunk 1, `\n…` opens the next), invalid-UTF8 line dropped
|
||||
- [ ] from_value structural tests: non-object doc, missing-`info`,
|
||||
non-object `paths` → `SchemaParse` naming the missing member
|
||||
- [ ] `WsTimeouts` redundant `Default` deleted (or justified+allowed)
|
||||
- [ ] `cargo llvm-cov --all-features`: named arms no longer dark;
|
||||
`cargo test --all-features`, clippy, fmt pass
|
||||
|
||||
## References
|
||||
|
||||
- docs/reviews/002-post-remediation-review.md (Part G' test-gaps; post-bulk re-analysis finding list)
|
||||
- src/adapters/forward.rs:1085-1090 (stream build-error arm), src/client/http_client.rs:484-545 (read-failure arms), src/websocket/byte_adapter.rs:789-798 (over-cap poll_write)
|
||||
- tests/client_tls.rs (the PEM harness: parse-failure tests at :226-448 are the pattern to mirror for read-failures)
|
||||
- tasks/infra/review-002-cov-deployment-knobs.md (the pre-bulk coverage task these extend)
|
||||
- tasks/infra/review-002-cov13-dead-code.md (the dead-code convention for the WsTimeouts Default)
|
||||
|
||||
## Notes
|
||||
|
||||
Tests-only + one deletion. The streaming-build-error test's
|
||||
load-bearing assertion is the *single envelope then end* semantics —
|
||||
mirror `oversized_upstream_sse_line_terminates_with_one_internal_envelope`'s
|
||||
assert structure but trigger pre-send instead (invalid input, no
|
||||
responder needed for the reject case — wire it against a live-responder
|
||||
anyway if the harness makes it cheap; the zero-upstream-contact assert
|
||||
is the point). The `spawn_responder`-family harnesses in
|
||||
forward.rs/from_jsonschema.rs cover the seam.
|
||||
Reference in New Issue
Block a user