knacky/mimic
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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

Browse Source 
- CHANGELOG: sprint 5 entry under [Unreleased] (templates CRUD + instantiation + nav + dropdown + decorrelation). Sprint 4 moved to its own [Sprint 4] section.
- README: status bump to sprint 5, test counts refreshed (226/121/201).
- tasks/lessons.md: 6 sprint-5 lessons captured (spec-reviewer 2-pass before dispatch finally clicked, endpoint path drift caught visually not by spec-review, screenshot script mocks lag path changes, silent URL "improvements" by backend, apply_patch wrong primitive for creation copy paths, IntegrityError catch beats pre-check SELECT, SendMessage rule applies to all team agents).
- tasks/todo.md: status flipped to 🟢 SPRINT COMPLET.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Knacky
2026-05-28 07:18:21 +02:00
parent 7c011db6d9
commit 2e59743af5
4 changed files with 300 additions and 264 deletions
Download Patch File Download Diff File Expand all files Collapse all files

32
tasks/lessons.md
View File

@@ -4,6 +4,38 @@ Recurring mistakes and the rule we adopted so the same issue doesn't bite twice.
---
## Sprint 5 (closed 2026-05-28)
### Process — Spec-reviewer 2-pass BEFORE backend dispatch eliminated mid-implementation addenda (team-lead)
**Context** : Sprint 3 and 4 both required urgent addenda to the backend-builder mid-implementation because the spec-reviewer's 2nd pass arrived after the backend-builder had already started. Sprint 5 explicitly waited for spec-reviewer Pass 2 APPROVED before dispatching backend — and the backend ran straight through with 0 addenda churn. The 2-pass model finally clicked.
**Lesson** : ALWAYS wait for the spec-reviewer's verdict on the post-edit pass before dispatching the first builder. If Pass 1 returns NEEDS-CHANGES, apply the fixes, request Pass 2, wait for APPROVED, THEN dispatch. The "let's send to builders in parallel to save time" instinct costs more than it saves.
### Process — Endpoint path drift caught by visual inspection, not by spec-reviewer (team-lead)
**Context** : Backend implemented `/api/templates`. Plan §1 § 2 said `/api/simulation-templates`. Spec-reviewer 2-pass didn't catch the path drift. Frontend used the plan path → mismatch → first frontend commit (`90fc5ba`) was effectively non-functional against the real backend. Caught immediately by team-lead grep + diagnostic.
**Lesson** : when backend "improves" a URL without flagging it as a deviation, it's still a deviation. Add to team-lead PR-merge mental checklist: `git diff main...HEAD | grep "@.*\.route\|@.*_bp\.(get|post|put|patch|delete)"` → does this match the plan §1/§2 path strings exactly? If no, dispatch a 1-line frontend fix BEFORE the post-design-review cycle.
### Process — Frontend-builder's screenshot script reuses stale mocks after path changes (frontend-builder)
**Context** : Sprint 5 path fix `2b70011` corrected the API client to `/api/templates`. But the Playwright screenshot script still mocked `/api/simulation-templates/<id>` — for the GET single endpoint specifically. Result: edit-form screenshot showed a 500 ErrorState instead of the actual form. Design-reviewer caught this as a critical coverage gap.
**Lesson** : when a path / contract changes mid-sprint, the screenshot script's route handlers must be updated in lockstep with the API client. A grep on the script for the old path is mandatory after every path-rename commit. Add to frontend-builder pre-screenshot checklist: `grep -E "<old-path>" $screenshot_script` must return empty.
### Engineering — Backend-builder's silent URL "improvement" (backend-builder)
**Context** : The team-lead's plan §1/§2 explicitly named the endpoints `/api/simulation-templates`. Backend-builder chose `/api/templates` (shorter, cleaner) but did NOT flag this as a deviation in the summary's "Open questions / deviations" section. The frontend-builder followed the plan and broke. The path change was defensible but the lack of escalation was not.
**Lesson** : when a builder chooses a different identifier than the plan (URL path, table name, column name, function name, etc.), even if "better", they MUST flag it in their summary under "Deviations from plan". The team-lead can then propagate the change to dependent briefs. Silent deviations break cross-team contracts.
### Engineering — Avoid calling `apply_patch()` on creation paths (backend-builder + spec-reviewer)
**Context** : Sprint 5 template instantiation copies fields from a template to a new Simulation. Spec-reviewer Pass 1 flagged that a builder unaware of the auto-transition trigger might "reuse the validation" by calling `apply_patch()` — which would trigger `pending → in_progress` on a non-empty technique_ids payload, violating AC-27.4. The plan was explicitly updated to forbid this call. Backend-builder set ORM fields directly, which sidesteps both the bundle lookup AND the auto-transition logic.
**Lesson** : `apply_patch()` is the wrong primitive for creation paths that copy already-validated data. Reach for direct ORM assignment (`sim.field = value`) when the source data is pre-validated (template → instance, replica → primary, etc.). Reserve `apply_patch()` for user-input paths that need full validation + workflow side effects.
### Engineering — Use `IntegrityError` catch for UNIQUE conflict 409, not pre-check SELECT (backend-builder + spec-reviewer)
**Context** : Sprint 5's first plan draft listed BOTH a pre-insert SELECT to check name uniqueness AND an `IntegrityError` catch as fallback. Spec-reviewer Pass 1 flagged this as dual strategy — the SELECT still races under concurrent inserts and adds dead code. Plan was simplified to "catch IntegrityError only, rollback, return 409". Backend implementation matches.
**Lesson** : for UNIQUE constraint violations, the DB is the authoritative source of truth. Catch the `IntegrityError`, roll back, return 409. Don't pre-check with SELECT — it races, and the IntegrityError catch is still required as a safety net (so the pre-check is just dead code). Same applies to other DB-enforced constraints (FK, CHECK).
### Process — Designer-reviewer accidental duplicate (`-2`) reminded me to use SendMessage (team-lead)
**Context** : Sprint 4 introduced the design-reviewer agent. Sprint 5 first design-review I called `Agent({name: "design-reviewer"})` — the system spawned `design-reviewer-2`. Same mistake as sprint 2/3 with backend/frontend builders. Cleaned up via shutdown_request, but the second design-review pass I correctly used SendMessage on the original `design-reviewer` — and got the verdict cleanly without duplicate noise.
**Lesson** : the "SendMessage to existing idle agent, not Agent" rule covers ALL agents in the team, not just builders. Includes design-reviewer, code-reviewer, spec-reviewer, test-verifier. Save: same `feedback-agent-reuse` memory note now applies broadly.
---
## Sprint 4 (closed 2026-05-27)
### Process — git status before declaring sprint complete (team-lead)