# 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.