chore: record cumulative review batches 34-36 + state

Cumulative code review for batches 34-36 (AZ-507, AZ-323, AZ-324,
AZ-306, AZ-322) per implement skill Step 14.5 K=3 cadence.

Verdict: PASS_WITH_WARNINGS — 0 Critical / 0 High / 0 Medium / 3 Low
(all Maintainability). Previous review's Medium F1 (doc-vs-lint) is
RESOLVED by AZ-507. Carryover-Low findings tracked:

- F1: manifest_verifier imports private _aggregate_tile_hash from
  manifest_builder; promote to public or extract to a shared module
  (1-pt follow-up PBI).
- F2: AZ-508 task spec stale — c6 already consolidated within-component,
  c7 has 2 active copies (+ a new thermal_publisher copy not in spec).
- F3: consumer-side Protocol cut pattern still un-documented in
  architecture.md; pattern now 9+ instances and is the established
  cross-component contract surface.

State updated: last_cumulative_review = batches_34-36; sub_step =
parse-tasks; batch 37 (AZ-325 C10 CacheProvisioner solo, 3pt) is next.

Co-authored-by: Cursor <cursoragent@cursor.com>

_docs/03_implementation/cumulative_review_batches_34-36_cycle1_report.md

@@ -0,0 +1,135 @@
+# Cumulative Code Review — Batches 34–36 / Cycle 1
+
+**Date**: 2026-05-13
+**Mode**: Cumulative (all 7 phases, emphasis on Phases 6 + 7)
+**Batches covered**: 34, 35, 36
+**Tasks covered**: AZ-507 (cross-cutting AZ-270 / module-layout alignment + `_types/inference_errors.py` shim), AZ-323 (C10 ManifestBuilder + Ed25519ManifestSigner), AZ-324 (C10 ManifestVerifierImpl), AZ-306 (C6 FaissDescriptorIndex), AZ-322 (C10 DescriptorBatcher)
+**Changed files in scope**: 16 production + 8 tests + 6 docs (see "Scope" below)
+
+| Domain | Files (changed since cumulative_review_batches_31-33_cycle1_report.md) |
+|--------|-----------------------------------------------------------------------|
+| `_types` (cross-cutting) | `_types/inference_errors.py` (new, AZ-507 shim) |
+| c10_provisioning (production) | `manifest_builder.py` (new, AZ-323), `manifest_verifier.py` (new, AZ-324), `descriptor_batcher.py` (new, AZ-322), `c7_engine_embedder.py` (new, AZ-322 adapter), `errors.py` (new, common error parent), `interface.py` (extended — BackboneEmbedder, ManifestSigner, SigningKeyHandle), `config.py` (extended — C10ManifestConfig, BackboneConfig, SigningMode), `engine_compiler.py` (narrowed `except Exception` → typed envelope, AZ-507), `__init__.py` (re-exports the full c10 surface) |
+| c6_tile_cache (production) | `faiss_descriptor_index.py` (new, AZ-306 — faiss-cpu HNSW32 + IndexIDMap2), `config.py` (extended — `faiss_index_path`, `faiss_warmup_query_path`), `postgres_filesystem_store.py` (extended/refactored — uses `_timestamp.iso_ts_now` consolidated helper), removed empty `_native/__init__.py` |
+| runtime_root (composition root) | `c10_factory.py` (added `build_descriptor_batcher`, `build_manifest_builder`, `build_manifest_verifier`, plus 4 c6→c10 adapter functions), `storage_factory.py` (extended for `BUILD_FAISS_INDEX` flag handling) |
+| Tests | `tests/unit/c10_provisioning/test_manifest_builder.py` (new, 685 lines), `test_manifest_verifier.py` (new, 721 lines), `test_descriptor_batcher.py` (new, 591 lines), `test_engine_compiler.py` (updated — typed-envelope catch), `tests/unit/c6_tile_cache/test_faiss_descriptor_index.py` (new, 650 lines), `test_protocol_conformance.py` (updated), `tests/unit/test_az507_inference_errors_shim.py` (new, 88 lines), `tests/conftest.py` (minor — fixture wiring) |
+| Docs | `_docs/02_document/architecture.md` (ADR-009 cross-component contract surface), `_docs/02_document/module-layout.md` (Rule 9 codified, c10/c6 entries updated), `_docs/02_document/components/{08_c6_tile_cache,11_c10_provisioning}/description.md` (updated), `_docs/02_tasks/_dependencies_table.md` (+ AZ-507, AZ-508, AZ-322/323/324 deps), AZ-508 task spec written (hygiene PBI tracking the carryover Finding F2) |
+
+**Verdict**: **PASS_WITH_WARNINGS**
+
+## Summary
+
+No Critical or High findings. Three findings total: all Low / Maintainability, all already partially tracked. The two trust-chain halves (AZ-323 build + AZ-324 verify) shipped together with the supporting Protocol contracts and unit coverage at 685 + 721 + 591 lines, the AZ-306 faiss-cpu strategy lands at 650 lines of tests, and AZ-507 closes the previous review's Medium finding (F1) via the typed-error shim + module-layout rule.
+
+The dominant architectural achievement of this window is the **maturation of the consumer-side structural Protocol cut pattern** into the established cross-component contract surface for C10:
+
+- AZ-507 codifies Rule 9 in `module-layout.md` and adds ADR-009 to `architecture.md`: only `_types/*` + composition-root adapters cross component boundaries.
+- AZ-322 puts the pattern into production at four cut points (`TilesByBboxBatchQuery`, `TilePixelOpener`, `DescriptorIndexRebuilder`, `BackboneEmbedder`) — each with a matching composition-root adapter in `runtime_root/c10_factory.py`.
+- AZ-323 adds two more cuts (`TilesByBboxQuery`, `ManifestSigner` / `SigningKeyHandle`).
+- AZ-324 reuses AZ-323's `TilesByBboxQuery` shape so the verifier and builder share the same C6 adapter at the composition root — no duplicated adapter logic.
+
+Across the four c10 / c6 batches, **zero `components.X` cross-component imports remain** inside `src/gps_denied_onboard/components/**/*.py`. The AZ-270 AST lint (`test_az270_compose_root.test_ac6_only_compose_root_imports_concrete_strategies`) is green and aligned with the documentation it enforces — the doc-vs-lint contradiction from the 31–33 review is fully resolved.
+
+Phase 6 (Cross-Task Consistency) verifies:
+
+- **AZ-507 ↔ AZ-321 typed-envelope contract** — `engine_compiler._compile_one` catches `(EngineBuildError, CalibrationCacheError)` imported from `_types.inference_errors`; unknown exceptions now propagate with original type. `tests/unit/test_az507_inference_errors_shim.py` confirms identity-preserving aliases (the shim re-exports the canonical `c7_inference.errors` classes, not duplicates).
+- **AZ-322 ↔ AZ-306 int64 id contract** — DescriptorBatcher hands `TileBboxRecord` rows to the rebuilder; the c10_factory adapter projects to `TileId`; AZ-306's `FaissDescriptorIndex.rebuild_from_descriptors` invokes the canonical `tile_id_to_int64` helper. `test_descriptor_batcher::test_ac6_descriptor_id_mapping_matches_az306_scheme` asserts the formula matches by importing `tile_id_to_int64` directly.
+- **AZ-323 ↔ AZ-324 trust-chain contract** — both modules consume the SAME canonical-JSON ordering (`orjson.OPT_SORT_KEYS | OPT_INDENT_2`), the SAME aggregate-tile-hash helper (`_aggregate_tile_hash` in `manifest_builder.py`), and the SAME Ed25519 envelope (32-byte pubkey, 64-byte sig). The verifier's MV-INV-5 fast-path (no `tile_metadata_store` in airborne mode) and MV-INV-9 takeoff-origin re-validation are wired by `build_manifest_verifier(with_tile_store=…)` so the composition root picks the right mode per binary (operator vs airborne).
+- **C6 enum / DTO traversal across the composition root** — `c10_factory` adapters consistently convert C6's `SectorClassification`, `Bbox`, `TileId`, `HnswParams` from c6 → c10 cuts via deferred (function-body) imports. No leakage of C6 types into c10's `components/*.py` files.
+
+Phase 7 (Architecture Compliance):
+
+- **Layer direction**: c10 / c6 production code imports only from `_types/*`, `helpers/*`, `config`, `logging`, `clock`, `fdr_client`. All Layer 1 or lower. No upward imports.
+- **Public API respect**: see "zero `components.X` cross-component imports" finding above. `runtime_root/c10_factory.py` is the single cross-component seam; the AZ-270 lint exempts `runtime_root/*`.
+- **No new cyclic module dependencies**: verified by import grep across `src/gps_denied_onboard/components/`.
+- **Duplicate symbols across components**: `_iso_ts_now` is now down to **2 active copies** in c7 (`onnx_trt_ep_runtime.py`, `thermal_publisher.py`) — c6 consolidated within-component to `_timestamp.iso_ts_now` (3 → 1), and AZ-507 dropped the `tensorrt_runtime.py` copy. AZ-508 in `_docs/02_tasks/todo/` is the planned cross-component consolidation; the task spec needs a minor refresh (see Finding F2 below).
+- **Cross-cutting concerns**: `helpers/sha256_sidecar.py` is consistently reused by AZ-306, AZ-323, AZ-324. No re-implementations.
+
+## Findings
+
+| # | Severity | Category | File:Line | Title |
+|---|----------|----------|-----------|-------|
+| 1 | Low | Maintainability | `c10_provisioning/manifest_verifier.py:35-37` ↔ `c10_provisioning/manifest_builder.py:592` | Verifier imports private `_aggregate_tile_hash` from builder — leaking-name dependency on a module-private helper |
+| 2 | Low | Maintainability | `c7_inference/{onnx_trt_ep_runtime,thermal_publisher}.py` (definition sites) + `_docs/02_tasks/todo/AZ-508_hygiene_iso_timestamps_consolidation.md` | AZ-508 task spec lists modules that no longer match reality (c6 already consolidated within-component to `_timestamp.py`; `tensorrt_runtime.py` no longer carries the helper) |
+| 3 | Low | Maintainability | `_docs/02_document/architecture.md` (no dedicated section) + 7-plus active consumer-side Protocol cut sites in c10 alone | "Consumer-side structural Protocol cut" pattern still un-documented in architecture.md — recurrence is now an established primitive, not an exception |
+
+### Finding Details
+
+**F1: Verifier reaches into builder's private helper** (Low / Maintainability)
+
+- Location:
+  - Consumer: `src/gps_denied_onboard/components/c10_provisioning/manifest_verifier.py:35-37` (`from ... manifest_builder import (TilesByBboxQuery, _aggregate_tile_hash)`) and call at `:447` (`computed = _aggregate_tile_hash(records)`).
+  - Producer: `src/gps_denied_onboard/components/c10_provisioning/manifest_builder.py:592` (`def _aggregate_tile_hash(records)`).
+- Description: The verifier (AZ-324) imports a leading-underscore module-private helper from the builder (AZ-323). The two tasks intentionally share the canonical aggregation formula — same `TileHashRecord` shape, same byte ordering, same SHA-256 of the concatenation. The shared dependency is correct; the import name is the smell. A reader of `manifest_builder.py` who sees `_aggregate_tile_hash` reasonably assumes it is a strictly module-internal helper, and a future refactor of the builder's hash format would silently break the verifier with no static signal beyond the underscore.
+- Suggestion: Choose ONE of these reconciliations:
+  - (a) Promote the helper. Rename to public `aggregate_tile_hash` and add it to `manifest_builder.__all__`. Cost: one-line rename + one-line export.
+  - (b) Extract to a shared module. Move into `c10_provisioning/_canonical_hash.py` (intra-component shared utility), have BOTH builder and verifier import from it. This makes the shared contract explicit and keeps `manifest_builder.py` focused on the build pipeline. Cost: ~10 lines.
+- Recommendation: (b) — the function encodes the canonical TileHashRecord ordering + concatenation, which is the trust-chain glue between AZ-323 and AZ-324; making it its own module communicates that contract status.
+- Task: AZ-324 (introduced the import) — but the resolution touches both AZ-323 and AZ-324 files, so file as a small follow-up hygiene PBI rather than re-opening either. Sized at 1 point.
+
+**F2: AZ-508 task spec is stale relative to current code** (Low / Maintainability)
+
+- Location:
+  - `_docs/02_tasks/todo/AZ-508_hygiene_iso_timestamps_consolidation.md` § Problem lines 22-26 (the five-module enumeration).
+- Description: AZ-508's task spec, written after the 31-33 review, lists five `_iso_ts_now` definition sites:
+  1. `c7_inference/tensorrt_runtime.py` — **no longer present** (AZ-507 cleaned it up as part of the typed-envelope refactor).
+  2. `c7_inference/onnx_trt_ep_runtime.py` — still present.
+  3. `c6_tile_cache/postgres_filesystem_store.py`, `freshness_gate.py`, `cache_budget_enforcer.py` — c6 has consolidated **within-component** to `_timestamp.iso_ts_now` (`_timestamp.py` exposes the canonical helper); the three .py files now `from ... _timestamp import iso_ts_now` instead of defining `_iso_ts_now` locally.
+  
+  Plus a sixth site emerged in batch 35: `c7_inference/thermal_publisher.py:343` (AZ-302) — present, NOT listed in AZ-508 spec.
+  
+  Net real state: 2 active copies in c7 (`onnx_trt_ep_runtime.py`, `thermal_publisher.py`) + 1 component-local helper in c6 (`_timestamp.py`). AZ-508's goal is still correct — promote to `helpers/iso_timestamps.py` — but the file list, the call-site list, and the migration plan need a refresh before AZ-508 starts so the implementer doesn't waste time on already-resolved sites.
+- Suggestion: Refresh AZ-508's "Problem", "Outcome", and "Included" sections to reflect the post-batch-36 state:
+  - Active definition sites to consolidate: `c7_inference/onnx_trt_ep_runtime.py`, `c7_inference/thermal_publisher.py`.
+  - Component-local helper to retire: `c6_tile_cache/_timestamp.py` (replace with the new `helpers/iso_timestamps.py` import; delete the `_timestamp.py` module).
+  - Add a regression test forbidding `def _iso_ts_now` or `def iso_ts_now` re-definitions anywhere under `src/gps_denied_onboard/components/**`.
+- Recommendation: refresh AZ-508 in the next "task hygiene" pass; the original intent and complexity (2 pts) remain valid. Do not gate downstream batches on this.
+- Task: AZ-508 (spec drift since 2026-05-12). Not blocking.
+
+**F3: Consumer-side structural Protocol cut pattern still un-documented** (Low / Maintainability)
+
+- Location:
+  - Current active cuts in production: `c10_provisioning/engine_compiler.py::CompileEngineCallable` (AZ-321), `descriptor_batcher.py::{TilesByBboxBatchQuery, TilePixelOpener, DescriptorIndexRebuilder}` (AZ-322), `interface.py::{BackboneEmbedder, ManifestSigner, SigningKeyHandle}` (AZ-322 / AZ-323), `manifest_builder.py::TilesByBboxQuery` (AZ-323).
+  - Pre-existing peer in `_types`: `_types/manifests.py::EngineHandle` (LightGlue cut, now consumed by future C2.5 / C3 matchers as well).
+  - Architecture doc: `_docs/02_document/architecture.md` — has ADR-009 ("interface-first DI") which mentions the pattern in passing but does NOT formalize the "consumer-side cut vs. shared `_types/` cut" decision rule.
+- Description: The 31-33 cumulative review's Finding F3 (Low / Maintainability) flagged this pattern as recurring (2 active sites then). The window since has produced **7 more** consumer-side Protocol cuts in c10 alone. The pattern is no longer an exception — it is the **established cross-component contract surface** of the codebase, and Rule 9 in `module-layout.md` describes its mechanics, but the architecture doc does not yet codify when a cut lives consumer-local vs. when it graduates to `_types/<concern>.py`.
+- Suggestion: Add a `## Consumer-Side Protocol Cuts` section (or extend the existing ADR-009) in `architecture.md` with these clauses:
+  - A consumer-side cut starts LOCAL to its consuming component (e.g. `c10_provisioning.descriptor_batcher.TilesByBboxBatchQuery`).
+  - It graduates to `_types/<concern>.py` ONLY when a SECOND consumer needs the same cut. Avoid pre-emptive shared-typing.
+  - The composition root (`runtime_root/*`) is the ONLY layer allowed to construct the adapter wrapping the concrete producer into the consumer-shaped cut. Adapter classes/functions live in `runtime_root/<consumer>_factory.py`.
+  - Both sides of a cut MUST be `@runtime_checkable Protocol` so the consumer can assert structural conformance in unit tests.
+- Recommendation: file a small "architecture-hygiene" PBI sized at 1 pt to add the section. Do not gate downstream batches on this.
+- Task: cumulative-review carryover (originally surfaced in 31-33 F3). Defer to the next architecture-hygiene window.
+
+## Baseline Delta
+
+`_docs/02_document/architecture_compliance_baseline.md` does not exist (greenfield project). The Baseline Delta section is omitted per `code-review/SKILL.md` "Baseline delta".
+
+## Verdict Logic
+
+- 0 Critical
+- 0 High
+- 0 Medium
+- 3 Low (all Maintainability)
+
+→ **PASS_WITH_WARNINGS**: only Low findings; all three are documented carryover / minor hygiene, none block progression to batch 37. Auto-fix gate matrix classifies all three as auto-fix-eligible if the implementer wants to address them inline (Low / Maintainability), but they are safely deferred to dedicated hygiene PBIs (F1 → new 1-pt follow-up, F2 → AZ-508 refresh, F3 → next architecture-hygiene cycle).
+
+## Test Suite (carried over from batch 36 report)
+
+- AZ-322 unit suite: 16 / 16 passing.
+- AZ-306 unit suite: 21 / 21 passing.
+- AZ-323 unit suite: covered by `test_manifest_builder.py` (685 lines of tests across builder + signer).
+- AZ-324 unit suite: covered by `test_manifest_verifier.py` (721 lines across all `VerifyFailReason` branches).
+- AZ-507 shim: covered by `test_az507_inference_errors_shim.py` (88 lines).
+- Combined targeted run (c10 + c6 + runtime-root): 197 / 197 passing on Tier-0 dev host (59 docker-skip).
+- Full project suite: 1352 passed, 79 skipped, 1 failed.
+  - 79 skipped: docker / Jetson / CUDA / actionlint env-gated (Tier-0 dev host).
+  - 1 failed: `tests/unit/test_ac1_scaffold_layout.py::test_cmake_files_configure` — pre-existing OKVIS2 git-submodule failure (not introduced by batches 34–36).
+
+## Carryover Status Against 31–33 Review
+
+| Previous finding | Severity | Status after batch 36 |
+|---|---|---|
+| F1 (doc-vs-lint contradiction — `module-layout.md` ↔ AZ-270 lint) | Medium / Architecture | **RESOLVED** by AZ-507 (Rule 9 + ADR-009 + `_types/inference_errors.py` shim) |
+| F2 (5× `_iso_ts_now` duplication) | Low / Maintainability | **PARTIALLY RESOLVED** — c6 within-component (3 → 1), AZ-507 dropped 1 c7 copy. 2 c7 copies remain. AZ-508 task spec needs minor refresh (this review's F2). |
+| F3 (consumer-side Protocol cut pattern un-documented) | Low / Maintainability | **CARRIED OVER** — pattern now 9+ instances; codified in `module-layout.md` Rule 9 but architecture.md still needs a dedicated section (this review's F3). |

_docs/_autodev_state.md

@@ -6,11 +6,11 @@ step: 7
 name: Implement
 status: in_progress
 sub_step:
-  phase: 3
-  name: compute-next-batch
-  detail: "batch 37 selected: AZ-325 solo (3pt, C10 CacheProvisioner orchestrator) — all deps satisfied (AZ-321/322/323 done); introduces new filelock dep; needs frozen contract doc"
+  phase: 1
+  name: parse-tasks
+  detail: "batch 37: AZ-325 solo (3pt, C10 CacheProvisioner orchestrator); cumulative 34-36 PASS_WITH_WARNINGS"
 retry_count: 0
 cycle: 1
 tracker: jira
 last_completed_batch: 36
-last_cumulative_review: batches_31-33
+last_cumulative_review: batches_34-36