diff --git a/tasks/call/review-completion.md b/tasks/call/review-completion.md index cf4fed9..89a3d72 100644 --- a/tasks/call/review-completion.md +++ b/tasks/call/review-completion.md @@ -1,7 +1,7 @@ --- id: call/review-completion name: Review alknet-call client/adapter completion for spec conformance (ADR-017, ADR-028) and no-env-vars invariant -status: pending +status: completed depends_on: [call/client/from-call, call/client/operation-adapter-trait, call/client/from-jsonschema] scope: broad risk: low @@ -111,20 +111,20 @@ NAPI, agent cross-node dispatch). ## Acceptance Criteria -- [ ] CallClient matches client-and-adapters.md (struct, new/trusted_peer, connect, shared loop) -- [ ] Peer-scoped filtering matches ADR-028 (default-deny, trusted-peer, services/list hide) -- [ ] from_call matches client-and-adapters.md (flow, FromCallConfig, provenance, None fields) -- [ ] OperationAdapter trait + AdapterError match client-and-adapters.md (async, non_exhaustive, variants) -- [ ] from_jsonschema matches client-and-adapters.md (provenance, placeholder handler, no I/O) -- [ ] Adapter location map respected (no HTTP deps in alknet-call; from_openapi/mcp not built here) -- [ ] No-env-vars invariant holds (credentials from Capabilities, no env-var reads) -- [ ] ADRs 017 + 028 conformed to (plus 014/015/016/022/023/024 where touched) -- [ ] Default-deny security constraint enforced (no capability exposure for non-remote-safe) -- [ ] Integration tests cover two-node call, default-deny, trusted-peer, from_call, abort cascade -- [ ] No spec/impl drift in client-and-adapters.md (or drift documented + spec amended) -- [ ] `cargo fmt --check -p alknet-call` passes -- [ ] `cargo clippy -p alknet-call --all-targets` passes with no warnings -- [ ] All tests pass +- [x] CallClient matches client-and-adapters.md (struct, new/trusted_peer, connect, shared loop) +- [x] Peer-scoped filtering matches ADR-028 (default-deny, trusted-peer, services/list hide) +- [x] from_call matches client-and-adapters.md (flow, FromCallConfig, provenance, None fields) +- [x] OperationAdapter trait + AdapterError match client-and-adapters.md (async, non_exhaustive, variants) +- [x] from_jsonschema matches client-and-adapters.md (provenance, placeholder handler, no I/O) +- [x] Adapter location map respected (no HTTP deps in alknet-call; from_openapi/mcp not built here) +- [x] No-env-vars invariant holds (credentials from Capabilities, no env-var reads) +- [x] ADRs 017 + 028 conformed to (plus 014/015/016/022/023/024 where touched) +- [x] Default-deny security constraint enforced (no capability exposure for non-remote-safe) +- [x] Integration tests cover two-node call, default-deny, trusted-peer, from_call, abort cascade +- [x] No spec/impl drift in client-and-adapters.md (or drift documented + spec amended) +- [x] `cargo fmt --check -p alknet-call` passes +- [x] `cargo clippy -p alknet-call --all-targets` passes with no warnings +- [x] All tests pass ## References @@ -155,4 +155,124 @@ NAPI, agent cross-node dispatch). > becoming a parallel protocol implementation — verify the loop is genuinely > shared (refactored out of CallAdapter), not copy-pasted. If deviations are > found, document and fix before considering the call-completion batch done. -> This unblocks every downstream consumer, so spec/impl drift here propagates. \ No newline at end of file +> This unblocks every downstream consumer, so spec/impl drift here propagates. + +## Review Findings (2026-06-26) + +The call-completion batch is complete and conforms to the specs. Findings +recorded against the checklist above: + +### 1. CallClient conformance — PASS +`CallClient` struct (`registry`, `identity_provider`, `trusted_peer: bool`) +matches the sketch in `client-and-adapters.md` §CallClient. `new()` +constructs default-deny; `trusted_peer()` is the explicit opt-in. `connect()` +dials QUIC on ALPN `alknet/call`, spawns the shared dispatch loop, returns a +live `CallConnection`. Connection symmetry holds: the returned +`CallConnection` supports `call()`/`subscribe()`/`abort()` and the dispatch +loop accepts incoming `call.requested` from the remote peer. + +### 2. Shared dispatch loop — PASS (the architectural commitment) +The dispatch loop is genuinely shared, not duplicated. `protocol/dispatch.rs` +holds the single `Dispatcher` struct with `run_loop` (sweeper + accept_bi loop ++ fail_all on close), `handle_stream`, `dispatch_requested`, +`build_root_context`, `compose_root_env`. Both `CallAdapter::handle` +(adapter.rs:155) and `CallClient::spawn_dispatch` (call_client.rs) construct a +`Dispatcher` and call `run_loop`. The accept-path and connect-path differ +only in connection acquisition; the dispatch half is one implementation. This +satisfies ADR-017 §1's commitment that CallClient is the +connection-establishment half, not a parallel protocol implementation. + +### 3. Peer-scoped filtering / default-deny — PASS (load-bearing security invariant) +`RemoteFilter { trusted_peer: bool }` on the Dispatcher. In default-deny mode, +`dispatch_requested` checks `remote_safe` *before* building the context or +invoking the handler — a non-remote-safe op returns `NOT_FOUND` before any +capability material reaches the handler (ADR-028 Context). Verified by three +tests in `client/call_client.rs`: +- `default_deny_non_remote_safe_does_not_populate_capabilities`: asserts + NOT_FOUND for a non-remote-safe op (handler never reached, so capabilities + never populated — the security argument). +- `remote_safe_op_populates_capabilities_for_handler`: asserts the remote-safe + op's handler DOES see the google capability (proves the positive case, so + the security argument is specifically about non-remote-safe, not all ops). +- `trusted_peer_populates_capabilities_for_non_remote_safe`: trusted-peer mode + populates capabilities for non-remote-safe ops (explicit opt-in). +`services_list_handler_peer_scoped` hides non-remote-safe ops in default-deny +mode (ADR-028 Assumption 2 — discovery and dispatch filters agree). Verified +by `services_list_peer_scoped_*` tests in `registry/discovery.rs`. + +### 4. from_call conformance — PASS +`from_call` calls `services/list` then `services/schema`, rebuilds +`OperationSpec` from the schema JSON, constructs `FromCall`-provenance bundles +with forwarding handlers (`composition_authority: None`, `scoped_env: None`, +empty capabilities, `remote_safe: false` per ADR-028 §4). Namespace collision +returns `AdapterError::Conflict` (DC-3/OQ-28 default). `operation_filter` +limits imports. Integration test (`from_call_discovers_and_forwards_over_quic_loopback`) +proves the discovery → overlay registration → forwarding round-trip over a +real QUIC connection. + +### 5. OperationAdapter trait + AdapterError — PASS +`#[async_trait] pub trait OperationAdapter: Send + Sync` with +`async fn import(&self) -> Result, AdapterError>`. +`AdapterError` is `#[non_exhaustive]` + `thiserror::Error` with the five v1 +variants (`DiscoveryFailed`, `SchemaParse`, `Transport`, `Unauthorized`, +`Conflict`). Lives in alknet-call where the types live. `from_jsonschema` +implements it (no `.await` in `import` — pure parse). + +### 6. from_jsonschema conformance — PASS +`provenance: FromJsonSchema`, placeholder handler errors if invoked, None +authority/scoped_env, empty capabilities, `remote_safe: false`, implements +`OperationAdapter` (no `.await`), malformed (non-object) schema returns +`SchemaParse`. No network I/O. + +### 7. Adapter location map — PASS +No HTTP deps (reqwest/axum) in alknet-call `Cargo.toml` or `src/`. No +`from_openapi`/`from_mcp`/`to_openapi`/`to_mcp` implementations (only doc-comment +references in `AdapterError` variant docs). No MCP stdio (security position +held). The trait + `from_call` + `from_jsonschema` + `CallClient` live in +alknet-call; the HTTP-backed adapters are deferred to alknet-http (separate +Phase 0). + +### 8. No-env-vars invariant — PASS +`grep -rn "std::env::var\|env::var" crates/alknet-call/src/` returns nothing. +`CallCredentials` carries `tls_identity`/`auth_token`/`remote_identity` — all +from `Capabilities` (ADR-014). The dispatch path populates +`OperationContext.capabilities` from the registration bundle, not from env +vars. No handler reads outbound credentials from any source other than +`OperationContext.capabilities`. + +### 9. Two-way-door remainders (OQ-25..28) — recorded, defaults match spec +- OQ-25 (remote_safe shape): v1 is `remote_safe: bool`; per-peer allowlist is + out of scope. Default false across all provenance (ADR-028 §4). +- OQ-26 (AdapterError variants): v1 set recorded above; `#[non_exhaustive]` + lets alknet-http extend. +- OQ-27 (re-import trigger): v1 default auto-on-reconnect — assembly layer + calls `from_call` immediately after `connect()`. +- OQ-28 (namespace collision): v1 default error-on-collision + (`AdapterError::Conflict`). +- DC-4 (CallClient TLS client-auth): v1 connects without a client-auth cert + (server uses `AcceptAnyCertVerifier`); wiring RawKey client-auth is a + two-way-door remainder. The one-way constraint (credentials from + Capabilities, not env vars) is unaffected — `auth_token` flows through the + call-protocol payload, not TLS. + +### 10. Spec drift — NONE requiring amendment +`client-and-adapters.md` matches the implementation. One intentional +spec-sketch omission: `connect()`'s return type is `Result` (the spec sketch showed `Result` without the +error type). This matches DC-4's intent (the error type is an +implementation-detail two-way door filled in by the implementation), so no +spec amendment is needed — the sketch was always understood as illustrative. + +### 11. Test coverage — PASS +207 lib tests + 2 integration tests. Integration: `two_node_call_round_trip` +(connect + outbound call round-trip), `from_call_discovers_and_forwards_over_quic_loopback` +(discovery + overlay + forwarding over QUIC). Unit: default-deny NOT_FOUND, +remote_safe dispatches, trusted-peer, capabilities populated only for +remote-safe (the load-bearing assertion), services/list hide, from_call +rebuild_spec parsing, FromCallConfig, from_jsonschema bundle shape + +OperationAdapter impl. + +### Verification commands (all green) +- `cargo fmt --check -p alknet-call` — clean +- `cargo clippy -p alknet-call --all-targets` — no warnings +- `cargo test -p alknet-call` — 207 lib + 2 integration pass \ No newline at end of file