fix: add hard gates to prevent review skipping in subagent-driven-development#613
fix: add hard gates to prevent review skipping in subagent-driven-development#613philippevezina wants to merge 3 commits intoobra:mainfrom
Conversation
…elopment Claude frequently skips spec compliance and code quality reviews under context pressure, rationalizing it as "going faster." This adds structural guards: a <HARD-GATE> block at the top, a mandatory completion gate checklist, and a rationalization prevention table targeting observed failure modes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a HARD-GATE enforcing dispatch and completion of two reviewer subagents per task (spec and code-quality), a per-task Completion Gate checklist, rationalization-prevention rules, explicit reviewer prompt templates, an example per-task review workflow, and expanded guidance (advantages, costs, red flags, integration/policy prompts). Changes
Sequence Diagram(s)sequenceDiagram
participant Controller
participant Implementer
participant SpecReviewer
participant CodeReviewer
participant TaskRecord
Controller->>Implementer: Dispatch task
Implementer->>Controller: Submit implementation (mark ready)
Controller->>SpecReviewer: Dispatch spec review subagent
Controller->>CodeReviewer: Dispatch code-quality review subagent
SpecReviewer->>Controller: Spec review result (approve / request changes)
CodeReviewer->>Controller: Code-quality result (approve / request changes)
Controller->>Implementer: Request fixes if changes requested
Implementer->>SpecReviewer: Re-dispatch spec review (if needed)
Implementer->>CodeReviewer: Re-dispatch code review (if needed)
SpecReviewer->>TaskRecord: Record approval
CodeReviewer->>TaskRecord: Record approval
Controller->>TaskRecord: Verify Completion Gate checklist -> close task
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
skills/subagent-driven-development/SKILL.md (1)
97-110: Tighten the completion gate to require evidence, not only checklist intent.Consider explicitly requiring recorded proof (reviewer outputs / verification artifacts) before checking boxes, to align with the repo’s verification-before-completion gate and reduce “checkbox-only” compliance drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/subagent-driven-development/SKILL.md` around lines 97 - 110, Update the "Completion Gate (Mandatory Per Task)" section to require concrete evidence for each checklist item rather than just checking boxes: add explicit requirements that the "Implementer subagent", "Spec reviewer subagent", and "Code quality reviewer subagent" entries must include attached reviewer outputs (e.g., reviewer transcripts, diff links, CI artifacts, or signed verification notes) and state that any fixes must include re-review artifacts before a box can be checked; reference the exact checklist lines (the four bullets under "Completion Gate (Mandatory Per Task)") and add a short sentence explaining acceptable artifact types and where to attach them (e.g., review comment threads or artifacts folder) so completion is only allowed when those artifacts are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/subagent-driven-development/SKILL.md`:
- Around line 101-106: The markdown fenced checklist at the block starting with
the three backticks is unlabeled and triggers markdownlint MD040; update that
fence in SKILL.md (the checklist block containing "Implementer subagent
dispatched and completed" etc.) to include a language identifier (e.g., add
"text" after the opening ``` so it becomes ```text) to silence the lint warning.
---
Nitpick comments:
In `@skills/subagent-driven-development/SKILL.md`:
- Around line 97-110: Update the "Completion Gate (Mandatory Per Task)" section
to require concrete evidence for each checklist item rather than just checking
boxes: add explicit requirements that the "Implementer subagent", "Spec reviewer
subagent", and "Code quality reviewer subagent" entries must include attached
reviewer outputs (e.g., reviewer transcripts, diff links, CI artifacts, or
signed verification notes) and state that any fixes must include re-review
artifacts before a box can be checked; reference the exact checklist lines (the
four bullets under "Completion Gate (Mandatory Per Task)") and add a short
sentence explaining acceptable artifact types and where to attach them (e.g.,
review comment threads or artifacts folder) so completion is only allowed when
those artifacts are present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 36879f6f-ea1c-4a2c-97ce-de8bf51fdf76
📒 Files selected for processing (1)
skills/subagent-driven-development/SKILL.md
obra
added
bug
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
skills/subagent-driven-development/SKILL.md (1)
311-314: Unify who owns follow-up work after a failed subagent run.These lines say to dispatch a new fix subagent, but the earlier reviewer loop says the implementer handles fixes. That leaves the controller with two different recovery paths for the same task. Pick one rule and use it consistently across the workflow.
Possible wording
**If subagent fails task:** -- Dispatch fix subagent with specific instructions -- Don't try to fix manually (context pollution) +- Re-dispatch the implementer for the same task with specific instructions +- Don't fix it manually in the controller context🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/subagent-driven-development/SKILL.md` around lines 311 - 314, Unify ownership of follow-up work for failed subagent runs by choosing one recovery path and making the SKILL.md text consistent: either change the "If subagent fails task" bullet to say the reviewer/implementer handles fixes (matching the reviewer loop) or change the reviewer loop to state the controller must dispatch a "fix subagent" for failed runs—update all references so the policy is consistent (search for "If subagent fails task", "Dispatch fix subagent", and the reviewer loop wording) and ensure the chosen rule is used throughout the workflow description.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/subagent-driven-development/SKILL.md`:
- Around line 99-112: The Completion Gate checklist in SKILL.md currently allows
behavior-changing fixes to bypass the spec reviewer; update the "Completion Gate
(Mandatory Per Task)" checklist by adding a new explicit checkbox after "Any
issues found by reviewers were fixed AND re-reviewed" that reads something like:
"If any post-review fix changed behavior or scope, spec reviewer re-approved the
final state" so any behavioral/scope changes require spec re-approval before
marking complete; ensure the new line matches the existing checkbox formatting
and clarify placement under the same section header.
---
Nitpick comments:
In `@skills/subagent-driven-development/SKILL.md`:
- Around line 311-314: Unify ownership of follow-up work for failed subagent
runs by choosing one recovery path and making the SKILL.md text consistent:
either change the "If subagent fails task" bullet to say the
reviewer/implementer handles fixes (matching the reviewer loop) or change the
reviewer loop to state the controller must dispatch a "fix subagent" for failed
runs—update all references so the policy is consistent (search for "If subagent
fails task", "Dispatch fix subagent", and the reviewer loop wording) and ensure
the chosen rule is used throughout the workflow description.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b76dc521-4d51-4bf9-b878-1991b3230e2c
📒 Files selected for processing (1)
skills/subagent-driven-development/SKILL.md
| ## Completion Gate (Mandatory Per Task) | ||
|
|
||
| Before marking ANY task complete, verify ALL of these are true: | ||
|
|
||
| ```text | ||
| □ Implementer subagent dispatched and completed | ||
| □ Spec reviewer subagent dispatched → result was ✅ | ||
| □ Code quality reviewer subagent dispatched → result was ✅ | ||
| □ Any issues found by reviewers were fixed AND re-reviewed | ||
| ``` | ||
|
|
||
| **If ANY box is unchecked → DO NOT mark task complete.** | ||
| **"Going faster" by skipping reviews = going slower (rework later).** | ||
| **This gate applies to every task. No exceptions. ESPECIALLY the last tasks.** |
There was a problem hiding this comment.
Require spec re-approval after behavior-changing quality fixes.
This gate still allows the final code to differ from what the spec reviewer approved: if code-quality review requests a behavioral change, the task can exit with only a code-quality re-review. Add an explicit rule that any post-spec fix affecting behavior/scope must go back through spec review before the task is marked complete.
Proposed update
## Completion Gate (Mandatory Per Task)
Before marking ANY task complete, verify ALL of these are true:
```text
□ Implementer subagent dispatched and completed
□ Spec reviewer subagent dispatched → result was ✅
□ Code quality reviewer subagent dispatched → result was ✅
□ Any issues found by reviewers were fixed AND re-reviewed
+□ If any post-review fix changed behavior or scope, spec reviewer re-approved the final state</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @skills/subagent-driven-development/SKILL.md around lines 99 - 112, The
Completion Gate checklist in SKILL.md currently allows behavior-changing fixes
to bypass the spec reviewer; update the "Completion Gate (Mandatory Per Task)"
checklist by adding a new explicit checkbox after "Any issues found by reviewers
were fixed AND re-reviewed" that reads something like: "If any post-review fix
changed behavior or scope, spec reviewer re-approved the final state" so any
behavioral/scope changes require spec re-approval before marking complete;
ensure the new line matches the existing checkbox formatting and clarify
placement under the same section header.
</details>
<!-- fingerprinting:phantom:triton:grasshopper -->
<!-- This is an auto-generated comment by CodeRabbit -->
2 participants
This adds structural guards: a block at the top, a mandatory completion gate checklist, and a rationalization prevention table targeting observed failure modes.
Motivation and Context
Claude frequently skips spec compliance and code quality reviews under context pressure, rationalizing it as "going faster." Fixes #528
How Has This Been Tested?
Tested different long-running task with different a few different changes to the prompts. Another change I tried was this but it did not give good results. So far these changes have proven to enforce the reviews pretty well.
Types of changes
Checklist