tasks: decompose review 002 findings into six remediation tasks
- M-1: the accept dispatcher's silent substrate fallback (plan-stamp shape recommended; failure-path test required) - D-3 + D-4: trivial batch (rename the overpromising schema test helper; trim the stale CHANGELOG Unreleased preamble) - D-1 + D-2: wire.md schema bounds + ADR-007 oneOf->if/then + establisher-time -> registry-gate sentence (doc-only, pre-consumer) - G-2 (+ N-2's write-happy-peer test): observe close()'s EOF sentinel at the far accept dispatcher - G-4 + G-5: unix accept-loop coverage + the unnamed unix fail-loud guards (G-1/G-3/G-6/G-7 explicitly left uncovered per the review) - N-1..N-7 recorder: the two durable-trail notes (N-3 telemetry ordering sentence, N-4 ADR lines) + the no-action dispositions All six independent (no depends_on) so sessions can parallelize; validated with taskgraph (28 tasks, no cycles).
This commit is contained in:
glm-5.3-flash committed
1 parent
46d3a87b24
commit
3df5c484c6
6 files changed
+545
No files matched your search
@@ -0,0 +1,90 @@
|
||||
---
|
||||
id: tunnels/review-002-coverage-gaps
|
||||
name: "G-4 + G-5 — close the remaining cheap coverage gaps (unix accept-loop test, unnamed unix fail-loud guards)"
|
||||
status: pending
|
||||
depends_on: []
|
||||
scope: narrow
|
||||
risk: low
|
||||
impact: component
|
||||
level: implementation
|
||||
tags: [tests, local, unix, coverage, review-002, review-remediation]
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
The two remaining cheap-to-close coverage gaps from review 002's
|
||||
coverage appendix (`docs/reviews/
|
||||
002-graduation-implementation-review.md`), batched — both are
|
||||
`local`-feature test additions (no protocol-layer changes).
|
||||
|
||||
### G-4 — `UnixListenerHalves::accept_loop` never driven
|
||||
|
||||
`src/local/mod.rs:591-609` has zero coverage: no test constructs it
|
||||
(the unix dial test hand-rolls its own echo listener; the bind-first
|
||||
unix test uses `accept_pair`). It is the unix parity twin of the
|
||||
covered `TcpListenerHalves::accept_loop` (hit at ~107-125) — 15 lines
|
||||
of the same shape, and the largest single uncovered block in the
|
||||
crate.
|
||||
|
||||
Test: mirror `tests/producer_listen.rs`'s
|
||||
`accept_loop_stops_feeding_after_close_and_maps_the_drop` with
|
||||
`bind_unix` + a temp path — closes the entire 593-607 range and
|
||||
exercises `UnixListenerHalves` beyond the bind-first path. Low value
|
||||
(copy-shaped) but the largest single uncovered block.
|
||||
|
||||
### G-5 — the unnamed-peer/unnamed-bind fail-loud arms (unix)
|
||||
|
||||
Two fail-loud guards the ADR pins (ADR-008 §3 — "a producer that
|
||||
cannot determine the peer does not serve a bind-first listener over
|
||||
that substrate") never fire:
|
||||
|
||||
- `src/local/mod.rs:626-633` — the `accept_pair` unnamed-peer
|
||||
`handler_error` (an autobind client)
|
||||
- `src/local/mod.rs:578-583` — the `local_addr` unnamed-bind error
|
||||
|
||||
The existing unix bind-first test binds a named client socket
|
||||
deliberately (the parity case); no test binds an abstract/autobind
|
||||
unix listener.
|
||||
|
||||
Test: bind an abstract unix listener (Linux: a leading-`\0` path via
|
||||
`socket2` — already a `local` dependency, or `UnixListener::bind` on
|
||||
an unnamed autobind socket), call `local_addr()` expecting the
|
||||
"no filesystem path" `handler_error`, and drive one autobind-client
|
||||
accept through `accept_pair` expecting the unnamed-peer error. Cheap,
|
||||
deterministic, and it pins the two ADR sentences the doc comments
|
||||
promise.
|
||||
|
||||
### Explicitly NOT in this task (review guidance)
|
||||
|
||||
- **G-7** — kernel-dependent readiness arms; leave uncovered.
|
||||
- **G-6** — the UDP empty-resolve arm; only if the `ResolveFn` seam
|
||||
refactor is ever wanted (optional, separate decision — do not do it
|
||||
here).
|
||||
- **G-1/G-3** — unreachable-by-construction serde/JoinError arms;
|
||||
leave uncovered.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] The unix `accept_loop` test added (G-4 — the 591-609 range
|
||||
covered)
|
||||
- [ ] The abstract-listener test added (G-5 — both fail-loud arms
|
||||
covered: `local_addr` unnamed-bind + `accept_pair`
|
||||
unnamed-peer)
|
||||
- [ ] `cargo test --features local` green; clippy `--all-targets
|
||||
-D warnings` clean (native + wasm32); fmt clean; the default
|
||||
crate stays wasm-clean
|
||||
|
||||
## References
|
||||
|
||||
- docs/reviews/002-graduation-implementation-review.md §G-4, §G-5
|
||||
(the coverage appendix)
|
||||
- src/local/mod.rs:591-609 (`UnixListenerHalves::accept_loop`),
|
||||
578-583 (`local_addr` unnamed-bind), 626-633 (`accept_pair`
|
||||
unnamed-peer)
|
||||
- tests/producer_listen.rs (the accept-loop test shape to mirror)
|
||||
- docs/architecture/decisions/008-listen-metadata-forwarded-op.md §3
|
||||
(the fail-loud sentences the tests pin)
|
||||
|
||||
## Notes
|
||||
|
||||
> Agent fills during implementation.
|
||||
@@ -0,0 +1,96 @@
|
||||
---
|
||||
id: tunnels/review-002-doc-sync
|
||||
name: "D-1 + D-2 — sync wire.md's schema bounds + ADR-007's oneOf→if/then + the establisher-time→registry-gate sentence"
|
||||
status: pending
|
||||
depends_on: []
|
||||
scope: narrow
|
||||
risk: low
|
||||
impact: project
|
||||
level: implementation
|
||||
tags: [docs, wire-format, adr-007, review-002, review-remediation]
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Two doc-only review 002 findings
|
||||
(`docs/reviews/002-graduation-implementation-review.md` §D-1, §D-2)
|
||||
batched: the shipped code is the wire-stable side and is correct; the
|
||||
*docs* lag it. Wire-contract-adjacent — do this before the first
|
||||
consumer (alksocks) ships.
|
||||
|
||||
### D-1 — wire.md omits the `port` bounds the shipped schema enforces
|
||||
|
||||
The shipped direct-op and forwarded-op schemas pin
|
||||
`"port": {"minimum": 0, "maximum": 65535}` plus
|
||||
`"additionalProperties": false` on the target/peer objects
|
||||
(`src/params.rs:208-222, 453-467`), and `listen` with
|
||||
`"minimum": 1` (`src/params.rs:442`). wire.md (the normative WHAT)
|
||||
says only `port: <u16>` and "integer > 0" in prose — it does not
|
||||
record the bounds as schema properties, nor the
|
||||
`additionalProperties: false` gate that makes cross-shape targets
|
||||
fail the registry's `INVALID_INPUT` class BEFORE the establisher (the
|
||||
behavior `malformed_target_is_schema_class_and_leaves_no_channel`
|
||||
pins on the wire). The BAST doc (`bast.md`) covers only the
|
||||
data-plane codec, not the params schemas — no second source records
|
||||
these bounds.
|
||||
|
||||
Fix: record the exact schema constraints in wire.md's §The Direct Op
|
||||
and §The Forwarded Op (the `port` bounds, `additionalProperties:
|
||||
false` per branch, the `listen` `minimum: 1`, and the gate-ordering
|
||||
sentence). The risk addressed: a future consumer implementing from
|
||||
wire.md alone accepting a shape the reference producer rejects (or
|
||||
vice versa).
|
||||
|
||||
### D-2 — ADR-007's `oneOf` wording is stale; the "validated at establisher time" sentence contradicts the shipped design
|
||||
|
||||
1. ADR-007 §2 (line ~82) says "the target sub-schema is per-substrate
|
||||
(`oneOf` on the sibling `substrate` value, validated at establisher
|
||||
time — see Consequences)". The shipped schema (and wire.md §The
|
||||
Forwarded Op, and the unit tests pinning it) use **`allOf` of two
|
||||
`if`/`then` branches** instead. `oneOf` of full-target-schemas
|
||||
would actually be WRONG here (a `unix`-substrate open would need to
|
||||
fail the `tcp` branch — plain `oneOf` on sibling-conditioned
|
||||
subschemas is awkward; `if`/`then` composes correctly with the base
|
||||
`properties`/`required`). The code is right; the ADR's one-word
|
||||
schema-structure reference is stale.
|
||||
2. The same line's "validated at establisher time" contradicts the
|
||||
shipped design and wire.md's own text: the test
|
||||
(`malformed_target_is_schema_class_and_leaves_no_channel`) proves
|
||||
the REGISTRY gate fires first (the establisher's dial never runs).
|
||||
The ADR/wire.md sentence should read "validated registry-side (the
|
||||
schema gate), before the establisher; re-checked by serde at
|
||||
establisher time as the backstop" — which is exactly what
|
||||
`src/params.rs`'s doc comments already say.
|
||||
|
||||
Fix: a one-line ADR-007 erratum (or an amendment note — house style
|
||||
has precedent: ADR-008 carries numbered amendments) replacing
|
||||
`oneOf` with the shipped `allOf`-of-`if`/`then` structure, plus the
|
||||
corrected gate-ordering sentence; mirror the same
|
||||
gate-ordering correction in wire.md §The Direct Op's
|
||||
"validated at establisher time" sentence if it says the same thing
|
||||
(verify both files while in there). No code changes.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] wire.md records the shipped schema bounds (`port`
|
||||
min/max, `additionalProperties: false`, `listen`
|
||||
`minimum: 1`) in both op sections
|
||||
- [ ] ADR-007 §2 corrected: `oneOf` → the shipped `allOf`/
|
||||
`if`/`then` structure (erratum or amendment, house style)
|
||||
- [ ] The "validated at establisher time" sentence corrected to
|
||||
"registry-side schema gate first; serde backstop at
|
||||
establisher time" in ADR-007 (and wire.md if it repeats it)
|
||||
- [ ] `cargo doc --no-deps` clean (docs changed — the verification
|
||||
battery's doc gate)
|
||||
|
||||
## References
|
||||
|
||||
- docs/reviews/002-graduation-implementation-review.md §D-1, §D-2
|
||||
- docs/architecture/wire.md (§The Direct Op, §The Forwarded Op)
|
||||
- docs/architecture/decisions/007-direct-open-op.md (§2, line ~82)
|
||||
- src/params.rs:208-222, 442, 453-467 (the shipped schemas the docs
|
||||
must match)
|
||||
|
||||
## Notes
|
||||
|
||||
> Agent fills during implementation.
|
||||
@@ -0,0 +1,80 @@
|
||||
---
|
||||
id: tunnels/review-002-g2-graceful-teardown-observation
|
||||
name: "G-2 — observe close()'s EOF sentinel at the far accept dispatcher (+ N-2's write-happy-peer teardown pin)"
|
||||
status: pending
|
||||
depends_on: []
|
||||
scope: narrow
|
||||
risk: low
|
||||
impact: component
|
||||
level: implementation
|
||||
tags: [tests, consumer, bind-first, teardown, review-002, review-remediation]
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Review 002 coverage gaps G-2 and the N-2 branch note, batched (the
|
||||
review itself suggests the same harness shape covers both — see its
|
||||
§Branch-coverage notes, last bullet).
|
||||
|
||||
### G-2 — `TunnelListener::close`'s `shutdown` arm never observed at the far end
|
||||
|
||||
`src/consumer.rs:529-531`: `close()` runs `write.shutdown().await`
|
||||
(the explicit EOF sentinel) then reaps — but no test currently
|
||||
observes the FAR accept dispatcher's idle-read firing from a `close()`
|
||||
(the graceful path). The Drop path (implicit EOF → far teardown) IS
|
||||
observed end-to-end
|
||||
(`bindfirst_open_surfaces_reply_fields_and_teardown_cascades`), and
|
||||
the close path is pinned only at the reap level
|
||||
(`close_and_drop_reap_the_listen_channel`) — the sentinel's far-side
|
||||
effect for `close()` specifically is asserted nowhere: the "the pushed
|
||||
accept was never consumed" assertion runs only in the Drop-shaped
|
||||
tests.
|
||||
|
||||
**This is the only coverage gap touching a *promised* behavior**
|
||||
(ADR-008 §1's teardown contract, graceful half) with no direct
|
||||
observation.
|
||||
|
||||
Test: mirror `bindfirst_full_flow…`'s harness (the
|
||||
`tests/consumer_listener.rs` bind-first topology) with a `close()`
|
||||
instead of a drop — open against a bind-first producer, push an
|
||||
accept, `listener.close().await`, then assert the far accept loop is
|
||||
gone (the same queue-len / never-consumed assertion the Drop tests
|
||||
use) and the serving-side listen channel reaped.
|
||||
|
||||
### N-2's testable residue — the write-happy-peer teardown posture
|
||||
|
||||
The idle-stream EOF watch (`src/producer.rs:781-794`) triggers
|
||||
teardown on `Ok(0)`, `Err(_)`, AND `Ok(1..)` — a misbehaving opener
|
||||
that writes to the idle listen channel also tears the listener down
|
||||
(fail-closed; the channel is not a data plane; the doc comment owns
|
||||
this). N-2 records the shape as correct, but the review's
|
||||
branch-coverage notes flag that the `Ok(_)` (non-empty read) arm is
|
||||
untested: a test would write a byte to the idle channel and assert
|
||||
far teardown, pinning the "any non-empty read tears down" posture.
|
||||
|
||||
Test: same harness — open the listen channel, write a byte into the
|
||||
idle stream from the opener side, assert the accept loop cancels and
|
||||
the listener tears down (the fail-closed posture).
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] The graceful-`close()` far-teardown test added (the sentinel's
|
||||
far-side effect observed)
|
||||
- [ ] The write-happy-peer teardown test added (the `Ok(_)` idle-read
|
||||
arm pinned)
|
||||
- [ ] `cargo test --features local` green; clippy `--all-targets
|
||||
-D warnings` clean (native + wasm32); fmt clean
|
||||
|
||||
## References
|
||||
|
||||
- docs/reviews/002-graduation-implementation-review.md §G-2 (Coverage
|
||||
appendix), §N-2, and §Branch-coverage notes (last bullet)
|
||||
- src/consumer.rs:529-531 (`TunnelListener::close`), src/producer.rs
|
||||
(the idle-stream EOF watch)
|
||||
- tests/consumer_listener.rs (the harness shape to mirror)
|
||||
- docs/architecture/decisions/008-listen-metadata-forwarded-op.md §1
|
||||
(the teardown contract's graceful half)
|
||||
|
||||
## Notes
|
||||
|
||||
> Agent fills during implementation.
|
||||
@@ -0,0 +1,95 @@
|
||||
---
|
||||
id: tunnels/review-002-m1-substrate-fallback
|
||||
name: "M-1 remediation — the accept dispatcher's silent substrate fallback (stamp the plan, fail loud)"
|
||||
status: pending
|
||||
depends_on: []
|
||||
scope: narrow
|
||||
risk: low
|
||||
impact: component
|
||||
level: implementation
|
||||
tags: [producer, bind-first, review-002, review-remediation]
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Review 002 finding M-1
|
||||
(`docs/reviews/002-graduation-implementation-review.md` §M-1): the
|
||||
bind-first accept dispatcher's substrate extraction
|
||||
(`src/producer.rs:766-768`, inside `make_bindfirst_accept_handler`)
|
||||
silently falls back to `Substrate::Tcp` when the second
|
||||
`serde_json::from_value::<TunnelParams>` parse fails:
|
||||
|
||||
```rust
|
||||
let substrate = serde_json::from_value::<TunnelParams>(input.clone())
|
||||
.map(|p| p.substrate)
|
||||
.unwrap_or(Substrate::Tcp);
|
||||
```
|
||||
|
||||
In the wired path the fallback is unreachable (the registry schema gate
|
||||
and the establisher's `parse_params` both validate first), but the
|
||||
shape itself is wrong: a future drift would silently stamp `Tcp` into
|
||||
every forwarded open — for a UDP listen tunnel that is a
|
||||
raw-vs-framed data-plane mismatch on the opener side (the exact
|
||||
invariant class review 001's U-1 was about), degrading silently
|
||||
instead of loudly.
|
||||
|
||||
### The fix (decide deliberately, then implement)
|
||||
|
||||
The review offers two shapes; **the second is recommended** (the
|
||||
review calls it strictly better, and the pre-consumer window is open —
|
||||
`BindFirstPlan` has no external consumer yet):
|
||||
|
||||
1. Log-loud-and-return on parse failure (teardown-at-birth — the
|
||||
same posture as every other early return in the file). Smaller,
|
||||
but keeps the second parse and the fallback branch.
|
||||
2. **Stamp the substrate into `BindFirstPlan` at establisher time and
|
||||
drop the second parse entirely** — one parse, one source of truth,
|
||||
no fallback branch at all. The establisher already parsed the
|
||||
params successfully; the substrate is per-open-static; the plan is
|
||||
already the per-open carrier. `BindFirstPlan` gains a
|
||||
`substrate: Substrate` field (a plan-payload change — cheap now,
|
||||
pre-consumer); the dispatcher reads it from the plan it already
|
||||
downcast.
|
||||
|
||||
If shape 2: the establisher's construction site
|
||||
(`bindfirst_establisher`) carries the parsed substrate into the plan;
|
||||
the per-dispatcher token-discipline test continues to pass; check the
|
||||
`Debug` impl for `BindFirstPlan` still compiles (opaque listener
|
||||
field) and the doc comment on the new field states why it rides the
|
||||
plan (the dispatcher must not re-parse wire input).
|
||||
|
||||
### Tests
|
||||
|
||||
- Keep the existing suite green (the two-open token-discipline test
|
||||
pins per-dispatcher stamping from its own plan — it now also pins
|
||||
the substrate's origin implicitly).
|
||||
- A failure-path test: with shape 2 there is no failure branch left
|
||||
in the dispatcher (assert the removed fallback is gone — e.g. the
|
||||
forwarded opens for a UDP bind-first listen carry `substrate: udp`
|
||||
from the plan, not a re-parse). With shape 1: a malformed-input
|
||||
accept teardown test (loud return, channel reaps, no phantom
|
||||
channel).
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] The silent `unwrap_or(Substrate::Tcp)` fallback is gone from
|
||||
`make_bindfirst_accept_handler`
|
||||
- [ ] The chosen shape documented in this task's Notes (plan-stamp vs
|
||||
log-and-return, with the rationale)
|
||||
- [ ] A test pins the forwarded-op substrate source (plan-stamped, or
|
||||
the loud teardown on parse failure)
|
||||
- [ ] `cargo test` green; clippy `--all-targets -D warnings` clean;
|
||||
fmt clean; wasm32 check + clippy clean
|
||||
|
||||
## References
|
||||
|
||||
- docs/reviews/002-graduation-implementation-review.md §M-1 (the
|
||||
finding, both fix shapes, and why not fixed inline)
|
||||
- src/producer.rs — `BindFirstPlan` (~line 97), the establisher, and
|
||||
`make_bindfirst_accept_handler` (~line 755)
|
||||
- tasks/tunnels/bindfirst-producer.md (the original implementation
|
||||
task + Notes on the plan's shape)
|
||||
|
||||
## Notes
|
||||
|
||||
> Agent fills during implementation.
|
||||
@@ -0,0 +1,114 @@
|
||||
---
|
||||
id: tunnels/review-002-recorder
|
||||
name: "N-1..N-7 — record the review's no-action notes in the code/ADR trail (N-4's ADR line + N-3's telemetry note)"
|
||||
status: pending
|
||||
depends_on: []
|
||||
scope: narrow
|
||||
risk: trivial
|
||||
impact: isolated
|
||||
level: implementation
|
||||
tags: [docs, notes, review-002, review-remediation]
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Review 002's [note] findings
|
||||
(`docs/reviews/002-graduation-implementation-review.md` §N-1..N-7)
|
||||
are correct-as-is with **no code action** — but two of them ask for a
|
||||
durable trail beyond the review doc itself, and the rest are recorded
|
||||
here as the tracker (the review's own remediation list only orders
|
||||
M-1/D-*/the coverage gaps).
|
||||
|
||||
### The two notes wanting a durable trail
|
||||
|
||||
- **N-3** — `make_forwarded_op_handler` parses params BEFORE
|
||||
`accept_bi` (`src/producer.rs:993-1006`), so the R-02 accepted-flag
|
||||
telemetry reads `false` for a malformed-params open — the
|
||||
birth-teardown hint fires for a handled-rejection case. The order
|
||||
is deliberately the better one (accepting first would hold a stream
|
||||
open for a params set the producer already got wrong). Record one
|
||||
sentence in the code's doc comment ("the accepted-flag telemetry
|
||||
reads false here by design — a malformed-params open IS a
|
||||
birth-teardown shape") so a future alkcall reviewer doesn't flag
|
||||
the flag.
|
||||
- **N-4** — `SubstrateAddr`'s untagged leniency is schema-gated (the
|
||||
schema's `additionalProperties: false` per branch is the real
|
||||
gate). Two latent sharp edges to keep in view, in the ADR trail:
|
||||
1. The establisher-level serde backstop keys `render_target` off
|
||||
the sibling `substrate` — a cross-shape target that sneaked past
|
||||
a looser registry would render *something* rather than fail.
|
||||
Worth an ADR line if a future consumer runs a schema-less
|
||||
registry variant. Add the line to ADR-007 (Consequences or the
|
||||
wire-stable section) while the doc-sync task's context is fresh
|
||||
— one sentence: "the serde backstop renders the sibling-keyed
|
||||
target; cross-shape garbage is the schema's absence, not a
|
||||
serde failure."
|
||||
2. If `SubstrateAddr` grows a variant whose shapes overlap (e.g. a
|
||||
future `{host, port, flowlabel}`), untagged disambiguation
|
||||
starts ordering — switch to an internally-tagged form at that
|
||||
point. This is additive-only, pre-consumer; the ADR line should
|
||||
say "don't let it happen by accident."
|
||||
|
||||
### The no-action notes (recorded here as the tracker)
|
||||
|
||||
- **N-1** — `parse_listen_reply`'s positive-`listen` requirement
|
||||
matches the forwarded params' wire contract (`minimum: 1`);
|
||||
JSON-null-as-absent is the standard lenient reading; the struct
|
||||
`deny_unknown_fields` warning in the doc comment is doing real
|
||||
work — keep both. No action.
|
||||
- **N-2** — the idle-stream EOF watch treats ANY byte as teardown
|
||||
(fail-closed). Correct; the ADR sentence is thinner than the
|
||||
implementation — the write-happy-peer test pin is tracked in
|
||||
`tunnels/review-002-g2-graceful-teardown-observation`. Optionally
|
||||
widen ADR-008 §1's "its EOF cancels the loop" sentence to name the
|
||||
any-byte posture while editing (coordinate with
|
||||
`tunnels/review-002-doc-sync` if both touch ADR-008).
|
||||
- **N-5** — the token-mint discipline is sound; the 256-channel
|
||||
per-connection cap makes the wrap-collision math even more
|
||||
favorable than the doc comment states. No action (optionally
|
||||
tighten the doc comment's math note).
|
||||
- **N-6** — the sleep-then-assert test pattern
|
||||
(`tests/consumer_listener.rs:276` + siblings) is the established
|
||||
house pattern; if CI ever flakes, the fix is a bounded poll loop on
|
||||
`channel_ids()` extended to the arrivals. No action now.
|
||||
- **N-7** — `test_bind()`'s bare `Arc<u32>` listener is fine (the
|
||||
downcast-miss probe covers the failure mode). No action.
|
||||
|
||||
### G-1/G-3 — the unreachable-by-construction arms (document-only)
|
||||
|
||||
Per the coverage appendix's recommendations: leave both uncovered,
|
||||
noted here as the review's "unreachable error arm" record —
|
||||
`TunnelSession::open`/`open_direct`/`TunnelListener::open` (+
|
||||
`open_reverse_channel`'s twin) params-serialize closures (serde_json
|
||||
cannot fail on these types) and `TunnelSession::join`'s pump-`Err`
|
||||
arm (the pump handle is never aborted before `join` through the
|
||||
public API). If a shared "unreachable error arm" prose note ever
|
||||
lands in the test suite, these are its members. Optionally collapse
|
||||
G-1's four duplicated closures into one `params_to_input` helper
|
||||
(cosmetic; the review says fine to skip).
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] The N-3 telemetry-ordering sentence added near
|
||||
`make_forwarded_op_handler`'s parse (doc comment)
|
||||
- [ ] The N-4 ADR lines added to ADR-007 (the serde-backstop render
|
||||
+ the overlapping-variants caution)
|
||||
- [ ] The N-2 ADR-008 §1 widening applied (or explicitly declined
|
||||
with rationale in Notes)
|
||||
- [ ] The no-action notes' disposition recorded in this task's Notes
|
||||
section (so the tracker carries the review's decisions)
|
||||
|
||||
## References
|
||||
|
||||
- docs/reviews/002-graduation-implementation-review.md §N-1..N-7,
|
||||
§G-1, §G-3
|
||||
- src/producer.rs:993-1006 (`make_forwarded_op_handler`'s
|
||||
parse-before-accept ordering)
|
||||
- docs/architecture/decisions/007-direct-open-op.md,
|
||||
docs/architecture/decisions/008-listen-metadata-forwarded-op.md
|
||||
- src/params.rs:329-339 (`parse_listen_reply`), src/producer.rs:97
|
||||
(`BindFirstPlan`), src/consumer.rs (the unreachable arms)
|
||||
|
||||
## Notes
|
||||
|
||||
> Agent fills during implementation.
|
||||
@@ -0,0 +1,70 @@
|
||||
---
|
||||
id: tunnels/review-002-trivial-batch
|
||||
name: "Trivial batch — D-3 (rename the overpromising schema test helper) + D-4 (trim the stale CHANGELOG preamble)"
|
||||
status: pending
|
||||
depends_on: []
|
||||
scope: narrow
|
||||
risk: trivial
|
||||
impact: isolated
|
||||
level: implementation
|
||||
tags: [tests, changelog, review-002, review-remediation]
|
||||
---
|
||||
|
||||
## Description
|
||||
|
||||
Two trivial review 002 findings
|
||||
(`docs/reviews/002-graduation-implementation-review.md`) batched —
|
||||
each a one-line-scale change; no behavior impact.
|
||||
|
||||
### D-3 — the test helper's name overpromises
|
||||
|
||||
`src/params.rs:628-637`: `jsonschema_valid` parses the value with
|
||||
serde and pattern-matches the substrate string — it validates the
|
||||
serde layer, NOT the JSON Schema (the registry compiles the real
|
||||
validators; adding a `jsonschema` dev-dependency purely for this unit
|
||||
test is not warranted). The call-site
|
||||
`schema_rejects_unknown_substrate_value` therefore promises more than
|
||||
it delivers; the sibling
|
||||
`direct_target_schema_gates_shape_per_substrate` is honest (it does
|
||||
structural schema assertions and says so).
|
||||
|
||||
Fix: rename `jsonschema_valid` → `serde_valid` (or inline the check)
|
||||
and rename the call-site test
|
||||
`schema_rejects_unknown_substrate_value` →
|
||||
`serde_rejects_unknown_substrate_value` (its doc comment should say
|
||||
"the serde layer; the registry's real schema gate is pinned
|
||||
end-to-end by the integration probes
|
||||
(`schema_rejects_unknown_substrate_loudly`,
|
||||
`malformed_target_is_schema_class…`, the forwarded `listen: 0`
|
||||
gate)"). Do NOT add a `jsonschema` dev-dependency.
|
||||
|
||||
### D-4 — the CHANGELOG "Unreleased" preamble is stale
|
||||
|
||||
`CHANGELOG.md`'s Unreleased section still opens with "implementation
|
||||
is decomposed in `tasks/tunnels/` and lands in the next release…
|
||||
the only behavior-affecting change so far (no wire or API change at
|
||||
this level yet)" — written before the six graduation implementation
|
||||
commits. The `### Added` section below it correctly lists all six
|
||||
features; the preamble contradicts the section it introduces.
|
||||
|
||||
Fix: trim the preamble to a one-liner (e.g. "The graduation
|
||||
implementation (ADRs 007/008) landed — see Added below." — final
|
||||
wording agent's choice) or drop it.
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
- [ ] The overpromising unit test/helper renamed (no `jsonschema`
|
||||
dev-dependency added)
|
||||
- [ ] The CHANGELOG Unreleased preamble trimmed or dropped
|
||||
- [ ] `cargo test` green (the renamed test included); clippy + fmt
|
||||
clean
|
||||
|
||||
## References
|
||||
|
||||
- docs/reviews/002-graduation-implementation-review.md §D-3, §D-4
|
||||
- src/params.rs:628-637 (the helper + call-site)
|
||||
- CHANGELOG.md (the Unreleased preamble)
|
||||
|
||||
## Notes
|
||||
|
||||
> Agent fills during implementation.
|
||||
Reference in new issue
Block a user