# Cumulative Code Review — Batches 49-51 (Cycle 1)

**Date**: 2026-05-14
**Range**: batches 49 (AZ-526 `iso_ts_from_clock` consolidation), 50 (AZ-339 MegaLoc + MixVPR), 51 (AZ-340 SelaVPR + EigenPlaces + SALAD)
**Compared against**: previous cumulative review batches 46-48
**Verdict**: **PASS_WITH_WARNINGS**

## Scope

21 files changed across batches 49-51 (12 src/, 4 test/, 2 docs, 3 indirect — package init + deps table + module-layout). 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, and follow-through on prior cumulative findings.

## Carry-over closure from cumulative review 46-48

| Prior finding | Status | Notes |
|---------------|--------|-------|
| F1 (Medium) — `_iso_ts_from_clock` duplicated 4-way | **CLOSED by AZ-526 / Batch 49** | All 4 local copies (`net_vlad.py`, `ultra_vpr.py`, `_faiss_bridge.py`, `operator_reloc_service.py`) now import `iso_ts_from_clock` from `helpers/iso_timestamps`. Regression guard: `tests/unit/test_az508_iso_timestamps.py::test_no_local_iso_ts_from_clock_definitions_remain` (AST-walk). |
| F2 (Low) — `_assert_engine_output_dim` near-duplicated between strategies | **OPEN, escalated to current F1** | Was 2-way at B47; now **7-way** after AZ-339 (B50, +2 copies) and AZ-340 (B51, +3 copies). Hygiene PBI **AZ-527** formally created (link below). |
| F3 (Low) — `_iso_ts_from_clock` outputs `+00:00`; canonical FDR `ts` is `Z` | **CLOSED by AZ-526 / Batch 49** | The new `iso_ts_from_clock(clock)` helper emits the `Z` suffix. All 4 callers now produce the canonical byte-shape. Verified by `test_az526_iso_ts_from_clock_emits_z_suffix`. |

## Findings (this window)

| # | Severity | Category | File:Line | Title |
|---|----------|----------|-----------|-------|
| F1 | Medium | Maintainability | `c2_vpr/{ultra_vpr,net_vlad,mega_loc,mix_vpr,sela_vpr,eigen_places,salad}.py` | `_assert_engine_output_dim` duplicated 7-way across c2_vpr strategy modules — **AZ-527 created** |
| F2 | Low | Spec-Gap | `_docs/02_tasks/done/AZ-{337,338,339,340}*.md` § AC-10 | C2 strategy specs literally name `ConfigurationError`; implementation raises `StrategyNotAvailableError` (env-flag OFF) and `ConfigError` (runtime-label mismatch). Documentation drift; no code change required. |

## Finding Details

### F1: `_assert_engine_output_dim` duplicated 7-way (Medium / Maintainability) — AZ-527 created

- **Locations**:
  - `src/gps_denied_onboard/components/c2_vpr/ultra_vpr.py` (AZ-337, B47)
  - `src/gps_denied_onboard/components/c2_vpr/net_vlad.py` (AZ-338, B46)
  - `src/gps_denied_onboard/components/c2_vpr/mega_loc.py` (AZ-339, B50)
  - `src/gps_denied_onboard/components/c2_vpr/mix_vpr.py` (AZ-339, B50)
  - `src/gps_denied_onboard/components/c2_vpr/sela_vpr.py` (AZ-340, B51 — new)
  - `src/gps_denied_onboard/components/c2_vpr/eigen_places.py` (AZ-340, B51 — new)
  - `src/gps_denied_onboard/components/c2_vpr/salad.py` (AZ-340, B51 — new)

- **Description**: Each copy is the same ~30-line helper that runs a zero-init dry-run inference, asserts the engine output dict has an `"embedding"` key, and asserts the output ndarray shape is `(1, DESCRIPTOR_DIM)`. The only thing that differs across copies is the local `DESCRIPTOR_DIM` constant (512 / 4096 / 4096 / 8448 / 512 / 2048 / 8448) and the local preprocessor type-hint. The error-message strings and the shape-check logic are byte-identical.

- **Action taken**: Hygiene PBI **AZ-527** formally opened in Jira (`Hygiene — consolidate _assert_engine_output_dim into a c2-internal helper`, 2 points, Epic AZ-255 E-C2). Scope: add `src/gps_denied_onboard/components/c2_vpr/_engine_dim_assertion.py` exposing `assert_engine_output_dim(...)` parametrised by `descriptor_dim`, `output_key`, `input_key`; replace the 7 local definitions with imports; add a focused unit test; re-run the existing AZ-337 / 338 / 339 / 340 AC-6 tests unmodified. Link: <https://denyspopov.atlassian.net/browse/AZ-527>.

