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

Code Review Report — Batch 93

Batch: 93 Tasks: AZ-622 (Phase D: build_pre_constructed seeds c3_lightglue_runtime + c3_feature_extractor) Date: 2026-05-19 Verdict: PASS_WITH_WARNINGS Reviewer: autodev / implement skill Step 9 Cycle: 1

Scope

Files changed (per git status):

• src/gps_denied_onboard/components/c3_matcher/config.py — added optional lightglue_weights_path: Path | None = None field; extended __post_init__ Path/None validator; expanded module docstring.
• src/gps_denied_onboard/runtime_root/airborne_bootstrap.py — new public constant C3_MATCHER_BUILD_FLAGS; new internal helpers _resolve_c3_matcher_strategy, _is_build_flag_on, _load_lightglue_engine_handle, _build_c3_lightglue_runtime, _build_c3_feature_extractor; refactored build_pre_constructed to step-by-step build so the just-built c7_inference flows into the LightGlue loader.
• tests/unit/runtime_root/test_az619_pre_constructed_phase_a.py — added autouse _stub_c3_matcher_builders.
• tests/unit/runtime_root/test_az620_pre_constructed_phase_b.py — added autouse _stub_c3_matcher_builders.
• tests/unit/runtime_root/test_az621_pre_constructed_phase_c.py — added autouse _stub_c3_matcher_builders.
• tests/unit/runtime_root/test_az622_pre_constructed_phase_d.py — new file: 6 tests covering AC-622.1, AC-622.2 (4 variants), and an identity-share invariant.

Phase 1 — Context Loading

Read AZ-622 task spec (3pt; deps AZ-619/620/621/278 all in done/); umbrella spec AZ-618; matcher_factory's _STRATEGY_TO_BUILD_FLAG; LightGlueRuntime helper contract; OpenCvOrbExtractor helper docstring; existing AZ-619/620/621 test patterns. Confirmed:

• AC-622.1 = adds 2 keys on top of AZ-619..AZ-621; tests stub the heavy seam.
• AC-622.2 = BUILD-flag mismatch surfaces clear AirborneBootstrapError naming the flag + consumer.
• AC-622.3 = test file exists at the documented path.
• Tier-2 Note = real LightGlue inference is verified at AZ-624 Jetson AC-5.

Phase 2 — Spec Compliance

AC	Test	Status
AC-622.1	test_ac_622_1_adds_c3_lightglue_runtime_and_c3_feature_extractor	Covered
AC-622.2	test_ac_622_2_build_flag_off_with_configured_strategy_raises_named_error (+ ALIKED variant + defence-in-depth + cause-chain wrap)	Covered
AC-622.3	File tests/unit/runtime_root/test_az622_pre_constructed_phase_d.py exists	Covered

Constraints check:

