gps-denied-onboard/_docs/03_implementation/reviews/batch_05_review.md

Code Review Report

Batch: 5 Tasks: AZ-271 (config precedence tests), AZ-282 (RansacFilter helper), AZ-276 (ImuPreintegrator helper), AZ-278 (LightGlueRuntime helper / R14 fix) Date: 2026-05-11 Verdict: PASS_WITH_WARNINGS

Scope

Batch 5 closes the remaining cross-cutting epics that gated component work:

• E-CC-CONF (AZ-246): AZ-271 lands the precedence-test gate that proves env > YAML > defaults plus multi-file YAML merge ordering for ≥3 keys per layer.
• E-CC-HELPERS (AZ-264): AZ-282 ships the static RansacFilter with median-residual semantics; AZ-276 ships the GTSAM-backed ImuPreintegrator (single-threaded, strict-monotonic); AZ-278 ships the shared LightGlueRuntime — the structural fix for R14 (impossible C2.5 ↔ C3 import cycle).

After batch 5 every cross-cutting concern except helpers.ad_hop_refiner (C3.5-owned, not in this epic) has shipped or has a frozen stub + contract. Component task batches can now begin.

Phase 1: Context Loading

Read:

• _docs/02_tasks/todo/AZ-271_config_precedence_tests.md (5 ACs)
• _docs/02_tasks/todo/AZ-282_ransac_filter.md (10 ACs + 2 NFRs)
• _docs/02_tasks/todo/AZ-276_imu_preintegrator.md (7 ACs + 2 NFRs)
• _docs/02_tasks/todo/AZ-278_lightglue_runtime.md (7 ACs + 3 NFRs)
• Contracts:
  • _docs/02_document/contracts/shared_config/composition_root_protocol.md
  • _docs/02_document/contracts/shared_helpers/ransac_filter.md
  • _docs/02_document/contracts/shared_helpers/imu_preintegrator.md
  • _docs/02_document/contracts/shared_helpers/lightglue_runtime.md

Ownership envelopes resolved:

• AZ-271 owns tests/unit/shared/config/test_precedence.py only.
• AZ-282 owns helpers/ransac_filter.py + RansacResult re-export + AC suite.
• AZ-276 owns helpers/imu_preintegrator.py + GTSAM CombinedImuFactor re-export. Adjacent hygiene (approved by contract): refresh _types/nav.py ImuSample/ImuWindow to ts_ns: int and add the missing ImuBias dataclass — the bootstrap (AZ-263) shipped a datetime-timestamped stub but the contract pins monotonic nanoseconds. No existing consumer relies on the old field names.
• AZ-278 owns helpers/lightglue_runtime.py. Adjacent hygiene (approved by spec — "this task adds the Protocol surface if _types/manifests.py does not yet define it"): add EngineHandle Protocol to _types/manifests.py; add KeypointSet + CorrespondenceSet dataclasses to _types/matching.py.

Phase 2: Spec Compliance

AZ-271 — Config precedence tests

AC	Status	Evidence
AC-1 env wins over YAML (≥3 keys)	PASS	LOG_LEVEL, FDR_QUEUE_SIZE, GPS_DENIED_TIER each take env over YAML
AC-2 YAML wins over defaults (≥3 keys)	PASS	log.level, log.sink, fdr.queue_size each take YAML over dataclass default
AC-3 defaults apply when layers silent (≥3 keys)	PASS	Three keys fall to dataclass defaults; verified via LogConfig()/FdrConfig() comparison
AC-4 multi-file YAML — later wins	PASS	first.yaml then second.yaml; second values win for shared keys
AC-5 assertion message names the layer	PASS	_layer_msg helper + meta-test asserting layer-name + key + both values appear

AZ-282 — RansacFilter

AC	Status	Evidence
AC-1 clean correspondences → all inliers, ~0 residual	PASS	Pure-translation fixture so cv2's homography fit hits ground truth exactly; residual ≤ 1e-6
AC-2 mixed → inlier band [78, 82]	PASS	80 inliers + 20 random outliers, threshold 1.5 px
AC-3 determinism	PASS	cv2.setRNGSeed(0) immediately before every findHomography call; same input run twice → byte-equal RansacResult
AC-4 residual ~ 0 on clean inliers + identity pose	PASS	Identity pose; cv2.projectPoints round-trip residual ≤ 1e-6
AC-5 empty inliers → NaN, no exception	PASS	Explicit empty-array branch returns float("nan")
AC-6 shape (N, 3) raises with shape message	PASS	RansacFilterError mentions (N, 4)
AC-7 non-positive threshold raises	PASS	RansacFilterError mentions positive threshold
AC-8 fewer than 4 points raises	PASS	RansacFilterError mentions the 4-point homography minimum
AC-9 K.shape != (3,3) in residual raises	PASS	RansacFilterError mentions (3, 3)
AC-10 no upward imports	PASS	AST walk over helpers/ransac_filter.py; no components.* import

