pr-review-merge

Files (1)

• SKILL.md

SKILL.md (first lines)

<!-- floor:cold-verify-completion -->

# PR Review-to-Green + Smart Merge

Source-agnostic. Consumers pass: PR number, base branch, and bot-reviewer/CI rules
from the project's `## Marathon Configuration` (defaults if absent).

## Run-completion is gated elsewhere (floor)

This skill takes one PR to merge — a **process** signal. A green, merged PR is not proof the run's assembled product works, and this skill never certifies run-completion. That is gated by the acceptance-contract scripts the marathon engine invokes, not here: `scripts/contract/start_gate.py` fails closed at run start unless the contract is frozen before decomposition, `scripts/contract/spawn_verifier.py` is the sole custody chokepoint that spawns the cold non-implementing verifier against the assembled product, and `scripts/contract/complete_gate.py` fails closed unless that verifier's completion record validates. Merging here never substitutes for those gates. This note is part of the constitutional floor (`FLOOR.md`); the retro may propose changes but never self-apply them.

## Ready Criteria (ALL must be true)

The PR is merge-ready only when all six are simultaneously true. Re-check from the top after every push — a fix can reopen an earlier criterion.

1. **Branch in sync** — no merge conflicts with base branch
2. **CI passing** — all checks succeed (or skipped)
3. **All inline comments addressed** — see thread resolution rules
4. **No unaddressed conversation comments** — actionable feedback responded to
5. **All review threads resolved** — no unresolved threads remain
6. **Re-reviewing bots have reviewed the head SHA** - every bot the project's Marathon Configuration flags `Re-reviews on push: yes` has completed its pass on the current head SHA. For a bot without `Re-review check name`, at least one of its reviews must carry that head SHA as its `commit_id` (a review of an earlier commit does not count), and that is the limit of this path: any review on the head SHA satisfies it, a placeholder or rate-limited one included, so a bot whose pass must be proven belongs on `Re-review check name`. For a bot configured with `Re-review check name`, only the check run of that name on the head SHA decides (its submitted reviews do not: the check run is what turns red when the reviewer exits with an error after posting), and that run has four observable states: **in progress** (or not yet created) - keep waiting until the max wait expires; **completed with conclusion `success`** - the criterion is satisfied for that bot, and the marathon skill's `Commit:`-line spot check still applies before merging on it; **completed with conclusion `skipped`** - the bot is not applicable to this PR (a reviewer job that skips some actors, such as Dependabot, by design), so the criterion is satisfied for that bot with no warning; **completed with any other conclusion** (failure, cancelled, neutral, timed_out) - a settled verdict that the bot did not complete a green pass, so the criterion takes the warning path at once. Any push restarts the wait. The wait is bounded per bot by that bot's `Max wait for re-review`, measured from when the head commit was pushed (the earliest check-suite creation on the head SHA, not the commit's committer date, which predates the push and resets on rebase); when `Re-reviews on push: yes` and `Max wait for re-review` is absent, the bound is 15m. If no check suite exists on the head SHA, fall back to the head commit's committer date as a floor - explicitly not the push time, it only guarantees the bound always expires. When the max wait expires before the bot completes, or a check-name bot's run settles on a conclusion other than `success` or `skipped`, the criterion passes with a warning recorded in the merge record (the PR comment or report that accompanies the merge) naming the bot, the head SHA, and what was observed: for a check-name bot, the check run's conclusion on that SHA (or that no completed run exists) and, so the record never contradicts the reviews endpoint, whether path (a) shows a review of that head SHA anyway; for a review bot, that no review of that SHA was found. The record states that the bot did not complete a green pass, not that it did not review. Merge proceeds with that warning; nothing holds forever on an advisory bot. Bots without `Re-reviews on push: yes` are never waited on, so a project with no such flags sees no change.

