docs(review 006): alkcall 0.3.0 consequence review — WS-24/25 resolved upstream, residual enumerated

Records the review pass of alkcall 0.3.0 against this crate: - WS-24 (dispatch-resolution mechanism) and WS-25 (client-side op registration) verified resolved upstream — per-session fork (alkcall ADR-047 §4 amendment #2) + op/register (alkcall ADR-022 amendment 2026-09-03 + 2026-09-04 collision sub-amendment); the G-01 serving- loop concurrency rework lands here via the bump alone. - One new finding: UP-01 [minor] — ALREADY_EXISTS (new 0.3 protocol code) unmapped in the gateway error table (would 500); fix rides Unit 2. UP-02 records the op/register ACL posture decision for Unit 2. - WS-20/21/22/26/27/23 re-verified still open, now unblocked; the remediation plan re-sequences (Unit 2 wiring with the concrete fork shape, Unit 3 gates + two upstream-mirroring additions, Unit 4 docs). Baseline gates at df86f89: cargo test 453/575, clippy both sides, fmt, doc — all clean.
2026-09-04 14:58:43 +00:00
parent df86f89440
commit 90790374fa
1 changed files with 386 additions and 0 deletions
@@ -0,0 +1,386 @@
+# Review 006 — alkcall 0.3.0 consequence review (WS-24/WS-25 upstream, alkhttp residual)
+
+## Status
+
+Verified, open for Unit 2. One new finding (UP-01); the 0.3 bump
+committed and verified (`3dee46a`).
+
+## Scope
+
+Consequence review of alkcall 0.3.0 (published 2026-09-04 — the
+remediation of alkcall reviews 004–005, which is the upstream
+resolution of this repo's review-003 findings WS-24/WS-25) against
+this crate. The pass answers three questions:
+
+1. What does the 0.3 bump itself cost here (source breaks, behavior
+   changes)?
+2. Which review-003 findings are now resolved upstream, and which
+   remain alkhttp-side work?
+3. Did the upstream remediation introduce anything this crate must
+   absorb (new protocol codes, new wiring contracts, new decisions)?
+
+Sources read: alkcall review 004 (`docs/reviews/004-…md`, remediated)
+and review 005 (`docs/reviews/005-…md`, resolved G-01..G-05), the
+0.3.0 changelog, the ADR amendments (alkcall ADR-047 §4 amendment #2,
+ADR-022 amendments 2026-09-03 + 2026-09-04), and the published
+0.3.0 crate source. Every claim below verified in source at this
+tree (`df86f89`, consuming alkcall 0.3.0) or the published crate.
+
+Findings use the new prefix `UP` (upstream-consequence); review 003's
+WS numbering is referenced for continuity.
+
+## Baseline verification (this pass)
+
+```
+cargo test                                → 453 passed, 0 failed
+cargo test --all-features                 → 575 passed, 0 failed
+cargo clippy --all-targets -- -D warnings → clean
+cargo clippy --all-features --all-targets -- -D warnings → clean
+cargo fmt --check                         → clean
+cargo doc --no-deps                       → clean
+```
+
+Verified after the 0.2 → 0.3 bump (`3dee46a`). The bump cost was
+two source-break classes, both mechanical:
+
+- `ChannelClient::take_call_connection` now returns
+  `Option<Arc<CallConnection>>` (was a bare `CallConnection`) —
+  `WssSession::connect` dropped its `Arc::new` re-wrap
+  (`src/adapters/from_wss.rs:268-274`).
+- `OperationRegistry::register`, `ChannelOperations::register_on`,
+  and `ChannelCore::register_openable` now take
+  `&OperationRegistry` (was `&mut`) — 51 stale `mut` bindings
+  dropped via `clippy --fix` across `src/adapters/{from_wss,to_mcp,
+  to_openapi}.rs`, `src/gateway/routes.rs`, `src/server/adapter.rs`,
+  and `tests/`.
+
+The `&self`-registration change is the interior-mutability change
+that makes the per-session fork surface possible; for every existing
+alkhttp call site it is source-compatible (reborrow). The
+`registration()` → owned and `list_operations()` → owned return-type
+changes required no edits here (call sites already cloned through
+the borrow or consumed by value-compatible iteration — verified by
+the clean build).
+
+## Verdict
+
+- **WS-24 and WS-25 are resolved upstream.** The two design gaps
+  review 003 Part B identified are now mechanized in alkcall 0.3.0
+  and decided in its ADRs: the per-session fork is the
+  dispatch-registry mechanism (alkcall ADR-047 §4 amendment #2 —
+  review-003 option (a), the wire-proven shape), and `op/register`
+  is the client-side registration mechanism (alkcall ADR-022
+  amendment 2026-09-03 — review-003's bootstrap-op candidate). The
+  alkhttp-side consequences of review-003 Unit 1's candidates are
+  exactly what landed.
+- **One upstream remediation result lands here for free:** review
+  005 Unit 1 mechanized the accept-side pending-resolution arms into
+  `run_loop_single_stream` (the loop our WS channel-0 dispatch
+  runs). Pre-0.3, an accept-side nested-composing handler (a
+  `from_call` imported-op stub riding the WS connection) could
+  deadlock against the inline dispatch; post-0.3 the loop spawns
+  Once invocations and Sub pumps, so the G-01 hazard class is shut
+  for WS sessions by the bump alone — no alkhttp code involved.
+- **The remaining review-003 findings are all still open and now
+  unblocked** (WS-20/21/22/26 mechanical, WS-27 cleanup, WS-23
+  tests): the hook still discards the `ChannelManager`
+  (`src/websocket/upgrade.rs:255`), no channel ops / openables /
+  bootstrap discovery are registered, no deployment surface declares
+  openable ALPNs, and no live connection handle is retained. All
+  grep-verified at this tree.
+- **One new finding** (UP-01): alkcall 0.3 introduced the
+  `ALREADY_EXISTS` protocol code; this crate's gateway error table
+  has no arm for it and would map it to 500. Unreachable today
+  (only `op_register_handler` emits it), load-bearing the moment
+  Unit 2 wires `op/register`.
+- **One wiring contract Unit 2 must honor** (UP-02, recorded as a
+  decision point, not a defect): the `op/register` collision policy
+  binds the WS session's handler to the session fork.
+
+## Severity legend
+
+Same scale as review 003: **[major]** — a decided behavior is
+missing/wrong or a real hazard; **[minor]** — drift, convention
+violation, or doc/spec inconsistency with no correctness impact.
+
+---
+
+# Part A — What 0.3.0 resolves for this crate
+
+## WS-24 [was major, cross-crate] — dispatch-resolution mechanism → RESOLVED upstream
+
+alkcall ADR-047 §4 amendment #2 (2026-09-03) decides option (a): the
+`install_channel_zero` hook forks the deployment's base registry,
+registers the per-session ops on the fork (`ChannelOperations::
+register_on`, `ChannelCore::register_openable`,
+`install_bootstrap_discovery` — the last closed over the fork so
+`services/list` sees per-session openables, review-004 F-06), and
+dispatches channel 0 over the fork. The connection overlay remains
+the peer-announced-op landing zone and nested-invocation target, not
+the dispatch-resolution path. The reference shape is alkcall's
+`fork_registry_open_op_resolves_and_is_discoverable` gate
+(`alkcall/src/channels/client.rs:1994` — fork → `register_openable`
+→ `install_bootstrap_discovery` → `Dispatcher::new(fork, …)`), which
+is the shape Unit 2 must reproduce in `install_channel_zero`.
+
+Upstream also resolved the fork's discovery half (review-004 F-06):
+`install_bootstrap_discovery` registers `services/list`,
+`services/list-peers`, and `services/schema` closed over the fork
+itself, so per-session openables are discoverable without a
+re-registration walk. The bootstrap set is closed at four with
+`op/register` (alkcall ADR-022 amendment, `services/list-peers`
+added 2026-09-04 per review-005 G-05).
+
+**alkhttp consequence:** none to code yet; Unit 2 consumes it.
+
+## WS-25 [was major, cross-crate] — client-side op registration → RESOLVED upstream
+
+alkcall ADR-022 amendment (2026-09-03) decides the bootstrap-op
+mechanism: `op/register` (`registry::op_register` module) — a peer
+announces an op spec over channel 0; the hub's per-session handler
+rebuilds the spec, wraps it as a forwarding stub that nested-calls
+back over channel 0, and lands it in the connection overlay via
+`register_imported` (forced `Visibility::Internal`, provenance
+`FromCall`). The wire stays the six `call.*` envelope kinds.
+Collision policy (2026-09-04 sub-amendment, review-005 G-03): an
+announced op may replace other *announced* ops (`replace` governs)
+but never the serving side's own registrations — `ALREADY_EXISTS`
+regardless of `replace`; the handler gates on the serving registry
+first, then the overlay.
+
+The serving half it composes on (`Dispatcher::serve_single_stream`,
+opt-in via `ChannelClient::from_connection_with_serving`) landed
+with review-005 Unit 1's concurrency rework: Once invocations, Sub
+pumps, and sink response writers are spawned in both single-stream
+loops, so same-connection nested composition (the flagship
+hub-composes-peer-op flow) resolves concurrently instead of
+deadlocking to the 30 s sweeper (review-005 G-01, empirically
+verified upstream with a since-removed probe, productized as two
+5s-bounded gates).
+
+**alkhttp consequences:** Unit 2 wires `op/register` on WS sessions
+by registering `op_register_spec(acl)` +
+`op_register_handler(connection, fork)` on the per-session fork
+(the collision set is the fork — exactly what the session
+dispatches over, mirroring the upstream gates' production shape).
+Two additional facts from review 005 apply:
+
+- The channel-0 `Connection` must carry the peer identity for
+  `compose_root_env` to attach the connection's overlay (keyed by
+  `identity.id`). alkhttp's hook already does this
+  (`src/websocket/upgrade.rs:264` — identity propagated from the
+  upgrade's `AuthContext`); verified correct as-is.
+- The serving loop's concurrency means a hub handler that composes
+  an announced op resolves live (no sweeper); no alkhttp-side
+  scheduling work is needed.
+
+ADR-048's browser bidirectionality promise is now mechanized on the
+hub half; the browser half (announcing via WS framing) is a
+client-side concern with no alkhttp code beyond serving the op.
+The re-point edits in ADR-048/OQ-05 (committed `df86f89`) record
+this.
+
+## Fixed-upstream items this crate inherits
+
+- **Accept-side nested-composition hazard** (review-005 G-01's
+  second consequence): mechanized shut by the pending-resolution
+  arms now in `run_loop_single_stream` — inherited by the bump, no
+  alkhttp code.
+- **`resource_id_path` wire round-trip** (review-005 G-04): a
+  `from_call`-imported or announced op declaring ownership-scoped
+  resource extraction previously rebuilt with `resource_id: None`
+  (ACL checks ran without the resource id). Both round-trip halves
+  now carry the field. `from_wss`'s import path inherits the fix.
+- **Head-of-line blocking / interleaved directions**: a serving
+  side's outbound calls resolve while it serves a Sub (the
+  interleaved-directions gate upstream) — relevant to any future
+  serving-enabled `from_wss` (see Part C).
+
+---
+
+# Part B — Findings at this tree
+
+## UP-01 [minor] — The `ALREADY_EXISTS` protocol code is unmapped in the gateway error table
+
+**Verified:** YES. alkcall 0.3 added `CallError::already_exists`
+(`"ALREADY_EXISTS"`, non-retryable —
+`alkcall-0.3.0/src/protocol/wire.rs:124`) for the `op/register`
+collision policy; its own ADR-022 amendment records the code.
+alkhttp's status map (`src/gateway/error.rs:57-80`) has no arm for
+it: `NOT_FOUND`, `FORBIDDEN`, `INVALID_INPUT`,
+`INVALID_OPERATION_TYPE`, `TIMEOUT`, `INTERNAL`, and `HTTP_*`
+prefixed codes are mapped; anything else falls to `STATUS_INTERNAL`
+(500). alkcall's ADR-016 code list (six codes) was not amended for
+the seventh — upstream doc drift, noted for that repo.
+
+**Reachability today:** none — the only emitter is
+`op_register_handler`, which no alkhttp path invokes yet (grep
+verified; the gateway dispatches base-registry ops directly). It
+becomes reachable exactly when Unit 2 wires `op/register` on WS
+sessions, at which point a collision rejection would surface to
+wire callers as 500 with an `ALREADY_EXISTS` body instead of a
+conflict status. Because the collision path is also how a peer
+experiences the reconnect-reannounce contract (`replace` refused
+for serving-registry names), fidelity matters there.
+
+**Fix (Unit 2, ride-along):** map `ALREADY_EXISTS` → 409 Conflict in
+`call_error_to_http_status_with_identity` (plus the test arm),
+matching ADR-023's `HTTP_<status>` discipline for the new code.
+One-line change + test; no consumer breaks (the code is new).
+
+## UP-02 [minor, decision point] — `op/register`'s AccessControl on the WS surface needs a decided default
+
+Not a defect — a decision Unit 2 cannot avoid. alkcall's
+`op_register_spec(access_control)` takes the ACL as a parameter; its
+doc notes the default (`AccessControl::default()`) lets any peer
+register, and the op is `Mutation`/`External`-typed. This crate's
+convention (AGENTS.md §13, ADR-015): adapter-registered ops are
+`Visibility::Internal` by default — but `op/register` cannot be
+Internal on the wire surface (the *peer* must call it; Internal ops
+resolve spec-404 to wire callers). The analogous precedent here is
+the built-in gateway's `NoCap` posture for channel opens (review-001
+SRV-10: crate default is permissive, deployments gate via the
+`ChannelsPolicy` extension). The consistent shape: register
+`op/register` on the per-session fork with `AccessControl::default()`
+on the built-in surface and let an assembly-layer override pass a
+stricter ACL the same way `ChannelsPolicy`/`OpenableAlpn` flow —
+record the posture in the Unit-2 task and in ADR-048's amendment
+note when the wiring lands.
+
+## WS-20/WS-21/WS-22/WS-26 [major] — unchanged, now unblocked
+
+Re-verified at this tree; all four stand as review 003 found them:
+
+- `install_channel_zero` still discards the manager
+  (`src/websocket/upgrade.rs:255` — `_manager` unused); no
+  `ChannelCore`, no `ChannelOperations::register_on` (grep: zero
+  callers in this crate), no openables.
+- No `OpenableAlpn` surface exists (`src/server/adapter.rs` builders
+  unchanged: `with_decoy` / `with_extra_routes` /
+  `with_ws_max_sessions` / `with_ws_idle_timeout`).
+- No live `Arc<CallConnection>` handle is retained by the session
+  (the hook moves channel 0's connection into the spawned task);
+  `WsSessions` still tracks pump handles only.
+
+The fix shape from review 003 Unit 2 stands, now with the upstream
+mechanism named concretely: per-session `fork()` in the hook →
+`ChannelOperations::register_on(&fork)` → openables via
+`register_openable(spec, handler, &fork, auth)` →
+`install_bootstrap_discovery(&fork)` → `op_register_spec` +
+`op_register_handler(Arc<CallConnection>, fork)` → dispatch over the
+fork (`Dispatcher::run_loop_single_stream` unchanged as the loop).
+The `OpenableAlpn` list + `with_ws_openable_alpns` threading
+(RouterState → SessionState → hook, request-extension fallback
+mirroring `ChannelsPolicy`) is unchanged, as is the WS-26
+handle-retention design (eviction hooks into `WsSessions`).
+
+## WS-27 [minor] — unchanged; folds into the Unit-2 hook rework
+
+`adapter_install_channel_zero` (`src/websocket/upgrade.rs:290-295`)
+is still `#[allow(dead_code)]` with its only caller test-gated
+(`src/adapters/from_wss.rs` test server). The rework gives both
+callers a real hook; fold the cfg cleanup there.
+
+## WS-23 [minor] — unchanged; Unit 3's e2e gates gain two upstream-mirroring gates
+
+The browser-opened-channel e2e test is still absent (no test opens a
+data channel through a live WS session). Unit 3's five scenarios
+stand as written. Two gates to add, mirroring what review 005
+taught: `services/list` on a live WS session lists the per-session
+openable (the F-06 shape), and the `op/register` announce → hub
+nested-composition → routes-back-to-consumer flow (the stub path —
+the one review-005's original gate missed and G-02 called out).
+
+---
+
+# Part C — Noted, not findings
+
+- **Connect-side serving for `from_wss`**: alkcall 0.3's
+  `from_connection_with_serving` makes the WSS consumer half capable
+  of serving inbound calls (the from_wss session could answer
+  hub→consumer calls). `from_wss` keeps the pure-consumer default
+  (`from_connection`, resolution-only pump — verified unchanged);
+  opt-in serving is a deliberate later decision (it changes
+  `from_wss`'s security posture: a dial would then execute inbound
+  requests against a caller-chosen registry). Recorded here so the
+  opportunity is visible; no finding, no unit.
+- **`op/register` forced-Internal semantics vs AGENTS.md §13**: the
+  announced op lands `Internal`/`FromCall` on the hub — consistent
+  with the adapter-registered-ops-are-Internal convention (it is
+  composition material, not wire-callable from the hub side). No
+  conflict; noted to preempt an apparent tension.
+- **`list_operations` returns `External` only** — the
+  `to_openapi`/`to_mcp` projections and the gateway `/search`
+  continue to see exactly the External set; unchanged from 0.2.
+- **Upstream doc drift (alkcall-side, not ours)**: ADR-016's
+  protocol-code list ("six codes", last amended by ADR-021) does not
+  name `ALREADY_EXISTS`. The 2026-09-04 ADR-022 collision
+  sub-amendment records the code's use; ADR-016's list is stale.
+  Flagged for the next alkcall doc pass; UP-01 is the alkhttp-side
+  half of the same gap.
+
+---
+
+# Remediation plan (alkhttp side; supersedes review 003's plan where noted)
+
+Review 003's Units 1 and 4 are partially closed by upstream; the
+remaining work re-sequences as:
+
+## Unit 2 — alkhttp wiring (WS-20/21/22/26/27 + UP-01) — next
+
+1. `OpenableAlpn { spec, open_handler }` +
+   `HttpAdapter::with_ws_openable_alpns(...)`, threaded
+   `RouterState` → `SessionState` → `run_channels_session` → hook,
+   with a request-extension fallback (mirror `ChannelsPolicy`).
+2. Hook rework per the upstream reference shape: fork → channel ops
+   → openables → bootstrap discovery → `op/register` (ACL per
+   UP-02's decided posture) → dispatch over the fork; retain the
+   `Arc<CallConnection>` in a `WsSessions`-evicted registry (WS-26);
+   `adapter_install_channel_zero` cfg cleanup (WS-27).
+3. UP-01: `ALREADY_EXISTS` → 409 in the gateway error map + test.
+
+Gate: a live WS session lists its openable in `services/list`; an
+open op on channel 0 returns a `channel_id`; `op/register` collides
+loudly against fork-registered names (409 over the WS path).
+
+## Unit 3 — e2e tests (WS-23) — after Unit 2
+
+Review 003's five scenarios stand, plus the two upstream-mirroring
+gates named in Part B (openable discoverable on the live session;
+announce → nested composition → routes back).
+
+## Unit 4 — spec reconciliation (docs) — after Unit 2
+
+Review 003's list stands (OQ-05 resolution, ADR-067/ADR-048 notes,
+websocket.md status block, idle-knob deployment note), with the
+`op/register` ACL posture recorded per UP-02.
+
+---
+
+## Verification log (this pass)
+
+- Bump verified empirically: `cargo check` (default + all-features)
+  surfaced exactly the two break classes; post-fix gates all green
+  (453 / 575 / clippy both / fmt / doc).
+- WS-24/WS-25 resolutions read in the published 0.3.0 crate source
+  (`registry::op_register` handler + collision gate;
+  `Dispatcher::serve_single_stream`; `install_bootstrap_discovery`;
+  `OperationRegistry::fork`) and in the upstream ADR amendment
+  texts; the reference fork shape read at
+  `alkcall/src/channels/client.rs:1988-2130`.
+- Review-003 residual claims re-verified at this tree: hook
+  discards manager (`upgrade.rs:255`), zero `register_on` callers,
+  no openable surface, no retained connection handle, WS-27 caller
+  census unchanged.
+- UP-01 verified: no `ALREADY_EXISTS` arm in
+  `src/gateway/error.rs:57-80`; emitter census in 0.3.0 = `op_register.rs`
+  only; fall-through confirmed to `STATUS_INTERNAL`.
+- UP-02 verified: `op_register_spec` signature and its
+  default-ACL doc note; AGENTS.md §13 vs wire-callable tension
+  resolved against the SRV-10 precedent (permissive crate default +
+  extension override).
+- Cross-repo state verified: alkcall review 005 resolved
+  (G-01..G-05 remediation log), 0.3.0 published with
+  `cargo semver-checks` 196 checks against 0.2.0 (changelog's
+  Changed section matches the two source-break classes this crate
+  experienced).