Skip to content

Commit d34477e

Browse files
committed
docs(skills): tighten review handoff checks
1 parent ef446f3 commit d34477e

2 files changed

Lines changed: 60 additions & 3 deletions

File tree

.claude/skills/final-review/SKILL.md

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,32 @@ Collect all information needed for the review:
5858

5959
1f. **Check for conflicts with main**: Run `gh pr view <number> --json mergeable`. If there are merge conflicts, launch a subagent to merge `origin/main` into the PR branch (in a worktree) and push the merge commit.
6060

61+
1g. **PR / issue comment audit (REQUIRED)**: Final review must check the comment history before recommending merge.
62+
- Set `REPO=$(gh repo view --json nameWithOwner --jq .nameWithOwner)`
63+
- Fetch and read:
64+
- PR conversation comments: `gh api repos/$REPO/issues/<number>/comments`
65+
- PR inline review comments: `gh api repos/$REPO/pulls/<number>/comments`
66+
- PR review bodies: `gh api repos/$REPO/pulls/<number>/reviews`
67+
- linked issue comments, if an issue exists
68+
- Build a list of every actionable comment and classify each as:
69+
- `addressed`
70+
- `superseded / no longer applicable`
71+
- `still open`
72+
- Pay special attention to the `## Review Pipeline Report` comment. If it contains a `Remaining issues for final review` section, those items must be reviewed explicitly here.
73+
- Do **not** recommend merge until every actionable comment has been dispositioned.
74+
75+
1h. **Comment status summary**: Prepare a short summary for later steps:
76+
77+
> **Comment Audit**
78+
>
79+
> [N addressed, M superseded, K still open]
80+
>
81+
> Open items:
82+
> - [comment / issue summary]
83+
> - ...
84+
85+
If no actionable comments remain, report `No open actionable comments`.
86+
6187
### Step 2: Usefulness assessment
6288

6389
Think critically about whether this model/rule is genuinely useful. Consider:
@@ -212,6 +238,7 @@ Present a summary table:
212238

213239
| Aspect | Result |
214240
|--------|--------|
241+
| Comments | [All addressed / Open: X, Y] |
215242
| Usefulness | [Useful/Marginal/Not useful] |
216243
| Safety | [Safe/Concerns found] |
217244
| Completeness | [Complete/Missing: X, Y] |
@@ -229,6 +256,8 @@ Then present all numbered issues from Step 5 as a multi-select `AskUserQuestion`
229256
230257
This lets the reviewer cherry-pick exactly which issues to fix. If the reviewer selects fixes, proceed to Step 7 Quick fix. If "Merge as-is", proceed to Step 7 Merge.
231258

259+
If any actionable PR / issue comment from Step 1g is still open, `Merge as-is` must **not** be your recommendation. Recommend either **Quick fix** or **OnHold** instead.
260+
232261
### Step 7: Execute decision
233262

234263
**If Merge:**

.claude/skills/review-pipeline/SKILL.md

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -245,9 +245,21 @@ Run agentic feature tests on the modified feature:
245245
246246
2. **Invoke `/agentic-tests:test-feature`** with the identified feature. This simulates a downstream user exercising the feature from docs and examples. You MUST use the Skill tool to invoke `agentic-tests:test-feature`.
247247
248-
3. **If test-feature reports issues:** fix them, commit, and push.
249-
250-
4. **If test-feature passes:** continue to next step.
248+
3. **If test-feature reports issues:** treat every reported issue as real until you have checked it in the **current PR worktree/branch**.
249+
- Reproduce each issue from the current PR branch/worktree before acting. If it does not reproduce there, classify it as `not reproducible in current worktree`.
250+
- Auto-fix every objective issue you reasonably can: code bugs, tests, docs, help text, examples, discoverability gaps, and validation/error-message problems. Do **not** leave "minor docs issues" unfixed by default.
251+
- If you changed user-facing behavior, docs, or CLI help, re-run `/agentic-tests:test-feature`.
252+
- Classify every reported issue as exactly one of:
253+
- `fixed`
254+
- `not reproducible in current worktree`
255+
- `needs human decision`
256+
257+
4. **Only `needs human decision` issues may remain unresolved.** If any remain:
258+
- continue to the next step only after you have written them down for the final PR report
259+
- include why they were not auto-fixed
260+
- include your recommended maintainer decision
261+
262+
5. **If test-feature passes with no remaining issues:** continue to next step.
251263
252264
### 4. Fix Loop (max 3 retries)
253265
@@ -325,15 +337,30 @@ gh pr comment $PR --body "$(cat <<'EOF'
325337
| Structural review | 17/17 passed |
326338
| CI | green |
327339
| Agentic test | passed |
340+
| Needs human decision | none |
328341
| Board | Review pool → Under review → Final review |
329342
343+
### Remaining issues for final review
344+
345+
- None.
346+
330347
🤖 Generated by review-pipeline
331348
EOF
332349
)"
333350
```
334351

335352
Adapt the table values to match the actual results for the PR. If CI failed after 3 retries, report `failed (3 retries)` instead of `green`.
336353

354+
This section is **mandatory**:
355+
- `Needs human decision` must be `none` or `N item(s)`
356+
- `### Remaining issues for final review` must always be present
357+
- If there are unresolved issues, list each one as a bullet with:
358+
- the concrete problem
359+
- why it was not auto-fixed
360+
- the recommended maintainer decision
361+
362+
If unresolved issues remain, do **not** write `Agentic test | passed`. Use `passed with notes` or another accurate status.
363+
337364
### 8. Batch Mode (`--all`)
338365

339366
If `--all` was specified, repeat Steps 1-7 for each PR (including posting a PR comment per Step 7). After all PRs, print a batch summary to console:
@@ -364,3 +391,4 @@ Completed: 2/2 | All moved to Final review
364391
| Skipping merge with main | Always merge origin/main in Step 1a to catch conflicts before fixing comments |
365392
| Ignoring issue comments | Always check the linked issue (`Fix #N`) for human feedback in Step 2a |
366393
| Only checking Copilot comments | Step 2a checks human PR reviews and linked issue comments too — bot-only review is insufficient |
394+
| Saying "passed" while deferring issues | If anything remains for maintainer judgment, list it explicitly under `Remaining issues for final review` and mark the agentic result accordingly |

0 commit comments

Comments
 (0)