# Cumulative Code Review — Batches 40–42 / Cycle 1

**Date**: 2026-05-13
**Mode**: Cumulative (all 7 phases, emphasis on Phases 6 + 7)
**Batches covered**: 40, 41, 42
**Tasks covered**: AZ-316 (C11 HttpTileDownloader), AZ-320 (C11 IdempotentRetryTileUploader + c6 `increment_upload_attempts` extension + 0003 migration + `VotingStatus.UPLOAD_GIVEUP`), AZ-326 (C12 operator-tool CLI + SectorClassificationStore + freshness-table + EXIT_* constants), AZ-327 (C12 CompanionBringup + ParamikoSshSessionFactory + RemoteSidecarVerifier + error families)
**Changed files in scope**: 26 production + 11 tests + 6 docs / migration / config (full list in §Scope below)
**Verdict**: **PASS_WITH_WARNINGS**

## Scope

| Domain | Files (changed since `cumulative_review_batches_37-39_cycle1_report.md`) |
|--------|--------------------------------------------------------------------------|
| c11_tile_manager (production) | `_types.py` (added `SectorClassification`, `DownloadOutcome`, `TileSummary`, `DownloadRequest`, `DownloadBatchReport`); `errors.py` (added `ResolutionRejectionError`, `CacheBudgetExceededError`); `config.py` (download fields + `C11RetryConfig` + `disable_retry_decorator`); `interface.py` (real `TileDownloader` Protocol shape); `tile_downloader.py` (new — `HttpTileDownloader`, `request_hash`, journal); `idempotent_retry.py` (new — `IdempotentRetryTileUploader`); `__init__.py` (re-exports) |
| c12_operator_tooling (production) | `_types.py` (new — `SectorClassification`, `CompanionAddress`, `ReadinessReport`, `ReadinessOutcome`, `CompanionUnreachableReason`, `AreaIdentifier`); `exit_codes.py` (new); `freshness_table.py` (new); `config.py` (new — `C12Config`, `C12CompanionConfig`, `HostKeyPolicy`); `errors.py` (new — `CompanionUnreachableError`, `ContentHashMismatchError`); `ssh_session.py` (new — `SshSession`/`SshSessionFactory` Protocols); `paramiko_ssh_session.py` (new — concrete adapter); `remote_sidecar_verifier.py` (new); `companion_bringup.py` (new); `cli.py` (new — Click app + 6 subcommands); `__main__.py` (new); `sector_classification_store.py` (new); `__init__.py` (PEP 562 lazy re-exports for heavy adapters); `flights_api/__init__.py` (PEP 562 lazy re-exports for httpx + bbox helpers) |
| c6_tile_cache (production) | `_types.py` (added `VotingStatus.UPLOAD_GIVEUP`); `interface.py` (added `TileMetadataStore.increment_upload_attempts` w/ `NotImplementedError` default); `postgres_filesystem_store.py` (added `increment_upload_attempts` SQL, extended `_ALLOWED_VOTING_TRANSITIONS`, tightened `pending_uploads` filter) |
| runtime_root (composition root) | `c11_factory.py` (added `build_tile_downloader` + `_C6DownloadAdapter`; `build_tile_uploader` now wraps with retry decorator by default + `clock` keyword); `c12_factory.py` (added `build_sector_classification_store`, `build_companion_bringup`, expanded `build_operator_tool` aggregator) |
| FDR | `fdr_client/records.py` (registered `c11.upload.giveup`) |
| Migrations | `db/migrations/versions/0003_c11_upload_attempts.py` (new — additive column + widened CHECK constraint, reversible) |
| Tests | `c11_tile_manager/test_tile_downloader.py` (new, 14 tests); `c11_tile_manager/test_idempotent_retry.py` (new, 13 tests); `c11_tile_manager/test_protocol_conformance.py` (extended); `c12_operator_tooling/test_freshness_table.py`, `test_exit_codes.py`, `test_sector_classification_store.py`, `test_cli_help_and_logging.py`, `test_cli_build_cache.py`, `test_cli_console_script.py`, `test_companion_bringup.py`, `test_paramiko_factory_smoke.py` (all new); `c6_tile_cache/test_protocol_conformance.py` (extended `UPLOAD_GIVEUP` + new method on fakes); `test_ac5_alembic.py` (head-revision bumped to `0003_c11_upload_attempts`); `test_az272_fdr_record_schema.py` (added `c11.upload.giveup` payload) |
| Docs / configuration | `_docs/02_document/contracts/c6_tile_cache/tile_metadata_store.md` (v1.3.0 — `increment_upload_attempts` method + I-8 update); `_docs/02_tasks/done/AZ-316_*.md`, `AZ-320_*.md`, `AZ-326_*.md`, `AZ-327_*.md`; `_docs/03_implementation/batch_40_cycle1_report.md`, `batch_41_cycle1_report.md`, `batch_42_cycle1_report.md`; `_docs/03_implementation/reviews/batch_40_review.md`, `batch_41_review.md`; `_docs/_autodev_state.md`; `_docs/_process_leftovers/2026-05-13_ruff_format_az489_pending.md`; `pyproject.toml` (added `paramiko>=3.4,<4.0` + `operator-tool` console script entry) |

