mirror of
https://github.com/azaion/gps-denied-onboard.git
synced 2026-06-22 13:51:13 +00:00
[AZ-416] [AZ-417] [AZ-419] Test batch 72: FT-P-09 AP/iNav + FT-P-11 cold start
- AZ-416 (FT-P-09-AP): fills mavproxy_tlog_reader.iter_messages with pymavlink body (AZ-406 surface kept); adds ap_contract_evaluator covering AC-1 (signing handshake <=5s), AC-2 (GPS_INPUT >=4.5 Hz), AC-3 (EK3_SRC1_POSXY=3), AC-4 (GPS_RAW_INT health >=80%); scenario forces fc_adapter=ardupilot. - AZ-417 (FT-P-09-iNav): msp_frame_observer covering AC-2 (MSP rate) and AC-3 (fix_type/provider/numSat); scenario forces fc_adapter=inav. - AZ-419 (FT-P-11): cold_start_evaluator covering AC-1 (operator manifest origin), AC-2 (FC EKF fallback), AC-3 (no-origin abort), AC-4 (bounded-delta conflict, ADR-010 Principle #11 amended); scenario parametrized on origin_source plus dedicated no-origin abort scenario. - All scenarios skip-gated on upstream frame_source_replay / imu_replay / fdr_reader / sitl_observer extensions. - +67 unit tests; full e2e unit suite: 460 passed. - K=3 cumulative review fired: PASS for batches 70-72. See _docs/03_implementation/batch_72_report.md, _docs/03_implementation/reviews/batch_72_review.md, _docs/03_implementation/cumulative_review_batches_70-72_cycle1_report.md. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
@@ -0,0 +1,176 @@
|
||||
# Code Review Report
|
||||
|
||||
**Batch**: 72 — AZ-416, AZ-417, AZ-419
|
||||
**Date**: 2026-05-16
|
||||
**Verdict**: PASS
|
||||
|
||||
## Findings
|
||||
|
||||
(none)
|
||||
|
||||
## Findings Sweep
|
||||
|
||||
### Phase 1 — Context Loading
|
||||
|
||||
Loaded specs `AZ-416_ft_p_09_ap_signing.md`, `AZ-417_ft_p_09_inav.md`,
|
||||
`AZ-419_ft_p_11_cold_start_init.md`. Re-read existing
|
||||
`runner/helpers/mavproxy_tlog_reader.py` (AZ-406 surface to be filled
|
||||
in by AZ-416 per the docstring), `sitl_observer.py`, `fdr_reader.py`,
|
||||
`geo.py`. Read `fixtures/cold-boot/cold_boot_fixture.json` for FT-P-11
|
||||
secondary path origin. Verified pymavlink ≥2.4 install + the
|
||||
`MAVLink.get_signed()` API surface in the venv.
|
||||
|
||||
### Phase 2 — Spec Compliance
|
||||
|
||||
**AZ-416 (FT-P-09-AP)**
|
||||
|
||||
| AC | Coverage | Status |
|
||||
|----|----------|--------|
|
||||
| AC-1 (signing handshake ≤5 s, no BAD_SIGNATURE) | `test_handshake_passes_when_first_signed_within_window`, `test_handshake_fails_when_no_signed_within_window`, `test_handshake_fails_when_signed_arrives_after_budget`, `test_handshake_fails_on_bad_signature_statustext`, scenario assertion via `observe_signing_handshake` | Covered |
|
||||
| AC-2 (GPS_INPUT ≥4.5 Hz for 5 Hz target) | `test_gps_input_rate_at_5hz_for_60s_passes`, `test_gps_input_rate_at_boundary_passes`, `test_gps_input_rate_below_minimum_fails`, scenario assertion via `compute_gps_input_rate` | Covered |
|
||||
| AC-3 (EK3_SRC1_POSXY == 3) | `test_validate_ek3_src1_posxy_passes_at_3`, scenario via `validate_ek3_src1_posxy(sitl_observer.read_ap_parameter(...))` | Covered |
|
||||
| AC-4 (GPS_RAW_INT healthy fraction ≥80 %) | `test_gps_raw_int_health_all_healthy_passes`, `test_gps_raw_int_health_at_80_pct_boundary_passes`, `test_gps_raw_int_health_below_80_pct_fails`, `test_gps_raw_int_health_eph_threshold_strict`, scenario via `evaluate_gps_raw_int_health` | Covered |
|
||||
| AC-5 (vio_strategy parameterization; `fc_adapter` fixed to `ardupilot`) | scenario uses `vio_strategy` fixture from conftest; `fc_adapter != "ardupilot"` is skipped — collection across 6 variants reduces to 3 active variants | Covered |
|
||||
| D-C8-9 (signing-handshake observability) | `traces_to` marker + handshake report includes `setup_signing_seen` | Covered |
|
||||
|
||||
Also: AZ-416's `mavproxy_tlog_reader.iter_messages` body landed
|
||||
(previously raised NotImplementedError per the AZ-406 commit). 6 unit
|
||||
tests in `test_mavproxy_tlog_reader.py` exercise the parser against
|
||||
synthetic tlogs.
|
||||
|
||||
**AZ-417 (FT-P-09-iNav)**
|
||||
|
||||
| AC | Coverage | Status |
|
||||
|----|----------|--------|
|
||||
| AC-1 (TCP connect to inav-sitl:5760 ≤5 s) | scenario via `sitl_observer.observe_inav_tcp_handshake` (skip-gated) | Covered (gated) |
|
||||
| AC-2 (MSP2_SENSOR_GPS ≥4.5 Hz for 5 Hz target) | `test_compute_rate_at_target_passes`, `test_compute_rate_at_boundary_passes`, `test_compute_rate_below_minimum_fails`, `test_compute_rate_filters_function_id`, scenario via `compute_rate_hz` | Covered |
|
||||
| AC-3 (fix_type ≥3, provider=MSP, numSat matches emitted) | `test_evaluate_gps_state_passes_at_minimum_fix`, `test_evaluate_gps_state_fails_on_low_fix_type`, `test_evaluate_gps_state_fails_on_wrong_provider`, `test_evaluate_gps_state_fails_on_num_sat_mismatch`, scenario via `evaluate_inav_gps_state` | Covered |
|
||||
| AC-4 (vio_strategy parameterization; `fc_adapter` fixed to `inav`) | scenario uses `vio_strategy` fixture; skips when `fc_adapter != "inav"` | Covered |
|
||||
|
||||
**AZ-419 (FT-P-11)**
|
||||
|
||||
| AC | Coverage | Status |
|
||||
|----|----------|--------|
|
||||
| AC-1 (operator_manifest: estimate ≤50 m of A; FDR `cold_start_origin.set(source="manifest")`) | `test_evaluate_operator_manifest_passes_at_origin`, `test_evaluate_operator_manifest_passes_just_inside_budget`, `test_evaluate_operator_manifest_fails_just_outside_budget`, scenario assertion | Covered |
|
||||
| AC-2 (fc_ekf: estimate ≤50 m of FC EKF snapshot; FDR `source="fc_ekf"`) | `test_evaluate_fc_ekf_passes`, scenario assertion | Covered |
|
||||
| AC-3 (no origin → SUT refuses takeoff; FDR `cold_start_origin.unavailable`) | `test_evaluate_no_origin_passes_when_silent_and_fdr_records_abort`, `test_evaluate_no_origin_fails_when_sut_emits_anything`, `test_evaluate_no_origin_fails_when_fdr_missing_unavailable_signal`, dedicated scenario `test_ft_p_11_cold_start_no_origin_aborts` | Covered |
|
||||
| AC-4 (bounded-delta conflict: operator wins; source_label != satellite_anchored; FDR `gps_bounded_delta.reject`) | `test_evaluate_bounded_delta_conflict_operator_wins`, `test_evaluate_bounded_delta_fails_when_label_is_satellite_anchored`, scenario assertion (third parametrize variant) | Covered |
|
||||
| AC-5 (parameterization across `fc_adapter, vio_strategy, origin_source`) | scenario uses conftest's `fc_adapter` + `vio_strategy`; parametrizes `origin_source ∈ {operator_manifest, fc_ekf, bounded_delta_conflict}` separately | Covered |
|
||||
|
||||
ADR-010 Principle #11 amended ("operator origin wins on bounded-delta
|
||||
conflict; FC GPS logged as suspect") explicitly encoded as
|
||||
`BOUNDED_DELTA_TRIGGER_M = 200.0` + the `c5.gps_bounded_delta.reject`
|
||||
record audit.
|
||||
|
||||
### Phase 3 — Code Quality
|
||||
|
||||
* **Single responsibility**: `mavproxy_tlog_reader` only iterates/counts
|
||||
tlog frames (file I/O concern); `ap_contract_evaluator` only consumes
|
||||
`TlogMessage` iterables (analytics concern); `msp_frame_observer`
|
||||
only consumes captured MSP samples. `cold_start_evaluator` is one
|
||||
module because the three FT-P-11 variants share a single FDR record
|
||||
vocabulary + Manifest schema; splitting them would force the scenario
|
||||
to import three near-identical modules.
|
||||
* **No suppressed errors**: `mavproxy_tlog_reader.iter_messages`
|
||||
catches the narrow `BAD_DATA` + per-message `to_dict` exceptions
|
||||
(documented in pymavlink) and continues, but the file-not-found
|
||||
+ connection-close paths raise / surface naturally. No bare `except`
|
||||
in any new module.
|
||||
* **AAA comment discipline**: every test uses `# Arrange / # Act /
|
||||
# Assert`; sections omitted when not needed.
|
||||
* **No narration comments**: docstrings explain non-obvious intent
|
||||
(AC mapping, why orphans excluded, why `materialize_to_list` exists,
|
||||
why `EK3_SRC1_POSXY = 3` is the only acceptance value).
|
||||
|
||||
### Phase 4 — Security
|
||||
|
||||
* **No SUT imports**: confirmed by `test_no_sut_imports.py` (passing in
|
||||
the full suite). None of the new modules import from
|
||||
`src.gps_denied_onboard`.
|
||||
* **Signing handshake stance**: the helper does NOT validate signatures
|
||||
itself (that's pymavlink's job); it only counts signed-frame arrivals
|
||||
and `BAD_SIGNATURE` STATUSTEXT incidents. If signing fails in any way
|
||||
AC-1 fails — the scenario does NOT bypass to an unsigned channel
|
||||
(per spec "Forbidden" list).
|
||||
* **No secrets in source**: the AP scenario looks up
|
||||
`mavlink-test-passkey.txt` from the on-disk fixture (already
|
||||
verified by `test_passkey_files_match` in `test_directory_layout.py`).
|
||||
The passkey itself is the AZ-407 / AZ-408 fixture, NOT a production
|
||||
key.
|
||||
* **No SQL/shell injection surface**: all helpers operate on bytes /
|
||||
pathlib / dict; no subprocess calls in the helper layer (subprocess
|
||||
for `msp_gps_toy` is the SITL-observer's responsibility).
|
||||
|
||||
### Phase 5 — Performance
|
||||
|
||||
* `mavproxy_tlog_reader.iter_messages` is a single pass over the tlog;
|
||||
pymavlink's `recv_match(blocking=False)` is the standard idiom.
|
||||
* `ap_contract_evaluator` consumes the materialised list ONCE per
|
||||
analyser; `collect_messages_to_list` is the documented choice
|
||||
(mavlink_connection's iterator closes on exhaustion so re-iteration
|
||||
isn't safe). For typical 60 s of mavproxy traffic at ~50 msg/s this
|
||||
is ≤3000 messages → trivial in memory.
|
||||
* `cold_start_evaluator._scan_fdr_for_cold_start` is one pass.
|
||||
* No nested loops over the same data.
|
||||
|
||||
### Phase 6 — Cross-Task Consistency
|
||||
|
||||
* **Pattern parity with batches 69 + 70 + 71**:
|
||||
- Skip gate (`_*_harness_implemented` fixture) for missing upstream
|
||||
replay/SITL/FDR helpers — same pattern as
|
||||
`test_ft_p_02/04/05/07/08/10_*`.
|
||||
- `_NullSink` probe — same idiom as the prior 5 scenario files.
|
||||
- Evidence side-channel via `nfr_recorder.record_metric(name, value,
|
||||
ac_id=…)` — same pattern as `test_ft_p_01/04/05/07/08/10_*`.
|
||||
- Module-level constants (`UPPER_SNAKE`) for budgets — matches
|
||||
`multi_segment_evaluator`, `mre_evaluator`, `smoothing_evaluator`,
|
||||
`sharp_turn_detector`.
|
||||
- Helper modules importable from `runner.helpers.*`.
|
||||
* **No drift**: scenarios reuse the helper's constants (no magic
|
||||
numbers) — `HANDSHAKE_BUDGET_S`, `GPS_INPUT_MIN_RATE_HZ`,
|
||||
`MIN_FIX_TYPE`, `ACCURACY_BUDGET_M`, `BOUNDED_DELTA_TRIGGER_M`,
|
||||
`FDR_RECORD_*`, `FORBIDDEN_FIRST_LABEL_BOUNDED_DELTA`.
|
||||
* **No legacy NotImplementedError test left behind**: verified no test
|
||||
asserts `iter_messages` raises NotImplementedError (was AZ-406's
|
||||
surface contract; AZ-416 owns the body per docstring).
|
||||
|
||||
### Phase 7 — Architecture Compliance
|
||||
|
||||
* **Public-boundary discipline**: confirmed by `test_no_sut_imports.py`
|
||||
(passing). Helpers consume pymavlink (a third-party MAVLink
|
||||
reference impl, not SUT internals) + FDR record schema (record_type
|
||||
+ payload dict) + outbound estimate schema. The signing handshake
|
||||
observer specifically does NOT import the SUT's signing-key state
|
||||
per the spec "Forbidden" list.
|
||||
* **Directory layout**: new paths added to `test_directory_layout.py`
|
||||
parametrize list (`runner/helpers/{msp_frame_observer,
|
||||
ap_contract_evaluator, cold_start_evaluator}.py`,
|
||||
`tests/positive/test_ft_p_{09_ap_signing, 09_inav, 11_cold_start_init}.py`).
|
||||
All variants pass.
|
||||
* **Determinism**: all helpers are deterministic — no `time.time()`,
|
||||
no RNG; pymavlink parses bytes deterministically.
|
||||
|
||||
### Phase 8 — Test Suite Health
|
||||
|
||||
* Total: **460 passed in 134.35 s** (was 393 at end of batch 71).
|
||||
* New tests this batch: **+67** (msp_frame_observer: 14;
|
||||
mavproxy_tlog_reader: 6; ap_contract_evaluator: 22;
|
||||
cold_start_evaluator: 19; directory_layout new entries: 6).
|
||||
* Pre-existing macOS-only `/e2e-results` plugin issue still present —
|
||||
affects scenario test invocation outside Docker only; unit suite
|
||||
unaffected. Out of batch scope.
|
||||
|
||||
## Cross-Task Consistency Verdict
|
||||
|
||||
PASS — no cross-task drift, no duplicated logic across the four new
|
||||
helpers, shared `TlogMessage` type used consistently between
|
||||
`mavproxy_tlog_reader` and `ap_contract_evaluator`.
|
||||
|
||||
## Architecture Compliance Verdict
|
||||
|
||||
PASS — public-boundary blackbox stance preserved; no SUT imports;
|
||||
pymavlink boundary correctly placed at the tlog reader.
|
||||
|
||||
## Final Verdict
|
||||
|
||||
**PASS** — Batch 72 (AZ-416 + AZ-417 + AZ-419) ready for commit.
|
||||
Reference in New Issue
Block a user