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

Code Review Report

Batch: 37 (AZ-325 — C10 CacheProvisioner orchestrator) Date: 2026-05-13 Verdict: PASS

Scope

Single-task batch implementing the CacheProvisioner orchestrator per _docs/02_tasks/todo/AZ-325_c10_cache_provisioner.md and the contract _docs/02_document/contracts/c10_provisioning/cache_provisioner.md (v1.1.0).

Changed Files

• pyproject.toml — added filelock>=3.13,<4.0
• src/gps_denied_onboard/components/c10_provisioning/errors.py — added BuildLockHeldError, ManifestCoverageError
• src/gps_denied_onboard/components/c10_provisioning/config.py — added C10ProvisionerConfig, integrated into C10ProvisioningConfig
• src/gps_denied_onboard/components/c10_provisioning/interface.py — replaced placeholder CacheProvisioner Protocol with v1.1.0 surface; added BuildOutcome, BuildRequest, BuildReport, SectorClassification, FileLockFactory
• src/gps_denied_onboard/components/c10_provisioning/provisioner.py — new file: CacheProvisionerImpl, _LockGuard, FilelockFileLockFactory
• src/gps_denied_onboard/components/c10_provisioning/__init__.py — re-exports
• src/gps_denied_onboard/runtime_root/c10_factory.py — added build_cache_provisioner composition root
• tests/unit/c10_provisioning/test_cache_provisioner.py — new file covering AC-1..AC-16 + NFR-perf-coverage-walk

Findings

#	Severity	Category	File:Line	Title
—	—	—	—	No new findings

Findings Carried Over (informational, not new)

• F1 (Low / Maintainability) — carried from batches 31–33 cumulative review. provisioner.py imports _compute_manifest_hash and _aggregate_tile_hash (leading-underscore private helpers) from manifest_builder.py to keep the build-identity hash byte-identical between AZ-323 emission and AZ-325 idempotence. Hygiene PBI to extract these into a shared _build_identity module is intentionally deferred and documented inline in provisioner.py:43-50. No new exposure introduced; the helpers are now used by exactly two sibling modules inside the same component.

Phase Walkthrough

Phase 2 — Spec Compliance

All 16 acceptance criteria are covered by tests in tests/unit/c10_provisioning/test_cache_provisioner.py:

AC	Test
AC-1	test_ac1_cold_build_composes_phases_and_writes_manifest
AC-2	test_ac2_warm_idempotent_re_run_skips_everything
AC-3	test_ac3_different_bbox_triggers_full_rebuild_atomic_replace
AC-4	test_ac4_empty_corpus_surfaces_failure_with_operator_hint
AC-5	test_ac5_concurrent_invocation_raises_build_lock_held_error
AC-6	test_ac6_manifest_coverage_error_rolls_back_to_prior
AC-7	test_ac7_coverage_non_strict_mode_warns_but_continues
AC-8	test_ac8_lock_released_on_every_exit_path
AC-9	test_ac9_hard_errors_propagate_without_state_corruption
AC-10	test_ac10_compile_engines_for_corpus_passthrough (+ test_diagnostic_engine_compile_does_not_acquire_lock)
AC-11	test_ac11_protocol_conformance_isinstance
AC-12	test_ac12_cold_build_benchmark_within_envelope (skipped — GPU-only manual run)
AC-13	test_ac13_warm_idempotent_benchmark_within_envelope
AC-14	test_ac14_takeoff_origin_mismatch_triggers_full_rebuild
AC-15	test_ac15_takeoff_origin_none_propagates_with_no_flight_block
AC-16	test_ac16_flight_id_participation_in_idempotence
NFR-perf-coverage-walk	test_nfr_perf_coverage_walk_under_one_second

Contract verification: interface.py matches contract v1.1.0 shape (BuildRequest carries takeoff_origin: LatLonAlt | None and flight_id: UUID | None, both defaulting to None for back-compat). CP-INV-1..CP-INV-9 are enforced (CP-INV-8 + CP-INV-9 covered by AC-14..AC-16 tests; CP-INV-4 by AC-5 + AC-8; CP-INV-3 by AC-6 + AC-7).