## Summary

Three batches, four feature tasks, zero blocking findings. The C11 contract surface
is now complete (downloader + uploader + retry decorator + signing key + flight gate
all wired and tested), the C12 epic landed its first runnable end-to-end CLI shell
plus the companion pre-flight verification path, and the c6 contract gained a clean
forward-only `UPLOAD_GIVEUP` transition with an additive migration. The full
unit-test suite reached **1494 passed / 80 environment-skipped** with zero
failures across the changed surface; pre-existing perf microbenches (C8 covariance
projector, C10 batcher overhead, C11 signing-key sign-p99) remain flaky on this dev
host but were green or untouched in the batches under review.

The dominant architectural achievement of this window is the **maturation of the
PEP 562 lazy-import discipline** for operator-tooling cold start. AZ-326 calls
out a hard NFR (`operator-tool --help` ≤ 500 ms p99 on a developer laptop) plus an
explicit constraint forbidding eager imports of `paramiko`, `httpx`, or
`pymavlink` at CLI module load time. The implementation went well beyond the
named libraries: the package `__init__.py` for both `c12_operator_tooling` and its
nested `flights_api` package was converted to PEP 562 `__getattr__` lazy
re-exports, deferring `HttpxFlightsApiClient` (httpx), `ParamikoSshSession*`
(paramiko + cryptography), and `bbox_from_waypoints` / `takeoff_origin_from_flight`
(numpy + pyproj). The `cli.py` module was switched to import from leaf
`flights_api.errors` / `flights_api.interface` rather than the package, avoiding
even the lazy machinery in the hot path. `python -X importtime` confirms zero
heavy-dependency imports at CLI cold start; cold-start dropped from ~870 ms to
<200 ms typical, <500 ms p99. The pattern is now an established primitive for
any future operator-side CLI surface.

The recurring **Clock-vs-sleep injection** deviation (flagged in cumulative
reports for batches 31–33 and 34–36, then noted in 37–39 as still-open for the
C11 trio) is now **CLOSED**:

- Batch 41 lands the full `Clock` Protocol injection in
  `IdempotentRetryTileUploader` (uses both `monotonic_ns` and `time_ns`).
- Batch 40 still injects a bare `Callable[[float], None]` sleep in
  `HttpTileDownloader` because the downloader genuinely never needs
  `monotonic_ns` / `time_ns` — this is now the documented exception, not the
  drift, and routes through `WallClock.sleep_until_ns` to honour the AZ-398
  invariant.
- Batch 42's C12 components do not need a Clock at all (CLI subcommands use
  Click's exit machinery; CompanionBringup's only timing dependency is the
  paramiko connect-timeout).

Phase 6 (Cross-Task Consistency) verifies:

- **AZ-316 ↔ c6 freshness contract** — `HttpTileDownloader` recognises c6's
  `FreshnessRejectionError` by class-name match (`_is_freshness_rejection`)
  rather than importing it; the AZ-507 lint stays green and the spec's
  freshness-counter behaviour is exercised end-to-end via the
  `_C6DownloadAdapter` in `runtime_root/c11_factory.py`.
