alkcall/docs/reviews/009-post-landing-audit-coverage-gaps.md

# Review 009 — Post-landing audit of the review-008 remediation (0.7.1 → 0.8.0)

## Status

Resolved — all seven findings landed in alkcall 0.8.0 (2026-09-18,
same working tree as the audit's inline fixes): C-1 (plain-bundle arm
pinned + the ADR-051 §5 coverage note), C-2 (`filtered` closure unit +
the empty-stash e2e gate), C-3 (the batch-form reserved key), C-4
(the `open_channel_with_reply` failure path), C-5 (both golden pins),
C-6 (the derivation edge shapes), C-7 (last-win pinned + the doc
sentence). Three errata on the as-filed text (marked per finding).
No wire or API changes were needed. Verified at resolution: 680
tests pass, clippy/fmt clean, wasm check clean.

Open — filed 2026-09-18 from the post-landing audit of the six commits
`82ddddf..50182d7` (review 008's three units). Scope: correctness
review of the full diff, test-coverage mapping, and the classic
implementation-issue sweep. Verified against HEAD at filing (0.8.0 +
the audit's inline hardening commit): 672 tests pass, clippy/fmt/doc
clean, wasm check clean.

The audit's code findings were fixed inline at filing time (see the
CHANGELOG [Unreleased] entry): the adopted-entry leak windows
(audit F-1/F-2 — `RelayPlan` drop guard), the empty explicit
`channel_open_alpn` (audit F-3), the `channels//sub` silent-stub
shape (audit N-4), and the reserved-key message/log-level nits
(audit N-1/N-2). ADR-051 §6 documents the closed post-adopt window.
What remains from the audit is the **test-coverage debt** below —
deliberately deferred to this review so the fix commit stayed small.

Findings continue the review numbering with prefix `C` (coverage).

## Scope

Test-coverage gaps in the 0.8.0 surface: the Unit 3 modules
(`src/channels/relay.rs`, `src/channels/hub_leg.rs`) and the Unit 1/2
diff surfaces (`src/channels/operations.rs`, `src/channels/client.rs`,
`src/client/from_call.rs`, `src/registry/discovery.rs`). No production
code changes are requested by this review unless a gap's write-up
says otherwise; each finding names the test(s) to add and what they
pin. None block a consumer — the e2e gates the review-008 plan pinned
are all landed and passing; these gaps are the paths the gates never
walk.

## C-1: `HubLegTemplate::install_hook` failure arms are loud-only-in-code

**Errata (2026-09-18, at resolution).** The as-filed route (a) —
"a duplicate name between a plain bundle and the generic channel ops"
— does not fail `OperationRegistry::register` (same-name registration
overwrites; the map is an insert). The honest failure mechanism used
is the registry's other fail-closed rule: an un-compilable
`input_schema` (`{"type": "object", "required": "not-an-array"}`,
the CF-003 shape). Resolved with route (c)'s documentation half for
the generic-ops/bootstrap-discovery arms: ADR-051 §5 now carries the
loud-install coverage note (two arms pinned, two arms
best-effort-loud — the crate-internal specs compile by construction,
so an injectable seam would test the seam, not the arm).

**Finding.** `src/channels/hub_leg.rs:232-275` — four install arms end
the leg with only a `tracing::warn!`: generic channel ops registration
failure, plain-bundle registration failure, relay-openable
registration failure, bootstrap-discovery install failure. ADR-051 §6
pins the posture as "loud, never a silent stub" and only the
relay-openable arm is test-pinned
(`hub_leg_tests.rs` `template_pub_typed_marked_spec_is_loud_at_install`).
The other three are near-unreachable (generic ops and bootstrap
discovery cannot realistically fail; plain bundles were already
registered on the producing side) — but the ADR's posture claim rests
on code that no test walks.

**Requested change.** Unit-test the three unpinned arms. The plain
routes: (a) a plain bundle that fails registration — e.g. a duplicate
name between a plain bundle and the generic channel ops — asserts the
install task ends before `run_loop_single_stream`; (b) the same shape
for a relay-openable failure is already pinned; (c) the
generic-ops/bootstrap-discovery arms need an injectable failure seam
if they are to be tested honestly — if that is disproportionate,
instead demote the claim: document in ADR-051 §5 that only the
relay-openable arm is test-pinned and the others are
best-effort-loud, so the ADR and the code say the same thing.

**Verification gates:**

1. The template's install task provably ends (channel 0 never
   dispatches) on each exercised failure arm.
