# Cumulative Code Review — Batches 82-84 (Cycle 1)

**Window**: batches 82, 83, 84
**Tasks covered**: AZ-421, AZ-422, AZ-423, AZ-427
**Date**: 2026-05-17
**Verdict**: PASS
**Last cumulative review**: batches 79-81 (`_docs/03_implementation/reviews/cumulative_79_81_review.md`)

## Scope

Union of files changed since the last cumulative review. Counts include the modifications to `e2e/_unit_tests/test_directory_layout.py` that each batch made.

### Files in window

| Batch | Tasks | New helpers | New scenarios | New unit tests |
|-------|-------|-------------|---------------|----------------|
| 82 | AZ-421 (FT-P-15 + FT-P-16 + FT-P-18) | `tile_cache_inspector` | `test_ft_p_15_cache_schema`, `test_ft_p_16_offline_only`, `test_ft_p_18_no_raw_retention` | `test_tile_cache_inspector` |
| 83 | AZ-422 (FT-P-17 + FT-N-06) | `mid_flight_tile_evaluator`, `mock_suite_sat_audit` | `test_ft_p_17_mid_flight_tiles`, `test_ft_n_06_mid_flight_freshness` | `test_mid_flight_tile_evaluator`, `test_mock_suite_sat_audit` |
| 84 | AZ-423 + AZ-427 (FT-P-19 + FT-N-05) | `retrieval_evaluator`, `aged_tile_rejection_evaluator` | `test_ft_p_19_sat_reloc_scale`, `test_ft_n_05_stale_tile_rejection` | `test_retrieval_evaluator`, `test_aged_tile_rejection_evaluator` |

Total: 5 new helper modules + 7 new scenario files + 5 new unit-test files across 4 tasks / 11 complexity points (3 + 3 + 3 + 2).

## Cross-Cutting Themes

### Theme 1 — FDR vocabulary fragmentation

Each new helper defines its own FDR record-kind constants. As of batch 84:

| Constant | Defined in |
|----------|------------|
| `MID_FLIGHT_TILE_FDR_KIND = "mid-flight-tile-output"` | `mid_flight_tile_evaluator.py` |
| `TILE_LOAD_REJECTED_FDR_KIND = "tile-load-rejected"` | `mid_flight_tile_evaluator.py` AND `aged_tile_rejection_evaluator.py` |
| `TILE_LOAD_REJECTED_STALE_REASON = "stale"` | same — duplicated |
| `RETRIEVAL_TOPK_FDR_KIND = "retrieval-topk"` | `retrieval_evaluator.py` |
| `SCENE_CHANGE_MATCH_FDR_KIND = "scene-change-match"` | `retrieval_evaluator.py` |

The `tile-load-rejected` constant is the only true duplication; the rest live in one place. The duplication is intentional in spirit (each helper exposes the vocabulary it consumes) but creates a small coordination cost — surfaced as a Low finding in batch 84 review (F2).

Recommendation (NOT blocking this window): consolidate into a single `runner/helpers/fdr_record_kinds.py` constants module. Hygiene PBI candidate (~1 point).

### Theme 2 — Geo math consolidation progress

`runner.helpers.geo.distance_m` (Vincenty WGS84) is now consumed by:

- `mid_flight_tile_evaluator.evaluate_dedup` (Batch 83, AC-3)
- `retrieval_evaluator.evaluate_top_k_within_distance` (Batch 84, AC-1)

Both via `from .geo import distance_m`. This is the correct pattern.

`runner.helpers.gcs_telemetry_evaluator.haversine_distance_m` (Batch 81) still exists as the lone outlier — surfaced in the prior cumulative review (79-81) as F1 / Low / Maintainability. Recommendation unchanged: refactor in a dedicated hygiene batch (≤1 point) before any further geo-math additions land.

### Theme 3 — Tests-as-gates discipline

Every scenario in this window follows the same skip/fail-loud pattern:

* If `sitl_replay_ready` is false → `pytest.skip(<rich diagnostic message>)`.
* If `sitl_replay_ready` is true BUT the required FDR records / env vars are missing → `pytest.fail(<rich diagnostic message>)`.

This ensures production-code or fixture gaps surface immediately when the SITL fixture is wired up, while CI runs without the fixture remain green.

Aggregate production-dependency list (collected from batches 82-84 batch reports):

