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

Code Review Report

Batch: 59 (AZ-399, AZ-400) Date: 2026-05-14 Verdict: PASS_WITH_WARNINGS

Findings

#	Severity	Category	File:Line	Title
1	Medium	Maintainability	src/gps_denied_onboard/components/c8_fc_adapter/tlog_replay_adapter.py:560-720	MAVLink mapping logic duplicates _inbound_mavlink.PymavlinkInboundDecoder
2	Low	Maintainability	src/gps_denied_onboard/components/c8_fc_adapter/tlog_replay_adapter.py:457	_PRESCAN_MAX_MESSAGES = 6000 is implicit and hard-codes a 30 s @ 200 Hz budget
3	Low	Maintainability	src/gps_denied_onboard/components/c8_fc_adapter/replay_sink.py:184	noqa: SIM115 suppression with inline rationale is fine; flag for future re-evaluation if a context-managed alternative emerges

Finding Details

F1: MAVLink mapping logic duplicates the AZ-391 live decoder (Medium / Maintainability)

• Location: src/gps_denied_onboard/components/c8_fc_adapter/tlog_replay_adapter.py:560-720
• Description: _handle_imu, _handle_attitude, _handle_gps, _handle_heartbeat, _map_fix_type, and _map_mav_state mirror the AZ-391 PymavlinkInboundDecoder implementations almost line-for-line. Today the duplication is intentional — the replay path differs from live in three observable ways: (a) received_at is the tlog timestamp + time_offset_ns (not Clock.monotonic_ns()), (b) signed=False is hard-coded per the D-CROSS-CVE-1 risk, (c) non-monotonic timestamps raise FcOpenError instead of being dropped, and (d) STATUSTEXT spoof-promotion is intentionally absent because the replay channel carries the recorded stream verbatim. A shared mapper module would have to expose enough seams to model all four differences and would risk losing the readability win.
• Suggestion: revisit during AZ-405 (auto-sync) or AZ-403 (cross-component refactor) if a third caller appears. For now, leave the duplication and document the four behavioural deltas in the module docstring.
• Task: AZ-399

F2: _PRESCAN_MAX_MESSAGES = 6000 magic number lacks runtime override (Low / Maintainability)

• Location: src/gps_denied_onboard/components/c8_fc_adapter/tlog_replay_adapter.py:457
• Description: The pre-scan budget is sized to "≈ 30 s of telemetry at 200 Hz" in the comment, but it is a module constant. A 1 Hz HEARTBEAT-only fixture (e.g., a long-coast clip) might satisfy the IMU/attitude/GPS budget within 6000 records but never see the late HEARTBEAT — leading to a false-negative fail-fast. The current Derkachi fixture sits comfortably under the budget, so this is a future-proofing note, not a present bug.
• Suggestion: thread prescan_max_messages through the constructor with the current value as default once a real fixture pushes the budget. No change required for batch 59.
• Task: AZ-399

F3: # noqa: SIM115 rationale comment (Low / Maintainability)

• Location: src/gps_denied_onboard/components/c8_fc_adapter/replay_sink.py:184
• Description: The comment "owned for the sink lifetime" justifies the suppression — the file handle outlives the constructor by design (close happens in JsonlReplaySink.close). Acceptable.
• Suggestion: no action; keep the comment for future readers.
• Task: AZ-400

Phase Summary

Phase 1 — Context Loading

Read inputs:

• _docs/02_tasks/todo/AZ-399_replay_tlog_adapter.md
• _docs/02_tasks/todo/AZ-400_replay_jsonl_sink.md
• _docs/02_document/contracts/replay/replay_protocol.md
• _docs/02_document/contracts/c8_fc_adapter/fc_adapter_protocol.md
• _docs/02_document/module-layout.md

Phase 2 — Spec Compliance