2. The ADR-051 §5/§6 text matches what is actually pinned.

## C-2: `HubLegImports::filtered` / `only` have no test

**Errata (2026-09-18, at resolution).** The as-filed "no unit test"
overstated: `stash_filter_keeps_named_ops_only` (landed with Unit 3b)
already exercised `only` at filing time. The real gaps — the
`filtered` closure path and the empty-filter e2e shape — are what
landed (`stash_only_keeps_the_marked_direct_spec_and_drops_the_rest`,
`stash_filtered_closure_partitions_both_halves`,
`template_empty_only_stash_installs_generic_ops_and_discovery_only`).

**Finding.** `src/channels/hub_leg.rs:91-111` — the per-consumer
op-subset filter, the mechanism behind ADR-051 §4's "per-consumer ACL
differentiation is a composition consequence" note, has no unit test.
Trivial code, but it is exported pub API (`HubLegImports` is a crate
export) and the filter story is load-bearing for the hub/spoke family.

**Requested change.** Unit tests: `from_bundles` splits by marker;
`filtered`/`only` partition both halves; a `#[must_use]` misuse
compiles with a warning (already enforced by the attribute — just
exercise the two builders).

**Verification gates:**

1. `only(&["channels/tunnel/direct"])` keeps the marked direct spec,
   drops everything else, plain bundle untouched by the marked list.
2. An empty `only` stash installs a leg that serves only the generic
   ops + discovery (no re-exposed ops in `services/list`).

## C-3: Batch-form reserved reply key

**Finding.** `src/channels/operations.rs` — the reserved-key teardown
test (`with_reply_field("channel_id", …)`, `operations.rs:~2606`)
covers only the builder form. The wrapper's check inspects the merged
map, so `with_reply_fields(map)` smuggling `channel_id` is covered by
construction — but the batch path is the shape a hub's
`Establishment::with_reply_fields(relay_map)` will actually use, and
it is unasserted.

**Requested change.** One-line extension of the existing test (or a
sibling): the batch form fails with reason `handler_error`, channel
torn down, ledger decremented.

**Verification gates:**

1. `Establishment::default().with_reply_fields(json_map_containing_channel_id)`
   → `channel:open_failed` / `handler_error`; `count_for == 0` after.

## C-4: `open_channel_with_reply` wire failure path

**Finding.** `src/channels/client.rs:1654` — the new client API's e2e
test covers the success path only. The failure path (a
`channel:open_failed` resolving through `open_channel_with_reply`) is
covered only indirectly (wrapper-level test + the generic
`establisher_failure_resolves_typed_open_failed_on_the_client`).
Low risk — the parse path is shared with `open_channel` — but the new
pub API's error path has no direct pin.

**Requested change.** One e2e test against the existing
establisher-always-fails harness: `open_channel_with_reply` resolves
`Err(ChannelOpenError::CallFailed)` with the `channel:open_failed`
code and details intact.

**Verification gates:**

1. The full reply shape (`reason`, `message` in `details`) is
   assertable by the caller through the new API.

## C-5: Byte-identical claims are shape-pinned, not golden-pinned

**Finding.** Two "byte-identical" claims are pinned structurally
(key-set equality), not by literal:

- `run_open_wrapper_without_reply_fields_is_byte_identical_to_pre_amendment`
  (`operations.rs:2553`) asserts `v == json!({"channel_id":
  v["channel_id"]})` — pins the exact key set but builds the expected
  from the actual (a wrong-typed `channel_id` value would pass).
