docs(review 010): stub sweep — four invocation-failing stubs in the published surface
This commit is contained in:
glm-5.3-flash committed
1 parent
3bda29c419
commit
1793265b04
1 file changed
+243
@@ -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/<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.
|
||||
Reference in new issue
Block a user