---
id: core/review-core
name: Review alknet-core implementation for spec conformance and pattern consistency
status: completed
depends_on: [core/endpoint]
scope: moderate
risk: low
impact: phase
level: review
---

## Description

Review the alknet-core implementation for spec conformance, pattern
consistency, and correctness before alknet-call (which depends on core)
begins implementation. This is the quality checkpoint at the end of the core
phase.

### Review Checklist

1. **Core types conformance** (core-types.md):
   - `ProtocolHandler` trait signature matches spec (alpn, handle)
   - `HandlerError` has all 4 variants (ConnectionClosed, StreamError, AuthRequired, Internal)
   - `Connection` has all methods, from_quinn/from_iroh feature-gated
   - `Connection::set_identity` is write-once via OnceLock
   - `BiStream` is a trait (AsyncRead + AsyncWrite + Send + Unpin)
   - `SendStream` implements AsyncWrite, `RecvStream` implements AsyncRead
   - `StreamError` has all 4 variants
   - `From<StreamError> for HandlerError` impl matches spec mapping table
   - `Capabilities` is non-serializable, zeroized, immutable, Clone+Send+Sync
   - `Capabilities` has builder API (new, with_api_key, with_http_token, get), private fields

2. **Config conformance** (config.md):
   - `StaticConfig` fields match (listen_addr, tls_identity, iroh_relay, drain_timeout)
   - `TlsIdentity` has X509, RawKey, SelfSigned
   - `DynamicConfig` has auth and rate_limits
   - `AuthPolicy` has authorized_fingerprints (HashSet<String>), api_keys (Vec<ApiKeyEntry>)
   - `ApiKeyEntry` has all 5 fields (prefix, hash, scopes, description, expires_at)
   - `ConfigReloadHandle` has reload() and dynamic()
   - No russh dependency (fingerprints as strings)
   - No removed fields (host_key, stealth, transport_mode, listeners)

3. **Auth conformance** (auth.md):
   - `AuthContext` has all 4 fields, derives Clone
   - `Identity` has id, scopes, resources
   - `AuthToken` has raw field
   - `IdentityProvider` trait with both methods
   - `ConfigIdentityProvider` reads from ArcSwap on every call
   - Fingerprint resolution looks up in authorized_fingerprints
   - Token resolution: alk_ prefix, hash match, expiry check
   - Two identity scopes documented (connection-level vs per-request)

4. **Endpoint conformance** (endpoint.md):
   - `AlknetEndpoint` has quinn/iroh (both Option, both feature-gated)
   - `HandlerRegistry` register/get/alpn_strings, panics on duplicate
   - Quinn accept loop: select on accept + shutdown, dispatch by ALPN
   - iroh accept loop: select on accept + shutdown, dispatch by ALPN
   - Dispatch spawns handler task via tokio::spawn
   - AuthContext constructed from connection (alpn, remote_addr, fingerprint, identity)
   - TLS RawKey: only_raw_public_keys(), on-the-fly cert from Ed25519
   - TLS X509: load from files
   - TLS SelfSigned: generate on startup
   - ALPN list in ServerConfig from registry.alpn_strings()
   - Graceful shutdown: drain timeout, force close
   - EndpointError has all 3 variants
   - No byte-peeking, no per-handler loops, no SSH-specific logic

5. **Pattern consistency**:
   - ArcSwap used consistently for DynamicConfig
   - Feature flags (quinn, iroh) gate transport code correctly
   - Error handling patterns consistent (thiserror, Result propagation)
   - No quinn/iroh types in public API (Connection wraps them)

6. **Security constraints**:
   - Capabilities non-serializable (no Serialize derive)
   - Capabilities zeroized (Zeroize, ZeroizeOnDrop)
   - Capabilities immutable (no mut accessors)
   - Config reload is privilege escalation (no unauthenticated reload endpoint)
   - Token entropy requirement documented

7. **Test coverage**:
   - Unit tests for Capabilities (build, get, clone, zeroize)
   - Unit tests for config types and reload
   - Unit tests for auth resolution (fingerprint, token, expiry)
   - Unit tests for HandlerRegistry
   - Integration test: endpoint dispatch by ALPN

## Acceptance Criteria

- [ ] All core types match core-types.md
- [ ] All config types match config.md
- [ ] All auth types match auth.md
- [ ] Endpoint matches endpoint.md (accept loops, TLS modes, shutdown)
- [ ] Capabilities security constraints satisfied (non-serializable, zeroized, immutable)
- [ ] No russh dependency in core
- [ ] No quinn/iroh types in public API
- [ ] ArcSwap pattern consistent
- [ ] Feature flags gate transport code correctly
- [ ] Test coverage adequate for all functionality
- [ ] `cargo fmt --check -p alknet-core` passes
- [ ] `cargo clippy -p alknet-core` passes with no warnings
- [ ] All tests pass

## References

- docs/architecture/crates/core/README.md
- docs/architecture/crates/core/core-types.md
- docs/architecture/crates/core/config.md
- docs/architecture/crates/core/auth.md
- docs/architecture/crates/core/endpoint.md
- docs/architecture/decisions/ (relevant ADRs: 001-011, 014, 015, 022)

## Notes

> This review verifies core is spec-conformant before alknet-call begins.
> alknet-call depends heavily on core types (ProtocolHandler, Connection,
> AuthContext, Capabilities, IdentityProvider) — any issues here propagate to
> call. If deviations are found, document and fix before proceeding.

## Summary

Reviewed alknet-core against all spec docs — implementation is spec-conformant.
Found and fixed one issue: `RawKeyCertResolver`/`Ed25519SigningKey`/`std::path::Path`
were gated on `#[cfg(feature = "iroh")]` but only used in the quinn TLS path,
causing clippy failures on iroh-only builds. Re-gated to
`#[cfg(all(feature = "quinn", feature = "iroh"))]`. All 4 feature combinations
now pass clippy -D warnings; 55 tests pass. Merged to develop.