# Code Review Report

**Batch**: 72 — AZ-416, AZ-417, AZ-419
**Date**: 2026-05-16
**Verdict**: PASS

## Findings

(none)

## Findings Sweep

### Phase 1 — Context Loading

Loaded specs `AZ-416_ft_p_09_ap_signing.md`, `AZ-417_ft_p_09_inav.md`,
`AZ-419_ft_p_11_cold_start_init.md`. Re-read existing
`runner/helpers/mavproxy_tlog_reader.py` (AZ-406 surface to be filled
in by AZ-416 per the docstring), `sitl_observer.py`, `fdr_reader.py`,
`geo.py`. Read `fixtures/cold-boot/cold_boot_fixture.json` for FT-P-11
secondary path origin. Verified pymavlink ≥2.4 install + the
`MAVLink.get_signed()` API surface in the venv.

### Phase 2 — Spec Compliance

**AZ-416 (FT-P-09-AP)**

| AC | Coverage | Status |
|----|----------|--------|
| AC-1 (signing handshake ≤5 s, no BAD_SIGNATURE) | `test_handshake_passes_when_first_signed_within_window`, `test_handshake_fails_when_no_signed_within_window`, `test_handshake_fails_when_signed_arrives_after_budget`, `test_handshake_fails_on_bad_signature_statustext`, scenario assertion via `observe_signing_handshake` | Covered |
| AC-2 (GPS_INPUT ≥4.5 Hz for 5 Hz target) | `test_gps_input_rate_at_5hz_for_60s_passes`, `test_gps_input_rate_at_boundary_passes`, `test_gps_input_rate_below_minimum_fails`, scenario assertion via `compute_gps_input_rate` | Covered |
| AC-3 (EK3_SRC1_POSXY == 3) | `test_validate_ek3_src1_posxy_passes_at_3`, scenario via `validate_ek3_src1_posxy(sitl_observer.read_ap_parameter(...))` | Covered |
| AC-4 (GPS_RAW_INT healthy fraction ≥80 %) | `test_gps_raw_int_health_all_healthy_passes`, `test_gps_raw_int_health_at_80_pct_boundary_passes`, `test_gps_raw_int_health_below_80_pct_fails`, `test_gps_raw_int_health_eph_threshold_strict`, scenario via `evaluate_gps_raw_int_health` | Covered |
| AC-5 (vio_strategy parameterization; `fc_adapter` fixed to `ardupilot`) | scenario uses `vio_strategy` fixture from conftest; `fc_adapter != "ardupilot"` is skipped — collection across 6 variants reduces to 3 active variants | Covered |
| D-C8-9 (signing-handshake observability) | `traces_to` marker + handshake report includes `setup_signing_seen` | Covered |

Also: AZ-416's `mavproxy_tlog_reader.iter_messages` body landed
(previously raised NotImplementedError per the AZ-406 commit). 6 unit
tests in `test_mavproxy_tlog_reader.py` exercise the parser against
synthetic tlogs.

**AZ-417 (FT-P-09-iNav)**

| AC | Coverage | Status |
|----|----------|--------|
| AC-1 (TCP connect to inav-sitl:5760 ≤5 s) | scenario via `sitl_observer.observe_inav_tcp_handshake` (skip-gated) | Covered (gated) |
| AC-2 (MSP2_SENSOR_GPS ≥4.5 Hz for 5 Hz target) | `test_compute_rate_at_target_passes`, `test_compute_rate_at_boundary_passes`, `test_compute_rate_below_minimum_fails`, `test_compute_rate_filters_function_id`, scenario via `compute_rate_hz` | Covered |
| AC-3 (fix_type ≥3, provider=MSP, numSat matches emitted) | `test_evaluate_gps_state_passes_at_minimum_fix`, `test_evaluate_gps_state_fails_on_low_fix_type`, `test_evaluate_gps_state_fails_on_wrong_provider`, `test_evaluate_gps_state_fails_on_num_sat_mismatch`, scenario via `evaluate_inav_gps_state` | Covered |
| AC-4 (vio_strategy parameterization; `fc_adapter` fixed to `inav`) | scenario uses `vio_strategy` fixture; skips when `fc_adapter != "inav"` | Covered |

