Skip to content

docs(skills): scope requesting-code-review to the final SDD review#1984

Open
muunkky wants to merge 1 commit into
obra:devfrom
muunkky:fix/sdd-review-scope
Open

docs(skills): scope requesting-code-review to the final SDD review#1984
muunkky wants to merge 1 commit into
obra:devfrom
muunkky:fix/sdd-review-scope

Conversation

@muunkky

@muunkky muunkky commented Jul 13, 2026

Copy link
Copy Markdown

Closes #1481.

Who is submitting this PR? (required)

Field Value
Your model + version Claude Opus 4.8 (claude-opus-4-8), 1M context
Harness + version Claude Code 2.1.207
All plugins installed gitban (muunkky.github.io/gitban-site), frontend-design, skill-creator, claude-patent-creator-standalone. Not superpowers itself.
Human partner who reviewed this diff Cameron Rout (@muunkky)

What problem are you trying to solve?

On dev, the two skills disagree about who reviews an SDD task.

subagent-driven-development reviews each task with its own task-reviewer-prompt.md, and routes only the final whole-branch review through this skill's code-reviewer.md. But requesting-code-review says:

Mandatory:

  • After each task in subagent-driven development

Follow both as written and every SDD task gets reviewed twice — once by SDD's task reviewer, then again by a whole-branch reviewer that has no reason to run yet. That's the ambiguity @qinhaihong-red filed.

What does this PR change?

The mandatory bullet in requesting-code-review, so it points at SDD's per-task reviewer and scopes this skill to the final whole-branch review.

 **Mandatory:**
-- After each task in subagent-driven development
+- At the end of subagent-driven development, on the whole branch — its per-task review is already done by [task-reviewer-prompt.md](../subagent-driven-development/task-reviewer-prompt.md), so don't also run this skill after every task
 - After completing major feature
 - Before merge to main

One line. git diff --numstat dev1 1 skills/requesting-code-review/SKILL.md.

Is this change appropriate for the core library?

Yes — it's a contradiction between two core skills, and it costs a reviewer subagent on every task of every SDD run.

What alternatives did you consider?

The other place to fix it is subagent-driven-development, by having it state that it owns per-task review. I didn't, for two reasons: that file is currently in three of your open PRs (#1931, #1934, #1943), and the sentence that's actually wrong lives here, not there.

I also kept the fix to one bullet rather than rewriting the "Integration with Workflows" section, which says the same thing a second way — see below.

Does this PR contain multiple unrelated changes?

No. One bullet, one file.

Existing PRs

#1572 (@Derekko-web) attempted this and was closed — not on merit, but because it pointed at code-quality-reviewer-prompt.md, which 6.0.0 deleted when the two per-task reviewer prompts merged into task-reviewer-prompt.md. It would have shipped a dangling link. This one points at task-reviewer-prompt.md; I checked the relative path resolves from skills/requesting-code-review/.

#1934 touches this same file, but a different hunk: it deletes the whole ## Integration with Workflows block — which happens to contain the other half of this contradiction ("Review after EACH task"). So #1934 already fixes half of #1481 as a side effect, probably without meaning to. The **Mandatory:** bullet is the half it leaves behind, and that's what this changes. The two don't overlap and land in either order.

#1982 is also mine and also touches skills/requesting-code-review/SKILL.md. Different hunk, different problem, different filed issue: that one adds a subagent-release step to 3. Act on feedback (#1927); this one fixes the double-review contradiction in When to Request Review (#1481). Two problems, so two PRs per the guidelines — but it's the same file, so you should hear it from me. They don't conflict (line 15 vs line 47). Happy to combine them if you'd prefer one.

Environment tested

Harness Harness version Model Model version/ID
Claude Code 2.1.207 Claude Opus 4.8 claude-opus-4-8 (1M context)

Evaluation

I ran a before/after, and it found no behavioural difference. You should know that before you decide.

Two trees, identical except this one-line change. Same prompt to a fresh headless agent (claude -p … --plugin-dir <tree>), three reps each:

You are about to run the subagent-driven-development workflow on a 5-task plan. After you finish Task 1 (implementer returns DONE, task reviewer approves), do you also run the requesting-code-review skill on the branch before starting Task 2? Answer YES or NO.

On dev, with the contradiction present: NO, 3 of 3. With this change: NO, 3 of 3.

So agents already resolve it correctly. The contradiction is real in the prose — this skill says review is mandatory after each SDD task, while SDD hands per-task review to task-reviewer-prompt.md and scopes this skill to the final whole-branch review — but on this evidence it isn't causing double-reviewing in practice.

So this is a text fix, not a behaviour fix. If your bar for touching skill prose is a demonstrated behaviour change, it doesn't clear it.

Rigor

  • If this is a skills change: I used superpowers:writing-skills and completed adversarial pressure testing
  • This change was tested adversarially, not just on the happy path
  • I did not modify carefully-tuned content (Red Flags table, rationalizations, "human partner" language) without extensive evals

First box is unticked: I didn't use writing-skills and I have no pressure-test results to paste, so ticking it would be a lie.

On the second: the specific failure I checked for is the one that killed #1572 — I verified task-reviewer-prompt.md exists and that the relative link resolves from this skill's directory, so this doesn't ship a dead reference.

On the third: none of the Red Flags table, the rationalization list, or the "human partner" language is touched. This is one bullet in "When to Request Review".

Human review

  • A human has reviewed the COMPLETE proposed diff before submission

subagent-driven-development does per-task review with its own
task-reviewer-prompt.md and uses this skill only for the final
whole-branch review. requesting-code-review said it was mandatory after
every SDD task, so following both gave you two reviews per task.
@muunkky muunkky closed this Jul 13, 2026
@muunkky muunkky reopened this Jul 13, 2026
@muunkky
muunkky marked this pull request as ready for review July 13, 2026 16:32
muunkky added a commit to muunkky/superpowers that referenced this pull request Jul 14, 2026
All four open PRs under-disclosed. Each named only gitban while
frontend-design, skill-creator and claude-patent-creator-standalone were
enabled, and each claimed Claude Code 2.1.207 after the CLI moved to
2.1.208. obra#1984 went further and asserted 'No others.' — a checkable false
statement, which is the sole-sufficient kill.

The cause was here: the disclosure one-liner in SKILL.md hardcoded gitban
and a version number, so we recited the template instead of reading the
machine. SKILL.md now points at 'claude --version' and settings.json, and
preflight diffs the disclosure against both.

Also in preflight:
- --body mode. A PR body is not a comment; obra's template mandates
  headings, so the length/heading rules fired on every body. A gate that
  always cries wolf is one you learn to wave through.
- the heading check is fence-aware. '# pass 6' from node --test is TAP
  output, not a heading.
- fix three pre-existing SC1087 errors in the state-drift loop.

Ledger: log the under-disclosure and the incomplete prior-art sweep; drop
the stale 'evals blocked on an API key' risk (the RED/GREEN test is free
and we have now shipped one).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant