# Code Review Report

**Batch**: 18 — AZ-659, AZ-660, AZ-661
**Date**: 2026-05-20
**Verdict**: PASS_WITH_WARNINGS

## Findings

| # | Severity | Category | File:Line | Title |
|---|----------|----------|-----------|-------|
| 1 | Medium | Maintainability | `runtime.rs:392-411` | Dead code: unused `Instant::now()` + no-op `let _ = in_flight` |
| 2 | Low | Architecture | `lib.rs (detection_client)` | `pub mod internal` exposes generated proto server types to external crates |
| 3 | Low | Maintainability | `stats.rs:66` | `note_orphan_response` increments `stream_errors_total` — imprecise bucket |
| 4 | Low | Performance | `runtime.rs:build_request` | `frame.pixels.to_vec()` copies the full pixel buffer for each gRPC encode |

### Finding Details

**F1: Dead code in `handle_response`** (Medium / Maintainability) — **FIXED**
- Location: `crates/detection_client/src/internal/runtime.rs`
- Description: `let now = Instant::now()` was captured but never used; `let _ = in_flight` was a no-op for a `Copy` type, suggesting incomplete RTT tracking that was never wired up.
- Fix applied: removed both dead statements; replaced multi-paragraph placeholder comment with a concise doc note.

**F2: `pub mod internal` exposes server proto types** (Low / Architecture)
- Location: `crates/detection_client/src/lib.rs:40`
- Description: `pub mod internal` is required for integration tests in `tests/stream.rs` that need `detection_service_server` types to spin up the fixture gRPC server. The side-effect is that `detection_client::internal::*` is also visible to external crates, which contradicts module-layout rule #3.
- Suggestion: gate the re-export behind `#[cfg(any(test, feature = "test-utils"))]` or move fixture server helpers into a private dev-dependency crate when test infra consolidation is next in scope. Not worth fixing now — the practical risk is negligible (no external crate is expected to consume `detection_client::internal`).

**F3: `note_orphan_response` uses wrong counter** (Low / Maintainability)
- Location: `crates/detection_client/src/internal/stats.rs:66`
- Description: An orphan response (response arrived after the in-flight slot was budget-evicted) is a normal consequence of drop-oldest budgeting, not a stream error. Incrementing `stream_errors_total` conflates two distinct observability signals and could mislead operators.
- Suggestion: Add a dedicated `orphan_responses_total: AtomicU64` field in a future stats refactor. Not blocking — the counter is additive and currently only consumed internally.

**F4: Pixel buffer copy per gRPC frame** (Low / Performance)
- Location: `crates/detection_client/src/internal/runtime.rs:build_request`
- Description: `pixels: frame.pixels.to_vec()` allocates a `Vec<u8>` copy of the full pixel buffer (potentially 3–25 MB at operational resolutions) for each frame before gRPC serialisation. The `Arc<Bytes>` on the frame prevents sharing across the gRPC encode path because prost requires owned `Vec<u8>` for `bytes` fields.
- Suggestion: Investigate `bytes::Bytes` integration with prost's `bytes` feature flag in a future optimisation pass. Not a regression — the copy existed implicitly before and is unavoidable with the current proto stack version.

---

## Phase 2: Spec Compliance Summary

### AZ-659 — frame_ingest_publisher

| AC | Status | Test |
|----|--------|------|
| AC-1: Three consumers at rate, no drops | PASS | `ac1_three_consumers_at_rate_lose_no_frames` |
| AC-2: Slow consumer drops, fast unaffected | PASS | `ac2_slow_consumer_drops_while_fast_consumers_unaffected` |
| AC-3: Fan-out is zero-copy via Arc<Bytes> | PASS | `ac3_fan_out_is_zero_copy_via_arc_bytes` |

### AZ-660 — detection_client_grpc_stream

| AC | Status | Test |
|----|--------|------|
| AC-1: 30 fps / 10 s / ≥285 batches / p99 ≤100 ms / drops=0 | PASS | `ac660_1_happy_path_30fps_285_batches` |
| AC-2: Reconnect within ≤2 s after stream close | PASS | `ac660_2_reconnects_after_stream_close` |
| AC-3: Budget drops > 0 on 200 ms server | PASS | `ac660_3_budget_drops_on_slow_server` |
| AC-4: ai_locked frames skipped | PASS | `ac660_4_ai_locked_frames_skipped` |

### AZ-661 — detection_client_schema_and_health

| AC | Status | Test |
|----|--------|------|
| AC-1: Schema mismatch → hard error + counter | PASS | `ac661_1_schema_mismatch_hard_error` |
| AC-2: model_version change → exactly one event | PASS | `ac661_2_model_version_change_emits_event` |
| AC-3: Tier1Degraded emitted exactly once on latency spike | PASS | `ac661_3_tier1_degraded_emitted_once_on_latency_spike` |

---

## Phase 7: Architecture Compliance

| Rule | Check | Result |
|------|-------|--------|
| Layer direction | `detection_client` imports only `shared` (Layer 1); no sibling crate imports | PASS |
| Layer direction | `frame_ingest` imports only `shared` (Layer 1) | PASS |
| Public API respect | No cross-component imports of internal modules | PASS |
| No new cyclic deps | Import graph: detection_client → shared, frame_ingest → shared; no cycles | PASS |
| Module-layout sync | `detection_client` public API section updated to reflect streaming shape | PASS (fixed) |
| Module-layout sync | `frame_ingest` public API section updated to include publisher methods | PASS (fixed) |

---

**critical_count**: 0
**high_count**: 0
**Medium findings auto-fixed inline**: 1 (F1)
**Verdict**: PASS_WITH_WARNINGS — proceed to commit.