# Code Review Report

**Batch**: AZ-777 Phase 1 (e2e-runner wire + C11 contract adapt + smoke test)
**Date**: 2026-05-21
**Verdict**: PASS_WITH_WARNINGS

## Scope

AZ-777 is an 8-pt task with 5 explicit STOP-gated phases. This batch
delivers **Phase 1 only** — the e2e-runner wiring to the existing
parent-suite satellite-provider service + the C11 `HttpTileDownloader`
contract adaptation to the AZ-505 v1.0.0 `tile-inventory.md` API +
the Tier-2 smoke test that validates the wire.

Phases 2–5 (catalog seed via `POST /api/satellite/request`, real
`operator_pre_flight_setup` fixture, un-xfail Tier-2 tests, docs)
are out of scope for this batch and are gated behind a STOP per the
task spec's Risk-5 mitigation.

## Files changed

Production (1):

- `src/gps_denied_onboard/components/c11_tile_manager/tile_downloader.py`
  — `_LIST_PATH` / `_GET_PATH` replaced with `_INVENTORY_PATH`
  (`POST /api/satellite/tiles/inventory`) + `_TILES_PATH`
  (`GET /tiles/{z}/{x}/{y}`); `_do_enumerate` rewritten to enumerate
  bbox tile coords client-side and POST chunked inventory requests;
  `_download_one_tile` re-routes to slippy-map URL; common retry /
  auth logic refactored into `_send_request`; new module helpers:
  `_enumerate_bbox_tile_coords`, `_tile_center_latlon`,
  `_tile_size_meters_at`, `_format_tile_id_str`, `_parse_tile_id_str`,
  `_chunk_iter`; new constants `_DEFAULT_ESTIMATED_TILE_BYTES`
  (50 KiB, conservative tile-size estimate since inventory no longer
  returns content-length hints), `_INVENTORY_MAX_ENTRIES_PER_REQUEST`,
  `_EARTH_EQUATORIAL_CIRCUMFERENCE_M`, `_TILE_SIZE_PIXELS`.

Tests (2):

- `tests/unit/c11_tile_manager/test_tile_downloader.py` — all 14
  AC tests rewritten to drive `_make_inventory_handler` (POST
  inventory + GET tile) instead of the old GET-list handler;
  `_StubTileWriter` rekeyed by call-index instead of by
  `(z,lat,lon)` strings (the downloader now derives lat/lon from
  the slippy-map coord, so fixtures cannot fabricate arbitrary
  lat/lons); `_DEFAULT_ESTIMATED_TILE_BYTES` constant mirrored.
  All 14 tests PASS.
- `tests/e2e/satellite_provider/test_smoke.py` (new) — two Tier-2
  smoke tests: (i) raw `POST /api/satellite/tiles/inventory` for a
  1-tile Derkachi-bbox query, asserts the documented response
  schema; (ii) drives the adapted `HttpTileDownloader` against the
  real service with an in-memory C6 stub (Phase-3 fixture will
  replace it with real C6).
- `tests/e2e/satellite_provider/__init__.py` (new).

Compose / env (2):

- `docker-compose.test.jetson.yml` — e2e-runner env block:
  `SATELLITE_PROVIDER_URL` switched from `http://mock-sat:5100` to
  `https://satellite-provider:8080`; `SATELLITE_PROVIDER_TLS_INSECURE=1`
  added (dev-only); `SATELLITE_PROVIDER_API_KEY` sourced from
  `.env.test`; `JWT_*` forwarded for in-container fallback minting;
  `depends_on: { satellite-provider: { condition: service_healthy } }`
  added.
- `.env.test.example` — new `SATELLITE_PROVIDER_API_KEY` variable
  with documentation + dev TLS bypass security note.

Tooling (2):

- `scripts/mint_dev_jwt.py` (new) — HS256 dev-JWT mint helper.
  Reads JWT secret / iss / aud from env or `.env.test`; emits a
  signed JWT to stdout. Convenience for dev workflows; production
  retrieves tokens from the admin API.
- `pyproject.toml` — added `pyjwt>=2.8,<3.0` to `[dev]` extras.

Tracker docs (1):

- `_docs/02_tasks/_dependencies_table.md` — AZ-777 row bumped from
  5pt to 8pt (matches the 2026-05-21 decision-log override in
  `_docs/_process_leftovers/2026-05-21_az777_complexity_override.md`).

