From 1793265b04e8c63fe0374b2669690e86d503f7b4 Mon Sep 17 00:00:00 2001 From: "glm-5.3-flash" Date: Tue, 13 Oct 2026 06:40:10 +0000 Subject: [PATCH] =?UTF-8?q?docs(review=20010):=20stub=20sweep=20=E2=80=94?= =?UTF-8?q?=20four=20invocation-failing=20stubs=20in=20the=20published=20s?= =?UTF-8?q?urface?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/reviews/010-stub-sweep.md | 243 +++++++++++++++++++++++++++++++++ 1 file changed, 243 insertions(+) create mode 100644 docs/reviews/010-stub-sweep.md diff --git a/docs/reviews/010-stub-sweep.md b/docs/reviews/010-stub-sweep.md new file mode 100644 index 0000000..290f632 --- /dev/null +++ b/docs/reviews/010-stub-sweep.md @@ -0,0 +1,243 @@ +# Review 010 — Stub sweep of the published surface (0.8.1) + +## Status + +Open — filed 2026-10-13. Triggered by a downstream finding: alkblobs' +ops-put task hits `channel:pub_open_not_implemented` when its Sink op +carrying the `channel_open` marker is invoked, exactly as the code +comments predict. The downstream project is paused on this. + +Policy context: shipping stubs to crates.io is now a hard failure per +the repo owner's ruling. Any handler that registers successfully and +returns an error of the form `*:not_implemented` on every invocation +is a bug, not a documented limitation. The state tracked in OQ-39, +OQ-40, and the C-08 blocker comment is re-classed from "deferred +open question" to "unpublishable stub" by this review. The rationale +previously used to justify shipping them (loud failure, OQ tracking) +was reviewed and rejected: a loud stub is still a stub, and the only +accepted endpoint is implement-or-stop-advertising before the next +published version. + +Full sweep of `src/` performed: no `TODO`/`FIXME`/`todo!`/ +`unimplemented!` markers exist anywhere; the four findings below are +the complete set of registered-but-always-failing handlers. No other +module (gateway, client, protocol, core) contains an +invocation-time unimplemented path. + +## Scope + +Every handler registered on a public surface — `OperationRegistry` +(local or per-connection overlay), `services/list` discovery output, +relay openables — that cannot serve its declared operation type. +Evidence includes pinned tests that *assert* the stub behavior: + +- `make_open_handler_sink_returns_not_implemented` + (src/channels/operations.rs:1695) +- `resources_subscribe_handler_returns_not_implemented` and + `resources_subscribe_handler_not_implemented_even_when_empty` (src/channels/operations.rs:1303, 1322) +- `control_handler_success_path_returns_not_implemented` (src/channels/operations.rs:1439) +- relay_tests.rs:113 asserts `pub_open_not_implemented` + +Findings continue the review numbering with prefix `S` (stub). + +--- + +## S-01 [critical] Pub-typed open wrapper: registered, advertised, returns `INTERNAL` on every invocation + +**Location:** `make_open_handler_sink`, src/channels/operations.rs:1158-1199. +Wired into `ChannelCore::register_openable_with_establisher` at +src/channels/operations.rs:682 (`OperationType::Pub => +HandlerKind::Sink(make_open_handler_sink(...))`). + +**Behavior:** registration for a `Pub`-typed open op **succeeds**. The +spec enters `OperationRegistry` with the `channel_open` marker and a +`publish_schema`, so it is discoverable via `services/list` / +`services/schema` and selectable by a consumer's open flow. The first +invocation fails with `channel:pub_open_not_implemented: Pub-typed +open ops are not yet implemented (requires channel-adoption path, +C-08/Unit 5)`. + +**Exposure window:** introduced 2026-08-13 (commit 8066d08), four +days before the first published version — this stub has shipped in +every release from v0.1.0 through 0.8.1, nine published versions. + +**Why this one is inexcusable rather than merely incomplete:** the +entire Sink stack ADR-046/047 call for is implemented and tested in +the same crate: + +- wire event `call.published` (src/protocol/wire.rs:17) +- serving-loop sink dispatch, chunk feeding, `publish_schema` + per-chunk validation (src/protocol/dispatch.rs:607-643, 1002, 1225) +- registry `resolve_sink_handler` / `invoke_sink` (src/registry/registration.rs:428-509) +- client-side sink forwarding, initiator→responder `call.published` + streaming (src/client/from_call.rs:478-496) +- gateway sink dispatch (src/gateway/dispatch.rs:160-550) +- discovery advertises per-chunk `publish_schema` on the open op's + spec (src/registry/discovery.rs:178) + +The only missing piece is the open-op wrapper itself. The stub's +textual justification (data plane must ride the adopted channel's +`BiStream`, gated on the C-08 adoption path) is in tension with +ADR-047 §1's own semantics, which define the open-op Pub case as +data flowing over channel 0's call plane via `call.published` — +precisely what the implemented stack does. The comment even admits +the design question is unresolved ("whether that data flows through +the allocated channel's `BiStream` ... or is consumed directly by the +`OpenHandler`"), i.e. the C-09-style blocker was asserted, not +established. This is hedges-cured-as-code. + +**Downstream impact:** alkblobs ops-put (`/workspace/@alkdev/alkblobs/tasks/ops-put.md`) +depends on this exact shape (`blobs/put` as a Sink spec with the +`channel_open` marker); the task is paused on the decision. ADR-051 +§6 documents the same gap from the relay side, where registration at +least fails loudly. + +**Remediation options (pick one before the next release):** + +1. **Implement the wrapper** — allocate the channel id, run the + establishment phase (reply fields carry the offer ack, per ADR-047 + §1 and ops-put's acceptance criteria), hand the `OpenHandler` the + `PublishStream` already produced by the serving loop along with the + channel's `Connection`. All plumbing exists; this is wrapper glue, + plus deciding the OpenHandler-reads-channel-vs-reads-PublishStream + question in an amended ADR-047. +2. **Reject at registration** — return an error from + `register_openable_with_establisher` for `OperationType::Pub`, + mirroring `RelayError::PubTypedOpen` (src/channels/relay.rs:147-156), + which already follows the correct fail-fast pattern. The op never + exists, so nothing is advertised that cannot serve. ops-put then + re-plans its establishment ack onto a plain Sink op without the + marker, or waits for option 1. + +Option 2 is the honest minimum for the next release; option 1 +unblocks ops-put directly. + +## S-02 [critical] `channel/control`: ADR-037 op registered on every channels connection, always fails + +**Location:** `make_control_handler`, src/channels/operations.rs:242-272. +Registered publicly by `ChannelOps::register_on` (src/channels/operations.rs:67), +spec exported (`channel_control_spec`, line 124). OQ-39; and +docs/architecture/channel-operations.md §"channel/control — +out-of-band control on channel 0" documents the request shape as if +the op works. + +**Behavior:** validates `channel_id`, checks `manager.has_channel`, +then returns `channel:control_not_implemented` unconditionally. The +channel-existence check is the only code that does anything — which +perversely means the stub returns *not found* for unknown channels +but *not implemented* for real ones. + +**Exposure window:** shipped since the ADR-037 surface landed +(0.1.0-era); nine published versions. + +**Remediation:** implement the control-handle surface (the design is +specified in OQ-39: ALPN-crate-registered per-channel control +callbacks, channels-layer registry keyed by `channel_id`), or remove +`channel_control_spec` from `ChannelOps::register_on` until a +consumer needs it. Publishing an op that exists on the wire's +discovery surface but cannot run is the failure mode being outlawed. + +## S-03 [critical] `channel/resources/subscribe`: ADR-037 §6 streaming op registered, always fails with a one-shot error + +**Location:** `make_resources_subscribe_handler`, +src/channels/operations.rs:283-297. Registered by +`ChannelOps::register_on`; spec exported (line 146). OQ-40. + +**Behavior:** the `StreamingHandler` emits exactly one +`channel:resources_not_implemented` error envelope — a fake stream +that terminates after one error, rather than a snapshot or a live +subscription. The doc comment says "callers fail loudly rather than +receiving a fake one-shot snapshot", but the loud failure *is* the +fake one-shot artifact. + +**Exposure window:** same as S-02. + +**Downstream note:** OQ-40 self-declares this "load-bearing for +alktunnels' discovery UI"; a hub or dashboard consumer that discovers +this op gets a capability that does not exist. + +**Remediation:** implement the aggregated-enumerator subscription +(ADR-047 §6), or drop the op from `ChannelOps::register_on` (the +static half — `OperationSpec.description` on `services/list` — +remains available, as OQ-40 itself records). + +## S-04 [major] Peer-announced Sub/Pub ops install invocation-failing stubs on connection overlays + +**Location:** `forwarding_stub_for_announced_op`, +src/registry/op_register.rs:215-229, called from `op_register_handler` +(line 184). + +**Behavior:** when a peer announces a `Sub`- or `Pub`-typed spec via +`op/register`, the handler is accepted and registered on the +connection overlay (`HandlerKind::Once`) with a body that returns +`INVALID_OPERATION_TYPE` on every invocation. The spec stays +discoverable on the peer-keyed overlay (`services/list-peers`, +provenance `FromCall`), and a composing handler that resolves through +`ctx.env.invoke` hits the error instead of the real op. + +Unlike S-01 through S-03 this path has a documented contract +rationale (nested composition is request/response-only; streaming/ +sink forwarding should ride `from_call` in the other direction), and +review 005 discusses the shape. Reviewed and re-classed: the +rationale may justify *not implementing nested sink forwarding*, but +it does not justify *accepting and advertising* the announcement. The +observable behavior is identical to a stub. + +**Remediation:** reject the announcement of `Sub`/`Pub` op types at +`op/register` time with a typed error (registration-time loudness, no +overlay entry), or wire them through the existing forwarding shapes +(`make_streaming_forwarding_handler` / `make_sink_forwarding_handler` +already exist in src/client/from_call.rs). Minimum-bar is the +rejection. + +--- + +## Non-findings (checked and cleared) + +The sweep had to distinguish real stubs from deferred-by-design +boundaries and test scaffolding. For the record: + +- `sink_empty_connection` (src/protocol/test_support.rs) — test-only + helper; every use is under `#[cfg(test)]`. Not a stub. +- `src/channels/adapter.rs` "stream header interpretation deferred to + the downstream ALPN crate (OQ-41)" — this is ADR-035's design + boundary (the channels layer does not interpret stream contents), + not missing functionality on a registered surface. +- Early-arrival chunk parking (src/channels/manager.rs:485) — + implemented behavior, the log text merely says "not yet adopted". +- `channels//sub` silent-stub shape (review 009 audit N-4) — already + fixed in 0.8.0; listed here as the precedent for the correct + remedy (loud failure at the boundary), though see the policy note + below since even loud invocation-time failure is no longer + sufficient for registered surfaces. + +## Verdict + +Four stubs shipped across nine published versions, three of them +reachable by any consumer doing routine discovery. The pinned tests +demonstrate the team knew, documented, tracked open questions for +each, and published anyway. Per the ruling, none may be present in +the next published version. The relay's `PubTypedOpen` / +`UnmarkedSpec` errors are the house pattern to generalize: **reject +at registration, never at invocation.** + +## Required actions before next release + +1. Resolve S-01/S-02/S-03 each as implement-or-stop-advertising. + Whichever path is chosen, no `*:not_implemented` error string may + remain reachable in `src/`. +2. Resolve S-04 at minimum by rejecting Sub/Pub announcements at + `op/register`; forwarding-through is the better fix and is + mechanically trivial given the existing from_call shapes. +3. Add a guard so this class cannot recur: a test that greps the + source tree for `not_implemented` (and a registration-time + invariant that a registered `HandlerKind` must be capable of + succeeding), wired into the release checklist in AGENTS.md. +4. Update OQ-39/OQ-40/C-08 entries and channel-operations.md to match + the post-fix reality; the "current stub" language in both is now + invalid documentation of shipped behavior. +5. Amend ADR-047 with the Pub open-op data-plane decision (if + implementing) or mark `channels//pub` as not-provided until + implemented (if rejecting); either way close the gap between the + ADR table (which lists `channels//pub` as a supported shape) + and the code. \ No newline at end of file