# Cumulative Code Review — Batches 52-54 (Cycle 1)

**Date**: 2026-05-14
**Range**: batches 52 (AZ-527 — c2_vpr `_assert_engine_output_dim` consolidation), 53 (AZ-333 — C1 VINS-Mono Strategy), 54 (AZ-334 — C1 KLT/RANSAC Strategy)
**Compared against**: previous cumulative review batches 49-51
**Verdict**: **PASS_WITH_WARNINGS**

## Scope

32 files changed across batches 52-54 (`git diff --stat f6a180e..HEAD`): 16 src/, 5 test/, 1 native binding (cpp), 2 cmake, 6 docs (3 batch reports + 3 review reports), 2 indirect (package init + deps table + state file). 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.

The trio is the strategic centre of this window: AZ-333 + AZ-334 completed the c1_vio `VioStrategy` triplet (Okvis2 + VinsMono + KltRansac), making the orchestration-spine duplication finding (deferred since B53) actionable for the first time. AZ-527 closed the parallel c2_vpr-side finding.

## Carry-over closure from cumulative review 49-51

| Prior finding | Status | Notes |
|---------------|--------|-------|
| F1 (Medium) — `_assert_engine_output_dim` duplicated 7-way across c2_vpr strategies | **CLOSED by AZ-527 / Batch 52** | All 7 strategy modules (`ultra_vpr.py`, `net_vlad.py`, `mega_loc.py`, `mix_vpr.py`, `sela_vpr.py`, `eigen_places.py`, `salad.py`) now import `assert_engine_output_dim` from `c2_vpr/_engine_dim_assertion.py`. Regression guard: `tests/unit/c2_vpr/test_az527_engine_dim_assertion.py::test_ac4_no_stray_engine_dim_assertion_definitions_outside_helper` (AST walk) + `::test_ac4_seven_strategy_modules_import_the_helper` (import grep). Helper stays c2-internal (underscore-prefixed module, NOT in `c2_vpr/__init__.py`'s Public API). |
| F2 (Low) — AC-10 spec wording drift (`ConfigurationError` vs `StrategyNotAvailableError`/`ConfigError`) across AZ-337/338/339/340 specs | **OPEN — carry forward** | Documentation-only drift in `_docs/02_tasks/done/AZ-337..AZ-340_*.md` § AC-10. No code defect. Sized at <1 point; can ride along with any future c2_vpr spec touch. Not escalated. |

## Findings (this window)

| # | Severity | Category | File:Line | Title |
|---|----------|----------|-----------|-------|
| F1 | Medium | Maintainability | `c1_vio/{okvis2,vins_mono,klt_ransac}.py` | c1_vio strategy facade orchestration spine duplicated 3-way — **AZ-528 created** |
| F2 | Low | Maintainability | `tests/unit/c1_vio/{conftest.py, test_klt_ransac_strategy.py}` | Test fake / `_patch_pose_recovery` patching helper not yet shared across the c1_vio strategy trio — deferred to a later hygiene pass |

## Finding Details

### F1: c1_vio strategy facade orchestration spine duplicated 3-way (Medium / Maintainability) — AZ-528 created

- **Locations**:
  - `src/gps_denied_onboard/components/c1_vio/okvis2.py` (AZ-332)
  - `src/gps_denied_onboard/components/c1_vio/vins_mono.py` (AZ-333, B53)
  - `src/gps_denied_onboard/components/c1_vio/klt_ransac.py` (AZ-334, B54)

- **Description**: The following module-level helpers are byte-identical across all three strategy modules:

  - `_now_iso() -> str` — ISO-8601 UTC timestamp for FDR record `ts`.
  - `_bias_norm(bias: ImuBias) -> float` — L2 norm of the concatenated 6-vector `(accel ‖ gyro)`.
  - `_se3_from_4x4(matrix) -> gtsam.Pose3` — lazy gtsam import + np-array coerce.

  The following instance methods are byte-identical modulo strategy-local config-knob references and the strategy-label / producer-ID constants:

  - `_classify_state(self, fq) -> VioState` — INIT/TRACKING/DEGRADED gate; differs only in the threshold attribute (`self._okvis2_cfg.degraded_feature_threshold` vs `self._cfg.min_features_for_pose`).
  - `_tick_lost(self, frame_id)` — DEGRADED/LOST transition counter; byte-identical.
  - `_emit_transition(self, new_state, frame_id)` — FDR `vio.health` record emit; differs only in the module-level `_PRODUCER_ID` and `_STRATEGY_LABEL` constants captured at emit time.
  - `_frame_ts_ns(frame)` + `_frame_image(frame)` — OKVIS2 + VINS-Mono only; KLT/RANSAC uses inline numpy access (equivalent shape; remains exempt from the consolidation since its geometry pipeline owns image coercion).

  The geometry-specific pipeline (OKVIS2 cascade init + native binding driver, VINS-Mono loosely-coupled estimator driver, KLT/RANSAC seed/track/recoverPose) IS unique to each strategy and lives in its own module. That boundary is correct — the hygiene PBI does NOT touch the geometry pipelines.

- **Action taken**: Hygiene PBI **AZ-528** formally opened in Jira (`Hygiene — consolidate c1_vio strategy facade orchestration spine`, 3 points, Epic AZ-254 E-C1). Scope: add `src/gps_denied_onboard/components/c1_vio/_facade_spine.py` exposing the 3 free functions + a `FacadeSpine` mixin parametrised by a strategy-local feature-threshold hook; replace the duplicated definitions in the 3 strategy modules with imports; add a focused unit test + AST-walk regression guard + import-grep regression guard; re-run existing AZ-332 / AZ-333 / AZ-334 AC tests unmodified. Link: <https://denyspopov.atlassian.net/browse/AZ-528>.

- **Why escalated from Low (B53 + B54 per-batch) to Medium**: at 3-way the per-edit risk of one copy drifting from the others (different error-message wording, different state-machine threshold semantics, different FDR payload field order) is non-trivial — the same logic the cumulative B46-48 review used to escalate `_iso_ts_from_clock`. Every future c1_vio strategy would add a 4th copy unless we close the pattern now. The right factoring (template-method spine + per-strategy geometry hook) is now visible because all three strategy shapes exist; doing the consolidation before AZ-334 landed would have over-fit the spine to the OKVIS2 + VINS-Mono pair.

- **Why Medium, not High**: the duplication is structurally bounded — three strategy files, no consumer code drifting around it. The spine is byte-identical today; there is no active drift. High would be appropriate only if one copy had already drifted from the others (it has not — the per-batch reviews verified parity at each land).

### F2: c1_vio test fakes / `_patch_pose_recovery` not yet shared (Low / Maintainability)

- **Locations**:
  - `tests/unit/c1_vio/conftest.py` (`FakeOkvis2Backend` + `FakeVinsMonoBackend`)
  - `tests/unit/c1_vio/test_klt_ransac_strategy.py` (`_patch_pose_recovery`)

- **Description**: `FakeOkvis2Backend` and `FakeVinsMonoBackend` are near-copies with renamed exception types and renamed scripted-output payloads. The shared `ScriptedOutput` dataclass + `_make_default_payload` helper ARE already extracted to `conftest.py` (the productive cut from B53). The KLT/RANSAC test file uses a different abstraction (`_patch_pose_recovery` monkey-patches real OpenCV bindings rather than substituting a fake backend) because KLT/RANSAC has no native binding to fake. A unified "scripted success per strategy" fixture surface is plausible but lives at a different abstraction layer than F1.

- **Suggestion**: defer to a later hygiene pass — explicitly NOT bundled into AZ-528. The facade-spine consolidation should land cleanly first; the test-fixture refactor can ride a future c1_vio touch (e.g. when AZ-335 / warm-start hint persistence lands). Sized at 1-2 points. Recording here so the next cumulative review sees it carried over rather than re-discovered.

- **Defer rationale**: Low severity, no active drift, no test instability. The two abstractions (fake backend vs monkeypatch real bindings) genuinely differ — they may not collapse into a single shape and forcing them would create the wrong base class.

## Phase Summary

### Phase 1 — Context Loading

- 3 task specs reviewed (AZ-527, AZ-333, AZ-334) from `_docs/02_tasks/done/`.
- 5 doc inputs: `_docs/02_document/architecture.md`, `_docs/02_document/module-layout.md`, the 3 per-batch reviews (`batch_52_review.md` / `batch_53_review.md` / `batch_54_review.md`), and `_docs/02_document/components/01_c1_vio/description.md` + `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 (B52: 6/6 + 1 NFR; B53: 10/10 + 1 NFR; B54: 11/11 + 1 NFR). Cross-batch checks:

- **AZ-333 + AZ-334 strategy-facade parity vs AZ-332 (OKVIS2)**: the two new strategies line up with OKVIS2's structural skeleton — module-level helpers (`_now_iso`, `_bias_norm`, `_se3_from_4x4`) + instance-method spine (`_classify_state`, `_tick_lost`, `_emit_transition`) + per-strategy geometry pipeline. The byte-equivalence is exactly what makes the AZ-331 factory + IT-12 comparative harness work — F1 captures it as the consolidation target, NOT as a drift defect.
- **`_STRATEGY_TO_BUILD_FLAG` / `_STRATEGY_TO_MODULE` pre-wiring** in `runtime_root/vio_factory.py`: the 3 rows for `okvis2`, `vins_mono`, `klt_ransac` were added at AZ-331 land time and remain untouched by AZ-333 / AZ-334 — verified by reading the factory module (unchanged in this window). Strategy modules now actually exist for all 3 entries — no more silent ImportError surprises if a downstream test toggles a build flag ON.
- **`vio.health` FDR record schema parity**: all three strategies emit FDR records with identical payload shape (`{state, consecutive_lost, bias_norm, strategy_label, frame_id}`) — verified by reading the `_emit_transition` bodies. The only difference is the `strategy_label` value; that field exists precisely to disambiguate at consumer-side.
- **AZ-527 closure** of cumulative-49-51 F1: verified — `_assert_engine_output_dim` definitions exist in exactly 1 place (`c2_vpr/_engine_dim_assertion.py`); the 7 strategy modules import the helper.

No Spec-Gap findings.

### Phase 3 — Code Quality

- **SRP**: each new strategy and the new c2_vpr helper has a single clear concern. The c1_vio strategies' SRP boundary is at the geometry-pipeline level; the facade-spine duplication is a DRY concern (F1), not an SRP one.
- **Error handling**: typed exception envelopes (`VioFatalError`, `VioInitializingError`, `RansacFilterError`, `ImuPreintegrationError`, `ConfigError`, `StrategyNotAvailableError`) used consistently across all three c1_vio strategies + the c2_vpr helper; no bare `except`; `__cause__` chaining preserved at every rewrap point.
- **Naming**: aligned across the three c1_vio strategies — method shapes mirror line-for-line (which is the F1 target).
- **Test quality**: B52, B53, B54 each follow the AAA pattern; behaviourally-meaningful assertions throughout; no "did not throw" placeholder tests. KLT/RANSAC's monkeypatch-based deterministic testing (F2 location) is the most novel pattern in the window — it correctly tests the facade's state machine without depending on real OpenCV geometry on synthetic correspondences; real-geometry validation is correctly deferred to C1-IT-12 (Jetson Tier-2 fixture).
- **Test-environment-mirrors-prod gate**: still satisfied — fakes mirror the production binding interfaces (`Okvis2Backend`, `VinsMonoBackend` Protocol-shaped fakes; KLT/RANSAC monkey-patches the real `cv2` symbols).
- **Annotation form**: `from __future__ import annotations` + bare type names — Ruff `UP037` clean across all 16 src files in the window.

### Phase 4 — Security Quick-Scan

No SQL, no `subprocess`, no `eval` / `exec`, no secrets. New code is pure-Python numerical wiring + FDR record emission + (for VINS-Mono) a pybind11 binding skeleton that is gated OFF by default (`BUILD_VINS_MONO=OFF`). Input validation present at the bindings boundary (numpy shape/dtype checks for 3×3 K, 4×4 pose, length-3 IMU vectors). n/a.

### Phase 5 — Performance Scan

- All three c1_vio strategies budget at p95 ≤ 80 ms on Tier-2 per ADR-002 (OKVIS2 + KLT/RANSAC bound by C1-PT-01; VINS-Mono exempt per task spec). Honest-covariance estimators avoid client-side floors (verified by AC-9 tier-2 tests in B53 + B54).
- AZ-527's helper consolidation has zero hot-path impact — assertion runs at engine `create()` time (startup), not per frame.
- No N+1, no unbounded buffers. FDR client capacity bounded by AZ-273 ringbuf at the producer side; tests cap explicitly at 256.

### Phase 6 — Cross-Task Consistency

- **Strategy-facade pattern parity across the c1_vio triplet**: confirmed — same spine, same FDR record kind (`vio.health`), same state enum (`VioState{INIT,TRACKING,DEGRADED,LOST}`), same error envelope. F1 IS the cost of this parity until AZ-528 lands.
- **`assert_engine_output_dim` import consistency across c2_vpr**: all 7 c2_vpr strategy modules now import the helper — verified by AZ-527's import-grep regression guard (`test_ac4_seven_strategy_modules_import_the_helper`).
- **No cross-component leakage**: c1_vio facade-spine duplication is intra-component (3 c1_vio modules); not duplicated into c2_vpr or any other component. c2_vpr `_assert_engine_output_dim` consolidation kept the helper c2-internal (underscore-prefixed module, not exported). Component boundaries preserved.
- **FDR record schema**: no schema-level additions in this window. The 3 c1_vio strategies emit the same `vio.health` record kind; the 5 c2_vpr secondary backbones (from B50-51) continue to emit `vpr.embedding_complete` / `vpr.retrieval_complete` / `vpr.query_failed`. Verified by `tests/unit/test_az272_fdr_record_schema.py` (passes unmodified).
- **`KltRansacConfig` schema extension**: added to `c1_vio/config.py` + exported via `c1_vio/__init__.py`; `KNOWN_STRATEGIES` already had `klt_ransac` from AZ-331. Wiring is symmetric with `Okvis2Config` + `VinsMonoConfig` — verified by `test_az269_config_loader.py` (passes).
- **AZ-527 closure** of cumulative-49-51 F1: verified — zero `_assert_engine_output_dim` definitions remain anywhere under `src/gps_denied_onboard/components/c2_vpr/` except in `_engine_dim_assertion.py`.

### Phase 7 — Architecture Compliance

1. **Layer direction**: all changed files in this window import only from L1 substrate (`_types`, `helpers/*`, `config`, `logging`, `fdr_client`, `clock`, `errors`) or intra-component siblings. Spot-checked all 3 c1_vio strategies + the new c2_vpr helper + the 7 modified c2_vpr strategies. No upward imports detected.
2. **Public API respect**: AZ-527's helper is underscore-prefixed (`_engine_dim_assertion.py`) and NOT exported from `c2_vpr/__init__.py` — confirmed. `KltRansacStrategy` and `VinsMonoStrategy` are deliberately NOT exported from `c1_vio/__init__.py` (lazy import only via `runtime_root.vio_factory`) — matches the OKVIS2 precedent and the Risk-2/Risk-3 mitigation pattern. `KltRansacConfig` + `VinsMonoConfig` ARE exported (correct — they participate in the AZ-269 config-loader's structural typing). Verified by `tests/unit/test_az270_compose_root.py::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. All three c1_vio strategies + the new c2_vpr helper are leaves in the import graph.
4. **Duplicate symbols across components**: F1 above is **intra-component** (scoped to `c1_vio/*.py`). No cross-component duplication detected. The c2_vpr helper consolidation explicitly stayed c2-internal — no new cross-component shared symbol introduced.
5. **Cross-cutting concerns not locally re-implemented**: AZ-526 + AZ-527 + AZ-508 have moved the genuinely cross-cutting helpers (`iso_ts_now`, `iso_ts_from_clock`) into `helpers/iso_timestamps`. The remaining open intra-component pattern (`c1_vio` orchestration spine, `c2_vpr` engine-dim assertion) is consciously kept component-internal — engine output-shape contracts and strategy state machines are correctly component-scoped concerns, not shared/* concerns. F1's AZ-528 keeps the spine inside `c1_vio/`, not in `shared/helpers`.
6. **Native binding location**: VINS-Mono added `src/gps_denied_onboard/components/c1_vio/_native/vins_mono_binding.cpp` next to the facade and `cpp/vins_mono/CMakeLists.txt` for the source build — matches `module-layout.md` rule #4. KLT/RANSAC is pure Python (no `_native/` directory by design) — matches AZ-334's architectural decision. Both gate paths (`BUILD_VINS_MONO`, `BUILD_KLT_RANSAC`) wired correctly at the AZ-331 factory.

**No Critical, no High Architecture findings in this window.**

## Verdict Justification

- Critical findings: 0
- High findings:     0
- Medium findings:   1 (F1 — already addressed via AZ-528 ticket creation)
- Low findings:      1 (F2 — deferred to later hygiene pass; informational carry-forward)

**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-528) ready to be sequenced into a future batch; F2 is informational and does not block.

## Recommended Follow-up

1. **Sequence AZ-528 before any future c1_vio strategy or VIO-facing change** (none planned in current scope, but defensively before any AZ-358 / AZ-335 / hypothetical secondary VIO). 3 points, 1 new module + 3 small edits + 1 test.
2. **F2 carry-forward**: revisit when the next c1_vio touch lands (e.g. AZ-335 warm-start hint persistence). Sized at 1-2 points. Do NOT bundle into AZ-528 — different abstraction layer.
3. **Cumulative-49-51 F2 carry-forward** (AC-10 spec wording drift across AZ-337/338/339/340 specs): still open; documentation-only; fold into any future c2_vpr spec touch.
4. **No other carry-over.** All previous cumulative findings are now either CLOSED (B49-51 F1 / F3 closed by AZ-526; B49-51 F1 closed by AZ-527 / B52), tracked (AZ-528 for B52-54 F1), or informational (B49-51 F2 + B52-54 F2).

## Next Cumulative Review

- **Trigger**: after Batch 57 (every K=3).
- **Range**: batches 55-57.
