docs: sprint 6 wrap-up — README + CHANGELOG + 6 lessons
- README "Status" bumped to sprint 6 + test counts (253 backend, 136 frontend, 223 e2e). - CHANGELOG [Unreleased] section for sprint 6: backend, frontend, e2e, security, and changed-section notes (SPEC commit-first + mimic team). - 6 sprint-6 lessons in tasks/lessons.md: 1. SPEC.md commit-first tamed the 4-sprint recurrence 2. Persistent team mimic + idle members > "never idle" 3. Security plugin caught CSV formula injection mid-sprint 4. Stdlib first before custom helpers 5. Tests that mock at module level can't exercise the target's branches 6. _engagement param for signature symmetry across render trio This is the team-lead wrap-up commit. PR body in tasks/pr-body-sprint-6.md will be ingested by make open-pr.
This commit is contained in:
@@ -4,6 +4,34 @@ Recurring mistakes and the rule we adopted so the same issue doesn't bite twice.
|
||||
|
||||
---
|
||||
|
||||
## Sprint 6 (closed 2026-06-08)
|
||||
|
||||
### Process — SPEC.md commit-first finally tamed the 4-sprint recurrence (team-lead)
|
||||
**Context** : Sprints 3, 4, AND 5 had each shipped initial PRs with `M SPEC.md` uncommitted. Sprint 5 lessons.md proposed concrete fix candidates ; sprint 6 adopted candidate #1 (stage SPEC.md as part of the FIRST sprint commit, before any code). Result: commit `7aaa5cc` was the SPEC update, all subsequent commits were code/tests/docs, and `git status` at sprint close was 100% clean — no orphan SPEC change to carry over.
|
||||
**Lesson** : process recurrences die when the fix is positional ("FIRST commit") rather than a checklist item at sprint close. The discipline is structural now: as soon as the team-lead writes the SPEC section in §0 of the plan, the matching SPEC.md edit commit follows immediately, before the plan commit itself. Memory updated. Keep doing this.
|
||||
|
||||
### Engineering — Persistent team `mimic` + idle members is worth the token cost (team-lead + user)
|
||||
**Context** : Sprint 5 left `~/.claude/teams/mimic/config.json` as an empty husk because past sprints had called `TeamDelete` at wrap-up. Sprint 6 attempted to spawn agents one-by-one with their phase tasks; hit "team roster is flat — Teammates cannot spawn other teammates" when respawning backend-builder for a security fix mid-sprint. User flipped policy: spawn ALL 7 project-defined agents (`.claude/agents/*.md`) into team `mimic` at sprint start with idle prompts, wake them via `SendMessage` per phase, no shutdown until sprint close.
|
||||
**Lesson** : the "never idle" rule from earlier memory was a token-economy heuristic that became a coordination problem. The team-roster gotcha (couldn't re-add a name after termination, couldn't add a fresh role after the first two spawns settled) confirms: pre-spawn the full roster idle, address by name throughout, accept the idle token cost. `feedback-team-spawn` memory now reflects this. DO NOT call TeamDelete unless we explicitly want to nuke the team for sprint 7+ work. The team persists, the members are re-spawned each sprint into the same team.
|
||||
|
||||
### Engineering — Security plugin caught CSV formula injection mid-sprint (team-lead + backend-builder)
|
||||
**Context** : `security-guidance@claude-code-plugins` automated review flagged the CSV writer in `render_engagement_csv()` as vulnerable to spreadsheet formula injection (MEDIUM). The team-lead's first instinct was "this is internal, RBAC limits the attacker to authenticated red-team users — likely not exploitable" ; on second reading, recognized that the **explicit consumption path** for the CSV is the SOC analyst opening it in Excel/LibreOffice, AND that the red-team and SOC are different users on different machines (handoff is the whole point of the sprint). Fix applied (`_csv_safe()` helper, apostrophe prefix on formula-trigger chars), 3 tests added (`57dbd14`).
|
||||
**Lesson** : when a security finding cites "internal service, may not be exploitable", ask one more layer of questions about the consumption path. If the cell content ever reaches a different user's spreadsheet, defuse it. Cost: 10-line helper + 3 tests, 60 seconds of work. Don't dismiss SEC findings on RBAC alone.
|
||||
|
||||
### Engineering — Stdlib first before custom helpers (code-reviewer + backend-builder)
|
||||
**Context** : The export.py service shipped with a local `_html_escape` helper that reimplemented `html.escape(text, quote=True)` from the stdlib. Code-reviewer's NIT flagged it. Replaced by `from html import escape as _html_escape` (-7 lines, identical behavior).
|
||||
**Lesson** : before writing an escape/format/parse helper, grep for an stdlib equivalent. The Python stdlib has `html`, `csv`, `unicodedata`, `re`, `email.utils`, `urllib.parse` — most "escape this safely" needs are already solved. Custom helpers age into security maintenance debt; stdlib doesn't.
|
||||
|
||||
### Engineering — Tests that mock the API client at module level can't exercise its fallback paths (frontend-builder + code-reviewer)
|
||||
**Context** : `ExportEngagementButton.test.tsx` uses `vi.mock('@/api/exports', ...)` at module level — replaces the whole `downloadEngagementExport` function with a stub. Code-reviewer flagged that the Content-Disposition fallback inside `exports.ts` (`engagement-<id>.<ext>` when the header is malformed) was uncovered. Fix: new dedicated test file `exports.test.ts` that mocks the underlying `apiClient` (axios) instead, so the real `downloadEngagementExport` runs (`123d981`).
|
||||
**Lesson** : a test file that mocks its target at module level can validate the CALLERS but not the TARGET'S internal logic. To cover a function's internal branches (fallbacks, error parsing, header parsing), you need a separate test file that mocks one layer DEEPER (axios, fetch, transport) and lets the function under test execute. Pattern : one test file per layer.
|
||||
|
||||
### Engineering — `engagement` param as `_engagement` for signature symmetry (backend-builder + code-reviewer)
|
||||
**Context** : `render_engagement_csv()` takes `engagement` but never reads it — by design, since the CSV is machine-readable strict (no engagement header inside the file, only in the filename). Pyright flagged "not accessed". Two options: rename to `_engagement` (intentional unused marker) or drop the param. We renamed — keeps the trio of render functions (`md`/`csv`/`pdf`) callable with the same arguments from the endpoint switch.
|
||||
**Lesson** : when one function in a sibling-trio doesn't need a parameter that the others use, keep the signature symmetric and prefix the unused param with `_`. The endpoint stays callable in a uniform way (`render_engagement_<fmt>(engagement, simulations)`), and the underscore signals "intentional" to mypy/pyright/ruff. Don't sacrifice symmetry for purity.
|
||||
|
||||
---
|
||||
|
||||
## Sprint 5 (closed 2026-05-28)
|
||||
|
||||
### Process — The "git status pre-sprint-close" discipline is still broken, 3 sprints in a row (team-lead)
|
||||
|
||||
Reference in New Issue
Block a user