# Cumulative Code Review — Batches 13–15 (Cycle 1)

**Trigger**: `implement/SKILL.md` Step 14.5 — `K=3` batches completed since the last cumulative review (`cumulative_review_batches_07-09_cycle1_report.md`). Note: triplet 10–12 was skipped at the time and remains an outstanding gap on the cumulative cadence; surfaced here for visibility but not retro-scored.
**Date**: 2026-05-20
**Cycle**: 1
**Scope**: union of files changed in `batch_13_cycle1`, `batch_14_cycle1`, `batch_15_cycle1` (since the close of `batch_12_cycle1`).
**Mode**: inline (matching the per-batch precedent).
**Baseline**: `_docs/02_document/architecture_compliance_baseline.md` still does not exist. No `## Baseline Delta` section is produced. The intent recorded in cumulative reviews 04–06 and 07–09 to promote a baseline remains carried forward.

## Tasks in scope

| Batch | Tasks | Components touched |
|-------|-------|--------------------|
| 13 | AZ-683 (`scan_controller_poi_queue_and_window`) | `scan_controller` |
| 14 | AZ-675 (`telemetry_stream_grpc_server`) | `telemetry_stream`, workspace tonic/prost stack |
| 15 | AZ-676 (`telemetry_stream_video_path`), AZ-677 (`telemetry_stream_mapobjects_snapshot`), AZ-678 (`operator_bridge_command_auth`), AZ-679 (`operator_bridge_poi_surface`) | `telemetry_stream`, `operator_bridge`, `shared` |

**Total AC verification (rolled up)**: **6 (batch 13) + 5 (batch 14) + 14 (batch 15) = 25 / 25** ACs verified locally with tests; no unverified spec gap.

**Code volume** (approximate, source + tests, excluding `_docs/` and `Cargo.lock`):

- Batch 13: ~1,100 LOC added (scan_controller POI queue + priority module + 6 integration + 13 unit tests).
- Batch 14: ~1,400 LOC added (telemetry_stream tonic infrastructure + publisher + server + 5 integration + 6 unit tests; first-time workspace tonic/prost/protoc pins).
- Batch 15: ~1,950 LOC added (telemetry_stream video + mapobjects modules + operator_bridge auth + poi_surface modules + 11 + 18 unit + 12 integration tests + 2 new shared modules).

## Phase 1 — Spec coverage

Every Included scope item across these three batches lands in production code:

- **AZ-683 (Batch 13)**: production POI queue with proximity/age-weighted priority math, rolling 60 s × 5/min cap, confidence floor, decision-window mapping, timeout sweep, `DeclinePoi` operator-command end-to-end → `DeclineAction` for AZ-685.
- **AZ-675 (Batch 14)**: production Tonic gRPC server (`TelemetryStream::Subscribe`), per-(client, topic) broadcast queue, drop-counter back-pressure, RAII shutdown, `TelemetrySink::push_detections` real impl. Closes architecture Q2 in favour of gRPC server-streaming.
- **AZ-676 (Batch 15)**: production `VideoPublisher` with rtsp_forward + bytes_inline modes, ai_locked atomic + session counter, SubscribeVideo RPC.
- **AZ-677 (Batch 15)**: production snapshot-on-subscribe stream-prepend + diff broadcast on `Topic::MapObjectsBundle`; `MapObjectsSnapshotSource` trait + `EmptyMapObjectsSource` fixture pending the real `mapobjects_store` adapter.
- **AZ-678 (Batch 15)**: production `HmacOperatorValidator` with HMAC-SHA256, per-session monotonic seq tracker, in-process session registry with TTL, rejection-reason counters, sliding 60 s sig-failure window → red-health gate. Trait `OperatorCommandValidator` in `shared::contracts` so dispatch can depend on the contract without importing `operator_bridge`.
- **AZ-679 (Batch 15)**: production `PoiSurfaceMapper` producing `OperatorPoiEvent` per `architecture.md §7.10`, `PoiDequeued` events on rotation/age-out/completion, pushed via the new `TelemetrySink::push_operator_event` extension.

**Contract verification**:
- `shared::contracts::operator_auth::{SignedCommand, ValidatedCommand, AuthError, OperatorCommandValidator}` — trait shape matches the AZ-678 task `Contract` section verbatim.
- `shared::models::operator_event::{OperatorPoiEvent, PoiDequeued, OperatorEvent}` — fields match `architecture.md §7.10` and the AZ-679 task spec's field list. One **known gap**: `vlm_label` is wired in the wire shape but the producer is deferred to AZ-684 (`scan_controller` VLM ladder); the `Poi` model does not carry the label string today. Surfaced as a Low finding rather than a High Spec-Gap because the wire is in place and the producer is a separately scheduled ticket.

PASS.

## Phase 2 — Code quality

| Concern | Finding | Severity |
|---------|---------|----------|
| `serde_json::to_vec(payload).unwrap_or_default()` in `HmacOperatorValidator::signing_material` | Silent fallback to empty bytes on a hypothetical serde failure produces a signing string that the sign-side would also produce on the same failure, masking the issue. Project rule "never suppress errors silently" applies even when the failure is unreachable today. | Medium / Maintainability |
| Optional builder pattern on `OperatorBridge` (`with_telemetry_sink`, `with_validator`) | Both surfaces compile and run without the sink/validator wired, returning `NotImplemented`. Used as the bridge between the AZ-678/679 landing and the AZ-680 composition-root wiring. Acceptable as a temporary shape; should be reduced once AZ-680 fully wires the runtime. | Low / Scope |
| `surface_poi` returns `NotImplemented` after pushing the side-effect | A caller doing naive retry-on-error would double-publish. The intent ("surface pushed; decision loop is AZ-680") is comment-only. | Low / Scope |
| `vlm_label` always `None` in `PoiSurfaceMapper::map` | The `Poi` model doesn't carry the label; AZ-684 will produce it. Wire field is correct; producer wiring is the gap. | Low / Spec-Gap |
| `VideoSnapshot.mode_label` string vs proto `VideoMode` enum | Both exist in parallel and serve different consumers (health surface vs proto). Acceptable; documented in `internal/video.rs` and tested for parity in `mode_label_matches_task_spec_strings`. | — |
| `unsafe` blocks | None added across all three batches. | — |
| Production `unwrap` / `expect` | All hits are in `#[cfg(test)]` modules, `serde_json::to_string`/`from_str` round-trips, or `HMAC::new_from_slice` which is documented infallible for any key length. No production crash sites. | — |
| Test back-door discipline | No new `#[doc(hidden)]` or `*_for_tests` surfaces this triplet beyond the batch 9 ones already documented. | — |

## Phase 3 — Security quick-scan

- HMAC compare uses `hmac::Mac::verify_slice` (constant-time). Verified per AZ-678 NFR-Security.
- No SQL / shell-string interpolation.
- Rejection logging uses `command_id` only, never the raw payload. Per AZ-678 NFR-Security: "reject-then-log; never log the raw payload of a rejected command at info level".
- Session secrets stored in-process only; no leak to logs or telemetry.
- No new external input deserialization. The `MapObjectsTopicMessage` and `OperatorEvent` round-trips are over `serde_json` of canonical Rust types; no untrusted-source deserialization path.
- gRPC server binds to an explicit config-driven `listen_addr` (no implicit binding to 0.0.0.0 unless configured).
- Note: the wire payload for `VideoFrame.bytes` is opaque to `telemetry_stream` — the producer (`frame_ingest`) owns the codec semantics. No new attack surface at the gRPC boundary.

PASS.

## Phase 4 — Performance scan