Phase 3 — Code Quality

• SRP: CacheProvisionerImpl has a clear public surface (build_cache_artifacts, compile_engines_for_corpus); each helper has a single purpose (idempotence check, active build, coverage walk, rollback, snapshot, etc.).
• Error handling: every failure path emits a structured ERROR/WARN log with kind + kv; every exception path is in a try/except that restores prior state (no bare except).
• Naming: _run_active_build, _check_idempotence, _verify_coverage, _snapshot_prior_manifest, _restore_prior_manifest — all caller-clear.
• Complexity: build_cache_artifacts is 50 lines and delegates to helpers; _run_active_build is ~110 lines but linearly walks the four phases (engine compile, descriptor populate, manifest build, coverage verify) with a single rollback point per phase.
• DRY: _restore_prior_manifest is the single rollback site; called from every error/abort path inside _run_active_build.
• Test quality: every test uses Arrange/Act/Assert markers; assertions cover both observable outcome (outcome, manifest_hash, on-disk files) AND collaborator behavior (call counts on fakes).
• Dead code: none introduced.

Phase 4 — Security Quick-Scan

• No SQL, no shell-out, no subprocess, no eval.
• No hardcoded secrets. Operator key is a Path injected via the BuildRequest and forwarded to AZ-323 (CP-INV-7 — key is read once, zeroized by AZ-323's signer).
• No sensitive data in logs (calibration / engine bytes / key bytes are never logged; only paths and SHA-256 prefixes).
• Lockfile path is bound to cache_root (operator-controlled); no path traversal vector.

Phase 5 — Performance Scan

• Coverage walk: single Path.rglob("*") pass, O(N files), benchmarked by test_nfr_perf_coverage_walk_under_one_second (well under 1 s for 2k files).
• Tile query: single query_by_bbox call per invocation; sorted once.
• Idempotence path: zero compute outside SHA-256 of calibration bytes and tile hash aggregate; warm path measured at < 1 ms in the unit test.
• No N+1, no unbounded fetch, no blocking I/O in async context.

Phase 6 — Cross-Task Consistency

• Composes AZ-321 (EngineCompiler), AZ-322 (DescriptorBatcher), AZ-323 (ManifestBuilder) per the contract.
• Build-identity hash uses AZ-323's existing _compute_manifest_hash + _aggregate_tile_hash — guaranteeing byte-for-byte agreement with the emitted build.manifest_hash. The shared-helper hygiene PBI is documented in-file.
• DTOs follow the project's existing pattern: frozen @dataclass, Protocols with @runtime_checkable.

Phase 7 — Architecture Compliance

• Layer direction: provisioner.py imports only from sibling C10 modules, _types/, helpers/, clock, errors, interface, config. No upward dependency.
• Public API respect: c10_factory.py imports from c10_provisioning's top-level __init__.py re-exports only — no internal-file imports across components.
• No new cyclic dependencies (verified by import graph: provisioner → manifest_builder is a peer-within-component dependency, no back edge).
• Cross-cutting concerns: logger / clock / atomic-write helpers come from the shared layers (gps_denied_onboard.clock, gps_denied_onboard.helpers.sha256_sidecar); none re-implemented locally.

Test Run

tests/unit/c10_provisioning/test_cache_provisioner.py  17 passed, 1 skipped
tests/unit/c10_provisioning/                            85 passed, 3 skipped, 1 pre-existing failure

Pre-existing failure: test_descriptor_batcher.py::test_ac6_descriptor_id_mapping_matches_az306_scheme — fails identically on HEAD without this batch's changes (ModuleNotFoundError: No module named 'faiss'). Not introduced by AZ-325.

Verdict Logic

• 0 Critical, 0 High, 0 Medium, 0 Low (new) findings → PASS.
• F1 carried over from prior cumulative review is informational only (Low / Maintainability) and remains tracked as a deferred hygiene PBI.