diff --git a/_docs/03_implementation/cumulative_review_batches_52-54_cycle1_report.md b/_docs/03_implementation/cumulative_review_batches_52-54_cycle1_report.md new file mode 100644 index 0000000..ea24116 --- /dev/null +++ b/_docs/03_implementation/cumulative_review_batches_52-54_cycle1_report.md @@ -0,0 +1,147 @@ +# 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: . + +- **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. diff --git a/_docs/_autodev_state.md b/_docs/_autodev_state.md index 62ee045..83961d1 100644 --- a/_docs/_autodev_state.md +++ b/_docs/_autodev_state.md @@ -13,5 +13,5 @@ retry_count: 0 cycle: 1 tracker: jira last_completed_batch: 54 -last_cumulative_review: batches_49-51 +last_cumulative_review: batches_52-54 current_batch: 55