# 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**.