# Code Review Report

**Batch**: 77 — AZ-597 (replay_mode helpers + 13 scenario stub rewires)
**Date**: 2026-05-17
**Verdict**: PASS

## Findings

(none)

## Findings Sweep

### Phase 1 — Context Loading

Read the AZ-597 task spec, the `FrameSink` / `FcInboundEmitter`
Protocol definitions in `frame_source_replay.py` and `imu_replay.py`
(b74) to verify the new `Null*` implementations match the exact
method signatures, the `outlier_tolerance_evaluator.GtPose` dataclass
shape consumed by FT-N-01, and the surface used by FT-P-03/14's
`_push_single_image_and_observe` return tuple. Re-read the b75
`sitl_observer.replay_dir` env-var resolution pattern for symmetry
(`E2E_SITL_REPLAY_DIR`, empty-string-as-None semantics).

### Phase 2 — Spec Compliance

| AC | Coverage | Status |
|----|----------|--------|
| AC-1 (`NullFrameSink.write_frame` / `NullFcInboundEmitter.emit` are pure counters) | `test_null_frame_sink_counts_writes`, `test_null_frame_sink_starts_at_zero`, `test_null_emitter_counts_emits`, `test_null_emitter_starts_at_zero` | Covered |
| AC-2 (`load_replay_json` raises `FileNotFoundError` env-unset + file-missing; `ValueError` malformed; round-trips otherwise) | `test_load_replay_json_raises_when_env_unset`, `test_load_replay_json_raises_when_env_empty`, `test_load_replay_json_raises_when_file_missing`, `test_load_replay_json_raises_on_malformed_json`, `test_load_replay_json_round_trips_dict`, `test_load_replay_json_round_trips_list` | Covered |
| AC-3 (`resolve_replay_subdir` raises `FileNotFoundError` env-unset + subdir-missing; returns Path otherwise) | `test_resolve_replay_subdir_raises_when_env_unset`, `test_resolve_replay_subdir_raises_when_subdir_missing`, `test_resolve_replay_subdir_returns_path_when_exists`, `test_resolve_replay_subdir_rejects_file_at_path` | Covered |
| AC-4 (`default_frame_period_ms()` returns 33; documented) | `test_default_frame_period_ms_is_30_fps` (asserts both function + constant); module docstring documents 30 fps default | Covered |
| AC-5 (13 scenarios have local `_resolve_*` / `_drive_*` / `_push_*` stubs deleted; import from `runner.helpers.replay_mode`) | Verified by `Grep raise NotImplementedError under e2e/tests` returning **no matches**. The 13 scenarios touch: FT-P-01/02/04/05/07/08/09-AP/09-iNav/10/11, FT-N-01/02/03/04, and FT-P-03/14. | Covered |
| AC-6 (≥6 unit tests for `replay_mode.py`) | 17 tests total (2 null-sink, 2 null-emitter, 1 frame-period, 2 imu-replay-noop, 6 load_replay_json, 4 resolve_replay_subdir) | Covered (exceeds floor) |
| AC-7 (full suite passes) | 626 passed (+18 from 608; +17 new replay_mode tests + 1 new directory-layout parametrize entry) | Covered |

### Phase 3 — Code Quality

* **Single responsibility**: each surface in `replay_mode.py` owns
  exactly one concern.
  * `NullFrameSink` / `NullFcInboundEmitter` — Protocol-compatible
    counter sinks. No I/O, no JSON, no env-var reads. Pure data.
  * `default_frame_period_ms` — constant lookup. Trivial; lives in
    the same module as the constant it wraps so callers see the
    rationale next to the value.
  * `imu_replay_noop` — explicit no-op with a comment explaining
    why the IMU CSV is ignored in replay mode. Signature mirrors
    `imu_replay.ImuReplayer.replay` so a future live-mode driver
    can be slotted in.
  * `load_replay_json` / `resolve_replay_subdir` — two file-system
    surfaces, distinct contracts ("file must exist + parse" vs
    "directory must exist"). Both go through one shared
    `_resolve_replay_root_or_raise` so env-var semantics are
    enforced exactly once.
* **No suppressed errors**:
  * `load_replay_json` converts `json.JSONDecodeError` → `ValueError`
    with the offending file path AND `raise … from exc` preserves
    the original.
  * `_resolve_replay_root_or_raise` includes the calling surface in
    the error message (`"load_replay_json('foo.json'): ${ENV} not set"`)
    so a test author seeing the failure knows exactly which scenario
    fired which loader.
  * No bare `except`, no `2>/dev/null`, no empty `pass`.
