gps-denied-onboard/_docs/03_implementation/reviews/batch_08_review.md

Batch 08 — Code Review

Batch: 8 of N Tasks: AZ-390 (FcAdapter / GcsAdapter Protocols + DTOs + factories + composition), AZ-392 (CovarianceProjector helper) Reviewer: autodev (7-phase) Verdict: PASS_WITH_INFO Date: 2026-05-11

Scope

Task	Component / Concern	Files touched (prod)	Files touched (tests)
AZ-390	C8 public Protocols + DTOs + errors + factories + Config blocks	_types/{fc.py,emitted.py,nav.py}, components/c8_fc_adapter/{__init__.py,interface.py,errors.py}, config/{schema.py,loader.py,__init__.py}, runtime_root/{__init__.py,fc_factory.py}	tests/unit/c8_fc_adapter/{test_az390_adapter_protocol.py,test_smoke.py}, tests/unit/test_ac1_scaffold_layout.py
AZ-392	C8 covariance projector	components/c8_fc_adapter/_covariance_projector.py	tests/unit/c8_fc_adapter/test_az392_covariance_projector.py

Phase 1 — AC compliance

Task	ACs	Coverage
AZ-390	10 ACs (Protocol conformance, frozen-slot DTOs, enum membership, flag-OFF rejection, config-load rejection, single-writer thread, GCS factory, public-API gate, error hierarchy, INFO log) + NFR-perf	All passing in test_az390_adapter_protocol.py (36 tests). Public-API gate also covered by test_smoke.py::test_internal_modules_not_in_public_all.
AZ-392	7 ACs (reduction correctness, AP float m, iNav int mm, SPD violation, NaN guard, bit-stability, uint16 clamp) + NFR-perf	All passing in test_az392_covariance_projector.py (17 tests).

53 new tests added in batch; 410 total in suite (was 357), 2 pre-existing skips, 0 failures.

Phase 2 — Contract drift

• composition_root_protocol.md v1.1.0 → v1.2.0 (minor): added cross-cutting fc: FcConfig + gcs: GcsConfig blocks on Config, and build_fc_adapter / build_gcs_adapter factories on the composition-root surface. Backwards-compatible — default FcConfig() + GcsConfig() preserve existing semantics; the four new env vars per block all have documented defaults.
• fc_adapter_protocol.md is unchanged at v1.0.0 — this batch implements the v1.0.0 surface as specified.

Phase 3 — Architectural compliance

