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

Code Review Report

Batch: 94 Tasks: AZ-623 (Phase E narrowed: c282_ransac_filter + c5_imu_preintegrator + c5_se3_utils + c5_wgs_converter; original c5_isam2_graph_handle work split out to AZ-625) Date: 2026-05-19 Verdict: PASS_WITH_WARNINGS

Phase 1: Context

Read in this review window:

• _docs/02_tasks/todo/AZ-623_pre_constructed_phase_e_ransac_c5_helpers.md (narrowed scope, 4 helpers; c5_isam2_graph_handle deferred to AZ-625 with full scope-split note)
• _docs/02_tasks/todo/AZ-624_pre_constructed_phase_f_wire_main.md (dependency edge updated: AZ-624 now blocks on BOTH AZ-623 and AZ-625)
• _docs/02_tasks/todo/AZ-625_c5_isam2_graph_handle_ordering.md (NEW — captures the deferred handle-ordering work)
• _docs/02_tasks/todo/AZ-618_airborne_bootstrap_pre_constructed.md (umbrella; constraints "MUST NOT touch any per-component factory signature" + "All changes confined to runtime_root/airborne_bootstrap.py, runtime_root/__init__.py, and the new test file")
• src/gps_denied_onboard/helpers/{ransac_filter,imu_preintegrator,se3_utils,wgs_converter}.py (helpers being wired)
• src/gps_denied_onboard/runtime_root/_replay_branch.py (existing _load_camera_calibration pattern — template for the airborne bootstrap loader)
• src/gps_denied_onboard/components/c5_state/_isam2_handle.py (Protocol seam constraint that drove the AZ-625 split)
• src/gps_denied_onboard/runtime_root/state_factory.py (build_state_estimator return-tuple — confirms seam)
• src/gps_denied_onboard/runtime_root/__init__.py (compose_root pre_constructed merge — confirms construction-order issue)

The autodev orchestrator escalated the c5_isam2_graph_handle ordering question via the AZ-623 spec's own escalation gate; the user chose Option B (split scope; new PBI for handle wiring). This review covers the narrowed AZ-623 only.

Phase 2: Spec Compliance