## Phase 2 — Spec Compliance

| AC | Status | Notes |
|----|--------|-------|
| AC-1 (`docker compose config` exits 0 with `depends_on satellite-provider`) | ✅ Verified | Compose lint passes locally with the new env block. |
| AC-2 unit half (`_do_enumerate` POST inventory + `_download_one_tile` slippy-map GET against stubbed responses) | ✅ Verified | 14/14 unit tests PASS against the new contract. |
| AC-2 live half (Bearer-authenticated round-trip against the running service) | ⏸ Deferred to Tier-2 Jetson run | Smoke test gated by `RUN_REPLAY_E2E=1` + `tier2`; auto-skips on dev macOS. |
| AC-3..6 | ⏳ Out of scope (Phases 2–5) | Phase 1 → 2 STOP gate. |

No spec gaps within Phase 1. AC-2's live validation runs the next
time the Jetson harness fires; the test code is in place.

## Findings

| # | Severity | Category | File:Line | Title |
|---|----------|----------|-----------|-------|
| 1 | Medium | Architecture | `src/gps_denied_onboard/runtime_root/c11_factory.py` | TLS_INSECURE flag not plumbed through production composition root |
| 2 | Medium | Maintainability | `src/gps_denied_onboard/components/c11_tile_manager/tile_downloader.py:84` | `_DEFAULT_ESTIMATED_TILE_BYTES` is a project-wide guess, not configurable |
| 3 | Low | Maintainability | `tests/e2e/satellite_provider/test_smoke.py` | Smoke test passes when catalog is empty (Phase-2 dependency) |

### Finding Details

**F1: TLS_INSECURE flag not plumbed through production composition root**
(Medium / Architecture)

- Location: `src/gps_denied_onboard/runtime_root/c11_factory.py`
- Description: `build_tile_downloader` takes a caller-owned
  `httpx.Client`, so the operator binary that wires C11 is the
  layer that must honour `SATELLITE_PROVIDER_TLS_INSECURE`. No
  production caller exists today — `build_tile_downloader` only
  has the Tier-2 smoke test as a live consumer. Phase 3 of AZ-777
  introduces the `operator_pre_flight_setup` fixture that will be
  the first live caller; the TLS_INSECURE handling will land there.
- Suggestion: When Phase 3 ships, the new caller must read
  `SATELLITE_PROVIDER_TLS_INSECURE` and pass the right `verify=`
  to `httpx.Client(...)` — mirror the approach used in
  `tests/e2e/satellite_provider/test_smoke.py::_make_http_client`.
  Also consider a `WARNING` log line at startup whenever the
  insecure flag is active so the operator can audit it.
- Task: AZ-777 Phase 3 (deferred)

**F2: `_DEFAULT_ESTIMATED_TILE_BYTES` is a project-wide guess**
(Medium / Maintainability)

- Location: `src/gps_denied_onboard/components/c11_tile_manager/tile_downloader.py:84`
- Description: The AZ-505 v1.0.0 inventory contract dropped the
  per-entry `estimatedBytes` field, so the AZ-308 budget pre-check
  reserves a constant 50 KiB per `present=true` tile. 50 KiB is
  conservative for typical CARTO Voyager tiles (8-30 KiB) but
  under-reserves for high-detail UAV uploads (30-80 KiB). The
  budget can over-reserve safely; under-reserving fails the
  AZ-308 contract.
- Suggestion: Either (a) add the constant to `C11Config` so
  operators can tune it per imagery source, or (b) file a
  parent-suite ticket to restore `estimatedBytes` in the inventory
  response. For Phase 1 the constant is acceptable; revisit in
  Phase 5 docs.
- Task: AZ-777 Phase 5 / future config refactor

**F3: Smoke test passes when catalog is empty** (Low / Maintainability)

- Location: `tests/e2e/satellite_provider/test_smoke.py:test_smoke_c11_download_via_http_pipeline`
- Description: The C11 pipeline smoke asserts SUCCESS with
  `tiles_downloaded == len(write_calls)`. Pre-Phase-2 the catalog
  is empty → every entry comes back `present=false` → the test
  passes with zero downloads, which proves the wire works but
  does NOT prove tiles actually land in C6. The conditional
  `if report.tiles_downloaded > 0` block tightens the assertion
  once the catalog is seeded.
