ghcp-review-resolve

# ghcp-review-resolve Orchestrates a dual-review-and-fix pipeline on an open PR. The workflow: 0. **Preflight** — detect PR, fetch size + merge state + head SHA, passively detect any existing Copilot review, check for prior resolved reviews. Emit a preflight table. If the PR has merge conflicts with base, run the **conflict-resolution subroutine** (study recent merged PRs in the repo for resolution conventions, then rebase and resolve) before continuing — merge conflicts are no longer a blocker. The only remaining blocker is "nothing useful to do" (e.g., prior Copilot review fully resolved at HEAD). 1. **Collect two reviews** synchronously by spawning two distinct subagent reviewers in parallel (Review A + Review B). If the PR also has a fresh Copilot review, read it as supplementary context (Review C); it does not replace either subagent. 2. Independently adjudicate findings via a third subagent that inspects the actual code. 3. Post inline PR comments only for verified bugs/fixes. 4. Run a tight inline fix loop per comment: edit → test → commit → push → reply on the thread → resolve the thread. 5. **Render a verdict and act on it** — `APPROVE` (clean), `APPROVE-WITH-CHANGES` (findings verified, fixed-or-noted), `CLOSE` (PR is fundamentally unsound), or `WITHHELD` (CI red post-fix and regression unresolvable — emit a blocker, no APPROVE, no CLOSE). Submit an APPROVE review, close the PR, or emit a blocker accordingly. 6. Summarize — never submit REQUEST_CHANGES, never merge, never assign reviewers. ## Why this exists Automated reviewers produce lots of findings. Some are real bugs. Some are stylistic noise. Some contradict each other. Blindly "fix everything the bots said" is how you ship regressions or waste a day on non-issues. This skill's job is to be the adult in the room: produce two independent reviews, have a fresh adjudicator verify each finding against the actual code, and only act on what's real. Overlapping findings are high-confidence. Unique findings are kept only when high-severity and verifiable. It also knows when **not** to run. A PR whose prior Copilot review is already fully resolved at the current HEAD shouldn't trigger another round of bot noise — the skill reports that state and gets out of the way. Merge conflicts, by contrast, are not a reason to stop: the skill resolves them as a preflight subroutine before reviewing. **The skill never assigns reviewers.** Asking GitHub to add `@copilot` (or anyone else) as a reviewer is silently dropped on most repos and 422s on others, and chasing those failure modes was a recurring source of wasted runs. Instead the skill always spawns two fresh subagent reviewers it controls, and treats any pre-existing Copilot review on the PR as supplementary context (Review C) for the adjudicator. ## Step 0 — Preflight The preflight is the first and only place allowed to abort the run. If it passes, every later step trusts its flags. If a blocker is reported, no review is produced, no comment is posted, no fix is attempted. ### 0a. Basic environment ```bash gh auth status ``` If this fails, stop with a clear error. Do not proceed. ### 0b. Detect the PR Auto-detect the PR for the current branch: ```bash PR_NUMBER=$(gh pr view --json number -q .number 2>/dev/null) ``` If the user passed an argument, prefer that. If `PR_NUMBER` is still empty, ask the user and stop. ### 0c. Fetch PR metadata ```bash gh pr view "$PR_NUMBER" --json \ headRefOid,changedFiles,additions,deletions,mergeStateStatus,mergeable,baseRefName,state \ > /tmp/ghcp-pr-meta.json ``` Extract into local variables: - `PR_HEAD_SHA` — head SHA (later mutations re-check this to detect mid-run pushes) - `CHANGED_FILES` — file count - `LINES_CHANGED` = additions + deletions - `MERGE_STATE_STATUS` — `CLEAN`, `DIRTY`, `BLOCKED`, `BEHIND`, `UNKNOWN`, etc. - `BASE_REF` — base branch name - `PR_STATE` — `OPEN`, `CLOSED`, or `MERGED` ### 0c.1 Bail on non-OPEN PRs If `PR_STATE` is `CLOSED` or `MERGED`, this PR is no longer a review target — there is nothing to fix and any review comment would land on a frozen artifact. Emit the preflight table with a blocker note (`PR is <state> — no review needed`) and stop. Reviews on `CLOSED`/`MERGED` PRs are out of scope by design. ### 0d. Classify PR size Size thresholds (named so they're easy to tune later): - `SIZE_THRESHOLD_FILES = 20` - `SIZE_THRESHOLD_LINES = 2000` ``` if CHANGED_FILES <= SIZE_THRESHOLD_FILES and LINES_CHANGED <= SIZE_THRESHOLD_LINES: SIZE_CLASS = "small" else: SIZE_CLASS = "large" ``` `small` → adjudicator and reviewer subagents use the full-diff path (`gh pr diff`). `large` → they use the per-file paginated path (`gh api .../pulls/{n}/files --paginate`). This avoids `gh pr diff`'s 20k-line API cap. ### 0e. Check merge state If `MERGE_STATE_STATUS == "UNKNOWN"`, GitHub hasn't finished computing mergeability (common right after a push). Wait up to 30 seconds, re-fetching every 10s, then proceed with whatever state is reported. If the final `MERGE_STATE_STATUS == "DIRTY"` (has conflicts with base), **do NOT stop by default** — run the conflict-resolution subroutine in §0e.1 to bring the PR up to date, then re-fetch metadata and continue preflight. Merge conflicts are work the skill is expected to do, not a reason to bail out. If the resolution subroutine fails after a reasonable effort (see exit criteria below), only then emit the preflight table with a blocker note and stop. **Opt-out:** if the user passed `--no-auto-resolve` (or the skill config sets `auto_resolve_conflicts: false`), skip the subroutine entirely on `DIRTY` and behave the way the skill did before this capability landed: emit the preflight table, log "auto-resolve disabled by flag/config", and stop with the recommended-action block: ``` Blocker: PR has merge conflicts with base (mergeStateStatus=DIRTY); auto-resolve disabled. Recommended next action — resolve conflicts before re-running this skill: Option A (manual): git fetch origin && git rebase origin/<base> # resolve conflicts, then: git push --force-with-lease Option B (delegated): Skill(skill="compound-engineering:ce-work", args="resolve the merge conflicts on PR #<N>") ``` `--no-auto-resolve` is orthogonal to `--force` (the existing escape hatch for `PRIOR_RESOLVED=true`); both can be passed independently and either may be active without affecting the other. #### 0e.1 Conflict-resolution subroutine The goal: produce a clean rebase of the PR branch onto `origin/<BASE_REF>` and force-push it, so the rest of the skill can review against an up-to-date head. The subroutine is **convention-aware** — it studies how this repo has handled similar conflicts before, and follows that style rather than guessing. 1. **Checkout the PR branch locally** (if not already): ```bash gh pr checkout "$PR_NUMBER" git fetch origin "$BASE_REF" ``` If checkout fails because the branch is on a fork the runner can't push to, abort the subroutine — emit a blocker explaining the fork ownership issue. Don't try to resolve conflicts you can't push back. 2. **Study repo conflict-resolution conventions.** Before attempting the rebase, gather context so the resolutions follow repo norms instead of arbitrary picks. Look at the most recent merged PRs that touched the same files as this PR: ```bash # Files this PR changes gh pr view "$PR_NUMBER" --json files -q '.files[].path' > /tmp/ghcp-pr-files.txt # Recently merged PRs (last 30) and their changed files gh pr list --state merged --limit 30 --json number,title,mergedAt,files \ > /tmp/ghcp-recent-merged.json # Recently closed-without-merge PRs (last 10) — often informative about # rejected resolution approaches gh pr list --state closed --limit 10 --json number,title,closedAt,body \ > /tmp/ghcp-recent-closed.json ``` Cross-reference: which of the last 30 merged PRs touched files that overlap with this PR's files? Read the merge commits and any "fixup conflict" / "rebase onto main" commits in those PRs to see how the maintainer resolved similar collisions. Note any pattern (e.g., "always keep the PR side for `AGENTS.md` table rows", "always take main for generated lockfiles"). Also check the PR body and review comments for any explicit instruction the author or reviewer left about the conflict (e.g., "this needs to merge after #251 because both touch X — keep my version"). Surface those in your subroutine plan. 3. **Attempt the rebase**: ```bash git rebase "origin/$BASE_REF" ``` If it succeeds with no conflicts, jump to step 6. 4. **Resolve conflicts file by file.** For each conflicted file: - Read the conflict markers and both sides. - Apply repo conventions from step 2. - Default heuristics when no convention is obvious: - **Lockfiles** (`pnpm-lock.yaml`, `package-lock.json`, `yarn.lock`, `Cargo.lock`, `Gemfile.lock`, `go.sum`, `poetry.lock`): take main's version, then re-run the package manager's install/lock command in the working tree to integrate the PR's dependency changes; commit the regenerated lockfile. - **Auto-generated files** (`schema.rb`, `next-env.d.ts`, anything marked "DO NOT EDIT"): take main's version and let regeneration sort it out. - **Documentation tables / lists** (e.g., `AGENTS.md` gate tables, README service lists): keep both sides' additions where possible — append the PR's new rows after main's rows. - **Code with overlapping additions**: prefer a manual merge that preserves both intents; never silently drop functionality from either side. - After editing, `git add <file>` it. - If a conflict is genuinely ambiguous and no repo convention applies, write a short note describing both options and **stop the subroutine** — surface the ambiguity to the user as a blocker rather than guessing. 5. **Continue the rebase** until clean: ```bash git rebase --continue ``` Repeat step 4 for each commit that conflicts. If the same file conflicts on multiple commits, suspect that the resolution at the earlier commit was wrong — reconsider before continuing. 6. **Verify the rebase locally** before pushing: - Run a fast sanity check that the working tree at least parses/builds. Use whatever the repo's cheapest verification command is (e.g., `pnpm install --frozen-lockfile`, `cargo check`, `go build ./...`, `python -m py_compile $(git diff --name-only origin/$BASE_REF -- '*.py')`). - If verification fails, attempt one repair pass. If that also fails, abort the subroutine — do not push a broken rebase. 7. **Force-push with lease**: ```bash git push --force-with-lease ``` `--force-with-lease` (not `--force`) — refuses if someone else pushed in the meantime, preventing clobbered work. 8. **Re-fetch PR metadata** so later preflight steps see the new head: ```bash gh pr view "$PR_NUMBER" --json \ headRefOid,changedFiles,additions,deletions,mergeStateStatus,mergeable,baseRefName \ > /tmp/ghcp-pr-meta.json ``` Update `PR_HEAD_SHA`, `CHANGED_FILES`, `LINES_CHANGED`, `MERGE_STATE_STATUS`. Recompute `SIZE_CLASS` if it changed. The new `MERGE_STATE_STATUS` should now be `CLEAN`, `BLOCKED`, or `BEHIND` — if it's somehow still `DIRTY`, the subroutine failed; treat as a blocker. **Exit criteria — when to give up and escalate to a blocker:** - A conflict exists in a file the runner can't reasonably resolve without product-level decisions (e.g., two divergent feature implementations of the same function with no clear convention). - Verification (step 6) fails twice in a row. - The PR branch is on a fork the runner can't push to. - The user's repo has a hook or branch protection that rejects the force-push. When any exit criterion fires, abort the rebase (`git rebase --abort`), emit the preflight table with the conflict-resolution subroutine's findings, and stop with a clear note: ``` Blocker: conflict-resolution subroutine could not complete. Reason: <one-line summary> Files attempted: <list> Conventions studied: <one-line summary of step 2 findings> Recommended next action: - Review the PR's conflicting files manually, OR - Rerun: Skill(skill="compound-engineering:ce-work", args="resolve the merge conflicts on PR #<N> using the repo conventions documented in <link>") ``` No inline comments are posted, no review is submitted, no fixes are attempted on the still-conflicted PR. The skill exits cleanly. ### 0f. Detect existing Copilot review (read-only) The skill **never** assigns reviewers. It only reads what's already on the PR. Use GraphQL to fetch reviews and review threads in one query: ```bash gh api graphql -f query=' query($owner:String!, $repo:String!, $number:Int!) { repository(owner:$owner, name:$repo) { pullRequest(number:$number) { reviews(first: 50) { nodes { author { login } state commit { oid } submittedAt body } } reviewThreads(first: 100) { nodes { id isResolved comments(first: 20) { nodes { id author { login } path line body commit { oid } } } } } } } }' -F owner=<owner> -F repo=<repo> -F number=$PR_NUMBER ``` A review is "Copilot-authored" if its author login matches `github-copilot[bot]` or `copilot-pull-request-reviewer[bot]` (case-insensitive). Compute `EXISTING_COPILOT_REVIEW`: - **`fresh`** — Copilot has at least one review whose `commit.oid` equals or is an ancestor of `PR_HEAD_SHA`, AND at least one Copilot-authored thread is unresolved (so there is something to feed the adjudicator). Use this review as Review A in Step 1. - **`stale`** — Copilot reviewed at some point, but `PR_HEAD_SHA` has moved past every Copilot review's commit. Logged in preflight; treated as `none` for sourcing. - **`none`** — no Copilot review on this PR at all. A fully-resolved fresh Copilot review (every Copilot thread `isResolved=true` at current head) does NOT count as `fresh` for sourcing — the work is already done. It feeds the `PRIOR_RESOLVED` check below instead. **REST fallback:** if the GraphQL query fails (auth scope, schema drift, older `gh`), fall back to `/pulls/{n}/reviews` and `/pulls/{n}/comments`, filter by author login the same way, and approximate the same classification. When in doubt, set `EXISTING_COPILOT_REVIEW=none` and proceed — the two-subagent path always works. ### 0g. Check for prior fully-resolved Copilot review (idempotency) Using the threads fetched in 0f, classify each Copilot-authored thread: - **resolved-and-fresh** — `isResolved=true` AND the thread's most recent comment SHA is an ancestor of (or equals) `PR_HEAD_SHA`. - **resolved-but-stale** — `isResolved=true` BUT `PR_HEAD_SHA` is newer than the thread's resolution SHA. - **open** — `isResolved=false`. Derive: ``` PRIOR_RESOLVED = (there is at least one Copilot thread) AND (every Copilot thread is resolved-and-fresh) ``` When `PRIOR_RESOLVED=true`, the skill has nothing useful to do on the Copilot side — and re-running pr review is low-value too. Emit the preflight table with a clear note ("Prior Copilot review found with all threads resolved at current HEAD; skipping run") and stop, unless the user passed `--force`. **Escape hatch:** `--force` (or explicit user instruction to re-run anyway) bypasses this short-circuit and proceeds with the normal two-source flow in Step 1, ignoring the existing Copilot review for sourcing purposes. ### 0g.1 Build the regression watch list Resolved Copilot findings are signal, not noise. They are issues Copilot once flagged that an author claimed to fix — and over many commits a "fix" can be reverted, refactored away, or recreated as the same bug in a new file. Carry every resolved Copilot finding forward into a structured **regression watch list** that the adjudicator (Step 3) checks against the current diff. From the Copilot threads classified in §0g, build the watch list from threads that are **resolved-and-fresh** OR **resolved-but-stale** (both buckets represent "Copilot caught it, author claimed to fix it"). Skip open threads (those feed Review C in §1b) and dismissed threads (author-rejected; carrying them forward re-litigates that judgment). For each qualifying thread, capture: ``` { "watch_id": "<thread id>", "file": "<path>", "line": <int|null>, "body": "<verbatim original Copilot comment>", "resolution_sha": "<sha at which the thread was resolved>", "comment_ids": [<original GitHub comment IDs>], "comment_url": "<URL of the original Copilot comment for human reference>" } ``` Record `REGRESSION_WATCH_COUNT = len(watch list)` and surface it in the preflight table so the user sees how much carry-forward signal will be checked. The watch list is bounded (one entry per resolved Copilot thread on this PR), the lookups are cheap, and the cost scales with the actual review history of the PR. **Cross-PR carry-forward is out of scope.** This watch list is per-PR only; it does not consult resolved findings from other PRs. **Rebase implication:** if the conflict-resolution subroutine in §0e.1 just rewrote the head SHA, original `comment_ids` and `resolution_sha` values still resolve correctly via the GitHub API (they're immutable), but file/line coordinates may now refer to lines that no longer exist. The adjudicator's regression-check pass (§3) is responsible for diffing against current HEAD; stale coordinates are filtered there, not here. ### 0j. Preflight CI / GitHub Actions status check Read the current GitHub Actions status for `PR_HEAD_SHA` and emit a `CI_STATUS` flag that downstream verdict logic can gate on. This step happens **after** the regression watch list (§0g.1) is built and **before** the preflight table (§0h) is emitted. The actual classifier is a pure function over `gh run list` JSON; reference implementation ships alongside this skill at `lib/ci-classifier.js` with unit tests at `tests/ci-classifier.test.js`. ```bash # Fetch all workflow runs for this branch, filter to PR_HEAD_SHA in classifier. gh run list \ --branch "$BRANCH" \ --json status,conclusion,name,databaseId,headSha,event \ --limit 50 \ > /tmp/ghcp-pr-runs.json ``` Pass the JSON plus `PR_HEAD_SHA` to the classifier. The classifier filters stale runs (any `headSha != PR_HEAD_SHA` is dropped — they belong to older pushes) and produces: