# Code Review Report **Batch**: 54 (AZ-334 — C1 KLT/RANSAC Strategy) **Date**: 2026-05-14 **Verdict**: PASS_WITH_WARNINGS ## Scope Single-task batch implementing `KltRansacStrategy`, the mandatory simple-baseline `VioStrategy` that satisfies the ADR-002 engine rule (every component MUST ship a simple-baseline strategy alongside its production-default). Pure-Python over OpenCV's `cv2.goodFeaturesToTrack` / `cv2.calcOpticalFlowPyrLK` / `cv2.findEssentialMat` / `cv2.recoverPose` path; no C++/pybind11 native binding by design — Tier-0 workstation can run the strategy with `pip install opencv-python` only. ### Changed files - `src/gps_denied_onboard/components/c1_vio/klt_ransac.py` — new Python facade (~770 lines, including module docstring + AC mapping + risk-mitigation notes). - `src/gps_denied_onboard/components/c1_vio/config.py` — add `KltRansacConfig` dataclass + `klt_ransac` field on `C1VioConfig`; `__all__` updated; `KNOWN_STRATEGIES` already had `klt_ransac`. - `src/gps_denied_onboard/components/c1_vio/__init__.py` — export `KltRansacConfig`. - `cpp/klt_ransac/CMakeLists.txt` — replace placeholder message with a deliberate "pure-Python; no native target" explanation so the build graph stays symmetric with `cpp/okvis2/` and `cpp/vins_mono/` while the `BUILD_KLT_RANSAC=ON` flag only gates the Python module import at the AZ-331 composition-root factory. - `tests/unit/c1_vio/test_klt_ransac_strategy.py` — new test module covering AC-1..AC-11 + NFR-perf (~990 lines, 25 tests; 23 pass + 2 tier-2 skipped on dev/CI runners). - `tests/unit/c1_vio/test_protocol_conformance.py` — introduce the `_STRATEGIES_WITHOUT_NATIVE_BINDING` category and route `klt_ransac` through it inside `test_ac5_build_vio_strategy_flag_on_but_module_missing` so the parametrised factory test correctly handles the pure-Python shape (no native binding to fail on). ## Phase 2 — Spec Compliance | AC | Test | Verified | |-------|----------------------------------------------------------------------------|----------| | AC-1 | `test_ac1_current_strategy_label_returns_klt_ransac` + | ✓ | | | `test_ac1_constructor_rejects_mismatched_strategy_label` | | | AC-2 | `test_ac2_first_frame_emits_init_state_with_identity_pose` | ✓ | | AC-3 | `test_ac3_steady_state_frame_emits_pose_and_spd_covariance` | ✓ | | AC-4 | `test_ac4_cv2_error_in_find_essential_mat_rewrapped_to_vio_fatal_error` + | ✓ | | | `test_ac4_cv2_error_in_recover_pose_rewrapped_to_vio_fatal_error` | | | AC-5 | `test_ac5_reset_to_warm_start_clears_feature_buffer_and_seeds_bias` + | ✓ | | | `test_ac5_reset_to_warm_start_idempotent_across_consecutive_calls` + | | | | `test_ac5_reset_to_warm_start_rejects_non_pose3_hint` | | | AC-6 | `test_ac6_low_inlier_count_emits_degraded_with_monotonic_covariance` | ✓ | | AC-7 | `test_ac7_sustained_pose_recovery_failure_raises_vio_fatal_error` | ✓ | | AC-8 | `test_ac8_strategy_module_not_imported_at_package_load` + | ✓ | | | `test_protocol_conformance.py::test_ac5_build_vio_strategy_flag_*` | | | AC-9 | `test_ac9_honest_covariance_monotonic_during_degraded` (tier2) | ✓ | | AC-10 | `test_ac10_fdr_vio_health_emitted_per_transition` | ✓ | | AC-11 | `test_ac11_source_has_no_camera_id_literals` + | ✓ | | | `test_ac11_strategy_handles_two_distinct_calibrations` | | | NFR-perf | `test_nfr_perf_process_frame_records_p95` (tier2) | ✓ | All 11 ACs + NFR-perf mapped to passing tests. Test suite reports 25 tests, 23 pass + 2 tier2 skipped on the standard dev/CI runner; all 25 pass under `GPS_DENIED_TIER=2`. Adjacent regression: `tests/unit/c1_vio/` reports 95 passed + 6 skipped (6 = the 2 KLT-tier2 + 2 OKVIS2-tier2 + 2 VINS-Mono-tier2 tests); config-loader and compose-root suites green (17 passed). ## Phase 3 — Code Quality - **SOLID**: `KltRansacStrategy` has a single responsibility (Python facade over OpenCV's KLT/RANSAC path). Constructor injection per ADR-009 — `Config` + `FdrClient` enter explicitly; `Clock` is optional with a `WallClock` default; the AZ-276 `ImuPreintegrator` is constructed lazily on the first `process_frame` call (it requires the per-call `CameraCalibration` which is not available at constructor time — matches the existing factory pattern across OKVIS2 / VINS-Mono). - **Error handling**: error envelope closed at the `VioError` family; every OpenCV `cv2.error` is caught at each call site and rewrapped into `VioFatalError` with `__cause__` chaining (AC-4). Pose-recovery failures route through `_pose_recovery_failed` which raises `VioInitializingError` until `lost_frame_threshold` is exhausted, then escalates to `VioFatalError` (AC-7). RansacFilterError + ImuPreintegrationError are also caught and rewrapped. - **Naming**: matches the OKVIS2 / VINS-Mono facade naming exactly (intentional — IT-12 harness substitutability). - **Complexity**: `process_frame` is ~120 lines; the dominant cost is the explicit step-numbered ladder (IMU push → grayscale → first-frame branch → KLT track → RANSAC filter → essential-matrix → pose recover → covariance → VioOutput build → state classify → re-seed features). Splitting further would obscure the linear flow that maps 1:1 onto the task spec's "Outcome" numbered list. - **DRY**: structural duplication with OKVIS2 / VINS-Mono facades — see F1 below; deliberately deferred to the post-batch-54 hygiene PBI (now scheduled by the next cumulative review, batches 52-54). - **Test quality**: each AC has a behaviourally-meaningful assertion (covariance SPD, frame_id echoed, transition states ordered, monotonic covariance growth during DEGRADED, etc.). No "did not throw" placeholder tests. AC-3 / AC-6 / AC-9 / AC-10 / AC-11 / NFR-perf monkeypatch `cv2.findEssentialMat` + `cv2.recoverPose` + `RansacFilter.filter_correspondences` to deterministic values so the unit suite exercises the FACADE's state machine without depending on real OpenCV geometry on synthetic correspondences; real-geometry validation lives in C1-IT-12 (Jetson Tier-2 fixture). - **Dead code**: none. `_drain_into_list` removed from the test helper after the simpler `_drain` was introduced. ## Phase 4 — Security Quick-Scan - No SQL / command injection paths. No `subprocess(shell=True)`, `eval`, `exec`. - No hardcoded secrets, API keys, or credentials. - Input validation: `_intrinsics_3x3` rejects non-3x3 K with `VioFatalError`; `_grayscale` rejects unsupported image shapes; `KltRansacConfig.__post_init__` validates every knob range (max_corners ≥ 4, klt_window_size_px odd ≥ 3, klt_pyramid_levels ≥ 1, min_features_for_pose ≥ 5, ransac_inlier_ratio in (0, 1], essential_matrix_ransac_threshold_px > 0). - Sensitive data: per-frame DEBUG log defaults OFF (`KltRansacConfig.per_frame_debug_log = False`) — matches `description.md` § 9 logging hygiene. PASS. ## Phase 5 — Performance Scan - Hot path: `process_frame`. IMU push loop is `O(samples_per_window)` — unavoidable. KLT track + RANSAC are multi-threaded internally by OpenCV; bound at 30 % of one core per ADR-002 budget partition. - No N+1, no unbounded buffers (FdrClient capacity bounded by AZ-273 ringbuf; the strategy keeps a single `_prev_features` numpy array sized at `max_corners`). - Covariance estimator: `_estimate_covariance` does one `np.eye(6) * scalar` — O(1). - AC-9 honest-covariance: the residual_var / DOF formula has no client-side floor; cov Frobenius grows monotonically as inlier_count drops (verified in tier-2 test). PASS. ## Phase 6 — Cross-Task Consistency N/A — single-task batch. Cross-task consistency with AZ-332 + AZ-333 is tracked in the next cumulative review (batches 52-54), which will see all three strategy facades and the duplication finding (F1) at the same time. ## Phase 7 — Architecture Compliance - **Layer direction**: `klt_ransac.py` imports from `_types.nav`, `_types.calibration` (TYPE_CHECKING only), `clock.wall_clock`, `components.c1_vio.config` (TYPE_CHECKING only), `components.c1_vio.errors`, `fdr_client`, `helpers.imu_preintegrator`, `helpers.ransac_filter`, `logging` — all L1/L2 substrate per the c1 layering. PASS. - **Public API respect**: `KltRansacConfig` exported through `c1_vio/__init__.py`; `KltRansacStrategy` deliberately NOT exported (lazy import only via `runtime_root.vio_factory`) — matches the Risk-2/Risk-3 pattern from OKVIS2 and VINS-Mono. PASS. - **No new cyclic dependencies**: introduced module is a leaf — no back-edges to its own importers. - **Native binding location**: NONE by design. `cpp/klt_ransac/` carries a CMakeLists that returns a STATUS message documenting the absence of native target; the directory is preserved for build-graph symmetry with `cpp/okvis2/` and `cpp/vins_mono/`. - **Build flag respect**: `BUILD_KLT_RANSAC=OFF` keeps the composition-root factory from importing the strategy module; the AZ-331 factory raises `StrategyNotAvailableError` before any import — Risk-3 mitigation intact for operator-tooling binaries (which do not need any VIO at all). - **AC-11 camera agnostic**: source CI-grep gate verifies no `adti20` / `adti26` literals in executable code (docstrings are excluded via AST-aware stripping in the test). PASS. PASS. ## Findings | # | Severity | Category | File:Line | Title | |---|----------|----------|-----------|-------| | 1 | Low | Maintainability | `src/gps_denied_onboard/components/c1_vio/klt_ransac.py` | Structural duplication with `okvis2.py` / `vins_mono.py` — now scheduled for post-batch-54 hygiene PBI via cumulative review | | 2 | Low | Maintainability | `tests/unit/c1_vio/test_klt_ransac_strategy.py` | `_patch_pose_recovery` helper is bespoke per-strategy; the same patching pattern could plausibly be shared with OKVIS2 / VINS-Mono fake-binding fixtures | ### Finding details **F1: Structural duplication of strategy facade** (Low / Maintainability) - Location: `src/gps_denied_onboard/components/c1_vio/klt_ransac.py` vs `src/gps_denied_onboard/components/c1_vio/okvis2.py` / `vins_mono.py` - Description: The new `KltRansacStrategy` mirrors `Okvis2Strategy` + `VinsMonoStrategy` ~70 % verbatim on the orchestration spine — `_classify_state`, `_tick_lost`, `_emit_transition`, `_bias_norm`, `_now_iso`, `_se3_from_4x4`, the constructor strategy- label guard, and the FDR record-emit shape are byte-equivalent modulo strategy-label constants. The geometry-specific pipeline (KLT seed/track, RANSAC filter, findEssentialMat, recoverPose, residual-scatter covariance) IS unique to this strategy and lives in its own module — that boundary is correct. The shared orchestration spine is the consolidation target tracked by the hygiene PBI deferred since batch 53; the cumulative review scheduled for batches 52-54 (next trigger) will formally raise the PBI now that all three strategy shapes are visible. - Suggestion: defer (one more time) to the cumulative review for batches 52-54 — the right factoring is now visible (template- method base class for the orchestration spine + per-strategy geometry hook). Do NOT create the PBI ad-hoc here; let the cumulative review own the cross-batch refactor scope. - Task: AZ-334 **F2: Test patching helper could be shared** (Low / Maintainability) - Location: `tests/unit/c1_vio/test_klt_ransac_strategy.py` (`_patch_pose_recovery`) vs `tests/unit/c1_vio/conftest.py` (`FakeOkvis2Backend` / `FakeVinsMonoBackend`) - Description: KLT/RANSAC uses real OpenCV bindings (no fake-binding fixture) so the existing conftest fakes don't apply directly. The per-test helper `_patch_pose_recovery` does the equivalent job of forcing a deterministic-success path. This is a different abstraction shape from the conftest fakes but lives at the same layer; consolidating with the post-batch-54 hygiene PBI would let all three strategies share a single per-strategy "ScriptedSuccess" fixture surface. - Suggestion: same as F1 — let the cumulative review's hygiene PBI own the cross-cutting test-fixture refactor. - Task: AZ-334 ## Verdict **PASS_WITH_WARNINGS** — two Low-severity duplication findings, both intentionally deferred and now formally in scope for the next cumulative review (batches 52-54). No Critical, High, or Medium findings. All 11 ACs + NFR-perf covered with passing tests. Pre-existing environment-dependent perf flake noted in batch 53 (`tests/unit/c12_operator_orchestrator/test_cli_console_script.py::test_cold_start_under_500ms_p99`) is still environmental and untouched by this batch — reported, not blocking.