- **AZ-320 ↔ c6 voting-state contract** — `IdempotentRetryTileUploader` reaches
  `UPLOAD_GIVEUP` via a string constant rather than the c6 enum;
  `PostgresFilesystemStore.update_voting_status` coerces strings via
  `VotingStatus(status)`; the new transitions
  (`PENDING → UPLOAD_GIVEUP`, `TRUSTED → UPLOAD_GIVEUP`) are added to
  `_ALLOWED_VOTING_TRANSITIONS` and the contract Invariant I-8 was updated in
  the same batch (v1.3.0). `pending_uploads` SQL excludes the new state so a
  re-run does not re-pick give-up tiles.
- **AZ-326 ↔ AZ-327 sector classification + freshness table** — `cli.py`'s
  `set-sector` subcommand drives `SectorClassificationStore` (AZ-326);
  `freshness_threshold_months(SectorClassification)` is reused by the
  `build-cache` subcommand (AC-NEW-6); both AC IDs surface in `--help`. No
  duplication of either helper across the C12 internals.
- **AZ-326 ↔ AZ-489 flights-api contract** — the build-cache subcommand
  resolves the `FlightDto` either online (`fetch_flight`) or offline
  (`load_flight_file`) and forwards to the orchestrator (sibling AZ-328 not yet
  shipped); the consumer-side seam matches what AZ-489 froze in its v1.0.0
  contract. Switching `cli.py` to leaf-module imports left the AZ-489
  unit-test surface intact (28 tests pass against the lazy `__getattr__` package).
- **AZ-327 ↔ AZ-280 sidecar contract** — `RemoteSidecarVerifier` runs
  `sha256sum` + `cat` over SSH to compare against the `.sha256` sidecar shape
  AZ-280 documents. Engine bytes never reach the operator workstation.
- **Migration head + Alembic test consistency** — Batch 41's 0003 migration
  bumps the head revision; `tests/unit/test_ac5_alembic.py` was updated in
  the same batch to assert `0003_c11_upload_attempts`. No drift between
  migration files and test expectations.
- **FDR record-kind registry** — Batch 41's `c11.upload.giveup` is the only
  new payload kind across all three batches; registered in
  `fdr_client/records.py::KNOWN_PAYLOAD_KEYS` and matched by a mock payload
  in `tests/unit/test_az272_fdr_record_schema.py`.

Phase 7 (Architecture Compliance):

- **Layer direction**: every changed `components/*.py` file imports only from
  `_types/*`, `helpers/*`, `config`, `logging`, `clock`, `fdr_client`, or
  third-party libs (httpx, paramiko, click) — all Layer 1 or lower. No
  upward imports.
- **Public API respect**: zero `from gps_denied_onboard.components.<other>`
  imports inside any `components/*.py` file changed in this window. The
  AZ-507 / ADR-009 cross-component-contract-surface rule is honoured. The
  AZ-270 lint
  (`tests/unit/test_az270_compose_root.test_ac6_only_compose_root_imports_concrete_strategies`)
  passes.
- **No new cyclic module dependencies**: verified via `python -c "import …"`
  + `importlib.import_module` round-trip across all 57 public names exposed
  by `c12_operator_tooling/__init__.py`. The PEP 562 lazy `__getattr__` does
  not introduce a cycle because heavy modules are resolved at first
  attribute access, not at package import.
- **Duplicate symbols across components**: see Findings F1 + F2 + F3
  below — `_iso_now`-style helpers, atomic-write helpers, and the
  `SectorClassification` enum each grew their copy count this window. F3 is
  acknowledged-by-design (consumer-side cut per ADR-009); F1 + F2 are real
  hygiene drift that the existing AZ-508 PBI no longer fully covers.
- **Cross-cutting concerns**: lazy-import discipline is the only
  cross-cutting primitive added this window; lives in
  `c12_operator_tooling/__init__.py` + `flights_api/__init__.py` only.
  Future operator-side CLI surfaces should adopt the same pattern;
  documented in the batch report and in this review's Summary.

