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

Code Review — Batch 87

Tasks: AZ-436 + AZ-437 + AZ-438 + AZ-439 (NFT-SEC-01..05 security scenarios) Total complexity: 16 points (5 + 3 + 3 + 5) Reviewer: autodev / implement skill phase 8 Date: 2026-05-17 Verdict: PASS_WITH_WARNINGS (5 Low; 0 Medium / High / Critical)

Phase 1 — Context

Per _dependencies_table.md these four NFT-SEC tasks complete the security NFT slice of E-BBT (AZ-262). All five scenarios (cache poisoning, no-egress, MAVLink signing, OpenCV CVE probe, OpenCV CVE ASan fuzz, DNS blackhole) follow the established batch-85 / batch-86 "helper + fixture-consumer scenario + unit-test trio" pattern.

Public-boundary discipline: every new helper module declares it in its module docstring; grep confirms none import src/gps_denied_onboard.

Phase 2 — Spec Compliance

Task	AC	Coverage location
AZ-436	AC-1 (N flights complete)	_parse_payload + len(flights) < NFT_SEC_01_CI_MIN_FLIGHTS gate + scenario flight_count NFR record
AZ-436	AC-2 (poison ratio + layer coverage + rejection-reason vocabulary)	passes_ratio + passes_layer_coverage + passes_rejection_reason_vocabulary (3 sub-assertions)
AZ-436	AC-3 (false-trust budget zero-tolerance)	passes_budget + scenario total_false_trust + budget NFR records
AZ-436	AC-4 (canonical-only default; release-gate full matrix)	_is_canonical_param + E2E_NFT_SEC_01_RELEASE_GATE env var
AZ-437	AC-1 (NFT-SEC-02 0 packets to non-internal)	evaluate_no_egress.passes + scenario AC-1 assert
AZ-437	AC-2 (NFT-SEC-05 sidecar healthy)	sidecar_healthy field + scenario AC-2 assert
AZ-437	AC-3a (NFT-SEC-05 lookup fails)	passes_lookup + scenario AC-3a assert
AZ-437	AC-3b (NFT-SEC-05 UDP-53 silent)	passes_udp_silence + scenario AC-3b assert
AZ-437	AC-4 (parameterization)	conftest matrix unchanged; both scenarios run per (fc_adapter, vio_strategy)
AZ-438	AC-1 (iNav SKIP)	if fc_adapter == "inav": pytest.skip(...)
AZ-438	AC-2/3/4 (per-sub-case rejection ≤500ms + no position update)	per-sub-case passes_rejection + passes_no_position_update
AZ-438	AC-5 (vio_strategy parameterization)	conftest matrix
AZ-439	AC-1a (probe: no crash)	passes_no_crash (last FDR record ≥ probe injection)
AZ-439	AC-1b (probe: graceful outcome)	passes_graceful_outcome (decode-success OR frame-decode-error within ±50 ms tolerance)
AZ-439	AC-2 (fuzz: 0 ASan findings ≥4 h)	passes_findings + passes_duration + scenario AC-2 assert
AZ-439	AC-3 (fuzz: ≥1000 corpus)	reached_corpus_floor (informational only — recorded in CSV; not asserted, matches spec "informational only; no hard threshold")
AZ-439	AC-4 (parameterization)	probe = full matrix; fuzz = ardupilot + per-vio (avoids duplicating a 4 h run, justified inline)

Spec compliance: PASS. AC-3 on AZ-439 fuzz is correctly modeled as informational-only per the task spec.

Phase 3 — Code Quality

• All five helpers use frozen dataclasses for verdicts. Single-responsibility: evaluators do verdict logic only; scenarios do fixture I/O + assertions.
• No 2>/dev/null, bare except, or empty try-catch blocks anywhere.
• Comment discipline upheld — only # Arrange/# Act/# Assert in tests; inline comments in helpers explain non-obvious intent (e.g. why ASan threshold patterns are explicitly enumerated rather than wildcard-matched).
• All paths are typed; Sequence / frozen=True / Enum / Path annotations appear consistently.

Phase 4 — Security Scan

• No new credentials, secrets, or API keys introduced.
• cve_probe_evaluator deliberately rejects "silent drop" outcomes as failure — preventing a regression where the SUT silently swallows malformed JPEGs.
• asan_fuzz_evaluator classifies any unknown ASan match as OTHER_FINDING and still fails — preventing a regression where a future sanitizer category is silently accepted.
• egress_observer treats SUCCESS DNS lookup as the only failing outcome.
• mavlink_signing_evaluator requires BOTH rejection STATUSTEXT AND no-position-update for pass — catches signaling-only rejection bug class.
• Test data (poisoned tile generators, BAD_SIGNATURE regex) does not contain exploit payloads, only string patterns + benign synthetic counters.

