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

Code Review Report

Batch: 74 — AZ-594 (harness stubs: core three) Date: 2026-05-17 Verdict: PASS

Findings

(none)

Findings Sweep

Phase 1 — Context Loading

Read the AZ-594 task spec, then the three stub files in their pre-implementation state (fdr_reader.py, frame_source_replay.py, imu_replay.py). Cross-checked the SUT-side FdrRecord wire schema in src/gps_denied_onboard/fdr_client/records.py to confirm field names (schema_version, ts, producer_id, kind, payload, extra) and the rename strategy the runner-side FdrRecord duplicates (kind → record_type, ts → monotonic_ms). Re-read the existing scenario _harness_helpers_implemented probes in the batch 71-73 scenario files to confirm the post-fix probe semantics.

Phase 2 — Spec Compliance

AC	Coverage	Status
AC-1 (fdr_reader.iter_records parses every *.jsonl under root, yields FdrRecord ordered by monotonic_ms; raises FileNotFoundError on missing root)	test_missing_root_raises_file_not_found, test_empty_root_yields_nothing, test_single_file_round_trip, test_multiple_files_are_merged_and_sorted, test_subdirectory_files_included, test_payload_passed_through; envelope-validation tests cover the wire-schema gate (test_missing_envelope_key_raises, test_non_object_line_raises, test_malformed_json_raises, test_empty_producer_id_raises, test_ts_iso_without_z_parses, test_blank_lines_are_skipped); archive_size_bytes covered by test_archive_size_bytes_sums_all_files and test_archive_size_bytes_missing_root_returns_zero	Covered
AC-2 (frame_source_replay.replay_video accepts .mp4 OR directory; decodes via OpenCV; emits via sink at cadence; raises FileNotFoundError on missing input)	test_video_missing_path_raises_file_not_found, test_video_dir_delegates_to_image_directory, test_video_mp4_round_trip; image-dir mode covered by test_image_dir_missing_raises_file_not_found, test_image_dir_empty_returns_zero, test_image_dir_emits_sorted_by_name, test_image_dir_non_jpeg_reencoded, test_image_dir_skips_non_image_files, test_image_dir_non_realtime_does_not_sleep, test_image_dir_realtime_sleeps_per_frame	Covered
AC-3 (imu_replay.ImuReplayer.replay parses CSV, constructs ImuSamples, calls emitter; tolerates scientific-notation floats)	test_happy_path_emits_all_rows, test_scientific_notation_parses, test_attitude_radians_converted, test_realtime_sleeps_per_sample, test_non_realtime_does_not_sleep, test_empty_csv_emits_nothing; error paths: test_missing_csv_raises_file_not_found, test_rate_hz_must_be_positive, test_missing_required_columns_raises, test_row_with_unparseable_value_raises	Covered
AC-4 (≥5 unit tests per body — happy + 2 error + 1 boundary)	fdr_reader: 14 tests; frame_source_replay: 10 tests; imu_replay: 10 tests	Covered (exceeds floor)
AC-5 (full suite passes)	558 passed (+31 from 527 baseline)	Covered

Phase 3 — Code Quality

• Single responsibility:
  • fdr_reader owns wire-envelope parsing + projection to the runner-side dataclass. It does NOT validate per-kind payload keys — that's left to the consuming evaluators, matching the AZ-272/273 contract that documents payload as opaque on the runner side.
  • frame_source_replay.FrameSourceReplayer owns frame decode + sink emission. Cadence pacing is parameterised via sleep_fn for testability; OpenCV is the only third-party import.
  • imu_replay.ImuReplayer owns CSV parse + emitter invocation. Cadence pacing same pattern as frame_source_replay. Row parsing is a pure function _parse_row so missing/malformed columns surface as a ValueError with a file + line pointer.
• No suppressed errors:
  • fdr_reader._parse_envelope raises ValueError (with file + line) on every malformed envelope branch; json.JSONDecodeError propagates naturally for unrecoverable JSON corruption.
  • frame_source_replay._decode_and_emit_video raises explicit ValueError if OpenCV cannot open the file or fails to encode a frame — never silently skips a frame.
  • imu_replay._parse_row re-raises KeyError/ValueError wrapped as a contextual ValueError; the original __cause__ is preserved via raise ... from exc.
  • No except Exception: pass, 2>/dev/null, or empty except blocks.