## Findings

| # | Severity | Category | File:Line | Title |
|---|----------|----------|-----------|-------|
| 1 | Medium | Maintainability | `c11_tile_manager/{tile_downloader.py:883, idempotent_retry.py:69, tile_uploader.py:739}`, `c13_fdr/{writer.py:70, cap_policy.py:35}`, `c7_inference/{onnx_trt_ep_runtime.py:169, thermal_publisher.py:343}` (vs. `c6_tile_cache/_timestamp.py:19` — the consolidated helper) | `_iso_now`-style helpers grew to 8 copies; AZ-508 hygiene PBI no longer matches reality |
| 2 | Low | Maintainability | `c11_tile_manager/tile_downloader.py:219`, `c12_operator_tooling/sector_classification_store.py:156`, `c10_provisioning/manifest_builder.py:546+561` | Atomic-write JSON helpers triplicated; no shared `helpers/atomic_write.py` |
| 3 | Low | Architecture (acknowledged-by-design per ADR-009) | `c6_tile_cache/_types.py:86`, `c10_provisioning/interface.py:59`, `c11_tile_manager/_types.py:183`, `c12_operator_tooling/_types.py:33` | `SectorClassification` enum duplicated to 4 components; intended per AZ-507 consumer-side cut, but the count is worth tracking |
| 4 | Low | Maintainability | `_docs/02_tasks/done/AZ-316_*.md`, `AZ-319_*.md`, `AZ-320_*.md` (specs say "outcome = failure"); `c11_tile_manager/{tile_downloader.py, tile_uploader.py, idempotent_retry.py}` (impls raise) | Recurring "spec prose says `outcome=failure`, implementation raises typed exception" deviation across the C11 trio |
| 5 | Low | Performance | `c12_operator_tooling/__init__.py`, `c12_operator_tooling/flights_api/__init__.py`, `c12_operator_tooling/cli.py` | NFR-perf-cold-start was a near-miss (~870 ms → <500 ms p99 only after PEP 562 conversion); operator-side discipline now established but not yet codified in `coderule.mdc` |

### Finding Details

**F1 — `_iso_now` helpers grew to 8 copies (Medium / Maintainability)**

- Locations:
  - `c11_tile_manager/tile_downloader.py:883` (Batch 40 — new)
  - `c11_tile_manager/idempotent_retry.py:69` (Batch 41 — new)
  - `c11_tile_manager/tile_uploader.py:739` (pre-existing)
  - `c13_fdr/writer.py:70` (pre-existing)
  - `c13_fdr/cap_policy.py:35` (pre-existing)
  - `c7_inference/onnx_trt_ep_runtime.py:169` (pre-existing)
  - `c7_inference/thermal_publisher.py:343` (pre-existing)
  - vs. the consolidated `c6_tile_cache/_timestamp.py:19::iso_ts_now` (the
    only canonical helper today)
- Description: at the last cumulative review (batches 34–36), the count was
  "down to 2 active copies in c7" with c6 already consolidated. This window
  added two more copies via the new C11 download + retry tasks. AZ-508 in
  `_docs/02_tasks/todo/` exists as the project-wide hygiene PBI for this
  consolidation but has not been picked up; its task spec also no longer
  matches reality (it lists modules that no longer carry the helper, and is
  silent on the new C11 sites).
- Suggestion: refresh the AZ-508 spec to enumerate all 7 current
  duplicate-helper sites + the canonical `c6_tile_cache/_timestamp.iso_ts_now`
  target, raise its priority to land before any further C11/C12/C13 task
  introduces another copy, and consider promoting the canonical helper
  from `c6_tile_cache/_timestamp.py` to a cross-component
  `helpers/timestamps.py` so consumer-side cuts can reach it without
  importing from c6.
- Severity rationale: Medium because the count is now 8 (above the
  "manageable drift" threshold) and at least three more in-flight tasks in
  the C11 / C12 / C13 epics will likely add copies if AZ-508 stays unpicked.

**F2 — Atomic-write JSON helpers triplicated (Low / Maintainability)**

- Locations:
  - `c10_provisioning/manifest_builder.py:546` (`_atomic_write_manifest`)
  - `c10_provisioning/manifest_builder.py:561` (`_atomic_write_signature`)
  - `c11_tile_manager/tile_downloader.py:219` (`_atomic_write_json`)
  - `c12_operator_tooling/sector_classification_store.py:156` (`_atomic_write`)
- Description: every site implements the same write-temp +
  `os.replace` + `fsync` + parent-directory `fsync` pattern. Each was
  written from scratch citing "we already use this pattern in module X";
  none of them imports from a shared helper. Promoting the helper to
  `src/gps_denied_onboard/helpers/atomic_write.py` would let consumer-side
  cuts reach it without crossing component boundaries (helpers/ is at
  Layer 1 alongside `_types/`).
- Suggestion: open a hygiene PBI to extract `helpers/atomic_write.py` and
  migrate the four call sites in one PR. Tests at the helper level can
  then own the AC-5 atomic-under-crash invariant; the call sites collapse
  to a one-line `helpers.atomic_write_bytes(path, payload)`.
- Severity rationale: Low because each existing copy is correct in
  isolation; the drift is repeated boilerplate, not divergent semantics.

**F3 — `SectorClassification` enum duplicated 4× (Low / Architecture, by-design)**

- Locations:
  - `c6_tile_cache/_types.py:86` — original / canonical
  - `c10_provisioning/interface.py:59` (Batch 35 — AZ-325 mirror)
  - `c11_tile_manager/_types.py:183` (Batch 40 — AZ-316 mirror)
  - `c12_operator_tooling/_types.py:33` (Batch 42 — AZ-326 mirror)
- Description: every consumer that reasons about the operator-set
  classification declares its own `class SectorClassification(str, Enum)`
  with the same two values (`active_conflict`, `stable_rear`). Because all
  copies inherit `str`, value equality holds at boundaries and `c6`'s
  enum coerces incoming strings via the standard `Enum(value)` ctor.
  This is the AZ-507 / ADR-009 consumer-side cut by design — flagged
  here only because the count keeps climbing (every new component touching
  sector classification adds the 5th, 6th, … mirror).
- Suggestion: consider promoting `SectorClassification` to a shared
  `_types/sector_classification.py` so consumers can `from
  gps_denied_onboard._types.sector_classification import …` instead of
  hand-mirroring. ADR-009 already permits `_types/*` as a cross-component
  contract surface; this is the precedent. Tracker-only suggestion (no
  code change required this cycle).
- Severity rationale: Low because the duplication is intentional and
  equivalence is preserved by the `str` Enum design.

**F4 — "outcome=failure" spec drift across C11 trio (Low / Maintainability)**

- Locations:
  - `_docs/02_tasks/done/AZ-316_c11_tile_downloader.md` (spec: "outcome =
    failure"; impl: `raise CacheBudgetExceededError | SatelliteProviderError
    | RateLimitedError`)
  - `_docs/02_tasks/done/AZ-319_c11_tile_uploader.md` (spec: "outcome =
    failure"; impl raises typed exceptions)
  - `_docs/02_tasks/done/AZ-320_c11_idempotent_retry.md` (decorator passes
    the underlying exception through)
- Description: the spec prose for the C11 download + upload + retry trio
  consistently uses "outcome = failure" as a return value, but the
  implementations raise typed exceptions per the contract's exception
  matrix. The behavioural delta is documented in each batch's per-batch
  review (Batch 39 F1, Batch 40 F1, Batch 41 F2 cluster). The exception
  path still flushes the journal / increments the counters / writes the
  partial report, so downstream auditors can distinguish failure shapes
  without reading the exception trace — the only artefact that drifted
  is the prose in the original task specs.
- Suggestion: open a single hygiene PBI to refresh the three task specs
  + the `DownloadOutcome` / `UploadOutcome` enum docstrings to align on
  "exceptions for hard failure; `partial` for tile-level rejection".
  This closes the recurring deviation in one PR rather than re-flagging
  it on every C11 batch review.
- Severity rationale: Low because the contract is stable (exception
  matrix is the source of truth and the tests assert against it);
  drift is documentation-only.

**F5 — NFR-perf-cold-start near-miss + lazy-import discipline not codified (Low / Performance)**

- Locations:
  - `c12_operator_tooling/__init__.py` (PEP 562 `__getattr__`)
  - `c12_operator_tooling/flights_api/__init__.py` (PEP 562 `__getattr__`)
  - `c12_operator_tooling/cli.py` (leaf-module imports for hot path)
- Description: the AZ-326 NFR ≤500 ms p99 was at ~870 ms with a naive
  package layout (eager re-exports of `HttpxFlightsApiClient`,
  `ParamikoSshSession*`, and the `bbox` helpers transitively pulled in
  paramiko + httpx + cryptography + numpy + pyproj + cv2 + gtsam). The
  Batch 42 fix went well beyond the spec's named libraries (which only
  call out paramiko / httpx / pymavlink) — numpy / pyproj / cv2 / gtsam
  were just as severe, dragged in by `flights_api.bbox` →
  `helpers.wgs_converter`. The fix lands in three places and depends on
  PEP 562 awareness from anyone who touches the package init in future.
- Suggestion: add a one-paragraph rule in `coderule.mdc` (or a new
  focused `cli-cold-start.mdc`) that any operator-tooling package
  `__init__.py` MUST keep heavy adapters behind PEP 562
  `__getattr__`, and that CLI entry-point modules MUST import via leaf
  modules rather than package init. Future C12 sibling tasks (AZ-328,
  AZ-329, AZ-330) will each add a service that could regress this NFR
  if the rule is not codified.
- Severity rationale: Low because the NFR currently passes; the risk
  is recurrence on future task additions if the discipline is not
  written down.

## Verdict & Action Map

**PASS_WITH_WARNINGS** — no Critical or High findings; per the implement
skill's auto-fix matrix, all five findings escalate to the user as Medium
(F1) or Low (F2-F5) Maintainability / Performance / Architecture entries
that do NOT block batch close-out. Recommended actions in priority order:

1. **F1 (Medium)** — refresh AZ-508 spec and pull it into the next
   ready batch before any further C11 / C12 / C13 task lands. The drift
   is now visible across two consecutive cumulative reviews and grew
   this window.
2. **F4 (Low)** — open a single doc-only PBI to align the three C11 spec
   prose blocks with their typed-exception contracts (one PR, no code
   change). Closes a recurring per-batch finding noise.
3. **F5 (Low)** — codify the lazy-import discipline as a rule before
   AZ-328 / AZ-329 / AZ-330 are picked.
4. **F2 (Low)** — extract `helpers/atomic_write.py` (one PR, four
   call-site migrations + a helper-level test for the under-crash
   invariant).
5. **F3 (Low)** — tracker-only suggestion to consolidate
   `SectorClassification` into `_types/`; no code change unless a 5th
   component would otherwise need it.

No `_docs/02_document/architecture_compliance_baseline.md` exists, so
no Baseline Delta section is emitted.

## Test posture

- Full unit suite: **1494 passed / 80 skipped** (skips are all
  pre-existing environment gates: Docker compose for c6 Postgres tests,
  CUDA / TensorRT / Tier-2 Jetson hardware for c7, `actionlint` for one
  GHA-lint test).
- Pre-existing perf microbench failures on this dev host (C8 covariance
  projector, C10 batcher overhead, C11 signing-key sign-p99) are
  **outside the changed surface** and are documented in batches 40 + 41
  per-batch reviews. They were not re-investigated in this cumulative
  pass; the recommendation in those reviews stands (re-bench on
  reference hardware before re-classifying).

## Next batch

Batch 43 candidates (per `_docs/02_tasks/_dependencies_table.md` after
AZ-326 + AZ-327 closed): AZ-328 (C12 build-cache orchestrator) is the
natural next consumer — it depends on AZ-326 + AZ-327 + AZ-489 (all
ready) plus AZ-321 / AZ-322 / AZ-323 (already done). The implement
skill should compute the next batch from the dependencies table at the
start of the next loop.
