[AZ-337] [AZ-338] [AZ-508] Cumulative review batches 46-48

Verdict: PASS_WITH_WARNINGS. Per-batch reviews already validated
each task's ACs; this cumulative review focuses on cross-batch
drift and surfaces 1 Medium + 2 Low maintainability findings:

- F1 (Medium): `_iso_ts_from_clock` Clock-injected helper duplicated
  across 4 files (c2_vpr/net_vlad + ultra_vpr + _faiss_bridge,
  c12_operator_orchestrator/operator_reloc_service). B46 + B47
  carry inline comments anticipating AZ-508 would consolidate this,
  but AZ-508 (Batch 48) scoped itself narrower (stdlib-only,
  Excluded the Clock-injected variant). Recommend a 2-point follow-up
  PBI adding `iso_ts_from_clock(clock)` to helpers/iso_timestamps.py
  before AZ-339 / AZ-340 / AZ-358 / AZ-389 add more copies.

- F2 (Low): `_assert_engine_output_dim` near-duplicated between
  NetVLAD and UltraVPR. Defer consolidation until 5 c2_vpr strategies
  are in flight (after AZ-339 / AZ-340).

- F3 (Low): Clock-driven helper outputs `+00:00`; canonical FDR `ts`
  is `Z`. Fold into F1 follow-up PBI.

No Critical or High findings; auto-chain to next batch allowed.

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

_docs/03_implementation/cumulative_review_batches_46-48_cycle1_report.md