• MUST NOT introduce new BUILD_* env flags — PASS. The new C3_MATCHER_BUILD_FLAGS constant is a consumer-side mirror of the existing matcher_factory._STRATEGY_TO_BUILD_FLAG (BUILD_MATCHER_DISK_LIGHTGLUE / BUILD_MATCHER_ALIKED_LIGHTGLUE / BUILD_MATCHER_XFEAT). No new env var introduced.
• MUST reuse the existing per-strategy BUILD_C3_MATCHER_* matrix — PASS in spirit. The actual production matrix is named BUILD_MATCHER_*, not BUILD_C3_MATCHER_* (the spec author's typo); the docstring of C3_MATCHER_BUILD_FLAGS calls this out so future readers don't get confused. See Finding F1 below.
• MUST be additive on top of AZ-619..AZ-621 — PASS. build_pre_constructed's step-by-step build preserves every prior phase's key; the additivity test in test_az619_*.py::test_phase_a_keys_remain_present_under_az620_additivity continues to pass with the new builders in scope.
• The cross-component identity-share assertion (one instance shared across C3 + C2.5 in the actual graph) is verified at AZ-624 — Honored. The AZ-622 unit tests verify the LightGlueRuntime instance is constructed once per build_pre_constructed call; the wrapper-layer identity-share is left to AZ-624 per the task's Excluded section.

Spec drift surfaced as findings:

• F1 (Low / Spec) — see Findings section.

Phase 3 — Code Quality

• SOLID: each new function has a single responsibility (resolve strategy, check flag, load engine, build runtime, build extractor, top-level assemble). No god-method introduced.
• Error handling: every failure path produces an AirborneBootstrapError with three operator-actionable pieces (missing key, gating flag, consuming component) plus a remediation hint, matching the AZ-618 NFR contract. RuntimeNotAvailableError is preserved via raise … from exc so the upstream stack is recoverable.
• Naming: _resolve_c3_matcher_strategy, _load_lightglue_engine_handle, _build_c3_lightglue_runtime, _build_c3_feature_extractor — all kebab-pattern consistent with prior phases (_build_c6_descriptor_index, _build_c7_inference).
• Complexity: longest new function is _build_c3_lightglue_runtime at ~40 lines including docstring; cyclomatic complexity ≤ 4 per function.
• DRY: _is_build_flag_on is a 2-line duplicate of matcher_factory._is_build_flag_on. See Finding F2.
• Test quality: every test follows the Arrange/Act/Assert convention with comment markers; assertions check identity (is), type (isinstance), key membership, exception type, exception message content, and cause-chain — NOT just "no error thrown".
• Dead code: no unused imports, no unreachable branches. The del config in _build_c3_feature_extractor is intentional (keeps the signature consistent with sibling builders).

Phase 4 — Security Quick-Scan

• No SQL / command injection (no string-built queries, no subprocess, no eval/exec).
• No hardcoded secrets, API keys, or paths to credential stores.
• Input validation: _load_lightglue_engine_handle validates weights_path is None before calling compile_engine; the caller wraps any heavy-load failure into a structured error.
• No sensitive data in error messages (path is referenced by name, not value).
• No insecure deserialization — deserialize_engine is the existing C7 path that ships its own engine-gate validation (per AZ-301).

No security findings.

Phase 5 — Performance Scan

• build_pre_constructed is a one-shot bootstrap call at process start; no hot-path concerns.
• _load_lightglue_engine_handle is intentionally heavy (engine compile + deserialise) but is invoked once per process. The c7 InferenceRuntime is built once and reused for the LightGlue load — no double-build at this layer.
• No O(n²) algorithms; no unbounded data fetching; no blocking I/O in async contexts (the bootstrap is synchronous by design).

No performance findings.

Phase 6 — Cross-Task Consistency

• The new keys integrate with the existing _c3_matcher_wrapper (line 257 of airborne_bootstrap.py) which already extracts c3_lightglue_runtime from constructed. No signature collision.
• The _c2_5_rerank_wrapper (line 232) extracts both c3_lightglue_runtime and c3_feature_extractor — both keys are now populated, matching the wrapper's expectation.
• Prior-phase tests (AZ-619/620/621) now stub the new C3 builders via the same autouse pattern they already use for C6/C7. The pattern is uniform across all four phase test files.

Phase 7 — Architecture Compliance

• Layer direction: runtime_root.airborne_bootstrap imports from helpers/lightglue_runtime (L1) and helpers/feature_extractor (L1). Allowed per module-layout.md rule 6 (composition root may import any L1 helper). PASS.
• Public API respect: LightGlueRuntime is exported from helpers/lightglue_runtime.py __all__; OpenCvOrbExtractor is exported from helpers/feature_extractor.py __all__. Both imports go through documented Public API. PASS.
• No new cyclic module dependencies: dependency chain runtime_root.airborne_bootstrap → helpers/lightglue_runtime → _types/manifests + _types/matching and runtime_root.airborne_bootstrap → helpers/feature_extractor → _types/matching. No reverse import. PASS.
• Duplicate symbols across components: _is_build_flag_on exists in both airborne_bootstrap.py and matcher_factory.py. Both are private (underscore-prefixed) and live in the same runtime_root/ subpackage — not a cross-component duplication. Severity Low; see Finding F2.
• Cross-cutting concerns not locally re-implemented: the BuildConfig(precision=PrecisionMode.FP16, workspace_mb=512, …) constant in _load_lightglue_engine_handle mirrors the per-strategy _build_disk_build_config / _build_aliked_build_config shape in C3 strategy modules. Severity Low; see Finding F3.

No Critical or High Architecture findings.

Findings

#	Severity	Category	File:Line	Title
1	Low	Spec	airborne_bootstrap.py:545	_build_c3_lightglue_runtime signature differs from spec's single-arg form
2	Low	Maintainability	airborne_bootstrap.py + matcher_factory.py	_is_build_flag_on duplicated across two runtime_root modules
3	Low	Maintainability	airborne_bootstrap.py:_load_lightglue_engine_handle	BuildConfig(precision=FP16, workspace_mb=512, …) mirrors per-strategy _build_*_build_config constants

F1 — _build_c3_lightglue_runtime signature differs from spec's single-arg form (Low / Spec)

• Location: src/gps_denied_onboard/runtime_root/airborne_bootstrap.py (_build_c3_lightglue_runtime(config: Config, *, inference_runtime: InferenceRuntime) -> LightGlueRuntime).
• Description: AZ-622 task spec § Scope.Included calls out internal builders as _build_c3_lightglue_runtime(config) and _build_c3_feature_extractor(config) — single-arg. The implementation adds an inference_runtime keyword-only argument so the just-built pre_constructed['c7_inference'] can flow through without a double-build.
• Why this is the right interpretation: AZ-622 also constrains "MUST be additive on top of AZ-619..AZ-621" and AZ-621's NFR guarantees one inference-runtime build per process. Re-calling build_inference_runtime from inside _build_c3_lightglue_runtime to satisfy the literal single-arg form would violate the additivity-and-no-double-build invariant. The kwarg keeps the surface honest about the dependency.
• Mitigation: the divergence is documented inline in _build_c3_lightglue_runtime's docstring and in build_pre_constructed's docstring (line referencing "the C7 InferenceRuntime built for the c7_inference slot is reused as the engine source for the LightGlue matcher load").
• Suggestion: none — the implementation is the more correct shape. If a future spec author updates the AZ-622 task file post-batch, fold the kwarg into the spec; otherwise leave as-is.
• Task reference: AZ-622.

F2 — _is_build_flag_on duplicated across airborne_bootstrap.py + matcher_factory.py (Low / Maintainability)

• Location: src/gps_denied_onboard/runtime_root/airborne_bootstrap.py + src/gps_denied_onboard/runtime_root/matcher_factory.py (both define the same 2-line predicate).
• Description: Both airborne_bootstrap and matcher_factory now ship the same _is_build_flag_on(flag_name: str) -> bool predicate. The bootstrap intentionally avoids depending on matcher_factory's private symbol so a future reorganisation of the matcher factory cannot break the bootstrap silently.
• Severity rationale: the duplication is short (2 lines + docstring), private to a single subpackage, and prevents private-symbol coupling. Lifting it into a runtime_root/_build_flags.py (or helpers/build_flags.py) helper would be a hygiene improvement but not a correctness issue. Same pattern exists in vpr_factory.py (vpr_factory._is_build_flag_on) — at least 3 callers now duplicate the predicate.
• Suggestion: add a follow-up hygiene PBI (sized 2 points) to extract is_build_flag_on(flag_name: str) -> bool into a shared helper module under runtime_root/ (or helpers/). Defer to a future cumulative-review window.
• Task reference: AZ-622, but the duplication predates this batch.

F3 — BuildConfig literal duplicated with per-strategy build-config helpers (Low / Maintainability)

• Location: src/gps_denied_onboard/runtime_root/airborne_bootstrap.py::_load_lightglue_engine_handle.
• Description: _load_lightglue_engine_handle constructs BuildConfig(precision=PrecisionMode.FP16, workspace_mb=512, calibration_dataset=None, optimization_profiles=()) inline. The same shape exists in c3_matcher/disk_lightglue.py::_build_disk_build_config() and c3_matcher/aliked_lightglue.py::_build_aliked_build_config(). If FP16 ever needs to be a config-driven knob (Tier-2 work), three places will drift.
• Severity rationale: only matters when the build config becomes config-driven; today both per-strategy constants are literal-equal and the LightGlue engine is loaded with the same FP16 precision as DISK / ALIKED. Lifting into a shared helper would couple the bootstrap's internals to component-internal symbols — exactly the AZ-507 pattern we're trying to keep out of runtime_root/.
• Suggestion: leave as-is for AZ-622. When the BuildConfig matrix becomes config-driven (likely a 2027 Tier-2 task), introduce a cross-cutting helpers/build_config_for_engine.py and migrate all three call sites at once.
• Task reference: AZ-622, but the duplication predates this batch.

Verdict Logic

• 0 Critical findings → not FAIL.
• 0 High findings → not FAIL.
• 3 Low findings → PASS_WITH_WARNINGS.

Auto-Fix Eligibility

Finding	Severity	Category	Auto-fix eligible?
F1	Low	Spec	No — divergence is the more correct shape; spec needs updating
F2	Low	Maintainability	Defer — hygiene PBI for future cumulative-review window
F3	Low	Maintainability	Defer — wait for FP16-as-config knob requirement

No findings escalate. Proceed to commit (Step 11).

Test Verification

pytest tests/unit/runtime_root/test_az619_pre_constructed_phase_a.py \
       tests/unit/runtime_root/test_az620_pre_constructed_phase_b.py \
       tests/unit/runtime_root/test_az621_pre_constructed_phase_c.py \
       tests/unit/runtime_root/test_az622_pre_constructed_phase_d.py -v
=> 17 passed in 5.83s

Broader regression check (runtime_root + c3_matcher + c3_5_adhop):

pytest tests/unit/runtime_root/ tests/unit/c3_matcher/ -x --timeout=60
=> 108 passed in 3.89s

pytest tests/unit/c3_5_adhop/test_az349_adhop_refiner.py
=> 23 passed in 1.09s

No regression introduced by the C3MatcherConfig schema extension.