- **Broadcast fan-out**: `tokio::sync::broadcast` with per-topic ring buffers (default `topic_capacity = 256`). Slow-subscriber drop is detected via `BroadcastStreamRecvError::Lagged(n)` and accounted in per-(client, topic) counters. Verified by `slow_subscriber_lags_fast_subscriber_does_not` (unit) and `ac2_slow_subscriber_drops_oldest_healthy_unaffected` (integration).
- **HMAC validate**: O(payload_size) HMAC compute + constant-time compare. Per AZ-678 NFR ≤1 ms p99 budget; the SHA-256 compute cost on a Jetson-class device for typical 64–256 byte payloads is well under that.
- **Session registry lookup**: `HashMap<token, SessionEntry>` — O(1) amortised. TTL check is O(1) per validate.
- **Sliding 60 s signature-failure window**: `VecDeque<Instant>`. Push + opportunistic prune is amortised O(1). The prune happens at every push and at every `health_is_red` call, so memory is bounded by `min(threshold × 2, 60 s of attempt traffic)`.
- **POI surface mapping**: `PoiSurfaceMapper::map` is a pure struct-to-struct copy plus an `Option::clone` of the Tier-2 evidence summary. Sub-millisecond by inspection; matches AZ-679 NFR ≤1 ms p99.
- **MapObjects snapshot serialisation**: `serde_json::to_vec` over the canonical bundle. Per AZ-677 NFR ≤200 ms p99 for ≤10 000 entries. Not benchmarked in this triplet; the `EmptyMapObjectsSource` fixture used in tests does not exercise that volume. **Open for next benchmark cycle**: add a `mapobjects_snapshot_serialise_10k_under_200ms` perf test once the real `mapobjects_store` adapter is wired.

PASS (with the snapshot perf-test as a noted follow-up, not a blocker).

## Phase 5 — Cross-task consistency

**Telemetry transport pattern (the load-bearing consistency check for this triplet)** — three independent topic categories now flow through the same `TelemetryPublisher`:

| Topic | Pattern | Snapshot? | Wire shape |
|-------|---------|-----------|------------|
| `TelemetrySample` / `GimbalState` / `DetectionEvent` / `MovementCandidate` | Pure broadcast | No | JSON of canonical Rust model |
| `MapObjectsBundle` | Snapshot-on-subscribe + broadcast diff | Yes (`MapObjectsBundleSnapshot`) | Tagged enum `MapObjectsTopicMessage { Snapshot, Diff }` |
| `OperatorEvent` | Pure broadcast (new in batch 15) | No (events are inherently incremental) | Tagged enum `OperatorEvent { PoiSurfaced, PoiDequeued }` |

Pattern convergence is intentional: every topic that needs to carry "structurally distinct kinds of message" uses a `serde(tag = "kind")` tagged enum; every topic that carries a single message type uses the bare model. This keeps the operator UI's deserialisation cheap and makes the topic catalogue easy to extend.

**Service expansion**: `TelemetryStream` proto grew from one RPC (`Subscribe`) in batch 14 to two RPCs (`Subscribe` + `SubscribeVideo`) in batch 15. The split is right — video has its own framing semantics (`oneof { session_start, frame }`) that don't belong in the generic `payload_json`-carrying telemetry channel. The two RPCs share zero implementation by design.

**Operator-side trait surface**: `OperatorCommandValidator` (auth, in `shared::contracts`) and `TelemetrySink::push_operator_event` (events, in `shared::contracts`) form the two halves of the operator boundary. The `Poi` → `OperatorPoiEvent` mapping owns the producer side; AZ-680 will own the dispatch side. Both halves cross the boundary through `shared::contracts`, so neither side imports the other directly.

**Naming**:
- `OperatorEvent` (the tagged enum) vs `OperatorCommand` (already in `shared::models::operator`) — clear directional split (events flow drone → GS, commands flow GS → drone). No collision.
- `MapObjectsDiff` (new in `telemetry_stream::internal::mapobjects`) vs `mission_client::MapObjectsDiff` (existing) — **different domains**: the transport-side diff (what `telemetry_stream` broadcasts to operator clients) vs the persistence-side diff (what `mission_client` pushes post-flight to the platform). Both are short snapshots of "what changed in the store"; the producers are disjoint and the consumers are disjoint, so the type collision is harmless. **Surfaced as a Low finding** for future cleanup: a shared `shared::models::mapobjects::Diff` would dedupe.

