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

Code Review Report

Batch: 50 — AZ-339 (C2 MegaLoc + MixVPR Secondary Backbones) Date: 2026-05-13 Verdict: PASS_WITH_WARNINGS

Findings

#	Severity	Category	File:Line	Title
1	Medium	Maintainability/Architecture	c2_vpr/mega_loc.py:438, mix_vpr.py:432, ultra_vpr.py:432, net_vlad.py:494	_assert_engine_output_dim now 4-way duplicated — schedule AZ-527
2	Low	Scope	AZ-339 task spec § AC-10	AC-10 names ConfigurationError; precedent + impl raise StrategyNotAvailableError / ConfigError

Finding Details

F1: _assert_engine_output_dim now 4-way duplicated (Medium / Maintainability + Architecture)

• Locations:
  • src/gps_denied_onboard/components/c2_vpr/ultra_vpr.py:432
  • src/gps_denied_onboard/components/c2_vpr/net_vlad.py:494
  • src/gps_denied_onboard/components/c2_vpr/mega_loc.py:438
  • src/gps_denied_onboard/components/c2_vpr/mix_vpr.py:432
• Description: Each strategy module ships a near-identical ~22-line _assert_engine_output_dim(inference_runtime, handle, preprocessor) helper. Bodies vary only on three values: _OUTPUT_KEY (always "embedding" for mega_loc / mix_vpr / ultra_vpr; "vlad_descriptor" for net_vlad), DESCRIPTOR_DIM (per-strategy constant), and preprocessor.input_shape(). Same drift signature as AZ-508 → AZ-526 (_iso_ts_now / _iso_ts_from_clock).
• The cumulative review (batches 46-48) flagged this duplication as F2 and recommended deferring "until a third VPR strategy joins (AZ-339 batch)". That trigger has fired.
• Suggestion: Create AZ-527 (Hygiene — consolidate _assert_engine_output_dim into a c2-internal helper). Signature: _assert_engine_output_dim(inference_runtime, handle, *, expected_dim, output_key, input_shape). 2 points; depends on AZ-339.
• Inline comments in the new mega_loc.py and mix_vpr.py already cite AZ-527 as the planned follow-on so the duplication is intentional, not accidental.
• Task: AZ-339 (carries forward from cumulative-46-48 F2).

F2: AC-10 names ConfigurationError but precedent / implementation raise StrategyNotAvailableError / ConfigError (Low / Scope)

• Location: _docs/02_tasks/todo/AZ-339_c2_megaloc_mixvpr.md § AC-10.
• Description: AC-10 literally reads "ConfigurationError is raised at composition-root time with message containing the missing flag; the binary refuses to start (fail-fast per AZ-336 factory's lazy-import → ImportError → ConfigurationError mapping)". The existing AZ-336 factory (build_vpr_strategy) raises StrategyNotAvailableError for the BUILD_VPR_<X>=OFF case (verified via tests/unit/c2_vpr/test_protocol_conformance.py:268-274 for UltraVPR; same pattern auto-extends to MegaLoc / MixVPR via the parametrized _STRATEGY_MODULES table). StrategyNotAvailableError is a RuntimeError subclass, NOT a ConfigError. AZ-337 / AZ-338 followed this precedent; AZ-339 does the same. The strategy module's own runtime-label guard raises ConfigError (the "wrong C7 runtime label" case), which satisfies AC-10's spirit of "composition-time fail-fast".
• Implementation choice: mirrored the existing precedent.
• Suggestion: amend AC-10 to read "StrategyNotAvailableError (for BUILD flag OFF) or ConfigError (for runtime-label mismatch) at composition-root time, with a message naming the missing flag or runtime". Recorded as drift; no code change required.
• Task: AZ-339.

Phase Summary

Phase 1 — Context Loading

