docs(review 008): remediation plan — findings verified, three units pinned
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.
This commit is contained in:
@@ -191,3 +191,204 @@ sketched above, with the `channel_id` reservation documented.
|
||||
evidence; the upstream-ask ledger this review retires)
|
||||
- Review 007 (the precedent for this filing: POC-driven upstream asks
|
||||
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<Map<String, Value>>` +
|
||||
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/<seg>/(sub|pub)` shape, emit `"channel_open": true` PLUS
|
||||
`"channel_open_alpn": "<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/<alpn>/<flavor>` is a
|
||||
valid open-op name, `<flavor>` 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.
|
||||
Reference in New Issue
Block a user