docs(review 002): pre-publish coverage, convention, and cleanup review
- One hard publishing blocker: missing README.md (R-01) - Three expect() calls in library code violate AGENTS.md §2 (R-02) - Unnecessary #[allow(dead_code)] on generate_request_id (R-03) - Seven files below 90% line coverage, clustered in post-Unit-2 paths: single-stream call mode (R-04), ChannelsAdapter::handle() (R-05), ChannelCore/open-op machinery (R-06), SendStream/RecvStream (R-07), make_sink_forwarding_handler (R-08), ChannelsSessionEnv (R-09), CallClient accessors (R-10) - Minor smells: too_many_arguments annotations (R-11), stub handlers (R-12) - Five-unit remediation plan, each independently shippable Verification: 483 tests pass, clippy/fmt/doc clean, 91.97% line coverage
This commit is contained in:
deepseek-v4-pro committed
1 parent
7cd8a57bc7
commit
deba5f7023
1 file changed
+591
@@ -0,0 +1,591 @@
|
|||||||
|
# Review 002 — Pre-Publish Coverage, Convention, and Cleanup Review
|
||||||
|
|
||||||
|
## Status
|
||||||
|
|
||||||
|
Verified, open for remediation.
|
||||||
|
|
||||||
|
## Scope
|
||||||
|
|
||||||
|
General review of the alkcall crate at tree `7cd8a57` (post-Units 1-10
|
||||||
|
remediation of Review 001). This review covers:
|
||||||
|
|
||||||
|
- **Publishing readiness** — `cargo publish --dry-run` blockers, Cargo.toml
|
||||||
|
metadata, license files, missing README.
|
||||||
|
- **Code coverage** — `cargo llvm-cov` line/region/function coverage
|
||||||
|
across all 27 source files; identification of weak spots in core
|
||||||
|
protocol paths.
|
||||||
|
- **Convention compliance** — `expect`/`unwrap` in library code,
|
||||||
|
`#[allow(dead_code)]` annotations, `#[allow(clippy::...)]` attributes,
|
||||||
|
producer/consumer naming.
|
||||||
|
- **Code smells** — dead code, stub handlers, unnecessary annotations,
|
||||||
|
large functions, public API surface hygiene.
|
||||||
|
|
||||||
|
Every finding below was **re-verified directly in the source** during
|
||||||
|
this pass (2026-08-14). Each finding carries: the convention or ADR it
|
||||||
|
drifts from (if any), a severity, the exact `file:line` reference(s),
|
||||||
|
the verification status, and the unit-of-work bucket it belongs to.
|
||||||
|
|
||||||
|
## Baseline verification (this pass)
|
||||||
|
|
||||||
|
```
|
||||||
|
cargo test → 483 passed, 0 failed
|
||||||
|
cargo clippy --all-targets -- -D warnings → clean
|
||||||
|
cargo fmt --check → clean
|
||||||
|
cargo doc --no-deps → clean (0 warnings)
|
||||||
|
cargo publish --dry-run --allow-dirty → FAILS — missing README.md
|
||||||
|
cargo llvm-cov --summary-only → 91.97% lines, 93.17% regions
|
||||||
|
```
|
||||||
|
|
||||||
|
The suite is green and the architecture is solid. The review-001
|
||||||
|
critical/major bugs (P-01 through C-08, C-12 through C-18) are all
|
||||||
|
fixed and verified. The remaining issues are coverage gaps in
|
||||||
|
post-Unit-2 code paths, convention violations, and one hard `cargo
|
||||||
|
publish` blocker.
|
||||||
|
|
||||||
|
## Verdict
|
||||||
|
|
||||||
|
- **Publishing readiness: one hard blocker** (missing README.md). All
|
||||||
|
other `cargo publish` requirements are met: license files present
|
||||||
|
(MIT + Apache-2.0), Cargo.toml has correct metadata (version, edition,
|
||||||
|
rust-version, license, description, repository, keywords, categories,
|
||||||
|
exclude list). No `unsafe` code, no `todo!()`/`unimplemented!()`
|
||||||
|
panicking macros, no `println!`/`eprintln!` in library code, no
|
||||||
|
hardcoded secrets.
|
||||||
|
- **Coverage: 91.97% overall is solid**, but seven files fall below 90%
|
||||||
|
line coverage. The gaps cluster in three areas: the single-stream call
|
||||||
|
mode (channel 0 multiplexed framing, added in Unit 2), the
|
||||||
|
`ChannelCore`/open-op machinery (added in Unit 3), and the
|
||||||
|
`SendStream`/`RecvStream` newtypes (vendored core types, ADR-092).
|
||||||
|
These are all post-Unit-2 code paths — the pre-Unit-2 surface is
|
||||||
|
well-covered.
|
||||||
|
- **Conventions: three `expect` calls in library code** violate
|
||||||
|
AGENTS.md §2. All are in crate-private or guarded contexts and won't
|
||||||
|
panic in practice, but the convention is explicit. One unnecessary
|
||||||
|
`#[allow(dead_code)]` annotation. No other convention violations
|
||||||
|
found.
|
||||||
|
- **Code smells: minor.** Stub handlers are intentional and documented
|
||||||
|
with OQ references. Two `#[allow(clippy::too_many_arguments)]`
|
||||||
|
annotations on functions with 9 parameters — acceptable for
|
||||||
|
builder-alternative constructors and well-structured wrappers.
|
||||||
|
|
||||||
|
## Severity legend
|
||||||
|
|
||||||
|
- **[critical]** — `cargo publish` fails; or a decided spec invariant is
|
||||||
|
violated in a way that corrupts data or causes silent permanent state
|
||||||
|
damage.
|
||||||
|
- **[major]** — a core protocol path has no test coverage; or a public
|
||||||
|
API type has no coverage; or a convention violation that would
|
||||||
|
surprise downstream consumers.
|
||||||
|
- **[minor]** — drift, convention violation, dead code, unnecessary
|
||||||
|
annotation, or a doc/spec inconsistency with no correctness impact.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
# Part A — Publishing Blockers
|
||||||
|
|
||||||
|
## R-01 [critical] — Missing README.md; `cargo publish --dry-run` fails
|
||||||
|
|
||||||
|
**Convention drift:** Cargo.toml `readme` field.
|
||||||
|
**Verified:** YES. `Cargo.toml:9` specifies `readme = "README.md"`. No
|
||||||
|
`README.md` exists at the repository root. `cargo publish --dry-run
|
||||||
|
--allow-dirty` fails with:
|
||||||
|
|
||||||
|
```
|
||||||
|
error: readme `README.md` does not appear to exist (relative to
|
||||||
|
`/workspace/@alkdev/alkcall`).
|
||||||
|
```
|
||||||
|
|
||||||
|
License files (`LICENSE-MIT`, `LICENSE-APACHE`) are present. All other
|
||||||
|
Cargo.toml metadata is correct: `version = "0.1.0"`, `edition = "2021"`,
|
||||||
|
`rust-version = "1.85"`, `license = "MIT OR Apache-2.0"`, `description`,
|
||||||
|
`repository`, `keywords`, `categories`, `exclude` list.
|
||||||
|
|
||||||
|
**Fix:** either create a `README.md` at the repo root, or change the
|
||||||
|
`readme` field to point to an existing doc file (e.g.
|
||||||
|
`docs/architecture/README.md`), or remove the `readme` field. The user
|
||||||
|
has stated they write the README as the second-to-last step before
|
||||||
|
publishing, so the pragmatic fix is to remove the `readme` field from
|
||||||
|
Cargo.toml for now and restore it when the README is written.
|
||||||
|
|
||||||
|
**Acceptance gate:** `cargo publish --dry-run --allow-dirty` succeeds.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
# Part B — Convention Violations
|
||||||
|
|
||||||
|
## R-02 [minor] — `expect` in library code (3 locations)
|
||||||
|
|
||||||
|
**Convention drift:** AGENTS.md §2 ("No `unwrap()` or `expect()` outside
|
||||||
|
tests. For poisoned `RwLock`/`Mutex`, use `unwrap_or_else(|e|
|
||||||
|
e.into_inner())`").
|
||||||
|
**Verified:** YES. Three `expect` calls in non-test, non-test-support
|
||||||
|
library code:
|
||||||
|
|
||||||
|
| File:Line | Code | Risk |
|
||||||
|
|-----------|------|------|
|
||||||
|
| `channels/operations.rs:415` | `channel_open.expect("checked above")` | Guarded by `is_none()` check on line 408; won't panic |
|
||||||
|
| `core/types.rs:436` | `self.stream.lock().expect("stream mutex poisoned")` | `StreamBidiStreamSource` is crate-private; lock held for microseconds |
|
||||||
|
| `core/types.rs:456` | `self.stream.lock().expect("stream mutex poisoned")` | Same as above |
|
||||||
|
|
||||||
|
Additionally, `protocol/test_support.rs:92,99,110` has three
|
||||||
|
`expect("source mutex poisoned")` calls — these are in test-support code
|
||||||
|
(`SingleStreamSource`), which is a gray area (not `#[cfg(test)]` but
|
||||||
|
only used by tests). The `StaticIdentityProvider` in
|
||||||
|
`adapter.rs:199,211` and `dispatch.rs:1005` uses
|
||||||
|
`std::sync::Mutex::lock().unwrap()` — these are inside `#[cfg(test)]`
|
||||||
|
blocks and are fine.
|
||||||
|
|
||||||
|
**Fix:** replace the three `expect` calls with
|
||||||
|
`unwrap_or_else(|e| e.into_inner())` per the convention. For
|
||||||
|
`operations.rs:415`, use `channel_open.as_ref().ok_or_else(||
|
||||||
|
...)?.alpn.to_string()` to avoid the expect entirely.
|
||||||
|
|
||||||
|
**Acceptance gate:** `grep -rn '\.expect(' src/ --include='*.rs' | grep
|
||||||
|
-v '#\[cfg(test)\]' | grep -v 'mod tests' | grep -v '#\[test\]' | grep
|
||||||
|
-v test_support` returns no matches.
|
||||||
|
|
||||||
|
## R-03 [minor] — Unnecessary `#[allow(dead_code)]` on `generate_request_id`
|
||||||
|
|
||||||
|
**Convention drift:** dead code annotation on a function that is
|
||||||
|
actually used.
|
||||||
|
**Verified:** YES. `registry/context.rs:147` has
|
||||||
|
`#[allow(dead_code)]` on `pub(crate) fn generate_request_id()`. The
|
||||||
|
function is called from `protocol/connection.rs` and
|
||||||
|
`protocol/dispatch.rs` — it is not dead code. The annotation is
|
||||||
|
unnecessary and misleading.
|
||||||
|
|
||||||
|
Additionally, `protocol/adapter.rs:236` has `#[allow(dead_code)]` on
|
||||||
|
`fn internal_spec()` — this is a test helper that is genuinely unused
|
||||||
|
in the current test suite. It can be removed or kept with the
|
||||||
|
annotation; either is fine.
|
||||||
|
|
||||||
|
**Fix:** remove the `#[allow(dead_code)]` from `context.rs:147`.
|
||||||
|
|
||||||
|
**Acceptance gate:** `grep '#\[allow(dead_code)\]' src/registry/context.rs`
|
||||||
|
returns no matches.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
# Part C — Coverage Gaps
|
||||||
|
|
||||||
|
## R-04 [major] — Single-stream call mode untested (connection.rs)
|
||||||
|
|
||||||
|
**ADR drift:** ADR-036 amendment (single-stream call mode for channel 0
|
||||||
|
multiplexed framing, added in Unit 2).
|
||||||
|
**Verified:** YES. `src/protocol/connection.rs` has 80.62% line coverage
|
||||||
|
(890/1104 lines). The uncovered regions are concentrated in the
|
||||||
|
single-stream call mode:
|
||||||
|
|
||||||
|
| Function | Lines | Status |
|
||||||
|
|----------|-------|--------|
|
||||||
|
| `CallConnection::new_single_stream` | 121-132 | Untested |
|
||||||
|
| `CallConnection::is_single_stream` | 137-139 | Untested |
|
||||||
|
| `CallConnection::single_stream_writer` | 143-145 | Untested |
|
||||||
|
| `CallConnection::call_single_stream` | 262-291 | Untested |
|
||||||
|
| `CallConnection::subscribe_single_stream` | 375-396 | Untested |
|
||||||
|
| `CallConnection::publish_single_stream` | 522-567 | Untested |
|
||||||
|
| `read_single_stream_until_closed` | 654-659 | Untested |
|
||||||
|
| `split_single_stream` | 669-676 | Untested |
|
||||||
|
| `SharedFrameWriter` (all methods) | 51-59 | Untested directly |
|
||||||
|
|
||||||
|
The stream-per-request path (`call_with_payload`, `subscribe_with_payload`,
|
||||||
|
`publish_with_payload` on multi-stream transports) is well-tested via
|
||||||
|
the publish end-to-end tests. The single-stream path is exercised
|
||||||
|
indirectly through the channel-0 end-to-end tests in
|
||||||
|
`channels/client.rs`, but `connection.rs` itself has no direct
|
||||||
|
single-stream tests. This is the largest coverage gap in a core protocol
|
||||||
|
file.
|
||||||
|
|
||||||
|
**Fix:** add unit tests for the single-stream call mode: construct a
|
||||||
|
`CallConnection` with `new_single_stream`, exercise `call`, `subscribe`,
|
||||||
|
`publish`, and `abort` on the single-stream path, and assert correct
|
||||||
|
behavior. The channel-0 e2e tests in `client.rs` provide integration
|
||||||
|
coverage; these would provide unit-level coverage of the
|
||||||
|
`CallConnection` single-stream API directly.
|
||||||
|
|
||||||
|
**Acceptance gate:** `connection.rs` line coverage ≥ 90%.
|
||||||
|
|
||||||
|
## R-05 [major] — `ChannelsAdapter::handle()` untested (adapter.rs)
|
||||||
|
|
||||||
|
**ADR drift:** ADR-034, ADR-036 (channels adapter accept-side entry
|
||||||
|
point).
|
||||||
|
**Verified:** YES. `src/channels/adapter.rs` has 82.13% line coverage
|
||||||
|
(170/207 lines). The `ProtocolHandler for ChannelsAdapter` impl's
|
||||||
|
`handle()` method (lines 208-253) is completely untested. This is the
|
||||||
|
main accept-side entry point for a channels connection — it installs
|
||||||
|
channel 0, spawns the mux runner, and runs the demux loop. The demux
|
||||||
|
loop itself (`run_demux_loop_for_client`) is well-tested with two good
|
||||||
|
tests (oversized chunk resync and backpressure), but the full
|
||||||
|
`handle()` flow is never exercised.
|
||||||
|
|
||||||
|
**Fix:** add a test that constructs a `ChannelsAdapter`, calls
|
||||||
|
`handle()` with a duplex connection, and asserts that channel 0 is
|
||||||
|
installed and the demux loop processes frames. This is the accept-side
|
||||||
|
counterpart to the channel-0 e2e tests in `client.rs` (which test the
|
||||||
|
connect side).
|
||||||
|
|
||||||
|
**Acceptance gate:** `adapter.rs` line coverage ≥ 90%.
|
||||||
|
|
||||||
|
## R-06 [major] — `ChannelCore` / open-op machinery unit paths untested (operations.rs)
|
||||||
|
|
||||||
|
**ADR drift:** ADR-047 §3 (per-ALPN open-op registration and
|
||||||
|
invocation).
|
||||||
|
**Verified:** YES. `src/channels/operations.rs` has 73.47% line coverage
|
||||||
|
(407/554 lines). The uncovered regions are concentrated in the
|
||||||
|
`ChannelCore` and open-op machinery added in Unit 3:
|
||||||
|
|
||||||
|
| Function | Lines | Status |
|
||||||
|
|----------|-------|--------|
|
||||||
|
| `ChannelCore::new` | 343-345 | Untested |
|
||||||
|
| `ChannelCore::manager` | 347-349 | Untested |
|
||||||
|
| `ChannelCore::policy` | 351-353 | Untested |
|
||||||
|
| `ChannelCore::check_open` | 358-360 | Untested |
|
||||||
|
| `ChannelCore::on_close` | 364-366 | Untested |
|
||||||
|
| `ChannelCore::register_openable` | 399-449 | Untested directly |
|
||||||
|
| `opener_identity_from_context` | 459-465 | Untested |
|
||||||
|
| `run_open_wrapper` | 480-552 | Untested directly |
|
||||||
|
| `map_channel_error_to_call_error` | 558-575 | Untested |
|
||||||
|
| `make_open_handler_once` | 581-612 | Untested directly |
|
||||||
|
| `make_open_handler_stream` | 622-653 | Untested directly |
|
||||||
|
| `make_open_handler_sink` | 672-695 | Untested (stub) |
|
||||||
|
| `make_control_handler` success path | 254-267 | Untested |
|
||||||
|
| `make_close_handler` success path | 205-217 | Untested |
|
||||||
|
| `channel_control_spec` | 122-140 | Untested |
|
||||||
|
|
||||||
|
The `register_openable` end-to-end test in `channels/client.rs`
|
||||||
|
exercises this machinery indirectly through channel 0, but the
|
||||||
|
unit-level coverage of the individual functions is missing. The
|
||||||
|
`ChannelOperations::new` constructor (line 52-54) is also untested
|
||||||
|
(only `with_default_policy` is used in tests).
|
||||||
|
|
||||||
|
**Fix:** add unit tests for `ChannelCore` methods (`new`, `manager`,
|
||||||
|
`policy`, `check_open`, `on_close`), `opener_identity_from_context`,
|
||||||
|
`map_channel_error_to_call_error`, and the `make_open_handler_*`
|
||||||
|
functions. Test the `make_close_handler` and `make_control_handler`
|
||||||
|
success paths (channel exists, handler invoked). Test
|
||||||
|
`channel_control_spec`.
|
||||||
|
|
||||||
|
**Acceptance gate:** `operations.rs` line coverage ≥ 85%.
|
||||||
|
|
||||||
|
## R-07 [major] — `SendStream`/`RecvStream` newtypes untested (types.rs)
|
||||||
|
|
||||||
|
**ADR drift:** ADR-092 (`BiStream` as the handler leaf; `SendStream` and
|
||||||
|
`RecvStream` are the public read/write half types).
|
||||||
|
**Verified:** YES. `src/core/types.rs` has 85.48% line coverage
|
||||||
|
(424/496 lines). The `SendStream` and `RecvStream` newtypes (lines
|
||||||
|
321-385) have zero test coverage:
|
||||||
|
|
||||||
|
| Item | Lines | Status |
|
||||||
|
|------|-------|--------|
|
||||||
|
| `SendStream::from_stream` | 334-338 | Untested |
|
||||||
|
| `SendStream` `AsyncWrite` impl | 353-375 | Untested |
|
||||||
|
| `RecvStream::from_stream` | 346-350 | Untested |
|
||||||
|
| `RecvStream` `AsyncRead` impl | 377-385 | Untested |
|
||||||
|
|
||||||
|
These are public API types that downstream crates will use. They are
|
||||||
|
thin newtypes over `tokio::io` halves, but their constructors and trait
|
||||||
|
impls should have basic smoke tests.
|
||||||
|
|
||||||
|
Additionally, `BiStream::from_joined` (lines 261-269) — the public
|
||||||
|
constructor for joining independent read/write halves — is untested.
|
||||||
|
Downstream crates (channels reassembly) use it, but it has no direct
|
||||||
|
test coverage.
|
||||||
|
|
||||||
|
**Fix:** add basic smoke tests for `SendStream`/`RecvStream` construction
|
||||||
|
and read/write round-trips, and a test for `BiStream::from_joined`.
|
||||||
|
|
||||||
|
**Acceptance gate:** `types.rs` line coverage ≥ 90%.
|
||||||
|
|
||||||
|
## R-08 [major] — `make_sink_forwarding_handler` untested (from_call.rs)
|
||||||
|
|
||||||
|
**ADR drift:** ADR-046 §9 (sink forwarding for Pub ops in `from_call`).
|
||||||
|
**Verified:** YES. `src/client/from_call.rs` has 87.60% line coverage
|
||||||
|
(742/847 lines). The `make_sink_forwarding_handler` function (lines
|
||||||
|
458-477) has no test coverage. The `Query`/`Mutation` forwarding handler
|
||||||
|
(`make_forwarding_handler`) and the `Sub` forwarding handler
|
||||||
|
(`make_streaming_forwarding_handler`) are well-tested. The Pub
|
||||||
|
forwarding path is the only `HandlerKind` without a forwarding test.
|
||||||
|
|
||||||
|
Additionally, the `from_call` happy path (lines 82-92) is untested —
|
||||||
|
only the error path (mock connection → `DiscoveryFailed`) is tested.
|
||||||
|
The happy path requires a live transport with `services/list` +
|
||||||
|
`services/schema` handlers registered, which is an integration test.
|
||||||
|
|
||||||
|
**Fix:** add a test for `make_sink_forwarding_handler` that verifies it
|
||||||
|
forwards chunks and terminates on `Err` (matching the `SinkHandler`
|
||||||
|
contract). The `from_call` happy path can be deferred to an integration
|
||||||
|
test suite.
|
||||||
|
|
||||||
|
**Acceptance gate:** `from_call.rs` line coverage ≥ 90%.
|
||||||
|
|
||||||
|
## R-09 [minor] — `ChannelsSessionEnv` delegation methods untested (env.rs)
|
||||||
|
|
||||||
|
**ADR drift:** ADR-047 §4 (per-connection `ChannelManager` resolution).
|
||||||
|
**Verified:** YES. `src/channels/env.rs` has 51.28% line coverage
|
||||||
|
(20/39 lines). The `OperationEnv for ChannelsSessionEnv` impl's
|
||||||
|
delegation methods (`invoke_with_policy`, `contains`, `peer_ids`,
|
||||||
|
`peer_contains`, `peer_operations`, `invoke_peer` — lines 57-98) are
|
||||||
|
completely untested. The only tested method is `channel_manager()`.
|
||||||
|
|
||||||
|
The module doc says this type is "retained for future per-connection
|
||||||
|
routing" and "not on the open-op path," so the low coverage is
|
||||||
|
intentional. The `ChannelOperationEnv` trait and `ChannelsSessionEnv`
|
||||||
|
struct exist as integration points for future work.
|
||||||
|
|
||||||
|
**Fix:** low priority. Either add basic delegation tests (construct a
|
||||||
|
`ChannelsSessionEnv` with a mock base `OperationEnv` and verify each
|
||||||
|
method delegates correctly) or document the intentional coverage gap
|
||||||
|
with an OQ reference. The file is small (39 lines) and the risk is low.
|
||||||
|
|
||||||
|
**Acceptance gate:** `env.rs` line coverage ≥ 80%, or an OQ filed
|
||||||
|
documenting the intentional gap.
|
||||||
|
|
||||||
|
## R-10 [minor] — `CallClient` accessors untested (call_client.rs)
|
||||||
|
|
||||||
|
**Verified:** YES. `src/client/call_client.rs` has 87.10% line coverage
|
||||||
|
(108/124 lines). Two trivial accessors are untested:
|
||||||
|
`CallClient::registry` (lines 50-52) and
|
||||||
|
`CallClient::identity_provider` (lines 54-56). These are one-line
|
||||||
|
getters that return `Arc` clones.
|
||||||
|
|
||||||
|
**Fix:** add one-line assertions that the accessors return the expected
|
||||||
|
values after construction.
|
||||||
|
|
||||||
|
**Acceptance gate:** `call_client.rs` line coverage ≥ 90%.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
# Part D — Code Smells
|
||||||
|
|
||||||
|
## R-11 [minor] — `#[allow(clippy::too_many_arguments)]` on large functions
|
||||||
|
|
||||||
|
**Verified:** YES. Three locations:
|
||||||
|
|
||||||
|
| File:Line | Function | Params | Notes |
|
||||||
|
|-----------|----------|--------|-------|
|
||||||
|
| `registry/spec.rs:210` | `OperationSpec::new` | 9 | Builder-alternative constructor; acceptable |
|
||||||
|
| `channels/operations.rs:479` | `run_open_wrapper` | 9 | Well-structured wrapper; could be refactored but not urgent |
|
||||||
|
| `registry/registration.rs:288` | `invoke_sink` | — | `#[allow(clippy::result_large_err)]` — acceptable |
|
||||||
|
|
||||||
|
None of these are correctness issues. The `run_open_wrapper` function
|
||||||
|
(72 lines) is the largest and could benefit from being split into
|
||||||
|
smaller helpers, but it is well-structured with clear error branches.
|
||||||
|
This is a refactoring opportunity, not a bug.
|
||||||
|
|
||||||
|
**Fix:** none required for v0.1.0. Consider refactoring `run_open_wrapper`
|
||||||
|
in a follow-up.
|
||||||
|
|
||||||
|
## R-12 [minor] — Stub handlers return "not implemented" errors
|
||||||
|
|
||||||
|
**Verified:** YES. Three handlers in `channels/operations.rs` are stubs
|
||||||
|
that return explicit error envelopes:
|
||||||
|
|
||||||
|
| Handler | Lines | Error code | OQ |
|
||||||
|
|---------|-------|------------|-----|
|
||||||
|
| `make_control_handler` | 260-267 | `channel:control_not_implemented` | OQ-39 |
|
||||||
|
| `make_resources_subscribe_handler` | 284-293 | `channel:resources_subscribe_not_implemented` | OQ-40 |
|
||||||
|
| `make_open_handler_sink` | 679-694 | `channel:sink_open_not_implemented` | — |
|
||||||
|
|
||||||
|
These are intentional — the features are decided but deferred, and the
|
||||||
|
stubs return honest errors rather than fake success. This is the correct
|
||||||
|
pattern. Not a smell, but worth noting for the feature-completeness
|
||||||
|
assessment.
|
||||||
|
|
||||||
|
**Fix:** none required. These will be implemented when the corresponding
|
||||||
|
OQs are resolved.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
# Cross-cutting: what is solid (verified)
|
||||||
|
|
||||||
|
These were checked and are correct — listed so the remediation plan can
|
||||||
|
focus on what actually needs work:
|
||||||
|
|
||||||
|
- **Wire formats:** `EventEnvelope` shape and the five+one event types
|
||||||
|
match spec exactly; the channels 8-byte chunk header
|
||||||
|
`[channel_id:u32 BE][length:u32 BE]` matches spec exactly. No
|
||||||
|
`stream_type` anywhere in the channels layer. ✓
|
||||||
|
- **`HandlerKind::Sink` validation:** Pub↔Sink both directions, all
|
||||||
|
builder methods tested. ✓
|
||||||
|
- **Wire `op_type` strings:** `"query"`/`"mutation"`/`"sub"`/`"pub"`
|
||||||
|
emit and parse correctly. ✓
|
||||||
|
- **`call.published` payload:** `{ "input": <chunk> }` shape, correlated
|
||||||
|
by envelope `id`. ✓
|
||||||
|
- **Gap F (`channel_open` marker):** `spec_to_json` emits,
|
||||||
|
`rebuild_spec_for` parses, well tested. ✓
|
||||||
|
- **Per-identity cap default 256:** tested. ✓
|
||||||
|
- **Channel 0 pre-installed, skips ledger:** correct bookkeeping. ✓
|
||||||
|
- **Odd/even ID split:** connect side starts at 1, accept side at 2 —
|
||||||
|
no collisions. ✓
|
||||||
|
- **Layering held:** no channels types leaked into call crate,
|
||||||
|
`OperationEnv` stayed a trait, auth stays on the one `AccessControl`
|
||||||
|
path. ✓
|
||||||
|
- **No `unsafe` code** anywhere in the codebase. ✓
|
||||||
|
- **No `todo!()` / `unimplemented!()` / `unreachable!()`** panicking
|
||||||
|
macros in library code. ✓
|
||||||
|
- **No `println!` / `eprintln!`** in library code. ✓
|
||||||
|
- **No hardcoded secrets, keys, or tokens.** ✓
|
||||||
|
- **All review-001 critical/major bugs fixed:** Units 1-10 landed and
|
||||||
|
verified (commits `8066d08` through `7cd8a57`). ✓
|
||||||
|
- **483 tests, 0 failures.** ✓
|
||||||
|
- **Clippy, fmt, doc: all clean.** ✓
|
||||||
|
- **License files present:** `LICENSE-MIT` and `LICENSE-APACHE`. ✓
|
||||||
|
- **Cargo.toml metadata:** version, edition, rust-version, license,
|
||||||
|
description, repository, keywords, categories, exclude list — all
|
||||||
|
correct. ✓
|
||||||
|
- **Dependencies:** all from crates.io with specific versions. `alktype`
|
||||||
|
is a declared dependency and is actively used (JSON schema validation
|
||||||
|
via `alktype::validation::build_validator`). ✓
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
# Remediation plan
|
||||||
|
|
||||||
|
The plan is split into **five units of work**, ordered by dependency and
|
||||||
|
risk. Each unit is an independently shippable commit with its own
|
||||||
|
acceptance gate. Units are sized for a single session each.
|
||||||
|
|
||||||
|
## Unit 1 — Publishing blocker + convention fixes (R-01, R-02, R-03)
|
||||||
|
|
||||||
|
**Goal:** `cargo publish --dry-run` succeeds; no `expect` in library
|
||||||
|
code; no unnecessary `#[allow(dead_code)]`.
|
||||||
|
**Files:** `Cargo.toml`, `src/channels/operations.rs`,
|
||||||
|
`src/core/types.rs`, `src/registry/context.rs`.
|
||||||
|
**Scope:**
|
||||||
|
- **R-01:** remove the `readme = "README.md"` field from `Cargo.toml`
|
||||||
|
(the README will be written as the second-to-last step before
|
||||||
|
publishing; the field can be restored then).
|
||||||
|
- **R-02:** replace the three `expect` calls in library code:
|
||||||
|
- `operations.rs:415`: use `channel_open.as_ref().ok_or_else(||
|
||||||
|
...)?.alpn.to_string()`.
|
||||||
|
- `types.rs:436,456`: use `unwrap_or_else(|e| e.into_inner())`.
|
||||||
|
- **R-03:** remove `#[allow(dead_code)]` from `context.rs:147`.
|
||||||
|
**Acceptance gate:** `cargo publish --dry-run --allow-dirty` succeeds;
|
||||||
|
`grep -rn '\.expect(' src/ --include='*.rs' | grep -v '#\[cfg(test)\]'
|
||||||
|
| grep -v 'mod tests' | grep -v '#\[test\]' | grep -v test_support`
|
||||||
|
returns no matches; `cargo test` green; clippy/fmt/doc clean.
|
||||||
|
|
||||||
|
## Unit 2 — Single-stream call mode tests (R-04)
|
||||||
|
|
||||||
|
**Goal:** `connection.rs` line coverage ≥ 90%.
|
||||||
|
**Files:** `src/protocol/connection.rs`.
|
||||||
|
**Scope:**
|
||||||
|
- Add unit tests for `CallConnection::new_single_stream`,
|
||||||
|
`is_single_stream`, `single_stream_writer`.
|
||||||
|
- Add unit tests for `call_single_stream`, `subscribe_single_stream`,
|
||||||
|
`publish_single_stream` — construct a single-stream `CallConnection`
|
||||||
|
over a duplex pair, exercise each call type, assert correct
|
||||||
|
request/response correlation.
|
||||||
|
- Add unit tests for `SharedFrameWriter` (write_frame, concurrent
|
||||||
|
writes).
|
||||||
|
- Add unit tests for `read_single_stream_until_closed` and
|
||||||
|
`split_single_stream`.
|
||||||
|
- The channel-0 e2e tests in `channels/client.rs` already provide
|
||||||
|
integration coverage; these tests provide unit-level coverage of the
|
||||||
|
`CallConnection` single-stream API directly.
|
||||||
|
**Acceptance gate:** `connection.rs` line coverage ≥ 90%; `cargo test`
|
||||||
|
green; clippy/fmt/doc clean.
|
||||||
|
|
||||||
|
## Unit 3 — Channels adapter + operations coverage (R-05, R-06)
|
||||||
|
|
||||||
|
**Goal:** `adapter.rs` line coverage ≥ 90%; `operations.rs` line coverage
|
||||||
|
≥ 85%.
|
||||||
|
**Files:** `src/channels/adapter.rs`, `src/channels/operations.rs`.
|
||||||
|
**Scope:**
|
||||||
|
- **R-05:** add a test for `ChannelsAdapter::handle()` — construct a
|
||||||
|
`ChannelsAdapter`, call `handle()` with a duplex connection, assert
|
||||||
|
channel 0 is installed and the demux loop processes frames. This is
|
||||||
|
the accept-side counterpart to the channel-0 e2e tests in `client.rs`.
|
||||||
|
- **R-06:** add unit tests for `ChannelCore` methods (`new`, `manager`,
|
||||||
|
`policy`, `check_open`, `on_close`).
|
||||||
|
- Add unit tests for `opener_identity_from_context`,
|
||||||
|
`map_channel_error_to_call_error`.
|
||||||
|
- Add unit tests for `make_open_handler_once` and
|
||||||
|
`make_open_handler_stream` (construct with mock manager/policy,
|
||||||
|
invoke, assert response).
|
||||||
|
- Add unit tests for `make_close_handler` and `make_control_handler`
|
||||||
|
success paths (channel exists, handler invoked).
|
||||||
|
- Add a unit test for `channel_control_spec`.
|
||||||
|
- Add a unit test for `ChannelOperations::new` (the non-default-policy
|
||||||
|
constructor).
|
||||||
|
**Acceptance gate:** `adapter.rs` ≥ 90%, `operations.rs` ≥ 85%; `cargo
|
||||||
|
test` green; clippy/fmt/doc clean.
|
||||||
|
|
||||||
|
## Unit 4 — Core types + from_call coverage (R-07, R-08, R-10)
|
||||||
|
|
||||||
|
**Goal:** `types.rs` line coverage ≥ 90%; `from_call.rs` line coverage ≥
|
||||||
|
90%; `call_client.rs` line coverage ≥ 90%.
|
||||||
|
**Files:** `src/core/types.rs`, `src/client/from_call.rs`,
|
||||||
|
`src/client/call_client.rs`.
|
||||||
|
**Scope:**
|
||||||
|
- **R-07:** add smoke tests for `SendStream::from_stream` and
|
||||||
|
`RecvStream::from_stream` — construct from a duplex pair, write/read a
|
||||||
|
few bytes, assert round-trip.
|
||||||
|
- Add a test for `BiStream::from_joined` — join independent read/write
|
||||||
|
halves, write on one side, read on the other.
|
||||||
|
- **R-08:** add a test for `make_sink_forwarding_handler` — construct a
|
||||||
|
mock `CallConnection`, feed a `PublishStream` with a few chunks,
|
||||||
|
assert the handler forwards them and terminates on `Err`.
|
||||||
|
- **R-10:** add assertions that `CallClient::registry` and
|
||||||
|
`CallClient::identity_provider` return the expected values.
|
||||||
|
**Acceptance gate:** `types.rs` ≥ 90%, `from_call.rs` ≥ 90%,
|
||||||
|
`call_client.rs` ≥ 90%; `cargo test` green; clippy/fmt/doc clean.
|
||||||
|
|
||||||
|
## Unit 5 — Remaining coverage gaps + final verification (R-09, R-11, R-12)
|
||||||
|
|
||||||
|
**Goal:** all files ≥ 80% line coverage; final verification pass.
|
||||||
|
**Files:** `src/channels/env.rs`, `src/channels/operations.rs`
|
||||||
|
(refactor opportunity).
|
||||||
|
**Scope:**
|
||||||
|
- **R-09:** either add basic delegation tests for `ChannelsSessionEnv`
|
||||||
|
(construct with a mock base `OperationEnv`, verify each method
|
||||||
|
delegates correctly) or file an OQ documenting the intentional
|
||||||
|
coverage gap. The file is small (39 lines) and the risk is low.
|
||||||
|
- **R-11:** optional — consider refactoring `run_open_wrapper` (72 lines,
|
||||||
|
9 params) into smaller helpers. Not required for v0.1.0.
|
||||||
|
- **R-12:** no action — stub handlers are intentional and documented.
|
||||||
|
- Final verification: run `cargo llvm-cov --summary-only` and confirm
|
||||||
|
all files ≥ 80% line coverage, overall ≥ 92%.
|
||||||
|
**Acceptance gate:** all files ≥ 80% line coverage; `cargo test` green;
|
||||||
|
clippy/fmt/doc clean; `cargo publish --dry-run --allow-dirty` succeeds.
|
||||||
|
|
||||||
|
## Suggested sequencing
|
||||||
|
|
||||||
|
```
|
||||||
|
Unit 1 (publishing blocker + conventions) → no deps; do first
|
||||||
|
Unit 2 (single-stream call mode tests) → no deps; independent
|
||||||
|
Unit 3 (adapter + operations coverage) → no deps; independent
|
||||||
|
Unit 4 (core types + from_call coverage) → no deps; independent
|
||||||
|
Unit 5 (remaining gaps + final verify) → depends on Units 1-4
|
||||||
|
```
|
||||||
|
|
||||||
|
Units 1-4 are independent and can proceed in any order (or in parallel
|
||||||
|
across separate branches). Unit 5 is the final verification pass and
|
||||||
|
should land last.
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## Verification log (this pass)
|
||||||
|
|
||||||
|
Findings re-verified directly in source on 2026-08-14 against tree
|
||||||
|
`7cd8a57`. Every `file:line` reference above was checked.
|
||||||
|
|
||||||
|
- **R-01:** verified by running `cargo publish --dry-run --allow-dirty`
|
||||||
|
and confirming the README.md error. License files present.
|
||||||
|
- **R-02:** verified by grepping `src/` for `\.expect(` and `\.unwrap(`
|
||||||
|
outside `#[cfg(test)]` blocks. Three matches in library code, all in
|
||||||
|
crate-private or guarded contexts.
|
||||||
|
- **R-03:** verified by reading `context.rs:147` and confirming
|
||||||
|
`generate_request_id` is called from `connection.rs` and
|
||||||
|
`dispatch.rs`.
|
||||||
|
- **R-04 through R-10:** verified by running `cargo llvm-cov
|
||||||
|
--json --summary-only` and cross-referencing uncovered regions against
|
||||||
|
the source. Coverage percentages confirmed against the JSON output.
|
||||||
|
- **R-11, R-12:** verified by reading the source at the referenced
|
||||||
|
lines.
|
||||||
|
|
||||||
|
No `unsafe` blocks found (verified by grep). No `todo!()` /
|
||||||
|
`unimplemented!()` / `unreachable!()` found (verified by grep). No
|
||||||
|
`println!` / `eprintln!` in library code (verified by grep). No
|
||||||
|
hardcoded secrets (verified by manual review of `Cargo.toml` and source
|
||||||
|
tree).
|
||||||
Reference in new issue
Block a user