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