Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 77 additions & 4 deletions .claude/skills/implement-issue/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ runners — only repo-level `.claude/skills/` and `.claude/agents/` exist there.
| 7. Confirm gate 2 | Replaced by the draft PR itself — the owner reviews the draft before anything merges. |
| 8. Local run & observe | Structurally unavailable in CI — this is the documented reason the local flow exists. Skip, and say so in the PR body's test plan so the reviewer knows verification is still owed. |
| 9. Commit + draft PR | Applies verbatim, including the `CHANGELOG.md` `## [Unreleased]` entry and the documentation check. Add the `## Scope assessment` section (Step 3 above). The workflow file owns CI-only mechanics: issue comment with the PR link, `has-fix-pr` label. |
| 10. Hard constraints | Apply verbatim. |
| 10. Watch this PR to green | Applies verbatim — `gh pr checks --watch` on the PR just opened, fix failures, never widen to other PRs. |
| 11. Hard constraints | Apply verbatim. |

## Process

Expand Down Expand Up @@ -337,6 +338,20 @@ to guess whether it was checked.
Commit per `docs/agents/workflow.md` format (subject + blank line + body
explaining WHY).

**Then bring the branch up to date before pushing — not after.**

```bash
git fetch origin && git merge origin/main
```

Resolve any conflicts here, in the worktree, where you have the context; if
the merge brought changes in, re-run `./scripts/quality-check.sh` before
pushing. Step 4 cut this branch from a current `origin/main`, but Steps 5–8
take hours (slow suite, `verify`) and other PRs merge during them. Opening a
PR that is already `CONFLICTING` is worse than it sounds: GitHub creates **no
workflow run at all** for it, so the PR shows no checks rather than a
conflict, and the first reader concludes CI dropped the event.

**Frontend diffs only:** before pushing, show the user the diff and the
Step 8 verification output (screenshot/dev-server observation) and wait for
explicit go-ahead. This is the one point in the fully-automatic flow where a
Expand Down Expand Up @@ -389,7 +404,55 @@ If you cannot produce a mutation that reddens your test, you have not
demonstrated the bug — say so in the PR and stop, rather than filling the
section in with the suite result.

### 10. Hard constraints
### 10. Watch this PR to green (and only this PR)

The draft PR is open, but CI has not run yet. Local `quality-check.sh` and
the slow suite are not the same as the CI matrix, and a PR left red or
`CONFLICTING` is a PR the user cannot review.

```bash
gh pr checks <n> --watch --fail-fast # blocks until the run settles
gh pr view <n> --json mergeable,mergeStateStatus
```

**`no checks reported on the '<branch>' branch` is not a result.** It has
two entirely different causes and you must tell them apart before doing
anything else:

```bash
gh pr view <n> --json mergeable,mergeStateStatus # CONFLICTING -> merge origin/main
gh run list --branch <branch> --limit 3 # in_progress -> --watch just raced it
gh run watch <run-id> --exit-status # then wait on the run directly
```

