From c7900f169897cc75e310383464f524dff9a9a845 Mon Sep 17 00:00:00 2001 From: Jesse Vincent Date: Tue, 9 Jun 2026 21:19:08 -0700 Subject: [PATCH] Close three review blind spots found by defect tracing Live eval deliverables shipped five polish defects; tracing each through the transcripts showed three mechanisms, each now addressed: - reviewers answered pointed checklist items with unsupported yes (evidence rule: every What-to-Check answer needs file:line evidence) - no reviewer ever saw the design's global constraints (controllers now paste binding constraints into task requirements) - test output noise was invisible everywhere (pristine-output checks in implementer self-review and quality review) --- .../2026-06-09-sdd-task-scoped-review-dispatch-design.md | 5 +++-- skills/subagent-driven-development/SKILL.md | 3 +++ .../code-quality-reviewer-prompt.md | 6 ++++++ skills/subagent-driven-development/implementer-prompt.md | 1 + 4 files changed, 13 insertions(+), 2 deletions(-) diff --git a/docs/superpowers/specs/2026-06-09-sdd-task-scoped-review-dispatch-design.md b/docs/superpowers/specs/2026-06-09-sdd-task-scoped-review-dispatch-design.md index 685cb144..6e30ba03 100644 --- a/docs/superpowers/specs/2026-06-09-sdd-task-scoped-review-dispatch-design.md +++ b/docs/superpowers/specs/2026-06-09-sdd-task-scoped-review-dispatch-design.md @@ -51,7 +51,8 @@ Stop delegating to `requesting-code-review/code-reviewer.md`. The per-task quali - **Spec compliance is settled:** spec review already passed; do not re-litigate requirements or plan alignment. - **Review dimensions kept:** code quality (clarity, duplication, error handling), test quality (real behavior, not mocks), maintainability, and the existing SDD-specific checks (single responsibility, independent testability, file structure from plan, file growth contributed by this change). Dropped: plan alignment, security/scalability/production-readiness dimensions, merge verdict. - **Scope budget:** start from `git diff BASE..HEAD`; read changed files first; inspect adjacent code only to evaluate a concrete risk you can name. Cross-cutting changes — lock ordering, changed function/API contracts, shared mutable state — are legitimate named risks that justify checking call sites. Do not crawl the codebase by default. -- **Test budget:** the shared principle above, plus: no package-wide suites, race detectors, or repeated/high-count runs unless you have first named a specific suspected flake or race. Otherwise, recommend heavy validation in the report instead of running it. +- **Test budget:** the shared principle above, plus: no package-wide suites, race detectors, or repeated/high-count runs unless you have first named a specific suspected flake or race. Otherwise, recommend heavy validation in the report instead of running it. Warnings or noise in the implementer's reported test output are findings — output should be pristine (the implementer's self-review checks this too). +- **Evidence rule:** reviewers answer each What-to-Check item with file:line evidence, not bare yes/no. (Added after live eval runs showed reviewers passing defects the prompt had pointed them at — an accessible-name check and a temp-dir-cleanup check both got unsupported "yes" answers while the defect sat in the reviewed diff.) - **Read-only rule** kept in trimmed form: no mutating the working tree, index, HEAD, or branch state. The `git worktree add` how-to sentence from the current templates is NOT carried into this file — a diff-scoped review never needs a checkout of another revision (same rationale as the spec-prompt cleanup below). - **Verdict:** Strengths / Issues (Critical/Important/Minor) / "Task quality: Approved | Needs fixes." @@ -65,7 +66,7 @@ Stop delegating to `requesting-code-review/code-reviewer.md`. The per-task quali ### 3. `skills/subagent-driven-development/SKILL.md` controller changes - **Model Selection:** replace "Architecture, design, and review tasks: use the most capable available model" with judgment guidance — pick reviewer models the way implementer models are picked, scaled to the diff's size, complexity, and risk. The "Task complexity signals" list is rescoped to make clear its bullets describe implementation tasks; reviewer model choice follows the same judgment, so a narrow diff review does not automatically map to "broad codebase understanding → most capable model." -- **Reviewer prompt construction** (new guidance near Red Flags): when dispatching reviewers, do not write open-ended directives ("check all uses," "run race tests if useful") without a concrete task-specific reason; do not ask reviewers to re-run tests the implementer already ran on the same code; do not pre-judge findings for the reviewer (never instruct a reviewer to ignore or not flag a specific issue — adjudicate suspected false positives in the review loop instead); per-task reviews are task-scoped gates — the broad review happens once, at the final whole-branch review. (The pre-judging rule was added after a live eval run caught the controller fabricating a "the plan forbids a shared helper" claim and instructing the quality reviewer not to flag a planted DRY violation.) +- **Reviewer prompt construction** (new guidance near Red Flags): when dispatching reviewers, do not write open-ended directives ("check all uses," "run race tests if useful") without a concrete task-specific reason; do not ask reviewers to re-run tests the implementer already ran on the same code; do not pre-judge findings for the reviewer (never instruct a reviewer to ignore or not flag a specific issue — adjudicate suspected false positives in the review loop instead); per-task reviews are task-scoped gates — the broad review happens once, at the final whole-branch review. (The pre-judging rule was added after a live eval run caught the controller fabricating a "the plan forbids a shared helper" claim and instructing the quality reviewer not to flag a planted DRY violation.) Controllers must also include the spec/design's global constraints that bind the task — version floors, naming and copy rules, platform requirements — in the requirements they paste: a live run shipped a `go 1.26.1` module floor against a "Go 1.21+" design because no reviewer ever saw the constraint. And controllers must specify a model explicitly on every dispatch — an omitted model inherits the session's (usually most expensive) model, which silently defeats model selection. - **Handling spec-reviewer ⚠️ items** (new guidance, alongside Handling Implementer Status): the controller resolves each "cannot verify from diff" item itself before marking the task complete; confirmed gaps go back to the implementer as failed spec review. - **Final review stays broad, explicitly:** the final whole-branch reviewer dispatch node gains an explicit pointer to `../requesting-code-review/code-reviewer.md`. (Today that template is reachable only through the per-task quality prompt's delegation; once that delegation is removed, an unreferenced final-review template would be orphaned.) The Integration section's note that `superpowers:requesting-code-review` provides "the code review template for reviewer subagents" is corrected to apply to the final review only. - **Example workflow:** the quality-reviewer lines in the example are updated to the new verdict vocabulary ("Task quality: Approved"); the final reviewer's "ready to merge" line stays. diff --git a/skills/subagent-driven-development/SKILL.md b/skills/subagent-driven-development/SKILL.md index de4eca33..7741a53a 100644 --- a/skills/subagent-driven-development/SKILL.md +++ b/skills/subagent-driven-development/SKILL.md @@ -149,6 +149,9 @@ final whole-branch review. When you fill a reviewer template: ignore or not flag a specific issue. If you believe a finding would be a false positive, let the reviewer raise it and adjudicate it in the review loop. +- Include the spec/design's global constraints that bind the task (version + floors, naming and copy rules, platform requirements) in the requirements + you paste — a reviewer can only enforce what you hand them. ## Prompt Templates diff --git a/skills/subagent-driven-development/code-quality-reviewer-prompt.md b/skills/subagent-driven-development/code-quality-reviewer-prompt.md index e6110739..cb9dbc64 100644 --- a/skills/subagent-driven-development/code-quality-reviewer-prompt.md +++ b/skills/subagent-driven-development/code-quality-reviewer-prompt.md @@ -61,6 +61,9 @@ Subagent (general-purpose): running it. If you cannot run commands in this environment, name the test you would run. + Warnings or other noise in the implementer's reported test output are + findings — test output should be pristine. + ## What to Check **Code quality:** @@ -81,6 +84,9 @@ Subagent (general-purpose): significantly grow existing files? (Don't flag pre-existing file sizes — focus on what this change contributed.) + Answer each item above with file:line evidence, not a bare yes or no. + An unsupported "yes" is not a review. + ## Calibration Categorize issues by actual severity. Not everything is Critical. diff --git a/skills/subagent-driven-development/implementer-prompt.md b/skills/subagent-driven-development/implementer-prompt.md index 7c50ae99..9456c0a0 100644 --- a/skills/subagent-driven-development/implementer-prompt.md +++ b/skills/subagent-driven-development/implementer-prompt.md @@ -94,6 +94,7 @@ Subagent (general-purpose): - Do tests actually verify behavior (not just mock behavior)? - Did I follow TDD if required? - Are tests comprehensive? + - Is the test output pristine (no stray warnings or noise)? If you find issues during self-review, fix them now before reporting.