From a1035b4dde01cc49512822b5aeb25e727ca0516e Mon Sep 17 00:00:00 2001 From: Johan Zander Date: Sat, 15 Aug 2026 15:10:00 +0200 Subject: [PATCH 1/2] chore: mark an approved PR ready for review in implement-issue Step 11 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The skill drove the PR all the way to a green, bot-approved state and then left it a draft, which forced the maintainer to re-derive "is this actually finished?" by hand before merging — three manual steps to restore a fact the review loop had already established. Step 11's APPROVED branch now runs `gh pr ready`, guarded on re-checking CI and mergeability when commits landed after the approving review. The merge constraint is untouched and now stated on its own: no agent ever merges, and nothing flips out of draft before an approval. CI mode keeps opening drafts and leaving them, since Step 11 never runs there. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013Jzvtwrzajuy8jKFKVj2rE --- .claude/skills/implement-issue/SKILL.md | 50 +++++++++++++++++++++---- CLAUDE.md | 5 ++- 2 files changed, 46 insertions(+), 9 deletions(-) diff --git a/.claude/skills/implement-issue/SKILL.md b/.claude/skills/implement-issue/SKILL.md index 27f199a8..2d6dfd80 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,31 @@ 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 (parking a nit in `TODO.md`, a + `git merge origin/main`), re-check `gh pr checks` and + `mergeable`/`mergeStateStatus` before flipping, and say in your report + which commits landed after the approving review and whether they touched + the reviewed code. `gh pr ready` on a PR whose approved diff has since + changed underneath it is the one way this step can mislead. - **`CHANGES_REQUESTED` / `COMMENTED`** — collect the findings: ```bash @@ -511,7 +536,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 +559,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 +608,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 +643,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 +669,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..4b452f02 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -129,7 +129,10 @@ 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 stay + that way until an independent review approves them — `implement-issue` + Step 11 then marks the PR ready (`gh pr ready`) so the only thing left is + the maintainer's merge. No agent ever merges. - 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. From 92e51364532e0b3963453fa33150e298e10cbd48 Mon Sep 17 00:00:00 2001 From: Johan Zander Date: Sat, 15 Aug 2026 15:19:31 +0200 Subject: [PATCH 2/2] =?UTF-8?q?chore:=20address=20review=20=E2=80=94=20sco?= =?UTF-8?q?pe=20the=20ready=20rule,=20harden=20the=20stale-approval=20guar?= =?UTF-8?q?d?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from the Stage 4 review, two fixed and one answered. 1. The CLAUDE.md bullet stated ready-on-approval as a blanket rule, but it sits under "General bot rules", which governs the Stage 1-5 pipeline — and this same PR says CI mode skips Step 11. A Stage 3 PR approved by a manual Stage 4 review therefore had nothing that would ever flip it. Now split: interactive runs flip their own, CI-opened PRs stay draft for the owner, who is triggering that review by hand anyway. 2. The post-approval guard named the stale-approved-diff risk and then flipped the PR regardless, disclosing it in the report. Disclosure is not a mitigation. It now branches on whether the new commits touched reviewed code: outside the reviewed diff, flip and justify; inside it, another review round, because the approval being held is for different code. 3. gh pr ready is deliberately left unattended, and CLAUDE.md's permissions section now says so rather than leaving it to be read as an oversight. The documented standard has two halves — escapes the repo AND git cannot undo it — and this clears the second: --undo reverts it, no content changes, and the push that made the diff public already prompted. Gating it would stall the one flow whose point is reaching that state unattended. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_013Jzvtwrzajuy8jKFKVj2rE --- .claude/skills/implement-issue/SKILL.md | 25 +++++++++++++++++++------ CLAUDE.md | 23 ++++++++++++++++++----- 2 files changed, 37 insertions(+), 11 deletions(-) diff --git a/.claude/skills/implement-issue/SKILL.md b/.claude/skills/implement-issue/SKILL.md index 2d6dfd80..3870abe4 100644 --- a/.claude/skills/implement-issue/SKILL.md +++ b/.claude/skills/implement-issue/SKILL.md @@ -500,12 +500,25 @@ On the verdict: 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 (parking a nit in `TODO.md`, a - `git merge origin/main`), re-check `gh pr checks` and - `mergeable`/`mergeStateStatus` before flipping, and say in your report - which commits landed after the approving review and whether they touched - the reviewed code. `gh pr ready` on a PR whose approved diff has since - changed underneath it is the one way this step can mislead. + **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 diff --git a/CLAUDE.md b/CLAUDE.md index 4b452f02..e7848d33 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -129,10 +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, and stay - that way until an independent review approves them — `implement-issue` - Step 11 then marks the PR ready (`gh pr ready`) so the only thing left is - the maintainer's merge. No agent ever merges. +- 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. @@ -260,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.