mirror of
https://github.com/github/awesome-copilot.git
synced 2026-07-19 03:59:17 +00:00
Add copilot-pr-autopilot skill (#1944)
* Add copilot-pr-autopilot skill Skill that drives any GitHub pull request through repeated rounds of Copilot Code Review until the agent has either resolved every thread or explicitly escalated it to the human. Triggered via GraphQL (no @copilot mention needed), triages every open thread with a fix / decline / escalate rubric, replies and resolves each thread citing the pushed SHA, then re-triggers until HEAD is reviewed with zero threads awaiting the agent's reply. Includes step scripts (01 request-review, 02 check-review-status, 03 list-open-threads, 08 reply-and-resolve, 10 cleanup-outdated), shared library (_lib.ps1) with gh-CLI wrappers (Invoke-Gh, Invoke-GhGraphQL, ConvertFrom-GhJson, Assert-GhReady), reply templates, and reference docs for each step. Repo-agnostic. Requires gh CLI on PATH and repo Triage/Write for full autopilot; external PR authors get single-iteration mode with manual re-trigger via the UI re-request button or a substantive push. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review: split per-step references + add recap-gate circuit breaker - Fix #1: give steps 1/7/10 their own reference files (01-request-review.md, 07-commit-push.md, 10-cleanup.md); trim the inline bodies out of orchestration.md so it stays cross-cutting only. - Fix #3: add a recurring round-cap & recap gate to 09-convergence.md — default STOP every 10th round, recap all prior rounds, detect drift (out-of-scope / over-engineering / wrong-direction / belongs-in-separate-PR) with CONTINUE / REVERT-AND-SHIP / HAND-OFF verdicts. Agent reasoning, no new script. - Surface the gate from SKILL.md and orchestration.md; regenerate docs/README.skills.md. Markdown-only change; scripts unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(copilot-pr-autopilot): surface recap gate in decision pseudo-code; clarify Copilot+human convergence and round definition - Inject the round-cap recap gate into the '## Decision: loop back or exit' pseudo-code else-branch so an agent following the code block (not just the prose) runs the STOP-every-10th-round check before looping. - Broaden the 'never terminal' paragraph: non-convergence is driven by a Copilot finding OR a human review comment (this skill handles both); the loop ends only when there are no new comments from either source AND every open thread (Copilot or human) has an agent reply/escalation. - Define a 'round' explicitly as one execution of step 1 (01-request-review), i.e. one Copilot-review trigger — the cap counts review rounds, not tool calls or fix edits. Markdown-only; no script changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * copilot-pr-autopilot: make recap-gate round count deterministic Add 09-review-round.ps1: counts Copilot Code Review submissions straight from the PR's API history (full GraphQL pagination), so the recap-gate trigger is a derived number, not a fallible agent mental tally. This removes the exact failure mode the skill exists to survive — a count drifting across a long run (the real 156-round case). The script reports Round + RecapDue (Round % RecapInterval == 0) only; it never stops the loop or picks the verdict. CONTINUE / REVERT-AND-SHIP / HAND-OFF stays agent reasoning. 09-convergence.md updated to reference the deterministic count while preserving 'no script stops the loop' and 'non-convergence = Copilot finding OR human comment'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Regenerate docs/README.skills.md for copilot-pr-autopilot (add 09-review-round.ps1) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -0,0 +1,44 @@
|
||||
# Step 1: Request review
|
||||
|
||||
Owner: **parent** (no sub-agent); budget: n/a.
|
||||
|
||||
## Inputs
|
||||
|
||||
- `PrNumber` for the target PR.
|
||||
|
||||
## Return contract
|
||||
|
||||
- Captured `baseline` = `LatestCopilotReview.submittedAt` string (or empty)
|
||||
to be passed to step 2.
|
||||
- Boolean `single_iteration_mode` — `true` if the trigger failed because
|
||||
Copilot isn't a valid reviewer; `false` otherwise.
|
||||
|
||||
## Procedure
|
||||
|
||||
1. Snapshot first to learn whether Copilot is already pending:
|
||||
|
||||
```pwsh
|
||||
$snap = pwsh ./scripts/02-check-review-status.ps1 -PrNumber <n>
|
||||
$baseline = if ($snap -match '"submittedAt":"([^"]+)"') { $Matches[1] } else { '' }
|
||||
$pending = ($snap -match '"CopilotPending":true')
|
||||
```
|
||||
|
||||
Regex on raw JSON keeps `submittedAt` a string across the
|
||||
parent → sub-agent boundary on any PS version (5.1 / 7.x), avoiding
|
||||
`[datetime]` rebinding.
|
||||
|
||||
2. **If `$pending`** — skip the trigger; jump to step 2 with `baseline`.
|
||||
|
||||
3. **Else** — fire the trigger:
|
||||
|
||||
```pwsh
|
||||
pwsh ./scripts/01-request-review.ps1 -PrNumber <n>
|
||||
```
|
||||
|
||||
The script keeps its own `InFlight` short-circuit as a safety net,
|
||||
but the canonical "is Copilot pending?" signal lives in
|
||||
`02-check-review-status.ps1` (above).
|
||||
|
||||
4. If `01-request-review.ps1` throws because Copilot isn't a valid
|
||||
reviewer (Copilot Code Review not enabled on the repo / account),
|
||||
take the [single-iteration fallback](orchestration.md#single-iteration-fallback).
|
||||
@@ -0,0 +1,49 @@
|
||||
# Step 2: Wait for review
|
||||
|
||||
Sub-agent type: `general-purpose`; budget: **20-minute hard cap** (one
|
||||
bounded sub-agent, NOT extension-driven).
|
||||
|
||||
**Skipped** when the loop is in [single-iteration
|
||||
mode](orchestration.md#single-iteration-fallback) — there's no Copilot
|
||||
review to wait for.
|
||||
|
||||
## Inputs
|
||||
|
||||
From step 1:
|
||||
- `PrNumber`.
|
||||
- `baseline` — the `LatestCopilotReview.submittedAt` string captured
|
||||
before the trigger fired (empty string if no prior Copilot review).
|
||||
|
||||
## Return contract
|
||||
|
||||
- `02-check-review-status.ps1` JSON snapshot.
|
||||
- `recommendation` ∈ {`ready`, `give-up-push-commit`}.
|
||||
- `ready` iff **both** `LatestCopilotReview.submittedAt > baseline`
|
||||
AND `ReviewAtHead: true`.
|
||||
|
||||
## Procedure
|
||||
|
||||
Poll `02-check-review-status.ps1` approximately every **3 minutes**
|
||||
until `ready` or the 20-minute cap is hit:
|
||||
|
||||
```pwsh
|
||||
pwsh ./scripts/02-check-review-status.ps1 -PrNumber <n>
|
||||
```
|
||||
|
||||
- Extract `submittedAt` and `ReviewAtHead` from the JSON each tick.
|
||||
- Stop and return `ready` on the first tick that satisfies both
|
||||
conditions vs. the captured `baseline`.
|
||||
- On cap reached without `ready`, return `give-up-push-commit`.
|
||||
|
||||
## Gotchas
|
||||
|
||||
- **Don't poll faster than ~3 minutes.** There is no progress signal
|
||||
from the API; faster polling only burns budget.
|
||||
- **`give-up-push-commit` fallback is parent-driven.** When the
|
||||
sub-agent returns this recommendation, the **parent** pushes a
|
||||
substantive (non-whitespace) commit — auto-assign on `synchronize` is
|
||||
the most reliable trigger. Then the parent re-enters the loop at
|
||||
step 1 with a fresh `baseline`.
|
||||
- **Single bounded run, not extension-driven.** Do not request
|
||||
extensions on this step — if 20 min isn't enough, the right move is
|
||||
the `give-up-push-commit` fallback, not more polling.
|
||||
@@ -0,0 +1,53 @@
|
||||
# Step 3: List + categorize open threads
|
||||
|
||||
Sub-agent type: `explore`; budget: 5 min.
|
||||
|
||||
## Inputs
|
||||
|
||||
- `PrNumber`.
|
||||
|
||||
## Return contract
|
||||
|
||||
Table of rows, one per open thread:
|
||||
|
||||
```
|
||||
{ thread_id, file, line, author, author_class, severity, summary }
|
||||
```
|
||||
|
||||
Where `author_class` ∈ `copilot` | `human-or-bot`, derived from the
|
||||
raw `author.login` (see Gotchas).
|
||||
|
||||
## Procedure
|
||||
|
||||
Run the listing script:
|
||||
|
||||
```pwsh
|
||||
pwsh ./scripts/03-list-open-threads.ps1 -PrNumber <n>
|
||||
```
|
||||
|
||||
This returns every unresolved review thread from **all reviewers**
|
||||
(Copilot, humans, `github-advanced-security`, other bots). The script
|
||||
emits `Path` as `<file>:<line>` when the comment is anchored to a
|
||||
specific line (e.g. `src/foo.js:42`); when the comment has no line
|
||||
anchor (file-level / PR-level comments), `Path` is just `<file>` with
|
||||
no `:<line>` suffix. Callers should split on the last `:` **only when
|
||||
the suffix parses as an integer**, and treat `Path` as the file alone
|
||||
otherwise. For each row, classify the `author`:
|
||||
|
||||
- `copilot-pull-request-reviewer` or
|
||||
`copilot-pull-request-reviewer[bot]` → `author_class: copilot`
|
||||
- everything else → `author_class: human-or-bot`
|
||||
|
||||
Pass the classified table to step 4 — the triage rubric depends on it.
|
||||
|
||||
## Gotchas
|
||||
|
||||
- **The `[bot]` suffix appears on some surfaces.** Match BOTH
|
||||
`copilot-pull-request-reviewer` AND
|
||||
`copilot-pull-request-reviewer[bot]` — they're the same actor.
|
||||
- **Default human / advanced-security threads to `escalate-to-user` in
|
||||
step 4.** Classification here just flags them; triage applies the
|
||||
policy. See [04-triage.md](04-triage.md).
|
||||
- **Unresolved is the source of truth.** Outdated-but-unresolved
|
||||
threads still show up — that's correct. Don't filter them out;
|
||||
they're handled like any other open thread in step 8.
|
||||
@@ -0,0 +1,190 @@
|
||||
# Step 4: Triage
|
||||
|
||||
Sub-agent type: `general-purpose`; budget: **5 min per ≤5 threads**
|
||||
(parent batches in waves of ≤5 if step 3 returned more).
|
||||
|
||||
## Inputs
|
||||
|
||||
From step 3:
|
||||
- Classified thread table — `{ thread_id, file, line, author,
|
||||
author_class, severity, summary }` per open thread.
|
||||
- Original PR context (description, recent commits to the affected
|
||||
files) for re-deriving from principles in oscillation cases.
|
||||
|
||||
## Return contract
|
||||
|
||||
Table of rows, one per thread:
|
||||
|
||||
```
|
||||
{ thread_id, action, rationale }
|
||||
```
|
||||
|
||||
Where `action` ∈ `fix` | `decline` | `escalate-to-user` and
|
||||
`rationale` is a single line citing the rule from the rubric below
|
||||
that fired.
|
||||
|
||||
## Procedure
|
||||
|
||||
For each thread, apply the rubric in this file in order:
|
||||
|
||||
1. **Reviewer-type policy** — `human-or-bot` threads default to
|
||||
`escalate-to-user` unless the user explicitly scoped them in.
|
||||
2. **ROI vs Risk** — score and decide for `copilot` threads.
|
||||
3. **Fix / Decline rule lists** — match the finding pattern.
|
||||
4. **Project-specific policy hooks** — research repo conventions
|
||||
before applying a generic "fix".
|
||||
5. **Conflicting-comments resolution** — if the finding flips a
|
||||
prior round's edit, apply the oscillation rules; if the agent is
|
||||
about to re-do an edit it already reverted, hard-stop and escalate.
|
||||
6. **Escalation rules** — escalate on design-level tradeoffs,
|
||||
large/cross-cutting changes, or high-risk/irreversible actions.
|
||||
|
||||
Return `{ thread_id, action, rationale }` per thread to the parent;
|
||||
step 5 consumes only the `fix` rows, step 8 consumes the full table.
|
||||
|
||||
## Gotchas
|
||||
|
||||
- **Batch in waves of ≤5 per sub-agent invocation.** Larger batches
|
||||
blur the rationale and overrun the 5-min budget.
|
||||
- **Human / advanced-security default is `escalate-to-user`.**
|
||||
Auto-replying or auto-resolving a human review thread hides
|
||||
unaddressed concerns and is socially wrong.
|
||||
- **Cite the rule that fired in `rationale`** — one line, concrete.
|
||||
"Style nit per rubric §Decline" is fine; "looks small" is not.
|
||||
- **The oscillation hard-stop is non-negotiable.** If the agent is
|
||||
about to make an edit it already reverted in a prior round, that's
|
||||
the unambiguous signature of an oscillation loop — escalate.
|
||||
|
||||
---
|
||||
|
||||
# Triage Rubric
|
||||
|
||||
Decision rubric for whether to fix or decline each Copilot finding.
|
||||
The goal is correctness, not appeasement — decline confidently when
|
||||
warranted.
|
||||
|
||||
## ROI vs Risk
|
||||
|
||||
Score every proposed change on two axes, then decide:
|
||||
|
||||
- **ROI** = value of the fix MINUS the cost to implement MINUS the
|
||||
user's cost to review.
|
||||
- **Risk** = blast radius, reversibility, effect on unrelated code.
|
||||
|
||||
| ROI | Risk | Action |
|
||||
|---|---|---|
|
||||
| Clear positive | Low | Fix unilaterally. |
|
||||
| Marginal | Low | Fix if cheap; otherwise decline with rationale. |
|
||||
| Clear positive | High / Irreversible | Propose to the user first. |
|
||||
| Marginal | High | Decline. |
|
||||
| Negative (over-engineering for hypothetical) | Any | Decline. |
|
||||
|
||||
## Reviewer-type policy (foundation, not a nit)
|
||||
|
||||
Each thread's `author` (returned by `03-list-open-threads.ps1`) drives
|
||||
who can decide it:
|
||||
|
||||
| Reviewer | Default action |
|
||||
|----------|----------------|
|
||||
| `copilot-pull-request-reviewer` / `copilot-pull-request-reviewer[bot]` | Loop-owned — triage with the rubric below. (`03-list-open-threads.ps1` reports the raw `author.login`, which may carry the `[bot]` suffix on some surfaces; match both forms.) |
|
||||
| Human reviewer | **Default `escalate-to-user`** unless the user explicitly scoped them into the loop. Auto-replying or auto-resolving a human thread can hide unaddressed concerns and is socially wrong. |
|
||||
| `github-advanced-security` / other automated bots | **Default `escalate-to-user`** unless the project has a documented suppression / fix convention you can follow. |
|
||||
|
||||
## Fix when the finding is...
|
||||
|
||||
- A **real correctness bug**: use-after-free / lifetime violation,
|
||||
race that drops user intent, gating logic that skips legitimate
|
||||
transitions, missing link dependency, off-by-one, null deref,
|
||||
unhandled error path.
|
||||
- A **cross-cutting concern with a clean local fix**: moving a mutex
|
||||
one scope up, pulling a duplicated check into a helper.
|
||||
- **Documentation / test-plan drift**: PR description claims behavior
|
||||
the code no longer matches; comment block describes the wrong
|
||||
thing; test-plan checkbox is inverted vs. the code.
|
||||
|
||||
## Decline when the finding is...
|
||||
|
||||
- A **purely hypothetical race** requiring cross-class plumbing,
|
||||
where actual exposure is negligible. Cite the interleaving you
|
||||
ruled out.
|
||||
- **Style, naming, or formatting**. Out of scope for the review loop.
|
||||
- **Suggestions to add abstractions** ("introduce a strategy pattern",
|
||||
"extract an interface") that don't pay for themselves at the
|
||||
current scale of the codebase.
|
||||
- **Suggestions that contradict an established project convention** —
|
||||
consistency with surrounding code is usually more valuable than the
|
||||
suggestion in isolation.
|
||||
- **Micro-optimizations** in code that is not on a hot path.
|
||||
|
||||
## Project-specific policy hooks
|
||||
|
||||
Some findings are decided by **project policy**, not by general
|
||||
correctness reasoning. Before applying a generic "fix", **research
|
||||
the repo's own conventions first** — `.github/instructions/*.md`
|
||||
files (often have an `applyTo` glob that pins them to the changed
|
||||
file's path), `.github/skills/`, `AGENTS.md`, `CONTRIBUTING.md`, CI
|
||||
config, and recent commits to similar files. Fan out multiple
|
||||
`explore` sub-agents when several axes need checking (lint, format,
|
||||
spell-check, license header, etc.) — don't invent answers. What
|
||||
looks like an obvious fix may violate a project rule:
|
||||
|
||||
- **Spell-check / dictionary findings.** If the project uses a
|
||||
spell-checker (`check-spelling`, `cspell`, `typos`, or similar),
|
||||
inspect its config and recent commits to learn the local
|
||||
convention (reword the document, add a pattern/regex, extend a
|
||||
dictionary/allowlist, use an inline ignore). Follow that
|
||||
convention; don't invent a new mechanism.
|
||||
- **Lint suppressions / inline ignore directives.** Most projects
|
||||
require an inline rationale comment. Bare suppressions get pushed
|
||||
back.
|
||||
- **License headers / file boilerplate.** Project-specific format,
|
||||
often CI-enforced — copy the existing header from a neighbor file.
|
||||
- **Test framework, mock library, formatter choices.** Follow the
|
||||
convention of the surrounding test files; never introduce a new
|
||||
framework because Copilot suggested one.
|
||||
|
||||
Cite the project's config file or convention in your reply.
|
||||
|
||||
## Conflicting comments — break oscillation early
|
||||
|
||||
Failure mode: round N "fixes" what comment A asked for; round N+1
|
||||
comment B objects and asks to revert it. Blindly flip-flopping
|
||||
ships oscillation and burns rounds.
|
||||
|
||||
Resolution rules — apply in order, stop at the first that fires:
|
||||
|
||||
1. **Re-derive from principles.** Pick the side that wins on the
|
||||
function's contract + surrounding patterns + user's stated
|
||||
preferences — not the latest comment.
|
||||
2. **Prefer the position with the explicit rationale.** Concrete
|
||||
failure modes (security, correctness, data loss, perf) win over
|
||||
stylistic positions.
|
||||
3. **Prefer human comments over bot comments** when they directly
|
||||
conflict.
|
||||
4. **If still ambiguous, escalate to the user.** Summarize both
|
||||
positions side-by-side with a recommendation; do not silently
|
||||
pick.
|
||||
|
||||
**Hard stop**: if you are about to make the same edit you
|
||||
*reverted* in an earlier round, stop and escalate. That is the
|
||||
unambiguous signature of an oscillation loop.
|
||||
|
||||
## Reply hygiene
|
||||
|
||||
Every reply — fix or decline — states the reasoning. This makes the
|
||||
PR self-documenting and gives the next review visible context.
|
||||
Declined findings cite the prior round and explain why the existing
|
||||
form is correct, so the next reviewer doesn't raise it again.
|
||||
|
||||
## Escalation rules
|
||||
|
||||
Stay in autopilot by default; escalate to the user when:
|
||||
|
||||
- The finding identifies a **design-level tradeoff** with multiple
|
||||
reasonable resolutions and no clear winner.
|
||||
- The fix would be **large or cross-cutting** (hundreds of lines,
|
||||
new architecture, refactor across modules).
|
||||
- The action is **high-risk or irreversible** (force-push, deletion,
|
||||
credentials, production).
|
||||
|
||||
Otherwise, decide and proceed.
|
||||
@@ -0,0 +1,59 @@
|
||||
# Step 5: Apply fixes
|
||||
|
||||
Sub-agent type: `general-purpose`, **one sub-agent per finding**,
|
||||
parallel **max 5 concurrent**; budget: 5 min each. If step 4 returned
|
||||
more than 5 `fix` rows, the parent runs step 5 in waves of ≤5.
|
||||
|
||||
## Inputs
|
||||
|
||||
Per sub-agent (one finding per invocation):
|
||||
- `thread_id`, `file`, `line` from step 3.
|
||||
- The finding `summary` and Copilot's suggested fix (if any).
|
||||
- The triage `rationale` from step 4 — the agent already decided this
|
||||
is a `fix`; this sub-agent only implements it.
|
||||
|
||||
## Return contract
|
||||
|
||||
```
|
||||
{ thread_id, files_touched, summary, status }
|
||||
```
|
||||
|
||||
Where `status` ∈ `complete` | `partial` | `blocked` and `summary` is
|
||||
a one-line description of the change.
|
||||
|
||||
## Procedure
|
||||
|
||||
1. **Discover repo conventions for the area being edited — first.**
|
||||
Before writing any code, read:
|
||||
- `.github/instructions/*.md` whose `applyTo` glob matches the
|
||||
file's path,
|
||||
- `.github/skills/`,
|
||||
- `AGENTS.md`,
|
||||
- `CONTRIBUTING.md`,
|
||||
- neighbor-file patterns in the same directory and recent commits
|
||||
touching similar files.
|
||||
2. Apply the fix in line with those conventions.
|
||||
3. Return `files_touched` + a one-line `summary` + `status`.
|
||||
|
||||
## Gotchas
|
||||
|
||||
- **Max 5 concurrent fix sub-agents.** The cap prevents fix-fanout
|
||||
chaos; the parent merges results and reconciles file conflicts
|
||||
between waves before step 6.
|
||||
- **Never invent a generic answer that contradicts repo practice.**
|
||||
That's the "elephant in school" anti-pattern — a Copilot suggestion
|
||||
in isolation looks right but breaks the project's lint, format,
|
||||
spell-check, license-header, or framework conventions. The
|
||||
discovery step is mandatory, not optional.
|
||||
- **One finding per sub-agent.** Fixes that need to touch the same
|
||||
file get serialized by the parent between waves — don't merge
|
||||
multiple findings into one sub-agent invocation.
|
||||
- **Project policy beats Copilot suggestion.** If discovery surfaces
|
||||
a documented convention (spell-check allowlist mechanism, lint
|
||||
suppression style, etc.) that contradicts the suggested fix, follow
|
||||
the convention and reflect that in the reply body drafted in step 8
|
||||
(see [04-triage.md](04-triage.md#project-specific-policy-hooks)).
|
||||
- **Push back with written rationale** if implementing the fix would
|
||||
over-engineer the design for a hypothetical edge case — flip the
|
||||
triage to `decline` and return `status: blocked` with the rationale
|
||||
so step 8 drafts the right reply.
|
||||
@@ -0,0 +1,59 @@
|
||||
# Step 6: Build + test per repo conventions
|
||||
|
||||
Sub-agent type: `task` (may fan out to several `explore` sub-agents in
|
||||
parallel for discovery); budget: 10 min (extension cap up to 2× for
|
||||
slow suites).
|
||||
|
||||
## Inputs
|
||||
|
||||
- The set of files touched in step 5 (from each fix sub-agent's
|
||||
`files_touched`).
|
||||
- Whatever the parent has cached from prior rounds about the repo's
|
||||
build / test / lint command set.
|
||||
|
||||
## Return contract
|
||||
|
||||
```
|
||||
{ status, failures }
|
||||
```
|
||||
|
||||
Where `status` ∈ `pass` | `fail` and `failures` is the relevant
|
||||
excerpt from the failing tool's output (build errors, test failures,
|
||||
lint diagnostics) — enough for the parent to decide whether to loop
|
||||
back to step 5 for a follow-up fix or push as-is.
|
||||
|
||||
## Procedure
|
||||
|
||||
**Discovery first** — read and combine:
|
||||
|
||||
- `.github/instructions/*.md`,
|
||||
- `AGENTS.md`,
|
||||
- `CONTRIBUTING.md`,
|
||||
- `README.md`,
|
||||
- `package.json` scripts,
|
||||
- `Makefile`,
|
||||
- language-specific tooling configs,
|
||||
- AND recent CI workflow runs (`gh run list`, `gh run view`) to learn
|
||||
the *actual* command set in use.
|
||||
|
||||
THEN run those exact commands on the changed code. Independent
|
||||
discovery axes (build tool / test runner / lint / spelling / format)
|
||||
can be dispatched as separate `explore` sub-agents in parallel; cache
|
||||
the discovered commands per round so re-runs don't re-discover.
|
||||
|
||||
## Gotchas
|
||||
|
||||
- **Never invent generic build commands.** A broken build wastes the
|
||||
next full review cycle (3–10 min). If discovery turns up nothing,
|
||||
surface the gap — don't guess.
|
||||
- **Respect repo-specific spell-check / lint / format policies.**
|
||||
Some repos prefer rewording over allowlist entries; some have a
|
||||
patterns/regex file; some accept inline-ignore directives. Inspect
|
||||
the repo's existing config and recent commits before applying a
|
||||
generic fix.
|
||||
- **Cache discovered commands per round, not per loop.** Repo configs
|
||||
can change between rounds (a fix may add a new lint), so re-discover
|
||||
at the start of each round, but reuse within the round.
|
||||
- **Failures route back to step 5.** When `status: fail`, the parent
|
||||
re-enters step 5 with the failure excerpts as a new finding — don't
|
||||
push a broken build to satisfy step 7.
|
||||
@@ -0,0 +1,22 @@
|
||||
# Step 7: Commit and push
|
||||
|
||||
Owner: **parent** (no sub-agent); budget: n/a.
|
||||
|
||||
## Inputs
|
||||
|
||||
- The fix results from step 5 and the green build from step 6.
|
||||
|
||||
## Return contract
|
||||
|
||||
- Pushed `HeadOid` (the new commit SHA), recorded for step 8 reply
|
||||
bodies and step 9 convergence proof.
|
||||
|
||||
## Procedure
|
||||
|
||||
- Parent runs `git commit` + `git push` directly. One focused commit
|
||||
per round — bundling rounds destroys the audit trail of which finding
|
||||
drove which change and breaks `git bisect`.
|
||||
- Include the trailer:
|
||||
`Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>`.
|
||||
- Record the pushed SHA so step 8 can cite it in every reply body and
|
||||
step 9 can compare it against `LatestCopilotReview.commitOid`.
|
||||
@@ -0,0 +1,103 @@
|
||||
# Step 8: Reply (always) + resolve (conditional)
|
||||
|
||||
Sub-agent type: `general-purpose` **drafts** the reply bodies; the
|
||||
**parent** posts them (mutations stay parent-owned). Budget for the
|
||||
drafting sub-agent: 5 min.
|
||||
|
||||
Runs AFTER step 7 (commit + push) so every reply can cite the
|
||||
**pushed commit SHA**.
|
||||
|
||||
## Inputs
|
||||
|
||||
- The full triage table from step 4 — `{ thread_id, action,
|
||||
rationale }` per open thread (including `escalate-to-user`).
|
||||
- The pushed `HeadOid` from step 7.
|
||||
- The per-thread fix `summary` and `files_touched` from step 5 (for
|
||||
`fix` rows).
|
||||
|
||||
## Return contract
|
||||
|
||||
One row per open thread:
|
||||
|
||||
```
|
||||
{ thread_id, action, reply_body }
|
||||
```
|
||||
|
||||
Where `action` ∈ `fix` | `decline` | `escalate-to-user`. The parent
|
||||
consumes this to drive the resolve/no-resolve decision (see
|
||||
Procedure).
|
||||
|
||||
## Procedure
|
||||
|
||||
1. **Drafting sub-agent** produces a `reply_body` per thread by
|
||||
selecting the appropriate template (see [#templates](#templates))
|
||||
based on the triage `action`. Cite the pushed SHA from step 7 in
|
||||
`fix` replies. For `escalate-to-user`, explain the disposition and
|
||||
the open question for the human merge owner; do not promise a
|
||||
resolve.
|
||||
2. **Parent posts each reply**, choosing whether to resolve:
|
||||
|
||||
```pwsh
|
||||
pwsh ./scripts/08-reply-and-resolve.ps1 -ThreadId <id> -Body <text>
|
||||
```
|
||||
|
||||
- `action ∈ { fix, decline }` → run as above (resolve happens).
|
||||
- `action == escalate-to-user` → **add `-NoResolve`** so the thread
|
||||
stays open for the human:
|
||||
|
||||
```pwsh
|
||||
pwsh ./scripts/08-reply-and-resolve.ps1 -ThreadId <id> -Body <text> -NoResolve
|
||||
```
|
||||
|
||||
## Gotchas
|
||||
|
||||
- **Reply to every open thread; resolve only when the loop owns the
|
||||
disposition** (`fix` or `decline`). Resolving without a reply
|
||||
leaves no record of why the issue was considered addressed.
|
||||
- **Escalated threads stay open *with our reply* explaining the
|
||||
disposition.** They're explicit hand-offs to the human merge
|
||||
owner, not loop failures — that's why convergence in step 9 can
|
||||
succeed with `OpenThreadCount > 0`.
|
||||
- **Mutations are parent-owned.** The sub-agent only drafts; it never
|
||||
posts. This keeps the audit trail of mutations on the parent and
|
||||
avoids double-post races between concurrent sub-agents.
|
||||
- **Cite the pushed SHA, not a local commit.** Step 7's recorded
|
||||
`HeadOid` is the only SHA reviewers can browse to.
|
||||
- **Reply hygiene matters for the next round.** Declines that don't
|
||||
cite reasoning get re-raised by the next Copilot review. See
|
||||
[04-triage.md](04-triage.md#reply-hygiene).
|
||||
|
||||
## Templates
|
||||
|
||||
Pick by triage action:
|
||||
|
||||
| Triage action | Template |
|
||||
|---------------|----------|
|
||||
| `fix` | [reply-fix.md](../templates/reply-fix.md) |
|
||||
| `decline` | [reply-decline.md](../templates/reply-decline.md) |
|
||||
| PR-description / comment drift acknowledgement | [reply-drift.md](../templates/reply-drift.md) |
|
||||
| Partial fix with deferred follow-up | [reply-partial.md](../templates/reply-partial.md) |
|
||||
|
||||
For `escalate-to-user`, there is no template — write a bespoke reply
|
||||
explaining the disposition and the open question, then post with
|
||||
`-NoResolve` so the thread stays open.
|
||||
|
||||
## Reply guidance
|
||||
|
||||
The reply has to do real work — it documents the decision for future
|
||||
maintainers and shapes what the next Copilot review will surface.
|
||||
|
||||
Be **concrete** (cite file paths, commit SHAs, function names),
|
||||
**direct** (no hedging when you have a position), and **brief** (2–4
|
||||
sentences is typical). Long replies usually mean the round should
|
||||
have been broken up.
|
||||
|
||||
### Anti-patterns — DO NOT use
|
||||
|
||||
- ❌ `"Thanks!"` / `"Good point."` with no substance.
|
||||
- ❌ `"Will fix later."` Either fix it now or decline with rationale;
|
||||
deferred fixes that aren't tracked anywhere get lost.
|
||||
- ❌ Resolve-without-reply. The next reviewer cannot reconstruct why
|
||||
the thread was closed.
|
||||
- ❌ `"I disagree."` with no reasoning. State the actual technical
|
||||
disagreement.
|
||||
@@ -0,0 +1,195 @@
|
||||
# Step 9: Convergence verify
|
||||
|
||||
Sub-agent type: `explore`; budget: 3 min.
|
||||
|
||||
## Inputs
|
||||
|
||||
- `PrNumber`.
|
||||
- The pushed `HeadOid` from step 7 (for the independent sanity check).
|
||||
- Whether the loop is in normal mode or [single-iteration
|
||||
mode](orchestration.md#single-iteration-fallback) (decided at step 1).
|
||||
|
||||
## Return contract
|
||||
|
||||
```
|
||||
{ converged, head_oid, latest_review_commit_oid, submitted_at,
|
||||
open_thread_count, open_threads_awaiting_reply, escalated_threads }
|
||||
```
|
||||
|
||||
`converged` is the single source-of-truth boolean — `Converged: true`
|
||||
returned by `02-check-review-status.ps1`.
|
||||
|
||||
## Procedure
|
||||
|
||||
Run the status check, passing `-SingleIteration` iff the loop took the
|
||||
fallback at step 1:
|
||||
|
||||
```pwsh
|
||||
pwsh ./scripts/02-check-review-status.ps1 -PrNumber <n>
|
||||
# single-iteration variant:
|
||||
pwsh ./scripts/02-check-review-status.ps1 -PrNumber <n> -SingleIteration
|
||||
```
|
||||
|
||||
Then run an **independent HEAD-vs-`LatestCopilotReview.commitOid`
|
||||
sanity check** — the parent's recorded `HeadOid` from step 7 should
|
||||
match `HEAD` and (in normal mode) match the latest review's
|
||||
`commitOid`.
|
||||
|
||||
## Decision: loop back or exit
|
||||
|
||||
After the status check, the parent agent **must** branch on `converged`:
|
||||
|
||||
```
|
||||
if converged == true:
|
||||
run step 10 once (cleanup outdated)
|
||||
call task_complete with proof (HeadOid, LatestCopilotReview.commitOid, submittedAt)
|
||||
DONE — exit the loop
|
||||
else:
|
||||
# non-converged = a fresh Copilot finding OR an unresolved human thread.
|
||||
# round = count of Copilot review submissions in the PR's history,
|
||||
# read deterministically from the API (NOT a mental tally):
|
||||
# pwsh ./scripts/09-review-round.ps1 -PrNumber <n> -> {Round, RecapDue}
|
||||
if RecapDue == true: # Round is 10, 20, 30, ...
|
||||
RUN THE RECAP GATE (see "Round cap & recap gate" below) BEFORE looping:
|
||||
recap ALL prior rounds, then pick CONTINUE / REVERT-AND-SHIP / HAND-OFF.
|
||||
CONTINUE -> fall through and start another round
|
||||
REVERT-AND-SHIP -> drop drifted commits, ship the in-scope result, exit
|
||||
HAND-OFF -> escalate to the user with the recap, exit
|
||||
GO BACK TO STEP 1 — start another round
|
||||
(re-trigger via 01-request-review.ps1, wait via 02-wait,
|
||||
list via 03-list-threads, triage, fix, push, reply+resolve,
|
||||
re-check via this step)
|
||||
```
|
||||
|
||||
A non-converged result is **never** terminal *on its own* — each round
|
||||
addresses the open review feedback on the previous round's HEAD,
|
||||
whether that's a **Copilot finding or a human review comment** (this
|
||||
skill handles both). The loop terminates only when there are **no new
|
||||
review comments from either source** AND **every open thread — Copilot
|
||||
or human — has a reply from the agent** (a thread the agent escalated
|
||||
to the user counts as replied; it stays open in `OpenThreadCount` as an
|
||||
explicit hand-off, not as loop work). But "never terminal" must not be
|
||||
read as "infinite": a bot-review loop has no guaranteed fixed point and
|
||||
can drift into over-engineering or oscillation. No script *enforces* a
|
||||
cap or stops the loop — capping is a reasoning decision the parent owns
|
||||
at the [round-cap recap gate](#round-cap--recap-gate-circuit-breaker)
|
||||
below. What *is* scripted is the round **count** itself
|
||||
([09-review-round.ps1](#round-cap--recap-gate-circuit-breaker)), so the
|
||||
gate's trigger is deterministic rather than a fallible mental tally
|
||||
(and oscillation — the same finding re-raised across rounds — is
|
||||
broken earlier per
|
||||
[04-triage.md](04-triage.md#conflicting-comments--break-oscillation-early)).
|
||||
|
||||
`-SingleIteration` mode is the **one** exception: by definition, it
|
||||
runs one round only (the trigger path is unavailable), and the
|
||||
`converged` result is taken as terminal whichever way it goes.
|
||||
|
||||
### Convergence semantics
|
||||
|
||||
`02-check-review-status.ps1` implements a PR-state guard plus three Converged branches (see the `Converged = if (...)` block near the end of that script for the canonical source):
|
||||
|
||||
- **PR State guard (overrides everything)** — if `State != 'OPEN'` (CLOSED / MERGED), `Converged: false` regardless of all other flags. The agent cannot push to a non-OPEN PR; surface the state change to the user and abort the loop rather than calling `task_complete`.
|
||||
- **Normal (Copilot-driven) mode** — a Copilot review exists OR `CopilotPending: true`:
|
||||
`Converged: true` iff
|
||||
`ReviewAtHead && NoNewComments && OpenThreadsAwaitingReply == 0`.
|
||||
- **Single-iteration mode** (`-SingleIteration` passed because the loop took the [fallback at step 1](orchestration.md#single-iteration-fallback)):
|
||||
`Converged: true` iff `OpenThreadsAwaitingReply == 0`. The stale-review checks can never advance without a new Copilot review, so they're omitted.
|
||||
- **No Copilot review ever observed AND not pending** (brand-new PRs with zero findings, or PRs where the trigger silently failed and the script wasn't called with `-SingleIteration`):
|
||||
`Converged: true` iff `OpenThreadsAwaitingReply == 0`. **Do NOT trust this as "loop done" before step 1 has fired** — it just means there's no human-thread work pending. The parent agent MUST run `01-request-review.ps1` first (per [step 1](01-request-review.md)) and re-check; treating brand-new-PR convergence as terminal will short-circuit the entire loop.
|
||||
|
||||
`OpenThreadCount` MAY be `> 0` when escalated-to-user threads stay
|
||||
open — that's an explicit human hand-off, not a loop failure. Return
|
||||
the list of escalated `thread_id`s so the parent can include them in
|
||||
the convergence proof.
|
||||
|
||||
## Round cap & recap gate (circuit breaker)
|
||||
|
||||
No script *enforces* a max-rounds cap or stops the loop — a hard number
|
||||
can't tell a *productive* round from a *drifting* one. Instead the parent
|
||||
agent runs a **recap gate** as reasoning: default **STOP at every 10th
|
||||
round** (10, 20, 30, …) **before** looping back to step 1, recap all
|
||||
prior rounds, and decide whether the loop is still serving the PR's
|
||||
original scope.
|
||||
|
||||
What *is* scripted is the **count**, so the gate's trigger is
|
||||
deterministic instead of a fallible mental tally. A **round** is **one
|
||||
execution of [step 1](01-request-review.md)** — one Copilot-review
|
||||
trigger at the top of the loop — which produces exactly one Copilot
|
||||
review submission. [`09-review-round.ps1`](../scripts/09-review-round.ps1)
|
||||
counts those submissions straight from the PR's API history and reports
|
||||
whether the cadence is hit:
|
||||
|
||||
```pwsh
|
||||
pwsh ./scripts/09-review-round.ps1 -PrNumber <n>
|
||||
# {"PrNumber":<n>,...,"Round":20,"RecapInterval":10,"RecapDue":true}
|
||||
```
|
||||
|
||||
Run it at the top of the non-converged branch and gate on `RecapDue`.
|
||||
Because the count is **derived from history, not remembered**, it can't
|
||||
drift even across a 100+ round run — the exact failure this gate exists
|
||||
to catch. The cap counts **review rounds** (Copilot review submissions),
|
||||
not sub-agent calls, tool calls, or individual fix edits — so a round
|
||||
that triages five threads still counts as one. The cadence is the
|
||||
`-RecapInterval` knob (default 10). The script reports the trigger only;
|
||||
it never decides the verdict.
|
||||
|
||||
This exists because an unbounded bot-review loop is the failure mode
|
||||
this skill was built to survive: a real run drifted for 156 rounds —
|
||||
later rounds "fixing" things the PR never set out to change, eventually
|
||||
reverting their own earlier fixes. The gate catches that class of drift
|
||||
early, every 10 rounds, instead of once at the end.
|
||||
|
||||
### What the recap reviews (ALL prior rounds, not just the last 10)
|
||||
|
||||
1. **Original PR scope** — the issue/PR title and the diff at the PR's
|
||||
base. This is the yardstick; everything else is measured against it.
|
||||
2. **Per-round ledger** — for each round so far: the Copilot finding,
|
||||
the disposition (fixed / declined / escalated), and the resulting
|
||||
change (files + intent in one line).
|
||||
3. **Drift signals** across the whole history:
|
||||
- **Out-of-scope** — a change that doesn't trace back to the
|
||||
original issue/PR goal (new feature, adjacent refactor, polish
|
||||
the PR never promised).
|
||||
- **Over-engineering** — defensive layers, abstractions, or config
|
||||
added solely to satisfy bot nits, not the PR's actual goal.
|
||||
- **Wrong-direction** — a fix that later rounds had to undo, work
|
||||
around, or re-fix (self-revert / oscillation across rounds).
|
||||
- **Belongs-in-separate-PR** — a legitimate improvement that is
|
||||
nonetheless unrelated to this PR's stated change.
|
||||
- **Scope/complexity growth** — diff size or file count climbing
|
||||
while the original goal was met rounds ago.
|
||||
|
||||
### Verdicts
|
||||
|
||||
| Verdict | When | Action |
|
||||
| --- | --- | --- |
|
||||
| **CONTINUE** | Every round so far traces to the original PR scope; no drift signals; Copilot is still surfacing in-scope findings. | Loop back to step 1 for the next 10-round block. |
|
||||
| **REVERT-AND-SHIP** | One or more rounds drifted (over-engineering / wrong-direction / oscillation) but the in-scope fixes are sound. | `git revert` (or drop) only the drifted commits, keep the in-scope ones, run step 6 build/test, then ship the clean result. Record which rounds were reverted in the convergence proof. |
|
||||
| **HAND-OFF** | Drift is entangled with in-scope work, the right fix is a redesign, or the change belongs in a separate PR. | Stop the loop, reply on the relevant threads, and escalate to the user with the recap and a recommendation (separate PR / redesign). Do **not** keep looping. |
|
||||
|
||||
The **trigger** is scripted but the **verdict** is agent reasoning —
|
||||
deliberately. [`09-review-round.ps1`](../scripts/09-review-round.ps1)
|
||||
makes the *count* deterministic (so the gate can't be missed), but
|
||||
*which verdict to pick* stays a judgment call: no number can tell a
|
||||
productive round from a drifting one. The recap is cheap (read the
|
||||
per-round commits + the PR base diff); the cost of *skipping* it is
|
||||
another runaway loop.
|
||||
|
||||
|
||||
|
||||
- **Trust `02-check-review-status.ps1`'s `Converged` flag, not your
|
||||
own re-derivation.** The script enforces all three conditions
|
||||
(normal mode) or the simplified condition (single-iteration) and
|
||||
is the canonical source.
|
||||
- **Don't call `task_complete` until `converged == true`.** Print
|
||||
the proof (`HeadOid`, `LatestCopilotReview.commitOid`,
|
||||
`submittedAt`, `OpenThreadsAwaitingReply: 0`, list of escalated
|
||||
threads if `OpenThreadCount > 0`) in the completion message.
|
||||
- **`-SingleIteration` is sticky to the fallback decision.** If
|
||||
step 1 took the fallback, every step 9 in this loop uses
|
||||
`-SingleIteration`; don't flip it mid-loop.
|
||||
- **PR State != OPEN aborts the loop.** If `State` is `CLOSED` or
|
||||
`MERGED`, `Converged` is forced `false` by the script's state
|
||||
guard. The parent agent cannot push to a non-OPEN PR — surface
|
||||
the state change to the user and stop the loop rather than
|
||||
retrying or calling `task_complete`.
|
||||
@@ -0,0 +1,25 @@
|
||||
# Step 10: Cleanup outdated
|
||||
|
||||
Owner: **parent** (no sub-agent); budget: n/a. Runs **once, after
|
||||
convergence** (step 9 returned `Converged: true`).
|
||||
|
||||
## Inputs
|
||||
|
||||
- `PrNumber` for the converged PR.
|
||||
|
||||
## Return contract
|
||||
|
||||
- None — step 10 is terminal. After it runs, the loop is complete and
|
||||
the parent calls `task_complete` with the convergence proof from
|
||||
step 9.
|
||||
|
||||
## Procedure
|
||||
|
||||
```pwsh
|
||||
pwsh ./scripts/10-cleanup-outdated.ps1 -PrNumber <n>
|
||||
```
|
||||
|
||||
Safety net only. Most loops converge with nothing to clean — outdated
|
||||
threads should already have been replied + resolved in step 8 like any
|
||||
other open thread. Unresolved state is the source of truth in the PR
|
||||
UI; `10-cleanup-outdated.ps1` only catches strays.
|
||||
@@ -0,0 +1,208 @@
|
||||
# GitHub API Quirks (Verified)
|
||||
|
||||
API behaviors that matter for the Copilot review loop. All verified
|
||||
against the current API surface — read this before reaching for an
|
||||
alternative API or modifying the bundled scripts.
|
||||
|
||||
## GraphQL trigger — `requestReviewsByLogin` is the supported path
|
||||
|
||||
```graphql
|
||||
mutation($p: ID!) {
|
||||
requestReviewsByLogin(input: {
|
||||
pullRequestId: $p,
|
||||
botLogins: ["copilot-pull-request-reviewer"]
|
||||
}) {
|
||||
pullRequest { number }
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Verified empirically against personal repos without Copilot Pro AND
|
||||
org repos with Copilot Enterprise. Works for both initial-add and
|
||||
re-request (no special re-request mutation).
|
||||
|
||||
Three GraphQL traps:
|
||||
|
||||
1. Mutation is **`requestReviewsByLogin`**, NOT `requestReviews`.
|
||||
`RequestReviewsInput` (used by `requestReviews`) does not expose a
|
||||
`botLogins` field, so it can't request a bot reviewer at all —
|
||||
`botLogins` is the central field on `requestReviewsByLogin`.
|
||||
2. Field is **`botLogins`**, NOT `userLogins`. The latter returns
|
||||
`Could not resolve user with login 'Copilot'`.
|
||||
3. Slug is **`copilot-pull-request-reviewer`** (the App slug). The
|
||||
display login `Copilot` returns `Could not resolve bot with slug
|
||||
'Copilot'`.
|
||||
|
||||
Verify success via a new `copilot_work_started` event on the issue's
|
||||
events feed — `GET /repos/{o}/{r}/issues/{n}/events` (see SKILL.md
|
||||
Gotchas "HTTP 200 / exit 0 is NOT proof"). Empirically this event
|
||||
type IS exposed on the `/events` endpoint (verified across 20+
|
||||
trigger rounds on PR 236); it is not timeline-only.
|
||||
`01-request-review.ps1` enforces this by comparing the event `id`
|
||||
(monotonic) before and after the trigger.
|
||||
|
||||
### Other trigger paths — DO NOT USE
|
||||
|
||||
- **`requestReviews` with `botLogins`** → input type rejects the
|
||||
field. Don't try variants.
|
||||
- **REST `POST /pulls/<n>/requested_reviewers` with
|
||||
`reviewers[]=Copilot`** → can return HTTP 201 while silently
|
||||
dropping the bot. Not used by the script.
|
||||
- **`gh pr edit --add-reviewer Copilot`** → returns `'Copilot' not
|
||||
found` on current `gh`. Not used by the script.
|
||||
|
||||
## GraphQL `latestReviews` — stale cache, do NOT use
|
||||
|
||||
```graphql
|
||||
# DO NOT — stale projection:
|
||||
pullRequest(number:$pr){ latestReviews(first:50){ nodes{...} } }
|
||||
|
||||
# USE INSTEAD — always current:
|
||||
pullRequest(number:$pr){ reviews(last:100){ nodes{...} } }
|
||||
```
|
||||
|
||||
`latestReviews` is a "latest per user" projection with stale-cache
|
||||
behavior: a fresh Copilot review can be absent for several minutes
|
||||
after submission, while `reviews(last:100)` reflects it immediately.
|
||||
Using `latestReviews` for in-flight or convergence checks causes the
|
||||
script to operate on an obsolete commit OID — either falsely
|
||||
declaring convergence or timing out for a review that already
|
||||
exists.
|
||||
|
||||
`02-check-review-status.ps1` uses `reviews(last:100)` filtered
|
||||
client-side to the Copilot reviewer login. It also emits a stderr
|
||||
warning when the result is exactly 100 reviews, so the caller knows
|
||||
the boundary was hit and the latest Copilot review COULD be older
|
||||
than the window — practically only possible if 100+ non-Copilot
|
||||
reviews landed after the last Copilot review, which doesn't happen
|
||||
in normal use. If you ever see the warning and the loop misbehaves,
|
||||
fetch the full review list manually:
|
||||
|
||||
```bash
|
||||
gh pr view <n> --json reviews --jq '.reviews[] | select(.author.login | test("copilot-pull-request-reviewer"))'
|
||||
```
|
||||
|
||||
### Tie-break for multiple Copilot reviews
|
||||
|
||||
When more than one Copilot review shares the same `submittedAt`
|
||||
(rare server-side clock collision under burst re-triggers), the
|
||||
script first prefers the review whose `commit.oid == HEAD`, then
|
||||
falls back to a stable sort. The intent is "the review that
|
||||
matches the current code is the one the agent should reply to" —
|
||||
preventing a stale-OID review from winning the tie and falsely
|
||||
flipping `ReviewAtHead` to false.
|
||||
|
||||
## Reply + resolve mutations — both work
|
||||
|
||||
```graphql
|
||||
mutation($tid: ID!, $body: String!) {
|
||||
addPullRequestReviewThreadReply(input: {
|
||||
pullRequestReviewThreadId: $tid,
|
||||
body: $body
|
||||
}) { comment { id } }
|
||||
}
|
||||
|
||||
mutation($tid: ID!) {
|
||||
resolveReviewThread(input: { threadId: $tid }) {
|
||||
thread { isResolved }
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
## `isOutdated` ≠ `isResolved` — current unresolved state is truth
|
||||
|
||||
A thread can be `isOutdated: true` (Copilot's comment points at lines
|
||||
that have since changed) while still `isResolved: false`. These
|
||||
threads:
|
||||
|
||||
- Still need reply + resolve in the per-round loop. A thread can
|
||||
become outdated mid-round when your own fix shifts the cited
|
||||
lines. Filtering on `!isOutdated` would silently drop those
|
||||
threads, leaving the PR's open-conversations list non-empty even
|
||||
after the underlying code is fixed.
|
||||
- `03-list-open-threads.ps1` therefore lists every unresolved
|
||||
thread with no `isOutdated` filter.
|
||||
- `10-cleanup-outdated.ps1` is a safety net only — for the rare
|
||||
case where a thread becomes outdated AFTER your last per-round
|
||||
fetch.
|
||||
|
||||
## Review latency — don't poll faster than ~3 min
|
||||
|
||||
Copilot reviews typically post 3–6 minutes after the request,
|
||||
occasionally up to ~10 minutes. There is no progress signal;
|
||||
polling more often than every ~3 min wastes API budget without
|
||||
making the review arrive sooner.
|
||||
|
||||
## `gh api graphql -F` coerces strings — use `-f` for `String!`
|
||||
|
||||
The `gh` CLI distinguishes its two flag forms:
|
||||
|
||||
- `-F key=value` — type inference. Values parsing as int, bool, or
|
||||
null are sent as that JSON literal.
|
||||
- `-f key=value` — always sends as raw string.
|
||||
|
||||
For any GraphQL variable declared `String!` (e.g. `owner`, `repo`,
|
||||
`body`, `tid`, `after`), use **`-f`** at call sites. A reply body that
|
||||
happens to be `"true"`, `"null"`, or all digits would otherwise be
|
||||
coerced and the call fails with a type error. Keep `-F` only for
|
||||
genuinely numeric or boolean variables (e.g. `pr: Int!`).
|
||||
|
||||
> Note: the shared `Invoke-Gh` wrapper may internally rewrite
|
||||
> `-f field=<body>` into `-F field=@<tempfile>` when the body contains
|
||||
> embedded `"` (Windows PowerShell 5.1 native-arg quoting bug — see
|
||||
> below). Even via `@file`, `-F` still applies type inference to the
|
||||
> file content (gh's documented behaviour) — this rewrite is safe
|
||||
> only because the rewrite trigger ("body contains `"`") guarantees
|
||||
> the content is a string that no JSON literal (`123`, `true`,
|
||||
> `null`, etc.) would match. Treat this `-F ...=@file` usage as an
|
||||
> internal transport detail of the wrapper, not as permission to
|
||||
> use `-F ...=@file` for arbitrary strings at call sites.
|
||||
|
||||
```powershell
|
||||
# Wrong — body could be coerced AND, under Windows PowerShell 5.1,
|
||||
# any embedded `"` in $Body will be mis-split by the native-arg
|
||||
# passer (gh sees a truncated body or a "received N args" error).
|
||||
gh api graphql -f query=$q -F body=$Body
|
||||
|
||||
# Right — go through Invoke-Gh / Invoke-GhGraphQL. The shared helper
|
||||
# auto-rewrites `-f field=<body>` and `-F field=<body>` pairs whose
|
||||
# body contains `"` to `-F field=@<tempfile>` so the value is read
|
||||
# from disk and never appears on the command line. This works
|
||||
# identically on Windows PowerShell 5.1 and PowerShell 7+.
|
||||
Invoke-GhGraphQL -GhArgs @('-f',"query=$q",'-f',"body=$Body") -Context 'reply body'
|
||||
```
|
||||
|
||||
Calling `gh` directly (e.g. via `& gh ...` or raw `gh api graphql`)
|
||||
bypasses the cross-version tempfile rewrite — if your value contains
|
||||
`"` you'll re-introduce the PowerShell-5.1-only splitting bug. Always
|
||||
funnel `gh` calls through `Invoke-Gh` / `Invoke-GhGraphQL`.
|
||||
|
||||
## Native `gh` exit codes bypass `$ErrorActionPreference`
|
||||
|
||||
`gh` is a native executable, not a PowerShell cmdlet, so a non-zero
|
||||
exit does **not** throw even when `$ErrorActionPreference = 'Stop'`.
|
||||
Without an explicit check the script will print misleading success
|
||||
messages after a failed API call, and the loop will falsely declare
|
||||
convergence on auth issues, rate limits, or transient 5xx.
|
||||
|
||||
Additional trap: `gh api graphql` can exit 0 for an HTTP 200 whose
|
||||
JSON body carries a top-level `errors` array. Treat that as a failed
|
||||
call too.
|
||||
|
||||
The shared helpers in [scripts/_lib.ps1](../scripts/_lib.ps1)
|
||||
(`Invoke-Gh` and `Invoke-GhGraphQL`) run `gh` via `& gh @args`
|
||||
with stderr redirected to a temp file (`2>$errFile`), then read
|
||||
`$LASTEXITCODE` and return `{ExitCode, Stdout, Stderr}`.
|
||||
`Invoke-GhGraphQL` additionally parses the GraphQL `errors` array
|
||||
on the response body and throws on either failure mode. All
|
||||
bundled scripts dot-source `_lib.ps1` and use these wrappers — do
|
||||
the same in any new script.
|
||||
|
||||
## `git stash push` argument order
|
||||
|
||||
```bash
|
||||
git stash push -m "local-build" -- src/path/a src/path/b # correct
|
||||
git stash push -- src/path/a src/path/b -m "local-build" # SILENTLY drops -m
|
||||
```
|
||||
|
||||
The `-m` MUST come before the `--` path separator.
|
||||
@@ -0,0 +1,106 @@
|
||||
# Orchestration — Parent-Owned Loop Control
|
||||
|
||||
Cross-cutting protocol for the Copilot PR review loop: time-boxing,
|
||||
the sub-agent delegation map, the single-iteration fallback, and the
|
||||
loop-wide notes. Every step — including the parent-owned steps 1, 7,
|
||||
and 10 — has its own `NN-*.md` contract file alongside this one; this
|
||||
file holds only what spans the whole loop.
|
||||
|
||||
Build, test, and lint commands are NOT prescribed here. Every step
|
||||
that needs them defers to the target repo's own conventions
|
||||
(`CONTRIBUTING.md`, `AGENTS.md`, `README`, `package.json` /
|
||||
`Makefile` / language tooling, etc.). Discover and follow the repo's
|
||||
existing practice — never invent build commands.
|
||||
|
||||
## Time-boxing & extension protocol
|
||||
|
||||
| Concept | Rule |
|
||||
|---------|------|
|
||||
| Default budget | 5 minutes per sub-agent invocation |
|
||||
| Sub-agent must return | `status` ∈ {`complete`, `partial`, `blocked`} + `next_action` + `needs_extension_minutes` (0 if none). Always summarize progress before the budget expires — never silently overrun. |
|
||||
| Extension | parent only extends when `status: partial` AND `next_action` is concrete; sends `write_agent "continue for N min"` with `N = min(needs_extension_minutes, 10)` |
|
||||
| Extension cap (default) | 2 extensions per step; step 6 (build/test) up to 2× for slow suites. Step 2 (wait) is a single bounded sub-agent — see [02-wait.md](02-wait.md) — not extension-driven. |
|
||||
| Parent never blocks | step 1 (request), step 7 (commit + push), step 8 reply/resolve mutations, and the `task_complete` decision stay in the parent |
|
||||
|
||||
When the cap is reached and the work is still `partial`, the parent
|
||||
narrows the input (batch smaller in step 4 / split fix scope in step 5)
|
||||
or takes the step over itself.
|
||||
|
||||
## Sub-agent delegation map
|
||||
|
||||
> **The loop:** one **round** = steps 1 → 2 → 3 → 4 → 5 → 6 → 7 → 8 → 9. After step 9, if `Converged: false`, **go back to step 1** for another round. Repeat until step 9 returns `Converged: true`; then run step 10 once and exit. **At every 10th round, the parent runs the [round-cap recap gate](09-convergence.md#round-cap--recap-gate-circuit-breaker) before looping back** — the circuit breaker that recaps all prior rounds and stops the loop if it has drifted out of the PR's original scope. See [09-convergence.md](09-convergence.md) for the convergence definition, the loop-exit / loop-back decision, and the recap gate.
|
||||
|
||||
Canonical order per round: **request → wait → list → triage → fix →
|
||||
build → commit + push → reply + resolve (citing pushed SHA) →
|
||||
convergence check**. Reply/resolve runs AFTER push so replies can cite
|
||||
the pushed commit SHA.
|
||||
|
||||
| Step | Owner | Contract |
|
||||
|------|-------|----------|
|
||||
| 1 — Request review | parent | [01-request-review.md](01-request-review.md) |
|
||||
| 2 — Wait for review | sub-agent (`general-purpose`, 20 min) | [02-wait.md](02-wait.md) |
|
||||
| 3 — List + categorize open threads | sub-agent (`explore`, 5 min) | [03-list-threads.md](03-list-threads.md) |
|
||||
| 4 — Triage | sub-agent (`general-purpose`, 5 min/≤5 threads) | [04-triage.md](04-triage.md) |
|
||||
| 5 — Apply fixes | sub-agents (`general-purpose`, parallel max 5, 5 min each) | [05-fix.md](05-fix.md) |
|
||||
| 6 — Build + test per repo conventions | sub-agent (`task` + `explore`, 10 min) | [06-build-test.md](06-build-test.md) |
|
||||
| 7 — Commit + push | parent | [07-commit-push.md](07-commit-push.md) |
|
||||
| 8 — Reply (always) + resolve (conditional) | sub-agent drafts → parent posts | [08-reply-resolve.md](08-reply-resolve.md) |
|
||||
| 9 — Convergence verify | sub-agent (`explore`, 3 min) | [09-convergence.md](09-convergence.md) |
|
||||
| 10 — Cleanup outdated (post-convergence, once) | parent | [10-cleanup.md](10-cleanup.md) |
|
||||
|
||||
## Single-iteration fallback
|
||||
|
||||
When `01-request-review.ps1` throws because Copilot Code Review isn't
|
||||
enabled on the repo / account (the GraphQL mutation reports the bot
|
||||
isn't a valid reviewer), the agent falls back to **single-iteration
|
||||
mode**:
|
||||
|
||||
- Skip step 2 (no Copilot review to wait for).
|
||||
- Run steps 3 – 8 once against whatever review threads already exist
|
||||
(human, advanced-security, other bots).
|
||||
- At step 9, pass `-SingleIteration` to `02-check-review-status.ps1` so
|
||||
the convergence check ignores the stale-review checks that can never
|
||||
advance without a new Copilot review. `Converged: true` collapses to
|
||||
`OpenThreadsAwaitingReply == 0`.
|
||||
- Re-iteration happens only when a human posts new comments later —
|
||||
re-run the skill at that point.
|
||||
|
||||
Single-iteration mode is **the agent's decision after the trigger
|
||||
fails**, not an auto-detected state — the script can't reliably tell
|
||||
"Copilot disabled" from "Copilot enabled but not yet triggered" from
|
||||
API state alone.
|
||||
|
||||
## Convergence proof
|
||||
|
||||
Print the proof of convergence in the `task_complete` message — proof,
|
||||
not assertion:
|
||||
|
||||
- `HeadOid`
|
||||
- `LatestCopilotReview.commitOid`
|
||||
- `submittedAt`
|
||||
- `OpenThreadsAwaitingReply: 0`
|
||||
- The list of any open `escalate-to-user` threads if
|
||||
`OpenThreadCount > 0`.
|
||||
|
||||
## Notes
|
||||
|
||||
- **Re-request is first-class.** `01-request-review.ps1` does not
|
||||
silently skip when Copilot has already reviewed; it issues the
|
||||
same mutation and verifies via a new `copilot_work_started` event
|
||||
(the script enforces this — see [api-quirks.md](api-quirks.md) for
|
||||
the GraphQL surface and the silent-drop trap).
|
||||
- **Outdated threads still need reply + resolve.** They show up in
|
||||
the PR UI as unresolved until you explicitly close them; step 10
|
||||
is a safety net, not the primary mechanism.
|
||||
- **Reopened / revisit requests reset the thread to step 4.** If a
|
||||
declined finding is reopened by the user (or by a follow-up
|
||||
Copilot review), pull it back into triage with the prior rationale
|
||||
as input rather than re-running the whole loop.
|
||||
- **Resumability after interruption.** On restart, snapshot HEAD,
|
||||
the latest Copilot review's `commit.oid` + `submittedAt`, the
|
||||
open-threads list, and any uncommitted local changes. Discard
|
||||
cached triage / drafts if HEAD or the open-threads set changed.
|
||||
- **Local-build patches.** For projects with uncommitted local-build
|
||||
patches held out of the PR: `git stash push -m "local-build" --
|
||||
<paths>` before committing, `git stash pop` after. Note `-m` must
|
||||
come BEFORE `--` (see [api-quirks.md](api-quirks.md)).
|
||||
Reference in New Issue
Block a user