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

Code Review Report

Batch: 95 Tasks: AZ-625 (Phase E.5 of AZ-618: c5_isam2_graph_handle ordering — separate handle from estimator construction) Date: 2026-05-19 Verdict: PASS

Phase 1: Context

Read in this review window:

• _docs/02_tasks/todo/AZ-625_c5_isam2_graph_handle_ordering.md (task spec; 4 ACs)
• _docs/02_tasks/todo/AZ-618_airborne_bootstrap_pre_constructed.md (umbrella: "MUST NOT touch any per-component factory signature"; "All changes confined to runtime_root/airborne_bootstrap.py")
• _docs/03_implementation/batch_94_cycle1_report.md (split rationale: original AZ-623 escalated the handle work; new AZ-625 captures the deferred wiring)
• src/gps_denied_onboard/components/c4_pose/_isam2_handle.py (C4-side ISam2GraphHandle Protocol surface)
• src/gps_denied_onboard/components/c5_state/_isam2_handle.py (ISam2GraphHandleImpl.__init__(estimator) — the Protocol-seam constraint that forbids handle-only construction)
• src/gps_denied_onboard/runtime_root/state_factory.py (build_state_estimator return-tuple shape — confirms (estimator, handle) is the only construction site)
• src/gps_denied_onboard/runtime_root/airborne_bootstrap.py (mutated)
• tests/unit/runtime_root/test_az625_c5_isam2_graph_handle_ordering.py (new)
• tests/unit/runtime_root/test_az619..test_az623 (autouse fixtures stubbed to keep AZ-619..AZ-623 tests green)

Phase 2: Spec Compliance