Verdict: PASS.

Phase 5 — Performance Scan

• Evaluators are O(N) over their input collections; no quadratic loops or unbounded recursion.
• No I/O in evaluator hot paths; CSV writing is one-shot at scenario end.
• AZ-436 default N=1000 with single canonical param keeps the per-CI cost bounded; release-gate N=10000 × 6 params is correctly opt-in via env flag.
• AZ-439 fuzz is correctly release-gated (≥4 h is too expensive for CI).

Verdict: PASS.

Phase 6 — Cross-Task Consistency

• All six scenarios share an almost-identical structure:
  1. tier-guard skip (where applicable),
  2. sitl_replay_ready skip,
  3. fixture-path resolution,
  4. fixture-not-found → pytest.fail with explicit production-dep pointer,
  5. payload parse → typed records (with pytest.fail on shape errors),
  6. evaluator call → CSV evidence + NFR records,
  7. AC assertions with diagnostic messages.
• The five helpers share the write_csv_evidence pattern (one row per result + aggregate footer where applicable).

Verdict: PASS (consistency upheld).

Phase 7 — Architecture Compliance

• All new files under e2e/ — owned by the Blackbox Tests cross-cutting component per _docs/02_document/module-layout.md.
• No src/gps_denied_onboard imports (verified by inspection of every new file's import section).
• New evaluators are leaves of the import DAG — they import only stdlib + the existing runner.helpers.sitl_observer for fixture-path resolution.
• No new infrastructure libraries; all helpers use stdlib csv, dataclasses, enum, re, pathlib, math.

Verdict: PASS.

Findings

F1 — write_csv_evidence boilerplate continues to grow (Low / Maintainability — carry-over of batch-85 F4 + batch-86 F1)

Each of the 5 new evaluators adds its own write_csv_evidence(out_path, report) -> Path with the same header-then-rows pattern. The duplication is now spread across 13 helpers (batches 85, 86, 87 combined).

Verdict: defer to a future hygiene PBI; the per-evaluator schema variation makes a generic abstraction non-trivial (each report's row shape differs significantly — aggregate row vs per-sub-case, single row vs many).

F2 — DNS-blackhole sidecar production dependency not yet realized (Low / Spec-Gap surfacing)

The NFT-SEC-05 scenario depends on a DNS-blackhole sidecar (per environment.md) that must be configured to absorb all UDP-53 probes. This sidecar does not exist in the e2e harness today (no service entry in e2e/docker/docker-compose.test.yml and no dns-blackhole directory under e2e/fixtures/).

Surfaced to AZ-595 + e2e infrastructure: the DNS-blackhole sidecar must be wired before NFT-SEC-05 can run end-to-end with live captures.

Verdict: not a code defect — surfaced as a production dependency in the batch report.

F3 — AP MAVLink 2.0 signing handshake required for NFT-SEC-03 (Low / Spec-Gap surfacing)

NFT-SEC-03 assumes AP has the MAVLink 2.0 signing handshake completed (SETUP_SIGNING exchange + passkey installed) before the test runs. That handshake is owned by FT-P-09-AP (AZ-416 — already in done/); the NFT-SEC-03 fixture builder (AZ-595) must invoke that handshake first when generating its replay.

Verdict: not a code defect — production dependency on AZ-595 noted in the scenario docstring.

F4 — _resolve_fixture_path duplicated across 6 scenarios (Low / Maintainability)

Carry-over of batch-85 F3 + batch-86 F4. The six new security scenarios each define their own _resolve_fixture_path() -> Path with identical logic differing only in env var name + default filename.

Verdict: defer to a future hygiene PBI alongside the NFT-PERF and NFT-RES instances.

F5 — ASan-fuzz AC-3 corpus floor is informational-only (Low / Spec-aligned)

AsanFuzzReport.reached_corpus_floor is correctly computed but does NOT contribute to passes (and is NOT asserted in the scenario). This matches the task spec's "informational only; no hard threshold" wording exactly.

Verdict: not a defect — flagged so a future reviewer doesn't mistake the read-only field for missing coverage.

Auto-Fix Gate

5 Low findings; no Critical / High / Medium. Per the implement-skill auto-fix gate, no auto-fix actions are triggered. F1 + F4 are deferred to hygiene PBIs; F2 + F3 are production dependencies surfaced to the cumulative review + AZ-595 fixture builder; F5 is a documented design decision matching the spec.

Final Verdict

PASS_WITH_WARNINGS — proceed to commit + tracker transition.