From 1bc937c061844610d13f0400c4108ea810323299 Mon Sep 17 00:00:00 2001 From: "glm-5.3-flash" Date: Wed, 16 Sep 2026 09:58:17 +0000 Subject: [PATCH] =?UTF-8?q?docs(review=20008):=20remediation=20plan=20?= =?UTF-8?q?=E2=80=94=20findings=20verified,=20three=20units=20pinned?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both findings verified against HEAD 3b36b40 (0.7.1); all citations check out (one line drift: U-2 reply construction is operations.rs:945, errata noted rather than silently edited). Plan decisions recorded above the as-filed option lists: - U-1 shape: combined (b)+(c) — explicit channel_open_alpn string for non-standard shapes; derivation generalized to strip-last-segment, gated on the boolean marker. As-filed option (a) is unsound (the derivation runs consumer-side; a registration-threaded allowlist has no path to rebuild_spec_for). - U-1 gate 2 escalated to an in-tree relay component (src/channels/ relay.rs), amending ADR-042's downstream-implementation scope note — the re-produce/relay shape is general (alkhttp fallback hub, alknodes) and a real consumer now requires it. - U-2 adds open_channel_with_reply (ergonomic read path for `bound`); open_channel unchanged. Units, sequenced 1 -> 2 -> 3, one minor release: 1. U-2 reply projection (ADR-049 am. 3) — lands first; the relay's establisher projects spoke reply fields through it. 2. U-1 derivation + wire field (ADR-047 am.) — standard shapes byte-stable; round-trip gates for flavor-form and boolean shapes. 3. Relay component + gate-2 e2e (ADR-042 am.) — establisher = translate hop (forwarded_for, adopt_channel, reason-code mapping, bound projection), OpenHandler = inline-await byte-forward hop; consumer -> hub -> producer e2e asserting bound survives the relay. As-filed findings retained untouched above the divider (the review-007 pattern). Verification: docs-only change; no build/lint/test impact. --- .../008-tunnels-graduation-upstream-review.md | 203 +++++++++++++++++- 1 file changed, 202 insertions(+), 1 deletion(-) diff --git a/docs/reviews/008-tunnels-graduation-upstream-review.md b/docs/reviews/008-tunnels-graduation-upstream-review.md index 42288ff..6a19c50 100644 --- a/docs/reviews/008-tunnels-graduation-upstream-review.md +++ b/docs/reviews/008-tunnels-graduation-upstream-review.md @@ -190,4 +190,205 @@ sketched above, with the `channel_id` reservation documented. - alksocks `poc-bind-findings.md` (the two fidelity asks with evidence; the upstream-ask ledger this review retires) - Review 007 (the precedent for this filing: POC-driven upstream asks - before the dependent implementation) \ No newline at end of file + before the dependent implementation) + +--- + +# Remediation plan (adopted 2026-09-16 — findings verified, plan pinned) + +Both findings verified against HEAD `3b36b40` (0.7.1) — all code +citations check out, with one line drift: U-2's reply construction is +`src/channels/operations.rs:945` (review cites 955; the cf-006/007 +commit shifted the wrapper down ten lines). One review-scope deviation +from the as-filed text, adopted in planning: + +- **U-1's gate 2 escalates from "test harness" to an in-tree relay + component.** The as-filed review treated ADR-042's relay wrapper as + consumer-side code (the ADR's §Scope note pins the implementation + downstream). The consumer decision is that the re-produce/relay shape + is general (alkhttp's fallback hub, alknodes, the vpn-like endgame all + compose it), and a real consumer now requires it — so ADR-042 is + amended: the relay implementation moves into alkcall as a reusable + component, and gate 2 becomes a genuine in-tree e2e. + +Design decisions pinned above the as-filed option lists: + +- **U-1 shape: combined (b) + (c).** The as-filed option (a) is + unsound as stated — the derivation runs consumer-side against a wire + JSON payload, so an allowlist "threaded from registration" has no + path to `rebuild_spec_for` (both its consumers — `from_call`, + `op/register` — are wire-fed). The pinned shape: `spec_to_json_pub` + emits an explicit `channel_open_alpn` string alongside the boolean + when the op name is not a standard `…/sub`/`…/pub` shape (standard + shapes stay byte-identical); `rebuild_spec_for` prefers the explicit + string, else generalizes the derivation to strip the LAST segment + when the boolean marker is present (the marker is the gate, so any + bare-segment flavor works; `op/register` is fixed by the same + parser). +- **U-2 client read path: add the ergonomic accessor now** — + `open_channel_with_reply` returning the extra reply fields alongside + the streams; `open_channel` unchanged. Keeps alkcall's primary + surface intact while giving alktunnels' `TunnelListener::open` a + first-class way to read `bound`. + +## Unit 1 — U-2: establisher reply projection (ADR-049 amendment 3) + +Land first: the relay component (Unit 3) projects a spoke's extra reply +fields through `Establishment`, so the projection must exist before the +e2e can assert `bound` flowing through a relay. + +1. `Establishment` gains `reply_fields: Option>` + + builder (`Establishment::new(plan).with_reply_field("bound", + json!({...}))`; also `with_reply_fields(map)` / `reply_fields()` + accessor). `#[non_exhaustive]` holds; additive, no break. +2. `run_open_wrapper`: on establisher success, merge + `reply_fields` into the success output AFTER reserving + `channel_id`. An establisher-supplied `channel_id` key is an + establisher bug — fail loudly as + `EstablishmentError::HandlerError` (reason `handler_error`, message + naming the reserved key), tearing the channel down (the wrapper's + existing failure path). No silent shadowing. +3. Client: `ChannelClient::open_channel_with_reply(operation_id, input, + alpn) -> Result<(u32, Value, MpscSendStream, MpscRecvStream), + ChannelOpenError>` — the full success output (the consumer reads + `bound` from it); `open_channel` delegates with the fields + discarded. Additive; no signature change. +4. Doc: ADR-049 amendment 3 (the projection, the `channel_id` + reservation, the accessor). The output schema remains the op's own + concern (alktunnels' listen-op spec documents `bound` as optional). + +Verification gates (from the review, tightened): + +- Establisher returning a reply field produces `{ channel_id, bound }` + on the wire; one returning none produces `{ channel_id }` + (byte-identical to today's — assert exact JSON). +- Establisher attempting `channel_id` → `channel:open_failed` with + reason `handler_error`; channel torn down; ledger decremented. +- The existing establishment tests (establisher-success pump + round-trip, plan-flow, no-establisher compat) pass unchanged. +- `open_channel_with_reply` returns the extra fields end-to-end over a + real channels connection; `open_channel` behavior unchanged. + +## Unit 2 — U-1: flavor-form derivation + explicit ALPN field (ADR-047 amendment) + +1. `derive_alpn_from_op_name` (`src/client/from_call.rs:305`) + generalizes from strip-`/sub`|`/pub` to strip the LAST path segment + (`rsplit_once('/')`), which is a strict superset of today's behavior + (standard two-suffix shapes derive identically; multi-segment ALPNs + survive the same way). The marker boolean remains the gate in + `rebuild_spec_for` — the derivation is only consulted for marked + ops, so plain ops named `channels/x/y` are unaffected (the + `channels/tty/query`-returns-None unit test moves from the function + to the marker-gated rebuild level). +2. `spec_to_json_pub` (`src/registry/discovery.rs:222`): when + `channel_open` is set and the op name is NOT a standard + `channels//(sub|pub)` shape, emit `"channel_open": true` PLUS + `"channel_open_alpn": ""`. Standard shapes keep the boolean + only — byte-stable for every existing consumer. The advertised + `operation_spec_schema` (`discovery.rs:159`) gains the optional + string property (schema-type widening, additive). +3. `rebuild_spec_for` (`from_call.rs:209`): prefer the explicit + `channel_open_alpn` string; else boolean → generalized derivation. + Covers both wire consumers (`from_call`, `op/register`) by + construction. +4. Doc: ADR-047 amendment — the convention sentence mirroring + alktunnels ADR-002 Amendment 1 (`channels//` is a + valid open-op name, `` a bare segment; flavors are additive, + the `…/sub` op is never reused), the explicit-field wire shape, and + the one-way-door note (the field joins the stable + `services/schema` payload; the boolean's meaning for standard-shape + ops is unchanged). Record the residual ambiguity: an ALPN segment + that itself collides with a flavor name (`channels/x/direct` where + the ALPN is `alk/x/direct`-shaped) is undecidable from the name + alone — the explicit field is the disambiguator. + +Verification gates: + +- Review gates 1 and 3 as unit/round-trip tests: + `rebuild_spec_for("channels/tunnel/direct" summary)` reconstructs + WITH `channel_open = alk/tunnel`; `channels/tty/sub` (boolean) and + `channels/custom/proto/sub` (multi-segment) round-trip unchanged; + flavor-form spec serializes with the explicit string and rebuilds + from it; `op/register` announced flavor-form spec round-trips with + the marker. +- Review gate 2 (hub-relay e2e) lands with Unit 3. + +## Unit 3 — the in-tree relay component (ADR-042 amendment) + gate 2 e2e + +The relay wrapper becomes a reusable alkcall export, composed from the +existing machinery rather than a parallel path: + +1. New module `src/channels/relay.rs` — + `ChannelRelay::register_relay_openable(consumer_registry, spec, + producer_client)` (naming bikeshed at implementation): for a + from_call-imported spec with `channel_open` set, registers on the + hub's consumer-leg channel-0 registry via + `register_openable_with_establisher`, where: + - the **establisher** is the translate hop (ADR-042 layer 1): + `call_open_op` on the producer leg's `ChannelClient`, populating + `forwarded_for` from the consumer's per-call identity (ADR-026 §3 + — the same overlay the existing forwarding handlers do), + `adopt_channel`ing the returned `spoke_id` into the producer-leg + manager, and returning `Establishment::new(plan)` whose plan + carries the spoke-leg streams; a spoke `channel:open_failed` + maps into `EstablishmentError` by reason code (the vocabularies + align 1:1); with U-2 landed, the establisher ALSO carries the + spoke reply's extra fields via `with_reply_field` — the wrapper + projects them onto the consumer reply with only `channel_id` + rewritten (wrapper-owned key). + - the **OpenHandler** is the byte-forward hop (ADR-042 layer 2): + pumps the consumer-leg channel `BiStream` ↔ the producer-leg + stream pair (the mux/demux absorbs the 8-byte header + ID rewrite + — the relay never parses chunk framing), awaited inline per the + R-02 lifetime contract; on completion it tears down BOTH legs + (the wrapper's handler-exit teardown covers the consumer leg; the + handler explicitly closes the producer leg). + - The consumer leg keeps the full wrapper machinery: ACL, + per-identity cap/ledger, establishment bound (bounds the spoke + dial round-trip), teardown-on-failure — no parallel + authorization path. +2. `build_bundles` (`from_call.rs:128`) or a documented follow-on + helper branches on the rebuilt spec's marker: marked op → relay + openable (given a `ChannelClient` for the producer leg via + `FromCallConfig`); unmarked → the plain forwarding handlers as + today. This is the "consumer branches on the marker" seam Gap C + named — now with an in-tree implementation downstream crates + compose directly. +3. ADR-042 amendment: the §"Scope note" (implementation lives in + `alknet-hub`/downstream) is revised — the relay implementation is + an alkcall export (`ChannelRelay`), hub crates compose it; the + translate-not-forward contract and the auth-model rationale are + unchanged. +4. Changelog + minor version bump (new pub API: `ChannelRelay`, + `open_channel_with_reply`, `Establishment::with_reply_field*`; + `Establishment` field addition is additive behind + `#[non_exhaustive]`). + +Verification gate 2 (the review's full harness): consumer → hub +(`ChannelsAdapter` on both legs + forked channel-0 registry + +`register_relay_openable`) → producer, opening `channels/tunnel/direct` +end to end: the open resolves on the consumer with the hub-allocated +`channel_id`, the establisher-contributed `bound` field (Unit 1) +survives the relay to the consumer reply, data flows both directions on +the data channel (the hub never parses it), and hub-side disconnect / +spoke-side close tears down both legs' channels. A companion +`channels/tty/sub` relay run pins no regression on standard shapes. + +## Sequencing and non-goals + +Order: Unit 1 → Unit 2 → Unit 3 (Unit 3's e2e asserts `bound` through +the relay, so Units 1–2 are prerequisites). One release (minor bump), +matching the review-007 precedent. Not in scope (extends the review's +non-asks): the data-plane wire format (untouched — the relay rides +`MpscSendStream`/`MpscRecvStream` and the existing pumps), any +`channel/control` translation surface (the relay wrapper translates the +open op; close/control ride the existing generic ops per leg — a hub +wanting cross-leg control translation composes it downstream), and the +alktunnels-side work (the bind-first establisher, the listen-op spec's +`bound` documentation, ADR-007/008 implementation) which sequences +AFTER these land. + +Review-008 status updates when units land: mark U-1/U-2 resolved with +commit refs (the review-007 Status pattern), and correct the U-2 line +citation (955 → 945) in an errata note rather than silently editing the +as-filed text. \ No newline at end of file