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

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.