PASS (one new Low finding).

## Phase 6 — Architecture compliance

**Layer direction** (per `_docs/02_document/module-layout.md`):

- `scan_controller` (Layer 3, Coordinator) — adds `serde_json` + `chrono` deps; imports from `shared`, `mission_client`, `mapobjects_store`. No Layer 3 → Layer 3 import.
- `telemetry_stream` (Layer 2, Transport) — imports from `shared` only. The new `bytes` workspace dep is a Layer 1 utility. No upward import.
- `operator_bridge` (Layer 2, Transport) — imports from `shared` only. **Does not** import from `telemetry_stream` — instead depends on the `TelemetrySink` trait in `shared::contracts`, which `telemetry_stream::TelemetryStreamHandle` implements. This is the boundary that keeps the operator boundary cleanly testable (the `RecordingSink` in `poi_surface.rs` tests is a `TelemetrySink` impl with no transport).
- `shared` — added two new modules (`models::operator_event`, `contracts::operator_auth`) and one trait method (`TelemetrySink::push_operator_event`). No upward imports.

PASS.

**Public API respect**:
- `shared::contracts::operator_auth::{SignedCommand, ValidatedCommand, AuthError, OperatorCommandValidator}` — all in Public API.
- `shared::models::operator_event::{OperatorEvent, OperatorPoiEvent, PoiDequeued, DequeueReason, PhotoMetadata, Tier2EvidenceSummary}` — all in Public API.
- `telemetry_stream::{video_message, MapObjectsDiff, MapObjectsBundleSnapshot, MapObjectsTopicMessage, MapObjectsSnapshotSource, EmptyMapObjectsSource, VideoPath, VideoSnapshot}` — all re-exported from the crate root for cross-component consumption.
- `operator_bridge::{HmacOperatorValidator, HmacValidatorConfig, AuthCounters, REJECTION_REASONS, PoiSurfaceMapper, PoiSurfaceMetrics}` — all in Public API.

No internal-file imports across components.

PASS.

**Cyclic dependencies**: built the import graph over the changed files plus direct deps.

- `shared` ← `telemetry_stream`, `operator_bridge`, `scan_controller`, … (no cycles; shared is the root).
- `telemetry_stream` and `operator_bridge` share no direct dependency in either direction.
- The runtime composition root (`autopilot/runtime.rs`) will wire `telemetry_stream::TelemetryStreamHandle` (as `Arc<dyn TelemetrySink>`) into `OperatorBridge::with_telemetry_sink`. That wiring lives in the composition root, not in either component — no cyclic dep introduced.

PASS.

**Duplicate symbols across components**:
- `MapObjectsDiff` collision noted in Phase 5 (Low / Maintainability finding for future consolidation).
- `Poi` (shared model) vs `OperatorPoiEvent` (wire model in `shared::models::operator_event`) — intentional split; the wire model is a subset projection. No collision.
- `SessionEntry`, `HmacSha256` are private to `operator_bridge::internal::auth`. No cross-component leakage.

PASS (one Low finding for the diff name collision).

**Cross-cutting concerns**: `tracing` is the only cross-cutting concern touched. Used consistently (`warn!` for rejections in auth; the rest of the triplet adds no new logging). No bespoke logging setup.

PASS.

**Module-layout drift** (carried from cumulative 07–09 + extended this triplet):
- `telemetry_stream/src/internal/{publisher,server,proto,video,video_server,mapobjects}.rs` — `module-layout.md` predates batches 14 + 15; the actual file layout is now denser than the doc lists.
- `operator_bridge/src/internal/{auth,poi_surface}.rs` — newly added; `module-layout.md` listed only `operator_bridge/src/lib.rs` before.
- Carried as Low / Architecture (doc-sync) finding; not a code issue.

