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

Code Review Report

Batch: 62 (AZ-402) Date: 2026-05-14 Verdict: PASS

Findings

#	Severity	Category	File:Line	Title
1	Low	Maintainability	src/gps_denied_onboard/runtime_root/__init__.py:621-660	runtime_root.main now accepts an optional Config (additive refactor) — additional surface for callers, but backward-compat
2	Low	Style	src/gps_denied_onboard/cli/replay.py:235-256	New shared_main test-injection kwarg added to cli/replay:main (third coordinator with this pattern; pre-flagged in batch 61 review F3)

Finding Details

F1: runtime_root.main accepts optional Config (Low / Maintainability)

• Location: src/gps_denied_onboard/runtime_root/__init__.py:621-660 (def main(config: Config | None = None) -> int)
• Description: AZ-402's task spec calls for the CLI to "dispatch into the same main() function the live gps-denied-onboard binary calls". Before this batch, runtime_root.main() was parameterless and loaded the Config itself from os.environ; the CLI couldn't pass a mutated config without either calling compose_root directly (FORBIDDEN per replay protocol Invariant 11) or rewriting os.environ (a fragile workaround). The smallest additive refactor is to accept Config | None: when None the live binary's behaviour is preserved (load from env), when supplied the CLI can hand in its mutated config. The function also gains a dedicated catch for ReplayInputAdapterError mapping to EXIT_FDR_OPEN_FAILURE (2) so the CLI's exit-code matrix (AC-9) holds end-to-end.
• Suggestion: keep — matches the spec's Excluded section ("This task assumes the shared main exists and is callable with (config, ...)"). The RuntimeError catch downstream still handles ReplayInputAdapterError if any caller bypasses the new branch — no regression for live mode.
• Task: AZ-402

F2: shared_main test-injection kwarg in cli/replay:main (Low / Style)

• Location: src/gps_denied_onboard/cli/replay.py:235-256 (def main(argv, *, shared_main=None))
• Description: A second optional kwarg defaulting to None (resolved lazily to runtime_root.main to avoid a circular import + cheap module-load). When provided (tests), the fake replaces the dispatch target. This is the same precedent as replay_input.tlog_video_adapter.ReplayInputAdapter.__init__'s test factories (batch 60) and AZ-401's compose_root(replay_components_factory=...) (batch 61). Production callers (the console-script entry point, if __name__ == "__main__" block) pass none of them.
• Suggestion: keep. Three coordinators now share this shape; if a fourth adopts it, factor into a shared _TestFactories helper.
• Task: AZ-402

Phase Summary

Phase 1 — Context Loading

Read inputs:

• _docs/02_tasks/todo/AZ-402_replay_cli.md
• _docs/02_document/contracts/replay/replay_protocol.md (v2.0.0 — CLI surface + Invariant 11)
• _docs/02_document/architecture.md (ADR-011)
• _docs/02_document/module-layout.md (Layer 5 — cli/replay)
• _docs/02_document/epics.md (E-DEMO-REPLAY ACs 1, 8, 11)

Phase 2 — Spec Compliance

AC	Verdict	Test
AC-1 (all 6 required args parsed)	PASS	test_ac1_all_required_args_parsed
AC-2 (--pace default asap)	PASS	test_ac2_pace_default_asap
AC-3 (--pace realtime)	PASS	test_ac3_pace_realtime
AC-4 (--time-offset-ms forwarded)	PASS	test_ac4_time_offset_forwarded + _none_when_absent
AC-5 (--mavlink-signing-key required, argparse exit 2)	PASS	test_ac5_missing_signing_key_exits_2 (+ _missing_video_exits_2)
AC-6 (malformed JSON → exit 1)	PASS	test_ac6_malformed_calibration_exits_1
AC-7 (missing intrinsics key → schema error)	PASS	test_ac7_missing_intrinsics_key_rejected (+ _top_level_not_object)
AC-8 (config.mode == "replay")	PASS	test_ac8_mode_set_to_replay
AC-9 (exit-code pass-through 0 / 1 / 2; ReplayInputAdapterError → 2)	PASS	test_ac9_exit_code_pass_through (parametrized × 3) + test_ac9_replay_input_adapter_error_maps_to_2 + test_unhandled_exception_exits_1_with_traceback
AC-10 (console script registered + --help works)	PASS	test_ac10_console_script_registered_in_pyproject + test_ac10_console_script_runs_help

22 unit tests in tests/unit/test_az402_replay_cli.py, all green. Plus extra coverage: signing-key redaction in banner, file-not-dir validation, signing-key propagation to Config.fc.dev_static_signing_key.

