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