# 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 `ImuSample`s, 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).