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