mirror of
https://github.com/azaion/gps-denied-onboard.git
synced 2026-06-22 17:31:13 +00:00
[AZ-325] C10 CacheProvisioner orchestrator
Implements the public top-level F1 build orchestrator for E-C10 per contract v1.1.0. Composes EngineCompiler (AZ-321), DescriptorBatcher (AZ-322), and ManifestBuilder (AZ-323) into a single idempotent operation guarded by a fcntl-backed cache_root/.c10.lock and a post-build coverage walk. Adds: - CacheProvisionerImpl + FilelockFileLockFactory (provisioner.py) - BuildRequest/BuildReport/BuildOutcome/SectorClassification DTOs + FileLockFactory Protocol + replaced placeholder CacheProvisioner Protocol with v1.1.0 surface (interface.py) - C10ProvisionerConfig wired into C10ProvisioningConfig (config.py) - BuildLockHeldError + ManifestCoverageError (errors.py) - build_cache_provisioner composition root (c10_factory.py) - 18 tests covering AC-1..AC-16 + NFR-perf-coverage-walk - filelock>=3.13,<4.0 (single new third-party dep) Idempotence (CP-INV-1) reuses AZ-323's _compute_manifest_hash / _aggregate_tile_hash so the build-identity decision agrees byte-for- byte with the Manifest's recorded manifest_hash. Coverage rollback uses a .prev rename snapshot. Diagnostic compile_engines_for_corpus is lock-free per AC-10. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Oleksandr Bezdieniezhnykh
@@ -0,0 +1,174 @@
|
||||
# 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`,
|
||||
`Protocol`s 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.
|
||||
Reference in New Issue
Block a user