Repository navigation
fix(skills/copilot-review-loop): stop review waits that never exit - #98
Merged
Merged
Conversation
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 <noreply@anthropic.com>
…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 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new wait instructions can mistake an exited task for a running one and stall indefinitely.
1 open finding
What changed in this PR
This PR addresses #97 by clarifying how review-loop agents wait for Copilot and clean up background tasks.
Changes:
- Defines background waiting and a bounded foreground fallback.
- Forbids secondary watchers and requires cleanup before reporting.
- Keeps subagent dispatch prompts from overriding wait instructions.
| File | Description |
|---|---|
| skills/fanning-out-with-worktrees/SKILL.md | Delegates subagent wait instructions to the review-loop skill. |
| skills/copilot-review-loop/SKILL.md | Adds wait handling, cleanup requirements, and related Red Flags. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… 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 <noreply@anthropic.com>
Owner
Author
|
Review round 1. Review base: 6857e9b. Review effort: Balanced (from the review's Suppressed findings: none. The review body has no Posted findings: 1, applied in 76c6f2a (the exit code of the wait decides, not the output file). The thread has a reply and is resolved. Comment gate ( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #97
Subagents in a review loop started a background
until ! pgrep -f "await-copilot-review.sh <pr>"loop to learn when the wait script ended, and that loop never exits becausepgrep -fmatches its own command line. Step 3 ofcopilot-review-loopnow says exactly how to wait: the one background shell is the whole wait, a wake means read its exit code and then its output file, and a harness with no wake runs the same script in the foreground with a bounded timeout. It also says that no background shell may still run when the agent reports back, so a finished subagent does not stay alive.Changes
copilot-review-loopStep 3: a recipe for the wait, for a top-level session and a subagent alike. On each wake the exit code decides: 0 goes to triage, 124 goes to the timeout rule, and any other exit is reported as a failure, with no new wait. It has a foreground fallback when the harness gives no wake on a background exit, or the agent cannot tell that it does.copilot-review-loopStep 3: forbids a second command that watches the wait script, and names thepgrep -f/ps | grepself-match.copilot-review-loopStep 3: before it reports back, the agent lists its background tasks and stops any that is still live.fanning-out-with-worktreesripening step 1: the dispatch prompt for a review-loop subagent names the skill and adds no wait instructions of its own. The prompt in the incident told subagents to "poll yourself with a backgrounded command", and that advice led them to the pgrep loop.Related
"$SINCE", but each shell call runs in a new shell, so the variable from Step 1 is empty there unless the agent writes the literal timestamp. All ten test agents noticed this and wrote the literal value. The skill does not say to.Testing
I tested with the RED → GREEN loop from
superpowers:writing-skills. Each run was an isolated subagent that read only the skill file and the reproduction from #97: a background subagent at Step 3 that is not sure it gets a wake, with the dispatch prompt from the issue ("poll yourself with a backgrounded command").main, 5 runs: 2 runs started a second background loop to watch the wait script, and one of them said that the loop would still run when it reported. 2 runs read the output file again and again in the foreground. All 5 said that nothing told them to check for live background shells before they report.pgrep/ps | grepcommand, and no run left a shell running when it reported. Each run took the bounded foreground wait (30 540) and said why it did not follow the dispatch prompt's poll advice.The JSON in
.claude-plugin/parses. No script changed, so no script tests apply.🤖 Generated with Claude Code