From f462a304c6101af4383de6f593af99510e549edc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Fri, 16 Oct 2026 10:07:04 +0100 Subject: [PATCH 1/3] fix(skills/copilot-review-loop): say how a subagent waits for the review Step 3 said only that the harness re-invokes the session when the wait script exits. A subagent that did not trust the wake made up its own wait, often a background `until ! pgrep -f "await-copilot-review.sh"` loop. That loop matches its own command line and never ends, so finished subagents stayed alive for hours. Step 3 now says that the one background shell is the whole wait, to read its output file on each wake, and to run the script in the foreground with a bounded timeout when no wake comes. It forbids a second watcher, and it says that no background shell may still run when the agent reports back. Towards #97 Co-Authored-By: Claude Opus 5.5 --- skills/copilot-review-loop/SKILL.md | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/skills/copilot-review-loop/SKILL.md b/skills/copilot-review-loop/SKILL.md index 3891b35..a5383ee 100644 --- a/skills/copilot-review-loop/SKILL.md +++ b/skills/copilot-review-loop/SKILL.md @@ -107,7 +107,17 @@ One cycle, in order: ${CLAUDE_PLUGIN_ROOT}/skills/copilot-review-loop/templates/await-copilot-review.sh "$SINCE" ``` - 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. + 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. Do **not** write a foreground `sleep`/`until` loop — foreground sleep is blocked and it freezes the session. + + **That one background shell is the whole wait.** Its exit is the signal, and its output file holds the result. A subagent waits the same way as a top-level session: + + - **End your turn after the launch.** The harness re-invokes the session when the script exits. + - **On every wake, read the script's output file.** A review or a timeout message means the script has ended: go on to Step 4, or to Step 6's timeout rule. An empty file means it is still running: end your turn again. + - **If your harness gives no wake on a background exit, or you cannot tell that it does, run the same script in the foreground instead,** with an interval and a timeout below your tool's command limit, for example `await-copilot-review.sh "$SINCE" 30 540`. It ends by itself, so nothing is left running. + + Never start a second command to learn when the wait script has ended. A watcher such as `until ! pgrep -f "await-copilot-review.sh "` never exits: `pgrep -f` matches whole command lines, and the watcher's own `bash -c` line holds the pattern, so it always finds itself. The same holds for `ps | grep` on that name. + + **Before you report back or hand off, no background shell you started may still run.** List your background tasks and stop any that is still live, such as a wait script that you left behind when you stopped early. A finished subagent with a live background shell stays alive, and the user sees it as still running. 4. **Triage every finding the review carries — the comments it posted *and* the comments it suppressed** — plus any human comments already on the PR. Copilot posts some findings as inline comments and withholds others into a `Suppressed comments (N)` block in the review body; a review can carry both, or carry suppressed findings alone with nothing posted at all. The body is also where a review states its effort level, when it states one (§Review effort level is a precondition). Read the body every round, not only the comment threads, or the loop triages a fraction of the review it was handed: ``` @@ -181,6 +191,8 @@ Not every GitHub mutation in the loop is the same kind of action, and conflating | --- | --- | | "I'll `gh api .../requested_reviewers` to add Copilot" | That endpoint rejects Copilot (422). Only `gh pr edit --add-reviewer "@copilot"` works. | | "I'll `sleep` in a loop until the review lands" | Foreground sleep is blocked and freezes the session. Use the background wait script; the harness wakes you. | +| "I'm a subagent and may not be woken, so I'll background a second loop that tells me when the wait script ends" | The second loop has the same wake problem, and a `pgrep -f` loop on the script's name matches itself and never ends. Read the script's output file on each wake. With no wake at all, run the script in the foreground with a timeout below the tool's limit. | +| "The round is done, so I can report back" | Not while a background shell you started still runs. Stop it first, or the finished agent stays alive. | | "The command exited 0, so Copilot is reviewing" | `gh pr edit` can exit 0 without attaching. Confirm via `reviewRequests` before waiting. | | "Copilot suggested it, so apply it" | Copilot is confidently wrong often. Triage via `receiving-code-review`; verify before applying. | | "The finding says the doc comment is incomplete, so I extend it" | A finding is input to judge. `feature-dev-workflow:addressing-review-feedback` finds its case first: a fact that a caller needs is added, and a description of how the body works is declined. Its script runs before the push. | From 6857e9b50035d379383000d26c2107604988c5d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Fri, 16 Oct 2026 10:07:04 +0100 Subject: [PATCH 2/3] fix(skills/fanning-out-with-worktrees): leave the review wait to the loop skill The orchestrator wrote its own wait instructions into the dispatch prompt of each review-loop subagent ("poll yourself with a backgrounded command"). That advice led subagents to the self-matching pgrep wait. The ripening step now says that the prompt names the review-loop skill and adds no wait instructions of its own. Towards #97 Co-Authored-By: Claude Opus 5.5 --- skills/fanning-out-with-worktrees/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/skills/fanning-out-with-worktrees/SKILL.md b/skills/fanning-out-with-worktrees/SKILL.md index 9829840..24c9180 100644 --- a/skills/fanning-out-with-worktrees/SKILL.md +++ b/skills/fanning-out-with-worktrees/SKILL.md @@ -103,7 +103,7 @@ When a sub-PR is ready (subagent reports `ready` and the relevant verification c Per sub-PR, in order: -1. **Automated review-loop, if the state file's `sub_pr_review_loop` is `on`.** **OPTIONAL SUB-SKILL:** `feature-dev-workflow:copilot-review-loop` against the sub-PR number, in fan-out mode — it requests the automated (Copilot) review, waits for it in the background, and drives the PR to review-clean. In fan-out mode a comment the loop wants to push back on does **not** pause the fan-out: log it as a bubble-up concern in the `## Bubble-up log` (surfaced at the wave checkpoint by `feature-dev-workflow:reviewing-feature-progress`) and continue. Skip this step when `sub_pr_review_loop` is `off` or missing. It runs before the orchestrator's own review so the diff reviewed below is the post-automated-review diff that will actually merge. +1. **Automated review-loop, if the state file's `sub_pr_review_loop` is `on`.** **OPTIONAL SUB-SKILL:** `feature-dev-workflow:copilot-review-loop` against the sub-PR number, in fan-out mode — it requests the automated (Copilot) review, waits for it in the background, and drives the PR to review-clean. When a subagent runs the loop, its prompt names the skill and adds no wait instructions of its own: the skill's Step 3 says how to wait for the review and that no background shell may still run when the subagent reports back. In fan-out mode a comment the loop wants to push back on does **not** pause the fan-out: log it as a bubble-up concern in the `## Bubble-up log` (surfaced at the wave checkpoint by `feature-dev-workflow:reviewing-feature-progress`) and continue. Skip this step when `sub_pr_review_loop` is `off` or missing. It runs before the orchestrator's own review so the diff reviewed below is the post-automated-review diff that will actually merge. 2. **Two-stage review — spec-compliance first, then code quality.** Both stages are the `review` skill, run against the sub-PR number with full PR context, scoped differently. This review is weaker than external review (which lands at the integration PR) but stronger than nothing; it catches issues that would otherwise pile onto the integration PR reviewer. The order is a gate, not a preference: a PR that doesn't yet implement its sub-issue is going back regardless, so quality findings on code about to change are wasted effort and noise. One blended "looks good" pass is the failure this split exists to prevent. 1. **Spec-compliance pass (the gate).** **REQUIRED SUB-SKILL:** the `review` skill, scoped to *intent*: walk the sub-issue's acceptance criteria one by one and map each to real code in the diff **and** a test that exercises it. Confirm nothing is missing and nothing extra was built — unrequested scope is a finding, not a bonus (it may collide with a sibling PR or belong elsewhere). This pass must come back clean before the quality pass starts. 2. **Code-quality pass.** **REQUIRED SUB-SKILL:** the `review` skill, scoped to *craft*: bugs the criteria didn't name, edge cases, error handling, test quality, simplification/reuse. Beyond bugs, check the diff against the `## Conventions` block — does its layout, naming, and vocabulary match the block and the sibling PRs already merged? A convention violation caught here is one the integration reviewer never sees. From 76c6f2a84bf9bafb7ad8fdb5065febe4c274733e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Fri, 16 Oct 2026 10:11:00 +0100 Subject: [PATCH 3/3] fix(skills/copilot-review-loop): judge the wait by its exit code, not its output An empty output file does not show that the wait script still runs. The script can exit early without output, for example when `gh repo view` fails under `set -e`. The agent then ends its turn again and waits for a wake that already came. Step 3 now reads the task's exit code first, and on an exit other than 0 or 124 it reports the failure and does not wait again. Towards #97 Co-Authored-By: Claude Opus 5.5 --- skills/copilot-review-loop/SKILL.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/skills/copilot-review-loop/SKILL.md b/skills/copilot-review-loop/SKILL.md index a5383ee..2636af8 100644 --- a/skills/copilot-review-loop/SKILL.md +++ b/skills/copilot-review-loop/SKILL.md @@ -112,7 +112,7 @@ One cycle, in order: **That one background shell is the whole wait.** Its exit is the signal, and its output file holds the result. A subagent waits the same way as a top-level session: - **End your turn after the launch.** The harness re-invokes the session when the script exits. - - **On every wake, read the script's output file.** A review or a timeout message means the script has ended: go on to Step 4, or to Step 6's timeout rule. An empty file means it is still running: end your turn again. + - **On every wake, read the task's status and exit code from the harness, then its output file.** Exit 0: go on to Step 4 with the review it printed. Exit 124: go to Step 6's timeout rule. Any other exit: the script failed before it could wait, so report the exit code and its output, and do not wait again. Still running: end your turn again. An empty output file alone does not show that the script still runs, because a failed script can exit without output. - **If your harness gives no wake on a background exit, or you cannot tell that it does, run the same script in the foreground instead,** with an interval and a timeout below your tool's command limit, for example `await-copilot-review.sh "$SINCE" 30 540`. It ends by itself, so nothing is left running. Never start a second command to learn when the wait script has ended. A watcher such as `until ! pgrep -f "await-copilot-review.sh "` never exits: `pgrep -f` matches whole command lines, and the watcher's own `bash -c` line holds the pattern, so it always finds itself. The same holds for `ps | grep` on that name. @@ -191,7 +191,7 @@ Not every GitHub mutation in the loop is the same kind of action, and conflating | --- | --- | | "I'll `gh api .../requested_reviewers` to add Copilot" | That endpoint rejects Copilot (422). Only `gh pr edit --add-reviewer "@copilot"` works. | | "I'll `sleep` in a loop until the review lands" | Foreground sleep is blocked and freezes the session. Use the background wait script; the harness wakes you. | -| "I'm a subagent and may not be woken, so I'll background a second loop that tells me when the wait script ends" | The second loop has the same wake problem, and a `pgrep -f` loop on the script's name matches itself and never ends. Read the script's output file on each wake. With no wake at all, run the script in the foreground with a timeout below the tool's limit. | +| "I'm a subagent and may not be woken, so I'll background a second loop that tells me when the wait script ends" | The second loop has the same wake problem, and a `pgrep -f` loop on the script's name matches itself and never ends. On each wake, read the task's exit code and output file. With no wake at all, run the script in the foreground with a timeout below the tool's limit. | | "The round is done, so I can report back" | Not while a background shell you started still runs. Stop it first, or the finished agent stays alive. | | "The command exited 0, so Copilot is reviewing" | `gh pr edit` can exit 0 without attaching. Confirm via `reviewRequests` before waiting. | | "Copilot suggested it, so apply it" | Copilot is confidently wrong often. Triage via `receiving-code-review`; verify before applying. |