From f1d6796436c0e06665daa01db1e4cb7a2dabf9c7 Mon Sep 17 00:00:00 2001 From: "glm-5.2" Date: Wed, 12 Aug 2026 13:03:36 +0000 Subject: [PATCH] =?UTF-8?q?docs:=20consolidated=20review=20001=20=E2=80=94?= =?UTF-8?q?=20Pub=20(ADR-046)=20+=20channels=20integration=20(ADR-047)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consolidates three code-review passes (one main summary + sub-review A on ADR-046 + sub-review B on channels ADRs 034–043/047) into a single verified document under docs/reviews/. Every finding was re-verified directly against the source at f305f8c with exact file:line refs. Findings (all verified): 10 critical, 19 major, 7 minor. - Pub end-to-end cannot work (chunks written to a different bi-stream than the request); publish() takes Vec not a Stream; 30s client timeout; abort-doesn't-cancel-Pub; dispatch re-implements invoke_sink inline; from_call sink forwarding swallows errors. - Channel 0 dead in both directions; register_openable absent; resolve_channel_manager stub; backpressure drops chunks; buffer cap in messages not bytes; ledger decrement on 1/4 teardown paths; demux desyncs on oversized chunk; no channel-adoption/collision scheme. - channel/control + resources/subscribe stubs; busy-wait spin; lost EOF sentinel; TOCTOU on max_channels; ~50 lines of abandoned deliberation in poll_write; cargo doc 4 warnings; spec docs inconsistent post-047. Includes a 6-unit remediation plan sequenced by dependency, with acceptance gates. Units 1/2/4 need no spec decisions; Units 3 and 5 each need one written ADR decision first (ADR-047 §4 env-resolution; §5 channel-id adoption). The overarching acceptance gate is the end-to-end ChannelClient ↔ ChannelsAdapter test whose absence let both commits land green. Two corrections to the original reviews noted in the verification log: - C-17 (lost EOF sentinel): Sub Review B overstated "the pump does not write an EOF chunk" — the pump does write EOF when it receives the sentinel; the real bug is narrower (sentinel lost on full buffer → pump exits on recv→None without writing EOF). - Warning count: main review said 5 cargo doc warnings; this pass sees 4 (2× register_openable, 1× default_policy, 1× env module/macro). Verification: - cargo test → 432 passed (no source changed; docs-only commit) - cargo clippy --all-targets -- -D warnings → clean - cargo fmt --check → clean - cargo doc --no-deps → 4 warnings (documented as C-09; not addressed here — fixing them requires the register_openable method to exist, which is Unit 3 of the remediation plan) --- ...001-pub-and-channels-integration-review.md | 1075 +++++++++++++++++ 1 file changed, 1075 insertions(+) create mode 100644 docs/reviews/001-pub-and-channels-integration-review.md diff --git a/docs/reviews/001-pub-and-channels-integration-review.md b/docs/reviews/001-pub-and-channels-integration-review.md new file mode 100644 index 0000000..5ae3b63 --- /dev/null +++ b/docs/reviews/001-pub-and-channels-integration-review.md @@ -0,0 +1,1075 @@ +# Review 001 — Pub (ADR-046) and Channels Integration (ADR-047) + +## Status + +Verified, open for remediation. + +## Scope + +Consolidated code review of the two commits that landed the `Pub` +operation type / `HandlerKind::Sink` (ADR-046, commit `ea66398`) and +the channels protocol + openable-ALPNs model (ADR-047, commit +`f305f8c`). The review consolidates three passes (one main summary + +two sub-reviews: one focused on ADR-046, one on the channels ADRs +034–043/047) and was produced against the tree at `f305f8c`. + +Every finding below was **re-verified directly in the source** during +this consolidation pass (2026-08-12). Each finding carries: the ADR it +drifts from (if any), a severity, the exact `file:line` reference(s), +the verification status, and the unit-of-work bucket it belongs to. +Findings that did not survive verification, or that the original +reviews overstated, are called out as such. + +## Baseline verification (this pass) + +``` +cargo test → 432 passed, 0 failed +cargo clippy --all-targets -D warnings → clean +cargo fmt --check → clean +cargo doc --no-deps → 4 warnings (see C-09) +``` + +The suite is green, but green is misleading here: **all coverage is +unit-level**. Not a single test wires two sides of either new protocol +together end-to-end (no `CallConnection::publish` ↔ +`Dispatcher::handle_stream` round-trip; no `ChannelClient` ↔ +`ChannelsAdapter` round-trip). The integration seams — which is exactly +where ADR-047 lives — are where the real problems live, and the absence +of end-to-end tests is what let both commits land green with multiple +implementation-killing defects. + +## Verdict + +- **ADR-046 registry half (types, `HandlerKind::Sink` validation, + `invoke_sink`, builders, wire strings, discovery strings): solid**, + close to spec, well tested at the unit level. +- **ADR-046 protocol half: has one implementation-killing bug** (P-01: + publish chunks are written to a different bi-stream than the request + — end-to-end publish cannot succeed on any transport), plus two + unimplemented decided behaviors and an abort story that exists only + in doc comments. +- **ADR-047: the components are good but the integration layer — the + part ADR-047 actually decided — is scaffolding.** As committed, the + channels protocol cannot open, use, or account for a single data + channel end-to-end, channel 0 is dead in both directions, and a + `publish()` cannot succeed on any transport. + +The architecture is right: no channels types leaked into the call +crate, `OperationEnv` stayed a trait, the wire formats (envelope + +8-byte chunk header) match spec exactly, the `channel_open` marker +plumbing (Gap F) is complete and tested, and auth stays on the one +`AccessControl` path. What's missing is the connective tissue a +tty/docker crate would actually call. + +## Severity legend + +- **[critical]** — the protocol cannot function end-to-end as committed + on any transport; or a decided spec invariant is violated in a way + that corrupts data or causes silent permanent state damage. +- **[major]** — a decided behavior is missing, wrong, or a real + reliability hazard; works in the happy path but fails a spec-required + edge case or stores wrong data. +- **[minor]** — drift, convention violation, dead code, or a + doc/spec inconsistency with no correctness impact. + +--- + +# Part A — Pub / `HandlerKind::Sink` (ADR-046) findings + +## P-01 [critical] — Publish chunks are written to the wrong stream; end-to-end publish cannot work + +**ADR drift:** ADR-046 §5, §8 (the stream lifecycle and `publish()` +client method). +**Verified:** YES. `src/protocol/connection.rs:285-311` (open stream A, +write `call.requested`), `src/protocol/connection.rs:334-362` +(`write_publish_chunks` opens a **fresh** `open_bi()` at line 343-346 +and writes all `call.published` + `call.completed` on stream B). +**Responder side:** `src/protocol/dispatch.rs:433-435, 516` +(`pump_sink` reads chunks from the *same* stream the request arrived +on — stream A). Stream B arrives as a fresh `accept_bi` whose frames +hit the `handle_stream` "ignoring non-requested/non-aborted event" +branch (`src/protocol/dispatch.rs:443-444`) and are silently +discarded. + +**Consequences (verified):** +- Multi-stream transport (QUIC): chunks are lost; `pump_sink` blocks + on stream A until the client's dropped write-half surfaces as + reset/EOF — best case the handler sees an *empty* stream and returns + a bogus result (e.g. `count: 0`), worst case it hangs until the 30s + client-side timeout (P-05). +- Single-stream transport (`Connection::from_bidi`): the second + `open_bi` returns `StreamClosed` (`src/core/types.rs`), so *every* + publish fails outright with "failed to open stream". + +No end-to-end publish test exists (see P-12), which is how this landed +green. **Fix:** keep stream A's write half and pump chunks on it. This +is also what a `Stream`-typed `publish` (P-02) would force +structurally. **Acceptance gate:** one end-to-end publish test that +moves ≥2 chunks and asserts the responder sees them. + +## P-02 [major] — `publish()` takes `Vec`, not a `Stream` + +**ADR drift:** ADR-046 §8 specifies +`stream: Pin + Send>>`. The implementation +is `chunks: Vec`. +**Verified:** YES. `src/protocol/connection.rs:249-254` (`publish`) +and `:268-272` (`publish_with_payload`) both take `chunks: Vec`; +`write_publish_chunks` (`:334-362`) iterates the `Vec`. + +This buffers the entire publish in memory before anything is sent and +makes the ADR's flagship use cases (telemetry ingest, live upload, +drag-drop file streaming) impossible from this API — the chunk set +must be fully materialized up front. **Fix P-01 + P-02 together:** +hold stream A's write half, accept a `Stream`, pump it as +`call.published` events. This also resolves P-05's tension (a streaming +publish should register like `subscribe` with `timeout: None`, not the +30s `Call` deadline). + +## P-03 [major] — `publish_schema` per-chunk validation is written but never read + +**ADR drift:** ADR-046 §4, and the field's own doc comment at +`src/registry/spec.rs:181-186` which says "the dispatch path validates +each `call.published` event's `payload.input` against this schema +before yielding it to the `SinkHandler`." +**Verified:** YES. `grep publish_schema` outside `src/registry/spec.rs` +matches nothing. The field is declared (`spec.rs:186`), defaults to +`None` (`:228`), and has a builder (`with_publish_schema`, `:236-239`), +and is **never read anywhere**. `pump_sink` forwards chunks as-is +(`src/protocol/dispatch.rs:527-535`). It is also missing from +`spec_to_json`, from `operation_spec_schema()`, and from `from_call`'s +`rebuild_spec_for` — so an imported Pub op loses the schema entirely. +`alktype` is a declared dependency (`Cargo.toml:21`) and AGENTS.md §10 +says to use it for exactly this; it is unused in `src/`. Dead field + +documented-but-unimplemented behavior. + +## P-04 [major] — Initiator `call.error` does not terminate the publish stream + +**ADR drift:** ADR-046 §5 event table and §6 ("`Err(call_error)` for an +initiator-side error (a `call.error` event from the initiator)"); plus +the `SinkHandler`/`PublishStream` doc contract +(`src/registry/registration.rs:26-31, 44-49`). +**Verified:** YES. `pump_sink` matches only `"call.published"`, +`"call.completed"`, `"call.aborted"` and drops everything else into a +debug-log ignore branch (`src/protocol/dispatch.rs:526-550`). A +`call.error` frame is silently ignored; the only `Err` ever injected is +the `call.aborted` case, and it's an ad-hoc +`CallError::internal("publish aborted by initiator")` +(`dispatch.rs:538-543`), not the initiator's error. + +## P-05 [major] — Client-side 30s timeout on Pub contradicts "long-lived, same as Sub" + +**ADR drift:** ADR-046 §7 ("the stream may be long-lived, same as +`Sub`") clears the deadline for Pub on the responder. +**Verified:** YES. `publish_with_payload` registers via `register_call` +with `DEFAULT_CALL_TIMEOUT` = 30s (`src/protocol/connection.rs:32, +296-303`). The sweeper resolves expired `Call` entries with `TIMEOUT` +(`src/protocol/pending.rs`). `subscribe` avoids this with +`register_subscribe(timeout: None)` (`connection.rs:215-218`). Any +publish whose stream takes >30s wall-clock gets a spurious client-side +`TIMEOUT` while the responder keeps consuming. **Fix:** register like +`subscribe` with `timeout: None` (naturally falls out of the P-01/P-02 +streaming rework). + +## P-06 [major] — Abort does not cancel a Pub + +**ADR drift:** ADR-046 §5 table (`call.aborted` | either side | "Cancel +the Pub"); ADR-020 (stream drop on abort; cascade through sink +handlers). +**Verified:** YES, both abort paths are broken: +- **Cross-stream (the path the shipped client exercises):** + `CallConnection::abort()` sends `call.aborted` on a **new** stream + (`src/protocol/connection.rs:236-243` → `write_envelope` → `open_bi` + at `:388-392`). On the responder that frame is handled by a different + `handle_stream` task, whose `handle_abort` only mutates the + `PendingRequestMap` (`src/protocol/dispatch.rs:381-389`). Inbound + requests are never registered in that map, and nothing links the map + to the spawned `handle_stream`/`pump_sink` task — the running + `SinkHandler` and feed loop are untouched. The doc comments at + `dispatch.rs:494-498` (and `:459-462` for `pump_stream`) describe a + cancellation mechanism that does not exist. +- **Same-stream (dead-untested):** `pump_sink` injects an `Err` and + breaks the feed on `call.aborted` (`dispatch.rs:538-543`), then + **still `tokio::join!`s the handler to completion and writes the + response back to the aborting initiator** (`:562-568`). The handler + future is never dropped. + +Net: an initiator cannot actually cancel an in-flight Pub. (The +`pump_stream` half of this predates ADR-046, but ADR-046 §5 explicitly +promises abort-cancels-Pub, so it is in scope here.) **Also:** during a +sink pump, `call.aborted` frames for **other** request IDs on the same +stream are dropped by the id-mismatch guard before the type match +(`dispatch.rs:518-525`) — a conforming peer that serializes calls on +one stream loses requests. Same for a second `call.requested` +multiplexed on that stream. + +## P-07 [major] — Handler-returns-early: response withheld until the initiator finishes + +**ADR drift:** ADR-046 §7 (long-lived publish streams). +**Verified:** YES. `pump_sink` uses `tokio::join!(feed_fut, handler)` +and only writes the response after **both** complete +(`src/protocol/dispatch.rs:562-568`). If the `SinkHandler` returns +early (e.g. rejects the upload after chunk 1), the feed loop notices +only when its next `chunk_tx.send` fails (`dispatch.rs:533-535`) — only +when another chunk arrives. An initiator that pauses between chunks, or +a long/infinite publish, never learns of the early error; the response +is deferred indefinitely. Not a deadlock, but an unbounded stall on a +path the ADR intends for long-lived streams. + +## P-08 [major] — The dispatch Pub branch re-implements `invoke_sink` inline + +**ADR drift:** ADR-046 §6, §7 (`Pub` → `invoke_sink()`; `dispatch()` +branches on `op_type` and routes Pub through `invoke_sink`). +**Verified:** YES. The `Pub` branch of `Dispatcher::dispatch` +re-implements the not-found / visibility / ACL / handler-kind checks +inline (`src/protocol/dispatch.rs:309-377`) and invokes the +`SinkHandler` directly, duplicating `OperationRegistry::invoke_sink` +(`src/registry/registration.rs:283-345`) line for line. `invoke_sink` +is only ever called from its own tests. Divergence hazard: any future +fix in `invoke_sink` (e.g. adding the missing `publish_schema` +validation, P-03) won't reach the wire path. + +## P-09 [major] — `from_call` sink forwarding is store-and-forward and swallows errors + +**ADR drift:** ADR-046 §9 (sink forwarding); the `SinkHandler` contract +(`src/registry/registration.rs:29-31`: an `Err` terminates the +stream). +**Verified:** YES. `make_sink_forwarding_handler` does +`publish_stream.filter_map(|item| async move { item.ok() }).collect::>().await` +then one `publish_with_payload` call +(`src/client/from_call.rs:464-475`). Three problems: +1. The whole stream is buffered in memory — unbounded, contradicting + the doc comment "No truncation — the full stream is forwarded + end-to-end" (`from_call.rs:452-453`) in spirit (forwarded, but not + streamed; a hub relaying a large upload holds it all in RAM). +2. `Err` items are **silently discarded and iteration continues** + (`item.ok()` → `None` → filtered out), contradicting the contract + that an `Err` terminates the stream — a partial/errored publish is + forwarded upstream as if it completed cleanly. +3. It inherits P-01, so the forwarded publish is broken anyway. + +## P-10 [minor] — String literals instead of wire constants in `pump_sink` + +**Verified:** YES. `pump_sink` matches `"call.published"`, +`"call.completed"`, `"call.aborted"` (`src/protocol/dispatch.rs:527, +537, 538`) instead of `EVENT_PUBLISHED`/`EVENT_COMPLETED`/`EVENT_ABORTED` +(`src/protocol/wire.rs:12-17`), which the rest of the file imports and +uses. Refactor hazard, not a correctness issue. + +## P-11 [minor] — ADR-046 §3 vs §6 `SinkHandler` stream-item-type inconsistency + +**Verified:** YES (internal ADR inconsistency, not a code bug). ADR-046 +§3 defines `SinkHandler` receiving `Pin + +Send>>`; §6 defines `invoke_sink`'s `publish_stream` as +`Item = Result`. The code resolves this in favor of +§6 uniformly: `SinkHandler` itself takes `Item = Result` (`src/registry/registration.rs:32-40`), aliased as +`PublishStream` (`:49`), and `invoke_sink` passes it straight through +(`:344`). This contradicts §3's literal type but is arguably the right +resolution (the handler must see initiator errors), and the ADR's +Door-type section explicitly marks the concrete stream item type a +two-way-door detail. **Action:** amend ADR-046 §3 text to match §6. + +## P-12 — Test coverage gaps (ADR-046) + +**Verified:** YES — all 23 new Pub tests are either registry-level +(`invoke_sink` with a synthetic stream), responder-side with +hand-crafted frames on **one** stream, or builder/wire-shape tests. +`src/protocol/connection.rs` has **zero** publish tests. Specific +decided behaviors with no test: +1. End-to-end publish through `CallConnection::publish` ↔ + `Dispatcher::handle_stream` (would have caught P-01 immediately). +2. `deadline: None` for Pub — `dispatch_pub_clears_deadline_to_none` + asserts nothing about the deadline; the test name is misleading. +3. `publish_schema` validation (untested + unimplemented, P-03). +4. Initiator `call.error` terminating the publish stream (P-04). +5. Abort during a Pub, both directions (P-06 — same-stream path is + dead-untested; no initiator-side `abort()` of an in-flight publish). +6. `from_call` Pub forwarding — no test that `build_bundles` maps + `"pub"` → `HandlerKind::Sink`; no behavioral test of + `make_sink_forwarding_handler` (Err-dropping, forwarded_for + population). +7. Backpressure — nothing exercises a full 64-slot chunk channel + (`PUBLISH_CHANNEL_BUFFER`, `dispatch.rs:45`) with a slow handler, + or the handler-returns-early feed-termination path. +8. `OverlayOperationEnv` Sink arm (`connection.rs:523-528`) — the Sub + arm has a test; Sink does not. + +## P-13 [minor] — `SinkDispatch` exposes `chunk_tx` publicly + +**Verified:** YES. `SinkDispatch` exposes `chunk_tx: +futures::mpsc::Sender` publicly (`src/protocol/dispatch.rs:78-81`). The +channel type and buffer size are effectively public API now; consider +opaque wrapping before crates.io. + +--- + +# Part B — Channels (ADR-034–043, 047) findings + +## C-01 [critical] — Channel 0 is dead in both directions + +**ADR drift:** ADR-036 (channel 0 pre-negotiated as `alknet/call`). +**Verified:** YES, both sides. +- **Accept side:** `src/channels/adapter.rs:181-188` — channel 0's + `Connection` is built and bound to `_channel0_conn`, which is + **never handed to anything**. The `InstallChannelZero` hook + (`adapter.rs:41-42`) receives only `(&ChannelManager, &AuthContext)` + — it has no way to reach the channel-0 `Connection` or its send/recv + halves (already consumed by `install_channel_zero` at `adapter.rs:181`, + which can't be called again — it returns `ChannelExists`). Inbound + `call.requested` chunks on channel 0 route into a receiver nobody + reads. Channel 0 on the accept side is a black hole. +- **Connect side:** `src/channels/client.rs:64-65` wraps channel 0 in + `CallConnection`, but `CallConnection::call_with_payload` opens a + **new bidi stream per request** via `connection.open_bi()`, and + `ChannelBidiStreamSource::open_bi` returns `Err(StreamClosed)` + unconditionally (`src/channels/source.rs:58-60`). So **every** + `ChannelClient::call_open_op` fails immediately with "failed to open + stream." + +The call protocol's stream-per-request model +(`Dispatcher::run_loop` accepting streams in a loop, +`dispatch.rs:603-620`; `CallConnection` opening one stream per call) +is structurally incompatible with channel 0's single yield-once +`BiStream`. **This needs a single-stream call mode** (multiplex all +requests on one framed stream for both the client and the dispatch +side), which is a real design gap, not just a bug fix. **Acceptance +gate:** one end-to-end channel-0 test: `ChannelClient::call_open_op` +↔ `ChannelsAdapter` accept-side call dispatch, one round-trip. + +## C-02 [critical] — `register_openable` / the ADR-047 §3 open flow does not exist + +**ADR drift:** ADR-047 §3 (`ChannelCore` wrapper + +`register_openable(spec, open_handler, channel_core)` helper; the +ACL→cap→allocate→ledger→spawn→respond flow). +**Verified:** YES. `ChannelCore` has only `new`/`manager`/`check_open`/ +`on_close` (`src/channels/operations.rs:280-311`). Doc comments link to +`ChannelCore::register_openable` (`operations.rs:10, 62`) — the +rustdoc warnings in C-09 are those dangling links. There is currently +**no way to register a channel-open op at all**; the +`channel:forbidden` / `channel:no_channels_session` / +`channel:allocation_failed` / `channel:too_many_channels` error-code +mapping to `CallError` exists only in comments. The whole open flow is +unreachable. + +## C-03 [critical] — `resolve_channel_manager` (ADR-047 §4) is a stub + +**ADR drift:** ADR-047 §4 (per-connection `ChannelManager` resolution +via the extension-trait downcast). +**Verified:** YES. `resolve_channel_manager` unconditionally returns +`None` (`src/channels/env.rs:108-128`). The comment at `:111-125` admits +a real downcast is impossible without `Any`-machinery and says "the +implementation uses the `ChannelCore`'s stored manager instead" — +which contradicts ADR-047 §4's core point (a statically-registered +handler closing over a static `ChannelCore` "has no way to know which +channels connection invoked it"). `ChannelsSessionEnv` exists +(`env.rs:46`) but nothing constructs it outside tests (`env.rs:148` is +the only construction site). `channel:no_channels_session` appears only +in comments. **Decision needed:** either add `as_any()` to +`OperationEnv` (or similar) to make §4 implementable, or amend ADR-047 +to bless the "manager passed at registration via `ChannelCore`" shape. +The current stub gives you neither per-connection resolution nor the +amended spec. + +## C-04 [critical] — Backpressure is inverted: the demux drops chunks + +**ADR drift:** ADR-040 DP-5 / REQ-CH-05 (lossless bounded-buffer +backpressure — the demux stalls until the consumer drains). +**Verified:** YES. `route_payload` uses `try_send` and on `Full` logs a +warn and **drops the chunk** (`src/channels/manager.rs:253-258`). +`route_payload` is sync (`manager.rs:246`), so it *cannot* await — the +shape itself deviates. This is a reliability-layer protocol with no +retransmit; a dropped mid-stream data chunk silently corrupts the +channel's byte stream for the handler. **Fix:** make `route_payload` +async (or use a `blocking_send`/`reserve`-equivalent) and await on +full. + +## C-05 [critical] — The buffer bound is in messages, not bytes + +**ADR drift:** ADR-040 ("1 MiB default"; "256 × 1 MiB = 256 MiB worst +case per connection" memory bound). +**Verified:** YES. `DEFAULT_BUFFER_CAP = 1024 * 1024` +(`src/channels/reassembly.rs:35`) is passed as the **capacity of +`tokio::mpsc::channel`** (`reassembly.rs:77`, also `manager.rs:180, +219`, `mux.rs:115`), which counts *messages*, not bytes. The actual +per-channel bound is 1,048,576 chunks × up to 16 MiB each ≈ 16 TiB, +not 1 MiB. The doc comment at `reassembly.rs:31-34` even claims "1 MiB" +while the bound is ~6 orders of magnitude larger. ADR-040's +per-connection memory bound effectively does not exist. **Fix:** a +byte-budgeted bound (track accumulated `payload.len()` against the +cap, not message count). + +## C-06 [critical] — Opener-ledger decrement happens on only 1 of 4 teardown paths + +**ADR drift:** ADR-047 §7 (the self-DoS fix: decrement on +close-received, close-sent, handler-exit, connection-drop). +**Verified:** YES. Only close-received is wired +(`src/channels/operations.rs:197-204`). On transport EOF, +`clear_all()` returns the `(channel_id, opener)` list +(`src/channels/manager.rs:305-316`) and the demux loop **only logs its +length** (`src/channels/adapter.rs:139-143`) — `on_close` is never +called. Close-sent locally: nothing sends `channel/close` locally, no +decrement hook. Handler exit: a handler task that finishes naturally +triggers no teardown at all. The exact "peer whose connection dies at +cap is permanently at cap" self-DoS that ADR-047 §7 was written to fix +is back. The auth-blind adapter has no way to reach the policy — there +is no hook — so this is an architectural gap, not just a missed call. + +## C-07 [critical] — Demux desynchronizes permanently on an oversized chunk + +**ADR drift:** channels-wire.md §MAX_CHUNK_LEN (resync by skipping +`length` bytes; the parsed length is available in the error variant). +**Verified:** YES. On `ChunkError::TooLarge` (and any header-parse +error) the loop `continue`s without skipping the payload bytes +(`src/channels/adapter.rs:104-110`). The next 8-byte header read +consumes the first 8 bytes of the oversized payload → permanent stream +desync; every subsequent "header" is garbage routed to random +`channel_id`s (lenient drop hides the corruption). `ChunkError::TooLarge` +carries the parsed `length` (`src/channels/wire.rs:95-98`) — it is +discarded. **Fix:** on `TooLarge`, read and discard `length` bytes +before continuing. + +## C-08 [critical] — No channel-adoption path → channel-id agreement is impossible + +**ADR drift:** implied by ADR-037/047 §5 (connection-owner allocates). +**Verified:** YES. The side that did **not** allocate a `channel_id` +has no way to install routing state for it: `open_channel` always +allocates a fresh id (`manager.rs:167`), `install_channel_zero` is +hardcoded to 0. A consumer that receives `{channel_id: 7}` from the +responder cannot register 7 in its local manager/mux — inbound chunks +for 7 are dropped as unknown (`manager.rs:268-274`) and there's nothing +to write to. Additionally, both sides construct independent managers +with `next_id` starting at 1 (`manager.rs:98`; adapter side +`adapter.rs:170`; client side `client.rs:55`), so if both sides ever +allocate on one connection (Sub opened by A + Pub opened by A, per +ADR-047 §5's Pub case), **IDs collide** — one wire `channel_id`, two +different channels. No odd/even split or any collision-avoidance scheme +exists in either the ADRs (post-047) or the code. **This needs a spec +decision** (odd/even split, or an adoption op), not just a code fix. + +## C-09 [major] — `cargo doc` emits 4 warnings (broken doc links) + +**Verified:** YES — 4 warnings (the original reviews said 5; this pass +sees 4): +- `unresolved link to ChannelCore::register_openable` ×2 (from + `operations.rs:10, 62` — the nonexistent helper, C-02). +- `unresolved link to default_policy` (from `operations.rs:49`). +- `env is both a module and a macro` (from the `env` module name + colliding with the `env!` macro). + +The commit message's "cargo doc generates" was technically true; clean +it is not. + +## C-10 [major] — `channel/control` is a stub returning success for a dropped message + +**ADR drift:** ADR-037 ("the channels layer routes `message` to the +handler's control handle for `channel_id`"). +**Verified:** YES. `src/channels/operations.rs:236-248` parses +`channel_id`, ignores `message` (`_message`), and returns +`{"ok": true}`. There is no control-handle concept anywhere (no field +on `ChannelState`). Returning success for a silently-discarded control +message is worse than returning `unimplemented`. + +## C-11 [major] — `channel/resources/subscribe` emits a one-shot snapshot of the wrong data + +**ADR drift:** ADR-037 (as amended by ADR-047 §6): live subscription of +**openable resources** aggregated from ALPN-crate resource enumerators, +with subsequent events on any change. +**Verified:** YES, three problems +(`src/channels/operations.rs:257-277`): +1. `futures::stream::once(...)`: emits only the initial snapshot, never + "subsequent events on any change." ADR-037 explicitly rejected + poll-shaped behavior; this is a one-shot poll wearing a `Sub` type. +2. Wrong content: emits the ALPNs of **currently-open channels**, not + the set of **openable ALPNs** aggregated from ALPN-crate resource + enumerators ("which containers are running…"). A fresh connection + reports an empty resource set even when TTY/tunnel ops are + registered. +3. (Positive) The `access` preview **is** correctly dropped per + ADR-047 §6 (`operations.rs:255`; the output schema at `:148-161` has + no `access`). + +## C-12 [major] — No guard on `channel/close` with `channel_id: 0` + +**Verified:** YES. `channel/close` has no guard against `channel_id: 0` +(`src/channels/operations.rs:190`). Any peer can tear down channel-0 +state via `teardown_channel(0)` (`manager.rs:284-298`), killing the +call channel's demux routing. **Fix:** reject `channel_id: 0` in the +`channel/close` handler (channel 0 is pre-negotiated and not +closeable). + +## C-13 [major] — `channel/close` abort ≠ drain; REQ-CH-06 ordering absent + +**ADR drift:** ADR-041 §3 (decrement "after the drain completes"); +ADR-037 (responder drains and signals EOF to the handler); +channels-wire.md REQ-CH-06 (exit-chunk-before-close ordering — "the +channels layer owns the ordering guarantee"). +**Verified:** YES. `channel/close`'s handler task is `abort()`ed +immediately and the decrement happens immediately +(`src/channels/operations.rs:190-204`); abort ≠ EOF-and-drain. +REQ-CH-06 (the close handler must observe data-pump completion) has no +implementation anywhere. + +## C-14 [major] — Substrate modes: only in-line is implemented + +**ADR drift:** ADR-034 §substrate modes, ADR-039 (QUIC-native +multi-stream substrate). +**Verified:** YES. `handle()` calls `connection.accept_bi()` **once** +and never loops (`src/channels/adapter.rs:159`). The doc comment at +`adapter.rs:80-84` claims the multi-stream behavior the code doesn't +have. The QUIC-native substrate ("accept remaining bidi streams… read +headers off each") is not implemented. + +## C-15 [major] — `ChannelClient` API does not match ADR-043 + +**ADR drift:** ADR-043 (`open_channel(...) -> Channel`, +`Channel { channel_id, source }`, `subscribe_resources`, `call()` +accessor). +**Verified:** YES. `src/channels/client.rs` has `from_connection` ✓ +(transport-agnostic, correct per the amendment), but: no +`open_channel(...) -> Channel`, no `Channel { channel_id, source }`, +no `subscribe_resources`, no `call()` accessor (instead `call_open_op` ++ `take_call_connection`). The doc comment at `client.rs:104-106` +refers to `manager.open_channel_stream(channel_id)` — **no such method +exists**. A consumer who receives `{channel_id: 7}` from the responder +has no way to obtain a `BiStream` for it (see C-08, channel adoption). + +## C-16 [major] — Busy-wait spin on full write buffer + +**Verified:** YES. `poll_write` on `TrySendError::Full` does +`cx.waker().wake_by_ref(); Poll::Pending` +(`src/channels/reassembly.rs:256-257`) — a hot busy-loop that pegs a +core until the mux drains (the waker is re-woken immediately, not +registered against capacity). The correct shape is storing a +`reserve()`/`poll_ready`-style future. The abandoned deliberation left +in comments (`reassembly.rs:205-255`) documents that the author knew +this (see C-20). + +## C-17 [major] — Lost EOF sentinel path → remote handler hang + +**Verified:** YES, with a precision correction to Sub Review B. The mux +pump **does** write an EOF chunk to the wire when it receives the +zero-length sentinel (`src/channels/mux.rs:124-133`). The real bug: +`MpscSendStream::poll_shutdown` (`reassembly.rs:283-291`) and `Drop` +(`:295-309`) emit the sentinel via `try_send`, best-effort; if the +buffer is full the sentinel is silently dropped. When the sentinel is +lost, the pump's `recv` yields `None` (sender dropped, no sentinel) and +the pump loop exits (`mux.rs:122-147`) **without** writing an EOF +chunk. Result: the remote handler never sees channel EOF until full +transport close. REQ-CH-01 violation on this path. The doc comments at +`reassembly.rs:283-291, 295-309` ("the peer's read will still EOF when +the sender drops — REQ-CH-02") are **wrong about which side of the +wire** this sender feeds (it feeds the local mux pump, not the peer's +demux). The doc comment at `mux.rs:109-112` ("the pump emits the EOF +sentinel") is misleading because it depends on the `Drop` impl's +`try_send` succeeding. **Fix:** the pump must write an EOF chunk when +its receiver ends without a sentinel (treat `recv → None` as implicit +EOF). + +## C-18 [major] — `open_channel` TOCTOU on `max_channels` + +**Verified:** YES. `src/channels/manager.rs:159-194` — the cap check +(`:161`) and the insert (`:188-189`) are under **separate** lock +acquisitions with `mux.register(...)` awaited in between +(`:170-176`). N concurrent opens at `len == max-1` all pass the check +and all insert → cap exceeded by N-1. **Fix:** hold the channels lock +across check + insert (release for the `mux.register` await, re-acquire +for insert, re-check on re-acquire). + +## C-19 [minor] — Misc channels correctness issues + +- **`next_id` wrap allocates channel 0:** `manager.rs:98` starts at 1; + `fetch_add(1, Relaxed)` wraps `u32::MAX → 0`, colliding with the + reserved channel 0. The `ChannelExists` defensive check catches the + map collision (`:189-193`), but only after registering a mux pump + that is never cleaned up. Effectively unreachable in practice + (ADR-040), but the wrap story is not handled as stated. +- **Unbounded loop in resources snapshot:** `operations.rs:267-274` — + `for channel_id in 0..u32::MAX` with a mutex lock per iteration, + break keyed on `resources.len() >= manager.open_count()`. + `open_count()` is re-read each iteration; a channel closing + mid-iteration (or sparse ids after churn — monotonic ids guarantee + sparseness) can iterate millions/billions of times. +- **Mux pump map leak + duplicate registration:** `mux.rs:148` — + finished pump `JoinHandle`s accumulate in `self.pumps` for the + connection's lifetime. Duplicate `register(channel_id)` silently + overwrites the handle while the old pump keeps running (two pumps, + one channel id). +- **Mux shutdown aborts pumps mid-`write_chunk`:** `mux.rs:154-158` — + pumps are `abort()`ed possibly mid-`write_chunk` while holding the + writer lock (a half-written chunk header corrupts the wire; moot + since the transport is going away, except the code then writes EOF + sentinels onto that possibly-mid-chunk stream). +- **`manager.rs:309`:** `drop(state.demux_sender.clone())` clones a + sender then drops the clone — a no-op (the real drop happens when + `drained` falls out of scope; the code works by accident, the line is + misleading). +- **`manager.rs:176, 218`:** mux `register` failure (runner closed) + mapped to `ManagerError::ChannelExists` — wrong error. +- **`client.rs:61`:** `install_channel_zero` error mapped to + `StreamError::StreamClosed` — conflation. +- **`policy.rs:127-140`:** `check_open` **increments** the count + (check-and-reserve). If allocation subsequently fails, no rollback + API exists other than `on_close(&Identity)`; a future open-op wrapper + must remember to decrement on every post-check failure path or quota + leaks. Not a bug today (nothing calls it), but a trap in the trait's + contract vs ADR-041's "check_open = check, on_close = decrement after + deallocation" framing. +- **REQ-CH-04's error counter (`Demux::stats()`):** no counter, no + stats surface; only a `debug!` log (`manager.rs:268-274`). +- **`derive_alpn_from_op_name`** (`src/client/from_call.rs:289-301`): + ADR-047 says non-`alknet/*` ALPNs "use their full ALPN string as the + path segment." The implementation takes only the **first** path + segment (`rest.split('/').next()` at `:291`), so + `channels/custom/proto/sub` derives `alknet/custom` — wrong. The + `segment.starts_with("alknet/")` branch at `:295` is dead code (a + single segment can't contain `/`), and `segment == "alknet"` yields + the nonsense ALPN `"alknet"` (`:296`). +- **`Box::leak` per discovery** (`from_call.rs:312-314`): bounded per + unique ALPN in theory, but it leaks on *every* rediscovery of every + marked op. Documented in-code as accepted; the `Arc` refactor + should happen before any long-lived hub uses `from_call` against + churning peers. +- **`from_call.rs:269` "SAFETY:" comment** is factually wrong (describes + a `'static`-only return that isn't what `derive_alpn_from_op_name` + does — it returns `Option`; the leak happens in `leak_alpn`), + and "SAFETY" is a loaded term that conventionally marks `unsafe` + blocks; there's no `unsafe` here. +- **Ledger placement:** ADR-047 §7 says "the ledger lives in + channels-call, not channels-core," but `OpenerLedger` is defined in + `src/channels/mux.rs:169-207` and owned by `ChannelManager` + (`manager.rs:81`) — the auth-blind core layer. Doc comment at + `manager.rs:62-69` argues for this; it's a deliberate deviation, but + it is a deviation, and the "which-crate" boundary matters when the + crate is split per ADR-044. + +## C-20 [major → convention] — Abandoned deliberation + pervasive inline comments + +**Convention drift:** AGENTS.md §1 (no inline `//` comments unless +asked; doc comments `///`/`//!` are fine). +**Verified:** YES. Worst instance: `src/channels/reassembly.rs:205-255` +— ~50 lines of abandoned stream-of-consciousness deliberation ("no, we +need `Sender::reserve_slot`… Actually, the simplest approach…") shipped +inside `poll_write`. Plus pervasive inline `//` comments across the +channels module: `adapter.rs:154-158, 163-165, 177-180, 189-194`, +`client.rs:57, 67, 74-76`, `manager.rs:170, 177-179, 190-191, 196, +234-237, 268-269, 291-292, 314`, `env.rs:112-126`, +`operations.rs:188-189, 195-196, 243-247, 262-266`, +`from_call.rs:256-262, 269-274`. A handful qualify as "non-obvious +correctness constraint" (REQ references); most don't. Several comments +are actively *wrong* (the mux EOF claim `mux.rs:109-112`, the +abort-cancels claim `dispatch.rs:495-498`, the +`reassembly.rs:283-291` "peer's read will still EOF" wrong-side claim, +the `from_call.rs:269` "SAFETY:" comment marking no `unsafe`). + +## C-21 [minor → convention] — `write_header` panics on short buffers + +**Convention drift:** AGENTS.md §2 (no panics in library code). +**Verified:** YES. `src/channels/wire.rs:110-114` — `write_header` +slices `out[..CHUNK_HEADER_LEN]`, panicking on short buffers; the doc +comment at `:106-107` declares the panic. All current callers pass +`[0u8; 8]`, but the API is `&mut [u8]`. Same class: a potential panic +path documented instead of typed. **Fix:** return a +`Result<_, ChunkError::HeaderTooShort>` (the error variant already +exists for the parse side, `wire.rs:87-90`). + +## C-22 [minor → convention] — Filler tests padding the count + +**Verified:** YES. `src/channels/client.rs:131-136` — a `PhantomData` +"smoke test" (`let _ = std::marker::PhantomData::;`). +`src/channels/env.rs:155` — `assert!(env.contains("anything") || +!env.contains("anything"))` tautology. These pad the "66 new channels +tests" / "23 new Pub tests" counts claimed in the commit messages. + +## C-23 [minor] — `dispatch_requested` Sink/Stream errors use empty request id + +**Verified:** YES. `dispatch_requested`'s Sink error uses +`String::new()` as request id (`src/protocol/dispatch.rs:244-247`), +producing an envelope with an empty id (shared flaw with the Stream +arm at `:236-243`). + +## C-24 [minor] — Producer/consumer naming leaked into docs + +**Convention drift:** AGENTS.md §8 (producer/consumer, not +server/client). +**Verified:** YES, minor. Prod code is largely fine; "client→server +streaming" leaked from ADR-046 into `docs/architecture/call-protocol.md:335` +and the ADR-046 reference table at `:584`. No public API names offend; +only test-local `duplex` variable names use client/server +(`mux.rs:217, 253`, etc.) — acceptable. + +## C-25 — Test coverage gaps (channels) + +**Verified:** YES — present coverage is unit-level only; **no +end-to-end test wires two sides together**. Missing: +1. Channel 0 end-to-end (ADR-036): adapter ↔ client, one + `call_open_op` round-trip (would catch C-01 immediately). +2. Demux resync on `TooLarge` (channels-wire.md §MAX_CHUNK_LEN): send + oversized chunk + valid chunk; assert the valid one survives (C-07). +3. Backpressure semantics (ADR-040/REQ-CH-05): slow reader on channel + A, fast writer, assert no data loss and channel B unaffected (C-04, + C-05, C-16). +4. Policy decrement on connection drop / handler exit (ADR-047 §7): + open at cap, drop transport, reopen (C-06). +5. Per-connection `max_channels` via `open_channel` — no test fills 256 + and asserts `TooManyChannels` (only `Display` is tested, + `manager.rs:446-452`); no concurrent-open race test (C-18). +6. REQ-CH-01 through the mux to the wire on drop-without-shutdown — + `mux_handle_register_and_write_round_trips_to_transport` covers + explicit shutdown only; the drop-with-full-buffer path is untested + (C-17). +7. REQ-CH-06 — untested (and unimplemented, C-13). +8. `channel/close` / `channel/control` / `resources/subscribe` handler + invocation — the handler bodies (`make_close_handler` etc.) have + zero tests; only spec-shape and registration are tested + (`operations.rs:329-364`). +9. `resolve_channel_manager` — untested (it's a stub; a test asserting + the ADR-047 §4 behavior would have flagged it, C-03). +10. ID wrap → 0 — untested (C-19). +11. Marker wire format: well tested ✓ (`discovery.rs:807-839`, + `from_call.rs:592-653`, `spec.rs:338-365`). + +## C-26 — Spec docs internally inconsistent post-047 + +**Verified:** YES. Only `channel-operations.md` was updated. The other +channels spec docs still describe the pre-047 model that ADR-047 +removed, and carry stale alknet ADR numbers (071/075/076/093/094/079/ +080) that point to nonexistent files after the renumbering (alkcall +decisions only go up to 047): +- `docs/architecture/channel-client.md:54-59, 94-98` still specifies + `open_channel(alpn, params, direction)`, `ChannelDirection`, and + `ResourceEntry.access` — all removed by ADR-047. +- `docs/architecture/channels-wire.md:241` lifecycle table still says + "`channel/open` call operation… responder allocates." +- `docs/architecture/channels-adapter.md:142-194` still describes the + generic `channel/open` handler with `channel:unknown_alpn`. +- Stale ADR-number references: `channel-client.md:170-171`, + `channels-wire.md:273-274`, `channels-adapter.md:294-299`. + +--- + +# Cross-cutting: what is solid (verified) + +These were checked and are correct/close to spec — listed so the +remediation plan can focus on what's actually broken: + +- **Wire formats:** `EventEnvelope` shape and the five+one event types + match ADR-014/046 exactly; the channels 8-byte chunk header + `[channel_id:u32 BE][length:u32 BE]` matches ADR-034/035 exactly + (`src/channels/wire.rs:85-114`). No `stream_type` anywhere in the + channels layer (verified by grep). ✓ +- **`HandlerKind::Sink` validation:** Pub↔Sink both directions + (`src/registry/registration.rs:108-124, 371-411`), `make_sink_handler` + (`:587-593`), `with_local_sink`/`with_leaf_sink`/ + `with_leaf_sink_provenance` (`:507-552`). ✓ +- **Wire `op_type` strings:** `"query"`/`"mutation"`/`"sub"`/`"pub"` + emit (`src/registry/discovery.rs:163-169`) and parse with + `"subscription"` correctly rejected (`src/client/from_call.rs:316-326`). + ✓ +- **`call.published` payload `{ "input": }`:** `src/protocol/wire.rs:54-56`, + correlated by envelope `id`. ✓ +- **Responder-side `deadline: None` for Pub:** `dispatch.rs:310` + (implemented; untested — P-12). +- **`invoke()` / `invoke_streaming()` on Pub → `INVALID_OPERATION_TYPE`:** + `registration.rs:193-197, 262-271`. ✓ +- **Gap F (`channel_open` marker):** `spec_to_json` emits + `"channel_open": true` only when set (`discovery.rs:213-215`), + schema documents it as optional (`:145-148`), `rebuild_spec_for` + parses it and ignores `false`/non-channels names (`from_call.rs:263-277`). + `ChannelOpenSpec { alpn: &'static str }` matches ADR-047 §2 + (`spec.rs:38-40`). Well tested. ✓ +- **Per-identity cap default 256:** `policy.rs:156`, `default_policy()`, + `PerIdentityChannelPolicy::new(256)` tested at `policy.rs:252-259`. ✓ +- **Channel 0 pre-installed, skips the ledger:** correct + (`manager.rs:234-237`), ALPN recorded as `alknet/call`. ✓ (Functionally + dead per C-01, but the bookkeeping is right.) +- **Connection-owner allocation mechanism:** `ChannelManager::open_channel` + allocating via `next_id.fetch_add` (`manager.rs:167`) — "whoever holds + the manager allocates" ✓ — but nothing invokes it in the Sub flow (no + open-op wrapper, C-02) and the Pub-case flow doesn't exist. +- **Layering held:** no channels types leaked into the call crate, + `OperationEnv` stayed a trait, auth stays on the one + `AccessControl` path. ✓ +- **Hub relay (ADR-042), broker (Gap B), `from_call` relay wrapper + (Gap C):** intentionally out of scope per ADR-047 — correctly not + flagged as bugs. ADR-047 Gap C expects alkcall's `from_call` to + reconstruct the marker so the consumer can branch — that half **is** + implemented ✓. +- **No `unwrap()`/`expect()`/panics** in the new library code outside + the `write_header` short-buffer path (C-21) and the channels + deliberation comments. No blocking I/O on async paths. Poisoning N/A + (`parking_lot`). ✓ + +--- + +# Remediation plan + +The plan is split into **six units of work**, ordered by dependency and +severity. Each unit is a independently shippable commit (or small +sequence) with its own acceptance gate. The units are sized so that +none of the critical bugs is blocked behind a spec decision except +where noted (C-03, C-08). The overarching acceptance gate for the +whole effort is the single end-to-end test both reviews identify as +missing: + +> **One end-to-end test:** `ChannelClient` ↔ `ChannelsAdapter`, open a +> channel via a registered op, move bytes, tear down, verify quota. + +That test is the gate because its absence is what let both commits land +green. Each unit below either moves toward it or removes a defect it +would surface. + +## Unit 1 — Fix Pub end-to-end (P-01, P-02, P-05, P-08, P-12 #1) + +**Goal:** a `publish()` that actually works on any transport. +**Files:** `src/protocol/connection.rs`, `src/protocol/dispatch.rs`. +**Scope:** +- P-01: keep stream A's write half; pump `call.published` + `call.completed` + on it (do not re-open `open_bi` in `write_publish_chunks`). +- P-02: change `publish`/`publish_with_payload` to take + `Pin + Send>>` (ADR-046 §8 shape). Hold + stream A's write half and pump the stream. +- P-05: register the publish like `subscribe` (`timeout: None`), not + with `DEFAULT_CALL_TIMEOUT`. Falls out of the streaming rework. +- P-08: route the dispatch Pub branch through + `OperationRegistry::invoke_sink` instead of re-implementing it inline + (removes the divergence hazard; the `invoke_sink` path becomes the + wire path). +- P-12 #1: add the end-to-end publish test (≥2 chunks, assert the + responder sees them and returns the right result). This is the + acceptance gate for the unit. + +**Note:** P-02 changes a public API (`publish` signature) — but per +ADR-046 §8 the `Stream` shape *is* the spec, so the current `Vec` +was the drift. This is correcting drift, not introducing a wire break +(the wire format is unchanged). No deployments exist. + +## Unit 2 — Channel 0 single-stream call mode, both sides (C-01, C-25 #1) + +**Goal:** channel 0 actually carries call traffic in both directions. +**Files:** `src/protocol/connection.rs`, `src/protocol/dispatch.rs`, +`src/channels/source.rs`, `src/channels/client.rs`, +`src/channels/adapter.rs`. +**Scope:** This is the real design gap, not a one-line fix. The call +protocol's stream-per-request model is incompatible with channel 0's +single yield-once `BiStream`. Options: +- (a) A single-stream call mode: multiplex all requests/responses on + one framed stream (length-prefixed `EventEnvelope`s on channel 0's + one `BiStream`). Both `CallConnection` (client) and `Dispatcher` + (accept side) gain a "single-stream mode" that reads/writes frames + on the one `BiStream` instead of opening/accepting one stream per + request. `ChannelBidiStreamSource::open_bi` stops being called. +- (b) Make `ChannelBidiStreamSource` yield a fresh `BiStream` per + `accept_bi`/`open_bi` by framing sub-streams over the one channel — + more complex, probably wrong. +Recommend (a). This unblocks C-01's connect side (every `call_open_op` +no longer fails) and accept side (channel 0's `Connection` is actually +driven by a call dispatch loop). **Acceptance gate:** C-25 #1 — one +end-to-end channel-0 round-trip test. **May require a short ADR or +ADR-036 amendment** recording the single-stream-mode decision (it's a +two-way-door implementation detail per ADR-046/036, but worth pinning). + +## Unit 3 — `register_openable` + decide the §4 env-resolution question (C-02, C-03) + +**Goal:** a channel-open op can actually be registered and invoked. +**Files:** `src/channels/operations.rs`, `src/channels/env.rs`, +`src/registry/env.rs` (possibly, if `as_any()` is added). +**Scope:** This unit requires a **spec decision first**: +- **Option A (implement §4 as written):** add `as_any()` (or equivalent + downcast machinery) to `OperationEnv`, implement + `resolve_channel_manager` for real, and have the open-op wrapper + resolve the per-connection manager at invocation time. +- **Option B (amend ADR-047 §4):** bless the "manager passed at + registration via `ChannelCore`" shape that the stub's comment + describes. This drops per-connection resolution but is simpler; it + works only if a `ChannelCore` is registered per-connection (an + overlay concern), which interacts with ADR-019's layering. +Recommend Option A (it preserves the ADR's stated design and the +session/connection overlay patterns from ADR-024). Whichever is +chosen, **amend or affirm ADR-047 §4 in writing** before coding. Then: +- Implement `ChannelCore::register_openable(spec, open_handler, + channel_core)` wrapping the ALPN open handler with the + ACL→`check_open`→allocate→ledger→spawn→respond flow + (`src/channels/operations.rs:280-311`). +- Map `channel:forbidden` / `channel:no_channels_session` / + `channel:allocation_failed` / `channel:too_many_channels` to + `CallError`. +- Fix the broken rustdoc links (C-09's `register_openable` ×2) once the + method exists. +- Test: register a no-op open op via `register_openable`, invoke it + end-to-end through channel 0 (depends on Unit 2), assert + `channel_id` is returned and quota is reserved. + +## Unit 4 — Backpressure fixes (C-04, C-05, C-07, C-16, C-17, C-25 #2 #3 #6) + +**Goal:** the channels layer is lossless and bounded as ADR-040 +promises. +**Files:** `src/channels/manager.rs`, `src/channels/reassembly.rs`, +`src/channels/adapter.rs`, `src/channels/mux.rs`. +**Scope:** +- C-04: make `route_payload` async (or use `blocking_send`/`reserve`), + await on full instead of dropping. The demux loop calls it on the + read path — make the demux loop await it. +- C-05: replace the message-count buffer cap with a byte-budgeted cap + (track accumulated `payload.len()` against the cap). Update + `DEFAULT_BUFFER_CAP`'s doc comment to match reality. +- C-07: on `ChunkError::TooLarge`, read and discard `length` bytes + before continuing. The parsed length is in the error variant. +- C-16: replace the `wake_by_ref` + `Pending` busy-loop with a stored + `reserve()` future (or `Poll`-friendly ready notification). Remove + the ~50 lines of abandoned deliberation while here (C-20 overlap). +- C-17: the mux pump must write an EOF chunk when its receiver ends + without a sentinel (treat `recv → None` as implicit EOF). Fix the + wrong-side doc comments in `reassembly.rs:283-291, 295-309` and the + misleading `mux.rs:109-112` claim. +- C-25 #2 #3 #6: add the demux-resync test, the backpressure test, and + the drop-without-shutdown EOF test. + +## Unit 5 — Ledger decrement on all teardown paths + channel-id adoption/collision decision (C-06, C-08, C-12, C-13, C-18) + +**Goal:** quota accounting is correct and channel ids don't collide. +**Files:** `src/channels/adapter.rs`, `src/channels/manager.rs`, +`src/channels/operations.rs`, `src/channels/mux.rs`. +**Scope:** +- C-06: wire `on_close` into the three missing teardown paths: + close-sent locally, handler-exit, connection-drop. On transport EOF, + `clear_all()` already returns the `(channel_id, opener)` list — call + `policy.on_close(&opener)` for each (the adapter needs a hook to the + policy; this is the architectural gap Sub Review B flagged — add a + `DecrementOnDrop` hook or have the adapter hold a `ChannelCore`). +- C-08: **spec decision needed** — odd/even id split, or an adoption op + so the non-allocating side can install routing for a remotely-assigned + id. Recommend odd/even (initiator allocates odd, responder even, or + vice versa) as the smaller change; record the decision in ADR-047 §5 + or a follow-on ADR. Then implement the adoption path: the + non-allocating side registers the remotely-assigned id in its + manager/mux. +- C-12: reject `channel_id: 0` in the `channel/close` handler. +- C-13: `channel/close` must drain (signal EOF to the handler, await + pump completion) before `abort`/decrement; implement REQ-CH-06 + ordering (the close handler observes data-pump completion). +- C-18: hold the channels lock across the `max_channels` check + insert + in `open_channel` (re-check on re-acquire after the `mux.register` + await). +- Test: C-25 #4 #5 — open-at-cap-then-drop-transport-then-reopen, and + the concurrent-open race test. + +## Unit 6 — Cleanup: comments, doc links, stale spec docs, stubs (C-09, C-10, C-11, C-14, C-15, C-19, C-20, C-21, C-22, C-23, C-24, C-26, P-03, P-04, P-06, P-07, P-09, P-10, P-11, P-13) + +**Goal:** conventions satisfied, docs consistent, the remaining +decided-but-unimplemented behaviors either implemented or explicitly +deferred with an OQ. +**Scope:** This is the largest unit by item count but the lowest risk; +do it last, after the integration is end-to-end functional. Items: +- **C-20:** remove the ~50 lines of abandoned deliberation in + `reassembly.rs:205-255` and the pervasive inline `//` comments + across the channels module that aren't carrying a non-obvious + correctness constraint. Fix the actively-wrong comments (mux EOF + claim, abort-cancels claim, wrong-side peer-EOF claim, "SAFETY:" + comment marking no `unsafe`). +- **C-09:** fix the remaining `cargo doc` warnings (`default_policy` + link, `env is both a module and a macro`). +- **C-26:** update `channel-client.md`, `channels-wire.md`, + `channels-adapter.md` to the post-047 model (remove + `open_channel(..., direction)`, `ChannelDirection`, + `ResourceEntry.access`, the generic `channel/open` handler, + `channel:unknown_alpn`; renumber stale alknet ADR refs to alkcall + ADR-001..047). +- **C-21:** make `write_header` return `Result<_, ChunkError>` instead + of panicking. +- **C-22:** remove the filler tests (`client.rs:131-136`, `env.rs:155`) + or replace them with real assertions. +- **C-23:** give `dispatch_requested`'s Sink/Stream error envelopes a + real request id (or document why empty is correct). +- **C-24:** replace "client→server streaming" in + `call-protocol.md:335` with producer/consumer phrasing; amend + ADR-046 §3 to match §6 (P-11). +- **C-19 misc:** fix `next_id` wrap, the unbounded resources-snapshot + loop, the mux pump map leak / duplicate registration, the wrong + `ManagerError` mapping on mux register failure, the misleading + `drop(state.demux_sender.clone())`, the `derive_alpn_from_op_name` + multi-segment bug + dead branch, the `Box::leak` `Arc` refactor, + the `policy.rs:127-140` check-and-reserve rollback trap (document or + add a rollback API), the `Demux::stats()` error counter (REQ-CH-04). +- **P-03:** implement `publish_schema` per-chunk validation using + `alktype` (the dependency is already declared and unused), and wire + it through `spec_to_json` / `operation_spec_schema()` / + `rebuild_spec_for` so imported Pub ops keep the schema. Or, if + deferred, file an OQ and remove the misleading doc comment at + `spec.rs:181-186`. +- **P-04:** make `pump_sink` match `call.error` and inject the + initiator's `CallError` as an `Err` item that terminates the stream + (per ADR-046 §6). +- **P-06:** implement abort-cancels-Pub. This likely needs the + single-stream call mode from Unit 2 (so an abort on the same stream + can reach the running pump) plus a mechanism to drop the handler + future on abort (the doc comments at `dispatch.rs:494-498` describe + the target shape). Decide whether to fix the cross-stream path + (`abort()` opening a new stream) or deprecate it in favor of + same-stream abort. +- **P-07:** fix the handler-returns-early stall — when the handler + returns, the feed should be short-circuited and the response written + immediately (don't `join!` to completion). Use `tokio::select!` or + feed the handler's completion back into the feed loop. +- **P-09:** fix `make_sink_forwarding_handler` to stream (not + `collect`), and to terminate on `Err` (not `filter_map` it away). + Depends on P-01/P-02. +- **P-10:** use the `EVENT_*` wire constants in `pump_sink` instead of + string literals. +- **P-13:** opaque-wrap `SinkDispatch::chunk_tx` before crates.io. +- **C-10 / C-11:** either implement `channel/control` routing and live + `resources/subscribe`, or file OQs and make the stubs honest (return + `unimplemented`-style errors, not fake success). +- **C-14 / C-15:** either implement the QUIC-native substrate and the + full `ChannelClient` API (`open_channel`, `Channel`, + `subscribe_resources`), or file OQs and remove the misleading doc + comments / nonexistent method references. + +## Suggested sequencing + +``` +Unit 1 (Pub end-to-end) → unblocks the Pub flagship use cases +Unit 2 (channel 0 single-stream) → unblocks all channel-0 integration tests +Unit 3 (register_openable) → needs a §4 spec decision first; unblocks opening channels +Unit 4 (backpressure) → independent; can run in parallel with 2/3 +Unit 5 (ledger + adoption) → needs a §5 spec decision first; depends on 3 +Unit 6 (cleanup) → last; after the integration is end-to-end functional +``` + +Units 1, 2, 4 can proceed immediately with no spec decisions. Units 3 +and 5 each need one written decision (ADR-047 §4 affirm/amend; ADR-047 +§5 odd/even-or-adoption). Unit 6 is the long tail. + +## On the baseline objective + +The layering held, the wire formats are right, the architecture is +right. What's missing is the connective tissue a tty/docker crate would +actually call: a working `publish()`, a working channel 0, +`register_openable`, channel adoption, and a streaming `publish()`. +**A tty extraction cannot start on this foundation yet** — treat the +end-to-end `ChannelClient` ↔ `ChannelsAdapter` test (Unit 2's gate, +then Unit 3's gate) as the acceptance gate for the next round. + +--- + +## Verification log (this consolidation pass) + +Findings re-verified directly in source on 2026-08-12 against tree +`f305f8c`. Every `file:line` reference above was checked. Findings that +did not survive verification or were overstated in the original +reviews, corrected here: + +- **Sub Review B's "the pump does not write an EOF chunk" (C-17):** + overstated. The pump *does* write EOF when it receives the sentinel + (`mux.rs:124-133`). The real bug is narrower: when the sentinel is + lost (full buffer `try_send`), the pump exits on `recv → None` + without writing EOF. C-17 above states the corrected mechanism. +- **Warning count (C-09):** the main review said 5; this pass sees 4 + `cargo doc` warnings (2× `register_openable`, 1× `default_policy`, + 1× `env` module/macro). Documented as 4. +- **P-11:** flagged in Sub Review A as drift; on verification this is + an internal ADR-046 inconsistency (§3 vs §6) that the code resolved + correctly and consistently. Reclassified from "drift" to "amend the + ADR §3 text." + +All other findings (P-01 through P-10, C-01 through C-08, C-12 through +C-26) verified as stated. \ No newline at end of file