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

- README: status bump to sprint 3, test counts refreshed (164/86/105), IPv6 note for the e2e runner
- CHANGELOG: sprint 3 entry under [Unreleased] (multi-tech model + matrix endpoint + auto-save UI); sprint 2 moved to its own [Sprint 2] section (merged 2026-05-27)
- tasks/lessons.md: 6 lessons captured (2-pass spec-review, inline summary scoping, "test in brief means test in commit" discipline, SQLite batch_alter_table, real migration round-trip, modal Apply 0 disambiguation)
- 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>
This commit is contained in:
Knacky
2026-05-27 04:55:12 +02:00
parent df8a6b605b
commit b001f57774
4 changed files with 290 additions and 193 deletions

View File

@@ -4,6 +4,34 @@ Recurring mistakes and the rule we adopted so the same issue doesn't bite twice.
---
## Sprint 3 (closed 2026-05-27)
### 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.
### UX — Modal Apply 0 disambiguation (frontend-builder)
**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.
---
## Sprint 2 (closed 2026-05-26)
### Testing — Vitest module hoisting (frontend-builder)