# 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.