## Phase 7 — Architecture compliance (baseline delta)

Skipped — no `architecture_compliance_baseline.md` exists yet. Recommendation to promote one once the operator-side composition root (AZ-680) lands and the public API surface is more stable.

## Findings (cumulative for batches 13–15)

| # | Severity | Category | File:Line | Title |
|---|----------|----------|-----------|-------|
| 1 | Medium | Maintainability | `crates/operator_bridge/src/internal/auth.rs:191-198` | Silent `unwrap_or_default()` in `signing_material` (carry from batch 15 F1) |
| 2 | Low | Maintainability | `crates/telemetry_stream/src/internal/mapobjects.rs` + `crates/mission_client/src/lib.rs` | `MapObjectsDiff` name collision across two unrelated domains (transport vs persistence) |
| 3 | Low | Spec-Gap | `crates/operator_bridge/src/internal/poi_surface.rs:103-111` | `vlm_label` producer deferred to AZ-684 (carry from batch 15 F2) |
| 4 | Low | Architecture | `_docs/02_document/architecture.md §7.x` + `_docs/02_document/module-layout.md` | Architecture doc topic table + module-layout paths drift across batches 13–15 |
| 5 | Low | Scope | `crates/operator_bridge/src/lib.rs:120-128` | `surface_poi` returns `NotImplemented` after side-effect (placeholder for AZ-680) |

### Finding details

**F1 (cumulative): silent fallback on signing-payload serialisation** (Medium / Maintainability)
- Carried unchanged from batch 15 F1.
- Suggestion (cumulative): replace with `.expect("serde_json::Value always serialises")` so the failure mode is loud. Single-line fix; folded into AZ-680 or a tiny refactor task at next pass.

**F2 (cumulative-new): `MapObjectsDiff` name collision** (Low / Maintainability)
- Location: `crates/telemetry_stream/src/internal/mapobjects.rs` defines `MapObjectsDiff`; `crates/mission_client/src/lib.rs` also defines `MapObjectsDiff`.
- Description: the two types live in different domains (operator-link broadcast vs post-flight persistence push) and have different shapes. Both are correct in their own crate; the name collision is benign today but creates ambiguity when grepping or in IDE auto-imports.
- Suggestion: extract a shared `shared::models::mapobjects::Diff` (or two clearly-named variants — `LiveDiff` vs `PersistDiff`) and have both crates consume it. Defer to a focused dedupe task; not blocking.
- Tasks: AZ-677 + (existing) AZ-668 / AZ-685.

**F3 (cumulative): `vlm_label` producer deferred** (Low / Spec-Gap)
- Carried unchanged from batch 15 F2.
- Resolved by AZ-684.

**F4 (cumulative): doc surface table drift** (Low / Architecture)
- The Tonic gRPC infrastructure (batch 14), the video + mapobjects topics + RPCs (batch 15), the operator authentication trait + HMAC default (batch 15), and the POI surface wire format (batch 15) all need to be reflected in `_docs/02_document/architecture.md §7.x` (topic catalogue, RPC catalogue) and `_docs/02_document/module-layout.md` (per-component file list + public-API list).
- Suggestion: schedule a doc sweep covering batches 13–15 that updates:
  - `architecture.md §7.x` — topic catalogue + RPC catalogue.
  - `decision-rationale.md` — Q2 (operator-link protocol = Tonic gRPC), and a note on the snapshot-then-diff pattern for `MapObjectsBundle`.
  - `module-layout.md` — `telemetry_stream/src/internal/{video, video_server, mapobjects}.rs`, `operator_bridge/src/internal/{auth, poi_surface}.rs`.
- Tasks: batches 13–15 collectively.

