# Cumulative Code Review — Batches 61-63 (Cycle 1)

**Date**: 2026-05-14
**Range**:
- batch 61 (AZ-401 + AZ-400 absorbed — `compose_root` replay branch + `MavlinkTransport` Protocol seam)
- batch 62 (AZ-402 — `gps-denied-replay` console-script + shared `runtime_root.main(config)`)
- batch 63 (AZ-404 — E2E replay fixture test + AZ-389 housekeeping; AZ-559 closed Won't Fix)

**Compared against**: previous cumulative review batches 58-60.
**Verdict**: **PASS_WITH_WARNINGS**

## Scope

The 61-63 trio closes the **replay subsystem** end-to-end:

- **Batch 61** wired the `compose_root` replay branch + retrofitted the missing `MavlinkTransport` Protocol seam from AZ-400 (originally specced under AZ-400 but missing when AZ-401 came up; absorbed to unblock the slice).
- **Batch 62** added the `gps-denied-replay` console-script CLI. The shared airborne `main()` was refactored additively to accept a pre-built `Config`, letting the CLI build → mutate → inject without violating ADR-011's "single composition root" constraint.
- **Batch 63** added the E2E test harness against the Derkachi 60 s clip — full AC matrix wired (some ACs deferred behind documented blockers); plus an AZ-389 spec-vs-impl reconciliation that proved the AZ-559 follow-up was unnecessary (the existing `TileStore.write_tile` + `TileSource.ONBOARD_INGEST` + `FreshnessRejectionError` cover the mid-flight ingest path).

The replay slice is now functionally complete on the airborne side: AZ-405 (coordinator) → AZ-401 (compose_root branch) → AZ-402 (CLI) → AZ-404 (E2E test).

## Carry-over status from cumulative review 58-60

| Prior finding | Status | Notes |
|---------------|--------|-------|
| 58-60 F1 (Medium) — Replay contract `ReplayInputAdapter.__init__` missing `fdr_client` | RESOLVED earlier | Contract updated in batch 60; no further work. |
| 58-60 F2 (Low) — Two parallel tlog message-type pre-validators | OPEN — carry forward | Untouched. The AZ-404 e2e fixture's `_tlog_synth.py` produces a tlog that satisfies BOTH validators by construction, so the duplication is observably harmless. |
| 58-60 F3 (Low) — Test-only injection kwargs on `ReplayInputAdapter.__init__` | OPEN — pattern formalised | Batches 61 + 62 + 63 all adopted the same "single optional kwarg defaulting to None, lazy-resolved at call time" pattern (`replay_components_factory` in AZ-401, `shared_main` in AZ-402, `replay_runner` closure in AZ-404). The pattern is now used by **four** coordinators. Recommend factoring to a shared `_TestInjections` helper after a fifth use case (still under threshold). |
| 58-60 F4 (Low) — Two `cv2.projectPoints` calls per Marginals frame | OPEN — carry forward | No code in batches 61-63 touched C4. |
| 58-60 F5 (Low) — AZ-361 AC-11 informational latency comparison | OPEN — carry forward | Informational metric per spec; no action. |
| 55-57 F1 (Low) — engine-output-probe FP32 vs FP16 dtype divergence | OPEN — carry forward | No code in batches 61-63 touched C2 / C3 TRT path. |
| 55-57 F2 (Low) — XFeat underscore-prefixed helper imports | OPEN — carry forward | No movement. |
| 55-57 F3 (Low) — AZ-347 latency benchmark not asserted | OPEN — carry forward | Informational. |
| 52-54 F2 (Low) — c1_vio test fakes not yet shared | OPEN — carry forward | Subsumed under AZ-528 (filed earlier). |

## Findings (this window)

| # | Severity | Category | File:Line | Title |
|---|----------|----------|-----------|-------|
| F1 | High | Spec-Gap | tests/unit/test_az401_compose_root_replay.py:526 + tests/e2e/replay/test_derkachi_1min.py:269-278 | AZ-401 AC-9 + AZ-404 AC-4b both blocked on AZ-558 (C8 encoder routing through `MavlinkTransport`) |
| F2 | High | Spec-Gap | tests/e2e/replay/test_derkachi_1min.py:357-371 | AZ-404 AC-8 (operator workflow rehearsal) blocked on D-PROJ-2 mock-suite-sat-service |
| F3 | Medium | Spec-Gap | tests/e2e/replay/test_derkachi_1min.py:113-148 | AZ-404 AC-3 (≤100m for 80%) `xfail` until real Topotek KHP20S30 calibration ships |
| F4 | Medium | Process | _docs/02_tasks/todo/AZ-389_c5_orthorectifier_c6.md (rewritten) + AZ-559 closed Won't Fix | Three back-to-back replay-track tasks (AZ-401, AZ-389, AZ-404) had upstream-dep gaps; pattern surfaced and was reconciled in this window |
| F5 | Low | Style | src/gps_denied_onboard/cli/replay.py:235-256 + src/gps_denied_onboard/runtime_root/__init__.py:621-660 | Optional-kwarg test-injection pattern adopted by AZ-402's `shared_main` and AZ-401's `replay_components_factory` (cumulative count: 4 coordinators) |
| F6 | Low | Maintainability | tests/e2e/replay/_tlog_synth.py | Synthetic tlog generation from CSV adds a build step to the e2e harness (Derkachi original tlog not in-repo) |

### Finding Details

#### F1: AZ-558 blocks two ACs (High / Spec-Gap)

- **Locations**:
  - `tests/unit/test_az401_compose_root_replay.py:526` (`test_ac9_noop_transport_bytes_written` — `pytest.skip`)
  - `tests/e2e/replay/test_derkachi_1min.py:269-278` (`test_ac4_encoder_byte_equality` — `pytest.skip`)
- **Description**: The C8 outbound adapters (`PymavlinkArdupilotAdapter`, `Msp2InavAdapter`) call `connection.mav.gps_input_send(...)` directly — bytes never flow through the `MavlinkTransport` seam. AZ-558 was filed in batch 61 to close this gap. Until it lands:
  - AZ-401 AC-9 (`NoopMavlinkTransport.bytes_written() > 0` after replay-mode runtime drive) is unsatisfiable.
  - AZ-404 AC-4b (encoder byte-equality between live and replay via `CapturingMavlinkTransport`) is unsatisfiable.
- **Risk-shape note**: AZ-558's spec flags a known-unknown — pymavlink's signing handshake runs inside `mav.*_send`, not at `connection.write` level. A naive seam shape (`MavlinkTransport.write(bytes)`) would skip signing. This may push AZ-558's nominal 3pt → 5pt during implementation; track at task-prep time.
- **Status**: explicit + tracked. The `CapturingMavlinkTransport` infrastructure is in place (with full unit coverage in `test_helpers.py`); when AZ-558 lands, both skips drop in a small follow-up.
- **Suggestion**: prioritise AZ-558 in a near-future batch — it's the single dep that closes two open ACs.

#### F2: AC-8 blocked on D-PROJ-2 mock (High / Spec-Gap)

- **Location**: `tests/e2e/replay/test_derkachi_1min.py:357-371` (`test_ac8_operator_workflow` — `pytest.skip`).
- **Description**: AZ-404's spec calls for the test to run the operator's full C10/C11/C12 pre-flight against a `mock-suite-sat-service` fixture before invoking the replay CLI (replay protocol Invariant 12 + epic AC-9). The current `tests/fixtures/mock-suite-sat-service/` is a bootstrap stub (`GET /healthz` only); the full D-PROJ-2 ingest contract isn't in the parent-suite design yet (`_docs/_process_leftovers/2026-05-09_satellite-provider-design-tasks.md`).
- **Status**: explicit + tracked at parent-suite level. The `operator_pre_flight_setup` fixture in `conftest.py` yields a placeholder cache directory so the test body fails fast with a clear reason rather than a surprise import error.
- **Suggestion**: when the parent-suite D-PROJ-2 design lands, file a separate task to implement the mock and unskip AC-8.

#### F3: AC-3 `xfail` until real calibration (Medium / Spec-Gap)

- **Location**: `tests/e2e/replay/test_derkachi_1min.py:113-148` (`test_ac3_within_100m_80pct_of_ticks` — `pytest.mark.xfail(strict=False)`).
- **Description**: AC-3 is the epic's primary acceptance gate but `_docs/00_problem/input_data/flight_derkachi/camera_info.md` explicitly states the Topotek KHP20S30 intrinsics are unknown. The test is fully implemented, runs against the placeholder `tests/fixtures/calibration/adti26.json`, and reports a real percentage. With wrong intrinsics, the percentage will land near 0%; `xfail(strict=False)` lets a future correct calibration eventually pass without a fail-on-pass surprise.
- **Status**: explicit + tracked at fixture level.
- **Suggestion**: when real KHP20S30 calibration ships, drop the marker (or flip to `strict=True` for one CI run before removing).

#### F4: Three replay-track tasks with upstream-dep gaps (Medium / Process)

- **Pattern**: AZ-401 (missing AZ-400 transport seam), AZ-389 (phantom `put_mid_flight_candidate` API — resolved by re-reading AZ-303's actual surface), AZ-404 (missing tlog fixture, missing real calibration, missing D-PROJ-2 mock, blocker on AZ-558). Three back-to-back replay-track follow-ons each surfaced an upstream gap.
- **Resolution**:
  - AZ-401: absorbed AZ-400's residual scope into the same batch.
  - AZ-389: investigation showed the gap was a spec-vs-impl naming mismatch — `TileSource.ONBOARD_INGEST` + `TileMetadata.quality_metadata` + `write_tile`'s built-in `FreshnessRejectionError` already cover the mid-flight ingest path. AZ-559 closed Won't Fix; AZ-389 spec rewritten to consume the existing API.
  - AZ-404: gaps that CAN be filled in-batch (synthetic tlog, AC-4a AST scan, helper unit coverage) shipped; gaps that CAN'T (real calibration, AZ-558 routing, D-PROJ-2 mock) are explicit `skip` / `xfail` markers with documented reasons + tracker links.
- **Why this is a finding**: the pattern shows that "shipped-tasks-vs-spec" can silently drift across feature boundaries. The mitigation is the **AC-4a mode-agnosticism AST scan** (now passing) — it gives the architecture a live structural-invariant check that fires regardless of `RUN_REPLAY_E2E`. A similar invariant for "every spec'd Protocol method is implemented" would catch future AZ-389-style phantoms; can be a future hygiene ticket.
- **Suggestion**: file a 2pt hygiene PBI to add an AST-level "Protocol completeness" check to the unit suite — for each `runtime_checkable` Protocol, verify each in-repo concrete class implements every method named in the Protocol's source. This catches the AZ-389 phantom-API pattern at task-prep time instead of batch-implementation time.

#### F5: Optional-kwarg test-injection pattern (Low / Style — carry-forward escalation)

- **Locations**:
  - `src/gps_denied_onboard/runtime_root/__init__.py:_compose(...)` accepts a `pre_constructed` kwarg (AZ-401).
  - `src/gps_denied_onboard/cli/replay.py:main(argv, *, shared_main=None)` (AZ-402).
  - `src/gps_denied_onboard/replay_input/tlog_video_adapter.py:ReplayInputAdapter.__init__(..., tlog_source_factory=None, video_frames_factory=None, video_timestamps_factory=None)` (AZ-405).
  - `src/gps_denied_onboard/components/c8_fc_adapter/tlog_replay_adapter.py:TlogReplayFcAdapter.__init__(..., source_factory=None)` (AZ-399).
- **Description**: Four production constructors / functions now accept a single optional kwarg that defaults to `None` and resolves to the production dependency lazily (or `None`-as-passthrough). Tests inject fakes through the kwarg without monkeypatching. Cumulative review 58-60 noted this at three sites; this window adds a fourth.
- **Suggestion**: still under the "factor when fifth case appears" threshold. Track for batch 64+.

#### F6: Synthetic tlog generation (Low / Maintainability)

- **Location**: `tests/e2e/replay/_tlog_synth.py`.
- **Description**: The Derkachi fixture ships `data_imu.csv` (already exported from a tlog) but not the source tlog itself. `_tlog_synth.py` reproduces a `pymavlink.dialects.v20.ardupilotmega` tlog from the CSV — `SCALED_IMU2` + `ATTITUDE` + `GPS_RAW_INT` + `HEARTBEAT`. Deterministic, atomic-write via `.tmp` + fsync + rename, ~1 s for 60 s of data. The conftest synthesizes once per session.
- **Why this is not a Critical**: the alternative (checking the source tlog into the fixture) would add a new ~5 MB binary to `_docs/00_problem/`; the synth approach keeps the fixture content surface narrow + reproducible.
- **Suggestion**: keep. Document in `tests/e2e/replay/README.md` (already done).

## Architecture Observations (this window)

- **ADR-011 holding**: AC-4a's mode-agnosticism AST scan is passing across all `src/gps_denied_onboard/components/**/*.py` files — confirms batches 60 / 61 / 62 / 63 honoured the structural guarantee. If a future batch introduces a `if config.mode` branch in any component, the e2e suite catches it on the next CI run regardless of `RUN_REPLAY_E2E`.
- **Single composition root holding**: the AZ-402 CLI does NOT call `compose_root` directly — it builds a `Config`, calls `runtime_root.main(config)`, and `compose_root` runs inside there. Replay protocol Invariant 11 (CLI MUST NOT compose) verified at the type level.
- **Layer direction holding**: `cli/replay.py` is Layer 5 per `module-layout.md`; imports flow Layer-5 → Layer-4 (`replay_input.errors`) → Layer-1 (`config`, `logging`). No backward edges.

## Verdict Reasoning

Two High spec-gap findings + one Medium spec-gap finding + one Medium process finding + two Low style/maintainability findings. All have explicit tracking via Jira / contract / spec-doc / code-comment links. No Critical, no Architecture violations. The architecture invariants the replay slice was supposed to deliver (ADR-011, single composition root, layer direction, mode-agnosticism) are all observably holding — verified by AC-4a's live structural test.

Verdict: **PASS_WITH_WARNINGS**.

## Action Items (recommended)

1. **AZ-558**: prioritise — closes AZ-401 AC-9 + AZ-404 AC-4b in a single follow-up. Acknowledge the signing-handshake risk (3pt → likely 5pt).
2. **Hygiene PBI candidate** (process F4): "Protocol completeness AST scan" — a 2pt unit-suite addition that compares each `runtime_checkable` Protocol to each in-repo class claimed to implement it, surfacing phantom-API specs at task-prep time. Catches the AZ-389 / AZ-559 pattern preemptively.
3. **D-PROJ-2 mock follow-up** (F2): when the parent-suite design lands, file a task to implement the full ingest contract in `tests/fixtures/mock-suite-sat-service/` so AZ-404 AC-8 can unskip.
4. **Real calibration delivery** (F3): when the Topotek KHP20S30 intrinsics + body-to-camera SE3 are obtained, drop the `xfail` on AZ-404 AC-3.
