diff --git a/skills/issue-driven-github-flow/SKILL.md b/skills/issue-driven-github-flow/SKILL.md index 65becf6..009ef6c 100644 --- a/skills/issue-driven-github-flow/SKILL.md +++ b/skills/issue-driven-github-flow/SKILL.md @@ -168,7 +168,27 @@ and visible, not trapped in a chat): `## πŸ” Review (review agent)`. It ends with an explicit verdict line: `**Verdict: APPROVED**` or `**Verdict: CHANGES REQUESTED**`. 3. If changes are requested, the planning agent revises in a new comment. Repeat - until the review agent posts `**Verdict: APPROVED**`. + until the review agent posts `**Verdict: APPROVED**` or a hard stop below + forces human escalation. + +**Orchestrator loop bound:** the orchestrator owns enforcement, not the +subagents. Maintain a visible counter in the issue comments or dispatch notes +(`Plan-review round 1/3`, then `2/3`, then `3/3`). One round is one planning +comment plus the review verdict for it. The plan ↔ review loop is capped at +**max 3 rounds**; never start a fourth review round. + +Stop early and escalate instead of burning remaining rounds when there is no +substantive progress. Compare the latest plan/review against the prior round: +if the reviewer repeats the same objection without new information, or the +planner's revision does not materially change the plan in response, treat the +loop as non-converged. This is a judgment about substantive change, not exact +text equality. + +On cap hit or no-progress, STOP and post a concise issue comment prefixed, for +example, `## β›” Escalation: plan review did not converge`. Summarize the +unresolved disagreement, include the final round counter (`3/3` when capped), +and hand off to the human for a decision. Never silently loop, silently give up, +or begin implementation without an approved plan or explicit human direction. Post comments by piping the body on **stdin via a quoted heredoc** β€” plans and reviews are full of code fences, backticks, and `$`, and `--body "..."` would let @@ -183,8 +203,8 @@ EOF Don't skip the loop even for "small" changes β€” the review agent approving a one-line plan in a single round costs almost nothing, and the discipline is what -keeps the *big* changes honest. Cap it at ~3 rounds; if they can't converge, -stop and bring the disagreement to the user. +keeps the *big* changes honest. Keep the counter visible even when you expect a +single round. ## Step 3 β€” Implement @@ -266,6 +286,24 @@ The code-review agent posts a PR review with `**Verdict: APPROVED**` or - **Minor** findings are non-blocking unless the human decides otherwise. - The implementer uses [receiving-code-review.md](references/receiving-code-review.md) to triage feedback rigorously rather than blindly applying suggestions. +- The orchestrator tracks the code-review ↔ implement loop with a visible + counter (`Code-review fix round 1/3`, `2/3`, `3/3`). One round is one + code-review verdict plus the implementer's response/fix attempt. The loop is + capped at **max 3 rounds**; never request a fourth code-review pass for the + same unresolved Critical/Important findings. +- If the implementation agent reports `BLOCKED` or `NEEDS_CONTEXT`, stop + immediately and escalate to the human instead of using another round. +- If a round produces no substantive progress, stop early and escalate. Compare + the latest diff, implementer status, and review against the prior round: an + implementer making no material edit, or a reviewer repeating the same blocking + objection against an unchanged diff, is non-convergence. This is substantive + comparison, not exact text equality. +- On cap hit, `BLOCKED`, `NEEDS_CONTEXT`, or no-progress, STOP and post a + concise PR comment prefixed, for example, `## β›” Escalation: code review did + not converge`. Summarize unresolved Critical/Important findings or the missing + context, include the final round counter when applicable, and hand off to the + human. Do not mark the PR ready until the human resolves or explicitly waives + the blockers. - The human gives final merge approval. An agent may prepare the PR, but it does not override human approval. @@ -358,12 +396,13 @@ items across status columns and linking the project to the repo. | Default branch unprotected | Offer branch protection from [branch-protection.md](references/branch-protection.md); require consent before mutation | | >3 open issues | Create/append to a GitHub Project | | Multiple independent issues | Use worktrees via [worktrees.md](references/worktrees.md) | -| Plan written | Hand to review agent; iterate until `Verdict: APPROVED` | +| Plan written | Hand to review agent; track visible `Plan-review round N/3`; stop after max 3 rounds or no-progress and escalate to the human | | Approved plan | Dispatch implementation agent | | Step 3 branch cut | Move Project item to **In Progress** via [projects.md](references/projects.md); no-op cleanly if no Project exists or Project auth is unavailable | | Code done | Run verification, commit, open a draft PR | | Draft PR open | Dispatch πŸ”¬ code-review agent; require a verdict | -| Critical/Important review finding | Loop back to implementation; re-verify and re-review | +| Implementer reports `BLOCKED` / `NEEDS_CONTEXT` | Stop the loop immediately; post the blocker/context gap and hand off to the human | +| Critical/Important review finding | Loop back to implementation with visible `Code-review fix round N/3`; re-verify and re-review, but stop after max 3 rounds or no-progress and escalate | | PR ready to land | Require human approval and green `gh pr checks` when CI exists | | PR squash-merged | Move Project item to **Done** via [projects.md](references/projects.md); no-op cleanly if no Project exists or Project auth is unavailable | | PR merged | `--delete-branch`, sync `main`, confirm issue closed | diff --git a/skills/issue-driven-github-flow/evals/evals.json b/skills/issue-driven-github-flow/evals/evals.json index 44263db..96b3ba1 100644 --- a/skills/issue-driven-github-flow/evals/evals.json +++ b/skills/issue-driven-github-flow/evals/evals.json @@ -132,6 +132,20 @@ "Asks for explicit consent before applying branch protection", "Does not mutate branch protection without consent" ] + }, + { + "id": 9, + "name": "plan-review-nonconvergence-escalates", + "prompt": "Work on issue #42 where the review agent keeps rejecting the plan with the same unresolved objection.", + "expected_output": "Tracks the plan-review round count visibly, stops after no more than 3 review rounds, posts a concise escalation/hand-off comment to the issue summarizing the unresolved disagreement, and does not keep looping or start implementation without an approved plan.", + "files": [], + "assertions": [ + "Orchestrator tracks the plan-review loop with a visible numeric counter", + "No more than 3 review-agent comments are posted for the non-converging plan", + "A final issue comment summarizes the unresolved disagreement and hands off to the human", + "The loop stops instead of requesting another planning/review round after the cap or no-progress guard", + "No implementation branch or PR is created without a review comment ending in 'Verdict: APPROVED'" + ] } ] } diff --git a/skills/issue-driven-github-flow/references/agent-prompts.md b/skills/issue-driven-github-flow/references/agent-prompts.md index 68d8477..e073532 100644 --- a/skills/issue-driven-github-flow/references/agent-prompts.md +++ b/skills/issue-driven-github-flow/references/agent-prompts.md @@ -15,6 +15,25 @@ cheaper/faster model is often enough for mechanical implementation of a reviewed plan, while planning and especially final diff review should use a stronger model when correctness, security, or architecture judgment matters. +## Loop-bounding contract for the orchestrator + +The parent orchestrator must make both agent loops provably terminating: + +- Plan ↔ review is capped at **max 3 rounds**. +- Code-review ↔ implement is capped at **max 3 rounds**. +- Track each loop with a visible counter (`Plan-review round N/3` or + `Code-review fix round N/3`) in comments or dispatch notes; never start round + 4. +- `BLOCKED` and `NEEDS_CONTEXT` from the implementation agent short-circuit the + code-review loop immediately; do not spend another round. +- Apply the no-progress guard before the cap: compare the latest plan/diff/review + to the prior round for substantive change, not exact text equality. Repeated + objections or no material edit means non-convergence. +- On cap hit, no-progress, `BLOCKED`, or `NEEDS_CONTEXT`, STOP and post a concise + issue/PR escalation comment summarizing the unresolved disagreement, findings, + or missing context, then hand off to the human. Never silently loop and never + silently give up. + --- ## πŸ—ΊοΈ Planning agent @@ -37,6 +56,10 @@ model when correctness, security, or architecture judgment matters. > 4. If you're revising after a review, address each point the reviewer raised > explicitly, and post a new comment (don't edit the old one β€” the back-and- > forth is the record). +> 5. The orchestrator will label each attempt `Plan-review round N/3` and stop +> after max 3 rounds or earlier on no-progress. Make each revision +> substantively address the review; if you cannot resolve an objection, say so +> plainly so the orchestrator can escalate to the human instead of looping. > > Post with a quoted heredoc so your markdown (code fences, backticks, `$`) > isn't mangled by the shell: @@ -66,6 +89,10 @@ model when correctness, security, or architecture judgment matters. > actionable critique β€” cite files/lines. End with exactly one verdict line: > - `**Verdict: APPROVED**` β€” the plan is sound and ready to implement. > - `**Verdict: CHANGES REQUESTED**` β€” list what must change. +> 4. The orchestrator caps plan ↔ review at max 3 rounds with a visible +> `Plan-review round N/3` counter. If the same unresolved objection repeats +> or the plan has not substantively changed, call out non-convergence clearly +> so the orchestrator can post the escalation/hand-off comment. > > Post with a quoted heredoc so your markdown (code fences, backticks, `$`) > isn't mangled by the shell: @@ -100,13 +127,19 @@ model when correctness, security, or architecture judgment matters. > improvising a different design β€” the plan was reviewed for a reason. If > you're in over your head, stop and escalate with the specific uncertainty; > don't invent a different architecture to keep moving. -> 4. Write and RUN the automated test the plan called for; confirm it passes. -> 5. Commit: `git add -A` then +> 4. If you're fixing Critical/Important code-review findings, the orchestrator +> will label the attempt `Code-review fix round N/3` and stop after max 3 +> rounds or earlier on no-progress. Make a substantive diff for each fix +> attempt. If you cannot safely proceed, return `BLOCKED`; if required context +> is missing, return `NEEDS_CONTEXT`. Those status codes immediately escalate +> to the human and must not burn another round. +> 5. Write and RUN the automated test the plan called for; confirm it passes. +> 6. Commit: `git add -A` then > `scripts/gitflow.sh commit "" {N}` > (adds `Closes #{N}.`). -> 6. Open the PR: `scripts/gitflow.sh pr "" {N}` +> 7. Open the PR: `scripts/gitflow.sh pr "" {N}` > (the title becomes the squash commit). Its test plan should describe the -> green automated check from step 4, not a promise. +> green automated check from step 5, not a promise. > > Report the PR URL back. End with exactly one status code: > - `DONE` β€” implementation is complete, verified, committed, and the PR is open. @@ -146,6 +179,11 @@ model when correctness, security, or architecture judgment matters. > - `**Verdict: CHANGES REQUESTED**` β€” Critical or Important issues must be > resolved, or explicitly waived by the human, before the PR may be marked > ready or merged. +> 6. The orchestrator caps code-review ↔ implement at max 3 rounds with a visible +> `Code-review fix round N/3` counter. If the same Critical/Important finding +> remains after a prior round, or the diff has not substantively changed, +> identify it as non-convergence so the orchestrator can post the +> escalation/hand-off comment instead of looping. > > Post with a quoted heredoc so your markdown (code fences, backticks, `$`) > isn't mangled by the shell: