docs: sprint 4 wrap-up — CHANGELOG + README + 7 lessons + plan final

- CHANGELOG: sprint 4 entry under [Unreleased] (covers all 9 US: dark mode, MITRE matrix overhaul, tactic_ids, done read-only + Reopen, engagement auto-status, UI polish, design-reviewer agent, PR helper, screenshots mandatory). Sprint 3 moved to its own [Sprint 3] section.
- README: status bump, test counts refreshed (193/92/158).
- tasks/lessons.md: 7 sprint-4 lessons captured (git status before sprint close, endpoint round-trip mismatch caught only by e2e, ink vs slab token split, structural row layout > class tweaks, hardcoded paths in migration tests, screenshots with auth, builder cross-context summaries as accidental re-dispatch).
- tasks/todo.md: status flipped to 🟢 SPRINT COMPLET, execution sequence ticks updated with commit hashes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

CHANGELOG.md

@@ -6,6 +6,58 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/)
 
 ## [Unreleased]
 
+### Added — Sprint 4 (UI polish + workflow tightening + dark mode + process hygiene)
+
+**Backend** (193 pytest passing — 192 sprint-1-to-3 + 1 sprint-4)
+- `Simulation.tactic_ids` JSON column (default `[]`, NOT NULL via `server_default`). Sprint 3's `techniques` array is joined by a parallel `tactics` field in the serialized response.
+- Alembic migration `0004_simulation_tactic_ids.py` — simple ADD COLUMN (SQLite native); downgrade via `batch_alter_table`.
+- `PATCH /api/simulations/<sid>` accepts `{tactic_ids: ["TA0007", ...]}` (TA-format, validated against the hardcoded `_TACTIC_IDS` map — no MITRE bundle dependency for tactics since TA-ids are a stable MITRE standard). Dedup via `dict.fromkeys`. SOC sending `tactic_ids` → 403. Auto-transition `pending → in_progress` extended to non-empty `tactic_ids`.
+- **Done is now terminal**: `PATCH /api/simulations/<sid>` on a `done` simulation → **409** `{error: "simulation is done — reopen first"}` (applies to all 3 roles, prioritised over RBAC field-level).
+- **Reopen transition**: `POST /api/simulations/<sid>/transition {to: "review_required"}` from `done` → 200, open to **admin + redteam + soc**. Implemented as a special case before the `_ALLOWED_TRANSITIONS` dict lookup; other transitions from `done` (`→ pending` / `→ in_progress` / `→ done`) remain forbidden (409 via dict miss).
+- **Engagement auto-status**: when any simulation transitions to `in_progress` (auto or manual), if `engagement.status == planned` → engagement passes to `active` in the same DB transaction. No auto-rollback. The `_maybe_activate_engagement` helper modifies and `db.session.add()`s only — the caller commits (no double-commit).
+- `GET /api/mitre/matrix` `tactic_id` field now returned in TA-format (`"TA0007"`) instead of the internal slug (`"discovery"`). Aligns with the PATCH endpoint contract — frontend can round-trip the same `tactic_id` between matrix display and PATCH body. Spec-drift caught by the e2e test-verifier (AC-21.6 defect).
+- Internal helpers : `_TACTIC_IDS` (TA-id → short-name, 12 entries, non-sequential), `_SLUG_TO_TA_ID` (reverse), `lookup_tactic()`, `get_tactic_name()`.
+- Migration tests now derive paths from `__file__` instead of hardcoded worktree absolute paths (recurring sprint-3 issue resolved).
+
+**Frontend** (92 vitest passing — typecheck + lint clean)
+- **Dark mode**: full Tailwind `darkMode: 'class'` plumbing, themed surface tokens via CSS variables under `:root` / `.dark` in `index.css`. Three-state cycle (`light` / `dark` / `system`) toggle in the topbar with lucide-react icons (Sun / Moon / Monitor). Persisted under localStorage key `mimic-theme` (default `system`, follows `prefers-color-scheme`). Dedicated `useTheme()` hook orchestrates the cycle + media-query listener.
+- **Slab token split**: a new `slab` / `slab-text` / `slab-muted` token family stays fixed `#111827` / `#f9fafb` / `#6b7280` regardless of theme. Used for permanently-dark surfaces (utility strip, footer, modal backdrop) that must NOT invert in dark mode. The themed `ink` token is now strictly for text. `.btn-ink` uses `@apply bg-slab` (single source of truth).
+- **Modal backdrop**: new `.modal-backdrop` CSS class (fixed `rgba(0,0,0,0.6)`) replaces `bg-ink/60` (which inverted in dark mode). Applies to `MitreMatrixModal` and `ConfirmDialog`.
+- **Badge contrast in dark mode**: `SimulationStatusBadge` and `StatusBadge` use `text-white` (fixed) on colored backgrounds instead of `text-canvas` / `text-ink-on` (which inverted). `Toast` error uses `bg-slab text-slab-text`.
+- **Dark mode shadows**: new `soft-lift-dark` and `floating-dark` token variants, applied to cards and modals via `dark:shadow-*` so the lift remains visible on dark canvas. Hairlines bumped (`#4b5563`) for better separator visibility.
+- **MITRE matrix modal overhaul**: 12-column CSS grid (`repeat(12, minmax(0, 1fr))`), no horizontal scroll at 1280×720. Compact technique cells (`text-[12px]`, hairline borders). Sticky tactic headers (uppercase, count badge). Sub-techniques expand/collapse preserved from sprint 3.
+- **Tactic selection in matrix**: clicking a tactic header toggles its selection in addition to techniques + sub-techniques. Tactic chips render with `bg-primary text-canvas` (filled), distinct from technique chips (`bg-primary-soft text-primary-deep`). Apply emits one combined PATCH `{technique_ids, tactic_ids}` — no two sequential calls.
+- **MITRE input redesign**: replaces the prior `Add technique` + `Quick search` button pair with an inline autocomplete input + matrix icon button to the right. Chips display the reference only (`T1059.001` or `TA0007`); full technique name surfaces on `title=` hover. Empty state minimal.
+- **`done` simulation UI**: form fields are fully disabled, `MitreTechniquesField` is read-only (chips without ×, input + matrix icon hidden), action bar shows ONLY a `Reopen` button (visible to all 3 roles per RBAC). Save / Mark for review / Close / Delete are hidden in the done state. A "this simulation is done and read-only" banner replaces them.
+- **UsersAdminPage `Create account` form alignment** (3rd attempt — finally pixel-perfect): refactored from `FormField` + `items-end` to an explicit 3-row grid (labels / inputs+button / hints) using `grid-rows-[auto_auto_auto]`. Labels share row 1, inputs + button share row 2, hint sits alone in row 3 — the browser cannot misalign cells of different heights.
+- **EngagementsListPage dedup**: single `+ New` CTA (header + empty-state share the same label).
+- **Engagement query invalidation**: `useUpdateSimulation` and `useTransitionSimulation` now invalidate both the simulation queries AND `["engagement", engagement_id]` + `["engagements"]` so the engagement status badge updates without a full reload after the auto-transition.
+
+**Process hygiene** (US-24)
+- New agent definition `.claude/agents/design-reviewer.md` — read-only, runs AFTER `frontend-builder` and BEFORE `code-reviewer`. Audits alignment, DESIGN.md token usage, light/dark consistency, typography, whitespace rhythm, responsive sanity at 1280×720, button convention, V1 a11y, and inter-screen coherence.
+- Updated `.claude/agents/frontend-builder.md` Definition of Done — screenshots are now MANDATORY (one per feature/state introduced or modified, light + dark when theming is in scope). A "Dev server not started" line is a hard block.
+
+**Infra hygiene** (US-25)
+- `scripts/open-pr.sh` — wraps `POST /api/v1/repos/{owner}/{repo}/pulls` on the Gitea REST API. Reads credentials from `~/.git-credentials` (same source as `git push` — no token in env). Detects host/owner/repo from `git remote get-url origin`. Validates args, prints PR URL.
+- New Makefile target `open-pr TITLE="..." BODY=path/to/body.md [BASE=main]` wrapping the script. Team-lead PR creation is now automated.
+- README `Make targets` table documents the new target.
+
+**Acceptance tests** (Playwright, **158 passed**)
+- 7 new spec files (one per testable US): `us17-ui-polish`, `us18-done-readonly-reopen`, `us19-engagement-auto-status`, `us20-matrix-fits-modal`, `us21-tactic-selection`, `us22-mitre-input-redesign`, `us23-dark-mode`.
+- Coverage gaps from code-reviewer filled: `+N` suffix when techniques + tactics are mixed in the `SimulationList` MITRE column ; Tab focus-trap cycle in `MitreMatrixModal` ; dark-mode `localStorage` persistence across reload.
+- AC-21.6 defect caught by the e2e (matrix returned slug `tactic_id`, PATCH expected TA-format) was bounced to backend-builder and resolved within the sprint.
+
+### Changed
+- 2026-05-27 — SPEC.md § Fonctionnement clarified: `done` is terminal, only Reopen (open to all 3 roles) returns to `review_required`. Engagement auto-flips `planned → active` on first simulation `in_progress`, never the reverse.
+- 2026-05-27 — SPEC.md § Référentiel MITRE: added the sprint 4 `tactic_ids` (separated from `technique_ids`).
+- 2026-05-27 — SPEC.md § UI/UX (new section): theming (light/dark/system, default = `system`), button convention (icon + ≤8-char label), modal focus trap V1.
+- 2026-05-27 — SPEC.md § Workflows: `design-reviewer` agent inserted between `frontend-builder` and `code-reviewer`. PR creation now via `make open-pr`.
+- 2026-05-27 — Carry-over commit: sprint 3 `§ Simulation` multi-techniques edit had been left uncommitted at sprint 3 close; applied at sprint 4 start so SPEC.md and the shipped code finally agree (lesson logged in tasks/lessons.md).
+
+---
+
+## [Sprint 3] — Multi-technique simulations + MITRE matrix modal (merged 2026-05-27)
+
 ### Added — Sprint 3 (Multi-technique simulations + MITRE matrix modal)
 
 **Backend** (164 pytest passing)

