From fbeeab60b3045e73a32c74972ea254e6014ba00d Mon Sep 17 00:00:00 2001 From: Oleksandr Bezdieniezhnykh Date: Wed, 13 May 2026 23:26:58 +0300 Subject: [PATCH] [AZ-337] [AZ-338] [AZ-508] Cumulative review batches 46-48 Verdict: PASS_WITH_WARNINGS. Per-batch reviews already validated each task's ACs; this cumulative review focuses on cross-batch drift and surfaces 1 Medium + 2 Low maintainability findings: - F1 (Medium): `_iso_ts_from_clock` Clock-injected helper duplicated across 4 files (c2_vpr/net_vlad + ultra_vpr + _faiss_bridge, c12_operator_orchestrator/operator_reloc_service). B46 + B47 carry inline comments anticipating AZ-508 would consolidate this, but AZ-508 (Batch 48) scoped itself narrower (stdlib-only, Excluded the Clock-injected variant). Recommend a 2-point follow-up PBI adding `iso_ts_from_clock(clock)` to helpers/iso_timestamps.py before AZ-339 / AZ-340 / AZ-358 / AZ-389 add more copies. - F2 (Low): `_assert_engine_output_dim` near-duplicated between NetVLAD and UltraVPR. Defer consolidation until 5 c2_vpr strategies are in flight (after AZ-339 / AZ-340). - F3 (Low): Clock-driven helper outputs `+00:00`; canonical FDR `ts` is `Z`. Fold into F1 follow-up PBI. No Critical or High findings; auto-chain to next batch allowed. Co-authored-by: Cursor --- ...tive_review_batches_46-48_cycle1_report.md | 246 ++++++++++++++++++ _docs/_autodev_state.md | 4 +- 2 files changed, 248 insertions(+), 2 deletions(-) create mode 100644 _docs/03_implementation/cumulative_review_batches_46-48_cycle1_report.md diff --git a/_docs/03_implementation/cumulative_review_batches_46-48_cycle1_report.md b/_docs/03_implementation/cumulative_review_batches_46-48_cycle1_report.md new file mode 100644 index 0000000..503c023 --- /dev/null +++ b/_docs/03_implementation/cumulative_review_batches_46-48_cycle1_report.md @@ -0,0 +1,246 @@ +# Cumulative Code Review — Batches 46-48 (Cycle 1) + +**Date**: 2026-05-13 +**Range**: batches 46 (AZ-338 NetVLAD), 47 (AZ-337 UltraVPR), 48 (AZ-508 ISO-ts helper) +**Compared against**: previous cumulative review batches 43-45 +**Verdict**: **PASS_WITH_WARNINGS** + +## Scope + +24 files changed across batches 46-48 (15 src/, 5 test/, plus 1 helper +package init and 3 c6/c7 import rewires). Cumulative review focuses on +cross-batch concerns that per-batch reviews cannot catch: duplicate +symbols introduced across different batches, architecture drift across +the trio, contract drift between producer/consumer batches. + +## Findings + +| # | Severity | Category | File:Line | Title | +|---|----------|----------|-----------|-------| +| F1 | Medium | Maintainability | `c2_vpr/{net_vlad,ultra_vpr}.py` + `c12_operator_orchestrator/operator_reloc_service.py` + `c2_vpr/_faiss_bridge.py` | `_iso_ts_from_clock` duplicated across 4 files | +| F2 | Low | Maintainability | `c2_vpr/net_vlad.py:502` & `c2_vpr/ultra_vpr.py:440` | `_assert_engine_output_dim` near-duplicated between strategies | +| F3 | Low | Maintainability | `c2_vpr/{net_vlad,ultra_vpr}.py` | `_iso_ts_from_clock` outputs `+00:00`; canonical FDR `ts` is `Z` | + +## Finding Details + +### F1: `_iso_ts_from_clock` duplicated across 4 files (Medium / Maintainability) + +- **Locations**: + - `src/gps_denied_onboard/components/c2_vpr/net_vlad.py:375` (added Batch 46) + - `src/gps_denied_onboard/components/c2_vpr/ultra_vpr.py:326` (added Batch 47) + - `src/gps_denied_onboard/components/c2_vpr/_faiss_bridge.py` (pre-batch-46) + - `src/gps_denied_onboard/components/c12_operator_orchestrator/operator_reloc_service.py` (pre-batch-46) + +- **Description**: Each copy is an identical 5-line function that converts + a `Clock.time_ns()` into a nanosecond-precision ISO-8601 string. Both + net_vlad and ultra_vpr carry an inline comment that says + "AZ-508 will consolidate the duplicate helpers across c2/c11/c12/c6" — + but AZ-508 (Batch 48) scoped itself narrower (stdlib `datetime.now` + consolidation only, the Clock-injected variant was explicitly excluded + under "any change to flight_clock, wall_clock, or other adapter-style + time sources"). + + As C2 (AZ-339 / AZ-340), C4 (AZ-358 / AZ-361), C5 (AZ-389), and + remaining c11/c12 batches land, this pattern will keep multiplying. + +- **Suggestion**: open a new hygiene PBI **AZ-XXX** (≤2 points) to add a + second function to `helpers/iso_timestamps.py`: + + ```python + def iso_ts_from_clock(clock: Clock) -> str: + """Format `clock.time_ns()` as RFC 3339 UTC with nanosecond precision.""" + ``` + + This intentionally keeps both `iso_ts_now()` (wall-clock, microseconds, + Z-suffix) and `iso_ts_from_clock(clock)` (injected-clock, nanoseconds) + in the same Layer-1 module. Both serve FDR envelope timestamps but + have legitimately different testability requirements (the second is + Clock-injected so replay tests can pin time). + + When the new helper lands, the 4 existing copies (and the two inline + comments anticipating AZ-508) get removed. Sized ≤2 points to match + the AZ-508 precedent. + +- **Task scope context**: F1 is a forward-looking finding; B46 and B47 + pre-acknowledged it in comments. Not a regression of Batch 48 — Batch + 48's own AC-4 (zero stray `iso_ts_now` definitions) passed because + the duplicates are named `_iso_ts_from_clock`, not `iso_ts_now`. + +### F2: `_assert_engine_output_dim` near-duplicated between strategies (Low / Maintainability) + +- **Locations**: + - `src/gps_denied_onboard/components/c2_vpr/net_vlad.py:502` + - `src/gps_denied_onboard/components/c2_vpr/ultra_vpr.py:440` + +- **Description**: Both files define a private `_assert_engine_output_dim` + helper with near-identical bodies. Differences are limited to: + 1. NetVLAD uses string literals `"input"` / `"vlad_descriptor"`; + UltraVPR uses module-level `_ENGINE_INPUT_KEY` / `_OUTPUT_KEY` + constants. + 2. NetVLAD takes `expected_dim` as a parameter; UltraVPR pins it to + the module-level `DESCRIPTOR_DIM = 512` constant. + 3. Different preprocessor types in the signature. + + The error-message strings and the shape-check logic are byte-identical. + +- **Suggestion**: when AZ-339 and AZ-340 land (3 more strategies), the + duplication will grow to 5 copies. Consolidate then into a single + `_assert_engine_output_dim` inside a new c2_vpr-internal module + (`_engine_probe.py`) accepting `input_key: str`, `output_key: str`, + `expected_dim: int`, `preprocessor: BackbonePreprocessor` (uses the + existing Protocol). Not urgent for Batch 48; revisit after AZ-339. + +- **Defer rationale**: with only 2 copies today the consolidation buys + little; with 5 copies expected after AZ-339 + AZ-340 it becomes + worthwhile. Bookmark, do not file yet. + +### F3: `_iso_ts_from_clock` outputs `+00:00`; canonical FDR `ts` is `Z` (Low / Maintainability) + +- **Locations**: same as F1. + +- **Description**: The Clock-driven helper formats as + `"%Y-%m-%dT%H:%M:%S.{nanos:09d}+00:00"`, while the canonical FDR `ts` + fixture in `test_az272_fdr_record_schema.py` and the new + `helpers/iso_timestamps.iso_ts_now()` use `Z` suffix + (`%Y-%m-%dT%H:%M:%S.%fZ`). Both are valid RFC 3339; the FDR `ts` + field is typed `str` without format validation, so consumers parse + both. This means c2_vpr-emitted FDR records carry a slightly + different `ts` byte-shape than c6/c7/c11/c13-emitted records. + +- **Suggestion**: fold the fix into the F1 PBI — the new + `iso_ts_from_clock` helper should emit the same `Z` suffix as + `iso_ts_now` for consistency. (Nanosecond precision is preserved + by using `f"{...}.{fraction_ns:09d}Z"` rather than the `+00:00` + variant.) + +## Phase Summary + +### Phase 1 — Context Loading + +- 3 task specs reviewed (AZ-338 / AZ-337 / AZ-508). +- 4 doc inputs: `architecture.md`, `module-layout.md`, the 3 per-batch + reviews (`batch_46_review.md`, `batch_47_review.md`, + `batch_48_review.md`), and `_docs/02_document/system-flows.md`. +- No `_docs/02_document/architecture_compliance_baseline.md` exists yet, + so the **Baseline Delta** section is omitted from this report (will + be enabled once that baseline is produced — likely an existing-code + flow Step 2 deliverable for future cycles). + +### Phase 2 — Spec Compliance + +Per-batch reviews already covered AC-by-AC compliance for each task. +Cross-batch check: the AZ-338 + AZ-337 strategies are wired into the +same `vpr_factory.py` and share the `inference_runtime_cut` / +`descriptor_index_cut` Protocols; the NetVLAD strategy explicitly +exposes `MODEL_NAME` / `architecture_factory` while UltraVPR does NOT +(it consumes a pre-compiled `.trt` engine). The composition root's +`_register_strategy_architecture` helper no-ops cleanly for the +UltraVPR case — verified by `test_create_does_not_register_pytorch_architecture` +in B47. No drift. + +### Phase 3 — Code Quality + +- **SRP**: NetVLAD and UltraVPR each have a single clear concern + (one backbone strategy); helpers separated into `_preprocessor_*.py`. +- **Error handling**: typed exception envelopes from `_types.inference_errors` + used consistently; no bare `except`. +- **Naming**: aligned across both strategies; method shapes for + `embed_query` / `retrieve_topk` / `_emit_*` mirror each other. +- **Test quality**: AC-by-AC coverage with regression spies (e.g., + `test_ac2_embedding_is_single_stage_l2_no_intra_cluster_path` in B47 + uses a spy normaliser to guard against a future refactor accidentally + introducing NetVLAD's two-stage path into UltraVPR — excellent + defensive testing). + +### Phase 4 — Security Quick-Scan + +No SQL, no `subprocess`, no `eval` / `exec`, no secrets. The new code +is pure-Python numerical wiring plus FDR record emission. n/a. + +### Phase 5 — Performance Scan + +- Both new strategies use FP16 inputs and pre-compiled engines — + consistent with the project's perf budget. +- Engine load + output-shape assertion happens at `create()` time + (startup) not first-frame — explicit AB47 architectural decision, + matches Constraint § 5. +- No obvious hot-path allocations beyond what numpy / FAISS already + do. + +### Phase 6 — Cross-Task Consistency + +- **NetVLAD vs UltraVPR pattern parity**: method shapes for + `embed_query` / `retrieve_topk` / `_emit_*` mirror line-for-line; + strategy-specific paths (single-stage L2 vs intra-cluster + L2, + `"vlad_descriptor"` vs `"embedding"` output key, PyTorch vs TRT + runtime, architecture registry vs no registry) are clearly + localised. Good design — drops AZ-339 / AZ-340 into the same + skeleton. +- **Shared `FaissBridge` + `inference_runtime_cut` + `descriptor_index_cut`**: + both strategies wire through the same Layer-2 internals; no + contract drift detected. +- **`intra_cluster_normalise` helper** (added in B46): used ONLY by + NetVLAD as intended. UltraVPR's preprocessor and forward path do + NOT call it — verified by the B47 spy test. No accidental + cross-coupling. +- **FDR record schema additions** in `test_az272_fdr_record_schema.py` + (21 new lines): these are NEW record kinds added by B46/B47, not + modifications of existing kinds — does not break AZ-508 AC-5's + "tests pass unmodified" requirement for the *existing* records. +- **AZ-508 closure** of the F1 (`_iso_ts_now`) finding from cumulative + 43-45: verified — zero `_iso_ts_now` definitions remain anywhere + under `src/` (asserted by AZ-508's AST-walk test). + +### Phase 7 — Architecture Compliance + +1. **Layer direction**: c2_vpr (Layer 3) imports from `_types` + (Layer 1), `helpers/*` (Layer 1), `config` (Layer 1), `logging` + (Layer 1), `fdr_client` (Layer 1), `clock` (Layer 1) — all + downward. No upward imports detected. + +2. **Public API respect**: no cross-component imports from c2_vpr + except via `_types` and `helpers`. The `InferenceRuntimeCut` / + `DescriptorIndexCut` patterns (consumer-side structural Protocol + cuts) correctly avoid `from gps_denied_onboard.components.c6_tile_cache import ...` + and `from gps_denied_onboard.components.c7_inference import ...`. + Verified by `test_az270_compose_root.test_ac6_only_compose_root_imports_concrete_strategies` + (passes). + +3. **No new cyclic deps**: built-import graph of changed files + + direct dependencies — no cycles introduced. + +4. **Duplicate symbols across components**: F1 + F2 + F3 above. + +5. **Cross-cutting concern not locally re-implemented**: B48's + `helpers/iso_timestamps` correctly hoisted `_iso_ts_now` out of + per-component locals. F1 is the same shape of issue for the + Clock-driven variant — recommend folding it into the same + helper module via the F1 PBI suggestion. + +## Verdict Justification + +- Critical findings: 0 +- High findings: 0 +- Medium findings: 1 (F1) +- Low findings: 2 (F2, F3) + +**PASS_WITH_WARNINGS** — auto-chain to next batch is allowed per +implement skill Step 14.5. The F1 / F2 / F3 findings are flagged for +follow-up but do not block batch 49. + +## Recommended Follow-up + +1. **Before AZ-339** (Batch 49 candidate): file a 2-point hygiene PBI + `iso_ts_from_clock` consolidation in `helpers/iso_timestamps.py` + that covers F1 + F3 in a single small task. This stops the pattern + recurring as AZ-339 / AZ-340 / AZ-358 / AZ-389 add more copies. +2. **After AZ-339 / AZ-340**: revisit F2 (`_assert_engine_output_dim` + consolidation) when there are 5 c2_vpr strategies in flight. +3. **Spec hygiene for F2 carried over from B46/B47** (C7 API name drift + in AZ-339 / AZ-340 / AZ-358 / AZ-349 specs) — STILL outstanding, + not addressed by Batch 48. Recommend filing before AZ-339 starts. + +## Next Cumulative Review + +- **Trigger**: after Batch 51 (every K=3). +- **Range**: batches 49-51. diff --git a/_docs/_autodev_state.md b/_docs/_autodev_state.md index cbff520..e3bee87 100644 --- a/_docs/_autodev_state.md +++ b/_docs/_autodev_state.md @@ -8,9 +8,9 @@ status: in_progress sub_step: phase: 7 name: batch-loop - detail: "cumulative review batches 46-48 due before next batch" + detail: "" retry_count: 0 cycle: 1 tracker: jira last_completed_batch: 48 -last_cumulative_review: batches_43-45 +last_cumulative_review: batches_46-48