# Code Review Report

**Batch**: 5 — AZ-461, AZ-464, AZ-470, AZ-472
**Date**: 2026-05-11
**Verdict**: PASS
**Mode**: Full (per-batch invocation by `/implement`)

## Inputs

- Task specs:
  - `_docs/02_tasks/todo/AZ-461_test_detection_endpoints.md` (3 ACs, 2 pts)
  - `_docs/02_tasks/todo/AZ-464_test_bulk_validate.md` (3 ACs, 2 pts)
  - `_docs/02_tasks/todo/AZ-470_test_panel_width_persistence.md` (3 ACs, 2 pts)
  - `_docs/02_tasks/todo/AZ-472_test_detection_classes.md` (4 ACs, 3 pts)
- Changed files (8 total, all under Blackbox Tests OWNED scope):
  - `tests/detection_endpoints.test.tsx`
  - `tests/bulk_validate.test.tsx`
  - `tests/panel_width_persistence.test.tsx`
  - `tests/detection_classes.test.tsx`
  - `e2e/tests/detection_endpoints.e2e.ts`
  - `e2e/tests/bulk_validate.e2e.ts`
  - `e2e/tests/panel_width_persistence.e2e.ts`
  - `e2e/tests/detection_classes.e2e.ts`

## Findings

| # | Severity | Category | File:Line | Title |
|---|----------|----------|-----------|-------|
| — | — | — | — | None |

No Critical, High, Medium, or Low findings.

## Phase Walkthrough

### Phase 1 — Context Loading

All 4 task specs read; ACs catalogued; module-layout.md consulted for OWNED/READ-ONLY/FORBIDDEN envelope. Every changed file falls under `tests/**` or `e2e/**`, both `Owns` globs of the `Blackbox Tests` cross-cutting component (epic AZ-455). No file outside the envelope was modified.

### Phase 2 — Spec Compliance

| Task | AC | Test | Today | Drift documented |
|------|----|------|-------|------------------|
| AZ-461 | AC-1 (FT-P-11 sync image URL) | `tests/detection_endpoints.test.tsx` | PASS | — |
| AZ-461 | AC-2 (FT-P-12 async video, QUARANTINE) | `it.fails()` + control | runs + emits "FT-P-12 awaits AC-25" log | spec mandates QUARANTINE marker |
| AZ-461 | AC-3 (FT-P-13 X-Refresh-Token header) | `it.fails()` + control | drift — production sets only Authorization | header wired in Phase B |
| AZ-464 | AC-1 (FT-P-20 URL) | `tests/bulk_validate.test.tsx` | PASS | — |
| AZ-464 | AC-2 (FT-P-20 body shape) | `it.fails()` + control | drift — `{annotationIds, status:2}` vs contract `{ids, targetStatus:30}` | flips with AC-04 wire enum |
| AZ-464 | AC-3 (FT-P-21 + NFT-PERF-07 ≤ 2 s) | wall-clock perf assertion | PASS | — |
| AZ-470 | AC-1 (FT-P-37 + NFT-PERF-08 debounce) | `it.fails()` + control | drift — `useResizablePanel` has no PUT writer | flips when PUT writer wired |
| AZ-470 | AC-2 (FT-P-37 body) | `it.fails()` | drift — depends on AC-1 writer | flips when writer wired |
| AZ-470 | AC-3 (FT-P-38 rehydration) | `it.fails()` + control | drift — no read of `panelWidths` from settings | flips with rehydration effect |
| AZ-472 | AC-1 (FT-P-44 load) | `tests/detection_classes.test.tsx` | PASS | — |
| AZ-472 | AC-2 P=0 (FT-P-45 hotkey) | direct assertion | PASS | — |
| AZ-472 | AC-2 P=20 (FT-P-45 hotkey) | `it.fails()` | drift — `classes[idx+P]` against dense array | flips with filter-then-index OR sparse array |
| AZ-472 | AC-2 P=40 (FT-P-45 hotkey) | `it.fails()` | drift — `classes[idx+40]` exceeds length | same as P=20 |
| AZ-472 | AC-3 (FT-P-46 click) | userEvent.click | PASS | — |
| AZ-472 | AC-4 (FT-P-47 fallback) | empty + 500 + id-set test | PASS | — |

