# Batch 45 — Code Review

**Tasks**: AZ-341 (C2 FAISS HNSW Retrieve Wiring)
**Cycle**: 1
**Reviewer**: autodev
**Verdict**: **PASS_WITH_WARNINGS**

## Scope reviewed

Production code:

- `src/gps_denied_onboard/components/c2_vpr/_faiss_bridge.py` (NEW —
  `FaissBridge` class with constructor validation, `retrieve(...)`
  method, INV-4 defensive check, WARN-threshold + DEBUG-flag log
  emission, FDR record + latency capture).
- `src/gps_denied_onboard/components/c2_vpr/descriptor_index_cut.py`
  (NEW — `DescriptorIndexCut` Protocol mirroring c6
  `DescriptorIndex.search_topk`; AZ-507 consumer-side structural cut
  so c2_vpr remains c6-import-free; defines the local `TileIdTuple`
  alias matching `VprCandidate.tile_id`'s composite identity).
- `src/gps_denied_onboard/components/c2_vpr/config.py` (added
  `warn_top1_threshold: float = 0.30` and
  `debug_per_frame_distances: bool = False` config knobs + their
  validators in `__post_init__`).
- `src/gps_denied_onboard/components/c2_vpr/__init__.py` (re-exports
  `DescriptorIndexCut` and `TileIdTuple` for the composition root's
  c6 adapter; deliberately does NOT re-export `FaissBridge` per the
  internal/`_underscore` convention — each strategy injects the
  bridge via its own `create(...)` factory).
- `src/gps_denied_onboard/fdr_client/records.py` (registered
  `vpr.retrieve_topk` in `KNOWN_PAYLOAD_KEYS` with payload field set
  `{frame_id, backbone_label, top10_distances, latency_us}`).

Tests:

- `tests/unit/c2_vpr/test_faiss_bridge.py` (NEW — 22 tests covering
  AC-1..AC-11 + NFR-perf microbench + constructor validation +
  retrieve-argument validation).
- `tests/unit/test_az272_fdr_record_schema.py` (added a fixture
  payload for `vpr.retrieve_topk` so the schema roundtrip suite
  covers the new record kind alongside `c12.reloc.requested`).

## Phase 1 — Context loading

Inputs read:

- Task spec: `_docs/02_tasks/todo/AZ-341_c2_faiss_retrieve_wiring.md`
- Contracts: `_docs/02_document/contracts/c2_vpr/vpr_strategy_protocol.md`,
  `_docs/02_document/contracts/c6_tile_cache/descriptor_index.md`,
  `_docs/02_document/contracts/c6_tile_cache/tile_metadata_store.md`.
- Module layout: `_docs/02_document/module-layout.md` (c2_vpr
  Per-Component Mapping; AZ-507 cross-component rule).
- Existing surfaces: `c2_vpr/{interface,config,errors,__init__}.py`,
  `c6_tile_cache/{interface,errors,_types}.py`, `_types/vpr.py`,
  `fdr_client/{client,records}.py`, `clock/interface.py`,
  `c12_operator_orchestrator/operator_reloc_service.py` (pattern
  reference for FdrClient + Clock + logger composition).

## Phase 2 — Spec compliance

AC coverage map (every AC has at least one covering test):

| AC | Test |
|----|------|
| AC-1 happy-path retrieve | `test_retrieve_happy_path_returns_vpr_result_and_emits_fdr` |
| AC-2 undersized → IndexUnavailableError | `test_retrieve_undersized_corpus_raises_index_unavailable_error` |
| AC-3 unordered → IndexUnavailableError | `test_retrieve_unordered_distances_raises_index_unavailable_error` |
| AC-4 c6 error propagates unchanged | `test_retrieve_propagates_index_unavailable_error_unchanged` |
| AC-5 WARN threshold trigger | `test_retrieve_emits_warn_log_when_top1_above_threshold` |
| AC-6 WARN not triggered when below | `test_retrieve_does_not_emit_warn_when_top1_below_threshold` |
| AC-7 DEBUG on → log | `test_retrieve_emits_debug_log_when_per_frame_distances_on` |
| AC-8 DEBUG off → no log | `test_retrieve_does_not_emit_debug_log_when_per_frame_distances_off` |
| AC-9 FDR record fields | `test_retrieve_fdr_record_fields_are_populated_with_positive_latency` |
| AC-10 retrieve_topk body is one line | `test_every_concrete_strategy_retrieve_topk_body_is_one_return_statement` |
| AC-11 per-strategy descriptor_dim | `test_descriptor_dim_carried_through_to_each_candidate` |
| NFR-perf p95 ≤ 0.5 ms | `test_bridge_retrieve_overhead_p95_under_500us` |

Contract verification:

- `vpr_strategy_protocol.md` — the bridge returns ``VprResult`` with
  exactly ``k`` candidates sorted ascending (INV-4), populates
  ``backbone_label`` non-empty (INV-5), and propagates
  ``IndexUnavailableError`` rather than returning stale candidates
  (C2-ST-01). All satisfied.
- `descriptor_index.md` — the c6 surface returns ``≤ k`` results
  (Invariant I-2) and ``IndexUnavailableError`` on stale handle / dim
  mismatch. The bridge enforces a stricter operational invariant
  (== k) on top, which is the task-spec's explicit defended-in-depth
  intent (see Constraints § "The defensive INV-4 check is
  mandatory"). The operational/structural distinction is documented
  in `_faiss_bridge.py`'s module docstring.

## Phase 3 — Code quality

- **SRP**: `FaissBridge` has exactly one job — share the retrieve
  plumbing across strategies. `DescriptorIndexCut` has one job —
  cut c6 imports per AZ-507.
- **Constructor validation**: every argument is type- and
  range-checked at construction; `ValueError` for value violations,
  `TypeError` for type violations.
- **Error handling**: no bare `except`; only catches nothing (lets
  c6 errors propagate per AC-4); raises `IndexUnavailableError` from
  c2_vpr's family for the bridge's own INV-4 violations.
- **Naming**: `_verify_invariants` / `_log_invariant_violation` /
  `_emit_fdr_record` read clearly from the call site.
- **Complexity**: `retrieve(...)` is ~30 lines including blank
  lines; `_verify_invariants` is ~22 lines. All under 50.
- **Test quality**: every test follows Arrange / Act / Assert with
  explicit comments; assertions check the actual record kind, the
  WARN log's structured `kv`, and the FDR payload by key — not
  just "no exception thrown".
- **Dead code**: none.
- **No verbose debug logging by default**: DEBUG per-frame distances
  is OFF by default (AC-8) — only ON when explicitly enabled by
  config; the WARN threshold is config-driven, not hard-coded.

## Phase 4 — Security quick-scan

- No SQL, no `subprocess`, no `eval` / `exec`.
- No hardcoded secrets.
- No user input is interpolated into log messages or FDR records
  beyond integer `frame_id` and string `backbone_label` already
  validated upstream.
- Sensitive data: the FDR record carries `top10_distances` (raw
  floats) — not PII, not signing material. Safe to persist.

## Phase 5 — Performance scan

- `retrieve` is O(k) for the INV-4 sorted check + O(k) for
  candidate construction; k=10 typical.
- No N+1 / unbounded fetch / blocking I/O / async-context issues.
- FDR enqueue is non-blocking (SPSC ring, drop-oldest on overrun
  delegated to the AZ-274 overrun policy).
- Microbench `test_bridge_retrieve_overhead_p95_under_500us`
  exercises 1000 calls with a wall-clock-backed `Clock` and asserts
  p95 ≤ 500 µs — passes.

## Phase 6 — Cross-task consistency

Single-task batch (AZ-341 only). N/A.

## Phase 7 — Architecture compliance

- **Layer direction**: c2_vpr (Layer 3) imports only from
  `_types.vpr` (L1), `clock` (L1), `config` (L1), `logging` (L1),
  `fdr_client` (L1), and its own component module
  (`descriptor_index_cut`). No upward imports. ✓
- **Public API respect**: ZERO direct imports of
  `gps_denied_onboard.components.c6_tile_cache.*` in any c2_vpr
  source file — the bridge consumes `DescriptorIndexCut` per the
  AZ-507 rule.
- **New cyclic dependencies**: none introduced.
- **Duplicate symbols**: `IndexUnavailableError` already exists in
  both `c2_vpr.errors` and `c6_tile_cache.errors` (pre-existing,
  documented in both modules' docstrings — intentional namespace
  duplication for the closed-envelope rewrap pattern).
- **Cross-cutting concerns**: `_iso_ts_from_clock` is duplicated
  across `c2_vpr/_faiss_bridge.py`, `c5_state`, `c11_tile_manager`,
  and `c12_operator_orchestrator/operator_reloc_service.py`. See
  Finding F1 below.

## Findings

| # | Severity | Category       | File:Line                                            | Title |
|---|----------|----------------|------------------------------------------------------|-------|
| 1 | Low      | Maintainability | `c2_vpr/_faiss_bridge.py`:`_iso_ts_from_clock`      | Duplicated `_iso_ts_from_clock` helper across c2/c5/c11/c12 |
| 2 | Low      | Spec-Gap       | `_docs/02_tasks/todo/AZ-341_c2_faiss_retrieve_wiring.md` | Task spec's constructor lists `normaliser: DescriptorNormaliser` but body never calls it; bridge correctly omits the parameter |

### Finding Details

**F1: Duplicated `_iso_ts_from_clock` helper** (Low / Maintainability)

- Location: `c2_vpr/_faiss_bridge.py` `_iso_ts_from_clock`
- Description: This six-line helper converts `clock.time_ns()` to an
  RFC 3339 UTC ISO string with nanosecond precision. The same body
  appears inline in `c12_operator_orchestrator/operator_reloc_service.py`,
  `c11_tile_manager`, and `c5_state`. Factoring it into a shared
  helper (e.g. `helpers.iso_ts_from_clock`) would let every FDR
  producer share one definition.
- Suggestion: defer to a cross-cutting hygiene PBI (AZ-508
  "ISO timestamps consolidation" is already in `todo/` for exactly
  this concern). No action this batch.
- Task: AZ-341

**F2: Spec lists unused `normaliser` constructor parameter** (Low / Spec-Gap)

- Location: `_docs/02_tasks/todo/AZ-341_c2_faiss_retrieve_wiring.md`
  § Outcome (Constructor)
- Description: The task-spec constructor signature names a
  `normaliser: DescriptorNormaliser` parameter, but the `retrieve(...)`
  body uses neither L2 nor intra-cluster normalisation — the
  embedding arrives already L2-normalised because every concrete
  `VprStrategy.embed_query` normalises before returning the
  `VprQuery` (per `VprStrategy` INV-3). Including the parameter
  would be dead weight on every strategy's `create(...)` factory
  with no behavioural effect.
- Suggestion: surface to the user; if the spec genuinely intended
  defensive re-normalisation in the bridge, add it explicitly and
  document why (it conflicts with INV-3 of the producing strategy).
  Otherwise update the spec to drop the parameter. The
  implementation matches the literal AC list either way.
- Task: AZ-341

## Verdict

PASS_WITH_WARNINGS — two Low-severity findings, both pre-existing
or stylistic. No Critical, no High, no Medium. The implementation
satisfies every AC, every NFR, the AZ-507 cross-component rule, and
the existing C2 contract surface.

The implement skill's Auto-Fix gate accepts this verdict without
intervention; both findings carry over to the cumulative review
(batches 43-45) where they may be batched with similar items.