• No new dependencies. Every new module uses stdlib + numpy + pyyaml (all already pinned). YAMSPy entry is gated on AZ-391 (batch 9).
• Module-layout adherence: _covariance_projector.py is prefixed _ per module-layout.md rule "internal helpers MUST start with _"; c8_fc_adapter/__init__.__all__ exposes ONLY the contract symbols (asserted by test_smoke.test_internal_modules_not_in_public_all).
• Layer 1 (helpers) discipline: fc_factory.py imports only from gps_denied_onboard.config, gps_denied_onboard.logging, and gps_denied_onboard.components.c8_fc_adapter.{errors,interface}. No upward imports from c8 internals into the composition root.
• ADR-002 build-time exclusion: the _FC_BUILD_FLAGS mapping is the single source of truth tying a strategy slug to its build flag. The factory consults os.environ (not config) because flag state is a build-time artifact per the ADR.
• ADR-009 interface-first DI: all factories take **deps keyword args; concrete strategies are registered via register_fc_adapter(slug, factory) from per-binary bootstrap modules, never imported directly by the composition root.
• AC-NEW-3 (every payload class from t=0) preserved: this batch did NOT wire take_off to call build_fc_adapter (the strict ordering is already enforced by batch 7's take_off); only the factory itself landed. The wire-up happens when the first concrete adapter (AZ-393) lands.
• Single-writer outbound thread (AC-6 / Invariant 8): bind_outbound_emit_thread is process-global state guarded by a single lock; idempotent on same-thread re-binds (test-isolation friendly).

Phase 4 — Performance & reliability

• Factory build path is O(strategy-name-lookup): two dict lookups + one env-var read; AC-NFR sanity check verifies < 50 ms (typically < 100 µs measured locally).
• CovarianceProjector is O(1) per call: 2×2 closed-form eigenvalue, no LAPACK dispatch. test_nfr_perf_projector_under_100us_per_call verifies < 100 µs avg over 1k iterations.
• iNav clamp path emits at most one WARN + one FDR record per occurrence — no rate-limit needed because the clamp itself is a per-frame event already gated by the projector call rate (5 Hz emit per AC-1.4).
• SPD violation path is allocation-light: one numpy view (cov_arr[:3, :3]), one 2×2 sub-view, two scalar reads. The FDR enqueue happens BEFORE the raise so the operator gets the post-flight forensic record even if the upstream caller swallows the exception.

Phase 5 — Test quality

• AC-1 Protocol tests use real stub classes (not mock.MagicMock) so isinstance(x, FcAdapter) exercises the @runtime_checkable protocol-fingerprint check honestly.
• AC-2 DTO tests parametrise over EVERY contract DTO, asserting both frozen-instance immutability AND __slots__ presence. Forgetting slots=True on a new DTO fails the matching parametrise.
• AC-3 enum tests assert exact member set (not a subset) — adding an unintended enum member also fails the gate.
• AC-4 / AC-5 tests separate the config-load gate from the build-time gate: unknown strategy fails at FcConfig(...) construction; flag-OFF fails at build_fc_adapter(config); unregistered-but-known-strategy fails at build_fc_adapter with a clear "registered strategies: [...]" message.
• AC-6 cross-thread rebind test uses a real threading.Thread — not just a different thread_ident argument — so the lock's cross-thread visibility is exercised.
• AC-10 INFO-log tests pin caplog.at_level(...) to the exact logger name so they don't accidentally pass on unrelated INFO records elsewhere in the process.
• AZ-392 SPD-violation tests assert both the raise AND the FDR record carrying reason + frame_id — a regression that drops the FDR record would not pass the raise-only test alone.
• AZ-392 bit-stability test calls 20 times on the same input and asserts len(set(results)) == 1 — drift between calls (e.g. from numpy.eigvalsh-style round-off) fails the gate.
• AZ-392 NFR-perf test uses time.perf_counter with 1 k iterations and a 100 µs/call budget — sufficient headroom for jittery CI runners while still catching an order-of-magnitude regression.

Arrange / Act / Assert pattern consistently applied in all new tests.

Phase 6 — Logging & FDR coverage

• fc_factory.build_fc_adapter: INFO log per build (kind="c8.adapter.strategy_loaded", kv={strategy, port_device}).
• fc_factory.build_gcs_adapter: INFO log per build (kind="c8.gcs.strategy_loaded", kv={strategy, port_device}).
• CovarianceProjector: WARN log on iNav clamp (kind="c8.cov_projector.inav_clamped", kv={radius_mm_raw, clamped_to, frame_id}).
• CovarianceProjector: FDR ERROR record on every projection rejection (kind="log", payload.level="ERROR", payload.kind="c8.cov_projector.spd_violation", payload.kv={reason, frame_id, …}). Uses the existing log record kind so no FDR schema bump is needed.
• All log records follow the kind + kv convention required by AZ-266's JsonFormatter.

Phase 7 — Security & risk surface

• RESTRICT-COMM-2 (iNav has no signing): enforced at config-load by FcConfig.__post_init__ — passing adapter="inav" with any signing_key_source != "none" raises ConfigError with the restriction code in the message. Covered by test_ac5_inav_signing_key_combination_rejected.
• Build-flag gate uppercases the env-var value (os.environ.get(...).upper() == "OFF") so BUILD_FC_INAV=off is also honoured. The DEFAULT (no env var set) is ON-per-binary; concrete binaries SHOULD set it explicitly to keep the registry → flag mapping honest.
• Strategy registry rejects duplicate registrations with different factories (FcAdapterConfigError); same-factory re-registration is idempotent. This prevents a second bootstrap module from silently overriding the first.
• No silent fallback: every config-load / build / projection failure raises or emits an FDR record. The covariance-projector FDR enqueue is best-effort but logs its own c8.cov_projector.fdr_enqueue_failed so a silenced FDR doesn't hide the SPD failure.
• bind_outbound_emit_thread does not unbind on its own — clear_outbound_thread_binding() is intentionally scoped to test-isolation. A process that wants to re-bind in production must restart, which matches the single-writer invariant.

Informational findings (non-blocking)

1. take_off is not yet wired to call build_fc_adapter / build_gcs_adapter — this is the carry-over from batch 7's PASS_WITH_INFO finding #3. The wire-up will happen when the first concrete adapter lands (AZ-393), where the fc_factory can be exercised end-to-end with a real adapter. Documented in the contract bump.
2. Subscription Protocol has only cancel() — the contract spec leaves open whether a Subscription should also be a context manager. We did NOT add __enter__ / __exit__ because no AC requires it and AZ-391 hasn't started consuming the type yet. If AZ-391 needs ctx-manager semantics, that's a v1.1.0 contract bump on fc_adapter_protocol.md, not a regression.
3. _covariance_projector.py is intentionally NOT in c8_fc_adapter/__init__.__all__ — concrete adapters (AZ-393 / AZ-394) instantiate it via direct module import, since it is an internal helper. The smoke test asserts the public-API gate; no contract bump required because the projector is a concrete helper, not a Protocol.

Verdict

PASS_WITH_INFO — all ACs satisfied, all tests green, no architectural drift, one minor contract bump (composition_root_protocol.md 1.1.0 → 1.2.0) documented inline with migration notes. The three informational findings are forward actions tied to upcoming batches, not blockers.