alktunnels/tasks/tunnels/review-002-recorder.md

id, name, status, depends_on, scope, risk, impact, level, tags

id	name	status	depends_on	scope	risk	impact	level	tags
tunnels/review-002-recorder	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)	completed		narrow	trivial	isolated	implementation	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, so 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, so 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.

Implementation Notes (2026-09-20): the review's no-action dispositions, recorded

The two durable-trail items landed as specified; the no-action notes' dispositions follow (this section is the tracker the task asked for).

Landed

• N-3: the parse-before-accept telemetry sentence added to make_forwarded_op_handler's doc comment (src/producer.rs): "the accepted-flag telemetry reads false here by design; a malformed-params open IS a birth-teardown shape; do not flag the flag (review 002 N-3)".
• N-4: both ADR-007 Consequences lines added: (1) the SubstrateAddr untagged-leniency-is-schema-gated paragraph (the sibling-keyed render_target backstop; "cross-shape garbage is the schema's absence, not a serde failure"; the schema-less-registry ADR-line-if-ever requirement) and (2) the overlapping-variants caution (untagged disambiguation starts ordering → switch to an internally-tagged form, deliberately, never by accident).
• N-2 (ADR-008 §1 widening: APPLIED, not declined): §1's accept-dispatcher bullet now states the any-byte idle-read posture (Ok(0) / error / any non-empty read all tear down; fail closed; the write-happy-peer test pins it) and names the Ok(0)-only hang-on-write-happy-peer trap the narrower reading would produce. Note: the doc-sync task and this task both touched ADR files but never ADR-008, so no coordination conflict materialized.
• N-5 (optionally tighten: done): mint_token's doc comment now records the review's cap analysis (the token is per-producer-connection; the 256-channels-per-connection cap makes the live-collision window unreachable by a wide margin).

No-action dispositions (unchanged from the review)

• N-1: no action. parse_listen_reply's positive-listen requirement matches the forwarded params' minimum: 1; JSON-null- as-absent is the standard lenient reading; the deny_unknown_fields warning in TunnelListenReply's doc comment is doing real work and is kept as-is.
• N-2's code: no action (fail-closed any-byte watch is correct as shipped; only the ADR sentence was widened, above).
• N-6: no action. Sleep-then-assert is the established house pattern (3x-repeat stable historically); if CI ever flakes on the forwarded-arrival tests, the fix is a bounded poll loop on channel_ids() extended to the arrivals; recorded here, not implemented.
• N-7: no action. test_bind()'s bare Arc<u32> listener is fine; the downcast-miss probe (accept_pair_loop_downcast_miss_fails_loud) covers the failure mode.
• G-1/G-3 (the unreachable-by-construction arms): left uncovered, per the review's recommendations: open/open_direct/ TunnelListener::open (+ open_reverse_channel's twin) params-serialize closures (serde_json cannot fail on these types) and join's pump-Err arm (unreachable through the public API; the only path in is a panicking pump, out of contract). This section is the "unreachable error arm" record the coverage appendix asked for; the optional params_to_input helper collapse was skipped (cosmetic; the review says fine to skip).

Verification

• cargo fmt --check; cargo clippy --all-targets -D warnings (native + wasm32); cargo test (all suites + doc-tests); cargo doc --no-deps are all green.