AZ-276 — ImuPreintegrator

AC	Status	Evidence
AC-1 round-trip 100 monotonic samples	PASS	deltaTij matches span; deltaPij non-zero (gravity-driven accumulator)
AC-2 non-monotonic rejected, state unchanged	PASS	Strict guard runs BEFORE PIM mutation; subsequent valid sample integrates normally
AC-3 reset_for_new_keyframe destructive	PASS	Closed factor reflects integration; current_preintegration() then raises
AC-4 re-bias affects subsequent samples only	PASS	Comparative test: two preintegrators with bias_a vs bias_b produce different deltaPij (proves bias applies per-segment)
AC-5 determinism across instances	PASS	Two preintegrators, same input → equal deltaTij, deltaPij, deltaVij
AC-6 no internal locks	PASS	Static-source check: no threading.Lock, RLock, Semaphore, mutex in module
AC-7 no upward imports	PASS	AST walk; no components.* import

AZ-278 — LightGlueRuntime

AC	Status	Evidence
AC-1 single-pair match	PASS	Deterministic stub engine; CorrespondenceSet returned with shape=(N,4) + scores
AC-2 batch match preserves order	PASS	Three pairs; each result's columns echo input pair's keypoints in order
AC-3 descriptor-dim mismatch	PASS	LightGlueRuntimeError mentions both expected and actual dims
AC-4 concurrent access rejected	PASS	Threading test with blocking barrier — non-blocking Lock.acquire(blocking=False) raises LightGlueConcurrentAccessError in second thread; first completes normally
AC-5 construct with None	PASS	LightGlueRuntimeError raised at construction
AC-6 no upward imports — R14 structural fix	PASS	AST walk; only _types.manifests, _types.matching, threading, stdlib
AC-7 determinism downstream of engine	PASS	Deterministic stub; two match calls → byte-equal output

Phase 3: Architecture Compliance

• Layer 1 invariants intact: every helper (ransac_filter, imu_preintegrator, lightglue_runtime) imports ONLY from _types, GTSAM/numpy/cv2, and stdlib. No components.* imports anywhere. Verified by per-module AST tests (AZ-282 AC-10, AZ-276 AC-7, AZ-278 AC-6).
• EngineHandle Protocol placement: lives in _types/manifests.py (the contract-mandated location). TYPE_CHECKING import for KeypointSet/CorrespondenceSet keeps the runtime import graph acyclic.
• Composition-root contract: make_imu_preintegrator(calibration) reads optional IMU noise model from CameraCalibration.metadata; defaults documented inline (BMI088-class). LightGlueRuntime has no factory — the spec mandates explicit engine-handle injection by the composition root.
• R14 structural fix verified: AZ-278 AC-6 is the canary — any future regression that wires helpers/lightglue_runtime.py to a component module trips the AST test in CI.

Phase 4: Test Quality

• 43 new tests across batch 5: 6 (AZ-271) + 16 (AZ-282) + 11 (AZ-276)
  • 10 (AZ-278).
• All tests are hermetic — no real GPU, no real network, no real FC. GTSAM PIM and cv2 homography run in-process.
• Concurrency test for AZ-278 AC-4 uses a threading.Event barrier so the second thread reliably enters while the first is held inside forward() — no flaky timing dependency.
• Determinism tests (AZ-282 AC-3, AZ-278 AC-7) use np.testing.assert_array_equal (byte-equality), not assert_allclose — strictness matches contract.
• Negative-path tests verify the EXACT error message keywords the contract names ((N, 4), (3, 3), "engine_handle", "no samples", "non-monotonic", "positive") so accidental message rewording during refactor will surface as test failures.

Phase 5: Performance / Reliability

