diff --git a/skills/review-loop/SKILL.md b/skills/review-loop/SKILL.md index 6ed2975..1416b8a 100644 --- a/skills/review-loop/SKILL.md +++ b/skills/review-loop/SKILL.md @@ -88,8 +88,8 @@ One cycle, in order: It polls `gh api .../pulls//reviews` for a Copilot review newer than `$SINCE` and exits 0 (printing the review) when one lands, or 124 on timeout. The harness re-invokes the session when it exits. Do **not** write a foreground `sleep`/`until` loop — foreground sleep is blocked and it freezes the session. 4. **Triage every open review comment** — Copilot's new ones plus any human comments already on the PR. **REQUIRED SUB-SKILL:** `superpowers:receiving-code-review`. Verify each against the codebase. No performative agreement, no blind implementation. Copilot is confidently wrong often enough that "apply all suggestions" is the wrong default. -5. **Address.** Apply the comments that are correct, one at a time, testing each; reply in the comment thread stating what changed; resolve the thread (`resolveReviewThread`). For a comment that is wrong or a judgment call, **do not silently apply or silently ignore** — handle the pushback per the calling context (below). -6. **Re-request and loop.** Capture a fresh watermark *first*, then commit and push the fixes (commit convention from the project's CLAUDE.md). Now **wait before re-requesting** — launch the background wait (Step 3) against that watermark: +5. **Address, then always reply and resolve.** Work one comment at a time, and before moving to the next, close the loop on it in this order: (a) for a comment that is correct, apply the fix, test it, and commit it; for a comment that is wrong or a judgment call, first handle the pushback per the calling context (below) to decide the resolution; (b) post a threaded reply stating the resolution — what you changed (name the fix or commit) for ones you applied, or why the comment does not apply for ones you reject; (c) resolve the thread (`resolveReviewThread`). The reply is mandatory and is never gated behind a confirmation prompt (see GitHub-mutation discipline) — every comment gets one, including rejected ones. **Never resolve a thread without a reply, and never silently apply or silently ignore** — a resolved-but-unexplained thread destroys the reasoning trail. +6. **Re-request and loop.** The fixes were committed per comment in Step 5 (commit convention from the project's CLAUDE.md). Capture a fresh watermark *first*, then push them. Now **wait before re-requesting** — launch the background wait (Step 3) against that watermark: - **A repo that runs Copilot review on every push will produce the new review from the push alone.** Waiting first lets that auto-review land and counts it (it is newer than the pre-push watermark). Re-requesting on top of it would double the review — so do not re-request until the wait has had a chance to catch a push-triggered review. - **What a wait timeout means depends on whether the repo auto-reviews on push — and only one case authorizes a re-trigger.** On a repo that auto-reviews on push, the push already queued the review, so a timeout means it is *latent, not absent* (auto-review can lag the push by hours). Re-triggering then stacks a second review of the same commit — do **not** re-trigger; stop and report that the push-triggered review is still pending and will land on its own schedule, and the user can resume the loop when it appears. Re-trigger explicitly with **remove-then-re-add** (`gh pr edit --remove-reviewer "@copilot"` then `--add-reviewer "@copilot"` — a plain re-add often no-ops once Copilot has already reviewed) **only** when the repo does *not* auto-review on push, so the push genuinely will not produce a review by itself; then launch the wait once more. @@ -118,7 +118,10 @@ The skill is told its context when invoked. This controls Step 5 pushback only. ## GitHub-mutation discipline -Requesting the reviewer, replying in threads, resolving threads, and pushing are GitHub mutations. Follow the project rule: no mutation without a fresh confirmation against the specific thing about to land, in the interactive contexts. In autonomous fan-out, follow the autonomous-mode discipline already established for sub-PRs (the user opted into the mechanical bundle up front). +Not every GitHub mutation in the loop is the same kind of action, and conflating them is what makes the reasoning trail disappear. Separate them: + +- **Replying in a thread and resolving it are how the loop records a decision you are already making — mandatory, and never confirmation-gated.** The reply's body is dictated by the comment it answers (what you changed, or why the comment does not apply), so there is nothing separate to confirm. Do **not** pause to ask permission before leaving a reply, and do **not** skip or batch-defer it to avoid pestering. A thread addressed without a reply, or resolved silently, destroys the human-readable trace — which is the entire audit value of the loop. This holds in every context, interactive or autonomous. +- **Requesting (or re-requesting) the reviewer and pushing commits are outward, harder-to-reverse mutations.** These follow the project rule: in interactive contexts, a fresh confirmation against the specific thing about to land; in autonomous fan-out, the autonomous-mode discipline already established for sub-PRs (the user opted into the mechanical bundle up front). ## What this skill does not do @@ -142,3 +145,5 @@ Requesting the reviewer, replying in threads, resolving threads, and pushing are | "The wait timed out, so re-trigger the review" | On an auto-review-on-push repo a timeout means the review is *latent* (it can lag hours), not absent — re-triggering stacks a second review. Stop and report it is pending; only re-trigger when the repo does not auto-review on push. | | "Copilot raised this again, so it's a new problem" | A re-review may repeat comments you already addressed or dismissed. Re-verify against the current diff before acting. | | "I'll auto-dismiss the comment I disagree with" | Wrong/judgment-call comments get pushback (interactive) or a bubble-up concern (fan-out) — never a silent drop. | +| "Replying is a mutation, so I'll confirm first / batch it" | The reply records a decision you're already authorized to make; it's mandatory and ungated. Confirmation is for pushing and (re-)requesting review, not for the audit trail. | +| "I fixed and resolved it; the diff shows what changed" | A resolved thread with no reply destroys the trace — the diff shows what, not why. Reply, then resolve. |