Ed3Design aggregate-code-review-after-tdd-tasks
What this skill does
After completing N atomic tasks via superpowers:subagent-driven-development, dispatch ONE final cross-commit code-reviewer subagent with the FULL commit-range as context, focused specifically on the lenses that per-task-reviews structurally cannot apply.
When per-task reviews are insufficient
Per-task two-stage-reviews (Spec-Compliance + Code-Quality) verify:
- Task N's diff matches Task N's spec ✓
- Task N's diff is well-written code ✓
They do NOT verify:
- Task N's code is consistent with Task M's code (M ≠ N)
- The accumulated implementation matches the SPEC-TEXT at end-to-end-behavior-level
- The data-flow chain (loader → core-function → consumer → renderer) preserves field semantics
The 6 aggregate-review-lenses
| Lens | Per-task miss-pattern | Detectable how |
|---|---|---|
| 1. State-Machine-Order | Spec says "block-first-then-resume", code does "resume-first-then-block" — per-task review checks signature + return shape but not algorithmic ordering against prose spec | Read spec-text-clauses side-by-side with implementation |
| 2. Naming-Drift | Task 1 introduces paused_indices, Task 3 references paused_trade_ids — both work but semantic confusion accumulates |
Grep all introduced names + cross-check usages |
| 3. Data-Flow-Integrity | Task 1 returns set[int], Task 2 expects set[int] | None, Task 4 uses result['c1_total_blocked']. Each per-task pair fits but full chain fragility hidden |
Trace one complete data path end-to-end |
| 4. Cross-Commit-Commit-Message-Honesty | Per-task review checks one commit. Aggregate checks: does each of N commits in the chain accurately describe its diff, or does one commit silently lie? | Read every commit message vs its diff |
| 5. Pre-Commit-Hook-Coverage-Gap | Lazy import inside function body bypasses pytest --collect-only. Per-task review didn't notice because the lazy-import worked at runtime. |
Search for ^\s+from .* import (mid-function imports) |
| 6. Test-Suite-Cohesion | Task 1 tests use assert x == 0, Task 2 tests use assert skipped.get('x', 0) == 0 defensively, Task 3 tests use re.search(...). Inconsistent assertion-styles per per-task-review-author. Aggregate sees the suite as a whole. |
Read all new tests as a single suite |
How to dispatch
Agent({
description: "Aggregate code-review all N tasks",
subagent_type: "pr-review-toolkit:code-reviewer",
model: "sonnet", # standard, not the lightest
prompt: """
Aggregate cross-commit code-review of an N-commit feature implementation.
Per-commit reviews already passed each commit individually. This review
looks for cross-commit issues that per-commit reviews could miss.
## Commit Range
BASE_SHA: <commit-before-feature-work>
HEAD_SHA: <current-HEAD>
Full diff: git diff BASE..HEAD
## Spec + Plan
Spec: <path>
Plan: <path>
## Cross-Commit Review Focus (the 6 lenses)
1. State-Machine / Algorithm-Order drift between spec-text-clauses and
accumulated code
2. Naming consistency across all N commits (variable renames, field-names)
3. Architectural integrity end-to-end: trace one complete data-flow from
<loader> through <core> to <consumer/renderer>. Field-name-drift or
shape-mismatch?
4. Commit-message-honesty across all N commits (each commit message
matches its diff per `commit-message-honesty-precheck`)
5. Pre-commit-hook coverage: any lazy imports inside function bodies
that would bypass `pytest --collect-only`?
6. Test-suite cohesion: do the K new tests form a coherent suite, or
are there assertion-style inconsistencies?
Categories: Strengths / Critical / Important / Minor.
Assessment: APPROVED / NEEDS_FIXES + suitability for next stage.
"""
})
The Genesis Case
8 atomic commits (4 task-commits + 4 fix-commits from per-task code-reviews) for a backtest simulation feature.
Per-task reviews passed all 8 commits individually. 33/33 tests green.
The aggregate-cross-commit-review found:
| Lens | Finding |
|---|---|
| 1. State-Machine-Order | Spec-text said "if paused: add(i) FIRST (unconditionally), then check resume". Code did "if paused: check resume FIRST, then add(i) only if still paused". Both per-task reviewers checked the implementation against the per-task spec's signature+return-shape but missed the algorithmic-ordering-text. Impact: 1 unit per resume-event differential — not measurement-blocking but a real spec-vs-code-drift. Resolution: documented as deliberate live-mirror semantics + spec-doc-update. |
| 5. Pre-Commit-Hook-Coverage | from strategic.macro_pause import simulate_pause_periods was a lazy import inside a run_backtest function body. Per-task review didn't flag it because the lazy-import works at runtime. Aggregate review noted: pytest --collect-only only catches module-level imports, so import-break would silently land. Fix: hoist to module-level. |
These two issues were invisible to per-task reviews by construction.
Anti-patterns
- ❌ Treating per-task two-stage-review as sufficient end-of-feature verification — they're necessary but cross-commit drift is a separate failure mode
- ❌ Dispatching the aggregate-review with the same prompt as per-task code-quality-review — same prompt = same lens = same blind spots. The aggregate prompt MUST explicitly name the 6 cross-commit lenses.
- ❌ Running aggregate-review on a 1-2-commit branch — there's no cross-commit accumulation possible. Use
superpowers:requesting-code-reviewdirectly. - ❌ Skipping aggregate-review because "all tests pass" — tests verify runtime-behavior-on-fixtures, not architectural integrity or commit-message-honesty.
Future refinements
Non-blocking polish items that would tighten the workflow further:
- Codify the dispatch-call as a reusable helper (e.g. an
aggregate-review.mdtemplate that takes BASE_SHA / HEAD_SHA / Spec-Path / Plan-Path) - Document a concrete sequencing pattern: aggregate-review goes BEFORE
superpowers:finishing-a-development-branch, AFTER the last per-task review - Add at least 1 more genesis-case from a different feature-domain to validate transferability
- Test the workflow in a session where the aggregate-review finds nothing — verify the false-negative-rate is acceptable
- Document interaction with
subagent-driven-developmentskill: should the SDD-skill itself trigger this at "all tasks complete"?
Related skills
- Upstream:
superpowers:subagent-driven-development— this skill is the natural epilogue - Sibling:
superpowers:requesting-code-review— single-commit / single-PR equivalent - Downstream:
superpowers:finishing-a-development-branch— aggregate-review-approval is the precondition
Genesis-session metadata
- Feature: Multi-task backtest simulation
- Commit-range: 8 atomic commits after spec/plan
- Per-task two-stage-reviews: all 8 passed individually
- Aggregate-review findings: 2 Important + 3 Minor (would-have-been-missed by per-task reviews)
- ABC-Verdict: A ✅ Repeatable (every SDD-driven plan with ≥3 atomic tasks), B ✅ Prevents-Error (silent spec-vs-code-drift, hook-coverage-gaps), C ✅ Transferable (cross-commit reviewing is domain-independent)