* SUT-side FDR record kinds: `mid-flight-tile-output`, `tile-load-rejected` (with `reason="stale"`), `retrieval-topk`, `scene-change-match`, `cache-self-check`.
* SUT-side public-input mechanisms: `simulate_landing()` MAVLink command.
* SUT-side production logic: C6 freshness gate sector-aware date comparison, C2 top-K=10 retrieval result emission, cross-domain matcher per-pair output emission, mid-flight orthorectification + tile generation + Mode B Fact #105 schema population.
* Fixture-builder-side: `FT_P_17_HIGH_QUALITY_WINDOW_S` env var, `E2E_FT_N_05_FIXTURE` env var, `synth-age-7mo` / `synth-age-13mo` snapshots, Derkachi 5-min replay scenario, Docker network inspect snapshots (FT-P-16).

All gated, all surfaced via skip messages with explicit fixture-owner references. This list feeds the AZ-595 (fixture-builder) backlog and the SUT-side production work.

### Theme 4 — Source-label contract centralisation

`aged_tile_rejection_evaluator` (Batch 84) reuses `ALLOWED_SOURCE_LABELS` from `estimate_schema` (AZ-411 deliverable). This is the correct pattern — the FT-P-03 / AC-1.4 source-label set is a one-source contract. No drift; new helpers should follow this lead when they need the same set.

### Theme 5 — Helper-module size + complexity

| Helper | Lines | Top-level public symbols |
|--------|-------|--------------------------|
| `tile_cache_inspector.py` | ~450 | 4 dataclasses + 4 evaluators + walk + JPEG probe |
| `mid_flight_tile_evaluator.py` | ~500 | 6 dataclasses + 6 evaluators + 2 projection helpers |
| `mock_suite_sat_audit.py` | ~77 | 1 function (`fetch_audit`) + constants |
| `retrieval_evaluator.py` | ~260 | 6 dataclasses + 2 evaluators + 2 projectors + 2 CSV writers + 2 iter filters |
| `aged_tile_rejection_evaluator.py` | ~190 | 3 dataclasses + 1 evaluator + 1 projector + 1 iter filter + constants |

All evaluators stay under the 50-line function complexity threshold. Helper surfaces are coherent — no single helper does too many things.

## Spec Compliance

Spot-checked each batch's per-batch review verdict; no contradictions surfaced under cumulative inspection.

Cumulative AC coverage (E-BBT blackbox tests, this window):

| AC ID | Task | Status |
|-------|------|--------|
| AC-8.4 | AZ-422 | Covered (FT-P-17 5 sub-evaluators) |
| AC-NEW-6 (mid-flight) | AZ-422 | Covered (FT-N-06 2 sub-evaluators) |
| AC-8.2 (cache schema + offline) | AZ-421 | Covered (FT-P-15, FT-P-16, FT-P-18) |
| AC-NEW-1 (no raw retention) | AZ-421 | Covered (FT-P-18) |
| AC-8.6 | AZ-423 | Covered (FT-P-19 scale + scene-change PARTIAL) |
| AC-NEW-6 (stale aged) | AZ-427 | Covered (FT-N-05 2 sub-cases) |
| AC-8.2 (sector freshness) | AZ-427 | Covered |

No carried-over AC misses; no AC drift between batches.

## Code Quality

- **SOLID / SRP**: each helper has one reason to change. No God modules. The `mid_flight_tile_evaluator` has 6 evaluators — at the upper end but still cohesive (all consume the same `TileSpec` input from the same FDR record kind).
- **Error handling**: every input validator raises `ValueError`; every HTTP wrapper raises `RuntimeError`. No bare `except`. `_parse_iso8601_utc_seconds` catches only `(TypeError, ValueError)`.
- **Test pyramid**: unit tests dominate (52 in batch 83, 48 in batch 84, ~30 in batch 82). Each scenario has a corresponding pure-logic unit-test file. Scenario tests delegate to evaluators for all AC math, asserting only `report.passes` + diagnostic context.
- **Naming**: `evaluate_*` for evaluators, `project_*` for FDR-payload projectors, `iter_*` for record-kind filters, `<Concern>Report` for report dataclasses. Consistent across all 5 new helpers.
- **Comments**: docstrings cite AC IDs and Mode B Fact / restriction references. Inline comments are minimal and only where non-obvious. Arrange/Act/Assert pattern present in every unit test.
- **Dead code**: none. Re-verified `_top_level_field_to_attr` (Batch 83) is a documented forward-compatibility seam.

## Security Quick-Scan

