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

Code Review Report

Batch: 79 — AZ-599 (FT-P-02 Derkachi fixture builder + b78 helper extraction) Date: 2026-05-17 Verdict: PASS

Findings

(none blocking)

Non-blocking notes

• Roll/pitch=0 simplification in synthesised ATTITUDE: acceptable for the Derkachi fixed-wing cruise data but unrealistic for aggressive manoeuvres. Documented as a Limitation in the README; re-visit if FT-P-04 (Derkachi F2F registration) or other manoeuvre-sensitive scenarios show fusion drift from this gap.
• SCALED_IMU2 → RAW_IMU pass-through (no unit conversion): the builder passes scaled accelerometer (mg) and gyro (mrad/s) values straight into RAW_IMU fields. Documented as a Limitation; if the SUT's tlog parser rejects, a units conversion pass needs to be added. Surfaced as a follow-up rather than fixed speculatively.
• Disk-full incident during regression gate: not a code issue; surfaced to the user, who freed space, after which the full suite (686 tests) passed cleanly.

Findings Sweep

Phase 1 — Context Loading

Read the FT-P-02 scenario to confirm: it (a) skips when sitl_replay_ready is false; (b) calls fdr_reader.iter_records(fdr_root) to walk the FDR archive; (c) filters for record_type == "estimate" and projects into apd.FdrEstimate. Confirmed FT-P-02 does NOT call wait_for_outbound (so the b78 schema doesn't apply). Read data_imu.csv to confirm column names + units (10 Hz; ms timestamps; SCALED_IMU2 in mg / mrad/s; GLOBAL_POSITION_INT.hdg in centidegrees). Inspected the production replay_input/auto_sync.py AC-13 contract for required tlog message types (RAW_IMU + ATTITUDE) — this drove the choice of which messages to pack per CSV row. Verified pymavlink>=2.4 already in pyproject.toml. Read the b78 build_p01_fixtures.py to identify the helper extraction boundary (run_gps_denied_replay + write_observer_fixture had no FT-P-01-specific code, ideal candidates).

Phase 2 — Spec Compliance

AC	Coverage	Status
AC-1 (parse every row, one pair per row)	test_convert_imu_csv_writes_pair_per_row, test_convert_imu_csv_real_pymavlink_round_trip	Covered
AC-2 (ValueError on missing cols / malformed numerics / empty CSV)	test_convert_imu_csv_empty_raises, test_convert_imu_csv_missing_required_column_raises, test_convert_imu_csv_malformed_numeric_raises, test_convert_imu_csv_missing_file_raises	Covered (4 distinct error paths)
AC-3 (heading centideg → rad)	test_hdg_centideg_to_rad (5 parametric cases: 0, π/2, π, 3π/2, near-2π)	Covered
AC-4 (build_p02 invokes replay + verifies ≥1 estimate)	test_build_p02_end_to_end_with_mocks, test_build_p02_propagates_verify_failure, test_verify_fdr_no_estimates_raises, test_verify_fdr_counts_estimates, test_verify_fdr_tolerates_malformed_lines	Covered
AC-5 (_common.py shared between b78 + b79)	test_common_module_exports_used_by_b01 + b78 suite (24/24) still passes	Covered
AC-6 (full unit-test suite passes)	686/686 in 123 s (previous: 664)	Covered

Phase 3 — Code Quality

• Single responsibility: convert_imu_csv_to_tlog parses CSV + packs MAVLink (one cohesive concern: CSV row → tlog pair). verify_fdr_has_estimates does one thing. build_p02_fixtures is the only orchestrator. Helper extraction into _common.py removed duplicate definitions cleanly — no copy-pasted code remains.
• No suppressed errors: every JSON-decode in verify_fdr_has_estimates is wrapped in a try / except json.JSONDecodeError that explicitly continues with a comment explaining the tolerance (real FDR archives can contain partial writes at the tail). Every CSV parse error wraps in ValueError with row context. subprocess.run uses check=True.
• AAA test discipline: all 20 new tests use # Arrange / # Act / # Assert; omitted when redundant.
• Comments: every public function has a docstring documenting contract + raises; no narrating comments in function bodies. The yaw conversion uses a named helper (_hdg_centideg_to_rad) rather than an inline magic-formula comment.
• Public boundary: b79 module does NOT import any src/gps_denied_onboard symbol (verified by grep). pymavlink imports are deferred to function bodies so the test factory injections don't need the real library available.

Phase 4 — Security

• No new credentials / secrets / network surfaces.
• convert_imu_csv_to_tlog uses csv.DictReader — no eval, no shell injection. Numeric fields go through int(float(s)) so bad input raises ValueError rather than executing arbitrary code.
• verify_fdr_has_estimates uses json.loads (safe) and silently ignores malformed lines (documented as tolerance for partial writes; not a security risk because we never act on the data).
• subprocess invocation uses a list-argument cmd (no shell injection surface; same pattern as b78).

Phase 5 — Performance

• convert_imu_csv_to_tlog is O(N) over CSV rows; for the Derkachi 4,900-row file → ~5 s wall-clock estimate (pymavlink packing is the bottleneck). Bounded by input size; no hidden quadratic behaviour.
• verify_fdr_has_estimates is single-pass O(N) over JSONL lines.
• _iter_imu_rows validates the header once and then streams rows; doesn't materialize the entire CSV in memory until the orchestrator asks for list(...) (could be optimized later if Derkachi data ever grows orders of magnitude larger; currently 4,900 rows is trivial).

Phase 6 — Cross-Task Consistency

• Builder shape parity with b78: BuilderConfig / P02BuilderConfig dataclass; build_pXX_fixtures(cfg, *, _runner=..., _mavlink_writer_factory=..., _verify_fdr=...) signature; same underscore-prefixed dependency-injection convention. A future contributor reading both side-by-side will see the same pattern.
• _common.py extraction validated by both: b78's existing test_run_gps_denied_replay_builds_correct_cmd / test_run_gps_denied_replay_creates_fdr_parent_dir / test_run_gps_denied_replay_passes_extra_args / test_write_observer_fixture_schema tests pass unchanged against the moved implementation. b79 adds test_common_module_exports_used_by_b01 to lock in the import relationship.
• No drift in observer_<fc_kind>_<host>.json schema: same payload structure as b75/b78.

Phase 7 — Architecture Compliance

• Module placement: new files under e2e/fixtures/sitl_replay_builder/ (existing package from b78); new tests under e2e/_unit_tests/fixtures/. All registered in test_directory_layout.py.
• No src/gps_denied_onboard imports.
• No new top-level dependencies — pymavlink already there from b78/b76; csv is stdlib; math is stdlib.
• __init__.py not modified — the b78 docstring already documents why we don't re-export symbols on the package namespace, and that's still the right choice for b79.
• Refactor preserves backwards compatibility: build_p01_fixtures re-imports run_gps_denied_replay + write_observer_fixture from _common.py. Any caller (test or production) doing bp01.run_gps_denied_replay(...) still works because the symbol is present in the module namespace via the from … import … re-export.

Test Results

• New unit tests: 20 (5 convert_imu_csv_to_tlog + 5 _hdg_centideg_to_rad parametric + 4 verify_fdr_has_estimates
  • 4 build_p02_fixtures end-to-end + 1 export-relationship
  • 1 missing-file negative).
• Full e2e/_unit_tests suite: 686 passed in 123 s (previous: 664 → +22 net = +20 new + +2 directory-layout entries).
• No new linter errors (ReadLints clean on all touched files).
• No regression in the b78 builder tests (24/24 still pass after the helper extraction).