@@ -0,0 +1,246 @@
+# Cumulative Code Review — Batches 46-48 (Cycle 1)
+
+**Date**: 2026-05-13
+**Range**: batches 46 (AZ-338 NetVLAD), 47 (AZ-337 UltraVPR), 48 (AZ-508 ISO-ts helper)
+**Compared against**: previous cumulative review batches 43-45
+**Verdict**: **PASS_WITH_WARNINGS**
+
+## Scope
+
+24 files changed across batches 46-48 (15 src/, 5 test/, plus 1 helper
+package init and 3 c6/c7 import rewires). Cumulative review focuses on
+cross-batch concerns that per-batch reviews cannot catch: duplicate
+symbols introduced across different batches, architecture drift across
+the trio, contract drift between producer/consumer batches.
+
+## Findings
+
+| # | Severity | Category | File:Line | Title |
+|---|----------|----------|-----------|-------|
+| F1 | Medium | Maintainability | `c2_vpr/{net_vlad,ultra_vpr}.py` + `c12_operator_orchestrator/operator_reloc_service.py` + `c2_vpr/_faiss_bridge.py` | `_iso_ts_from_clock` duplicated across 4 files |
+| F2 | Low | Maintainability | `c2_vpr/net_vlad.py:502` & `c2_vpr/ultra_vpr.py:440` | `_assert_engine_output_dim` near-duplicated between strategies |
+| F3 | Low | Maintainability | `c2_vpr/{net_vlad,ultra_vpr}.py` | `_iso_ts_from_clock` outputs `+00:00`; canonical FDR `ts` is `Z` |
+
+## Finding Details
+
+### F1: `_iso_ts_from_clock` duplicated across 4 files (Medium / Maintainability)
+
+- **Locations**:
+  - `src/gps_denied_onboard/components/c2_vpr/net_vlad.py:375` (added Batch 46)
+  - `src/gps_denied_onboard/components/c2_vpr/ultra_vpr.py:326` (added Batch 47)
+  - `src/gps_denied_onboard/components/c2_vpr/_faiss_bridge.py` (pre-batch-46)
+  - `src/gps_denied_onboard/components/c12_operator_orchestrator/operator_reloc_service.py` (pre-batch-46)
+
+- **Description**: Each copy is an identical 5-line function that converts
+  a `Clock.time_ns()` into a nanosecond-precision ISO-8601 string. Both
+  net_vlad and ultra_vpr carry an inline comment that says
+  "AZ-508 will consolidate the duplicate helpers across c2/c11/c12/c6" —
+  but AZ-508 (Batch 48) scoped itself narrower (stdlib `datetime.now`
+  consolidation only, the Clock-injected variant was explicitly excluded
+  under "any change to flight_clock, wall_clock, or other adapter-style
+  time sources").
+
+  As C2 (AZ-339 / AZ-340), C4 (AZ-358 / AZ-361), C5 (AZ-389), and
+  remaining c11/c12 batches land, this pattern will keep multiplying.
+
+- **Suggestion**: open a new hygiene PBI **AZ-XXX** (≤2 points) to add a
+  second function to `helpers/iso_timestamps.py`:
+
+  ```python
+  def iso_ts_from_clock(clock: Clock) -> str:
+      """Format `clock.time_ns()` as RFC 3339 UTC with nanosecond precision."""
+  ```
+
+  This intentionally keeps both `iso_ts_now()` (wall-clock, microseconds,
+  Z-suffix) and `iso_ts_from_clock(clock)` (injected-clock, nanoseconds)
+  in the same Layer-1 module. Both serve FDR envelope timestamps but
+  have legitimately different testability requirements (the second is
+  Clock-injected so replay tests can pin time).
+
+  When the new helper lands, the 4 existing copies (and the two inline
+  comments anticipating AZ-508) get removed. Sized ≤2 points to match
+  the AZ-508 precedent.
+
+- **Task scope context**: F1 is a forward-looking finding; B46 and B47
+  pre-acknowledged it in comments. Not a regression of Batch 48 — Batch
+  48's own AC-4 (zero stray `iso_ts_now` definitions) passed because
+  the duplicates are named `_iso_ts_from_clock`, not `iso_ts_now`.
+
+### F2: `_assert_engine_output_dim` near-duplicated between strategies (Low / Maintainability)
+
+- **Locations**:
+  - `src/gps_denied_onboard/components/c2_vpr/net_vlad.py:502`
+  - `src/gps_denied_onboard/components/c2_vpr/ultra_vpr.py:440`
+
+- **Description**: Both files define a private `_assert_engine_output_dim`
+  helper with near-identical bodies. Differences are limited to:
+  1. NetVLAD uses string literals `"input"` / `"vlad_descriptor"`;
+     UltraVPR uses module-level `_ENGINE_INPUT_KEY` / `_OUTPUT_KEY`
+     constants.
+  2. NetVLAD takes `expected_dim` as a parameter; UltraVPR pins it to
+     the module-level `DESCRIPTOR_DIM = 512` constant.
+  3. Different preprocessor types in the signature.
+
+  The error-message strings and the shape-check logic are byte-identical.
+
+- **Suggestion**: when AZ-339 and AZ-340 land (3 more strategies), the
+  duplication will grow to 5 copies. Consolidate then into a single
+  `_assert_engine_output_dim` inside a new c2_vpr-internal module
+  (`_engine_probe.py`) accepting `input_key: str`, `output_key: str`,
+  `expected_dim: int`, `preprocessor: BackbonePreprocessor` (uses the
+  existing Protocol). Not urgent for Batch 48; revisit after AZ-339.
+
+- **Defer rationale**: with only 2 copies today the consolidation buys
+  little; with 5 copies expected after AZ-339 + AZ-340 it becomes
+  worthwhile. Bookmark, do not file yet.
+
+### F3: `_iso_ts_from_clock` outputs `+00:00`; canonical FDR `ts` is `Z` (Low / Maintainability)
+
+- **Locations**: same as F1.
+
+- **Description**: The Clock-driven helper formats as
+  `"%Y-%m-%dT%H:%M:%S.{nanos:09d}+00:00"`, while the canonical FDR `ts`
+  fixture in `test_az272_fdr_record_schema.py` and the new
+  `helpers/iso_timestamps.iso_ts_now()` use `Z` suffix
+  (`%Y-%m-%dT%H:%M:%S.%fZ`). Both are valid RFC 3339; the FDR `ts`
+  field is typed `str` without format validation, so consumers parse
+  both. This means c2_vpr-emitted FDR records carry a slightly
+  different `ts` byte-shape than c6/c7/c11/c13-emitted records.
+
+- **Suggestion**: fold the fix into the F1 PBI — the new
+  `iso_ts_from_clock` helper should emit the same `Z` suffix as
+  `iso_ts_now` for consistency. (Nanosecond precision is preserved
+  by using `f"{...}.{fraction_ns:09d}Z"` rather than the `+00:00`
+  variant.)
+
+## Phase Summary
+
+### Phase 1 — Context Loading
+
+- 3 task specs reviewed (AZ-338 / AZ-337 / AZ-508).
+- 4 doc inputs: `architecture.md`, `module-layout.md`, the 3 per-batch
+  reviews (`batch_46_review.md`, `batch_47_review.md`,
+  `batch_48_review.md`), and `_docs/02_document/system-flows.md`.
+- No `_docs/02_document/architecture_compliance_baseline.md` exists yet,
+  so the **Baseline Delta** section is omitted from this report (will
+  be enabled once that baseline is produced — likely an existing-code
+  flow Step 2 deliverable for future cycles).
+
+### Phase 2 — Spec Compliance
+
+Per-batch reviews already covered AC-by-AC compliance for each task.
+Cross-batch check: the AZ-338 + AZ-337 strategies are wired into the
+same `vpr_factory.py` and share the `inference_runtime_cut` /
+`descriptor_index_cut` Protocols; the NetVLAD strategy explicitly
+exposes `MODEL_NAME` / `architecture_factory` while UltraVPR does NOT
+(it consumes a pre-compiled `.trt` engine). The composition root's
+`_register_strategy_architecture` helper no-ops cleanly for the
+UltraVPR case — verified by `test_create_does_not_register_pytorch_architecture`
+in B47. No drift.
+
+### Phase 3 — Code Quality
+
+- **SRP**: NetVLAD and UltraVPR each have a single clear concern
+  (one backbone strategy); helpers separated into `_preprocessor_*.py`.
+- **Error handling**: typed exception envelopes from `_types.inference_errors`
+  used consistently; no bare `except`.
+- **Naming**: aligned across both strategies; method shapes for
+  `embed_query` / `retrieve_topk` / `_emit_*` mirror each other.
+- **Test quality**: AC-by-AC coverage with regression spies (e.g.,
+  `test_ac2_embedding_is_single_stage_l2_no_intra_cluster_path` in B47
+  uses a spy normaliser to guard against a future refactor accidentally
+  introducing NetVLAD's two-stage path into UltraVPR — excellent
+  defensive testing).
+
+### Phase 4 — Security Quick-Scan
+
+No SQL, no `subprocess`, no `eval` / `exec`, no secrets. The new code
+is pure-Python numerical wiring plus FDR record emission. n/a.
+
+### Phase 5 — Performance Scan
+
+- Both new strategies use FP16 inputs and pre-compiled engines —
+  consistent with the project's perf budget.
+- Engine load + output-shape assertion happens at `create()` time
+  (startup) not first-frame — explicit AB47 architectural decision,
+  matches Constraint § 5.
+- No obvious hot-path allocations beyond what numpy / FAISS already
+  do.
+
+### Phase 6 — Cross-Task Consistency
+
+- **NetVLAD vs UltraVPR pattern parity**: method shapes for
+  `embed_query` / `retrieve_topk` / `_emit_*` mirror line-for-line;
+  strategy-specific paths (single-stage L2 vs intra-cluster + L2,
+  `"vlad_descriptor"` vs `"embedding"` output key, PyTorch vs TRT
+  runtime, architecture registry vs no registry) are clearly
+  localised. Good design — drops AZ-339 / AZ-340 into the same
+  skeleton.
+- **Shared `FaissBridge` + `inference_runtime_cut` + `descriptor_index_cut`**:
+  both strategies wire through the same Layer-2 internals; no
+  contract drift detected.
+- **`intra_cluster_normalise` helper** (added in B46): used ONLY by
+  NetVLAD as intended. UltraVPR's preprocessor and forward path do
+  NOT call it — verified by the B47 spy test. No accidental
+  cross-coupling.
+- **FDR record schema additions** in `test_az272_fdr_record_schema.py`
+  (21 new lines): these are NEW record kinds added by B46/B47, not
+  modifications of existing kinds — does not break AZ-508 AC-5's
+  "tests pass unmodified" requirement for the *existing* records.
+- **AZ-508 closure** of the F1 (`_iso_ts_now`) finding from cumulative
+  43-45: verified — zero `_iso_ts_now` definitions remain anywhere
+  under `src/` (asserted by AZ-508's AST-walk test).
+
+### Phase 7 — Architecture Compliance
+
+1. **Layer direction**: c2_vpr (Layer 3) imports from `_types`
+   (Layer 1), `helpers/*` (Layer 1), `config` (Layer 1), `logging`
+   (Layer 1), `fdr_client` (Layer 1), `clock` (Layer 1) — all
+   downward. No upward imports detected.
+
+2. **Public API respect**: no cross-component imports from c2_vpr
+   except via `_types` and `helpers`. The `InferenceRuntimeCut` /
+   `DescriptorIndexCut` patterns (consumer-side structural Protocol
+   cuts) correctly avoid `from gps_denied_onboard.components.c6_tile_cache import ...`
+   and `from gps_denied_onboard.components.c7_inference import ...`.
+   Verified by `test_az270_compose_root.test_ac6_only_compose_root_imports_concrete_strategies`
+   (passes).
+
+3. **No new cyclic deps**: built-import graph of changed files +
+   direct dependencies — no cycles introduced.
+
+4. **Duplicate symbols across components**: F1 + F2 + F3 above.
+
+5. **Cross-cutting concern not locally re-implemented**: B48's
+   `helpers/iso_timestamps` correctly hoisted `_iso_ts_now` out of
+   per-component locals. F1 is the same shape of issue for the
+   Clock-driven variant — recommend folding it into the same
+   helper module via the F1 PBI suggestion.
+
+## Verdict Justification
+
+- Critical findings: 0
+- High findings:     0
+- Medium findings:   1 (F1)
+- Low findings:      2 (F2, F3)
+
+**PASS_WITH_WARNINGS** — auto-chain to next batch is allowed per
+implement skill Step 14.5. The F1 / F2 / F3 findings are flagged for
+follow-up but do not block batch 49.
+
+## Recommended Follow-up
+
+1. **Before AZ-339** (Batch 49 candidate): file a 2-point hygiene PBI
+   `iso_ts_from_clock` consolidation in `helpers/iso_timestamps.py`
+   that covers F1 + F3 in a single small task. This stops the pattern
+   recurring as AZ-339 / AZ-340 / AZ-358 / AZ-389 add more copies.
+2. **After AZ-339 / AZ-340**: revisit F2 (`_assert_engine_output_dim`
+   consolidation) when there are 5 c2_vpr strategies in flight.
+3. **Spec hygiene for F2 carried over from B46/B47** (C7 API name drift
+   in AZ-339 / AZ-340 / AZ-358 / AZ-349 specs) — STILL outstanding,
+   not addressed by Batch 48. Recommend filing before AZ-339 starts.
+
+## Next Cumulative Review
+
+- **Trigger**: after Batch 51 (every K=3).
+- **Range**: batches 49-51.

_docs/_autodev_state.md

@@ -8,9 +8,9 @@ status: in_progress
 sub_step:
   phase: 7
   name: batch-loop
-  detail: "cumulative review batches 46-48 due before next batch"
+  detail: ""
 retry_count: 0
 cycle: 1
 tracker: jira
 last_completed_batch: 48
-last_cumulative_review: batches_43-45
+last_cumulative_review: batches_46-48