**AZ-419 (FT-P-11)**

| AC | Coverage | Status |
|----|----------|--------|
| AC-1 (operator_manifest: estimate ≤50 m of A; FDR `cold_start_origin.set(source="manifest")`) | `test_evaluate_operator_manifest_passes_at_origin`, `test_evaluate_operator_manifest_passes_just_inside_budget`, `test_evaluate_operator_manifest_fails_just_outside_budget`, scenario assertion | Covered |
| AC-2 (fc_ekf: estimate ≤50 m of FC EKF snapshot; FDR `source="fc_ekf"`) | `test_evaluate_fc_ekf_passes`, scenario assertion | Covered |
| AC-3 (no origin → SUT refuses takeoff; FDR `cold_start_origin.unavailable`) | `test_evaluate_no_origin_passes_when_silent_and_fdr_records_abort`, `test_evaluate_no_origin_fails_when_sut_emits_anything`, `test_evaluate_no_origin_fails_when_fdr_missing_unavailable_signal`, dedicated scenario `test_ft_p_11_cold_start_no_origin_aborts` | Covered |
| AC-4 (bounded-delta conflict: operator wins; source_label != satellite_anchored; FDR `gps_bounded_delta.reject`) | `test_evaluate_bounded_delta_conflict_operator_wins`, `test_evaluate_bounded_delta_fails_when_label_is_satellite_anchored`, scenario assertion (third parametrize variant) | Covered |
| AC-5 (parameterization across `fc_adapter, vio_strategy, origin_source`) | scenario uses conftest's `fc_adapter` + `vio_strategy`; parametrizes `origin_source ∈ {operator_manifest, fc_ekf, bounded_delta_conflict}` separately | Covered |

ADR-010 Principle #11 amended ("operator origin wins on bounded-delta
conflict; FC GPS logged as suspect") explicitly encoded as
`BOUNDED_DELTA_TRIGGER_M = 200.0` + the `c5.gps_bounded_delta.reject`
record audit.

### Phase 3 — Code Quality

* **Single responsibility**: `mavproxy_tlog_reader` only iterates/counts
  tlog frames (file I/O concern); `ap_contract_evaluator` only consumes
  `TlogMessage` iterables (analytics concern); `msp_frame_observer`
  only consumes captured MSP samples. `cold_start_evaluator` is one
  module because the three FT-P-11 variants share a single FDR record
  vocabulary + Manifest schema; splitting them would force the scenario
  to import three near-identical modules.
* **No suppressed errors**: `mavproxy_tlog_reader.iter_messages`
  catches the narrow `BAD_DATA` + per-message `to_dict` exceptions
  (documented in pymavlink) and continues, but the file-not-found
  + connection-close paths raise / surface naturally. No bare `except`
  in any new module.
* **AAA comment discipline**: every test uses `# Arrange / # Act /
  # Assert`; sections omitted when not needed.
* **No narration comments**: docstrings explain non-obvious intent
  (AC mapping, why orphans excluded, why `materialize_to_list` exists,
  why `EK3_SRC1_POSXY = 3` is the only acceptance value).

### Phase 4 — Security

* **No SUT imports**: confirmed by `test_no_sut_imports.py` (passing in
  the full suite). None of the new modules import from
  `src.gps_denied_onboard`.
* **Signing handshake stance**: the helper does NOT validate signatures
  itself (that's pymavlink's job); it only counts signed-frame arrivals
  and `BAD_SIGNATURE` STATUSTEXT incidents. If signing fails in any way
  AC-1 fails — the scenario does NOT bypass to an unsigned channel
  (per spec "Forbidden" list).
* **No secrets in source**: the AP scenario looks up
  `mavlink-test-passkey.txt` from the on-disk fixture (already
  verified by `test_passkey_files_match` in `test_directory_layout.py`).
  The passkey itself is the AZ-407 / AZ-408 fixture, NOT a production
  key.
