mirror of
https://github.com/azaion/gps-denied-onboard.git
synced 2026-06-22 13:01:14 +00:00
[AZ-402] Replay — gps-denied-replay console-script + shared main(config)
Implements the replay-mode CLI dispatcher per ADR-011 (replay-as- configuration): - src/gps_denied_onboard/cli/replay.py: argparse with all 6 required args (--video, --tlog, --output, --camera-calibration, --config, --mavlink-signing-key) plus --pace and --time-offset-ms; path validation, calibration JSON schema-validation, config mutation (mode='replay' + replay sub-block + signing-key hex on dev_static field), dispatch into runtime_root.main(config). - runtime_root.main() now accepts an optional Config (additive, backward-compat). Adds dedicated catch for ReplayInputAdapterError mapping to EXIT_FDR_OPEN_FAILURE (2) so the CLI's exit-code matrix holds end-to-end (AC-9 + epic AZ-265 AC-8). - Signing-key contents stored as hex; redacted in startup banner. - Top-level except logs full traceback via logger.exception + stderr print and exits 1. The CLI does NOT call compose_root directly — it builds a Config and hands it to the shared airborne main, which calls compose_root, which branches on config.mode (AZ-401 / replay protocol Invariant 11). Tests: 22 unit tests covering AC-1..AC-10 + extras (signing-key redaction, file-not-dir validation, dev_static propagation, unhandled exception traceback). Full regression: 2085 passed (+22) green; no new flaky tests. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Oleksandr Bezdieniezhnykh
@@ -0,0 +1,98 @@
|
||||
# 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**.
|
||||
@@ -12,7 +12,7 @@ sub_step:
|
||||
retry_count: 0
|
||||
cycle: 1
|
||||
tracker: jira
|
||||
last_completed_batch: 61
|
||||
last_completed_batch: 62
|
||||
last_cumulative_review: batches_58-60
|
||||
current_batch: 62
|
||||
current_batch: 63
|
||||
current_batch_tasks: ""
|
||||
|
||||
Reference in New Issue
Block a user