If `mergeable` is `CONFLICTING`, there is genuinely no run and never will
be — GitHub does not build a conflicted PR. If a run is `in_progress`,
`--watch` simply returned before the run was registered (observed on this
skill's own PR, ~8s after the push) and you wait on the run id instead.
Reading "no checks" as green is how a red PR gets handed over as finished.

Then, on this PR only:

- **Checks fail:** read `gh run view --log-failed`, fix in the worktree,
re-run `./scripts/quality-check.sh`, push. This is your diff, so a real
test failure is yours to fix — not merely to report.
- **Went `CONFLICTING`** (another PR merged in the minutes since Step 9):
`git merge origin/main`, resolve, `quality-check.sh`, push.
- **Green and mergeable:** report the PR link and stop. Do not merge, do not
take it out of draft — Step 11 still holds.

**Scope: this issue's PR, nothing else.** If the sweep in Step 4 or your own
`gh pr list` shows other PRs red or conflicted, that is not this session's
job — hand it to the `sweep-prs` skill, which owns fleet-wide maintenance
and has the ownership skip gate needed to touch a worktree another agent may
be sitting in. Widening a single-issue session into fleet cleanup is how two
sessions end up pushing to the same branch.

`gh pr checks --watch` blocks rather than polls, so this costs one wait, not
a re-read of the whole session context every 60s. If CI is badly backed up,
say so and leave the PR — don't hold the session open indefinitely.

### 11. Hard constraints

- Draft PR only. Never auto-merge.
- Never push directly to `main`.
Expand All @@ -409,7 +472,7 @@ section in with the suite result.

A **separate, later invocation** — often a different session, sometimes days
later once CI is green and the user has reviewed. Not part of the numbered
flow above, which stops at draft-PR-open per the Step 10 constraints.
flow above, which stops at draft-PR-green per the Step 11 constraints.

**Treat this as best-effort, not the cleanup mechanism.** Because it depends
on someone returning after the merge, it reliably does not happen; Step 4's
Expand Down Expand Up @@ -454,6 +517,9 @@ net is upstream, not this section.
| "I'll clean up this other thing while I'm in here" | Out of scope. Minimal fix only. |
| "code review can wait until after I've verified it works" | Reordered on purpose — catch cheap issues before spending time on manual verification, not after. |
| "there's already a bot diagnosis, let me re-derive it anyway to be safe" | Re-verify the cited evidence; don't redo the whole investigation. |
| "the branch was current when I cut it, no need to merge before pushing" | Steps 5–8 take hours and other PRs merge during them. And a CONFLICTING PR gets no workflow run at all, so it reads as "CI never fired" — the conflict stays invisible until someone digs. |
| "while I'm watching my PR I may as well fix the other red ones" | That's `sweep-prs`, which has the ownership skip gate this skill doesn't. Another agent may be sitting in that worktree; merging under it puts two sessions on one branch. |
| "the PR is open, my job is done" | Open isn't green. CI runs a matrix `quality-check.sh` doesn't, and the user can't review a red or conflicted PR. Step 10 finishes the job. |
| "the plan doc is useful context, keep it in the PR" | Once code and tests exist, the plan only drifts — it's not the source of truth. Delete it before Step 9; keep the spec if one exists. |
| "the user is in a hurry, just open the PR" | Time pressure from the user is not permission to skip Step 8 — it's the reason to say so explicitly and give a real ETA instead. |
| "I'll clean up the worktree after it merges" | You won't — that's the postcondition that already failed 24 times. Prune at Step 4, before creating the next one. |
Expand Down Expand Up @@ -482,6 +548,12 @@ net is upstream, not this section.
in `docs/agents/bess-knowledge.md` or `docs/SOFTWARE_DESIGN.md`.
- About to write only a synthetic-input unit test for a DP/intent/control-
mapping change instead of a plan-faithfulness (`R == P`) scenario test.
- About to push the branch without having merged `origin/main` since Step 4.
- About to stop at "draft PR opened" without watching CI settle (Step 10).
- About to read `no checks reported` as green. It means either a conflict
or a run that hasn't registered yet — distinguish before reporting.
- About to touch another PR or another worktree from inside this session —
that is `sweep-prs`, not this skill.
- About to write a repro test from hand-built data when a user debug log/
bundle is available and `from_debug_log.py` could build it from real data.

Expand All @@ -497,4 +569,5 @@ net is upstream, not this section.
| 6. Quality gate + code review | `quality-check.sh` + slow suite + `code-review` (background agent) | No |
| 7. Confirm gate 2 | — | Conditional (auto-continue if backend-only + clean; otherwise No) |
| 8. Local run & observe | `verify` | **Never** |
| 9. Commit + PR | `finishing-a-development-branch` | No |
| 9. Commit + PR | `finishing-a-development-branch` (incl. pre-push `git merge origin/main`) | No |
| 10. Watch this PR to green | `gh pr checks --watch` — this PR only | No |
195 changes: 195 additions & 0 deletions .claude/skills/sweep-prs/SKILL.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,195 @@
---
name: sweep-prs
description: Use when asked to check, refresh, or keep the open bess-manager PRs green and mergeable across the whole fleet — pruning merged worktrees, merging main into stale branches, and reporting red CI. Reports only by default; pass PR numbers to act on those, or --all (pair with /loop) for unattended maintenance.
---

# Sweep PRs

## Overview

Walk every git worktree in the repo and bring the fleet back to a known
state: prune the ones whose PR has merged, refresh the ones whose PR has
gone stale, report everything else.

This exists because a PR rots without anyone touching it. A branch that was
green and mergeable when its session ended goes `CONFLICTING` the moment two
or three others merge into `main` ahead of it — and a `CONFLICTING` PR
**creates no workflow run at all**, so it presents as "CI never fired", not
as a conflict. Nobody investigates a PR that appears to have no checks. The
first dry run of this sweep found two such PRs (#437, #579) sitting
conflicted with no one aware.

## When to Use

- Asked to check whether the open PRs are green/mergeable, or to fix up the
ones that aren't.
- Under `/loop /sweep-prs --all` to hold the fleet green between issues.
Self-paced is right: a fully-green fleet costs a noop tick. `--all` is
required here — a bare `/sweep-prs` on a loop just re-reports the same
fleet forever without fixing anything.
- **Not** from inside an `implement-issue` session. That skill owns exactly
one PR — its own — and deliberately does not widen into fleet cleanup
(see its Step 10). Run this separately.

## Modes

Classification always covers every worktree in `git worktree list`. What
differs is **what gets acted on**, and that is chosen by the invocation:

| Invocation | Behaviour |
|---|---|
| `/sweep-prs` | **Report only — the default.** Classify the whole fleet, print what each PR *would* get, change nothing. No merges, no pushes. |
| `/sweep-prs 437 579` | Act, restricted to the listed PR numbers. Everything else is classified and reported, untouched. This is the normal way to use it: read the report, pick. |
| `/sweep-prs --all` | Act on every PR that survives the skip gate. Pair this one with `/loop` for unattended maintenance. |

**Report-only is the default on purpose.** Merging and pushing to a PR the
user has not looked at is not something to do because they forgot to pass a
flag. The safe mode is the one you get when you don't think about it; acting
is the one you have to ask for.

**Pruning merged worktrees runs in every mode, including report-only.** It
is not gated, because it deletes only work that is already merged into
`main` — nothing is at risk — and because gating it recreates exactly the
failure it was introduced to fix: a cleanup that depends on someone choosing
to run it doesn't run. That is how 39 worktrees accumulated on disk, 24 of
them long merged. The uncommitted-changes and live-session guards still
apply to it.

Whatever the mode, a branch is only ever *acted on* when it survives the
skip gate below; everything else is reported with its reason.

## Process

### 1. Skip gate (before anything else)

Another agent may be mid-`implement-issue` in any of these worktrees.
Merging `origin/main` underneath it moves its HEAD mid-run and puts two
sessions on the same branch pushing to the same PR. Leave a worktree alone
when any of these hold:

| Condition | Why |
|---|---|
| A live Claude session has its `cwd` at or under the worktree | Owned. **Any status counts — `idle` and `blocked` included.** An agent parked between Step 6 and Step 8 reads as idle and still holds that branch. |
| `git status --porcelain -uno` is non-empty | Uncommitted tracked work. One such worktree held a 375-line module that existed nowhere else. |
| HEAD commit is under 30 minutes old | Likely active even if its session already exited. |
| Detached or locked | Almost always another agent's live session. |

Read ownership with `claude agents --json` run **unscoped** (from `~`) —
per `CLAUDE.md`'s Worktree Conventions, sibling worktrees never appear in
the project-scoped view, and missing one is exactly the collision this gate
exists to prevent.

### 2. Classify

```bash
git fetch origin --prune
# NOTE: `git branch --merged` / `rev-list origin/main..branch` DO NOT WORK
# here -- this repo squash-merges, so a merged branch's commits are never
# reachable from main and every worktree looks unmerged forever. PR state is
# the only authoritative signal.
merged=$(gh pr list --state merged --limit 200 --json headRefName -q '.[].headRefName')
owned=$(cd ~ && claude agents --json 2>/dev/null | jq -r '.[].cwd')

git worktree list | awk 'NR>1 {print $1}' | while read -r wt; do
b=$(git -C "$wt" branch --show-current 2>/dev/null)
[ -n "$b" ] || { echo "SKIP (detached): $wt"; continue; }
if echo "$owned" | grep -qF "$wt"; then
echo "SKIP (live session): $b"; continue
fi
if [ -n "$(git -C "$wt" status --porcelain -uno)" ]; then
echo "SKIP (uncommitted changes): $b"; continue
fi
if echo "$merged" | grep -qx "$b"; then
echo "PRUNE: $b"; continue
fi
age=$(( ($(date +%s) - $(git -C "$wt" log -1 --format=%ct)) / 60 ))
if [ "$age" -lt 30 ]; then
echo "SKIP (HEAD ${age}m old, likely active): $b"; continue
fi
# GitHub computes `mergeable` LAZILY: the first query on a cold PR returns
# UNKNOWN *and* triggers the computation, so a single pass reports UNKNOWN
# for every stale PR -- i.e. exactly the ones this sweep exists to find.
# Ask again until it resolves. Verified: all three resolved on retry.
for _ in 1 2 3; do
pr=$(gh pr view "$b" --json number,mergeable,mergeStateStatus,statusCheckRollup \
-q '"#\(.number) \(.mergeable) \(.mergeStateStatus) " +
([.statusCheckRollup[]?|select(.conclusion=="FAILURE").name]|join(","))' \
2>/dev/null)
[ -z "$pr" ] && { echo "NO PR (never pushed): $b"; break; }
case "$pr" in *UNKNOWN*) sleep 3; continue;; esac
echo "OPEN $pr <- $wt"; break
done
done
```

### 3. Act

**`PRUNE`** — the PR merged:

```bash
git worktree remove "$wt" && git branch -D "$b"
```

Force-delete is expected, not a warning sign: squash-merge means the
branch's commits never become reachable from `main`, so `git branch -d`'s
ancestry check always refuses.

**`OPEN`** — in report-only mode (the default), print the row from the table
below that this PR matches and take no action. In `--all` mode, or when the
PR number was listed explicitly, act on it. This is judgment, not script:

| State | Action |
|---|---|
| `CONFLICTING`, or `mergeStateStatus: BEHIND` | In that worktree: `git merge origin/main`. Auto-resolve **mechanical** conflicts only — `CHANGELOG.md` under `## [Unreleased]` (keep both bullets), import ordering, lockfiles. Then `./scripts/quality-check.sh`, then push. |
| Conflict is semantic | `git merge --abort` and report it. Do not guess at someone else's fix. |
| Checks `FAILURE` | Read the failing job log (`gh run view --log-failed`). Auto-fix only Black/Ruff formatting and a missing `## [Unreleased]` changelog entry — the mechanical CI failures. A real test failure belongs to whoever wrote the diff; report it, never patch it blind. |
| Green and mergeable | Nothing. |
| `NO PR (never pushed)` | Report only, never delete. A branch with local commits and no PR is abandoned work or someone's parked experiment; the first dry run found 11. The sweep is not what decides their fate. |

### 4. Report

In report-only mode, end with the invocation that would act on what you
found — `/sweep-prs 437 579` — so choosing is one paste, not a re-read.

Print what was pruned, what was refreshed and pushed (or *would* be), what
failed, and **what was skipped with the reason** — "3 skipped: #589 owned by live
session `bess-manager-34`" is the useful output. A sweep that silently does
nothing is indistinguishable from a broken one.

## Hard constraints

- Never push without `./scripts/quality-check.sh` green.
- Never `git stash` — denied repo-wide, one shared stack (`CLAUDE.md`).
- Never `--force` / `--force-with-lease` on someone else's branch.
- Never merge a PR, take it out of draft, or close it.
- One commit per PR per sweep: `chore: merge main into <branch>`.
- Never resolve a semantic conflict or fix a real test failure on a PR that
isn't yours to diagnose. Report and move on.
- **Run one sweep at a time.** Two collide with each other for exactly the
reason the skip gate exists, and nothing enforces it.
- Never act on a PR the invocation didn't select. A bare `/sweep-prs`
reports; it does not merge or push.

## Rationalizations — Reality

| Excuse | Reality |
|---|---|
| "that worktree's session is idle, so nobody's using it" | Idle and blocked both mean owned. An agent parked between Step 6 and Step 8 is idle and still holds that branch. |
| "the PR shows no CI checks, so the workflow must have failed to trigger" | Two causes, never a dropped event. A CONFLICTING PR creates no run at all; a just-pushed PR has a run that hasn't registered yet. Check `mergeable`, then `gh run list --branch`. |
| "`mergeable` came back UNKNOWN, so gh can't tell" | It's computed lazily — the first query only triggers the computation. Ask again. A single pass reports UNKNOWN for precisely the stale PRs you're looking for. |
| "`git branch --merged` will tell me what's safe to delete" | Not in this repo. Squash-merge means a merged branch is never an ancestor of main, so that check reports everything as unmerged and cleanup never fires. Use `gh pr list --state merged`. |
| "they asked me to sweep, obviously they want it fixed" | A bare `/sweep-prs` reports. Merging and pushing to a PR the user hasn't looked at is not something to infer from a missing flag — hand back the report and the `/sweep-prs <numbers>` line. |
| "they named #437, and #579 has the same problem, I'll do both" | They picked one. Selecting PRs is the whole point of the mode; extending the selection silently makes it meaningless. |
| "this conflict is small, I can see what they meant" | Mechanical means CHANGELOG/imports/lockfiles. Anything touching logic is a guess at someone else's fix — abort and report. |
| "I'm already in an implement-issue session, I'll sweep while I'm here" | That session owns one PR and lacks the skip gate. Widening it is how two sessions end up pushing to one branch. |

## Red Flags — Stop and Go Back

- About to act on a worktree with a live session, uncommitted tracked
changes, or a HEAD under 30 minutes old.
- About to treat `mergeable: UNKNOWN` as a final answer.
- About to push a merge without `quality-check.sh` green.
- About to fix a real test failure on a PR you didn't write.
- About to run this from inside an `implement-issue` session.
- About to merge or push on a bare `/sweep-prs` — that mode reports only.
- About to act on a PR the user didn't list.
Loading