* **No SQL/shell injection surface**: all helpers operate on bytes /
  pathlib / dict; no subprocess calls in the helper layer (subprocess
  for `msp_gps_toy` is the SITL-observer's responsibility).

### Phase 5 — Performance

* `mavproxy_tlog_reader.iter_messages` is a single pass over the tlog;
  pymavlink's `recv_match(blocking=False)` is the standard idiom.
* `ap_contract_evaluator` consumes the materialised list ONCE per
  analyser; `collect_messages_to_list` is the documented choice
  (mavlink_connection's iterator closes on exhaustion so re-iteration
  isn't safe). For typical 60 s of mavproxy traffic at ~50 msg/s this
  is ≤3000 messages → trivial in memory.
* `cold_start_evaluator._scan_fdr_for_cold_start` is one pass.
* No nested loops over the same data.

### Phase 6 — Cross-Task Consistency

* **Pattern parity with batches 69 + 70 + 71**:
  - Skip gate (`_*_harness_implemented` fixture) for missing upstream
    replay/SITL/FDR helpers — same pattern as
    `test_ft_p_02/04/05/07/08/10_*`.
  - `_NullSink` probe — same idiom as the prior 5 scenario files.
  - Evidence side-channel via `nfr_recorder.record_metric(name, value,
    ac_id=…)` — same pattern as `test_ft_p_01/04/05/07/08/10_*`.
  - Module-level constants (`UPPER_SNAKE`) for budgets — matches
    `multi_segment_evaluator`, `mre_evaluator`, `smoothing_evaluator`,
    `sharp_turn_detector`.
  - Helper modules importable from `runner.helpers.*`.
* **No drift**: scenarios reuse the helper's constants (no magic
  numbers) — `HANDSHAKE_BUDGET_S`, `GPS_INPUT_MIN_RATE_HZ`,
  `MIN_FIX_TYPE`, `ACCURACY_BUDGET_M`, `BOUNDED_DELTA_TRIGGER_M`,
  `FDR_RECORD_*`, `FORBIDDEN_FIRST_LABEL_BOUNDED_DELTA`.
* **No legacy NotImplementedError test left behind**: verified no test
  asserts `iter_messages` raises NotImplementedError (was AZ-406's
  surface contract; AZ-416 owns the body per docstring).

### Phase 7 — Architecture Compliance

* **Public-boundary discipline**: confirmed by `test_no_sut_imports.py`
  (passing). Helpers consume pymavlink (a third-party MAVLink
  reference impl, not SUT internals) + FDR record schema (record_type
  + payload dict) + outbound estimate schema. The signing handshake
  observer specifically does NOT import the SUT's signing-key state
  per the spec "Forbidden" list.
* **Directory layout**: new paths added to `test_directory_layout.py`
  parametrize list (`runner/helpers/{msp_frame_observer,
  ap_contract_evaluator, cold_start_evaluator}.py`,
  `tests/positive/test_ft_p_{09_ap_signing, 09_inav, 11_cold_start_init}.py`).
  All variants pass.
* **Determinism**: all helpers are deterministic — no `time.time()`,
  no RNG; pymavlink parses bytes deterministically.

### Phase 8 — Test Suite Health

* Total: **460 passed in 134.35 s** (was 393 at end of batch 71).
* New tests this batch: **+67** (msp_frame_observer: 14;
  mavproxy_tlog_reader: 6; ap_contract_evaluator: 22;
  cold_start_evaluator: 19; directory_layout new entries: 6).
* Pre-existing macOS-only `/e2e-results` plugin issue still present —
  affects scenario test invocation outside Docker only; unit suite
  unaffected. Out of batch scope.

## Cross-Task Consistency Verdict

PASS — no cross-task drift, no duplicated logic across the four new
helpers, shared `TlogMessage` type used consistently between
`mavproxy_tlog_reader` and `ap_contract_evaluator`.

## Architecture Compliance Verdict

PASS — public-boundary blackbox stance preserved; no SUT imports;
pymavlink boundary correctly placed at the tlog reader.

## Final Verdict

**PASS** — Batch 72 (AZ-416 + AZ-417 + AZ-419) ready for commit.