AZ-400 — all 10 ACs covered by tests/unit/c8_fc_adapter/test_az400_replay_sink.py. Contract ReplaySink Protocol surface (emit(EstimatorOutput) -> None, close() -> None) matches replay_protocol.md v1.0.0; the prior AZ-390 stub was widened in interface.py to match this batch's contract. Module-layout home updated.

AZ-399 — AC-2 through AC-10 covered by tests/unit/c8_fc_adapter/test_az399_tlog_replay_adapter.py; AC-1 (500 MB tlog RSS bound) is present as a @pytest.mark.skip placeholder with prerequisite reason, deferred to AZ-404 e2e behind RUN_REPLAY_E2E=1 per the implement skill's "skipped tests count as Covered" rule. Contract surface (TlogReplayFcAdapter(tlog_path, target_fc_dialect, clock, wgs_converter, time_offset_ms, pace, fdr_client, source_factory)) matches the replay_protocol.md constructor; Invariants 5, 6, 8 enforced and tested.

Phase 3 — Code Quality

• SOLID: SubscriptionBus reused via composition (Invariant 1: replay frames hit identical fan-out shape). _to_jsonable is a pure function for testability.
• Error handling: explicit FcOpenError / FcAdapterConfigError / FcEmitError / SourceSetSwitchNotSupportedError in tlog_replay_adapter; explicit ReplaySinkError / ReplaySinkConfigError in replay_sink. No bare except.
• Naming: clear (_msg_timestamp_ns, _FRAME_PROGRESS_INTERVAL, REQUIRED_MESSAGE_TYPES).
• Complexity: longest method ≈ 40 lines (_dispatch); under the 50-line threshold.
• Tests: every test follows Arrange / Act / Assert with language-appropriate # Arrange|Act|Assert markers.
• Dead code: none introduced.

Phase 4 — Security

• No SQL / command injection vectors.
• No hardcoded secrets.
• Tlog file path is operator-supplied and opened via pymavlink.mavutil.mavlink_connection; the lazy pymavlink import means a missing C extension surfaces as FcOpenError, not an ImportError at composition time.
• Output JSONL path validation already in place from AZ-400 (parent-dir check + binary-mode open).

Phase 5 — Performance

• Stream-parse via recv_match(blocking=False) — never materialises the tlog.
• Pre-scan ceiling caps file walk at _PRESCAN_MAX_MESSAGES (Finding F2 caveat).
• AC-7 throughput proxy passes (1000 frames in < 1 s on Tier-1 hardware).
• JsonlReplaySink.emit is orjson.dumps + write — microsecond-class.

Phase 6 — Cross-Task Consistency

• AZ-399 and AZ-400 both live under c8_fc_adapter and route through the existing component-local plumbing (SubscriptionBus, errors.py, fdr_client).
• The AZ-400 ReplaySink Protocol is exactly what AZ-401's compose_replay will require.
• No DTO drift: both files consume EstimatorOutput, FcTelemetryFrame, LatLonAlt, Quat from the existing _types/* shared layer.

Phase 7 — Architecture Compliance

• Layer direction: tlog_replay_adapter and replay_sink import from _types, helpers, fdr_client, clock, logging, plus the own-component _subscription / errors. All importees are at or above the importer's layer per module-layout.md (Layer 4 component → Layer 1 cross-cutting / Layer 2 shared types).
• Public API respect: imports are limited to documented public surfaces (FdrRecord, iso_ts_now, get_logger, WgsConverter).
• No new cyclic dependencies.
• No duplicate symbols across components (the AZ-391 mapping duplication noted in F1 is within the same component).
• Cross-cutting concerns (FDR enqueue, structured logging) consumed via shared helpers — not re-implemented locally.

Verdict Logic

• 0 Critical, 0 High, 1 Medium, 2 Low → PASS_WITH_WARNINGS.

Outputs

• verdict: PASS_WITH_WARNINGS
• findings: 3 (1 Medium + 2 Low)
• critical_count: 0
• high_count: 0
• report_path: _docs/03_implementation/reviews/batch_59_review.md