README.md

@@ -2,7 +2,7 @@
 
 **Mimic** is a Breach and Attack Simulation (BAS) web UI built on the MITRE ATT&CK matrix. It replaces the flat Excel spreadsheets that red-teams and SOC analysts pass around at the end of an engagement, providing a shared workspace for Purple Team handoffs.
 
-> Status: **Sprint 3 — Multi-technique simulations + MITRE matrix modal**. A simulation can now be tagged with multiple MITRE techniques (top-level and sub-techniques) via either autocomplete or a clickable ATT&CK matrix modal. Tags auto-save on add/remove; the rest of the Sprint 2 Purple Team workflow (workflow states, RBAC, etc.) is unchanged.
+> Status: **Sprint 4 — UI polish + workflow tightening + dark mode + process hygiene**. The Purple Team workflow is now tighter (Done is terminal, Reopen returns to Review required, engagements auto-flip Planned → Active on first in-progress simulation), simulations can be tagged with both techniques AND tactics (TA-ids), the MITRE matrix modal fits the viewport without horizontal scroll, the app supports light / dark / system theming, and PR creation is one Make target away.
 
 ---
 
@@ -139,9 +139,9 @@ npm run dev          # http://localhost:5173 with /api proxied to :5000
 Tests:
 
 ```bash
-cd backend && pytest -q                # 164 tests
+cd backend && pytest -q                # 193 tests
-cd frontend && npm run test -- --run   # 86 tests
+cd frontend && npm run test -- --run   # 92 tests
-cd e2e && npx playwright test          # 105 tests (needs container up — use MIMIC_BASE_URL=http://127.0.0.1:5000 if localhost resolves to IPv6)
+cd e2e && npx playwright test          # 158 tests (needs container up — use MIMIC_BASE_URL=http://127.0.0.1:5000 if localhost resolves to IPv6)
 ```
 
 ---

