148 lines
7.7 KiB
Markdown
148 lines
7.7 KiB
Markdown
---
|
|
id: call/review-call
|
|
name: Review alknet-call implementation for spec conformance and pattern consistency
|
|
status: completed
|
|
depends_on: [call/protocol/abort-cascade]
|
|
scope: broad
|
|
risk: low
|
|
impact: phase
|
|
level: review
|
|
---
|
|
|
|
## Description
|
|
|
|
Review the alknet-call implementation for spec conformance, pattern
|
|
consistency, and correctness. This is the quality checkpoint at the end of the
|
|
call phase — the most complex crate in this batch.
|
|
|
|
### Review Checklist
|
|
|
|
1. **Registry conformance** (operation-registry.md):
|
|
- `OperationSpec` has all 8 fields, `path()` adds leading slash
|
|
- `OperationType` (Query/Mutation/Subscription), `Visibility` (External/Internal)
|
|
- `ErrorDefinition` with code, description, schema, http_status (ADR-023)
|
|
- `AccessControl` with required_scopes (AND), required_scopes_any (OR), resource checks
|
|
- `AccessControl::check` returns Allowed/Forbidden, None identity with restrictions → Forbidden
|
|
- `OperationContext` has all 10 fields, `internal` is pub(crate), `is_internal()` reads
|
|
- `AbortPolicy` (AbortDependents default, ContinueRunning opt-in)
|
|
- `CompositionAuthority` with label, scopes, resources, `as_identity()`
|
|
- `ScopedOperationEnv` with `allows()` reachability check
|
|
- `Handler` type (async closure → ResponseEnvelope)
|
|
- `HandlerRegistration` with all 6 fields (spec, handler, provenance, authority, scoped_env, caps)
|
|
- `OperationProvenance` with all 6 variants
|
|
- `OperationRegistry` with register, registration, invoke, list_operations
|
|
- `OperationRegistryBuilder` with with_local, with_leaf, with, build
|
|
- `OperationEnv` trait with invoke, invoke_with_policy, contains
|
|
- `LocalOperationEnv` reachability check, authority switch, fresh metadata, inherited deadline
|
|
- `CompositeOperationEnv` overlay dispatch (session → connection → base), contains probe
|
|
- `services/list` returns External only, `services/schema` returns full spec
|
|
|
|
2. **Protocol conformance** (call-protocol.md):
|
|
- `EventEnvelope` with type, id, payload (JSON, length-prefixed framing)
|
|
- `ResponseEnvelope` with request_id, result
|
|
- `CallError` with code, message, retryable, details
|
|
- 5 event types: call.requested, call.responded, call.completed, call.aborted, call.error
|
|
- Wire payload schemas match spec table
|
|
- `call.requested` has operationId (leading slash), input, optional auth_token
|
|
- `call.error` has protocol-level codes (NOT_FOUND, FORBIDDEN, INVALID_INPUT, INTERNAL, TIMEOUT)
|
|
- `PendingRequestMap` correlates by ID (not stream), handles all event types
|
|
- `CallConnection` with Layer 2 overlay, register_imported, overlay_env, call/subscribe/abort
|
|
- `CallAdapter` implements ProtocolHandler for alknet/call
|
|
- CallAdapter stream handling (accept_bi loop, FrameFramedReader/Writer)
|
|
- Per-request identity resolution (auth_token overrides connection-level)
|
|
- `build_root_context` sets internal: false, deadline, capabilities from registration
|
|
- `compose_root_env` builds CompositeOperationEnv (base + session + connection)
|
|
- operationId leading slash stripped before lookup
|
|
- ResponseEnvelope → EventEnvelope conversion
|
|
- Timeout: 30s default, composed calls inherit parent deadline
|
|
- Abort cascade: walks tree by parent_request_id, AbortDependents/ContinueRunning
|
|
|
|
3. **ADR conformance**:
|
|
- ADR-005: irpc framing used
|
|
- ADR-012: bidirectional streams, ID-based correlation
|
|
- ADR-014: no secret material on wire, Capabilities non-serializable
|
|
- ADR-015: internal flag switches authority (handler_identity vs identity), Visibility
|
|
- ADR-016: abort cascade, AbortPolicy, default AbortDependents
|
|
- ADR-017: connection direction independent of call direction
|
|
- ADR-022: registration bundle (provenance, authority, scoped_env, capabilities)
|
|
- ADR-023: ErrorDefinition, typed details in call.error
|
|
- ADR-024: registry layering (curated + session + connection), OperationEnv as trait
|
|
|
|
4. **Security constraints**:
|
|
- Capabilities non-serializable (no Serialize derive)
|
|
- Capabilities zeroized, immutable after construction
|
|
- Metadata does not propagate through composition (fresh HashMap::new())
|
|
- Call protocol carries no secret material
|
|
- Internal ops return NOT_FOUND from wire (don't leak existence)
|
|
- Reachability check (scoped_env.allows) bounds composition attack surface
|
|
- Request IDs are UUID v4 (non-deterministic, no collisions)
|
|
|
|
5. **Pattern consistency**:
|
|
- OperationEnv is a trait (not concrete) — enables layering
|
|
- CompositeOperationEnv uses contains() probe before dispatch
|
|
- Authority switch in invoke_with_policy (child identity = parent handler_identity)
|
|
- Deadline inheritance (children don't get fresh 30s)
|
|
- ArcSwap not used in call (that's core's pattern)
|
|
|
|
6. **Test coverage**:
|
|
- Unit tests for AccessControl::check (all combinations)
|
|
- Unit tests for OperationContext construction
|
|
- Unit tests for OperationEnv (LocalOperationEnv, CompositeOperationEnv)
|
|
- Unit tests for PendingRequestMap (all event types, timeouts, fail_all)
|
|
- Unit tests for framing (round-trip, truncation)
|
|
- Unit tests for abort cascade (both policies, tree walking)
|
|
- Integration test: call.requested → dispatch → call.responded
|
|
- Integration test: auth_token overrides identity
|
|
- Integration test: Internal op → NOT_FOUND from wire
|
|
- Integration test: ACL denied → FORBIDDEN
|
|
- Integration test: subscription streaming (multiple responded, completed)
|
|
|
|
## Acceptance Criteria
|
|
|
|
- [ ] All registry types match operation-registry.md
|
|
- [ ] All protocol types match call-protocol.md
|
|
- [ ] All ADRs conformed to (005, 012, 014, 015, 016, 017, 022, 023, 024)
|
|
- [ ] Capabilities non-serializable, zeroized, immutable
|
|
- [ ] Metadata does not propagate through composition
|
|
- [ ] Internal ops return NOT_FOUND from wire
|
|
- [ ] Reachability check bounds composition
|
|
- [ ] Request IDs are UUID v4
|
|
- [ ] OperationEnv is a trait (not concrete)
|
|
- [ ] CompositeOperationEnv uses contains() probe
|
|
- [ ] Authority switch correct (internal: true → handler_identity)
|
|
- [ ] Deadline inheritance correct (children inherit parent deadline)
|
|
- [ ] Test coverage adequate for all functionality
|
|
- [ ] `cargo fmt --check -p alknet-call` passes
|
|
- [ ] `cargo clippy -p alknet-call` passes with no warnings
|
|
- [ ] All tests pass
|
|
|
|
## References
|
|
|
|
- docs/architecture/crates/call/README.md
|
|
- docs/architecture/crates/call/call-protocol.md
|
|
- docs/architecture/crates/call/operation-registry.md
|
|
- docs/architecture/decisions/ (relevant ADRs: 005, 012, 014-017, 022-024)
|
|
|
|
## Notes
|
|
|
|
> This is the most complex crate in this batch. The review should verify that
|
|
> the registry layering (ADR-024), authority switch (ADR-015), abort cascade
|
|
> (ADR-016), and composition model (ADR-022) all work correctly together. The
|
|
> OperationEnv trait-object design is load-bearing — verify it's a trait, not
|
|
> concrete. If deviations are found, document and fix before considering the
|
|
> call crate complete.
|
|
|
|
## Summary
|
|
|
|
Reviewed the entire alknet-call crate against operation-registry.md,
|
|
call-protocol.md, and ADRs 005/012/014/015/016/017/022/023/024. All registry
|
|
types (OperationSpec 8 fields, AccessControl AND/OR + resource checks,
|
|
HandlerRegistration 6 fields, OperationProvenance 6 variants, CompositionAuthority,
|
|
ScopedOperationEnv, AbortPolicy), protocol types (EventEnvelope, ResponseEnvelope,
|
|
CallError, 5 event types, framing), security constraints (Capabilities
|
|
non-serializable/zeroized/immutable, metadata fresh on composition, internal ops
|
|
→ NOT_FOUND from wire, reachability bounds composition, UUID v4 request IDs),
|
|
and pattern consistency (OperationEnv trait, CompositeOperationEnv contains()
|
|
probe, authority switch on internal: true, deadline inheritance) are conformant.
|
|
No source deviations found. 159 tests pass; build/clippy/fmt/test clean.
|
|
Merged to develop. |