Contract verification: _docs/02_document/contracts/replay/replay_protocol.md v2.0.0 §CLI Surface + Invariant 11 (signing key mandatory) match the implementation. module-layout.md §shared/cli/replay description matches the new file's purpose verbatim.

Phase 3 — Code Quality

• SOLID: cli/replay.py is a single-responsibility CLI dispatcher. Each helper has one job: _build_argparser, _validate_paths, _load_calibration_json, _build_replay_config, _print_startup_banner. main() orchestrates.
• Error handling: explicit, layered. ReplayCliError for operator-input failures (chains __cause__); ReplayInputAdapterError caught and mapped to exit 2; SystemExit re-raised so argparse's --help / --version propagate; everything else logged with full traceback. No bare except: and no except: pass.
• Naming: clear (_build_replay_config, _print_startup_banner, EXIT_SYNC_IMPOSSIBLE).
• Complexity: longest function is main() at ~35 LOC (linear flow with explicit guards). No cyclomatic-complexity > 10.
• Test quality: every test asserts a meaningful behaviour. Parametrised exit-code test exercises 0 / 1 / 2 in one place. The signing-key redaction test asserts the path string itself is NOT in stderr (positive AND negative assertion).
• Dead code: none introduced. The previous cli/replay.py stub (5-line placeholder returning exit 2) is fully replaced.

Phase 4 — Security Quick-Scan

• Signing key redaction: the startup banner replaces the mavlink_signing_key value with "<redacted>" before printing. Test enforces (test_signing_key_redacted_in_startup_banner). The path is sanitised; the file contents are also stored as hex in Config.fc.dev_static_signing_key and never logged.
• No SQL / shell / eval / exec / pickle: argparse, json.loads, Path operations only.
• Calibration JSON: parsed with json.loads (safe; no schema-injection vector). Schema validation rejects unexpected shapes at top level.
• No hardcoded secrets: the signing key is operator-supplied at runtime via a file path.

Phase 5 — Performance Scan

• argparse setup + calibration JSON load + config-mutation are all constant-time on small inputs (calib.json is < 4 KB). The CLI's contribution to cold-start is measured in milliseconds, well within the AZ-402 NFR (argparse + calibration loading p99 ≤ 100 ms).
• The CLI calls runtime_root.main exactly once. No retry loop, no polling.

Phase 6 — Cross-Task Consistency

• Only AZ-402 in this batch. The runtime_root.main refactor is additive: main() (no args) still works identically — proven by the 2085-test regression sweep with no failures introduced.
• The CLI's Config mutation uses dataclasses.replace with the existing Config, RuntimeConfig, ReplayConfig, FcConfig shapes added in batch 61 (AZ-401). No schema drift.
• The exit-code semantic on ReplayInputAdapterError (2) is consistent with EXIT_FDR_OPEN_FAILURE (2) — both mean "fatal startup hard-fail; operator action required". The shared code makes the airborne binary's exit surface predictable.

Phase 7 — Architecture Compliance

• Layer direction: cli/replay.py is Layer 5 per module-layout.md. It imports from Layer 1 (config, logging), Layer 4 (replay_input.errors), and Layer 5 (runtime_root.main). All Layer-5 → Layer-1/4/5 — correct direction.
• Public API respect: the CLI imports Config, ReplayConfig, load_config from gps_denied_onboard.config (the package public surface), not deep submodules. It imports ReplayInputAdapterError from gps_denied_onboard.replay_input (also a package re-export). It imports runtime_root.main lazily inside main() to avoid circular imports.
• No new cyclic deps: cli/replay.py is leaf-imported only by the console-script entry point; the lazy runtime_root.main import inside main() further insulates it.
• Duplicate symbols: EXIT_SUCCESS, EXIT_GENERIC_FAILURE, EXIT_SYNC_IMPOSSIBLE are the CLI's own constants. They mirror runtime_root's EXIT_GENERIC_FAILURE / EXIT_FDR_OPEN_FAILURE by value (1 / 2). The mirror is intentional: each layer documents its own exit semantics. If the values ever drift, the AC-9 parametrised test catches the regression.
• Cross-cutting concerns: calibration loading is duplicated in three places (live composition root via env, replay branch in _replay_branch._load_camera_calibration, and now CLI in _load_calibration_json). The CLI loader is a fail-fast SCHEMA gate, not a parsing layer (the actual CameraCalibration build happens inside _replay_branch). The duplication is small and intentional. Already pre-flagged in batch 61 review F4 as "factor when a third call site appears" — this is the third call site, but with a different responsibility (validation vs. construction); leaving as-is and re-evaluating after AZ-326 / live-CLI work touches the calibration loading path.

Verdict Reasoning

Two Low findings, both deliberate design choices documented in the spec's Excluded / Constraints sections. No Critical, no High. Verdict: PASS.