Read AZ-339 task spec (208 lines, AC-1..AC-11 per strategy + NFRs), the AZ-337 UltraVPR + AZ-338 NetVLAD precedents (ultra_vpr.py, net_vlad.py, _preprocessor_ultra_vpr.py), the AZ-336 composition-root factory (vpr_factory.py), the AZ-336 C2VprConfig + KNOWN_STRATEGIES, and the cumulative_review_batches_46-48_cycle1_report.md F2 finding. Mapped 5 new files (2 strategy, 2 preprocessor, 1 test) to AZ-339.

Phase 2 — Spec Compliance

AC	Verified by	Status
AC-1 (Protocol conformance)	test_ac1_protocol_conformance[mega_loc], [mix_vpr]	PASS
AC-2 (L2-norm FP16 correct dim)	test_ac2_embed_query_returns_unit_norm_fp16_correct_dim[*], test_ac2_single_stage_l2_no_intra_cluster_call[*]	PASS
AC-3 (deterministic embeddings)	test_ac3_embed_query_deterministic_for_same_frame[*]	PASS
AC-4 (retrieve_topk k + label)	test_ac4_retrieve_topk_returns_exactly_k_with_correct_label[*]	PASS
AC-5 (descriptor_dim stable)	test_ac5_descriptor_dim_stable[*]	PASS
AC-6 (engine shape mismatch → ConfigError at create)	test_ac6_create_rejects_engine_output_shape_mismatch[*], test_ac6_create_rejects_missing_embedding_key[*]	PASS
AC-7 (VprBackboneError on forward failure)	test_ac7_runtime_error_yields_vpr_backbone_error[*], test_ac7_wrong_forward_output_shape_yields_vpr_backbone_error[*]	PASS
AC-8 (VprPreprocessError on corrupt image)	test_ac8_corrupt_image_yields_vpr_preprocess_error[*]	PASS
AC-9 (compose wiring + INFO ready log)	test_ac9_create_emits_ready_log_with_correct_label_and_dim[*]	PASS
AC-10 (build-flag exclusion fail-fast)	test_ac10_runtime_label_mismatch_raises_config_error[*] + tests/unit/c2_vpr/test_protocol_conformance.py parametrised over _STRATEGY_MODULES (auto-covers mega_loc + mix_vpr)	PASS with F2 wording drift
AC-11 (preprocessor input shape)	test_ac11_preprocessor_input_shape[*], test_preprocess_output_is_nchw_fp16[*]	PASS

36 / 36 tests in test_az339_mega_loc_mix_vpr.py pass; 25 / 25 in test_protocol_conformance.py pass (now auto-covering the two new strategies via the existing parametrised module-import table).

Phase 3 — Code Quality

• SRP: Strategy class = embed + retrieve via injected dependencies. Preprocessor class = decode + crop + resize + normalise. Each error handler is a separate helper method. Factory create() is wiring-only.
• Error handling: Every failure path emits a structured ERROR log AND an FDR record before raising. Errors are explicitly re-raised; no swallowed exceptions.
• Naming: Consistent with the UltraVPR precedent — _BACKBONE_LABEL, _OUTPUT_KEY, _LOG_KIND_*, _FDR_KIND_*, _assert_engine_output_dim. DESCRIPTOR_DIM is module-level Final per strategy (2048 / 4096), matching the AZ-337 / AZ-338 pattern.
• Complexity: Strategy class ~310 lines (incl. error handlers); embed_query ~55 lines (within the 50-line guidance; same shape as UltraVPR). Cyclomatic complexity low.
• DRY: Strategy-pair duplication (mega_loc vs mix_vpr) is intentional per the task spec § Constraints: "Each strategy ships its own concrete preprocessor — preprocessing parameters per upstream code drop … sharing would couple weights-versions across strategies and let one strategy's upgrade silently break another's preprocessing." _assert_engine_output_dim duplication is unintentional — see F1.
• Test quality: AAA pattern with explicit markers. Parametrised across _StrategySpec to keep cross-strategy assertions semantically identical. Each AC has at least one parametrised test plus targeted variants for failure modes.
• Dead code: None introduced. Literal import in strategy modules is used by _BACKBONE_LABEL: Final[Literal["mega_loc"]] / ["mix_vpr"] annotations.