Every AC has at least one test (running or `it.fails()` per spec direction). AC-2 and AC-3 of AZ-461 explicitly require running tests with documented drift markers — both satisfied. All `it.fails()` markers have inline justification anchored to a documented production behavior, with control tests pinning the current shape so a regression does not slip through silently.

No `Spec-Gap` findings.

### Phase 3 — Code Quality

- AAA pattern (`// Arrange / // Act / // Assert`) applied throughout, with sections elided where empty per `coderule.mdc` test convention.
- No bare catch / no error suppression. Every test uses MSW handlers + `seedBearer/clearBearer` deterministically.
- Helper functions (`captureDetectAndBootstrap`, `rigDatasetAndBulk`, `rigPanelEnv`, `captureClassesGets`) under 50 lines each; named for caller intent.
- No DRY violations across the batch — each task isolated; the only shared helper is `tests/helpers/auth` which already existed.
- `it.fails()` placements match documented drift. Comments explain *why* and *when each test flips green*, never narrating *what the code does*.

No findings.

### Phase 4 — Security Quick-Scan

- No SQL, no shell exec, no eval/new Function in any test.
- `seedBearer()` uses test-fixture token; no hardcoded production secrets.
- No sensitive data in logs (`console.log` exists in only one place — the AZ-461 AC-2 quarantine marker, mandated by spec).

No findings.

### Phase 5 — Performance Scan

- `waitFor` timeouts bounded (1000–3000 ms); no infinite waits.
- No N+1 patterns. `selectItemsWithCtrlClick` iterates the bounded `seedItems` (3 rows).
- Fake-timer use in `tests/panel_width_persistence.test.tsx` is correct (`shouldAdvanceTime: true`) and reset in `afterEach`.
- Wall-clock perf assertion (`elapsed ≤ 2000 ms`) for AC-3 of AZ-464 / NFT-PERF-07 measured from click time, not request-receipt time — slightly stricter than spec, which is fine.

No findings.

### Phase 6 — Cross-Task Consistency

- All 4 fast tests share the same scaffolding shape: `server.use(...)`, `seedBearer()`, `renderWithProviders`, AAA structure, `clearBearer()`.
- No conflicting MSW patterns; each task's handler block is self-contained and uses the same `paginate` / `jsonResponse` / `errorResponse` helpers from `tests/msw/helpers`.
- All 4 tasks declare `Dependencies: AZ-456_test_infrastructure`, which is satisfied (test infra was completed in earlier batches).
- E2E companions follow the established Playwright pattern (`page.route` interception + `test.fail()` for known drifts + `test.skip(...)` for seed gaps).

No findings.

### Phase 7 — Architecture Compliance

- Layer direction: every import in the batch flows leaf-ward (test → production); no upstream production code added or modified.
- Public API respect: imports from `src/types`, `src/components/FlightContext`, `src/components/DetectionClasses`, `src/features/annotations/AnnotationsPage`, `src/features/annotations/classColors`, `src/features/dataset/DatasetPage`. Per `module-layout.md` Public API tables, all five are de-facto Public API entries of their owning components. Static profile (STC-S6, STC-S13) passes against the same rule set.
- No new cyclic dependencies — tests are leaves of the import graph.
- No duplicate symbols across components — each task's test helpers are file-private.
- No cross-cutting concerns re-implemented locally — all logging goes through `console.log` only at the spec-mandated AZ-461 AC-2 quarantine marker.

No findings.

## Baseline Delta

`_docs/02_document/architecture_compliance_baseline.md` does not exist for this workspace — no baseline delta to compute.

## Verdict Logic

- 0 Critical findings
- 0 High findings
- 0 Medium findings
- 0 Low findings

→ **PASS**

## Notes

- The batch is test-only. No production source was modified. Every `it.fails()` is paired with documented drift evidence in the task spec or in the test file's header comment.
- `bun run test:fast` — 18 files / 102 passed / 13 skipped (pre-existing skip count unchanged).
- `./scripts/run-tests.sh --static-only` — all checks PASS.
- No new lint errors introduced (ReadLints clean on all 8 changed files).

## Outputs (for /implement)

- `verdict`: PASS
- `findings`: []
- `critical_count`: 0
- `high_count`: 0
- `report_path`: `_docs/03_implementation/reviews/batch_05_review.md`