diff --git a/.claude/skills/implement-issue/SKILL.md b/.claude/skills/implement-issue/SKILL.md index 27f199a8..3870abe4 100644 --- a/.claude/skills/implement-issue/SKILL.md +++ b/.claude/skills/implement-issue/SKILL.md @@ -48,7 +48,7 @@ runners — only repo-level `.claude/skills/` and `.claude/agents/` exist there. | 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. Watch this PR to green | Applies verbatim — `gh pr checks --watch` on the PR just opened, fix failures, never widen to other PRs. | -| 11. Independent review loop | Skip — CI opens the PR as a draft and the owner triggers Stage 4 by hand after reading it. A CI run that requested its own review would be the fix bot grading itself on a PR nobody has looked at yet. | +| 11. Independent review loop | Skip — CI opens the PR as a draft and the owner triggers Stage 4 by hand after reading it. A CI run that requested its own review would be the fix bot grading itself on a PR nobody has looked at yet. Since the loop never runs here, the PR also stays a draft: `gh pr ready` is Step 11's, and there is no approval in CI mode to earn it. | | 12. Hard constraints | Apply verbatim. | ## Process @@ -440,8 +440,9 @@ Then, on this PR only: - **Went `CONFLICTING`** (another PR merged in the minutes since Step 9): `git merge origin/main`, resolve, `quality-check.sh`, push. - **Green and mergeable:** continue to Step 11's review loop — a green PR is - the precondition for asking the bot to review it, not the finish line. Do - not merge, do not take it out of draft — Step 12 still holds. + the precondition for asking the bot to review it, not the finish line. It + stays a draft *here*, because nothing has reviewed it yet; Step 11 is what + marks it ready, and never merge — Step 12 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 @@ -480,7 +481,44 @@ when it exits. This is a hard session boundary, same as Step 6. On the verdict: -- **`APPROVED`** — the loop is done. Report the PR link and stop. +- **`APPROVED`** — the loop is done. **Mark the PR ready for review, then + report the link and stop:** + + ```bash + gh pr ready + ``` + + This is the one status change the skill makes on its own, and the approval + is what earns it. A draft says "not finished"; once the bot has approved a + green PR, that is no longer true, and leaving it draft means the maintainer + has to notice it, judge whether it is actually done, and flip it by hand + before merging — three steps to re-derive something the loop already + established. Ready says "reviewed, green, and waiting on your judgement", + which is exactly the state it is in. + + Two things this does *not* license, both still hard constraints (Step 12): + never merge it, and never flip it early — an unreviewed or red PR stays a + draft, no matter how confident you are in the diff. + + **If you push anything after the approval, the approval covers a diff that + no longer exists, and what you do next depends on what you pushed.** Ask + whether the new commits touched code the reviewer actually reviewed: + + - **They didn't** (parking a nit in `TODO.md`, a `CHANGELOG.md` line, a + clean `git merge origin/main` that changed nothing under review): + re-check `gh pr checks` and `mergeable`/`mergeStateStatus`, then flip. + Name those commits in your report and say why they fall outside the + reviewed diff, so the maintainer can check that call rather than take + it on trust. + - **They did:** do NOT flip. Go back and run another review round — the + approval you are holding was for different code, and `gh pr ready` on a + stale-approved diff is the one way this step can actively mislead. A + round costs minutes; a wrongly-ready PR spends the maintainer's trust in + the ready flag, which is the only thing that makes it worth setting. + + The round cap counts this round like any other. If it lands you on the cap, + say so and hand over a draft PR with the outstanding state — an honest + draft beats a ready flag that means less than it claims. - **`CHANGES_REQUESTED` / `COMMENTED`** — collect the findings: ```bash @@ -511,7 +549,12 @@ not about a bug, and another round will not settle it. ### 12. Hard constraints -- Draft PR only. Never auto-merge. +- **Never merge, ever** — not after a green CI run, not after an `APPROVED` + review, not when the diff is trivial. The merge is the maintainer's final + judgement and it is the one thing this skill never takes. Marking the PR + ready once Step 11 approves it (and only then) is not merging, and is + required rather than forbidden. +- Open the PR as a draft and leave it that way until Step 11's approval. - Never push directly to `main`. - Do NOT modify the version in `bess_manager/config.yaml` — bumping it is a release-time step, not a per-PR one. DO add a `CHANGELOG.md` entry under @@ -529,8 +572,8 @@ not about a bug, and another round will not settle it. 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 a green, bot-reviewed draft PR per the Step 12 -constraints. +flow above, which stops at a green, bot-approved, ready-for-review PR that +the maintainer has not merged yet, per the Step 12 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 @@ -578,6 +621,7 @@ net is upstream, not this section. | "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. | +| "it's approved, but flipping it out of draft is the maintainer's call" | Merging is their call; marking it ready is just reporting the state the loop already established. Leaving it draft makes them re-derive "is this finished?" by hand. | | "Step 6's code review already covered this, skip Step 11" | Step 6 is you reviewing your own diff with the reasoning that produced it. The Stage 4 bot reads the diff cold against the checklist, and in practice takes two to four rounds to run out of real findings. | | "the reviewer asked for it, so change it" | The reviewer has the diff, not the diagnosis. A finding that contradicts a deliberate Step 3 scope decision gets a reply explaining why, not a commit. | | "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. | @@ -612,6 +656,9 @@ net is upstream, not this section. - About to stop at "draft PR opened" without watching CI settle (Step 10). - About to stop at "CI is green" without running the Step 11 review loop. Your own Step 6 review is not the independent one. +- About to hand over an `APPROVED`, green PR still marked draft — Step 11 + flips it with `gh pr ready`; the maintainer should only have to merge. +- About to run `gh pr ready` before an approval, or `gh pr merge` at all. - About to implement a review finding because the bot said so, without checking it against the Step 2 diagnosis and the Step 3 scope assessment. - About to read `no checks reported` as green. It means either a conflict @@ -635,4 +682,4 @@ net is upstream, not this section. | 8. Local run & observe | `verify` | **Never** | | 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 | -| 11. Independent review loop | `scripts/request-pr-review.sh` (background) + `receiving-code-review`, max 3 rounds | No | +| 11. Independent review loop | `scripts/request-pr-review.sh` (background) + `receiving-code-review`, max 3 rounds; `gh pr ready` on `APPROVED` | No | diff --git a/CLAUDE.md b/CLAUDE.md index e409a0a1..e7848d33 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -129,7 +129,15 @@ of `analyzed`. Stages 1–3 and 5 stay owner-only — those spend money on work nobody has asked for yet. - Always use `gh` CLI for all GitHub operations (issues, PRs, labels). -- Never push directly to `main`. PRs are always opened as drafts. +- Never push directly to `main`. PRs are always *opened* as drafts, and no + agent ever merges one — the merge is the maintainer's, always. +- **Who takes a PR out of draft depends on which flow opened it.** An + interactive `implement-issue` run drives its own review loop, so its + Step 11 marks the PR ready (`gh pr ready`) the moment Stage 4 returns + `APPROVED`, leaving only the merge. A **Stage 3 (`issue-fix.yml`) PR stays + a draft even after Stage 4 approves it** — CI mode skips Step 11, so + nothing there runs `gh pr ready`, and you are triggering that review by + hand anyway. Flip it yourself when you're satisfied. - The bot identity is `bess-manager-claude-bot` (a custom GitHub App). The official Anthropic Claude App is **suspended** to avoid collisions — do not unsuspend it. @@ -257,7 +265,15 @@ matching `ask` with no matching `deny` turns a prohibition into a prompt. The gate checks stash and podman shapes against `deny` **only** for that reason. The standard for adding an entry: **the effect escapes the repo and git cannot -undo it.** Not "the command looks dangerous". The second category exists +undo it.** Not "the command looks dangerous". Both halves have to hold, which +is why **`gh pr ready` is deliberately unattended** even though it plainly +escapes to GitHub: `gh pr ready --undo` puts the PR straight back, it changes +no content (the diff was already public — the `git push` that created it +prompted), and it is the codified endpoint of `implement-issue` Step 11's +review loop. Prompting there would stall the one flow whose entire point is to +reach that state without you. Contrast `gh pr merge` one row up, which is the +same category and *is* gated: nothing undoes a merge to `main`. The +second category exists because leaving `rm` and `reset --hard` unattended is only defensible while the object database and reflog can recover them — a `gc --prune=now` that ran unprompted would remove the ground that argument stands on.