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

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).