**Checking criterion 6** (per flagged bot; `<bot-login>` and the optional `<check-name>` from its `Re-review check name` field in the Marathon Configuration):
```bash
HEAD_SHA=$(gh pr view $PR --json headRefOid | jq -r '.headRefOid')
# Push time of the head commit: GitHub creates check suites on push.
# --paginate emits one JSON page after another; jq -s gathers them so min spans all pages.
PUSHED_AT=$(gh api --paginate "repos/<owner>/<repo>/commits/$HEAD_SHA/check-suites?per_page=100" \
  | jq -r -s '[.[].check_suites[].created_at] | min // empty')
# No check suite on the head SHA (min of an empty list is null): use the committer date as a
# floor so the max wait always expires. This is not the push time; it predates the push.
if [ -z "$PUSHED_AT" ]; then
  PUSHED_AT=$(gh api "repos/<owner>/<repo>/commits/$HEAD_SHA" | jq -r '.commit.committer.date')
fi
# (a) Bot that submits reviews: its reviews whose commit_id is the head SHA.
# Reviews page in ascending order, so the newest head-SHA review sits on the last page:
# --paginate reads every page and jq streams matches from each one.
gh api --paginate "repos/<owner>/<repo>/pulls/$PR/reviews?per_page=100" \
  | jq -r --arg sha "$HEAD_SHA" --arg bot "<bot-login>" '.[] | select(.commit_id == $sha and .user.login == $bot) | .submitted_at'
# (b) Bot hosted as a check run (Re-review check name set): matched by check-run name.
# Prints every run of that name as "<status> <conclusion> <completed_at>", whatever its outcome.
gh api --paginate "repos/<owner>/<repo>/commits/$HEAD_SHA/check-runs?per_page=100" \
  | jq -r --arg n "<check-name>" '.check_runs[] | select(.name == $n) | "\(.status) \(.conclusion // "in_progress") \(.completed_at // "")"'
```
Complete when, for a bot with `Re-review check name` set, path (b) prints a line whose conclusion is `success` or `skipped` (`skipped` means the bot does not apply to this PR, so no warning is recorded; path (a) does not decide for that bot, and its output is recorded only on the warning path); a `completed` line with any other conclusion (failure, cancelled, neutral, timed_out) is the settled warning-path case, which takes the warning path at once; no line, or only lines whose status is not `completed`, means keep waiting until the max wait expires. For a bot without one, complete when path (a) prints at least one line (a head-SHA review), and otherwise keep waiting until the max wait expires. The max wait is `Max wait for re-review` since `PUSHED_AT`, 15m when that field is absent. The filters use `--arg` and pipe to `jq` rather than `gh api --jq`, per the Shell Pitfalls below. Never match check runs by app slug: every GitHub Actions job, the reviewer included, reports as `github-actions`, so an unrelated job completing would pass the criterion. Merge proceeds with the warning on expiry or on a settled run whose conclusion is neither `success` nor `skipped`. A green reviewer check still warrants the spot-check the marathon skill describes (the reviewer's summary cites the head SHA) before merging on the strength of it.

**Thread resolution rules:**
Follow bot reviewer rules from the project's CLAUDE.md Marathon Configuration. Generic defaults:
- **Bot threads**: Fix the code and push. Resolve via GraphQL if addressed. Use jq JSON builder (avoids zsh `$` escaping):
  ```bash
  jq -n --arg tid "$THREAD_ID" '{"query": "mutation { resolveReviewThread(input: {threadId: \"\($tid)\"}) { thread { isResolved } } }"}' | gh api graphql --input -
  ```
- **Human threads**: Fix the code, reply inline explaining the fix, `@mention` the reviewer. Do NOT resolve human threads — let the reviewer confirm.

## Shell Pitfalls

**Never use `gh ... --jq` with complex filters.** Always pipe to `jq` separately.

**Use positive jq filters, not negative.** zsh escapes `!=` to `\!=`, breaking filters silently:
```bash
# WRONG: gh pr view --json reviews --jq '.reviews[] | select(.state != "APPROVED")'
# WRONG: gh pr view --json reviews | jq '.reviews[] | select(.state != "APPROVED")'
# RIGHT:
gh pr view --json reviews | jq '.reviews[] | select(.state == "CHANGES_REQUESTED")'
```

## Review Loop (each iteration)

### Step 1: Sync with base branch (FIRST, every iteration)

```bash
BASE=$(gh repo view --json defaultBranchRef --jq '.defaultBranchRef.name' 2>/dev/null || echo "main")
git fetch origin $BASE && git merge origin/$BASE --no-edit
# If conflicts: resolve them, commit, push
# If can't auto-resolve: report blocked with details
```

**Resolving a `.claude-plugin/plugin.json` conflict (the common one in multi-PR waves).** The conflict is **not always confined to `.version`** — a sibling PR may also have reframed the marketplace `description` or another field. A version-only line edit (`sed`-ing just the version line, or a regex that targets only `.version`) silently keeps the stale side of *every other* field and can leave merge markers behind. Resolve the **whole file** deterministically: take the side carrying the siblings' already-merged field changes (usually base), then overwrite only the version with `jq`:

```bash
git checkout --theirs .claude-plugin/plugin.json   # base side, which has the merged siblings' description/other-field edits
jq --arg v "<your-next-version>" '.version = $v' .claude-plugin/plugin.json > /tmp/pj && mv /tmp/pj .claude-plugin/plugin.json
git add .claude-plugin/plugin.json
```

Always `git diff` the full `plugin.json` before resolving — never assume the only divergence is the version line.

**Conflict resolution patterns:**
- **Import/route files** (e.g., App.tsx, index.ts): Accept BOTH sides — additions are additive
- **Barrel exports** (e.g., shared/index.ts): Accept both sides — each adds its own export
- **Config/manifest files**: Accept both sides unless the same key is modified differently (then ask)

### Step 2: Check all criteria - delegate CI to background agent

```bash
PR=$(gh pr view --json number --jq '.number')
```

**Spawn a background agent to watch CI** (never block on CI yourself):
```
Agent(
  run_in_background: true,
  prompt: """
  Monitor PR #$PR for CI completion and review state.

  1. Block on CI: `gh pr checks $PR --watch --fail-fast`
  2. When CI settles, gather:
     - CI result: `gh pr checks $PR --json name,state,conclusion`
     - Unresolved threads: `gh api graphql ...` (all unresolved with path/line/author/body)
     - Conversation comments: `gh pr view $PR --comments --json comments`
     - Pending checks count
  3. Return structured report.
  """
)
```

**While CI runs (you are FREE)**, do an immediate check for threads and comments:
```bash
# Quick thread check - fix what you can now

From bjcoombs/ai-native-toolkit, Apache-2.0.

More from bjcoombs/ai-native-toolkit