Phase 4 — Security Quick-Scan

• No SQL, no shell, no eval / exec, no dynamic deserialisation.
• cv2.resize is the only third-party call; inputs are typed np.ndarray and validated for dtype / ndim / shape upstream.
• error_message[:512] truncation prevents pathological log-line / FDR-payload growth on a long backbone error.
• No hardcoded secrets, API keys, or paths beyond test-fixture placeholders (/models/mega_loc.trt, /cache/vpr/index.faiss).
• Image inputs are byte-bounded (uint8 only); rejection paths emit VprPreprocessError not raw cv2.error.

Phase 5 — Performance Scan

• Construction is O(1) (no GPU ops in __init__ per the task spec § Constraints).
• embed_query is O(H·W) for decode / resize / normalise — same algorithmic cost as UltraVPR. The 2048-d / 4096-d FP16 embedding is allocated once per frame.
• No N+1 patterns, no unbounded fetching.
• One FDR-record allocation per frame on the success path — same per-frame allocation cost as UltraVPR; sits well below the bounded capacity of the FdrClient ring.
• NFR-perf budgets (MegaLoc ≤ 80 ms p95, MixVPR ≤ 100 ms p95) are research-side guidance per the task spec § NFR; not engine-rule-binding. Cannot be measured in unit tests; deferred to Step 9 / E-BBT against the real engines per the task spec § Risks 1 + 4.

Phase 6 — Cross-Task Consistency

Single-batch with two strategies — they were implemented in lockstep, share the same factory create() shape, the same error envelope, the same FDR record kinds (vpr.embed_query, vpr.backbone_error, vpr.preprocess_error), and the same log kinds. The parametrised test surface verifies behavioural equivalence directly.

Phase 7 — Architecture Compliance

• Layer direction: c2_vpr modules import from _types (L1), clock (L1), helpers.descriptor_normaliser (L1), helpers.iso_timestamps (L1), config.schema (L1), fdr_client (L2), and internal c2_vpr modules. No upward imports. PASS.
• Public API respect: The strategies do NOT import from c6_tile_cache or c7_inference directly — they use the consumer-side cuts (DescriptorIndexCut, InferenceRuntimeCut) defined locally in c2_vpr per AZ-507. PASS.
• No new cyclic dependencies: New modules sit in c2_vpr leaf positions; no incoming imports from c2_5 / c3 / runtime_root that didn't already exist for ultra_vpr / net_vlad. PASS.
• Duplicate symbols: F1 (above) — _assert_engine_output_dim is the only new duplication. Strategy class names are unique (MegaLocStrategy, MixVprStrategy). Preprocessor class names are unique. Constants (DESCRIPTOR_DIM, _BACKBONE_LABEL) are module-scoped and intentionally per-strategy.
• Cross-cutting concerns not locally re-implemented: The new strategies import iso_ts_from_clock from gps_denied_onboard.helpers.iso_timestamps — they do NOT re-introduce a local _iso_ts_from_clock body (verified by test_ac4_az526_no_module_level_iso_ts_from_clock_outside_helper continuing to pass post-AZ-339). PASS. AZ-526's regression guard worked exactly as designed.

Pre-existing failure noted (not blocking)

tests/unit/c12_operator_orchestrator/test_cli_console_script.py::TestConsoleScript::test_cold_start_under_500ms_p99 — fails on this dev laptop with a subprocess.TimeoutExpired after 5 seconds when running operator-orchestrator --help. Confirmed pre-existing by stashing the AZ-339 changes, running the test against the prior commit 5dfd9a5 (AZ-526), and observing the same failure. Cold-start latency depends on local Python interpreter startup + import time and is unrelated to this batch. Not blocking; logged here for traceability.

Verdict Rationale

One Medium finding (F1: _assert_engine_output_dim 4-way duplication, planned for AZ-527) and one Low finding (F2: AC-10 wording drift, mirroring established AZ-337 / AZ-338 precedent). No Critical, no High. Verdict: PASS_WITH_WARNINGS.