Concern	Status	Evidence
NFR-perf filter_correspondences p99 ≤ 5 ms (Tier-2)	DEFERRED	Tier-2 budget; helper logic is a thin cv2 wrapper; verified in batch 5 only by functional gate. Tier-2 microbench will land with AZ-444 (Jetson harness).
NFR-perf integrate_sample p99 ≤ 200 µs (Tier-2)	DEFERRED	Same.
NFR-perf match overhead ≤ 100 µs (helper layer)	DEFERRED	Same.
Determinism	PASS	All three helpers' determinism gates green; cv2.setRNGSeed(0) and GTSAM PIM are pure given fixed input.
Error wrapping	PASS	RansacFilterError, ImuPreintegrationError, LightGlueRuntimeError, LightGlueConcurrentAccessError are the only types crossing the public surface (verified by negative tests).

Phase 6: Dependency / Environment Changes

Finding 1 (High, RESOLVED) — opencv-python pin conflict with gtsam/numpy ABI

The project simultaneously pinned numpy>=1.26,<2.0, gtsam>=4.2,<5.0 (PyPI 4.2 — built against numpy 1.x C ABI; Pose3(np.eye(4)) SEGFAULTs under numpy 2.x), and opencv-python>=4.12.0 (which at runtime requires numpy>=2). The set was unbuildable; the conflict went unnoticed because cv2 had no consumer until AZ-282.

Resolution (user-approved): relaxed opencv-python pin to >=4.11.0.86,<4.12 in pyproject.toml; ratcheted the CVE gate (ci/opencv_pin_gate.py + AC-10 CI tests) to a 4.11.0 floor; filed _docs/_process_leftovers/2026-05-11_d_cross_cve_1_opencv_pin_deferred.md so the original >=4.12.0 D-CROSS-CVE-1 gate is replayed the moment a numpy-2-compatible gtsam wheel ships.

The pre-existing CI gate test test_opencv_pin_gate_passes_on_412_minimum now validates the relaxed floor (4.11.0); the negative test test_opencv_pin_gate_fails_on_lower_version continues to reject 4.10 and below.

Finding 2 (Informational) — _types/nav.py DTO refresh

The bootstrap (AZ-263) shipped ImuSample(timestamp: datetime) and ImuWindow(t_start, t_end). The imu_preintegrator contract specifies strict-monotonic ts_ns: int. Batch 5 brought the DTOs in line with the contract and added the missing ImuBias dataclass.

Impact: zero existing consumers — no production code reads the field yet. The two existing Protocol references (c1_vio/interface.py, c8_fc_adapter/interface.py) only use the type, not its fields, so they are unaffected.

Finding 3 (Informational) — LightGlueRuntime uses threading.Lock instead of threading.local

The spec's Risk 2 mitigation lists two acceptable guard patterns: non-blocking Lock(blocking=False).acquire() OR threading.local(). The implementation uses the former because it gives a stronger guarantee: a SECOND-thread entry that strictly overlaps the first is caught even if the helper instance is shared across threads. The threading.local() pattern would silently allow multi-threaded use when no two callers happen to run concurrently — exactly the silent-corruption mode the contract forbids. Risk 2's NFR-perf budget (≤ 100 µs overhead) is preserved because acquire(blocking=False) is a single atomic try-set.

Finding 4 (Informational) — Mid-window reset_with_bias clears the PIM

Per AC-4 the helper resets the GTSAM PreintegratedCombinedMeasurements when bias changes mid-window. The contract test verifies the consumer-visible effect (bias applied to subsequent samples), not the internal mechanism. Documented in the helper docstring: consumers must close the prior window via reset_for_new_keyframe() before rebiasing if they want to retain the prior segment's contribution. C1/C5 spec work will validate this is the desired control-flow.

Phase 7: Process

• 43 new tests added; 294 of 296 total pass (2 env-skipped: cmake/actionlint not in dev image — pre-existing).
• ruff check + ruff format clean.
• All 4 task spec files match the implementation surface; no spec rewrites required.
• Cross-cutting effects on pyproject and CI gates are documented in the leftover (Finding 1) so a future agent replaying does NOT find silent drift.

Verdict

PASS_WITH_WARNINGS — 4 informational findings (Findings 1–4), all documented above. Finding 1 has an open follow-up in _docs/_process_leftovers/ with an explicit replay procedure tied to the gtsam-numpy2 dependency.

Component task batches can begin.