chore: mark an approved PR ready for review in implement-issue Step 11 - #603
Conversation
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013Jzvtwrzajuy8jKFKVj2rE
|
@claude-bot review |
![bess-manager-claude-bot[bot]](https://avatars.githubusercontent.com/in/3024553?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: mark an approved PR ready in implement-issue Step 11
No linked issue — this is a self-identified skill gap (per the PR body), not an issue fix, so the "matches issue root cause" checklist item doesn't apply. It's pure prose (.claude/skills/implement-issue/SKILL.md, CLAUDE.md), no executable code, so the "would the test catch a regression" item is also N/A — agreed with the PR's own "Evidence the test discriminates" section on that.
1. CLAUDE.md and SKILL.md now contradict each other on CI-mode PRs (file:line)
CLAUDE.md:132-134 (new):
Never push directly to
main. PRs are always opened as drafts, and stay that way until an independent review approves them —implement-issueStep 11 then marks the PR ready (gh pr ready) so the only thing left is the maintainer's merge. No agent ever merges.
This is stated as a blanket rule under "General bot rules" — the section governing the GH Actions bot pipeline (Stages 1–5), including Stage 3 (issue-fix.yml, which runs implement-issue in CI mode).
But this same PR's own CI-mode table row says the opposite for that exact case:
SKILL.md:51 (new):
- 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 readyis Step 11's, and there is no approval in CI mode to earn it.
So a Stage-3-opened PR that later gets APPROVED via a manual Stage-4 @claude-bot review (not the Step 11 loop, since that's skipped in CI mode) has no mechanism in either file that ever runs gh pr ready on it — it stays draft forever unless a human does it by hand. That directly contradicts the new CLAUDE.md claim that PRs "stay [draft] until an independent review approves them ... Step 11 then marks the PR ready" — for CI-opened PRs, an approval does not lead to ready via this skill. Either scope the CLAUDE.md bullet to the interactive/local flow explicitly, or say who/what flips a CI-mode PR to ready after Stage 4 approves it.
2. New autonomous gh pr ready <n> isn't guarded in .claude/settings.json (worth resolving explicitly)
SKILL.md:484-488 has Step 11 run gh pr ready <n> unattended on APPROVED, with no confirm gate. CLAUDE.md's own Permissions section (this repo's normative, heavily-worked-out policy) draws the guard line at "escapes to GitHub" / "visible to others" — and lists exactly which gh subcommands get an ask prompt for that reason: gh pr merge, gh release, gh repo edit, gh secret, gh workflow run, and blanket gh api. gh pr ready isn't in that list, isn't in .claude/settings.json's ask/deny, and this PR doesn't add it — so per CLAUDE.md's own description of defaultMode: auto, it "decides without involving you," meaning the skill can flip a PR's visibility/notify-reviewers state with zero human confirmation.
Flipping ready-for-review is exactly the class of action the top-level system guidance calls out as needing confirmation by default ("actions visible to others... creating/closing/commenting on PRs"). This may be a deliberate call (it's not a merge, and it's reversible via gh pr ready --undo), but the PR doesn't discuss it, and it's the kind of gap CLAUDE.md's Permissions section is explicit about needing to be a deliberate decision, not an oversight. Worth either adding Bash(gh pr ready*) to ask, or stating explicitly in this PR why it's fine to leave to the auto classifier.
3. Post-approval-commit guard identifies the risk but doesn't resolve it (minor)
SKILL.md:500-507:
If you push anything after the approval (parking a nit in
TODO.md, agit merge origin/main), re-checkgh pr checksandmergeable/mergeStateStatusbefore flipping, and say in your report which commits landed after the approving review and whether they touched the reviewed code.gh pr readyon a PR whose approved diff has since changed underneath it is the one way this step can mislead.
This names the failure mode correctly (stale-approved diff) but the only mitigation given is CI/mergeability re-checks plus a report — nothing tells the agent not to run gh pr ready when the post-approval commits did touch reviewed code. As written, the step still flips it ready regardless, just with a note in the report. If a post-approval commit touched reviewed code, the safe action is presumably to loop back into Step 11 for a fresh review before running gh pr ready, not just disclose it after the fact.
Fitness of approach
- Scope is minimal and matches the stated intent (two files, both plausibly need updating for this change).
- Internally, most of the diff is consistent (Step 10, Step 12, Rationalizations, Red Flags, Quick Reference all line up with the new Step 11 behavior) — the CLAUDE.md/CI-mode contradiction above is the one place it isn't.
- No
docs/agents/rules.mdarchitecture/testing violations apply (no code).
Given #1 is a direct self-contradiction introduced by this PR's own two edited files, and #2 is a real gap against the repo's own documented permissions philosophy, requesting changes rather than approving as-is.
…al guard 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013Jzvtwrzajuy8jKFKVj2rE
|
Findings 1 and 3 were right and are fixed in 92e5136. 1 — CLAUDE.md / CI-mode contradiction. Confirmed by reading it back: the bullet sits under "General bot rules", which governs Stages 1–5, so it claimed ready-on-approval for exactly the flow this PR says skips Step 11. A Stage 3 PR approved by a manual Stage 4 review had nothing that would ever flip it. Now split in two — interactive runs flip their own; a CI-opened PR stays a draft for the owner, who is triggering that review by hand anyway. 3 — post-approval guard. Agreed, and the sharper way to put it is that disclosure isn't mitigation. The guard now branches on the answer instead of just reporting it: if the post-approval commits fall outside the reviewed diff, flip and justify which commits and why; if they touched reviewed code, don't flip, run another round, because the approval being held is for different code. The round cap counts it, and hitting the cap hands over an honest draft. 2 — The other half is that gating it would defeat the change. This flip is the codified endpoint of Step 11's loop, which exists to reach that state without the maintainer in the loop; a prompt there stalls the one flow whose whole point is not stalling. You were right that it shouldn't be silent, though, so the permissions section now states the call and its reasoning rather than leaving the absence to read as an oversight. Worth adding: the guard from finding 3 has already been exercised for real. This session flipped #601 after parking a |
|
@claude-bot review |
There was a problem hiding this comment.
Review: mark an approved PR ready in implement-issue Step 11 (round 2)
No linked issue (self-identified skill gap per the PR body), and no executable code changes (.claude/skills/implement-issue/SKILL.md, CLAUDE.md only) — the "matches issue root cause" and "test would catch a regression" checklist items are N/A, matching the PR's own "Evidence the test discriminates" section.
All three findings from the prior review round are resolved in 92e5136:
-
CLAUDE.md/SKILL.md contradiction on CI-mode PRs — fixed. The old blanket "Step 11 marks the PR ready" bullet sat under "General bot rules" (which governs Stages 1–5, including CI-mode Stage 3), but SKILL.md's own CI-mode table row skips Step 11 entirely — so an approved CI-opened PR had no mechanism that ever flipped it. CLAUDE.md now explicitly splits this: interactive runs flip their own PR via Step 11; a Stage-3 PR "stays a draft even after Stage 4 approves it... Flip it yourself when you're satisfied." Consistent with SKILL.md's CI-mode row, which now says the same thing from the other side.
-
Post-approval stale-diff guard just disclosed the risk instead of resolving it — fixed. The new branch is correct: commits that "didn't" touch reviewed code (nit in
TODO.md/CHANGELOG.md, a no-opgit merge origin/main) get a mergeability re-check and a named justification before flipping; commits that "did" touch reviewed code block the flip and require another review round instead. That's the right shape — a stale approval no longer gets silently honored. -
gh pr readyleft unattended with no discussion — now explicitly addressed rather than left as an apparent oversight. CLAUDE.md states the reasoning against the file's own stated two-part standard ("escapes the repo and git cannot undo it"):gh pr ready --undoreverses it and it changes no content, so it fails the second half, unlikegh pr mergeone row up. This was one of two options offered in the prior round (add anaskrule, or state explicitly why it's fine to leave to theautoclassifier) — documenting the reasoning is a legitimate resolution, and the reasoning holds up (checked: no workflow in.github/workflows/*.ymlgates on draft vs.ready_for_review, so flipping ready doesn't trigger any new CI side effect in this repo either).
Fitness of approach
- Scope stays minimal — two files, both plausibly needed for this change, no unrelated edits.
- Internally consistent end to end: Step 10, Step 12, the Rationalizations table, and the Red Flags list all line up with the new Step 11 behavior; the round-1 CI-mode contradiction was the one place it wasn't, and that's now fixed.
- No
docs/agents/rules.mdarchitecture/testing violations apply (no code touched).
Approving — no blockers remain.
…ove (#653) * fix: stop the fleet prune from destroying the worktrees it cannot remove `git worktree remove` is sandbox-denied, and unlike `git worktree add` it fails DESTRUCTIVELY. Removal deletes the working tree first and only then unlinks `.git/worktrees/<name>` -- and that unlink is the denied one: error: failed to delete '.../worktrees/backlogger': Operation not permitted error: failed to delete '.git/worktrees/backlogger': Operation not permitted By then ~393 tracked files are gone. It does not roll back. What is left is a carcass: a registered worktree whose `git status` is a few hundred ` D` lines and nothing else. Both prune loops read that as "uncommitted tracked changes" and correctly refuse to auto-delete it -- so the failure makes the worktree permanently unprunable BY ITSELF. Re-running hits the no-`--force` refusal; `--force` re-hits the denial. `git worktree prune` performs the same unlink, so it cannot clear the wreckage either. 13 carcasses accumulated across three sweeps (#568, #596, #597, #600, #601, #603, #609, #612, #617, #629, #633, #634, #641) before anyone read the diff. The last sweep reported them back as "a real backlog of stranded edits worth reviewing" -- they were its own wreckage from the previous runs, and not one byte of real work was in them. Because the filename set is identical in every worktree, so is APFS's readdir order, so every carcass loses the SAME ~393 paths (`core/`, `frontend/`, `bess_manager/`, `pyproject.toml`, ...). Identical damage across many worktrees is the signature, not a coincidence. - Both prune loops now report `PRUNE` and emit one `!`-prefixed command for the maintainer to run unsandboxed, instead of removing anything themselves. - Both classify a dirty set that is entirely ` D` as `CARCASS`, distinct from real edits. The predicate anchors on `^ D ` (unstaged deletions only), so a STAGED deletion still reads as intentional work. - `local-agent-environment.md` gains the `remove`/`prune` half of the `.git/worktrees` denial, next to the `add` half it already documented. `verify-sandbox.sh:88` predicted this in a parenthetical ("a stray directory when `git worktree remove` then fails"); nothing acted on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014FQA38o22RkHZJ4zEHDZS3 * fix: measure the prune half, which exits 0 while failing The previous commit inferred that `git worktree prune` was denied because it performs the same `.git/worktrees/<name>` unlink as `remove`. Measured now, using a carcass as the probe -- `rm -rf` its directory, then prune: $ git worktree prune -v; echo "exit=$?" Removing worktrees/backlogger: gitdir file points to non-existent location error: failed to delete '.../.git/worktrees/backlogger': Operation not permitted exit=0 Denied as expected, but it EXITS 0. `remove` at least exits 255; prune reports success having done nothing, so `git worktree prune && echo done` prints done. That is the nastier of the two and was not in the inferred version. Two further measured facts change the guidance: - The denial is on the `.git/worktrees/<name>` unlink ONLY. `rm -rf` of the working tree from Bash SUCCEEDS. An agent can therefore always destroy the files and never the registration, and half-doing it converts a carcass into a `prunable` phantom -- no better, still needing the same unsandboxed fix. - A phantom breaks both classification loops. Its directory is gone, so `git -C "$wt" branch --show-current` fails, `$b` comes back empty, and the detached-HEAD guard swallows it as "SKIP (detached)". Both loops now test `[ -d "$wt" ]` first and report PHANTOM. Verified over four cases: missing dir with and without a branch name, detached, and on a branch. The emitted cleanup command gains a trailing `git worktree prune` (a phantom has no directory, so `remove` has nothing to work with), uses `[ -n "$b" ]` instead of `&&` since a phantom yields no branch name, and now says it must run from a NON-worktree-isolated session -- an isolated one refuses the `cd` to the shared checkout, which is why the first cleanup attempt silently did nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014FQA38o22RkHZJ4zEHDZS3 * docs: record that `git branch -D` warns on .git/config but strands nothing Third instance of the `.git/config` denial, alongside `checkout -b --track` and `push -u`, and the same exit-0-while-partially-failing shape as the prune bullet: $ git branch -D worktree-backlogger; echo "exit=$?" error: could not lock config file .../.git/config warning: update of config-file failed Deleted branch worktree-backlogger (was 6c70a77). exit=0 The first draft of this bullet asserted the denial leaves a stale `[branch "<name>"]` stanza behind. Checked before committing, and it does not: grepping .git/config afterwards found no stanza for the deleted branch, and the only stale one in the file is an unrelated `undefined`. The branch had no stanza to drop in the first place -- writing one needs `push -u` or `checkout -b --track`, both denied by the two bullets above, so branches created under this sandbox never have one. Recorded as the general case, with the pre-sandbox branch explicitly marked untested rather than assumed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014FQA38o22RkHZJ4zEHDZS3 * docs: reconcile worktree-remove bullets with the rest of the permissions doc The new destructive-failure bullets for `git worktree remove`/`prune` contradicted three claims in the same file: the unattended list (which still named `git worktree remove`), the "sandbox makes the unattended list safe" thesis, and the "git already refuses the dangerous case" reasoning for the ask list. A reader of only the Permissions section could conclude the command was safe to call from sandboxed Bash — the exact bug the skills no longer call. Carve the two verbs out as explicit exceptions and scope the git-refuses reasoning to the cases git actually covers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: report worktree paths, not branch names, in the prune output The sweep's emitted maintainer command reconstructed `.claude/worktrees/<name>` from branch names, but `git worktree list` yields paths and includes sibling worktrees outside `.claude/worktrees/` — the branch name alone cannot locate the worktree, so removal would silently miss its target. Report `$wt` in the PRUNE/CARCASS lines and drive the command from those paths. Also count PHANTOM in implement-issue's emit-one-command line, matching sweep-prs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs: fold the branch delete into Step 11's deferred worktree removal The After-Merge cleanup splits in two: `ExitWorktree action=remove` clears the worktree in-session, or removal is handed to the maintainer when the session has already left. The old item 3 ran `git branch -D` right after either path, but git refuses to force-delete a branch while its worktree registration persists — the exact state the deferred path leaves behind, since only the maintainer's not-yet-run command clears the registration. Emit the branch delete as part of that same deferred command (remove first, then delete, as sweep-prs does), and scope item 3 to the in-session path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Summary
implement-issueStep 11 now runsgh pr ready <n>when the Stage 4 bot returnsAPPROVED, instead of leaving a reviewed, green PR sitting in draft.Root cause
Not a bug — a gap in the skill, found by running it. Step 11 (added in #600) drives the independent review loop to
APPROVED, but Steps 10 and 12 were written before that loop existed, back when the skill handed over an unreviewed PR. So they still said "do not take it out of draft" and "Draft PR only", and the skill stopped at "approved, green, still a draft".That end state makes the maintainer re-derive something the loop already established: notice the PR, judge whether it is finished, flip it, then merge. Three manual steps standing in for one.
Fix
APPROVEDbranch — runsgh pr ready, with the reasoning for why an approval is what earns the flip. Guarded: if commits landed after the approving review (aTODO.mdnit, agit merge origin/main), re-checkgh pr checksandmergeablefirst and report which commits landed after the review and whether they touched reviewed code. That is the one way this step could mislead.Test plan
./scripts/quality-check.shpasses locallyAPPROVED,gh pr ready 601flipped it, and fix: stop rewriting a running TOU window's end before the change matters (#589) #601 is now green,MERGEABLE/CLEAN, ready, unmerged — exactly the end state this diff codifies. TheTODO.mdnit parked on fix: stop rewriting a running TOU window's end before the change matters (#589) #601 after its approval is also the concrete case the "commits landed after the review" guard is written for.Evidence the test discriminates
Not applicable, and not skipped silently: this diff is prose in
.claude/skills/andCLAUDE.mdwith no executable behaviour, so there is no mutation that could redden a test. The equivalent evidence is the manual execution above — the flow was run against a real PR first, and the wording follows what that run actually needed (including the post-approval-commit guard, which only surfaced because it happened).Outcome-level coverage
None, because no optimizer behaviour changes — no DP, intent, control-mapping or economics code is touched. The affected files are
.claude/skills/implement-issue/SKILL.mdandCLAUDE.md.Documentation check
Grepped
docs/agents/bess-knowledge.mdanddocs/SOFTWARE_DESIGN.md— neither mentions PR draft state or the review loop, so nothing to update there.docs/agents/workflow.mdanddocs/agents/rules.mdmention drafts only in the CI/Stage-3 sense, which this change deliberately leaves alone.CHANGELOG.mdis intentionally not touched: agent tooling has no user-visible effect, matching #600, which changed the same skill and added no entry.