• AAA comment discipline: every new test uses `# Arrange / # Act /

  Assert`; sections omitted when not needed.

• No code comments narrating what code does — only the module-level docstrings explain the wire-vs-runner schema rename, the OpenCV realtime/non-realtime distinction, and the scientific-notation tolerance rationale.
• Public boundary: confirmed all three modules import only stdlib (csv, json, math, time, pathlib, datetime) + cv2. No from gps_denied_onboard ... anywhere.

Phase 4 — Security

• No new credentials, secrets, or network surface. All three helpers are deterministic file-I/O over caller-supplied paths.
• Wire-schema gate in fdr_reader._parse_envelope is the safety invariant — if the SUT FDR schema drifts, the runner blows up at parse time with a file+line pointer rather than silently producing records with default-zero fields. This is the explicit drift-detection rationale documented in the FdrRecord docstring.
• No eval, exec, pickle, or subprocess in any of the three modules.

Phase 5 — Performance

• All three implementations are O(N) over their input streams (JSONL lines, frames, CSV rows). Sorting in iter_records is the one O(N log N) step — necessary for the multi-file merge-sort guarantee, and acceptable since the typical archive is bounded by the AZ-441 50 GB / 8 h budget.
• No file I/O at module-import time.
• FrameSourceReplayer._decode_and_emit_video always re-encodes frames to JPEG (lossy double-encode if source was MP4). The intent is so the sink always receives JPEG bytes regardless of source format — matches the SUT's ONBOARD_FRAME_SOURCE_PATH expectation of file-system-readable JPEG frames.

Phase 6 — Cross-Task Consistency

• Sleep injection pattern: frame_source_replay.FrameSourceReplayer and imu_replay.ImuReplayer both accept sleep_fn as a keyword argument defaulting to time.sleep. Tests pass lambda _: None or a recording stub. Single pattern across the two replay surfaces.
• realtime vs non-realtime flag: both replayers default to realtime=True and skip the sleep when False. Consistent semantics so that future scenarios can choose between wall-clock replay (for live-FC tests) and fast replay (for FDR/evidence-only tests).
• FileNotFoundError: all three new bodies surface missing input paths via FileNotFoundError, consistent with how the existing helpers accuracy_evaluator, multi_segment_evaluator, mavproxy_tlog_reader, cold_start_evaluator handle missing inputs.
• Scenario probe interaction: the existing _harness_helpers_implemented probes in batches 71-73 (FT-P-07, FT-P-08, FT-P-10, FT-P-11, FT-N-01, FT-N-02, FT-N-03, FT-N-04) pass /tmp/non-existent to each helper. Previously the inner except NotImplementedError: return False fired; now the outer except Exception: return False catches the new FileNotFoundError and still returns False. The scenarios continue to skip — which is correct because the OTHER probe-gated helpers (sitl_observer, mavproxy_tlog_reader for some scenarios, fc_proxy_runtime) are still pending. Probe cleanup is explicitly excluded from AZ-594 scope and will happen once the remaining harness stubs land.

Phase 7 — Architecture Compliance

• Module placement unchanged: existing files at e2e/runner/helpers/{fdr_reader,frame_source_replay,imu_replay}.py were edited in place (bodies replace NotImplementedError); no layout changes. New unit-test files in e2e/_unit_tests/helpers/. Layout invariant test still passes — these helpers were already listed.
• No src/gps_denied_onboard imports anywhere. Verified.
• Existing scenario surface preserved: FdrRecord dataclass field names and types are unchanged from the AZ-406 contract. ImuSample likewise. ReplayCadence and FrameSink Protocol unchanged. Consumers in batches 71-73 stay valid.

Test Results

• New unit tests: 14 (fdr_reader) + 10 (frame_source_replay) + 10 (imu_replay) = 34 new tests.
• Full e2e/_unit_tests suite: 558 passed in 139 s (previous cumulative: 527 → +31 net).
• No new linter errors (ReadLints on all six new/modified files reported clean).