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

Code Review Report — Batch 64

Batch: 64 Tasks: AZ-558 (Route C8 outbound encoder bytes through MavlinkTransport seam — closes AZ-401 AC-9) Date: 2026-05-16 Verdict: PASS_WITH_WARNINGS

Summary

Batch 64 retrofitted the C8 outbound MAVLink path to route every byte through the MavlinkTransport Protocol seam introduced by AZ-401. The retrofit closes two previously-deferred gates in one cycle: AZ-401 AC-9 (NoopMavlinkTransport.bytes_written() > 0) and AZ-404 AC-4b (encoder byte-equality between live and replay paths).

Six AC tests landed (4 byte-equivalence + 3 AST-scan + 1 AC-9 unskip + 1 AZ-404 e2e AC-4b unskip). Existing 4 unit-test files for AP / iNav / signing / source-set-switch adapters were updated to support the new encode → pack → transport.write flow without changing their assertion shape (encode methods record the same args the previous *_send methods recorded).

Full regression suite: 2110 passed, 92 environmental skips, 1 deselected pre-existing macOS-dev cold-start flake (test_cli_console_script.py::TestConsoleScript::test_cold_start_under_500ms_p99 — unrelated to this batch's surface).

Spec Compliance — AZ-558

AC	Spec	Test(s)	Status
AC-1	AP / iNav constructors accept transport kwarg; replace every mav.*_send	test_az393_ardupilot_outbound.py (11) + test_az394_inav_outbound.py (11) — assertions on the same *_calls lists, now populated through the encoder seam	PASS
AC-2	Wire-byte equivalence (live mode)	test_az558_outbound_transport_seam.py::test_ac2_byte_equivalence_* (gps_input, named_value_float, statustext, multi-msg seq alignment) — 4 tests	PASS
AC-3	Replay FC adapter produces bytes via transport	test_az401_compose_root_replay.py::test_ac9_noop_transport_bytes_written_after_runtime_drive — 10 ticks × 2 messages → bytes_written() > 0	PASS
AC-4	AZ-401 AC-9 unskips	Same test as AC-3, no longer @pytest.mark.skip	PASS
AC-5	No .mav.<name>_send( AST nodes in retrofitted adapters	test_az558_outbound_transport_seam.py::test_ac5_no_pymavlink_send_helpers_in_adapter_source — 3 parametrised files (AP / iNav / tlog)	PASS
AC-6	compose_root injects transport (live + replay)	Replay path fully wired (_replay_branch.py builds transport before bundle, threads through ReplayInputAdapter → TlogReplayFcAdapter); see findings F4 for live mode	PASS_WITH_NOTE

Bonus closure: AZ-404 AC-4b unskipped via test_derkachi_1min.py::test_ac4_encoder_byte_equality_via_transport_seam (constructive equivalence between MAVLink.send and encode → pack → transport.write paths against the same MAVLink instance).

Findings

#	Severity	Category	File:Line	Title
1	Low	Maintainability	runtime_root/_replay_branch.py; replay_input/tlog_video_adapter.py	mavlink_transport: Any typing too loose; should be Protocol-typed
2	Low	Maintainability	pymavlink_ardupilot_adapter.py:_ConnectionWriteTransport; msp2_inav_adapter.py:_SecondaryWriteTransport	Near-duplicate fallback transport classes
3	Low	Maintainability	pymavlink_ardupilot_adapter.py:_ConnectionWriteTransport.write	Fallback transport does not type-check payload
4	Low	Spec	live compose_root path	SerialMavlinkTransport injection point exists but no production binary registers AP/iNav strategies yet

Finding Details

F1: mavlink_transport: Any typing too loose (Low / Maintainability)

• Location: src/gps_denied_onboard/runtime_root/_replay_branch.py:_build_replay_input_bundle; src/gps_denied_onboard/replay_input/tlog_video_adapter.py:ReplayInputAdapter.__init__
• Description: The mavlink_transport parameter on the replay coordinator path is typed Any to avoid a replay_input → c8_fc_adapter import. The Protocol type would be more honest.
• Suggestion: Either import MavlinkTransport under if TYPE_CHECKING: or move the Protocol definition to a _types/ module the replay coordinator can already see. Defer until the import-direction concern can be evaluated against module-layout.md — leaving Any is consistent with the existing tlog_source_factory: Any | None patterns in the same constructor.

F2: Duplicate fallback transport classes (Low / Maintainability)

• Location: src/gps_denied_onboard/components/c8_fc_adapter/pymavlink_ardupilot_adapter.py:_ConnectionWriteTransport; src/gps_denied_onboard/components/c8_fc_adapter/msp2_inav_adapter.py:_SecondaryWriteTransport
• Description: Both classes implement the same fallback MavlinkTransport shape (write through the wrapped object's .write, count bytes, drop on close). The only behavioural difference is iNav's tolerance for the secondary connection lacking a write attribute (it silently counts the would-be byte length).
• Suggestion: Extract into a shared _outbound_fallback_transport.py module within c8_fc_adapter/ once a third caller appears. With only two, the duplication cost is lower than the indirection cost.

F3: Fallback transport does not type-check payload (Low / Maintainability)

• Location: src/gps_denied_onboard/components/c8_fc_adapter/pymavlink_ardupilot_adapter.py:_ConnectionWriteTransport.write
• Description: Production SerialMavlinkTransport.write rejects non-bytes-like inputs with MavlinkTransportError. The fallback variant does not. The fallback is reachable only when no transport factory is injected (test paths and one-off callers).
• Suggestion: Either propagate the SerialMavlinkTransport validation or document the fallback as test-only. Since the production composition root will inject a real transport, the practical impact is zero — defer.

F4: Live compose_root does not yet inject SerialMavlinkTransport (Low / Spec)

• Location: live compose_root path
• Description: The retrofit defines the mavlink_transport_factory kwarg on PymavlinkArdupilotAdapter and the secondary_mavlink_transport_factory kwarg on Msp2InavAdapter, but no production binary currently calls register_fc_adapter("ardupilot_plane", ...) or register_fc_adapter("inav", ...). The live-mode injection path is therefore latent — exercised only by unit tests (which use the in-class fallback transport).
• Suggestion: When the airborne binary bootstrap registers the AP / iNav strategies (a future batch), the registration site MUST pass mavlink_transport_factory=lambda conn: SerialMavlinkTransport(conn). Add an architecture-test entry to module-layout.md or to a binary-bootstrap test once the registration lands. Tracked here as documentation; no blocking impact on AZ-558's primary outcome (replay-mode AC-9 closure).

Code Quality Observations

• SOLID: the encode helpers (_outbound_mavlink_payloads.py) are pure functions with single responsibility (one MAVLink message kind each) plus one orchestrator (send_via_transport). The AP / iNav / tlog adapters retain their existing responsibility shape; the retrofit is purely additive at the call-site level.
• Tests: every existing AP / iNav assertion still holds without change. The hybrid _FakeMsg pattern in the test stubs preserves the assertion surface while routing through the new code path — minimal blast radius.
• Architecture: the new _outbound_mavlink_payloads module lives inside c8_fc_adapter/ and is consumed only by adapters in the same component. No new cross-component imports introduced.
• Determinism: send_via_transport snapshots mav.seq into msg._header.seq (via pack) BEFORE bumping. Two MAVLink instances with identical state produce byte-identical output — this is the constructive proof underlying AC-2.

Security

No new attack surface. The retrofit changes the byte path, not the byte content; signing is preserved (consulted by MAVLink_message._pack from mav.signing.sign_outgoing). No subprocess, no external input, no file I/O changes.

Performance

One additional method dispatch (encode, pack, transport.write) per MAVLink message versus the prior mav.*_send. At a 10 Hz emit rate this is negligible. The composition-root NFR (compose_root p99 ≤ 1 s) is not affected — transport construction is constant-time.

Cumulative Architecture Notes

• _replay_branch.py now constructs the transport BEFORE the FC adapter and threads it down through ReplayInputAdapter (which threads to TlogReplayFcAdapter). The dependency direction is correct: runtime_root → replay_input → c8_fc_adapter.
• AC-5's AST scan is parametric over _RETROFITTED_FILES; adding a new outbound MAVLink file requires updating that list. Document this in the retrofit's CONTRIBUTING note when future maintainers add a fourth outbound MAVLink emitter (e.g., the GCS adapter, currently still using mav.*_send directly per its task scope).

Verdict Rationale

PASS_WITH_WARNINGS: zero Critical / High findings. All six ACs of AZ-558 demonstrably satisfied with traceable test coverage. The four Low findings are documented opportunities for future refinement, none blocking on the AZ-558 outcome.

Action Items (Carried to Future Batches)

1. Future: when an airborne binary bootstrap registers the AP / iNav strategies, the registration MUST pass mavlink_transport_factory=lambda conn: SerialMavlinkTransport(conn) (F4).
2. Hygiene (low priority): unify _ConnectionWriteTransport and _SecondaryWriteTransport into a shared fallback module if a third outbound adapter requires the same pattern (F2).
3. Out of scope for AZ-558: the GCS adapter (mavlink_gcs_adapter.py) still calls mav.*_send directly. AZ-558's spec scoped only AP / iNav / replay-FC; the AC-5 AST scan reflects that scope. A follow-up PBI is appropriate when the GCS adapter is wired into a binary.