# Cumulative Code Review — Cycle 2 — Batches 98–102

**Date**: 2026-05-20
**Scope**: union of files changed across cycle-2 batches 98–102 (AZ-697, AZ-702, AZ-698, AZ-699, AZ-700, AZ-701)
**Mode**: cumulative (all 7 phases)
**Verdict**: **PASS_WITH_WARNINGS** (0 Critical, 0 High, 1 Medium, 1 Low)
**Baseline file**: `_docs/02_document/architecture_compliance_baseline.md` — **absent**, no `## Baseline Delta` section emitted (see Notes)

## Findings

| # | Severity | Category | File | Title |
|---|----------|----------|------|-------|
| F1 | Medium | Architecture | `_docs/02_document/module-layout.md` | Module layout stale — 6 new artefacts unregistered |
| F2 | Low | Maintainability | `src/gps_denied_onboard/replay_api/app.py:163-180` | Inline imports for stdlib + first-party modules |

### Finding Details

**F1: Module layout stale — 6 new artefacts unregistered** (Medium / Architecture)

- Location: `_docs/02_document/module-layout.md`
- Description: cycle-2 introduced six new package/file artefacts that are not registered in the authoritative file-ownership map:
  - **New top-level package** `src/gps_denied_onboard/replay_api/` (AZ-701; 7 production files). No Per-Component Mapping entry, no Layering-table row, no Build-Time Exclusion Map row.
  - **New files** in `src/gps_denied_onboard/replay_input/`: `tlog_ground_truth.py` (AZ-697) and `errors.py`. The Shared `replay_input` entry still lists only `__init__.py`, `interface.py`, `tlog_video_adapter.py`, `auto_sync.py`, `tests/`.
  - **New CLI entrypoints** `src/gps_denied_onboard/cli/render_map.py` (AZ-700) and `src/gps_denied_onboard/cli/replay_api_entrypoint.py` (AZ-701). The Shared section has an entry for `cli/replay` but not for the two new console scripts.
  - **New helpers** `src/gps_denied_onboard/helpers/gps_compare.py` (AZ-697/699) and `src/gps_denied_onboard/helpers/accuracy_report.py` (promoted in AZ-701). The Shared/helpers entries do not list them.
- Impact: `/implement` Step 4 (File Ownership) resolves a task's Component field against this file. Any future task touching `replay_api/`, the new CLI scripts, or these helpers will hit the BLOCKING ownership check at Step 4 — the skill explicitly STOPs when the component isn't found and forbids guessing from prose.
- Suggestion: Step 13 (Update Docs) for cycle 2 should append: (a) a `replay_api` component entry under "Per-Component Mapping" with Layer-5 classification and `[operator-tools]` build gate, (b) an updated `shared/replay_input` files list, (c) `shared/cli/render_map` and `shared/cli/replay_api` entries, (d) `shared/helpers/gps_compare` and `shared/helpers/accuracy_report` entries with Consumed-by lists.
- Task: AZ-701 (primary), AZ-697 / AZ-699 / AZ-700 (secondary)

**F2: Inline imports for stdlib + first-party modules** (Low / Maintainability)

- Location: `src/gps_denied_onboard/replay_api/app.py:163-180` (`_maybe_render_report` method body)
- Description: `_maybe_render_report` lazy-imports `json` (stdlib) plus three first-party modules (`helpers.accuracy_report`, `helpers.gps_compare`, `replay_input`). The same first-party modules are imported eagerly elsewhere in the call graph — `helpers/__init__.py` already pulls `accuracy_report` and `gps_compare` at package import time, and `app.py` itself eagerly imports neighbours like `replay_api.errors` / `interface` / `handlers` at the module top. Lazy import provides no measurable startup-time benefit here and inconsistently styles the file.
- Suggestion: hoist these imports to the top of `app.py`. Keep them inside the `try:` block only if there is a runtime reason (e.g. defensive import to tolerate missing optional dependency) — none of these four modules is optional in `[operator-tools]`.
- Task: AZ-701

## Verdict Logic

- 0 Critical → no FAIL trigger
- 0 High → no FAIL trigger
- 1 Medium → triggers PASS_WITH_WARNINGS
- 1 Low → triggers PASS_WITH_WARNINGS
- Result: **PASS_WITH_WARNINGS** — implement skill auto-fix gate does not block; findings reported as info to the user.

