mirror of
https://github.com/github/awesome-copilot.git
synced 2026-07-18 03:40:02 +00:00
chore: publish from main
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