mirror of
https://github.com/azaion/gps-denied-onboard.git
synced 2026-06-22 20:01:12 +00:00
[AZ-596] Batch 76: fc_proxy_runtime driver (FDR-replay mode)
Add `runner/helpers/fc_proxy_runtime.py` wrapping the existing
`BlackoutSpoofProxy` (AZ-406) with a scenario-facing `drive_fc_proxy`
entry point. FDR-replay mode only: loads `schedule.json`, optionally
activates the proxy against a caller clock for alignment verification,
and writes a `proxy_drive_report.json` audit record into
`${E2E_SITL_REPLAY_DIR}` for downstream evaluators.
Replaces the local `_drive_fc_proxy` stub in FT-N-04. Adds 3
@property accessors on `BlackoutSpoofProxy` so the wrapper does not
reach into private attributes. +11 unit tests (608 total, up from
596). Live-mode router wiring remains out of scope (future ticket).
Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
@@ -0,0 +1,142 @@
|
||||
# Code Review Report
|
||||
|
||||
**Batch**: 76 — AZ-596 (fc_proxy_runtime driver, FDR-replay mode)
|
||||
**Date**: 2026-05-17
|
||||
**Verdict**: PASS
|
||||
|
||||
## Findings
|
||||
|
||||
(none)
|
||||
|
||||
## Findings Sweep
|
||||
|
||||
### Phase 1 — Context Loading
|
||||
|
||||
Read the AZ-596 task spec, the existing
|
||||
`fixtures/injectors/fc_proxy.py` (`BlackoutSpoofProxy`,
|
||||
`SpoofGpsRecord`, `ProxyAlignmentReport`, lifecycle methods), the
|
||||
FT-N-04 scenario's local `_drive_fc_proxy` stub and the surrounding
|
||||
call site, and the AZ-595 `sitl_observer` env-var pattern
|
||||
(`E2E_SITL_REPLAY_DIR` resolution via `replay_dir()`). Verified that
|
||||
`BlackoutSpoofProxy.from_schedule_file` already raises
|
||||
`FileNotFoundError` for missing input and `json.JSONDecodeError` for
|
||||
malformed JSON, so the runtime wrapper only needs to (a) convert
|
||||
`JSONDecodeError → ValueError` with a file pointer for symmetry with
|
||||
the rest of the helper layer and (b) project the proxy state into a
|
||||
small audit dataclass.
|
||||
|
||||
### Phase 2 — Spec Compliance
|
||||
|
||||
| AC | Coverage | Status |
|
||||
|----|----------|--------|
|
||||
| AC-1 (`drive_fc_proxy` loads schedule via `BlackoutSpoofProxy.from_schedule_file`; missing → `FileNotFoundError`; malformed → `ValueError`) | `test_missing_schedule_raises_file_not_found`, `test_malformed_json_raises_value_error`, `test_happy_path_returns_well_formed_report` | Covered |
|
||||
| AC-2 (`now_ms_provider` supplied → proxy activated, `alignment_err_ms` recorded; absent → `alignment_err_ms=0`, `was_replay_mode=True`) | `test_now_ms_provider_activates_proxy_and_reports_alignment`, `test_now_ms_provider_with_replay_mode_false_distinguishes_from_default`, plus the `was_replay_mode is True` assertion in the happy-path test | Covered |
|
||||
| AC-3 (`replay_dir` supplied OR `E2E_SITL_REPLAY_DIR` set → `proxy_drive_report.json` written; neither → no write) | `test_writes_report_when_replay_dir_supplied`, `test_writes_report_when_env_var_set`, `test_explicit_replay_dir_overrides_env_var`, `test_no_file_written_when_neither_supplied`, `test_no_file_written_when_env_var_empty`, `test_replay_dir_is_created_when_missing` | Covered |
|
||||
| AC-4 (≥5 unit tests covering happy + 3 error/edge + 1 boundary) | 11 tests total (3 schedule-load, 2 activation, 6 replay-dir write paths) | Covered (exceeds floor) |
|
||||
| AC-5 (full suite passes) | 608 passed (+12 from 596 baseline; +11 new tests + 1 layout parametrize entry) | Covered |
|
||||
|
||||
### Phase 3 — Code Quality
|
||||
|
||||
* **Single responsibility**: `drive_fc_proxy` owns three things and
|
||||
three things only — (a) construct a `BlackoutSpoofProxy` from a
|
||||
schedule path, (b) optionally activate it against a caller-supplied
|
||||
clock, (c) project the proxy state into `ProxyDriveReport` and
|
||||
optionally persist it. Each branch is straight-line.
|
||||
* **`ProxyDriveReport` is a frozen dataclass** with seven plain fields
|
||||
— no methods, no factories. The dataclass IS the contract; downstream
|
||||
evaluators read it via `asdict` or per-field access.
|
||||
* **`_resolve_replay_dir` is the single env-var reader** in this
|
||||
module. It mirrors the equivalent reader in `sitl_observer.replay_dir`
|
||||
(same env var, same "empty string → None" semantics) — the two
|
||||
modules deliberately do not import each other so the dependency
|
||||
surface stays one-way (`fc_proxy_runtime` → `BlackoutSpoofProxy`,
|
||||
`fc_proxy_runtime` → `os.environ`; nothing else).
|
||||
* **No suppressed errors**: the one `try`/`except` block converts
|
||||
`json.JSONDecodeError` to a `ValueError` with the offending file
|
||||
path AND preserves the original via `raise … from exc`. No bare
|
||||
`except`, no `2>/dev/null`, no empty `pass`.
|
||||
* **Public-accessor addition on `BlackoutSpoofProxy`**: added three
|
||||
`@property` accessors (`window_start_ms`, `window_end_ms`,
|
||||
`spoof_frame_count`) so the runtime driver does NOT reach into
|
||||
private `_window_start_ms` / `_spoof_gps` attributes. The properties
|
||||
are pure, side-effect-free, single-line reads — they purely
|
||||
formalise the existing public read surface that `from_schedule_file`
|
||||
already establishes.
|
||||
* **AAA comment discipline**: all 11 new tests use
|
||||
`# Arrange / # Act / # Assert`; sections omitted when not needed.
|
||||
* **No code comments narrate code** — module docstring explains the
|
||||
FDR-replay rationale and the live-mode out-of-scope boundary.
|
||||
Per-function docstrings document the parameter contract.
|
||||
* **Public boundary**: imports only stdlib (`json`, `os`,
|
||||
`dataclasses`, `pathlib`, `typing`) +
|
||||
`fixtures.injectors.fc_proxy.BlackoutSpoofProxy` (an existing
|
||||
test-side module). Zero `from gps_denied_onboard ...` imports.
|
||||
|
||||
### Phase 4 — Security
|
||||
|
||||
* **No new credentials, secrets, or network surface**. The driver is
|
||||
pure file I/O over caller-supplied (or env-var-rooted) paths.
|
||||
* **`E2E_SITL_REPLAY_DIR`** is read-only (consistent with AZ-595).
|
||||
Written paths use `Path` arithmetic — no string-interpolation into
|
||||
shell, no `eval`, no `subprocess`.
|
||||
* **JSON write path** uses `json.dumps(asdict(report))` — no opaque
|
||||
pickle, no untrusted deserialisation of caller input.
|
||||
* **`replay_dir.mkdir(parents=True, exist_ok=True)`** silently creates
|
||||
intermediate directories. This is acceptable because the path comes
|
||||
from the test harness's own env var, not from external input.
|
||||
|
||||
### Phase 5 — Performance
|
||||
|
||||
* O(1) work beyond the upstream `BlackoutSpoofProxy.from_schedule_file`
|
||||
load (which itself is O(N) in the number of spoof frames; the
|
||||
dataclass projection is constant-time using the new properties).
|
||||
* No I/O at module-import time.
|
||||
* The JSON write path is a single `write_text` call — atomic-enough
|
||||
for the audit-only use case.
|
||||
|
||||
### Phase 6 — Cross-Task Consistency
|
||||
|
||||
* **Env-var pattern matches AZ-595**: the same `E2E_SITL_REPLAY_DIR`
|
||||
semantics (set → use; unset / empty / whitespace → ignore). A
|
||||
future fixture builder will set the env var once for the whole
|
||||
scenario run, and both `sitl_observer` (reads) + `fc_proxy_runtime`
|
||||
(writes the audit report) consume it from a single source of truth.
|
||||
* **`ProxyDriveReport` field names mirror the existing
|
||||
`ProxyAlignmentReport`** in `fc_proxy.py` for `alignment_err_ms`.
|
||||
No name churn for the AC-3 / AC-NEW-3 evaluator that will eventually
|
||||
read it (`blackout_spoof_evaluator` already references
|
||||
`alignment_err_ms` from the proxy's own activation report).
|
||||
* **`FileNotFoundError` / `ValueError` discipline matches the rest of
|
||||
`e2e/runner/helpers/`** (per the b73-b75 cumulative review): missing
|
||||
inputs → `FileNotFoundError`, malformed inputs → `ValueError` with a
|
||||
file pointer.
|
||||
* **FT-N-04 scenario rewire**: the local `_drive_fc_proxy` stub now
|
||||
imports `runner.helpers.fc_proxy_runtime.drive_fc_proxy` and calls
|
||||
it. The scenario's `sitl_replay_ready` skip gate (added in b75)
|
||||
continues to gate on `E2E_SITL_REPLAY_DIR`; the new helper writes
|
||||
its audit report into that same directory when the gate is open.
|
||||
|
||||
### Phase 7 — Architecture Compliance
|
||||
|
||||
* **Module placement**: `e2e/runner/helpers/fc_proxy_runtime.py` (new)
|
||||
+ `e2e/_unit_tests/helpers/test_fc_proxy_runtime.py` (new). Both
|
||||
registered in `e2e/_unit_tests/test_directory_layout.py`; the
|
||||
layout invariant test still passes.
|
||||
* **No `src/gps_denied_onboard` imports** anywhere. Confirmed.
|
||||
* **No new top-level dependencies** — stdlib only on the runner side;
|
||||
the test side adds nothing new. `requirements.txt` untouched.
|
||||
* **Backwards-compatible**: the `BlackoutSpoofProxy` change is purely
|
||||
additive — three new `@property` accessors. Existing callers
|
||||
(the injector's own self-tests, the FT-N-04 fixture) keep working
|
||||
unchanged.
|
||||
|
||||
## Test Results
|
||||
|
||||
* New unit tests: **11** (3 schedule load/error, 2 activation, 6
|
||||
replay-dir write).
|
||||
* Full `e2e/_unit_tests` suite: **608 passed in 124 s** (previous
|
||||
cumulative: 596 → +12 net = +11 new fc_proxy_runtime tests + 1
|
||||
new directory-layout parametrize entry).
|
||||
* No new linter errors (`ReadLints` clean on `fc_proxy_runtime.py`,
|
||||
`test_fc_proxy_runtime.py`, `fc_proxy.py`,
|
||||
`test_ft_n_04_blackout_spoof.py`, `test_directory_layout.py`).
|
||||
Reference in New Issue
Block a user