From c16c9d8bbbb6d01791f512cc1fa25781e0021c89 Mon Sep 17 00:00:00 2001 From: Oleksandr Bezdieniezhnykh Date: Mon, 11 May 2026 06:15:46 +0300 Subject: [PATCH] [AZ-455] Cumulative review batches 07-08 (cycle close, PASS_WITH_WARNINGS) K=3 cadence cumulative review for the final 2 batches of Phase A. Verdict: PASS_WITH_WARNINGS (0 Critical / 0 High; F-CUM-5 lifts production-drift backlog to 23 entries; F-CUM-4 long-running soak tagging carries over). Phase A is now COMPLETE: 25 test tasks delivered across 8 batches; 0 production source mutations; 26/26 ACs covered in this window; 100% cumulative AC coverage; 29 commit-time static gates active. Next autodev action: Step 7 (Run Tests). Co-authored-by: Cursor --- ...tive_review_batches_07-08_cycle1_report.md | 203 ++++++++++++++++++ _docs/_autodev_state.md | 11 +- 2 files changed, 209 insertions(+), 5 deletions(-) create mode 100644 _docs/03_implementation/cumulative_review_batches_07-08_cycle1_report.md diff --git a/_docs/03_implementation/cumulative_review_batches_07-08_cycle1_report.md b/_docs/03_implementation/cumulative_review_batches_07-08_cycle1_report.md new file mode 100644 index 0000000..840f0e7 --- /dev/null +++ b/_docs/03_implementation/cumulative_review_batches_07-08_cycle1_report.md @@ -0,0 +1,203 @@ +# Cumulative Code Review Report + +**Batches**: 07–08 (6 tasks: AZ-471 / AZ-473 / AZ-478 / AZ-479 + AZ-474 / AZ-480) +**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; **closes the cycle** (only 2 batches in this window because Phase A ends at batch 8 — there is no batch 9) +**Verdict**: **PASS_WITH_WARNINGS** + +## Inputs + +- Task specs (6) in `_docs/02_tasks/done/`: + AZ-471, AZ-473, AZ-478, AZ-479 (batch 7); AZ-474, AZ-480 (batch 8). +- Per-batch reviews: `_docs/03_implementation/reviews/batch_0{7,8}_review.md` (both PASS). +- Per-batch reports: `_docs/03_implementation/batch_0{7,8}_report.md`. +- Architecture baseline: `_docs/02_document/architecture_compliance_baseline.md` (F1–F9). +- Previous cumulative: `_docs/03_implementation/cumulative_review_batches_04-06_cycle1_report.md` (PASS_WITH_WARNINGS, F-CUM-3 + F-CUM-4). + +## Scope (changed files since the previous cumulative review) + +Union across batches 7 + 8 — 9 distinct paths: + +- `tests/**` (3 created): `canvas_editor.test.tsx`, `photo_mode.test.tsx`, `network_resilience.test.tsx`, `tile_split_zoom.test.tsx` (4 files). +- `e2e/**` (5 created): `canvas_bbox.e2e.ts`, `photo_mode.e2e.ts`, `network_resilience.e2e.ts`, `perf_fcp.e2e.ts`, `perf_annotation_memory_soak.e2e.ts`, `tile_split_zoom.e2e.ts`, `prod_image_nginx_ram.e2e.ts` (7 files; the `prod_image_nginx_ram.e2e.ts` is the largest, exercising the running prod image via docker stats). +- `scripts/**` (1 modified): `run-tests.sh` — 5 new `static_check_*` functions promoted to per-commit static checks (`STC-PERF01` in batch 7; `STC-RES02` / `STC-RES03` / `STC-RES09` / `STC-RES10` in batch 8). +- `_docs/**` (created): per-batch reports + reviews; renamed task specs `todo/` → `done/`; `_autodev_state.md` updated each batch. + +**No production source mutated** in batches 7 + 8. Test infrastructure mutations are scoped to: 1 batch-7 lesson follow-up in `tests/setup.ts` (Image stub + serviceWorker stub patterns already landed in batch 6), and 4 new commit-time static gates added behind their own helper functions in `scripts/run-tests.sh`. + +## Phase 1 — Context + +All 6 task specs re-read end-to-end. The OWNED scope (`Blackbox Tests` envelope per `_docs/02_document/module-layout.md`) remains `tests/**` + `e2e/**` + `src/**/*.test.{ts,tsx}` + selected static-check artefacts (`scripts/run-tests.sh`, `tests/security/banned-deps.json`). Both batches stayed strictly inside the envelope. `nginx.conf` and `Dockerfile` are READ-ONLY for AZ-480 (their contents are the system under test). + +## Phase 2 — Spec Compliance + +| Batch | ACs covered | Drift markers | Quarantines / gates | Notes | +|-------|-------------|---------------|---------------------|-------| +| 07 | 15 / 15 | 7 `it.fails()` + 4 `test.fail` | AC-3 (FCP) + AC-4 (memory soak) e2e gated to suite-e2e + `RUN_LONG_RUNNING=1` | AZ-471 AC-3/4/5 + AZ-478 AC-1/2/3 → drift; AZ-473 + AZ-479 PASS today | +| 08 | 11 / 11 | 7 `it.fails()` + 2 `test.fail` | AZ-480 e2e: 1 docker-availability gate + 1 RAM-soak gate (`RUN_LONG_RUNNING=1`) | AZ-474 entirely drift (split surface QUARANTINED per D11); AZ-480 all 5 ACs PASS today (4 static + 1 e2e gated) | + +**Total: 26 / 26 ACs covered** across the two 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 the new files: + +- AAA structure preserved on every `*.test.tsx` body. `// Arrange` / `// Act` / `// Assert` markers present where setup is non-trivial; omitted (per `coderule.mdc`) when the act+assert are a single line. +- Drift comments document the production fix that flips the test (`Drift: ...` → `Resolves when: ...`). Quarantine markers cite the deferral row by ID (`D11`). +- No `console.log` / `console.error` introduced in the new test bodies. +- `tests/network_resilience.test.tsx` uses the URL-constructor patch pattern from the AZ-476 lesson (`URL.createObjectURL` and `URL.revokeObjectURL` set directly on the constructor, then restored in `afterEach`). The cumulative-04-06 lesson is now a re-applied pattern, not a new finding. +- `scripts/run-tests.sh` keeps each new static check in its own single-responsibility shell function. The most complex one (`static_check_nginx_prefix_strip`) delegates to `node -e` because the conditional "proxy_pass with trailing slash OR rewrite" logic is much clearer in JS than awk; the threshold (every /api/* block has at least one of the two patterns within its block-scope) is explicit in the script. `node` is already a hard dep of the static profile (used by 3 prior `check-*.mjs` scripts), so no new toolchain. +- `e2e/tests/prod_image_nginx_ram.e2e.ts` uses `docker run -d --rm -p 0:80 ${IMAGE}` so the container picks an ephemeral port; the test does not require port 80 free on the runner. + +No Phase 3 findings. + +## Phase 4 — Security + +- No new fixture secrets across the two batches (`'test-bearer-default'` constant reused; placeholder argon2 hashes only). +- `tests/network_resilience.test.tsx` blocks ALL `/api/*` requests at the MSW boundary (`http.all('/api/*', () => HttpResponse.error())`) — the offline simulation is fully self-contained; no real network egress possible. +- `e2e/tests/prod_image_nginx_ram.e2e.ts` shells out to `docker exec ${id} which node` and `docker stats ${id}`. Both invocations interpolate only a docker-issued container ID (returned by `docker run`) — no user-controllable interpolation. The `${IMAGE}` env var (default `azaion/ui:test`) flows into the `docker run` command line; in CI/dev environments where the env is trusted, this is acceptable. Adding shell-escape would not change behaviour for the documented happy path; flagged as informational only. +- `STC-RES03` (Dockerfile `nginx:alpine` no Node) and `STC-RES10` (prefix-strip on every /api/* route) are defence-in-depth gates that catch supply-chain regressions at commit time — no longer opt-in. +- `tests/setup.ts` MSW boundary (`onUnhandledRequest: 'error'`) is preserved; the AZ-474 fast suite adds two narrowly-scoped `beforeEach` handlers (`/api/admin/auth/refresh` → 401 and `/api/annotations/settings/user` → 404) so the AuthProvider + FlightProvider mounts complete without leaking unhandled-request errors. + +No Phase 4 findings. + +## Phase 5 — Performance + +| Batch | Fast files | Fast tests | Fast wall-clock | Static checks | Static wall-clock | +|-------|-----------|------------|-----------------|---------------|-------------------| +| 07 | 25 | 150 + 13 skipped | ~16.0 s | 25 (was 24 in batch 6) | ~13 s | +| 08 | 26 | 163 + 13 skipped | ~16.4 s | 29 (was 25 in batch 7) | ~13 s | + +- The cumulative wall-clock envelope is stable across the two batches; the 13 new tests in batch 8 add ≤0.5 s end-to-end (most are PASS controls; the `it.fails()` drift assertions short-circuit via the `findByX` 1500 ms timeout but only one such timeout per AC). +- The four new static checks added in batch 8 collectively run in ~150 ms (`grep`-only checks complete in <30 ms each; the `node -e` prefix-strip parser is the slowest at ~80 ms). Static profile total wall-clock unchanged at ~13 s — dominated by `STC-T1` (`tsc --noEmit`) + `STC-B1` (`vite build`). +- The MSW handler set has not grown in batches 7–8; the batch-7 / batch-8 tests reuse existing handlers via `server.use(...)` overrides scoped to `beforeEach` — no leak across tests. +- The e2e profile gains 7 new files; suite-e2e wall-clock is dominated by container boot (~30 s) and is unaffected by the new test count beyond per-test setup. AC-3 (FCP) is the longest measured-test at ~30 s (warmup + 5 navigations); AC-4 (memory soak) runs 30 min only when `RUN_LONG_RUNNING=1`. AZ-480 RAM soak runs 5 min only when `RUN_LONG_RUNNING=1`. Neither gates the per-PR e2e lane. + +No Phase 5 findings. + +## Phase 6 — Cross-Batch Consistency + +### Symbol audit (across batches 7 + 8) + +- `tests/helpers/{auth,render,navigate,sse-mock}.ts` — single definition each; consumed by both batches without re-export. +- `tests/fixtures/seed_*.ts` — seeded by AZ-456 (batch 1); reused **without redefinition** by both batches. Spot-checked `seedAnnotations`, `seedFlights`, `seedClasses` — same IDs, same shape across all consumers. +- `FlightProvider` / `AuthProvider` / `RtlSafeImage` import paths are consistent across all 4 new test files (`'../src/components/FlightContext'`, `'../src/auth/AuthContext'`). +- `STC-*` IDs across `scripts/run-tests.sh`: 29 unique identifiers, none reused. `STC-PERF01` (bundle size) added in batch 7; `STC-RES02` / `STC-RES03` / `STC-RES09` / `STC-RES10` added in batch 8. None of the new IDs collide with the 24 IDs from batches 1–6. +- MSW handler routes: each handler file owns a disjoint URL prefix; no handler file modified in batches 7–8 (only test-local `server.use(...)` overrides). The settings/user 404 + auth/refresh 401 overrides used by `tile_split_zoom.test.tsx` are scoped to its `beforeEach` and reset in `afterEach` (MSW v2 default). + +**No duplicate symbol** across the two batches. **No fixture redefinition** across consumers. + +### Drift handling pattern uniformity (across all 8 batches) + +- `it.fails()` — production element exists, asserted attribute / behavior is missing today. +- `it.skip` + `// QUARANTINE: ...` — production capability is wholly absent (still used; not re-introduced in 7–8 because the batch-8 `[Q]` ACs are paired with explicit drift assertions instead of skips). +- `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 8 batches. Batches 7 + 8 introduce no new pattern variations. + +### Test infrastructure mutation discipline + +- `scripts/run-tests.sh` extended only by adding new `static_check_*` functions and corresponding `run_static` rows; existing functions / rows untouched. Each new function is single-responsibility and each `run_static` row carries the AC ID it covers (e.g. `STC-RES02 ... NFT-RES-LIM-02`). +- `tests/security/banned-deps.json` not modified in batches 7–8 (the alert-allowlist + destructive-surfaces deny-list landed in batch 4 are sufficient). +- `tests/setup.ts` not modified in batches 7–8. + +No Phase 6 findings beyond the pattern uniformity record above. + +## Phase 7 — Architecture Compliance + +### Cross-component import audit (4 new fast test files in batches 7–8) + +| Test file | Cross-component imports | Verdict | +|-----------|-------------------------|---------| +| `tests/canvas_editor.test.tsx` | `App` (default — exercises `` to mount the canvas surface) + helpers | OK — public composition root | +| `tests/photo_mode.test.tsx` | `DetectionClasses` (default) + `AnnotationsPage` (default) + `FlightProvider` + helpers | OK — all are public defaults | +| `tests/network_resilience.test.tsx` | `App` (default) + `AnnotationsPage` + `FlightProvider` + helpers | OK | +| `tests/tile_split_zoom.test.tsx` | `DatasetPage` (default) + `FlightProvider` + helpers | OK — all are public defaults | + +- **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 7 + 8. 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. + +### Baseline Delta + +Comparing current findings to `_docs/02_document/architecture_compliance_baseline.md`: + +**Carried over** — present at baseline, still present (unchanged from cumulatives 01–03 and 04–06): + +| # | 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//...` | +| 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. + +## Findings (cumulative) + +### F-CUM-5 — Production drift backlog grows to 23 items (Low / Maintainability / cumulative) + +Carries forward F-CUM-3 from cumulative 04–06 (18 items) and adds the new drifts from batches 7–8: + +| # | Source AC / scenario | Production file | Phase B touchpoint | +|---|----------------------|-----------------|--------------------| +| 27 | AZ-471 AC-3 — Ctrl+click multi-select never reached (production enters draw mode on Ctrl+button-0) | `src/features/annotations/CanvasEditor.tsx` `handleMouseDown` | gate Ctrl+button-0 to "is there a selectable target underneath?" | +| 28 | AZ-471 AC-4 — Ctrl+wheel zoom-around-cursor: pan not adjusted, cursor pixel drifts | same `handleWheel` | adjust pan to keep cursor invariant during zoom | +| 29 | AZ-471 AC-5 — Ctrl+drag empty-canvas pan never reached (same Ctrl-gate as #27) | same `handleMouseDown` | resolves with #27 | +| 30 | AZ-478 AC-1 — silent /login redirect on offline boot (no user-visible network-error indicator) | `src/App.tsx` boot path | render an offline error banner / toast on boot fetch failure | +| 31 | AZ-478 AC-2 — tainted-canvas `toBlob` SecurityError unhandled (no fallback) | `src/features/annotations/AnnotationsPage.tsx` `handleDownload` | wrap `toBlob` in try/catch; fall back to a "right-click → save image as" hint | +| 32 | AZ-478 AC-3 — no SSE consumer renders connection-lost banner | every `createSSE` consumer (`src/features/flights/FlightsPage.tsx`, future annotation-status SSE) | wire `createSSE`'s `onError` to a localised banner | +| 33 | AZ-474 AC-1..6 — entire tile-split surface QUARANTINED (no Split-tile button, no parser, no ``, no zoom indicator, no malformed-label error region) | `src/features/dataset/DatasetPage.tsx`; new parser module + `` component | Phase B feature: `Split tile` affordance + YOLO label parser + viewer + indicator + alert region (5 sub-tasks; share the new YOLO parser module) | + +(AZ-473, AZ-479, AZ-480 contributed **0 new drifts** — those tasks PASS today. AZ-480 e2e gated portions are deployment-environment gates, not drifts.) + +**Recommendation**: file these 7 new entries (#27–#33) as Phase B feature tasks during Step 9 (New Task) once Phase A baseline closes. Several share files (`CanvasEditor.tsx` for #27/29; the AZ-474 entries share a parser module) and could be combined for review efficiency. None are blocking for Step 6 or Step 7. + +This is a **non-blocking** finding; verdict contribution = PASS_WITH_WARNINGS only. + +### F-CUM-4 carry-over — Long-running soak gating still env-flag-only (Low / Maintainability) + +Reaffirmed: AZ-479 AC-4 (annotation memory soak) and AZ-480 AC-3 (RAM soak) e2e companions are gated by `process.env.RUN_LONG_RUNNING === '1'`. The original recommendation (move to Playwright `@long-running` `grep` tag in `e2e/playwright.config.ts`) remains open. + +**Recommendation**: combine with the existing AZ-463 entry under one Phase B / Step 7 ticket: "tag all long-running e2e tests `@long-running` and add the Playwright config grep filter so CI lanes skip them by default; per-PR lane uses `--grep-invert='@long-running'`, dev/stage merge lane drops the filter". + +This is the same finding as F-CUM-4 from the previous cumulative; not double-counted. + +## Auto-Fix Attempts: 0 + +No findings escalate to Auto-Fix. F-CUM-5 + F-CUM-4 (carry-over) are both bookkeeping for Phase B / Step 7. + +## Stuck Agents + +None in batches 7–8. The AZ-474 batch-8 `getContext` JSDOM warning was triaged inline and documented in the batch-8 report rather than being mocked away (the AC-6 assertions target the dataset card surface and the no-`alert()` defence-in-depth control, not the canvas itself; the warning is stderr noise without affecting the test outcome). + +## Verdict: PASS_WITH_WARNINGS + +Reason: 0 Critical / 0 High; 1 Low / Maintainability finding new (F-CUM-5: 7 new production-drift entries lifting backlog to 23 items) + 1 Low / Maintainability carry-over (F-CUM-4: long-running soak gating mechanism). Implement skill may proceed to Step 7 (Run Tests). + +## Cycle Close — Phase A Wrap + +Phase A — One-time baseline setup is now COMPLETE. + +- 25 Phase A test tasks delivered across 8 batches. +- 0 production source files mutated (Blackbox Tests envelope respected end-to-end). +- All 26 ACs in batches 7–8 covered; cumulative 100% AC coverage across all 8 batches (per the per-batch reports). +- 23 production drifts catalogued and pinned to runnable contract tests; each test flips green automatically when the matching production fix lands. +- 29 commit-time static gates active (up from 13 at baseline `729ad1c`). +- Fast-profile suite: 26 files / 163 PASS / 13 SKIP / ~16 s wall. +- Static profile: 29/29 PASS / ~13 s wall. + +**Next autodev action**: Step 7 (Run Tests) — full fast + static + e2e profile run end-to-end. After Step 7 completes, the autodev re-detects the next step and either advances to Step 8 (Refactor — optional) or prompts the user for Phase B task selection at Step 9. + +No cumulative-review-gated changes need to be applied before Step 7 starts. diff --git a/_docs/_autodev_state.md b/_docs/_autodev_state.md index 01a6bf6..a72f95f 100644 --- a/_docs/_autodev_state.md +++ b/_docs/_autodev_state.md @@ -8,7 +8,7 @@ status: in_progress sub_step: phase: 14 name: batch-loop - detail: "batch 8 closed; cumulative 07-08 due" + detail: "Phase A closed; 25 tasks delivered; advance to Step 7" retry_count: 0 cycle: 1 tracker: jira @@ -23,8 +23,9 @@ step_3_ac_gap_handling: rollback-to-6c (option A) `glossary.md`, plus `_docs/01_solution/solution.md` and `_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`. + `_docs/03_implementation/batch_0{1..8}_report.md`. - 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). + `_docs/03_implementation/cumulative_review_batches_01-03_report.md`, + `_docs/03_implementation/cumulative_review_batches_04-06_cycle1_report.md`, + `_docs/03_implementation/cumulative_review_batches_07-08_cycle1_report.md` + (cycle close — Phase A wrap, no batch 9).