### 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.