- **Why escalated from Low (B46-48 cumulative) to Medium**: at 7-way the per-edit risk of one copy drifting from the others (different error-message wording, different shape-check operator order, different early-return semantics) is non-trivial. Each future c2_vpr secondary strategy would add an 8th, 9th copy unless we close the pattern now.

### F2: AC-10 spec drift across the C2 strategy specs (Low / Spec-Gap)

- **Locations**:
  - `_docs/02_tasks/done/AZ-337_c2_ultra_vpr_strategy.md` § AC-10
  - `_docs/02_tasks/done/AZ-338_c2_netvlad_strategy.md` § AC-10
  - `_docs/02_tasks/done/AZ-339_c2_mega_loc_mix_vpr.md` § AC-10
  - `_docs/02_tasks/done/AZ-340_c2_selavpr_eigenplaces_salad.md` § AC-10

- **Description**: Each spec literally names `ConfigurationError` for the "build flag OFF" case and the "runtime-label mismatch" case. The implementation (correctly, by precedent) raises `StrategyNotAvailableError` for the env-flag-OFF case (composition-root concern) and `ConfigError` for the runtime-label-mismatch case (strategy-internal concern). All four AZ-337..AZ-340 implementations made the same choice; per-batch reviews documented this consistently.

- **Suggestion**: amend the four AC-10 wordings in a single shared spec-pass (5 minutes' work) so future batches and audit trails do not have to re-explain the divergence. Sized at <1 point — could ride along with any future c2_vpr spec touch. Optionally fold into AZ-527 if it touches the spec wording for AC-6 anyway.

- **Defer rationale**: pure documentation drift, no code defect, no missing test coverage. Filed as F2 here so it surfaces in the audit trail without blocking.

## Phase Summary

### Phase 1 — Context Loading

- 3 task specs reviewed (AZ-526, AZ-339, AZ-340).
- 4 doc inputs: `architecture.md`, `module-layout.md`, the 3 per-batch reviews (`batch_49_review.md` / `batch_50_review.md` / `batch_51_review.md`), and `_docs/02_document/components/02_c2_vpr/description.md`.
- No `_docs/02_document/architecture_compliance_baseline.md` exists yet; the **Baseline Delta** section is omitted from this report (consistent with prior cumulative reviews).

### Phase 2 — Spec Compliance

Per-batch reviews covered AC-by-AC compliance for each task. Cross-batch checks:

- **AZ-339 + AZ-340 vs AZ-337 + AZ-338 pattern parity**: the five new strategies (MegaLoc, MixVPR, SelaVPR, EigenPlaces, SALAD) line up with UltraVPR's structural skeleton — `create()` factory + `__init__` + `embed_query` + `retrieve_topk` + private `_assert_engine_output_dim` + `_emit_*` FDR helpers. NetVLAD remains the lone outlier (PyTorch architecture-factory path). All five new strategies use the TRT-only path identical to UltraVPR. No accidental drift onto the NetVLAD-only intra-cluster + L2 path.
- **`_STRATEGY_TO_BUILD_FLAG` / `_STRATEGY_TO_MODULE` pre-wiring** in `runtime_root/vpr_factory.py`: the 5 rows for `mega_loc`, `mix_vpr`, `sela_vpr`, `eigen_places`, `salad` were added at AZ-336 land time and remain untouched by AZ-339 / AZ-340 — confirmed by `git log --diff-filter=M -- src/gps_denied_onboard/runtime_root/vpr_factory.py` (no commits in this window). Strategy modules now actually exist for all 5 entries — no more silent ImportError surprises if a downstream test toggles a build flag ON.
- **AC-10 spec wording drift** captured as F2 above.

### Phase 3 — Code Quality

- **SRP**: each new strategy and each new preprocessor has a single clear concern. Helpers separated into `_preprocessor_<backbone>.py` siblings.
- **Error handling**: typed exception envelopes (`ConfigError`, `VprBackboneError`, `VprPreprocessError`, `StrategyNotAvailableError`) used consistently across all 5 new strategies; no bare `except`.
- **Naming**: aligned across all 7 c2_vpr secondary strategies; method shapes for `embed_query` / `retrieve_topk` / `_emit_*` mirror line-for-line.
- **Test quality**: AZ-339 and AZ-340 both adopted the parametrised single-file pattern (driven by `_STRATEGY_MODULES` table) instead of duplicating per-strategy test files. Reduces test surface from ~2300 lines (3 copies of `test_ultra_vpr.py`) to ~700 lines for AZ-340 with strictly equivalent AC coverage. Same precedent as the AZ-339 review noted.
- **Test-environment-mirrors-prod gate**: still satisfied — fakes mirror the production interfaces (`InferenceRuntimeCut`, `DescriptorIndexCut`, `BackbonePreprocessor` Protocols) and never live entirely in test files.

### Phase 4 — Security Quick-Scan

No SQL, no `subprocess`, no `eval` / `exec`, no secrets. New code is pure-Python numerical wiring + FDR record emission. n/a.

### Phase 5 — Performance Scan

- All 5 new strategies use FP16 inputs and pre-compiled TensorRT engines — consistent with the project's perf budget.
- Engine load + output-shape assertion happens at `create()` time (startup) not first-frame — explicit AZ-337 architectural decision; preserved across AZ-339 / AZ-340.
- SALAD's higher D=8448 will produce a larger FAISS index footprint — operator-side concern at corpus build time; flagged in the AZ-340 batch report § Architectural Decisions.
- No obvious hot-path allocations beyond what numpy / FAISS already do.

### Phase 6 — Cross-Task Consistency

- **Pattern parity across all 7 c2_vpr secondary strategies**: confirmed — same skeleton, same FDR record kinds, same log kinds, same error envelope. The duplication F1 is the cost of this parity until AZ-527 lands.
- **`iso_ts_from_clock` import consistency**: all c2_vpr strategy modules now import `iso_ts_from_clock` from `gps_denied_onboard.helpers.iso_timestamps` — verified by AZ-526 regression guard.
- **`intra_cluster_normalise` helper**: still used ONLY by NetVLAD as intended. Verified by spy tests in `test_net_vlad.py` and absence of import in the 6 other strategies.
- **FDR record schema**: no schema-level additions in this window beyond the records the existing strategies already emit. The 5 new strategies emit the same record kinds (`vpr.embedding_complete`, `vpr.retrieval_complete`, `vpr.query_failed`) as UltraVPR / NetVLAD / MegaLoc / MixVPR — verified by `tests/unit/test_az272_fdr_record_schema.py` (passes unmodified).
- **AZ-526 closure** of cumulative-46-48 F1 + F3: verified — zero `_iso_ts_from_clock` definitions remain anywhere under `src/`.

### 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 in any of the 5 new strategy or 5 new preprocessor modules.
2. **Public API respect**: all c2_vpr → c6 / c7 communication continues to go through the structural Protocol cuts (`InferenceRuntimeCut`, `DescriptorIndexCut`). Verified by `test_az270_compose_root.test_ac6_only_compose_root_imports_concrete_strategies` (passes — 8/8).
3. **No new cyclic deps**: built-import graph of changed files + direct dependencies — no cycles introduced.
4. **Duplicate symbols across components**: F1 above (intra-component duplication, scoped to `c2_vpr/*.py`). No cross-component duplication detected.
5. **Cross-cutting concern not locally re-implemented**: AZ-526 correctly hoisted `_iso_ts_from_clock` out of per-component locals. The remaining open pattern is `_assert_engine_output_dim` — but this is intentionally c2-internal (engine output-shape contract is a c2 concern; C7 has its own engine-shape assertions for its own purposes inside the runtime). AZ-527 keeps it inside c2_vpr/, not in shared/helpers.
6. **Preprocessor duplication across the 7 backbones**: per `components/02_c2_vpr/description.md` § 6 and the AZ-339 / AZ-340 task specs § Constraints, this is **intentional and architectural** — each backbone owns its own input-shape constants (224×224, 322×322, 384×384, 480×480, 512×512) so a future model-weights drop can change a backbone's preprocessing without coupling other strategies' weights-versions. **Not a finding.**

## Verdict Justification

- Critical findings: 0
- High findings:     0
- Medium findings:   1 (F1 — already addressed via AZ-527 ticket creation)
- Low findings:      1 (F2 — pure documentation drift)

**PASS_WITH_WARNINGS** — auto-chain to next batch is allowed per implement skill Step 14.5. The F1 finding has an open Jira ticket (AZ-527) ready to be sequenced into a future batch; F2 is documentation-only and does not block.

## Recommended Follow-up

1. **Sequence AZ-527 before any future c2_vpr secondary backbone (none planned in current scope, but defensively before any AZ-358 / AZ-389 / AZ-358 c2-side adjacent work).** 2 points, 1 new module + 7 small edits + 1 test.
2. **F2 spec-pass** (~5 minutes, <1 point): fold the AC-10 wording correction into AZ-527's spec touch if it lands first; otherwise file as a tiny standalone hygiene PBI.
3. **No other carry-over.** All previous cumulative findings are now either CLOSED or have a tracked follow-up.

## Next Cumulative Review

- **Trigger**: after Batch 54 (every K=3).
- **Range**: batches 52-54.
