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

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 excepts; 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.