# Cumulative Code Review — Batches 58-60 (Cycle 1)

**Date**: 2026-05-14
**Range**: batches 58 (AZ-358 + AZ-361 — C4 OpenCVGtsam pose estimator + Jacobian/thermal hybrid), 59 (AZ-399 + AZ-400 — TlogReplayFcAdapter + JsonlReplaySink/MavlinkTransport), 60 (AZ-405 — `replay_input/` coordinator + auto-sync)
**Compared against**: previous cumulative review batches 55-57
**Verdict**: **PASS_WITH_WARNINGS**

## Scope

The 58-60 trio covers two distinct concerns:

- **Batch 58** finished C4 pose estimation (Marginals + Jacobian-thermal hybrid). All 11 ACs across AZ-358 + AZ-361 are covered; no Architecture findings; one open follow-up (AZ-361 AC-11 informational latency comparison) carried forward.
- **Batches 59 + 60** brought the **replay subsystem** online end-to-end: AZ-399 added the tlog FC adapter, AZ-400 added the JSONL replay sink + the `MavlinkTransport` Protocol cut-out, and AZ-405 added the `replay_input/` coordinator + auto-sync detector. The composition root branch (AZ-401) is the next consumer in line.

## Carry-over status from cumulative review 55-57

| Prior finding | Status | Notes |
|---------------|--------|-------|
| F1 (Low) — two parallel engine-output-probe helpers (C2 / C3) with FP32 vs FP16 probe dtype divergence | **OPEN — carry forward** | No code in batches 58-60 touched either helper. The TRT engine path that would surface this remains gated behind AZ-321 (lands in a later cycle). Sized at <1 point. |
| F2 (Low) — XFeat imports underscore-prefixed helpers from `_pipeline.py` | **OPEN — carry forward** | No code in batches 58-60 touched `c3_matcher/xfeat.py`. Convention-only; documented for the next refactor pass. |
| F3 (Low) — AZ-347 AC-special-2 latency benchmark not tested | **OPEN — carry forward** | Informational metric per the task spec; remains documented in the per-batch report for traceability. |
| (52-54) F2 (Low) — c1_vio test fakes not yet shared | **OPEN — carry forward** | No movement; remains a future hygiene pass. |

## Findings (this window)

