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

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.