* **AAA comment discipline**: all 17 new tests use
  `# Arrange / # Act / # Assert`; sections omitted when not needed.
* **Code comments**: only the module docstring narrates "why
  replay-mode no-ops are correct". Per-function docstrings document
  contracts and the live-mode follow-up. No line-narration.
* **Public boundary**: `replay_mode.py` imports stdlib only
  (`json`, `os`, `pathlib`). Zero `from gps_denied_onboard ...` imports.

### Phase 4 — Security

* **No new credentials, secrets, or network surfaces**. All work is
  in-process counter state + file I/O over a controlled env-var-rooted
  path.
* **`E2E_SITL_REPLAY_DIR`** read consistently with b75/b76 (set →
  use; unset / empty / whitespace → treated as absent). No
  shell-injection surface — the path is fed straight into `Path`
  arithmetic.
* **No `eval`, `exec`, `pickle`, `subprocess`, or
  `yaml.load(unsafe=True)`** in the new module.
* **JSON parse is pure stdlib** with explicit error wrapping. No
  schema validation — that's the caller's job (the scenarios that
  consume the parsed payload validate field types at the call site).

### Phase 5 — Performance

* All surfaces are O(1) or O(N) where N is the input JSON size
  (single `json.loads` call). No file I/O at module-import time.
* `NullFrameSink` / `NullFcInboundEmitter` are constant-time per call
  with single integer increment + no allocations.

### Phase 6 — Cross-Task Consistency

* **Env-var pattern matches b75 (`sitl_observer.replay_dir`) and b76
  (`fc_proxy_runtime._resolve_replay_dir`)**: same env var, same
  "empty string → None" semantics, same lazy resolution per call.
  The three modules deliberately do not share a helper — each owns
  its own resolution so the import graph stays flat
  (`replay_mode` ↛ `sitl_observer`, `replay_mode` ↛ `fc_proxy_runtime`).
  The cost is ~12 lines of duplicate env-var code across three
  modules; the benefit is no cross-dependency surface.
* **Skip-gate interaction**: the b75 `sitl_replay_ready` fixture
  still skips before any of these loaders fire in unit-test mode.
  When the SITL replay fixture builder lands and the env var is set,
  scenarios will reach the loaders — at which point the explicit
  `FileNotFoundError` messages ("replay fixture 'gt_per_frame.json'
  not found at …") provide a precise pointer to which fixture file
  is missing.
* **`FileNotFoundError` / `ValueError` discipline matches the rest
  of `e2e/runner/helpers/`** (b73-b76 cumulative): missing inputs →
  `FileNotFoundError`, malformed inputs → `ValueError` with a file
  pointer.
* **Scenario-side import convention**: every rewired stub imports
  inside the function body, not at module top-level. This matches
  the existing scenario convention (`from runner.helpers import …`
  is deferred so that `pytest --collect-only` doesn't pay the import
  cost). 13 scenarios, one pattern.
* **`_push_single_image_and_observe` and `_resolve_gt_per_frame`
  field-name discipline**: both load JSON and project into a
  scenario-local dataclass / tuple. The JSON keys (`frame_idx`,
  `lat_deg`, `lon_deg`, `record`, `source_label`) match exactly what
  the evaluators / consumers downstream already expect — no schema
  translation layer required.

### Phase 7 — Architecture Compliance

* **Module placement**: `e2e/runner/helpers/replay_mode.py` (new)
  + `e2e/_unit_tests/helpers/test_replay_mode.py` (new). Both
  registered in `e2e/_unit_tests/test_directory_layout.py`; the
  layout invariant test still passes.
* **No `src/gps_denied_onboard` imports** anywhere. Confirmed.
* **No new top-level dependencies** — stdlib only. `requirements.txt`
  untouched.
* **Backwards-compatible scenario contract**: every `_resolve_*` /
  `_drive_*` / `_push_*` keeps its original name + signature + return
  type. The 13 rewires are body-only changes — no call site changes
  in the scenario test functions themselves.

## Test Results

* New unit tests: **17** (2 null-sink + 2 null-emitter + 1
  frame-period + 2 imu-replay-noop + 6 load_replay_json + 4
  resolve_replay_subdir).
* Full `e2e/_unit_tests` suite: **626 passed in 127 s** (previous
  cumulative: 608 → +18 net = +17 new replay_mode tests + 1 new
  directory-layout parametrize entry).
* No new linter errors (`ReadLints` clean on `replay_mode.py`,
  `test_replay_mode.py`, `test_directory_layout.py`, and all 13
  rewired scenario files).
* `Grep raise NotImplementedError` under `e2e/tests/` returns **no
  matches** — confirming AC-5 (every scenario stub deleted).