[autodev] Cumulative review (batches 04-06) — PASS_WITH_WARNINGS

- 38/38 ACs covered across 12 tasks; no silent failures.
- 0 Critical, 0 High, 2 Low (drift backlog F-CUM-3 carried+
  extended; long-running soak gating F-CUM-4 — both bookkeeping
  for Phase B / Step 7).
- Phase 7: no production source mutated outside batch 4 test
  infrastructure; no new cyclic deps; F1-F9 baseline unchanged.
- LESSONS.md entry captures vi.stubGlobal('URL', ...) anti-
  pattern surfaced during AZ-476 debugging.
- Implement skill cleared to batch 7 (AZ-471, AZ-473, AZ-474,
  AZ-478, AZ-479, AZ-480 — 6 tasks remain).

Co-authored-by: Cursor <cursoragent@cursor.com>

_docs/03_implementation/cumulative_review_batches_04-06_cycle1_report.md

@@ -0,0 +1,213 @@
+# Cumulative Code Review Report
+
+**Batches**: 04–06 (12 tasks: AZ-460/462/466/475 + AZ-461/464/470/472 + AZ-463/469/476/477)
+**Date**: 2026-05-11
+**Cycle**: Phase A baseline, Step 6 — Implement Tests
+**Mode**: cumulative (`/code-review` cumulative mode, all 7 phases; emphasis on Phase 6 + 7)
+**Trigger**: implement skill Step 14.5 — every K=3 batches
+**Verdict**: **PASS_WITH_WARNINGS**
+
+## Inputs
+
+- Task specs (12) in `_docs/02_tasks/done/`:
+  AZ-460, AZ-462, AZ-466, AZ-475 (batch 4); AZ-461, AZ-464, AZ-470, AZ-472 (batch 5); AZ-463, AZ-469, AZ-476, AZ-477 (batch 6).
+- Per-batch reviews: `_docs/03_implementation/reviews/batch_0{4,5,6}_review.md` (all PASS).
+- Per-batch reports: `_docs/03_implementation/batch_0{4,5,6}_report.md`.
+- Architecture baseline: `_docs/02_document/architecture_compliance_baseline.md` (F1–F9).
+- Previous cumulative: `_docs/03_implementation/cumulative_review_batches_01-03_report.md` (PASS_WITH_WARNINGS, F-CUM-1 + F-CUM-2).
+
+## Scope (changed files since the previous cumulative review)
+
+Union across batches 4 + 5 + 6 — 31 distinct paths:
+
+- `tests/**` (12 created): `destructive_ux.test.tsx`, `form_hygiene.test.tsx`, `overlay_membership.test.tsx`, `annotations_endpoint.test.tsx`, `bulk_validate.test.tsx`, `detection_classes.test.tsx`, `detection_endpoints.test.tsx`, `panel_width_persistence.test.tsx`, `browser_support_responsive.test.tsx`, `flight_selection_persistence.test.tsx`, `settings_resilience.test.tsx`, `upload_size_cap.test.tsx`.
+- `tests/**` (3 modified): `setup.ts` (JSDOM polyfills), `msw/handlers/annotations.ts` (doubly-prefixed paths), `msw/handlers/flights.ts` (plural `aircrafts`), `security/banned-deps.json` (alert allowlist + destructive surfaces).
+- `src/**` (1 created): `components/ConfirmDialog.test.tsx`.
+- `e2e/**` (10 created): one companion per fast file in batches 4 + 5 + 6 except `form_hygiene` and `overlay_membership` (intentionally fast-only per their specs).
+- `scripts/**` (2 modified): `check-banned-deps.mjs` (added `alert_calls` + `destructive_surfaces` kinds; later modified for AZ-466 / AZ-482 routing), `run-tests.sh` (added `STC-SEC7` + `STC-SEC8`).
+- `_docs/**` (created): `LESSONS.md`; per-batch reports + reviews; renamed task specs `todo/` → `done/`; `_autodev_state.md` updated each batch.
+
+## Phase 1 — Context
+
+All 12 task specs re-read end-to-end; baseline + module-layout envelopes (`Blackbox Tests` `Owns` glob = `tests/**` + `e2e/**` + `src/**/*.test.{ts,tsx}` + selected static-check artifacts) remain the only OWNED scope of these batches.
+
+## Phase 2 — Spec Compliance
+
+| Batch | ACs covered | Drift markers | Quarantines | Notes |
+|-------|-------------|---------------|-------------|-------|
+| 04 | 13 / 13 | 6 `it.fails()` + 4 `test.fail` | 4 `it.skip` (focus trap; AI-suggestion-accept; bulk-edit-save; structural placeholders) | All drift carries a control test |
+| 05 | 13 / 13 | 6 `it.fails()` + 3 `test.fail` | AC-2 FT-P-12 quarantine inside `it.fails()` per AZ-461 spec direction | All drift carries a control test |
+| 06 | 12 / 12 | 6 `it.fails()` + 4 `test.fail` | 2 long-running soaks gated by `RUN_LONG_RUNNING=1` env flag | All drift carries a control test |
+
+**Total: 38 / 38 ACs covered** across the three batches. No silent failures. Every `it.fails()` placement either anchors to an explicit task-spec QUARANTINE direction, paired control test, or both.
+
+## Phase 3 — Code Quality
+
+Spot-checks across new files:
+
+- Test bodies follow Arrange / Act / Assert with the language-appropriate `// Arrange` / `// Act` / `// Assert` markers (AAA explicit per `coderule.mdc`).
+- Comments document drift rationale (`// Drift: ...`), QUARANTINE reasons, and `it.fails()` flip conditions — never narrate code.
+- No `console.log` / `console.error` left behind; the AZ-476 debug instrumentation that uncovered the URL stub bug was fully removed before commit (verified against the final committed `tests/upload_size_cap.test.tsx`).
+- `tests/settings_resilience.test.tsx` installs a scoped `process.on('unhandledRejection')` handler that swallows ONLY the documented drift signature (`500: upstream failure` and network-error patterns); any other rejection rethrows. Same posture the production code will adopt once try/finally lands; enforced at the test boundary in the meantime.
+- `tests/upload_size_cap.test.tsx` patches `URL.createObjectURL` / `URL.revokeObjectURL` directly on the URL constructor with full restore in `afterEach`. The `LESSONS.md` entry captures the alternative anti-pattern (`vi.stubGlobal('URL', { ...URL, ... })`) so future sessions don't reintroduce it.
+
+No Phase 3 findings.
+
+## Phase 4 — Security
+
+- AZ-466 + AZ-482 (batches 3–4 boundary) introduce a **closed-loop guard** for `alert()` and destructive surfaces — every `alert()` call site is enumerated in `tests/security/banned-deps.json::alert_calls`, every destructive UI is tagged `gated` or `drift` in `destructive_surfaces`. New `STC-SEC7` / `STC-SEC8` static checks fail-closed on additions. AZ-476 reaffirms the `alert()` prohibition for the 413 path (PASS today vacuous; e2e dialog spy adds runtime defence).
+- No new fixture secrets across the three batches (`'test-bearer-default'` constant is reused; placeholder argon2 hashes only).
+- AZ-477 unhandled-rejection swallowing is **scope-narrowed** by message pattern; cannot mask unrelated rejections.
+- AZ-476 `URL.createObjectURL` patching restores the original constructor in `afterEach`; cannot leak across tests.
+
+No Phase 4 findings.
+
+## Phase 5 — Performance
+
+| Batch | Fast files | Fast tests | Fast wall-clock | Static checks |
+|-------|-----------|------------|-----------------|---------------|
+| 04 | 14 | 80 + 13 skipped | ~5.5 s | 24 (was 22 in batch 3) |
+| 05 | 18 | 102 + 13 skipped | ~7.31 s | 24 |
+| 06 | 22 | 120 + 13 skipped | ~46.5 s (warm setup; 173 s setup time on the cumulative run) | 24 |
+
+The batch-6 wall-clock jump is dominated by `it.fails()` polling for elements that never appear (drift): AZ-477's six contract tests each wait the full 2 000 ms `findByRole('alert')` budget, plus the `userEvent.click` interaction setup. This is **expected** test-side cost given the spec; once the production try/finally + alert region land, the same tests will short-circuit on a found alert and the suite returns to the ~5–7 s envelope.
+
+No Phase 5 findings — but log this for the Phase B planning: lifting AZ-477's drifts produces a measurable ~30–40 s suite speedup on its own.
+
+## Phase 6 — Cross-Batch Consistency
+
+### Symbol audit (across batches 4 + 5 + 6)
+
+- `tests/helpers/{auth,render,navigate,sse-mock}.ts` — single definition each; consumed by every batch.
+- `tests/fixtures/seed_*.ts` — seeded by AZ-456 (batch 1); reused **without redefinition**. Spot-checked `seedFlights`, `seedAircraft`, `seedUserSettings`, `seedUsers` — same IDs, same shape across all consumers in batches 4–6.
+- `FlightProvider` import path is consistently `'../src/components/FlightContext'` in every test that needs it.
+- `STC-*` IDs across `scripts/run-tests.sh`: 24 unique identifiers, none reused. `STC-SEC7` (alert-allowlist) and `STC-SEC8` (destructive-surfaces) added in batch 4; not modified by batches 5–6.
+- MSW handler routes: each handler file owns a disjoint URL prefix; `tests/msw/handlers/annotations.ts` and `tests/msw/handlers/flights.ts` were extended (not replaced) in batch 4 to add the doubly-prefixed and plural shapes that production uses. Backward compatibility for the single-prefix shape was preserved.
+
+**No duplicate symbol** across the three batches. **No fixture redefinition** across consumers.
+
+### Drift handling pattern uniformity (across all 6 batches)
+
+- `it.fails()` — production element exists, asserted attribute / behavior is missing today.
+- `it.skip` + `// QUARANTINE: ...` — production capability is wholly absent.
+- `test.fail` (e2e) — drift mirror; flips the moment production lands the contract.
+- Every drift is paired with a control PASS test pinning the current shape so the gap is observable today.
+
+This pattern is now uniform across all 6 batches. Batch 6 introduces no new pattern variations.
+
+### Test infrastructure mutation discipline
+
+- `tests/security/banned-deps.json` extended only by adding new sections (`alert_calls`, `destructive_surfaces`); existing sections never edited in place.
+- `scripts/check-banned-deps.mjs` extended only by adding new `--kind=` branches; the shared `checkSourceTree` matcher and the JSON loader are unchanged.
+- `tests/setup.ts` extended only by adding JSDOM polyfills behind `if (!g.X)` guards; no global mutation that wasn't conditional.
+
+No Phase 6 findings beyond the pattern uniformity record above.
+
+## Phase 7 — Architecture Compliance
+
+### Cross-component import audit (12 new test files in batches 4–6)
+
+| Test file | Cross-component imports | Verdict |
+|-----------|-------------------------|---------|
+| `tests/destructive_ux.test.tsx` | `AdminPage` (default) + helpers + fixtures | OK |
+| `tests/form_hygiene.test.tsx` | `SettingsPage` (default) + helpers | OK |
+| `tests/overlay_membership.test.tsx` | `CanvasEditor` (default) + public enums | OK |
+| `tests/annotations_endpoint.test.tsx` | `AnnotationsPage` + `FlightProvider` + public enums | OK |
+| `tests/bulk_validate.test.tsx` | `DatasetPage` + helpers | OK |
+| `tests/detection_classes.test.tsx` | `DetectionClasses` (default) + helpers | OK |
+| `tests/detection_endpoints.test.tsx` | `AnnotationsPage` + `FlightProvider` + helpers | OK |
+| `tests/panel_width_persistence.test.tsx` | `useResizablePanel` (public hook) + minimal harness components | OK |
+| `tests/browser_support_responsive.test.tsx` | `Header` + helpers | OK |
+| `tests/flight_selection_persistence.test.tsx` | `Header` + `FlightProvider` + helpers | OK |
+| `tests/settings_resilience.test.tsx` | `SettingsPage` + helpers | OK |
+| `tests/upload_size_cap.test.tsx` | `AnnotationsPage` + `FlightProvider` + `useFlight` (consumer hook) + helpers | OK — `useFlight` is a documented public hook on `FlightContext` |
+| `src/components/ConfirmDialog.test.tsx` | `ConfirmDialog` (default) | OK — colocated with source |
+
+- **No imports of `*.internal.*`**.
+- **No new cyclic module dependencies** (verified via `bunx tsc --noEmit -p tsconfig.test.json` + `bun run build` in `STC-T1` / `STC-B1`).
+- **No production source mutated** in batches 5 + 6. Batch 4 mutated only test infrastructure (handlers, polyfills, banned-deps deny-list, run-tests script). The Public API surface of every imported component remains backwards compatible.
+- **`STC-S6`** (no WS / GraphQL / gRPC / SSR libs) and **`STC-S13`** (no client-side persistence libs) re-confirm across all 24 checks for batches 4 + 5 + 6.
+
+### Baseline Delta
+
+Comparing current findings to `_docs/02_document/architecture_compliance_baseline.md`:
+
+**Carried over** — present at baseline, still present (unchanged from cumulative 01–03):
+
+| # | File | Category | Rule |
+|---|------|----------|------|
+| F1 | `mission-planner/**` vs `src/features/flights/**` | Architecture | Convergence-pending duplication |
+| F2 | `src/features/dataset/DatasetPage.tsx:9` | Architecture | Cross-feature same-layer edge |
+| F3 | `src/features/annotations/classColors.ts` | Architecture | Physical/logical owner split |
+| F4 | every component | Architecture | No Public API barrels |
+| F5 | `mission-planner/src/flightPlanning/{MapView,MiniMap}.tsx` | Architecture | Pre-existing cycle inside port-source |
+| F6 | codebase-wide | Architecture | No `src/shared/` |
+| F7 | `api.*` / `createSSE` call sites | Architecture | Hardcoded `/api/<service>/...` |
+| F8 | `_docs/02_document/module-layout.md` | Architecture | Layering-table inconsistency |
+| F9 | `mission-planner/src/{main,App,setupTests,vite-env}.tsx` | Architecture | Inert second Vite entry tree |
+
+**Resolved**: none in scope. The baseline issues belong to Step 8 Refactor or Phase B feature cycles.
+
+**Newly introduced**: none. Every architecture rule observed (test files leaf-only in the import graph; no cyclic deps; no shared layer reimplemented; no Public API regressions).
+
+## Findings (cumulative)
+
+### F-CUM-3 — Production drift backlog has grown to 18 items (Low / Maintainability / cumulative)
+
+Carries forward F-CUM-1 from cumulative 01–03 (11 items) and adds the new drifts from batches 4 + 5 + 6:
+
+| # | Source AC / scenario | Production file | Phase B touchpoint |
+|---|----------------------|-----------------|--------------------|
+| 12 | AZ-466 AC-1 — ConfirmDialog `role/aria-modal/aria-labelledby/aria-describedby` + focus trap | `src/components/ConfirmDialog.tsx` | a11y attrs + focus trap (one file) |
+| 13 | AZ-466 AC-2 — AdminPage class-delete bypasses ConfirmDialog | `src/features/admin/AdminPage.tsx` | wire class-delete through ConfirmDialog |
+| 14 | AZ-466 AC-3/AC-5 — 4-entry `alert()` allowlist drained over time | `MediaList.tsx`, `FlightsPage.tsx`, `JsonEditorDialog.tsx`, `flightPlan.tsx` | one task per call site |
+| 15 | AZ-475 AC-1 — silent-zero coercion in numeric inputs + missing `htmlFor` | `src/features/settings/SettingsPage.tsx` | combined input-validation hook + label association |
+| 16 | AZ-462 AC-1 — strict `<` vs inclusive boundary in `getTimeWindowDetections` | `src/features/annotations/CanvasEditor.tsx` (or its helper) | one-character `<` → `<=` |
+| 17 | AZ-460 AC-2 — annotation save body missing 4 fields | `src/features/annotations/AnnotationsPage.tsx` save handler | wire-contract update |
+| 18 | AZ-460 AC-3 — AI-suggestion-accept and bulk-edit-save entry points absent | same | 2 Phase B tasks |
+| 19 | AZ-461 AC-2 (FT-P-12) — async-video detect endpoint + SSE absent (QUARANTINE) | `src/features/annotations/AnnotationsSidebar.tsx` Detect handler | unblocks with AC-25 |
+| 20 | AZ-461 AC-3 — `X-Refresh-Token` header missing on detect | `src/api/client.ts` | header wiring |
+| 21 | AZ-464 AC-2 — bulk-validate body shape `{annotationIds, status}` vs contract `{ids, targetStatus:30}` | `src/features/dataset/DatasetPage.tsx` | wire-contract update |
+| 22 | AZ-470 AC-1/AC-2/AC-3 — `useResizablePanel` has no PUT writer / no rehydration reader | `src/hooks/useResizablePanel.ts` | full Phase B remediation |
+| 23 | AZ-472 AC-2 — hotkey index `classes[idx + photoMode]` against dense array (P=20 / P=40 fail) | `src/components/DetectionClasses.tsx` | filter-then-index OR sparse length-60 fixture |
+| 24 | AZ-476 AC-1 — 413 silently swallowed in `MediaList.uploadFiles` | `src/features/annotations/MediaList.tsx` | toast + i18n key for the 413 path |
+| 25 | AZ-477 AC-1/AC-2 — `saveSystem` / `saveDirs` lack try/finally and an error region | `src/features/settings/SettingsPage.tsx` | try/finally + role="alert" region |
+| 26 | AZ-477 AC-3 — 2 s deadline unmeasurable today (depends on #25) | same | resolves with #25 |
+
+**Recommendation**: same as F-CUM-1 — file these as Phase B feature tasks during Step 9 (New Task) once Phase A baseline closes. None are blocking for Step 6 (test infrastructure) or Step 7 (test execution). Several share files (`AnnotationsPage.tsx`, `SettingsPage.tsx`, `MediaList.tsx`) and could be combined for review efficiency.
+
+This is a **non-blocking** finding; verdict contribution = PASS_WITH_WARNINGS only.
+
+### F-CUM-4 — Long-running soak gating is env-flag-only (Low / Maintainability / batch 6)
+
+AZ-463 AC-3 + AC-4 e2e companions are gated by `process.env.RUN_LONG_RUNNING === '1'`. The task spec explicitly calls for **playwright-config-level tagging**: "Long-running tests (NFT-RES-LIM-06, 07) tagged `@long-running` in the Playwright config; CI only runs them on `dev`/`stage` merges, not on every commit." The current implementation skips inside the test body when the flag is absent — functional but not what the spec describes.
+
+**Recommendation**: when CI lanes are configured (Step 7 / Phase B), update `e2e/playwright.config.ts` to add a `grep` / `grepInvert` filter for `@long-running` and rename the affected test titles to carry the tag. Until then, the env-flag gate is acceptable; the soak tests are NOT blocking PR commits today.
+
+This is a **non-blocking** finding; verdict contribution = PASS_WITH_WARNINGS only.
+
+## Auto-Fix Attempts: 0
+
+No findings escalate to Auto-Fix. F-CUM-3 + F-CUM-4 are bookkeeping for Phase B / Step 7.
+
+## Stuck Agents
+
+One AZ-476 investigation in batch 6 traversed several hypotheses before instrumenting fetch and discovering the `vi.stubGlobal('URL', ...)` constructor-destruction bug. The retro is captured in `_docs/LESSONS.md`. No process improvement gap — the debug-over-contemplation rule fired correctly (the agent stopped speculating after 3 hypotheses and added runtime instrumentation).
+
+## Verdict: PASS_WITH_WARNINGS
+
+Reason: 0 Critical / 0 High; 2 Low / Maintainability findings (F-CUM-3 production-drift bookkeeping; F-CUM-4 long-running-soak gating mechanism). Implement skill may proceed to batch 7.
+
+## Next Batch Recommendation
+
+6 tasks remain in `todo/`:
+- AZ-471 (CanvasEditor draw/resize/multi-select/zoom/pan, 5 pts)
+- AZ-473 (PhotoMode switch + auto-select + yoloId wire, 2 pts) — soft dep on AZ-472 (✓ done)
+- AZ-474 (Tile-split + YOLO parser + auto-zoom + indicator, 3 pts)
+- AZ-478 (Network offline + SSE disconnect + tainted-canvas, 3 pts)
+- AZ-479 (Bundle ≤ 2 MB + mission-planner excluded + FCP + soak, 3 pts)
+- AZ-480 (Prod image nginx:alpine + 500M + 9 routes + edge RAM, 3 pts)
+
+Suggested batch 7 (4 tasks, ~13 pts, dependency-disjoint at the file level): **AZ-471 (5) + AZ-473 (2) + AZ-478 (3) + AZ-479 (3) = 13 pts**. AZ-471 is the heaviest remaining task; pairing it with the lighter / deployment-touching items keeps the batch bounded. AZ-474 (tile-split) + AZ-480 (prod nginx image) form a natural batch 8.
+
+No cumulative-review-gated changes need to be applied before batch 7 starts.

_docs/_autodev_state.md

@@ -6,9 +6,9 @@ step: 6
 name: Implement Tests
 status: in_progress
 sub_step:
-  phase: 14.5
-  name: cumulative-review-due
-  detail: "batch 6 complete; cumulative 04-06 due"
+  phase: 14
+  name: batch-loop
+  detail: "cumulative 04-06 PASS_WITH_WARNINGS; 6 tasks remain"
 retry_count: 0
 cycle: 1
 tracker: jira
@@ -24,7 +24,7 @@ step_3_ac_gap_handling: rollback-to-6c (option A)
   `_docs/00_problem/{problem,acceptance_criteria,restrictions,security_approach}.md`.
 - Implement-skill batch reports at
   `_docs/03_implementation/batch_0{1,2,3,4,5,6}_report.md`.
-- Cumulative review (batches 01-03) PASS_WITH_WARNINGS at
-  `_docs/03_implementation/cumulative_review_batches_01-03_report.md`.
-  Next cumulative review (batches 04-06) due now per `implement/SKILL.md`
-  Step 14.5 (K=3 cadence).
+- Cumulative reviews PASS_WITH_WARNINGS at
+  `_docs/03_implementation/cumulative_review_batches_01-03_report.md` and
+  `_docs/03_implementation/cumulative_review_batches_04-06_cycle1_report.md`.
+  Next cumulative review due after batch 9 (covers batches 07-09).