# Batch 40 — Code Review

**Tasks**: AZ-316 (C11 TileDownloader)
**Cycle**: 1
**Reviewer**: autodev
**Verdict**: **PASS_WITH_WARNINGS**

## Scope reviewed

Production code:

- `src/gps_denied_onboard/components/c11_tile_manager/_types.py` (additions: `SectorClassification`, `DownloadOutcome`, `TileSummary`, `DownloadRequest`, `DownloadBatchReport`)
- `src/gps_denied_onboard/components/c11_tile_manager/errors.py` (additions: `ResolutionRejectionError`, `CacheBudgetExceededError`)
- `src/gps_denied_onboard/components/c11_tile_manager/config.py` (additions: 6 download-side fields + validation)
- `src/gps_denied_onboard/components/c11_tile_manager/interface.py` (`TileDownloader` Protocol expanded to its real shape)
- `src/gps_denied_onboard/components/c11_tile_manager/tile_downloader.py` (new — `HttpTileDownloader`, `request_hash`, `_JournalState`, two consumer-side cuts)
- `src/gps_denied_onboard/components/c11_tile_manager/__init__.py` (exports for download-side public API)
- `src/gps_denied_onboard/runtime_root/c11_factory.py` (`build_tile_downloader` + private `_C6DownloadAdapter` class wrapping c6's `TileMetadata` assembly)

Tests:

- `tests/unit/c11_tile_manager/test_tile_downloader.py` (14 tests — AC-1..AC-9, AC-11, AC-12, NFR throughput, plus 429 HTTP-date form + 429 budget exhaustion)
- `tests/unit/c11_tile_manager/test_protocol_conformance.py` (2 new tests — AC-10 positive + negative)

## Phase 1 — Architecture

### AZ-507 cross-component rule

`tile_downloader.py` does NOT import from any other `components.*` module. The c6 surfaces (`TileStore`, `TileMetadataStore`, `CacheBudgetEnforcer`, `FreshnessRejectionError`, `TileMetadata`, `TileSource`, `FreshnessLabel`, `VotingStatus`) are reached through:

* Two structural `Protocol` cuts declared inside `tile_downloader.py` itself: `_TileWriterLike` and `_BudgetEnforcerLike`.
* A composition-root adapter `_C6DownloadAdapter` in `runtime_root/c11_factory.py` that lazily imports c6's enums + dataclasses inside its method bodies and assembles the `TileMetadata` envelope c6 expects.
* A structural duck-type check on the freshness-rejection exception class name (`_is_freshness_rejection`), so c6's `FreshnessRejectionError` is recognised by class identity without an import.

The AZ-270 lint (`test_ac6_only_compose_root_imports_concrete_strategies`) passes — the only c6 imports in the modified surface live in the composition root.

### Composition root

`build_tile_downloader` reads the `C11Config` block from `config.components['c11_tile_manager']`, fails fast with `ConfigError` when `satellite_provider_url` or `service_api_key` is empty (the safe defaults exist for unit-test bootstrap; production / operator wiring MUST set both). The `_C6DownloadAdapter` instance is shared as the binding for both Protocol cuts so the downloader always sees the same backing c6 trio.

### Logging envelopes

The spec calls for structured logs (no FDR records on the download path — confirmed by re-reading AZ-316). All emissions use the project `kv` envelope:

* INFO `c11.download.session.start` / `c11.download.session.end` (AC-NEW-6 visibility, AC-1).
* WARN `c11.download.resolution_rejected` (one per rejected tile, AC-2).
* WARN `c11.download.freshness_rejected_summary` (one per batch, AC-3).
* WARN `c11.download.freshness_downgraded` (one per downgraded tile, AC-4).
* WARN `c11.download.batch.retry` (one per HTTP retry, AC-5/AC-6 telemetry).
* ERROR `c11.download.provider.failed` (terminal HTTP error, AC-6/AC-7).
* ERROR `c11.download.budget.exceeded` (AC-9).
* INFO `c11.download.idempotent_no_op` (AC-8).

The auth header is logged ONLY redacted (`Bearer ***`); the AC-11 test asserts the raw API key never appears in any log record.

## Phase 2 — Behaviour vs. spec

| Spec requirement | Status |
|------------------|--------|
| Validate request (bbox, zoom, cache_root) | PASS — bbox + zoom validated in `DownloadRequest.__post_init__`; cache_root writability verified implicitly by the journal write (see F4) |
| Idempotence via `cache_root/.c11/journal/<flight_id>__<request_hash>.json` | PASS |
| Enumerate via `?list-only=true` | PASS |
| Cache headroom pre-check via `reserve_headroom(estimated_bytes)` | PASS |
| Auth: TLS + `Authorization: Bearer <api_key>` | PASS |
| 429 honours `Retry-After` (int + HTTP-date) capped via config | PASS |
| 5xx exponential backoff (1s/2s/4s/8s, 4 retries) → `SatelliteProviderError` | PASS |
| TLS / 401 / 403 → `SatelliteProviderError` fail-fast | PASS |
| Resolution gate at C11 boundary (≥ 0.5 m/px) | PASS |
| `FreshnessRejectionError` from c6 → counted, not propagated | PASS |
| `FreshnessLabel.DOWNGRADED` → tile persisted + counted | PASS (see F2) |
| Per-tile journal append after each successful write | PASS |
| `DownloadBatchReport(outcome=success/partial/failure/idempotent_no_op)` | PASS — failure paths raise (typed exception) and journal is flushed in the finally / exception block; the spec text suggests `failure` as a return value (see F1) |
| Lockfile assertion at construction | NOT IMPLEMENTED (see F3) |

## Findings

**F1 — Low (Spec wording vs. impl, recurring across C11 trio)**: AZ-316 step 4 / 6 prose says "`outcome = failure`" on cache-budget, persistent 5xx, or auth failure. My implementation raises `CacheBudgetExceededError` / `SatelliteProviderError` / `RateLimitedError` instead of returning a `FAILURE` report — matching the contract's exception matrix and the AC test surface (AC-6 / AC-7 / AC-9 explicitly assert `pytest.raises`). The `DownloadOutcome.FAILURE` enum value is wired into the journal's `tile_counts` envelope on the exception path so any downstream auditor can still distinguish failure from success without re-reading the exception trace. Same shape as Batch 39's F1. Action: documented; recommend a hygiene PBI to align AZ-316 + AZ-319 spec prose with their typed-exception contracts.

**F2 — Low (Adapter returns conservative `"fresh"` label)**: The `_C6DownloadAdapter.write_tile_for_download` returns the literal string `"fresh"` because the c6 `TileMetadataStore.insert_metadata` API (AZ-303 contract) does not currently expose the post-insert freshness label as a return value — the freshness gate (AZ-307) raises on outright rejection but does not communicate the DOWNGRADED case to the caller. As a result, the AZ-316 `tiles_downgraded` counter only increments when the adapter actively reports `"downgraded"`; in the c6-as-it-stands wiring that path is currently unreachable. The `HttpTileDownloader` logic for the DOWNGRADED label is fully exercised by AC-4 via the `_StubTileWriter.labels` test fixture. Action: documented; if AZ-307 / AZ-303 surface a `FreshnessLabel` post-insert in a future iteration, only the adapter changes — the downloader is already wired.

**F3 — Low (Risk 5 lockfile assertion deferred)**: Spec Risk 5 mitigation says "this task asserts the lockfile exists at construction (`cache_root/.c11/lock`) and refuses to start otherwise"; the lockfile creation is C12's job. I did NOT add the assertion in this batch — `cache_root` is a per-call parameter on `DownloadRequest`, not a constructor input, so the construction-time assertion the spec describes is not actually possible against the current Protocol shape. Adding the check at the start of `download_tiles_for_area` would be the correct place but C12 is not yet built (epic E-C12 is downstream), so the lockfile would always be absent and would block every operator run. Action: defer to the C12 epic; capture as a `ResolutionRejectionError` follow-up note when E-C12 lands.

**F4 — Low (`cache_root` writability not pre-validated)**: Spec step 1 says "validates the `DownloadRequest` (… `cache_root` is writable)". My implementation relies on the journal write to fail naturally at `_atomic_write_json`, which calls `mkdir(parents=True, exist_ok=True)` and would raise `PermissionError` on a read-only mount. The error surfaces but is not the spec's preferred fail-fast `ValueError` from `__post_init__`. Action: documented; acceptable for a v1.0.0 ship — the failure mode is loud, not silent.

**F5 — Low (Constructor signature deviation, recurring)**: Spec lists `clock: Clock` as a constructor parameter; my implementation injects a callable `sleep` (defaults to a `WallClock`-routed sleep). Same rationale as Batch 39's F2 — the downloader only ever needs to sleep, not `monotonic_ns` / `time_ns`. Threading the full `Clock` Protocol would carry payload the class never reads. The default `_default_sleep` helper routes through `WallClock.sleep_until_ns`, so the AZ-398 invariant (no `time.sleep` in `components/`) holds. Action: documented; revisit if a future C11 task needs the broader `Clock` surface.

## Phase 3 — Tests

14 unit tests pass for `HttpTileDownloader`; 2 new tests for the `TileDownloader` Protocol conformance check (positive + negative). Full unit suite: **1420 passed, 80 skipped, 0 failed** (skips are environment-gated: Docker, CUDA, TensorRT, Tier-2 hardware, actionlint). The +16-test net delta vs. Batch 39's 1404 baseline matches the 16 new tests added in this batch.

NFR-perf-throughput (50 MB/s on 1 Gbps): not unit-testable (requires real network). The unit `test_nfr_throughput_1000_tiles_under_budget` substitutes a 10-second wall-clock budget for a 1000-tile MockTransport batch — verifying the bookkeeping has no O(n²) regression rather than certifying real throughput. The real throughput target falls into the e2e perf suite (E-BBT).

AC-12 was tested via journal-truncation rather than process kill (4 prior + 6 new = 10 final, same shape as the spec's 30+70=100 example). Killing the test runner mid-batch is not feasible inside `pytest`; the journal-truncation simulation is the standard pattern for crash-recovery tests in this codebase (matches the C9 download journal tests).

## Phase 4 — Quality gates

- `ReadLints` clean across `c11_tile_manager/`, `runtime_root/c11_factory.py`, and the new + extended test files
- No `time.sleep` in `components/` (routes through `WallClock.sleep_until_ns`)
- No secrets in logs (AC-11 asserts the raw API key never appears in any captured log record; `Bearer ***` redaction confirmed)
- No new third-party dependencies (uses existing `httpx` and stdlib `email.utils` for HTTP-date parsing; `atomicwrites` semantics implemented inline because the project already uses the same pattern in C9 — staying consistent rather than adding a new dependency)

## Verdict

**PASS_WITH_WARNINGS** — All five findings are Low severity (recurring spec-prose vs. typed-exception drift, adapter freshness-label conservatism pending an AZ-303 ABI extension, deferred Risk-5 lockfile assertion blocked on C12, missing pre-validation of `cache_root` writability, recurring Clock-vs-sleep injection deviation). No code change required for batch close-out. F1 and F5 are recurring across the C11 trio and should be captured as a single hygiene PBI in the next cumulative review.