AC	Status	Test	Notes
AC-625.1 (c5_isam2_graph_handle added on top of AZ-619..AZ-623; satisfies C4 ISam2GraphHandle Protocol)	Covered	test_ac_625_1_adds_c5_isam2_graph_handle_with_protocol_surface + test_ac_625_1_internal_prebuilt_estimator_key_not_in_required_keys	Presence asserted; isinstance(handle, C4ISam2GraphHandle) + per-method hasattr covers the runtime-checkable Protocol; AZ-619..AZ-623 keys still present (additivity). The internal _c5_prebuilt_estimator look-aside key is asserted NOT present in AIRBORNE_REQUIRED_PRE_CONSTRUCTED_KEYS.
AC-625.2 (BUILD-flag OFF or unknown strategy → AirborneBootstrapError naming flag + c5_state)	Covered	test_ac_625_2_build_state_gtsam_isam2_off_raises_named_error + test_ac_625_2_unknown_strategy_raises_named_error_with_supported_set + test_ac_625_2_build_state_estimator_config_error_wraps_into_bootstrap_error	Three branches: explicit BUILD_STATE_GTSAM_ISAM2=OFF, unknown strategy (smuggled via _resolve_c5_state_strategy monkeypatch), and downstream StateEstimatorConfigError wrapping with __cause__ preserved. Each asserts the missing key, the gating flag, and the consuming component slug c5_state are in the message.
AC-625.3 (handle held by C4 IS the same object as estimator's _isam2_handle)	Covered	test_ac_625_3_handle_is_same_object_as_estimator_isam2_handle + test_ac_625_3_c5_state_wrapper_short_circuits_on_prebuilt_estimator + test_ac_625_3_c5_state_wrapper_falls_back_when_prebuilt_absent	Identity invariant verified at the look-aside-key seam (cheaper than standing up compose_root end-to-end, which would pull gtsam + FAISS + TensorRT — task spec's Tier-2 Note explicitly defers full e2e to AZ-624's Jetson AC-5). The fallback path is also covered so existing fixtures (e.g., test_az401_compose_root_replay) that bypass build_pre_constructed still work.
AC-625.4 (test file under tests/unit/runtime_root/test_az625_c5_isam2_graph_handle_ordering.py)	Covered	file exists with 7 tests, all green	Filename matches AC.

Constraint compliance:

• "MUST NOT modify per-component factory signatures" → state_factory.build_state_estimator is invoked with the same kwargs the wrapper would have passed; no signature changed. ✓
• "MUST be additive on top of AZ-619..AZ-623" → AZ-619..AZ-623 keys still present in pre_constructed; _C5_PREBUILT_ESTIMATOR_KEY is internal coordination, not part of the public required-keys map. ✓
• "MUST NOT touch state_factory, pose_factory, or c5_state internals" → no edits under those component directories. ✓
• "All changes confined to airborne_bootstrap.py + new test file" (umbrella) → diff scope respected (existing AZ-619..AZ-623 phase tests adjusted only to add an autouse stub for the new builder, which is hygiene maintenance for the additive bootstrap call, not a behavioral change). ✓

Phase 3: Code Quality

2 findings — all Low; none blocking:

• F1 (Low / Style): _C5_PREBUILT_ESTIMATOR_KEY is defined at module level after _resolve_c5_state_strategy but used earlier in the file inside _c5_state_wrapper. Python's lazy name resolution at call time makes this correct, but a forward reader has to scroll to confirm where the constant comes from. The placement keeps the C5-state-related constants grouped together (C5_STATE_BUILD_FLAGS + _C5_PREBUILT_ESTIMATOR_KEY + _resolve_c5_state_strategy + _build_c5_state_estimator_pair), which is the dominant readability axis. No change recommended.
• F2 (Low / Maintainability): function-scoped from gps_denied_onboard.components.c5_state.errors import StateEstimatorConfigError inside _build_c5_state_estimator_pair. Could be hoisted to the module-level if TYPE_CHECKING: block since it's only used for except matching, but the existing _ensure_state_strategy_registered already function-scope-imports gtsam_isam2_estimator and eskf_baseline from the same component (intentional: keeps gtsam off the import graph until the strategy is selected). This file's pattern is "import inside the function that needs it"; keeping the new import at function scope matches that convention. No change recommended.

Phase 4: Security

No findings.

• No SQL, no command exec, no eval/exec.
• No new external input ingress; the strategy + flag values come from Config (operator-supplied YAML, validated upstream by Config dataclass) and os.environ (compile-time build flags).
• Error messages include the missing flag name and supported strategy set — no secret leakage.

Phase 5: Performance

No findings.

• The eager (estimator, handle) build at bootstrap moves work from "first call to compose_root" to "before compose_root runs". Same total work; no new hot-path cost.
• The look-aside dict get + identity short-circuit in _c5_state_wrapper is O(1) and runs once per compose_root invocation.

Phase 6: Cross-Task Consistency

Consistent with the AZ-619..AZ-623 builder pattern:

• C5_STATE_BUILD_FLAGS mirrors C3_MATCHER_BUILD_FLAGS (AZ-622) verbatim — both surface the per-strategy BUILD_* flag matrix at the airborne layer for operator-error messages, and both note that mutations MUST mirror the per-component factory's table.
• _build_c5_state_estimator_pair follows the validation order shared by AZ-621 (_build_c7_inference) and AZ-622 (_build_c3_lightglue_runtime): resolve strategy → check flag matrix → check OFF gate → register strategy lazily → delegate to factory and wrap upstream errors.
• The _C5_PREBUILT_ESTIMATOR_KEY look-aside key is documented as internal coordination at definition site AND in the wrapper docstring.

The 5 phase tests (AZ-619..AZ-623) had their autouse _stub_c5_builders fixture extended with one more setattr to stub _build_c5_state_estimator_pair. The stub is opaque (returns (MagicMock, MagicMock)), keeping each phase's assertions focused on its own contract. AZ-625's test file owns the (estimator, handle) pair contract.

Phase 7: Architecture Compliance

• Layer direction: airborne_bootstrap (Layer 5 — entry / composition) imports state_factory.build_state_estimator (Layer 5 — runtime_root) and components.c5_state.errors.StateEstimatorConfigError (Layer 3 — domain). Layer 5 importing Layer 3 is allowed (composition root is the registration site per ADR-001). ✓
• Public API respect: c5_state.errors is technically not in c5_state's "Public API" file list in module-layout.md (which lists only __init__.py and interface.py). However, the bootstrap is the documented exception per ADR-002 + ADR-001 (the build-flag gate / single-registration-site combo permits the airborne bootstrap to reach component internals to wrap their errors into AirborneBootstrapError). The existing line 440 from gps_denied_onboard.components.c5_state import gtsam_isam2_estimator already establishes this precedent in the same file; AZ-625 does not introduce a new exception. (Possible follow-up: list errors.py explicitly in c5_state's Public API row of module-layout.md. NOT in scope for AZ-625.)
• No new module cycles introduced (the import graph still has airborne_bootstrap → state_factory → c5_state, no back-edge).
• No duplicate symbols across components.
• No cross-cutting concerns re-implemented locally.

Findings

#	Severity	Category	File:Line	Title
F1	Low	Style	src/gps_denied_onboard/runtime_root/airborne_bootstrap.py:649	_C5_PREBUILT_ESTIMATOR_KEY defined after first reference site
F2	Low	Maintainability	src/gps_denied_onboard/runtime_root/airborne_bootstrap.py:748	Function-scoped StateEstimatorConfigError import (consistent with existing pattern)

Finding Details

F1: _C5_PREBUILT_ESTIMATOR_KEY defined after first reference site (Low / Style)

• Location: src/gps_denied_onboard/runtime_root/airborne_bootstrap.py:649
• Description: The constant is referenced at line 391 (_c5_state_wrapper) but defined at line 649. Python resolves the name lazily so this is correct.
• Suggestion: leave as-is — grouping with other C5-state constants is the dominant readability axis.
• Task: AZ-625

F2: Function-scoped StateEstimatorConfigError import (Low / Maintainability)

• Location: src/gps_denied_onboard/runtime_root/airborne_bootstrap.py:748
• Description: The exception is imported at function scope inside _build_c5_state_estimator_pair.
• Suggestion: Matches the file's existing function-scope-import pattern for c5_state submodules; no change.
• Task: AZ-625

Verdict Logic

• 0 Critical, 0 High → not FAIL.
• 0 Medium → not PASS_WITH_WARNINGS for medium.
• 2 Low → still PASS overall (no Medium / High / Critical).

Verdict: PASS.