- No SQL, no `subprocess` invocations, no `exec`/`eval`.
- No hardcoded secrets, no logging of sensitive data.
- HTTP client (`mock_suite_sat_audit`) validates URL non-empty, run_id non-empty, status 2xx, JSON body is dict, `entries` is list. Error messages truncate body to 200 chars.
- `aged_tile_rejection_evaluator` separates `illegal_labels` (contract violation) from `anchored_count` (freshness-gate violation) so a defect can't mask the other.

## Performance Scan

- `evaluate_dedup` is O(N²) — documented + bounded by < 100 tiles per 5-min replay.
- `evaluate_top_k_within_distance` is O(N·K) = 60 × 10 = 600 Vincenty calls per scenario.
- `evaluate_aged_tile_rejection` is O(M) where M ≤ 60.
- FDR iteration is generator-based via `fdr_reader.iter_records` (unchanged from prior windows).
- No N+1, no unbounded fetches, no async-in-sync, no hot-path allocations.

## Cross-Task Consistency

- All four tasks consume the same conftest fixtures (`sitl_replay_ready`, `fc_adapter`, `vio_strategy`, `evidence_dir`, `nfr_recorder`, `run_id`, plus task-specific helpers).
- `traces_to` marker convention consistent.
- Skip messages have a uniform shape: "<scenario> requires <fixture>; <pure-logic alternative path>".
- CSV evidence file naming: `ft-<id>-<aspect>-<fc>-<vio>.csv`.
- TileSpec / TilePublishRequest mirror is single-source in `mid_flight_tile_evaluator`.

## Architecture Compliance (Phase 7)

Verified against `_docs/02_document/module-layout.md`:

- **Layer direction**: all new files under `e2e/` (Blackbox Tests). No imports from `src/gps_denied_onboard`. Public-boundary discipline notes present in every new helper.
- **Public API respect**: cross-component imports limited to `runner.helpers.{geo, estimate_schema, fdr_reader, accuracy_evaluator}` — all helpers that are intra-component to the Blackbox Tests package.
- **No new cyclic dependencies**: import graph for new files is a tree (helpers → geo / estimate_schema; tests → helpers).
- **Duplicate symbols**: `TILE_LOAD_REJECTED_FDR_KIND` appears twice (intra-component duplicate, surfaced in F2 below). No other duplicates.
- **Cross-cutting concerns**: no concern that should live in `shared/*` has been re-implemented locally. Geo, schema, CSV emission, HTTP all delegate.

## Baseline Delta

`_docs/02_document/architecture_compliance_baseline.md` does not exist for the Blackbox Tests workspace (this workspace was greenfield-bootstrapped); no Baseline Delta section to emit.

## Findings

| # | Severity | Category | Status | Title |
|---|----------|----------|--------|-------|
| F1 | Low | Maintainability | Carried over from 79-81 | `gcs_telemetry_evaluator.haversine_distance_m` duplicates `geo.distance_m` |
| F2 | Low | Maintainability | New in 82-84 (B84 review F2) | `TILE_LOAD_REJECTED_FDR_KIND` + stale-reason constant duplicated across two helpers |

### F1 (carry-over)

- **Location**: `e2e/runner/helpers/gcs_telemetry_evaluator.py`
- **State**: still present. No batch in this window touched it.
- **Recommendation unchanged**: consolidate to `geo.distance_m` in a dedicated ≤1-pt hygiene batch.

### F2 (newly introduced)

- **Location**: `e2e/runner/helpers/mid_flight_tile_evaluator.py:43-44`, `e2e/runner/helpers/aged_tile_rejection_evaluator.py:33-34`
- **Description**: Two helpers define the same FDR record-kind constant + stale-reason string. The duplication exists because each helper aims to be self-contained, but the underlying contract is single-source (FDR record-type vocabulary).
- **Recommendation**: address in the same hygiene batch as F1 by creating `runner/helpers/fdr_record_kinds.py` and importing the constants from there. Combine with F1 → estimated 2-pt hygiene batch.

## Resolved Findings

None. F1 from the prior window still stands; no fix landed in this window.

## Carried Forward to Next Window (85-87)

- F1, F2 unresolved.
- Production-dependency list above is the AZ-595 fixture-builder + epic-side SUT work pending. Tracker visibility: the items are surfaced in each batch report; an aggregator PBI for the fixture-builder side is recommended but not required this cycle.

## Verdict

0 Critical, 0 High, 2 Low (1 carry-over + 1 new). No FAIL trigger; cumulative posture remains **PASS** with two known maintainability items eligible for a dedicated hygiene batch.

Batch 84 proceeds to commit + push. Next cumulative review covers batches 85-87.