| # | Severity | Category | File:Line | Title |
|---|----------|----------|-----------|-------|
| F1 | Medium | Spec-Gap | _docs/02_document/contracts/replay/replay_protocol.md:134-145 | Replay contract `ReplayInputAdapter.__init__` was missing `fdr_client` (resolved in batch 60) |
| F2 | Low | Maintainability | src/gps_denied_onboard/replay_input/auto_sync.py + src/gps_denied_onboard/components/c8_fc_adapter/tlog_replay_adapter.py | Tlog message-type pre-validation logic exists in two places (coordinator-side `_load_tlog_samples` + AZ-399's `_prescan_required_messages`) |
| F3 | Low | Maintainability | src/gps_denied_onboard/replay_input/tlog_video_adapter.py | Three test-only injection kwargs (`tlog_source_factory`, `video_frames_factory`, `video_timestamps_factory`) on the production constructor (batch 60 carry-forward) |
| F4 | Low | Performance | src/gps_denied_onboard/components/c4_pose/opencv_gtsam_estimator.py | Two `cv2.projectPoints` calls per Marginals frame (batch 58 carry-forward) |
| F5 | Low | Spec-Gap | tests/unit/c4_pose/test_az358_361_opencv_gtsam_estimator.py | AZ-361 AC-11 informational Jacobian-vs-Marginals RMSE comparison not asserted (batch 58 carry-forward) |

### Finding Details

#### F1: Replay contract `ReplayInputAdapter.__init__` was missing `fdr_client` (Medium / Spec-Gap)

- **Location**: `_docs/02_document/contracts/replay/replay_protocol.md:134-145`
- **Description**: The replay protocol contract v2.0.0 specified the `ReplayInputAdapter.__init__` signature without an `fdr_client` parameter. The implementation needs `fdr_client` to (a) forward to `TlogReplayFcAdapter` (mandatory per AZ-399) and (b) emit the coordinator's own `replay.auto_sync.{detected,low_confidence,ac8_validation_failed}` FDR records. AZ-405's task spec already lists `fdr_client` in its allowed-imports list, so this was a contract-side gap, not an implementation drift.
- **Status**: resolved in batch 60 — contract updated to include `fdr_client: FdrClient` in the constructor signature. No Architecture finding because the dependency is at the documented Layer-1 boundary.
- **Why surfaced cumulatively**: the gap only became visible when AZ-405 wired the FC adapter into the coordinator; batches 58-59 do not consume the coordinator.

#### F2: Two parallel tlog message-type pre-validators (Low / Maintainability)

- **Locations**:
  - `src/gps_denied_onboard/replay_input/auto_sync.py` (`_load_tlog_samples` + caller `_load_and_validate_tlog`) — checks `RAW_IMU` / `SCALED_IMU2` + `ATTITUDE` presence to satisfy AC-13.
  - `src/gps_denied_onboard/components/c8_fc_adapter/tlog_replay_adapter.py:_prescan_required_messages` (AZ-399) — checks `RAW_IMU` / `SCALED_IMU2` + `ATTITUDE` + `GPS_RAW_INT` / `GPS2_RAW` + `HEARTBEAT`.
- **Description**: The two checks have **partially overlapping** required-message sets and **different error message shapes** (`"tlog missing required message types: [...]"` from the coordinator vs `"tlog missing required messages: [...]; consumed by: [...]"` from the FC adapter). Both fire today: the coordinator runs first to satisfy AC-13's "fail-fast BEFORE any video read", then the FC adapter's pre-scan re-runs as a defensive second sanity check during `open()`.
- **Why this is not a duplicate-symbol violation**: the two checks have **different jobs**. The coordinator-side check is the AC-13 surface — it raises with the coordinator's contract-mandated message shape so the CLI exit-code mapping works. The FC adapter check is the AZ-399 INV-3 (R-DEMO-3) surface — it lists the consumers of the missing groups so the operator knows which downstream component is starved. Merging them would either lose information or leak coordinator concepts into a Layer-4 component that should be coordinator-agnostic.
- **Suggestion**: keep both; revisit if a third caller (e.g., a future analytics tool that wants the same fail-fast behavior) appears. Document the relationship in a future hygiene task.
- **Why Low**: both surfaces are tested; the duplication is documented; no current fixture surfaces a divergent error shape.

#### F3: Test-only injection kwargs on the production constructor (Low / Maintainability — carry-forward from batch 60)

- **Location**: `src/gps_denied_onboard/replay_input/tlog_video_adapter.py:ReplayInputAdapter.__init__`
- **Description**: Three kwargs (`tlog_source_factory`, `video_frames_factory`, `video_timestamps_factory`) default to `None` and exist solely so the unit tests can swap in fakes without hitting pymavlink / OpenCV. Mirrors the AZ-399 `TlogReplayFcAdapter`'s `source_factory` precedent in the same epic.
- **Suggestion**: keep — established project pattern. Consider a shared `_TestInjections` Protocol if a third coordinator adopts the same shape.

#### F4: Two `cv2.projectPoints` calls per Marginals frame (Low / Performance — carry-forward from batch 58)

- **Location**: `src/gps_denied_onboard/components/c4_pose/opencv_gtsam_estimator.py:_compute_reprojection_residuals` + `_jacobian_covariance`
- **Status**: same as the per-batch report; no AC-blocking impact. Sized at 1-2 points for a future hygiene pass.

#### F5: AZ-361 AC-11 informational RMSE comparison not asserted (Low / Spec-Gap — carry-forward from batch 58)

- **Location**: `tests/unit/c4_pose/test_az358_361_opencv_gtsam_estimator.py`
- **Status**: per the task spec, AC-11 is informational and explicitly does not block. Documented for traceability.

## Phase Summary

### Phase 1 — Context Loading

Read inputs:

- `_docs/03_implementation/reviews/batch_58_review.md`
- `_docs/03_implementation/reviews/batch_59_review.md`
- `_docs/03_implementation/reviews/batch_60_review.md`
- `_docs/03_implementation/cumulative_review_batches_55-57_cycle1_report.md`
- `_docs/02_tasks/done/AZ-358_c4_opencv_gtsam_marginals.md`
- `_docs/02_tasks/done/AZ-361_c4_jacobian_thermal_hybrid.md`
- `_docs/02_tasks/done/AZ-399_replay_tlog_adapter.md`
- `_docs/02_tasks/done/AZ-400_replay_jsonl_sink.md`
- `_docs/02_tasks/todo/AZ-405_replay_auto_sync.md`
- `_docs/02_document/contracts/replay/replay_protocol.md` v2.0.0
- `_docs/02_document/architecture.md` (ADR-011)
- `_docs/02_document/module-layout.md`

### Phase 2 — Spec Compliance

Per-batch reports already verified each AC; this cumulative pass spot-checked the following cross-cutting promises:

- **Replay protocol Invariant 1** (no mode-aware branches outside the composition root): the `replay_input/` coordinator is the boundary; C1–C7 + C13 see only standard `FrameSource` / `FcAdapter` / `Clock`. AZ-401 will provide the AST-scan test that asserts no `if config.mode == "replay"` lines exist in component files. Not violated by batches 58-60.
- **Replay protocol Invariant 2** (single Clock instance): both batches 59 and 60 honour single-instance construction; the coordinator builds the Clock once and bundles it.
- **Replay protocol Invariant 5** (replay never writes to FC): AZ-399's `emit_external_position` / `emit_status_text` raise `FcEmitError`; AZ-405's coordinator never calls them. Verified by tests in batch 59.
- **Replay protocol Invariant 8** (`time_offset_ms` baked at construction, no live re-tuning): AZ-405's coordinator resolves the offset before constructing `TlogReplayFcAdapter`; the FC adapter receives the resolved value as a constructor argument.

### Phase 3 — Code Quality

No new findings beyond per-batch reports + F2 above. Tests across all three batches follow Arrange / Act / Assert with comment markers.

### Phase 4 — Security

No new findings. Replay file paths (video, tlog) are operator-supplied and validated for existence before any consumer call. No sensitive data in logs / FDR records.

### Phase 5 — Performance

No new findings beyond F4 (carry-forward).

### Phase 6 — Cross-Task Consistency

- AZ-405 cleanly consumes AZ-398 (frame_source + clock) + AZ-399 (TlogReplayFcAdapter) + AZ-400 (FdrClient via the existing AZ-273 surface) + AZ-279 (WgsConverter). All Public API surfaces match.
- The `ReplayInputBundle` shape is exactly what AZ-401 will need (the contract documents this).
- `BUILD_VIDEO_FILE_FRAME_SOURCE` and `BUILD_TLOG_REPLAY_ADAPTER` flags are checked at the right boundaries (component-internal in AZ-398/AZ-399; coordinator does NOT add a third flag, per ADR-011).

### Phase 7 — Architecture Compliance

- **Layer direction**: `replay_input/` is at Layer 4 per `module-layout.md`. It imports from Layer 1 (foundation) and from two specific Layer-4 strategies (`c8_fc_adapter.tlog_replay_adapter`, `frame_source.video_file`) — this cross-Layer-4 wiring is the documented coordinator pattern from ADR-011 (the coordinator IS the seam where Layer-4 strategies are instantiated). No Layer 3 imports. No back-channel.
- **Public API respect**: every cross-component import in batches 58-60 lives in the imported module's `__all__`. Verified by grepping `__all__` against the new files' import lists.
- **No new cyclic dependencies**: `replay_input/` is a leaf in the import graph until AZ-401 lands the composition-root consumer.
- **Duplicate symbols**: F2 above is the only candidate; classified as Low because the two checks have legitimately different responsibilities.
- **Cross-cutting concerns**: structured logging, FDR enqueue, ISO timestamps, WGS conversion all consumed from shared helpers — no local re-implementation.

## Verdict Logic

- 0 Critical, 0 High, 1 Medium (resolved in-batch by contract update), 4 Low (3 carry-forward + 1 new) → **PASS_WITH_WARNINGS**.

## Outputs

- `verdict`: PASS_WITH_WARNINGS
- `findings`: 5 (1 Medium + 4 Low)
- `critical_count`: 0
- `high_count`: 0
- `report_path`: `_docs/03_implementation/cumulative_review_batches_58-60_cycle1_report.md`
