chore: mark an approved PR ready for review in implement-issue Step 11

.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 <n>
+  ```
+
+  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 |

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.