243 lines
12 KiB
Markdown
243 lines
12 KiB
Markdown
# 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/<alpn>/pub` as not-provided until
|
|
implemented (if rejecting); either way close the gap between the
|
|
ADR table (which lists `channels/<alpn>/pub` as a supported shape)
|
|
and the code. |