-
Notifications
You must be signed in to change notification settings - Fork 7.3k
[Feature]: /speckit.review — comments-only mode, batch-reject, post-merge verification #2054
Description
Problem Statement
During a real-world spec execution (75 tasks, PR #130 with 38 external reviewer comments), /speckit.review was invoked three times:
- Self-review: Spec compliance + code quality + security passes → all PASS
- Copilot comments: 22 comments from
copilot-pull-request-reviewer→ 8 ADDRESSED, 14 REJECTED - CodeRabbit comments: 16 comments from
coderabbitai[bot]→ 2 ADDRESSED, 14 REJECTED (1 overlap with Copilot)
Each invocation re-ran all three review passes even when the user only wanted to process new PR comments. 71% of external comments were rejected, many following obvious patterns (vendor code, salvaged reference material). A post-merge linter silently reverted several reviewed changes, which the skill did not detect.
Proposed Solution
1. --comments-only mode
Skip spec compliance / code quality / security passes entirely. Jump straight to the Comment Response Protocol for new PR comments.
/speckit.review --comments-only
This addresses the common pattern: self-review passes once, then external reviewers leave comments that need responses without re-reviewing the entire implementation.
2. Batch-reject for path patterns
When 3+ rejections share a path pattern, offer batch rejection:
Detected pattern: 6 comments target .specify/scripts/* (vendor code)
Batch reject all with: "REJECTED — vendor code managed upstream by spec-kit"? [y/N]
Configurable via an exclusion paths list in the spec feature directory:
<!-- review-exclusions.md -->
- .specify/scripts/* — vendor code, upstream responsibility
- docs/research/kanban-exploration/* — salvaged reference material, not production
- specs/legacy/* — historical specs, already executed3. Post-merge verification
After merge, diff main against the last reviewed commit. Flag any silent reversions:
Post-merge verification:
REVERTED: .github/workflows/ci.yml:3 — permissions moved back to workflow-level
REVERTED: .claude-plugin/plugin.json:3 — version changed from "0.1.0" to "5.1.1"
REVERTED: scripts/query-index.py:166 — nosec comment removed
This catches linters, auto-fixers, or post-merge hooks that silently undo reviewed changes.
4. Conversation thread resolution guidance
Add to the PR lifecycle section:
Merge readiness: If branch protection requires conversation resolution, all review threads must be resolved before merge. Use
gh api graphqlwithresolveReviewThreadmutation to batch-resolve threads after responding to all comments.
During Spec 1, 34 unresolved threads blocked the merge and required a GraphQL batch operation to resolve.
5. CI debug structure for new integrations
Replace the vague "fix and push again" loop with structured guidance for new CI tool integration:
For new CI tools (first-time scanner integration):
1. Enable one scanner at a time
2. Run locally first to identify findings
3. Commit triage per-scanner (nosec, config, exclusions)
4. Push and verify one scanner passes before adding the next
5. Expect 3-5 rounds for a multi-scanner setup
6. Reviewer profile awareness
Track what each external reviewer focuses on to pre-categorize comments:
| Reviewer | Focus Areas | Common False Positives |
|---|---|---|
| copilot-pull-request-reviewer | Error handling, input validation, type safety | Flags vendor code, reference material |
| coderabbitai[bot] | Architecture, documentation, naming | Flags salvaged historical content |
Alternatives Considered
- Separate
/speckit.respondcommand: Considered but adds command proliferation —--comments-onlyon existing command is simpler - Auto-reject all vendor paths: Too aggressive — some vendor comments may be valid (e.g., security issues in vendor code you ship)
- Skip post-merge verification: Tempting but the linter reversion problem is real and silent — better to catch it
Component
Spec templates (BDD, Testing Strategy, etc.)
AI Agent
All agents
Use Cases
- Iterative PR review: Self-review passes once, then Copilot comments arrive, then CodeRabbit comments arrive. Each pass should only process new comments, not re-run the full review.
- Vendor code noise: Projects using spec-kit have
.specify/scripts/(vendor) that external reviewers always flag. Batch-reject eliminates repetitive work. - Post-merge integrity: Linters, auto-formatters, or CI bots that run after merge can silently revert reviewed changes. Post-merge verification catches this before it becomes a bug.
- Branch protection with conversation resolution: Common enterprise GitHub setup that blocks merges until all threads are resolved. The skill should guide users through this.
Acceptance Criteria
-
--comments-onlyflag skips review passes and processes only new PR comments - Batch-reject detection when 3+ comments share a path pattern
- Optional
review-exclusions.md(or equivalent) for declaring known exclusion paths - Post-merge verification step that diffs merged code against last reviewed commit
- Conversation thread resolution mentioned in PR lifecycle documentation
- Structured CI debugging guidance for new scanner integrations
- review.md clearly separates self-review findings from external comment responses
Additional Context
Field report from perf-lab Spec 1 (002-cleanup-cicd-release): 38 external comments (22 Copilot + 16 CodeRabbit), 27 rejected (71%). Security scanner integration required 5+ CI rounds. Post-merge linter reverted workflow permissions, plugin version, and nosec comments on main.
Source file: templates/commands/review.md