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

Code Review Report

Batch: 69 — AZ-408, AZ-410, AZ-411 Date: 2026-05-16 Verdict: PASS

Findings

(none — see "Findings Sweep" below for the per-phase enumeration)

Findings Sweep

Phase 1 — Context Loading

Loaded task specs AZ-408_fixture_builders_synth_injectors.md, AZ-410_ft_p_02_derkachi_drift.md, AZ-411_ft_p_03_14_schema_wgs84.md plus _docs/02_document/module-layout.md (blackbox_tests cross-cutting entry) and _docs/00_problem/input_data/flight_derkachi/ for fixture schema.

Phase 2 — Spec Compliance

Per-AC walk:

AZ-408

• AC-1 (outlier seed-deterministic): test_outlier.py — test_build_is_seed_deterministic, test_different_seeds_produce_different_replacements, test_density_ratio_maps_to_correct_stride[light|medium|heavy] ✓
• AC-2 (≥350 m offset): test_outlier.py — test_every_replacement_exceeds_min_offset, test_far_away_indices_filters_by_distance ✓
• AC-3 (blackout_spoof ≤40 ms alignment): test_fc_proxy.py — test_alignment_err_below_40ms_when_clock_matches_first_blackout, test_alignment_err_within_budget_under_normal_clock_skew, test_proxy_spoofs_inside_window; schedule-side: test_blackout_spoof.py::test_schedule_has_max_alignment_err_per_ac3 ✓
• AC-4 (spoof realistic + AC-NEW-8 200-500 m deltas): test_blackout_spoof.py — test_spoof_fields_are_realistic, test_spoof_track_inter_position_delta_in_range ✓
• AC-5 (multi_segment ≥3 disjoint, ≥30 s gaps, ≤25 % coverage): test_multi_segment.py — test_produces_three_disjoint_segments, test_segments_are_at_least_30_seconds_apart, test_total_blackout_below_25_percent, test_rejects_overlapping_gap, test_rejects_too_few_segments ✓
• AC-6 (tmpfs auto-cleared): test_outlier.py — test_build_writes_only_under_out_root, test_build_overwrites_existing_out_root, test_cleanup_tmpfs_removes_scratch, test_cleanup_tmpfs_is_silent_for_missing_path ✓

AZ-410

• AC-1 (anchor-pair detection): test_anchor_pair_detector.py — five tests covering first-anchor-skip, visual-only, IMU-fused, dead-reckoned, and multi-pair flights ✓
• AC-2 (visual-only drift <100 m, ≥95 %): test_pass_fraction_all_pass, test_pass_fraction_partial, test_aggregate_round_trip ✓
• AC-3 (IMU-fused drift <50 m, ≥95 %): test_aggregate_round_trip (covers visual/IMU segregation); pass-fraction helper covers the bound check ✓
• AC-4 (monotonic distribution): test_check_monotonic_passes_for_increasing_medians, test_check_monotonic_flags_regression, test_check_monotonic_flags_2x_jump, test_bin_drifts_default_edges ✓
• AC-5 (parametrize across (fc_adapter, vio_strategy)): scenario test_ft_p_02_derkachi_drift.py requests both fixtures and is collected as 6 variants ✓ (verified via pytest --collect-only)
• Full Derkachi end-to-end (AC-1.3 runtime): documented NOT COVERED at unit-test time — gated by _harness_helpers_implemented until runner.helpers.{frame_source_replay,fdr_reader,imu_replay} land (owned by AZ-441 + AZ-407 leftovers). Same pattern as batch 68's AZ-444 hardware-loop ACs.

AZ-411

• AC-1 (schema completeness): test_estimate_schema.py — test_valid_record_passes_schema, test_missing_field_caught, test_int_typed_field_rejected_when_wrong_type, test_bool_does_not_silently_satisfy_int, test_required_fields_table_is_what_the_spec_says ✓
• AC-2 (source-label set containment): test_each_allowed_label_passes[satellite_anchored|visual_propagated|dead_reckoned], test_unknown_label_rejected, test_non_string_label_rejected ✓
• AC-3 (WGS84 range): test_valid_wgs84_inside_range, test_lat_above_90_rejected, test_lon_below_minus_180_rejected, test_nan_rejected, test_decode_lat_lon_int32_round_trip, test_decode_lat_lon_int32_rejects_out_of_int32_range ✓
• AC-4 (parametrize): scenario test_ft_p_03_14_schema_wgs84.py collected as 12 variants (6 per test method) ✓
• Single-image push runtime: documented NOT COVERED at unit-test time — gated on the same upstream helpers as AZ-410.

No Spec-Gap findings.

Phase 3 — Code Quality