- Suggestion: Accepted by design for Phase 1; Phase 2's catalog
  seed automatically turns this from "wire works" into "tiles
  land" without test changes.
- Task: AZ-777 Phase 2

## Phase 3 — Code Quality

- **SOLID**: `_send_request` consolidates GET / POST retry + auth
  in one place instead of two near-duplicates; methods stay small
  (`_send_get` / `_send_post` are 5-line shims over the common
  path). Slippy-map helpers are module-level pure functions —
  they don't reach for `self` and don't depend on `httpx`, so
  the unit tests can reuse them directly.
- **Error handling**: every failure path raises a typed C11 error
  (`SatelliteProviderError`, `RateLimitedError`,
  `CacheBudgetExceededError`); no bare `except`s; no silently
  swallowed errors. The Retry-After parser handles both seconds-
  and HTTP-date forms; OOB values clamp to 0 instead of
  propagating garbage.
- **Naming**: `_inventory_path` / `_tiles_path` / `_tile_id_str` /
  `_parse_tile_id_str` etc. all read directly against the AZ-505
  contract; no surprises.
- **Complexity**: `_send_request` is the longest method at ~80 LOC
  but it's a linear retry ladder; cyclomatic complexity is
  bounded by the four response branches (transport-error / 401-3
  / 429 / 5xx / 200). `_do_enumerate` is 14 LOC.
- **Test quality**: every AC test arranges a specific contract
  scenario (POST inventory + GET tile) and asserts both the
  downloader's report counts AND the C6 stub's call records.
  Tests do NOT just "assert no exception".

## Phase 4 — Security Quick-Scan

- No SQL strings touched.
- JWT mint helper uses PyJWT's `jwt.encode` with HS256; no
  hand-rolled crypto; secrets come from env, never hardcoded.
- `.env.test.example`'s `SATELLITE_PROVIDER_API_KEY` placeholder
  is `PASTE-MINTED-JWT-HERE` — the smoke test treats that exact
  string as "unset" and skips, so a developer accidentally
  committing the placeholder cannot get false confidence.
- `Authorization` header redacted in error logs as
  `Bearer ***` per AZ-316 AC-11; the AC-11 test verifies the
  real API key never appears in any log record.
- `SATELLITE_PROVIDER_TLS_INSECURE` is opt-in via env var;
  default is verify=True. The dev-only nature is documented in
  the compose comment, in `.env.test.example`, and (when Phase 3
  lands) will be logged at startup.

## Phase 5 — Performance Scan

- Inventory POST chunks at 5000 entries per the contract cap; one
  POST per up-to-5000-tile bbox.
- Backoff schedule unchanged (`_DEFAULT_BACKOFF_SCHEDULE_S =
  (1, 2, 4, 8)`); session retry budget enforced.
- `test_nfr_throughput_1000_tiles_under_budget` passes in <1 s
  locally (budget is 10 s) — no O(n²) bookkeeping regression.
- No N+1 patterns; no blocking I/O in async paths (whole module
  is sync).

## Phase 6 — Cross-Task Consistency

Single task in this batch (AZ-777 Phase 1). N/A.

## Phase 7 — Architecture Compliance

- **Layer direction**: C11 still does not import C6 directly; the
  Protocol cuts (`_TileWriterLike`, `_BudgetEnforcerLike`) stay
  in `tile_downloader.py`. The composition root
  (`runtime_root/c11_factory.py::_C6DownloadAdapter`) remains the
  single bridge.
- **Public API respect**: no cross-component imports added.
- **No new cyclic deps**: no new module-level imports between
  components.
- **Architecture principle #5** (`satellite-provider` owns OSM /
  CARTO tile network I/O; the onboard companion is read-only via
  C11 during pre-flight): this batch is the first time C11 is
  actually wired to consume that contract — the principle is
  honoured for the first time end-to-end.
- **ADR compliance**: ADR-004 (event-driven cross-component
  comms): C11 → satellite-provider is HTTP, which is explicitly
  scoped out of ADR-004 (the ADR governs intra-onboard comms,
  not external-service calls). No drift. No new ADR required —
  the task spec explicitly states this is execution of existing
  decisions.

## Verdict justification

PASS_WITH_WARNINGS — three Medium / Low findings, no Critical or
High. The Medium findings are deferred to later AZ-777 phases or
to future tuning, with clear ownership; no blocking gap in Phase 1
itself.