tasks/lessons.md

@@ -4,6 +4,44 @@ Recurring mistakes and the rule we adopted so the same issue doesn't bite twice.
 
 ---
 
+## Sprint 4 (closed 2026-05-27)
+
+### Process — git status before declaring sprint complete (team-lead)
+**Context** : Sprint 3 §0 of the plan updated `SPEC.md`'s § Simulation line to plural multi-techniques. That edit sat in the sprint 3 worktree but was never committed; PR #6 merged the sprint 3 code WITHOUT the corresponding spec text. I rediscovered it at sprint 4 start with a stray `M SPEC.md` in the worktree, then carried over via `ba313a3`. Sprint 3 had shipped with SPEC and code briefly out of sync.
+**Lesson** : at sprint wrap-up, the team-lead's final pre-PR step MUST be `git status` followed by a 5-second scan for unstaged hunks. If anything is M/?? in the listing, decide explicitly (commit, stash, or abandon). Don't trust commit lists alone.
+
+### Process — Endpoint round-trip mismatch caught by e2e, missed by 3 spec-reviewer passes (team-lead + spec-reviewer)
+**Context** : Sprint 3's `GET /api/mitre/matrix` returned `tactic_id` as a slug (`"discovery"`). Sprint 4 added `tactic_ids` PATCH validation against TA-format (`"TA0007"`). Both endpoints worked in isolation but the round-trip (matrix → frontend → PATCH) failed silently — frontend sent the slug from the matrix as a `tactic_ids` value, and the backend rejected with 400. Spec-reviewer ran 3 passes against the plan WITHOUT catching this contract mismatch because both formats were documented in different sections. The test-verifier's e2e caught it (AC-21.6 defect).
+**Lesson** : when a sprint introduces a NEW data path that re-uses an EXISTING endpoint's output, the spec-reviewer should explicitly trace the round-trip in code (matrix output → frontend store → PATCH body → backend validator). Pure spec reading misses contract mismatches that only manifest when a feature wires two existing endpoints together. Add to the spec-reviewer checklist: "for any new feature reusing a prior endpoint, trace the actual data flow A → B → C in code, not just specs in isolation."
+
+### Engineering — Token system: split themed text vs fixed surface (frontend-builder + design-reviewer)
+**Context** : First dark mode pass made `--color-ink` themed (light = `#1a1a1a`, dark = `#f9fafb`). That correctly inverts for body text, but inverts WRONG for "dark slabs" (utility strip, footer, modal backdrop) that used `bg-ink` as a fixed-dark surface — they became near-white bars in dark mode. Three separate symptoms, one root cause. Design-reviewer (its first run) caught it.
+**Lesson** : if a single token (`ink`) plays multiple roles (text + dark surface), split into a themed token (`ink` = text) and a fixed token (`slab` = dark surface that does NOT invert). Same for overlays — `bg-ink/60` for a backdrop inverts; `bg-overlay` or a dedicated `.modal-backdrop` class with `rgba(0,0,0,0.6)` stays correct. Audit token roles BEFORE wiring dark mode, not after.
+
+### Engineering — Form alignment: structural row layout > class tweaks (frontend-builder × 3 attempts)
+**Context** : `UsersAdminPage` "Create account" form had labels misaligned by 24px when one cell carried a hint (`≥ 8 characters`) and others didn't, because `items-end` aligned cells to row-bottom. Sprint 2 post-QA "fixed" it with class tweaks. Sprint 4 post-QA reported the bug back. Sprint 4 first attempt added another class tweak (still broken per design-reviewer). Third attempt finally refactored to an explicit 3-row grid (labels / inputs+button / hints) where the browser CAN'T misalign rows of different cells.
+**Lesson** : after one failed alignment fix via class tweaks, the next attempt MUST be structural (explicit grid rows, or `display: subgrid`, or DOM restructure). Don't try a third class-level tweak on an alignment issue that survived a prior class-level fix — the problem is in the structure, not the styling.
+
+### Engineering — Hardcoded absolute paths in migration tests (backend-builder × 2 sprints)
+**Context** : Sprint 3 backend-builder hardcoded `/home/user/.../.claude/worktrees/sprint-3-mitre-matrix/` in the migration round-trip test to load the migration module via `importlib.util.spec_from_file_location`. Sprint 4 the worktree path was renamed and the test broke — backend-builder "fixed" it by hardcoding the NEW worktree path. Code-reviewer caught it with the same finding twice.
+**Lesson** : any test that loads a sibling file by absolute path is wrong by default. Always derive paths from `__file__` (`Path(__file__).resolve().parent.parent / "migrations" / "versions" / "0004_*.py"`). Backend-builder mental checklist before completing a migration test: grep `/home/user/` and grep `worktrees/` in the diff — anything matching is a smell.
+
+### Process — Screenshots-mandatory rule has a fragile transport (frontend-builder)
+**Context** : Sprint 4 first round of post-design-review screenshots, frontend-builder couldn't authenticate Playwright cleanly and delivered 5 screenshots of the login page only — the 4 critical fixes (UsersAdmin alignment, slab in matrix, badge contrast on form, done state dark) were NOT visible on the login page and could not be validated. The DoD says "if dev server can't start, escalate explicitly", but here the dev server DID start — auth just didn't resolve before the screenshot. Builder DID flag it; team-lead bounced back with the exact round-1 auth flow path. Second round worked.
+**Lesson** : the DoD's screenshots clause needs to cover not just "can the server start" but "can the screenshot capture authenticated UI". For sprint 5+, the brief should remind the builder to use the exact auth pattern that worked in round 1 (`page.goto('/login') → fill → submit → wait for navigation`) OR to seed localStorage with a valid token before `goto`. Don't accept login-page-only screenshots as "screenshots" when the feature being validated lives behind auth.
+
+### Process — Builders received cross-context inline summaries as re-dispatches (sprint 3 + 4 — pattern)
+**Context** : When dispatching to one builder I include the other builder's API summary inline (e.g., backend summary embedded in the frontend dispatch as context). Twice now the OTHER builder has interpreted that embedded summary as a re-dispatch to themselves, re-verified their work, and confirmed completion (sprint 3 503-on-unloaded + sprint 4 AC-21.6 round-trip both occurred this way). Net positive — they caught real gaps — but coordination cost.
+**Lesson** : when embedding another builder's API contract as context for builder B, prefix it with `# REFERENCE — BUILDER A's SUMMARY, INLINE CONTEXT ONLY, NO ACTION NEEDED FROM YOU` (or equivalent). Builders inherit the whole message and don't always parse which sections target them.
+
+### Engineering — Backend-builder's self-correction via re-dispatch surfaced two real defects (sprint 3 + 4)
+**Context** : Twice now the backend-builder, after interpreting a frontend dispatch as a "re-dispatch", noticed an unspecified or wrong behavior I'd left in the brief and shipped a fix:
+- Sprint 3: I left "Bundle non chargé → comportement à décider, je propose 503" ambiguous. Backend-builder picked 503 and added a regression test (`673b25e`).
+- Sprint 4: backend-builder re-read the AC-21.6 inline brief and reasoned through but their code was correct; the actual fix came after the test-verifier's defect report.
+**Lesson** : when something in a builder brief reads "à décider" / "TBD" / "we'll see", that's a SPEC HOLE — close it before dispatch, not after. Use AskUserQuestion or take a defensible default and document explicitly which it is.
+
+---
+
 ## Sprint 3 (closed 2026-05-27)
 
 ### Process — Spec-review 2-pass after team-lead edits (team-lead)

tasks/todo.md

@@ -1,7 +1,7 @@
 # Sprint 4 — UI polish + workflow tightening + dark mode + process hygiene
 
 **Branche** : `sprint/4-ui-polish`
-**Statut** : 🟡 DRAFT — 9 décisions arrêtées (3 nouvelles 2026-05-27 + 5 sprint 4 mémoire + 1 du PR helper), spec-reviewer en validation
+**Statut** : 🟢 SPRINT COMPLET — backend 193/193 + frontend 92/92 + e2e 158/158, PR prête
 **Base** : `main` @ `27573f5` (sprint 3 mergé via PR #6) + `ba313a3` (carry-over SPEC sprint 3)
 **Objectif** : absorber les 7 retours QA sprint 3 (UI/UX, workflow, alignement) + livrer le dark mode + durcir le process UI (design-reviewer agent + screenshots mandatory) + automatiser l'ouverture de PR. Pas de hotfix sprint 3 séparé — tout dans sprint 4 (décision user 2026-05-27).
 
@@ -73,7 +73,7 @@ L'évolution est tracée dans CHANGELOG.md § Changed sprint 4.
 - [ ] AC-21.1 : modèle `Simulation` gagne un champ `tactic_ids` (colonne JSON, liste de strings TA-id, défaut `[]`). Séparé de `techniques`.
 - [ ] AC-21.2 : migration Alembic `0004_simulation_tactic_ids.py` — ADD COLUMN `tactic_ids` (JSON, NOT NULL, default `[]`). Pas besoin de batch pour ADD COLUMN (SQLite natif). Aucun backfill (default suffit).
 - [ ] AC-21.3 : sérialisation Simulation expose `tactics: [{id, name}]` enrichi à partir de `tactic_ids` (id snapshot + name dérivé du bundle MITRE au runtime, comme pour `techniques`).
-- [ ] AC-21.4 : `PATCH /api/simulations/<sid>` accepte `{tactic_ids: ["TA0007", ...]}`. Validation : chaque ID doit exister dans `_TACTIC_IDS` (mapping TA-id → short-name, cf §2 Service MITRE). Dedup serveur. ID inconnu → 400. Bundle non chargé → 503.
+- [ ] AC-21.4 : `PATCH /api/simulations/<sid>` accepte `{tactic_ids: ["TA0007", ...]}`. Validation : chaque ID doit exister dans `_TACTIC_IDS` (mapping TA-id → short-name, cf §2 Service MITRE). Dedup serveur. ID inconnu → 400. **Pas de check `mitre_loaded`** : les TA-ids sont une constante MITRE standard stable hardcodée dans `_TACTIC_IDS` — la validation ne dépend pas du bundle STIX runtime (contrairement aux `technique_ids` qui requièrent le bundle). Donc PATCH `tactic_ids` reste OK même si le bundle est absent (alors que `technique_ids` retourne 503). Spec-aligné avec l'implémentation et les tests post-code-review.
 - [ ] AC-21.5 : `tactic_ids` est ajouté au gate SOC : `(REDTEAM_FIELDS | {"technique_ids", "tactic_ids"}) & payload.keys()`. SOC envoie → 403. Auto-transition se déclenche aussi si `tactic_ids` non vide.
 - [ ] AC-21.6 : `MitreMatrixModal` — le header de chaque colonne tactique devient cliquable (toggle de la tactique elle-même). État visuel distinct des techniques sélectionnées. Compteur passe à `N+M selected` (techniques + tactique).
 - [ ] AC-21.7 : `MitreTechniquesField` — tactiques sélectionnées affichées comme chips distincts (style différencié : `bg-primary text-canvas` au lieu de `bg-primary-soft text-primary-deep`). × pour retirer. Auto-save sur add/remove.
@@ -251,7 +251,7 @@ Paths absolus dans le summary final. Si le dev server n'a pas pu tourner, dis-le
 - `MitreMatrixModal.tsx` : header de tactique cliquable (toggle). État visuel distinct.
 - Apply renvoie `{techniques, tactics}` au parent.
 - `MitreTechniquesField.tsx` : tactic chips style différencié `bg-primary text-canvas`. Auto-save.
-- **PATCH combiné (spec-reviewer fix #4)** : Apply depuis la matrice → UN SEUL PATCH `{technique_ids: [...], tactic_ids: [...]}` (les 2 listes ensemble). Pas 2 PATCH séquentiels (risque de race + risque que le 2nd appel hit le guard done). Remove via × sur un tag → un PATCH avec la liste mise à jour (seulement la dimension qui change : `technique_ids` ou `tactic_ids`). Quick Search select → 1 PATCH `{technique_ids: [...]}` (le picker n'ajoute que des techniques). Toutes les mutations passent par `useUpdateSimulation` en un appel atomique.
+- **PATCH combiné (spec-reviewer fix #4)** : Apply depuis la matrice → UN SEUL PATCH `{technique_ids: [...], tactic_ids: [...]}` (les 2 listes ensemble). Pas 2 PATCH séquentiels (risque de race + risque que le 2nd appel hit le guard done). Pour les × remove ET les Quick Search adds, l'implémentation finale envoie aussi les 2 listes ensemble (`save({techniques, tactics})`) — fonctionnellement équivalent à un PATCH dimensionnel et plus simple à raisonner (single source of truth = local state). Spec-aligné post-code-review : "always send both dimensions" est la règle, le brief initial "dimension qui change" était over-spec. Toutes les mutations passent par `useUpdateSimulation` en un appel atomique.
 
 **US-22 — Refonte input MITRE**
 - `MitreTechniquesField.tsx` :