## Phase-by-Phase Notes

### Phase 1 — Context Loading

Inputs read:
- Task specs: `AZ-697`, `AZ-698`, `AZ-699`, `AZ-700`, `AZ-701`, `AZ-702` (all in `_docs/02_tasks/done/`)
- Batch reports: `batch_98_cycle2_report.md` … `batch_102_cycle2_report.md`
- Architecture / layout: `_docs/02_document/module-layout.md`, `_docs/02_document/architecture.md`
- Restrictions / solution overview: not re-read (already covered in per-batch reviews)

### Phase 2 — Spec Compliance

Per-batch code reviews (Step 9 of `/implement`) already verified each task's ACs against its implementation. Cumulative scope checks **only** the points where cross-batch promises interact:

- AZ-701 contract requires consuming AZ-700's `gps-denied-render-map` and AZ-699's accuracy report. Confirmed:
  - `replay_api/app.py::_maybe_render_map` invokes `gps-denied-render-map` via `subprocess.run` with argv list. ✅
  - `replay_api/app.py::_maybe_render_report` calls `helpers.accuracy_report.render_report` directly. ✅
- AZ-697's `helpers/gps_compare.py` (`l2_horizontal_m`, `match_percentage`, `GroundTruthRow`) is consumed by AZ-699's `HorizontalErrorDistribution` and by AZ-701's runner. No duplicate implementations.
- AZ-698's `--auto-trim` CLI flag in `cli/replay.py` is invoked from AZ-701's subprocess argv when `inputs.auto_trim` is set. Wired correctly.

No Spec-Gap findings.

### Phase 3 — Code Quality

All cycle-2 production modules have module docstrings explaining intent. No 50+-line functions detected; all DTOs are `@dataclass(frozen=True[, slots=True])` matching the project pattern. Tests follow Arrange/Act/Assert per coderule.

The only quality nit is F2 (inline imports in `_maybe_render_report`).

### Phase 4 — Security Quick-Scan

- `subprocess.run` in `replay_api/app.py` uses argv lists (no `shell=True`). ✅
- `subprocess.run` has `timeout` parameter. ✅
- `json.loads` on JSONL emissions in `cli/render_map.py` — safe (json, not pickle).
- Bearer-token validation in `replay_api/handlers.py`; default-on (`REPLAY_API_AUTH_REQUIRED=true` unless explicit dev opt-out).
- Magic-byte validation on multipart uploads (AC-9): MAVLink `\xfd`/`\xfe` for `.tlog`, `ftyp` for `.mp4`.
- Per-job storage directory is wiped on `release_job` and `cleanup_all` — no residual artefacts.
- Subprocess zero-bytes signing-key file (`signing_key_path.write_bytes(b"\x00" * 32)`) — acceptable in replay mode (paired with `noop_mavlink_transport`; outbound bytes are never transmitted) and the file is scoped to the per-job temp dir. Not a finding.

No Security findings.

### Phase 5 — Performance Scan

- `replay_api/jobs.py` uses `ThreadPoolExecutor` with `max_concurrent` and `max_queued` caps; excess returns HTTP 429. ✅
- `python-multipart` buffers each part in memory; bounded by `REPLAY_API_MAX_UPLOAD_BYTES`. Documented limitation, not a finding.
- `helpers/gps_compare.py` `match_percentage` uses `bisect_left` for O(log N) timestamp lookup. ✅
- `cli/render_map.py` reads JSONL line-by-line; no full-file slurp. ✅

No Performance findings.

### Phase 6 — Cross-Task Consistency

Cross-task wiring across the 5 batches:

- **Module promotion**: `tests/e2e/replay/_report_writer.py` (batch 100) → `src/gps_denied_onboard/helpers/accuracy_report.py` (batch 102). All four consumers re-pointed:
  - `tests/e2e/replay/test_derkachi_real_tlog.py` (line 49)
  - `tests/unit/test_az699_report_writer.py` (line 23)
  - `src/gps_denied_onboard/helpers/__init__.py` (line 19)
  - `src/gps_denied_onboard/replay_api/app.py` (line 167, inline)
  Old path deleted from disk. No orphaned imports.
