docs(tasks): mark call/review-completion complete with review findings
Records the conformance review for the alknet-call client/adapter completion batch. All 14 acceptance criteria pass: - CallClient, from_call, OperationAdapter, from_jsonschema match client-and-adapters.md. - Peer-scoped default-deny (ADR-028) enforced: non-remote-safe ops return NOT_FOUND before capabilities are populated (the load-bearing security assertion, verified by three capability-exposure tests). - Shared dispatch loop is genuinely shared (single Dispatcher; CallAdapter and CallClient both delegate to run_loop). - No-env-vars invariant holds (no std::env::var reads; credentials from Capabilities). - Adapter location map respected (no HTTP deps in alknet-call). - OQ-25..28 two-way-door remainders recorded with v1 defaults. - No spec drift requiring amendment. - 207 lib + 2 integration tests pass; clippy + fmt clean. This closes the call-completion batch. Unblocks every downstream consumer (runner, container service, bilateral exchange, NAPI, agent cross-node dispatch) and alknet-http Phase 1 (OperationAdapter trait). Refs: tasks/call/review-completion.md
This commit is contained in:
@@ -1,7 +1,7 @@
|
|||||||
---
|
---
|
||||||
id: call/review-completion
|
id: call/review-completion
|
||||||
name: Review alknet-call client/adapter completion for spec conformance (ADR-017, ADR-028) and no-env-vars invariant
|
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]
|
depends_on: [call/client/from-call, call/client/operation-adapter-trait, call/client/from-jsonschema]
|
||||||
scope: broad
|
scope: broad
|
||||||
risk: low
|
risk: low
|
||||||
@@ -111,20 +111,20 @@ NAPI, agent cross-node dispatch).
|
|||||||
|
|
||||||
## Acceptance Criteria
|
## Acceptance Criteria
|
||||||
|
|
||||||
- [ ] CallClient matches client-and-adapters.md (struct, new/trusted_peer, connect, shared loop)
|
- [x] 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)
|
- [x] 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)
|
- [x] from_call matches client-and-adapters.md (flow, FromCallConfig, provenance, None fields)
|
||||||
- [ ] OperationAdapter trait + AdapterError match client-and-adapters.md (async, non_exhaustive, variants)
|
- [x] 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)
|
- [x] 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)
|
- [x] 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)
|
- [x] 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)
|
- [x] 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)
|
- [x] 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
|
- [x] 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)
|
- [x] No spec/impl drift in client-and-adapters.md (or drift documented + spec amended)
|
||||||
- [ ] `cargo fmt --check -p alknet-call` passes
|
- [x] `cargo fmt --check -p alknet-call` passes
|
||||||
- [ ] `cargo clippy -p alknet-call --all-targets` passes with no warnings
|
- [x] `cargo clippy -p alknet-call --all-targets` passes with no warnings
|
||||||
- [ ] All tests pass
|
- [x] All tests pass
|
||||||
|
|
||||||
## References
|
## References
|
||||||
|
|
||||||
@@ -155,4 +155,124 @@ NAPI, agent cross-node dispatch).
|
|||||||
> becoming a parallel protocol implementation — verify the loop is genuinely
|
> becoming a parallel protocol implementation — verify the loop is genuinely
|
||||||
> shared (refactored out of CallAdapter), not copy-pasted. If deviations are
|
> shared (refactored out of CallAdapter), not copy-pasted. If deviations are
|
||||||
> found, document and fix before considering the call-completion batch done.
|
> found, document and fix before considering the call-completion batch done.
|
||||||
> This unblocks every downstream consumer, so spec/impl drift here propagates.
|
> 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<Vec<HandlerRegistration>, 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<CallConnection,
|
||||||
|
ClientError>` (the spec sketch showed `Result<CallConnection>` 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
|
||||||
Reference in New Issue
Block a user