**F5 (cumulative): `surface_poi` placeholder** (Low / Scope)
- Carried unchanged from batch 15 F4.
- Resolved by AZ-680.

## Verdict

**PASS_WITH_WARNINGS** — 0 Critical, 0 High, 1 Medium, 4 Low.

Per the implement skill's auto-fix matrix:
- F1 (Medium / Maintainability) → **auto-fix eligible**, single-line change. Recommendation: fold into AZ-680 or a tiny clean-up at next batch.
- F2 (Low / Maintainability, cross-crate shared-type extraction) → **schedule as a focused refactor** rather than auto-fix; touches two component public surfaces.
- F3 (Low / Spec-Gap, deferred producer) → **wait for AZ-684**.
- F4 (Low / Architecture, doc-only) → **doc-sweep ticket**.
- F5 (Low / Scope, deferred consumer) → **wait for AZ-680**.

None of the findings block batch 16 implementation. The cumulative review gate **PASSES** and the implement loop proceeds.

## Cumulative metrics

| Metric | Value (batches 13–15) | Trend vs. prior cumulative (batches 7–9) |
|--------|-----------------------|------------------------------------------|
| Total source LOC added (ex tests, approximate) | ~3,000 | – (prior was ~3,470; smaller scope but denser deps — first-time tonic stack) |
| Total test LOC added (approximate) | ~1,450 | – (prior was ~1,770) |
| Test/source ratio | ~0.48 | stable (~0.51 prior) |
| New public API symbols (approximate) | ~40 | + (prior was ~35; the operator-bridge + telemetry_stream split-out drives most of it) |
| Cyclomatic complexity hot-spots | `HmacOperatorValidator::validate` (4 sequential gates, 1 happy path), `TelemetryService::subscribe` (snapshot-prepend branch on `MapObjectsBundle`) | All under the 10-arm SOLID threshold |
| New `unsafe` blocks | 0 | stable |
| New `unwrap` / `expect` in production paths | 0 | stable |
| Layer-violation Architecture findings | 0 | stable |
| Cyclic-dep Architecture findings | 0 | stable |
| Open cumulative Mediums (cycle 1) | 2 (this triplet's F1 + carry-over C1 from cumulative 07–09 — `SendCommandError` dedupe) | + (1 new; 1 carry) |
| Open cumulative Highs (cycle 1) | 1 (C5 — pre-existing `autopilot::Runtime::vlm_provider_name` dead-code lint) | stable |

## Carried-forward cumulative findings (from prior cumulatives)

| ID | Severity | Origin | Status this triplet |
|----|----------|--------|---------------------|
| C1 | Medium | Cumulative 07–09 F1 | OPEN — `SendCommandError` mapping still duplicated across `lost_link.rs` / `geofence.rs` / `battery_thresholds.rs`. Not touched by batches 13–15. |
| C2 | Low | Cumulative 07–09 F2 | OPEN — `MavlinkCommandIssuer` naming inconsistency. Not touched by batches 13–15. |
| C3 | Low | Cumulative 07–09 F3 + extended | OPEN — `module-layout.md` drift; now extended by batches 14 + 15 to include `telemetry_stream/internal/*` + `operator_bridge/internal/*`. |
| C4 | Low | Batch 11 | OPEN — `data_model.md §PanPlan` definition still missing. |
| C5 | High | Batch 4 (pre-existing) | OPEN — workspace `-D warnings` still blocks on `autopilot::Runtime::vlm_provider_name` dead-code lint. Tracked in `_docs/_process_leftovers/2026-05-20_autopilot_clippy.md`. |
| C6 | Medium | Batch 14 | OPEN — `mission_executor::state_machine::ac3_bounded_retry_then_success` flake. Tracked in `_docs/_process_leftovers/2026-05-20_mission_executor_ac3_flake.md`. |
| C7 | Low | Batch 14 | OPEN — Tonic-gRPC decision not yet recorded in `decision-rationale.md`. Now subsumed under F4 (cumulative doc sweep). |
