# Code Review Report

**Batch**: 80 — AZ-600 (refactor sitl_replay_builder to strategy pattern)
**Date**: 2026-05-17
**Verdict**: PASS

## Findings

(none blocking)

### Non-blocking notes

* **AC-2 / AC-3 line-count target (≤80 lines) slightly missed.** The
  refactored `build_p01_fixtures.py` lands at ~107 lines and
  `build_p02_fixtures.py` at ~98 lines. The bulk of the overage is
  module-level docstring (14 lines), imports (16 lines), and the
  argparse CLI (~25 lines) — all of which the AC implicitly allowed
  ("only contains: image-glob helper, scenario config factory, CLI
  wrapper") but didn't budget. The refactor still achieves a ~75 %
  size reduction vs. the 423-line / 292-line pre-refactor versions,
  which is the substantive win. Tightening to ≤80 would require
  inlining the docstring or stripping argparse help text — both hurt
  readability more than they're worth. Recording the actual numbers
  here so future scenarios can hit a realistic ceiling (~120 lines).

* **`_common.py` deleted in favour of `builder.py`.** AZ-599's AC-5
  ("`run_gps_denied_replay` is in `_common.py`") used file location as
  a proxy for "the helper is shared, not duplicated". The refactor
  preserves the *intent* — the helper is now in `builder.py` and
  imported by both scenarios. AZ-599 stays satisfied; only the
  file location wording is stale. Updated `test_directory_layout.py`
  to reference `builder.py`. The previous re-export-from-`_common`
  guard (`test_common_module_exports_used_by_b01`) is dropped —
  its replacement is the broader "both scenarios import from
  `builder.py`" pattern enforced by `test_sitl_replay_builder*.py`
  integration tests.

* **`Mp4PassthroughSource.materialize` returns the input path, not
  the orchestrator-supplied output_path.** This is intentional —
  there is no value in copying or re-encoding an already-correct MP4.
  The orchestrator's downstream `run_gps_denied_replay` call receives
  the real MP4 path, which `test_build_fixtures_uses_passthrough_video`
  pins. Documented in the ABC docstring so future strategies follow
  the same convention: "either write a new file at output_path (and
  return output_path) or pass through an already-existing MP4
  (returning its real location, ignoring output_path)."

* **Roll/pitch=0 + `SCALED_IMU2` pass-through limitations** still
  apply (unchanged from b79 review). Documented in README.

## Findings Sweep

### Phase 1 — Context Loading

Read the existing `_common.py`, `build_p01_fixtures.py`,
`build_p02_fixtures.py`, both test files, and `test_directory_layout.py`
to inventory the actual duplication and the test contracts that had
to be preserved. Confirmed `.gitignore` already excludes generated
`*.mp4` / `*.tlog` / `fdr_output/` so no generated-fixture cleanup
was needed (user's secondary ask was a non-issue).

### Phase 2 — AC Verification

* **AC-1** ✓ `builder.py` exports `VideoSource`, `TlogSource`,
  `FdrProjection` ABCs + four concrete impls (`StillImagesSource`,
  `Mp4PassthroughSource`, `SyntheticStationaryTlog`, `ImuCsvTlog`,
  `RawFdrPassthrough`, `OutboundMessagesProjection`) + a
  `build_fixtures(cfg)` orchestrator and `FixtureBuilderConfig`
  dataclass. Verified by direct module read; covered by
  `test_sitl_replay_builder_builder.py`.
* **AC-2** ◑ `build_p01_fixtures.py` is 107 lines (target was ≤80).
  See non-blocking note above; substantive contract ("only contains:
  image-glob helper, scenario config factory, CLI wrapper") is met.
* **AC-3** ◑ `build_p02_fixtures.py` is 98 lines (target was ≤80).
  Same caveat.
* **AC-4** ✓ `pack_raw_imu` + `pack_attitude` are single parameterized
  helpers; `SyntheticStationaryTlog` calls
  `pack_raw_imu(time_us, zacc=STATIONARY_Z_ACCEL_MG)`, `ImuCsvTlog`
  calls `pack_raw_imu(time_us, xacc=..., yacc=..., zacc=...,
  xgyro=..., ygyro=..., zgyro=...)`. The previous
  `_pack_raw_imu_zero` / `_pack_raw_imu` pair is gone. Sanity-checked
  by `test_pack_raw_imu_returns_nonempty_bytes` /
  `test_pack_attitude_returns_nonempty_bytes`.
* **AC-5** ✓ Tests reorganized: 33 strategy-level tests in
  `test_sitl_replay_builder_builder.py`, 6 FT-P-01 integration tests
  in `test_sitl_replay_builder.py`, 7 FT-P-02 integration tests in
  `test_sitl_replay_builder_p02.py`. All AC-1..AC-5 behaviors of
  AZ-598/AZ-599 still validated.
* **AC-6** ✓ Full `e2e/_unit_tests` suite: **700 passing** (up from
  686 baseline; +14 net from more granular strategy coverage and
  added `Mp4PassthroughSource` + `build_fixtures` orchestrator
  tests). Single run, no flakes, 135 s.
* **AC-7** ✓ README §"Adding a new scenario (worked example: FT-P-04)"
  shows a ~30-line config factory that composes
  `Mp4PassthroughSource + ImuCsvTlog + RawFdrPassthrough` with no
  new strategy code.

### Phase 3 — Code Quality / Lint

`ReadLints` on all six modified/new files: no errors.

### Phase 4 — Coding-rule Compliance

* SRP: each strategy class owns one materialization concern;
  scenario modules own only scenario-specific config factories;
  `build_fixtures` is a thin orchestrator. ✓
* No silent error suppression: every `try/except` either re-raises
  with context (`malformed IMU CSV row at ...`, `malformed FDR JSON
  at ...:N`) or short-circuits a strictly-tolerable case
  (`verify_fdr_has_estimates` skipping JSON-decode errors on
  individual lines, which is documented). ✓
* No comment narration; comments only on non-obvious invariants
  (the STATIONARY_Z_ACCEL_MG constant explains the gravity
  encoding). ✓
* Existing project patterns reused (subprocess wrapper signature
  unchanged; `mavutil.mavlogfile(write=True)` factory unchanged;
  fixture file naming `<artifact>_<fc_kind>_<host>.json` unchanged).
  ✓
* Scope discipline: edits confined to
  `e2e/fixtures/sitl_replay_builder/` +
  `e2e/_unit_tests/fixtures/` + the single
  `test_directory_layout.py` entry that referenced the deleted
  `_common.py`. ✓
* Deleted-file gate: `_common.py` had two consumers
  (`build_p01_fixtures.py`, `build_p02_fixtures.py`) plus a test
  reference. All three updated in the same batch. No external
  consumers (grep confirmed). ✓

### Phase 5 — Regression Gate

Full `e2e/_unit_tests` suite: 700 passed in 135.32 s, single run.
Baseline was 686; the +14 delta reflects:

* +3 net strategy-level tests (extra `Mp4PassthroughSource`
  happy-path + missing-file; extra `RawFdrPassthrough` skip-verify
  path; extra `OutboundMessagesProjection` length-mismatch test
  that was implicit before).
* +2 `build_fixtures` orchestrator tests.
* +2 `pack_raw_imu` / `pack_attitude` parametric tests.
* +2 `resolve_p01_image_paths` / `resolve_derkachi_inputs` tests.
* +5 misc (slightly finer-grained coverage of duplicated helpers
  now that each lives in one place).

No tests removed without an equivalent replacement; the
`test_common_module_exports_used_by_b01` was retired because the
"shared helper" pattern it pinned is now structural rather than
file-location-based.