- **Symbol uniqueness**: `class HorizontalErrorDistribution`, `def load_tlog_ground_truth`, `def render_report`, `class TlogGroundTruth` — each defined exactly once. No duplicates.
- **AZ-698 → AZ-701 plumbing**: `--auto-trim` flag added to `cli/replay.py` (batch 99) is consumed by `replay_api/app.py` argv builder (batch 102). `AlignedWindow` DTO + `ReplayAutoSyncConfig` are threaded through composition root (`runtime_root/_replay_branch.py`) to `tlog_video_adapter` to `c8_fc_adapter.tlog_replay_adapter` — consistent signatures across all four batches.

No Cross-Task Consistency findings.

### Phase 7 — Architecture Compliance

**Layer-direction analysis** (against module-layout.md "Allowed Dependencies"):

- `replay_api/*` is best classified as Layer 5 (Entry / Composition). All its imports stay within `replay_api/*` plus stdlib + FastAPI / Pydantic / python-multipart (third-party). The deferred `helpers.accuracy_report` / `helpers.gps_compare` / `replay_input` imports inside `_maybe_render_report` reach Layer 1 / Layer 4 — both lower than Layer 5. ✅
- `cli/render_map.py` (Layer 5) imports `replay_input.load_tlog_ground_truth` (Layer 4). ✅
- `cli/replay_api_entrypoint.py` (Layer 5) imports `replay_api.app` / `replay_api.storage` (Layer 5 self-layer). ✅
- `replay_input/tlog_ground_truth.py` (Layer 4) imports only `replay_input.errors` (intra-package). ✅
- `replay_input/tlog_video_adapter.py` (Layer 4) imports `c8_fc_adapter.errors` + `c8_fc_adapter.tlog_replay_adapter` (Layer 4). Layer-4 → Layer-4 is explicitly **documented** in `module-layout.md` § shared/replay_input ("instantiates Layer-4 strategies `c8_fc_adapter.tlog_replay_adapter`, `frame_source.video_file_frame_source`"). ✅
- `runtime_root/_replay_branch.py` (Layer 5) imports `replay_input` (Layer 4) + `c8_fc_adapter` (Layer 4) + Layer-1 shareds. ✅
- `c8_fc_adapter/tlog_replay_adapter.py` (Layer 4) imports stay within `c8_fc_adapter/*` plus Layer-1 shareds. ✅

**Public-API respect**: every cross-component import goes through the target component's Public API surface — no internal-file imports detected.

**Cyclic-dependency check**: cycle-2's import edges form a strict DAG: `replay_api → helpers + replay_input + (subprocess) cli/replay + cli/render_map`. `replay_input` does not depend on `replay_api`. `helpers` does not depend on anything cycle-2-added. No new cycles.

**Duplicate-symbol check**: see Phase 6 — no duplicates.

**Cross-cutting concerns**: none re-implemented locally. Logging, FDR, helpers, clock, config — all consumed from their canonical Layer-1 homes.

**Single finding**: F1 — module-layout.md is the authoritative file-ownership map and is stale relative to the cycle-2 work. The code itself respects the layering; the documentation has not caught up. Severity Medium (Architecture) because the staleness blocks `/implement` Step 4 on the next task that touches these areas.

## Notes

- **No `## Baseline Delta` section**: `_docs/02_document/architecture_compliance_baseline.md` was identified in the 2026-05-20 LESSONS entry as a cycle-2 Step 6 (Decompose) prerequisite, seeded with 0 violations from the structural snapshot in `_docs/06_metrics/structure_2026-05-20.md`. The baseline file does not exist at the time of this review. Partition against baseline therefore not possible. This is captured as a separate follow-up (retrospective / next-cycle decompose), not a per-batch finding.
- **Cumulative-review cadence drift**: per `/implement` Step 14.5, a cumulative review was due after batch 100 (3rd batch into cycle 2). It was not run; this is the make-up review covering all 5 cycle-2 batches in one pass. Cadence drift should be noted in the cycle-2 retrospective.

## Artifacts

- Verdict consumed by: `/implement` Step 14.5 gate (PASS_WITH_WARNINGS → proceed to Step 15 Product Implementation Completeness Gate)
- Findings (F1, F2) carried forward to the cycle-2 retrospective for action assignment