AC	Status	Test	Notes
AC-623.1 (4 keys added on top of AZ-619..AZ-622)	Covered	test_ac_623_1_adds_c282_ransac_and_c5_helpers + test_ac_623_1_keeps_existing_keys_intact	Each new key is type-asserted; c5_se3_utils is the helpers.se3_utils module (matches existing C5 estimator's MagicMock fixture pattern via attribute access).
AC-623.2 (c5_imu_preintegrator cached per camera_calibration_path; stateless 3 may be fresh)	Covered	test_ac_623_2_imu_preintegrator_cached_across_calls + test_ac_623_2_imu_preintegrator_per_path_cache	Cache short-circuit verified by counter; per-path isolation verified by two distinct paths producing distinct instances.
AC-623.3 (operator-actionable error on missing/bad calibration)	Covered	test_ac_623_3_empty_calibration_path_raises_named_error + _unreadable_ + _malformed_json_	All three error paths assert the message names c5_imu_preintegrator, the missing-input symptom, and the consuming component slug c5_state. Mirrors AZ-622's c3_lightglue_runtime error-message contract.
AC-623.4 (test file exists)	Covered	tests/unit/runtime_root/test_az623_pre_constructed_phase_e.py	7 tests, all passing.

Spec drift / scope split: The user-approved scope split (handle work → AZ-625) is documented in three places — AZ-623 spec § "Scope split note", AZ-625 spec § "Why split out of AZ-623", AZ-624 Dependencies update. The Jira addCommentToJiraIssue write on AZ-623 also captures the split. No silent drift.

Phase 3: Code Quality

3 findings — all Low; none blocking:

• F1 (Low / Maintainability): _load_camera_calibration (airborne_bootstrap) duplicates _load_camera_calibration (_replay_branch.py). The bodies share the JSON shape and field defaults; they differ only in exception class (AirborneBootstrapError here vs CompositionError in replay) and the airborne-camera vs replay-camera default camera_id. The umbrella's "MUST be confined to runtime_root/airborne_bootstrap.py" constraint nominally permits the duplication, but a future hygiene PBI should consolidate the JSON-decode core into a shared helper that takes the exception class as a callable parameter.
• F2 (Low / Maintainability): empty-path check duplicated in _build_c5_imu_preintegrator AND _load_camera_calibration. Intentional defense-in-depth (the loader is a public-ish seam tests monkeypatch directly, so it must validate independently); the upper check enables a tighter cache lookup before the loader fires. Documented in both docstrings. No change recommended unless the seam is removed in a future refactor.
• F3 (Low / Style): c5_se3_utils returns a Python module (gps_denied_onboard.helpers.se3_utils) as a namespace handle. Unusual at first glance but matches the existing MagicMock() fixture pattern in tests/unit/c5_state/test_az386_eskf_baseline.py; consumers store as self._se3_utils: Any and dispatch via attribute access. The docstring spells this out and references the existing C5 test pattern. A future enhancement could introduce a Se3UtilsHandle Protocol if the inferred shape grows, but YAGNI applies here.

Phase 4: Security

No findings.

• No SQL, no command exec, no eval/exec.
• JSON parsing uses json.loads (safe) on file content.
• File paths flow from config.runtime.camera_calibration_path (operator input via YAML, not user-provided per request); Path(path).read_text(encoding="utf-8") reads the file with explicit encoding — no path traversal, no encoding ambiguity.
• No secrets in error messages: the path string is included for operator-actionability, but the file content is never echoed.

Phase 5: Performance

No findings.

• _IMU_PREINTEGRATOR_CACHE is a single-key dict lookup per build_pre_constructed call. The expensive path (JSON read + GTSAM PreintegrationCombinedParams construction) fires at most once per process per calibration path.
• The 3 stateless helpers (RansacFilter(), WgsConverter(), the se3_utils module reference) are O(1) constructions.
• Bootstrap is a startup path — no latency-budget concerns at this layer beyond AZ-618's NFR (60s on Jetson Orin Nano), which is dominated by C7 GPU model load (AZ-621), not the AZ-623 helpers.

Phase 6: Cross-Task Consistency

• Autouse stubs added to test_az619 / test_az620 / test_az621 / test_az622 with consistent pattern (named lambdas returning MagicMock(name="...") or object() sentinels). Each fixture explains why it's needed and what it stubs. No drift in stub style across the 4 prior phase test files.
• Error-message contract for AirborneBootstrapError follows the AZ-622 pattern (names the missing key, the gating input, and the consuming component slug).
• c5_se3_utils module-as-namespace pattern is documented in both the production code and the test suite.

Phase 7: Architecture Compliance

No findings.

• New imports in airborne_bootstrap.py: json (stdlib), pathlib.Path (stdlib), numpy (stack-wide), gps_denied_onboard._types.calibration.CameraCalibration (cross-cutting types — bootstrap may import any _types per AZ-507), gps_denied_onboard.helpers.{imu_preintegrator,ransac_filter,wgs_converter} (CC-HELPERS layer; bootstrap may import any helper). All within the bootstrap's allowed dependency surface per _docs/02_document/module-layout.md.
• No new cyclic dependencies: airborne_bootstrap imports from helpers + types + factories; helpers import from _types; no back-edge introduced.
• No Public API bypass: every imported symbol is in the helper module's documented __all__ or is a stdlib-equivalent re-export.
• The umbrella's "MUST NOT touch any per-component factory signature" constraint is honored: zero edits to state_factory.py, pose_factory.py, or any c5_state / c4_pose internal.
• The umbrella's "All changes confined to runtime_root/airborne_bootstrap.py, runtime_root/__init__.py, and the new test file" constraint is honored: production-side changes are only in airborne_bootstrap.py (this batch did not touch runtime_root/__init__.py — AZ-624's job).

Findings Summary

#	Severity	Category	File:Line	Title
F1	Low	Maintainability	airborne_bootstrap.py (_load_camera_calibration)	Duplicates _replay_branch._load_camera_calibration
F2	Low	Maintainability	airborne_bootstrap.py (_build_c5_imu_preintegrator + _load_camera_calibration)	Defense-in-depth duplicates the empty-path check
F3	Low	Style	airborne_bootstrap.py (_build_c5_se3_utils)	Returns a module as a namespace handle

Verdict: PASS_WITH_WARNINGS — 0 Critical, 0 High, 0 Medium, 3 Low. Auto-fix gate not triggered.

Test Run

Targeted: tests/unit/runtime_root/test_az619..623 — 24 passed in 2.78s. Regression: tests/unit/runtime_root/ tests/unit/c5_state/ — 247 passed in 1.41s.

Suggested Follow-Ups (informational)

• Hygiene PBI (~2pt): consolidate _load_camera_calibration between airborne_bootstrap and _replay_branch into a shared runtime_root/_camera_calibration_loader.py that accepts an exception-class parameter. Mirrors the F2 of batch 93 (_is_build_flag_on triple-duplication PBI). The two leftover hygiene PBIs (F2-batch93 + this F1-batch94) could land together.