alknet/tasks/review/phase1-core-modifications.md at 83ea66b5d13ffc14c43c8978a2c518569c7ced73

Files

glm-5.1 eed3396705 tasks: add 5 cleanup tasks from Phase 1 review, mark review complete

Review found Phase 1 approved with minor issues. Created cleanup tasks:
- napi-identity-provider-wiring (bug: NAPI bypasses IdentityProvider)
- panic-free-static-config (code smell: panic/unwrap in production path)
- non-exhaustive-public-api (future-proofing public API types)
- adr-doc-comments (doc convention: ADR references in new modules)
- ssh-session-recv-stub-doc (documentation: mark stubs as planned)

2026-06-08 04:35:52 +00:00

3.2 KiB

Raw Blame History

id, name, status, depends_on, scope, risk, impact, level

name

status

depends_on

scope

risk

impact

level

review/phase1-core-modifications

Review Phase 1 core modifications — config split, identity, forwarding, OperationEnv, interface abstraction

completed

core/ssh-interface-extraction

core/operationenv-local-dispatch

core/auth-service-irpc

core/config-service-irpc

core/napi-reload-api

broad

medium

project

review

Description

Review the Phase 1 core modifications after all implementation tasks are complete. This is a quality checkpoint before Phase 2 (external crates) and Phase 3 (integration/wiring).

Review checklist:

All ADRs (026-034) are correctly reflected in implementation
Crate dependencies are acyclic — core doesn't depend on secret, storage, or flowgraph
Terminology is consistent — head/worker everywhere, no hub/spoke remaining
Layer boundaries are clean — Interface produces call protocol events, Protocol is agnostic
IdentityProvider trait is the sole contract for auth — no direct ServerAuthConfig usage remains
DynamicConfig + ArcSwap provides hot-reload for auth and forwarding
ForwardingPolicy default-allow preserves current behavior
OperationEnv local dispatch works correctly through the registry
Feature flags (irpc) compile correctly — core without feature flag has no irpc dependency
All existing tests pass
New test coverage for config reload, identity resolution, forwarding policy
NAPI reload API functions correctly
Interface trait and SshInterface extraction don't break SSH tunnel functionality

Acceptance Criteria

Code adheres to architecture specs (configuration.md, identity.md, interface.md, call-protocol.md, services.md)
Patterns are consistent (IdentityProvider, DynamicConfig/ArcSwap, OperationEnv, Interface trait)
Tests cover core functionality: config hot-reload, identity resolution, forwarding policy evaluation, local dispatch
No cargo build errors or warnings
All feature flag combinations compile: default, irpc, tls, iroh, acme
Documentation comments reference ADR numbers
Phase 1 implementation notes are filled in on all task files

References

docs/architecture/overview.md
docs/architecture/configuration.md
docs/architecture/identity.md
docs/architecture/interface.md
docs/architecture/call-protocol.md
docs/architecture/services.md
docs/architecture/decisions/ (ADR-026 through ADR-034)

Notes

Review completed by code-reviewer agent. All 13 checklist items passed. 5 warnings identified (W1: NAPI bypasses IdentityProvider = functional bug with cleanup task created; W2: task notes unfilled = by design; W3: SshSession::recv() stub = documented as future work with cleanup task; W4: ServerAuthConfig bridge = acceptable transition debt; W5: panic/unwrap in StaticConfig = code smell with cleanup task created). 4 non-blocking suggestions also generated cleanup tasks.

Summary

Phase 1 core modifications approved with minor changes recommended. 385 tests pass, all feature flags compile, clippy clean. All ADRs 026-034 correctly reflected. Main concern: NAPI handler bypasses IdentityProvider/ForwardingPolicy (cleanup task created). 5 cleanup tasks created to address warnings and suggestions before proceeding to Phase 2.

3.2 KiB Raw Blame History