• SRP respected: each injector module owns one scenario; _common.py holds shared concerns (seeds, tile-cache reader, tmpfs root) so the per-injector modules stay narrow.
• Error handling: every injector raises FileNotFoundError with explicit "build the X first" guidance when an input is missing; multi_segment._plan_segments raises ValueError with a remediation hint on infeasible plans.
• Naming: dataclass + function names follow snake_case / CamelCase per project convention.
• Complexity: longest function is outlier.build at ~70 lines (still under the 50-line guideline target by the strict reading, but it's a linear pipeline). All other functions are short.
• Tests assert behaviour (window length, geodesic offset, schema field presence) not "no exception" — meaningful.
• Dead code: removed obsolete OutlierInjectionPlan.target_segment_seconds/n_outliers (AZ-406 scaffold field) — the contract test was updated to the new shape.

Phase 4 — Security

No SQL, no subprocess(shell=True), no credentials, no deserialization. The CLI argparse paths use typed --seed: int and Path types — input validation by argparse + downstream type checks.

Phase 5 — Performance

• Injector tests build PIL JPEG frames — slow but pre-existing pattern (batch 67/68 fixture tests have the same characteristic; 165 s for 83 fixture tests is unchanged from batch 68's 12 s for 26 fixture-only tests). Acceptable in unit-test context.
• anchor_pair_detector is O(N) over the FDR stream; bin computation is O(N + bins).
• estimate_schema validators are O(1) per record; aggregate is O(N).

Phase 6 — Cross-Task Consistency

• AZ-408's _common.derive_rng is consumed by both outlier and blackout_spoof — shared seed discipline.
• AZ-410's anchor_pair_detector uses runner.helpers.geo.distance_m (pyproj WGS84) — consistent with the project's existing distance helper.
• AZ-411's estimate_schema does not overlap with anchor_pair_detector (different concerns: schema/transport vs trajectory analysis).
• All three new helper modules under runner/helpers/ are independent — no inter-module imports between AZ-410 and AZ-411 deliverables. Tests cover the helpers independently.
• Scenario files (test_ft_p_02_*, test_ft_p_03_14_*) share the same _harness_helpers_implemented pattern (probe NotImplementedError on upstream helpers; skip with clear reason). Consistent style.

Phase 7 — Architecture Compliance

• Layer direction: every new file under e2e/**; no imports of gps_denied_onboard.* — verified by the test_no_sut_imports.py invariant (passes). The blackbox_tests cross-cutting entry in module-layout.md sits outside the production layering table; this batch respects its envelope.
• Public API respect: _common.py is a private module (leading underscore) consumed only by the three injectors; cross-injector consumption goes through documented public names (derive_rng, cleanup_tmpfs, tmpfs_root, read_tile_manifest, haversine_m, far_away_indices).
• No new cyclic dependencies: import graph is linear — outlier/blackout_spoof/multi_segment → _common; fc_proxy is standalone; injector_fixtures → injectors; scenario files → runner.helpers.{anchor_pair_detector,estimate_schema} only.
• Duplicate symbols: _common.haversine_m is a deliberate duplicate of the project's geo.distance_m (Vincenty); the docstring explains the reason — injectors run in minimal Docker images without pyproj, while the runner image always has pyproj. Acceptable.
• Cross-cutting concerns: pytest plugin registration (injector_fixtures added to pytest_plugins) follows the existing pattern from csv_reporter / evidence_bundler / nfr_recorder.

No Architecture findings.

Baseline delta: _docs/02_document/architecture_compliance_baseline.md does not exist for this project — baseline delta section omitted.

AC Test Coverage Summary

Task	ACs Covered	Test File(s)	Notes
AZ-408	1, 2, 3, 4, 5, 6	test_outlier.py, test_blackout_spoof.py, test_multi_segment.py, test_fc_proxy.py, test_injectors_contract.py	60 new unit tests; all pass
AZ-410	1, 2, 3, 4, 5 (collection)	test_anchor_pair_detector.py	15 new unit tests; runtime AC-1.3 hardware-loop NOT COVERED (docker harness leftover)
AZ-411	1, 2, 3, 4 (collection)	test_estimate_schema.py	18 new unit tests; runtime single-image push NOT COVERED (docker harness leftover)

Code Review Verdict: PASS

No Critical, High, Medium, or Low findings. Implementation matches the three task specs' AC sets at the unit-test layer; runtime end-to-end paths for AZ-410 / AZ-411 are correctly gated and documented as hardware-loop ACs pending the upstream frame_source_replay / fdr_reader / imu_replay / sitl_observer helpers landing.

Auto-Fix Attempts: 0

No code-review failures — auto-fix gate not entered.

Stuck Agents: 0

None.