- The Unit-2 standard-shape wire payload
  (`spec_standard_shape_channel_open_stays_boolean_only`) pins the
  key *absence* of `channel_open_alpn` (the real pin, solid) but not
  the payload's full key set.

**Requested change.** Golden-pin both: assert serialized bytes or a
literal `json!` against the actual for the no-fields reply, and a
full-object comparison for the standard-shape payload. Cheap, and it
converts "structurally equal" into "these exact bytes" for the two
claims the ADRs advertise as wire-stable.

**Verification gates:**

1. The no-fields open reply equals `json!({"channel_id": <exact
   literal>})` — not a self-referential compare.
2. The standard-shape `services/schema` payload compares equal to a
   literal object including key order (serde_json default map).

## C-6: `derive_alpn_from_op_name` edge shapes unpinned

**Errata (2026-09-18, at resolution).** The as-filed expectation
`"channels/x/sub/extra"` → `Some("alk/x/sub")` mis-stated the actual
behavior: the last-segment strip yields `rest = "x/sub/extra"`, and
`x/sub` (multi-segment, non-`alk/*`) rides as a full ALPN per the
ALPNs-without-the-prefix rule — `Some("x/sub")`, the same rule the
existing 5-segment test (`vendor/service/run`) pins. The landed test
asserts the actual behavior with the behavior-change-vs-pre-amendment
annotation.

**Finding.** `src/client/from_call.rs:326-337` — the empty-segment
guard (`segment.is_empty()`) and the bare-no-slash name have no unit
test, and the 4-segment name behavior *changed* (pre-amendment:
`None`; now: `Some("x/sub")`) without any test noticing. The audit
closed the `channels//sub` serialization half (N-4); the derivation
function's own edge-shape unit tests remain unlanded.

**Requested change.** Unit tests over the derivation directly:
`"channels//sub"` → `None`; `"channels//direct"` → `None`;
`"channels"` → `None`; `"channels/x/sub/extra"` → `Some("alk/x/sub")`
(the deliberate strict-superset behavior, worth pinning as intended);
`"channels/alk/tty/sub"` → `Some("alk/tty")` (the verbatim case).

**Verification gates:**

1. All five shapes assert as above; the 4-segment case is annotated
   as a behavior change vs the pre-amendment derivation.

## C-7: Builder overwrite semantics unpinned

**Finding.** `src/channels/operations.rs:420-434` — two
`with_reply_field("same-key", …)` calls silently last-win; the batch
form extends (also last-win). Establisher-own concern, nit-level, but
it is pub-API behavior a consumer will rely on or be confused by.

**Requested change.** Either pin last-win with a test (and one doc
sentence on `with_reply_field`), or reject duplicates loudly at
build time. Prefer pinning: rejection adds a failure mode with no
consumer ask behind it.

**Verification gates:**

1. `.with_reply_field("k", v1).with_reply_field("k", v2)` →
   `reply_fields()["k"] == v2`.

## Non-goals (recorded to bound the review)

- **No wire-format or API changes** — everything above is tests plus
  at most doc text. The audit's code findings are already landed;
  they are not re-opened here.
- **No new ADRs** — C-1's documentation route (if chosen) edits
  ADR-051 §5/§6 text only.
- **alktunnels-side work** (the bind-first establisher, the listen-op
  spec) sequences after this review as planned; nothing here blocks
  it.

## References

- ADR-051 §6 (the post-adopt teardown bullet the drop guard landed;
  C-1 pins its assembly-arm siblings)
- ADR-047 amendment 3 (C-6's derivation shapes), ADR-049 amendment 3
  (C-3/C-7's reply projection), ADR-047 (C-5's wire-stability claims)
- CHANGELOG [Unreleased] — the audit's inline fixes this review
  complements
- Review 008 — the remediation whose surface this review audits