Co-authored-by: Cursor <cursoragent@cursor.com>
6.4 KiB
Batch 52 — Implementation Report (Cycle 1)
Tasks: AZ-527 (Hygiene — consolidate _assert_engine_output_dim into a c2-internal helper)
Date: 2026-05-14
Cycle: 1
Status: COMPLETE (review verdict: PASS, zero findings)
What was done
Closed cumulative review batches 49-51 Finding F1 (Medium / Maintainability) — the 7-way duplication of _assert_engine_output_dim across the c2_vpr secondary VPR strategy modules. Added one new c2-internal helper module + one new test file; migrated 7 strategy modules to import the helper and deleted their local copies.
Files added (3)
| File | Purpose |
|---|---|
src/gps_denied_onboard/components/c2_vpr/_engine_dim_assertion.py |
The c2-internal helper. Exposes a single function assert_engine_output_dim(inference_runtime, handle, preprocessor, descriptor_dim, *, output_key="embedding", input_key="input") -> None that runs a zero-init dry-run inference at preprocessor.input_shape() and raises gps_denied_onboard.config.schema.ConfigError on output-shape or output-key mismatch. |
tests/unit/c2_vpr/test_az527_engine_dim_assertion.py |
14 tests, AAA pattern, Protocol-conforming fakes. Covers AC-1..AC-4 + a regression guard for stray definitions outside the helper module + an import-grep regression guard verifying all 7 strategy modules import the helper. |
_docs/03_implementation/reviews/batch_52_review.md |
Code-review report (verdict: PASS, zero findings). |
Files changed (7)
| File | Change |
|---|---|
src/gps_denied_onboard/components/c2_vpr/ultra_vpr.py |
Add helper import; replace local _assert_engine_output_dim(...) call with assert_engine_output_dim(...) (passing output_key=_OUTPUT_KEY, input_key=_ENGINE_INPUT_KEY); delete the local function definition (~30 lines). |
src/gps_denied_onboard/components/c2_vpr/net_vlad.py |
Same pattern, with output_key="vlad_descriptor" override (NetVLAD's output key) + runtime-resolved descriptor_dim arg. |
src/gps_denied_onboard/components/c2_vpr/mega_loc.py |
Same pattern as UltraVPR. Also deletes the inline AZ-527 (planned) comment (4 lines). |
src/gps_denied_onboard/components/c2_vpr/mix_vpr.py |
Same pattern as MegaLoc. |
src/gps_denied_onboard/components/c2_vpr/sela_vpr.py |
Same pattern as MegaLoc. The "7-way duplication … tracked by AZ-527" comment is gone. |
src/gps_denied_onboard/components/c2_vpr/eigen_places.py |
Same pattern as SelaVPR. |
src/gps_denied_onboard/components/c2_vpr/salad.py |
Same pattern as SelaVPR. |
Net diff: +615 / -191 across 10 files (8 src + 1 test + 1 review report).
AC coverage
All 6 ACs verified.
| AC | Status | Notes |
|---|---|---|
| AC-1 (helper exists with expected signature) | PASS | test_ac1_helper_callable_with_default_keys |
AC-2 (wrong shape raises ConfigError with both dims named) |
PASS | test_ac2_wrong_descriptor_width_raises_config_error (3 parametrised) + test_ac2_wrong_ndim_or_batch_raises_config_error (4 parametrised) |
AC-3 (missing output key raises ConfigError naming the missing key) |
PASS | test_ac3_missing_default_output_key_raises_config_error + test_ac3_missing_overridden_output_key_raises_config_error |
| AC-4 (no stray local definitions remain) | PASS | test_ac4_no_stray_engine_dim_assertion_definitions_outside_helper (AST walk) + test_ac4_seven_strategy_modules_import_the_helper (import grep) |
| AC-5 (AZ-337/338/339/340 AC-6 sub-tests pass unmodified) | PASS | tests/unit/c2_vpr/ — 230 / 230 PASS, no test file modified outside the new test_az527_* |
| AC-6 (AZ-270 + AZ-507 layer lints pass) | PASS | tests/unit/test_az270_compose_root.py — 8 / 8 PASS |
Test results
tests/unit/c2_vpr/test_az527_engine_dim_assertion.py— 14 / 14 PASS.tests/unit/c2_vpr/(full directory: faiss_bridge + net_vlad + ultra_vpr + AZ-339 + AZ-340 + AZ-527 + protocol_conformance) — 230 / 230 PASS.tests/unit/test_az270_compose_root.py(composition-root layer lint) — 8 / 8 PASS.tests/unit/test_az508_iso_timestamps.py(helpers AST-walk regression guards from AZ-526) — 18 / 18 PASS (sanity check only; AZ-527 doesn't touch helpers/).ruff checkon all 10 changed files — CLEAN after one round-trip fix (auto-detectedUP037quoted-annotation issue in the helper; switched to bare names sincefrom __future__ import annotationsis in effect).
Architectural decisions
- Helper stays inside c2_vpr/, NOT in shared/helpers/ — engine output-shape contracts are a c2 internal concern. C7 owns its own engine-shape assertions inside the runtime; sharing the helper across components would entangle the c2 / c7 boundaries that AZ-507 carved cleanly. Documented in the helper module's docstring.
- Underscore-prefixed module name (
_engine_dim_assertion.py) — keeps the helper out ofc2_vpr/__init__.py's Public API surface. Strategy modules import it as a sibling internal module; no cross-component import is added. - Keyword-only
output_key/input_key(the*separator afterdescriptor_dim) — forbids positional misuse. A future strategy author cannot accidentally swap them. - Per-strategy explicit
output_key=_OUTPUT_KEY+input_key=_ENGINE_INPUT_KEYat the 6 default-key call sites — even though the helper's defaults match the strategy constants, passing them explicitly documents the strategy's engine-IO contract at the call site. Only NetVLAD overrides (output_key="vlad_descriptor"). from __future__ import annotations+ bare type names — RuffUP037correctly flags quoted annotations as redundant whenfrom __future__ import annotationsis in scope. The helper uses bare types in the signature;TYPE_CHECKING-guarded imports keep runtime overhead at zero.- AST-walk + import-grep regression guards modeled on AZ-526's
test_no_local_iso_ts_*_definitions_remain— same pattern, same enforcement strength. Future c2_vpr strategy authors who slip a copy back in get caught at CI time.
Spec drift noted
None.
Cumulative review obligation
Next cumulative review trigger fires after Batch 54 (every K=3 from B51 → B54). Batch 52 is the first batch in the new window. No interim cumulative review obligation.
Follow-on PBIs
None. AZ-527 closes the F1 finding cleanly. The earlier F2 (AC-10 spec drift across AZ-337/338/339/340) is documentation-only and remains carried over for a future shared spec-pass; it does not block.