diff --git a/.claude/skills/implement-issue/SKILL.md b/.claude/skills/implement-issue/SKILL.md index 27f0072e..eeed3532 100644 --- a/.claude/skills/implement-issue/SKILL.md +++ b/.claude/skills/implement-issue/SKILL.md @@ -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 @@ -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 @@ -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 --watch --fail-fast # blocks until the run settles +gh pr view --json mergeable,mergeStateStatus +``` + +**`no checks reported on the '' 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 --json mergeable,mergeStateStatus # CONFLICTING -> merge origin/main +gh run list --branch --limit 3 # in_progress -> --watch just raced it +gh run watch --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`. @@ -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 @@ -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. | @@ -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. @@ -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 | diff --git a/.claude/skills/sweep-prs/SKILL.md b/.claude/skills/sweep-prs/SKILL.md new file mode 100644 index 00000000..769e198b --- /dev/null +++ b/.claude/skills/sweep-prs/SKILL.md @@ -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 `. +- 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 ` 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.