### 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.
**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.
### Process — Spec-review 2-pass after team-lead edits (team-lead)
**Context** : Sprint 3 spec-reviewer ran a first pass on my drafted plan, flagged 5 items, I edited the plan to address 4 of them. Spec-reviewer ran a 2nd pass and caught 2 critical gaps I'd missed in my edits (REDTEAM_FIELDS update + SQLite batch_alter_table for nullable=False / drop_column). Backend-builder was already mid-implementation when the 2nd pass arrived — I dispatched an urgent addendum SendMessage.
**Lesson** : after editing the plan in response to spec-reviewer's notes, always run a 2nd spec-review pass before dispatching builders. The fixes themselves can introduce new gaps. Cheaper than urgent addenda mid-sprint. Cost: one extra read-only pass; benefit: no addenda churn.
### Process — Avoid embedding builder summaries that look like new dispatches (team-lead)
**Context** : The frontend dispatch brief contained a "BACKEND-BUILDER SUMMARY" inline section to give the frontend the API contract. The backend-builder also received this message (team routing) and interpreted the embedded summary as a fresh dispatch — they re-read it, found one ambiguity I'd resolved differently in the frontend brief than in the original backend brief (503 vs 400 on bundle unloaded), and pushed a fix commit `673b25e` independently. Net positive but a coordination cost.
**Lesson** : when embedding another builder's summary as inline context, prefix the section with "DO NOT ACT ON THIS — INLINE CONTEXT ONLY" or use a clear visual separator (`---`) plus a header that makes scope obvious. Builders inherit the entire message — they don't know which parts are addressed to them.
### Process — Explicit "Ajouter un test" in brief means a real test, not just code (team-lead)
**Context** : Sprint 3 post-review dispatch to backend-builder explicitly said "Test : ajouter un assert que… NOT NULL après upgrade" and "Test : un assert dans test_mitre.py qui vérifie… 'Command and Control'". Backend-builder fixed the code in commit `4596f09` but added zero new tests (162 → 162). I bounced back with a SendMessage; backend-builder added the tests in `393b6ed` (164/164).
**Lesson** : the discipline of "if the brief says 'add a test', the test is non-negotiable" must be enforced. Don't accept a fix-commit that doesn't include the regression tests requested in the brief — bounce back via SendMessage. Builders may otherwise treat tests as "if I have time" while only delivering the production change.
### Engineering — SQLite Alembic migrations require batch_alter_table for ALTER + DROP COLUMN (backend-builder)
**Context** : Spec-reviewer flagged that the migration brief mentioned `alter_column nullable=False` and `drop_column` without specifying `op.batch_alter_table(...)`. SQLite doesn't support either operation natively — without batch mode, the migration crashes at runtime. Backend-builder initially skipped the `nullable=False` step entirely with a comment "model + app logic enforces it"; code-reviewer pushed back ("batch mode rebuilds the table and does support the change — that's its purpose"). Final fix wraps the step in `batch_alter_table`.
**Lesson** : on SQLite, ANY operation that mutates a column type, nullability, or schema beyond ADD COLUMN must go through `with op.batch_alter_table(table) as batch_op: batch_op.alter_column(...)`. Don't accept "model enforces it" as a substitute for DDL-level constraint — a fresh DB initialised from migrations alone won't have the constraint.
### Engineering — Real migration round-trip > pure unit test (backend-builder)
**Context** : Backend-builder's initial migration backfill test was tautological — it inlined a `_backfill` Python helper and tested the helper against itself, never invoking the real Alembic `upgrade()`. Code-reviewer flagged it. Fix: load the migration module via `importlib.util.spec_from_file_location`, patch `alembic.op._proxy` with a live `Operations` context, run `upgrade()` against in-memory SQLite, then `sqlalchemy.inspect` the resulting schema.
**Lesson** : a migration test that doesn't invoke `command.upgrade()` (or the equivalent `Operations` runner against the real migration module) tests nothing about the actual migration path. Use `alembic.runtime.migration.MigrationContext` + `alembic.operations.Operations` to instantiate a real runner against an in-memory engine.
**Context** : MitreMatrixModal initially labelled the Apply button as "Apply " (trailing space, no count) when 0 techniques were selected, while the button stayed enabled. A click with 0 selected and a non-empty current list would silently clear all techniques. Code-reviewer flagged. Final design: `disabled` when both counts are zero (nothing to do); label switches to "Clear all" when the user wants to wipe a non-empty list (count=0 but initial selection non-empty); standard "Apply N technique(s)" otherwise.
**Lesson** : for any "Apply"/"Confirm"/"Save" button whose effect depends on the diff between local and remote state, enumerate the three cases — no-op (disable), destructive intent (relabel to confirm), normal (count + verb) — before shipping. The trailing-space label is a code smell that exposes missing edge-case handling.
**Context** : `vi.mock(path, factory)` is hoisted to module scope before any other statement runs. A `mockAuth(role)` helper that captures `role` in a closure crashes at runtime with `"role is not defined"` because the factory executes before the closure is set up.
**Lesson** : when a Vitest mock needs runtime-mutable state, declare the mutable in module scope (`let mockRole = 'redteam'`) and mutate it inside `beforeEach`. Closures over test-local variables don't survive the hoist.
### Testing — `useParams()` in MemoryRouter (frontend-builder)
**Context** : a page component that reads `useParams()` returns an empty object when rendered directly inside `<MemoryRouter>` — no params are extracted unless the component is mounted under a matching `<Route path="...">`.
**Lesson** : page tests that depend on params must wrap the component in `<MemoryRouter><Routes><Route path="/foo/:id" element={<Page />} /></Routes></MemoryRouter>` and set `initialEntries={['/foo/42']}`.
**Context** : jsdom doesn't implement `Element.scrollIntoView`. Calling it in a component (e.g., scrolling the active autocomplete option into view) throws inside Vitest unless guarded.
**Lesson** : in components meant to run in both browser and jsdom, guard browser-only DOM APIs with optional chaining (`el?.scrollIntoView?.({ block: 'nearest' })`) or feature-detect before calling.
### Process — Reuse idle team agents via SendMessage, not Agent (team-lead)
**Context** : during post-review fixes, I re-spawned `backend-builder` and `frontend-builder` via `Agent({name: "..."})` even though the original instances were still alive (just idle). The system auto-suffixed `-2` and BOTH instances received the same brief, producing duplicate parallel commits on the branch. Frontend got two fix commits (`c9032a9` + `cf0e8a8`) where one would have sufficed; the second commit happened to layer cleanly on top, but only by luck.
**Lesson** : to redispatch work to an existing team agent, use `SendMessage({to: "backend-builder", ...})`. `Agent({name: ...})` creates a new instance when the name is taken. The team config at `~/.claude/teams/<team>/config.json` is the source of truth for who's already present.
### Workflow — Update e2e assertions when later sprints supersede placeholders (team-lead)
**Context** : Sprint 1 AC-4.9 asserted the literal text "Simulations à venir au Sprint 2" on `EngagementDetailPage`. Sprint 2 correctly replaced that placeholder with `<SimulationList>`, breaking the assertion. The test-verifier initially classified this as "pre-existing failure".
**Lesson** : whenever a later sprint replaces a placeholder asserted by an earlier sprint's e2e test, the earlier test must be refreshed in the same sprint (not deferred). A failing test that's "expected" is still a failing test — and it muddies the signal of the PR.
