feat: close the implement-issue review loop with the Stage 4 bot - #600
Conversation
Step 6's code-review is the session reviewing its own diff, and in practice the independent Stage 4 bot still takes two to four rounds to run out of real findings. Driving those rounds by hand from a second session is purely mechanical, so make the skill do it. New Step 11 runs the loop from the main session, not a subagent: the wait is a background bash script (scripts/request-pr-review.sh) that costs no tokens while it blocks, and the fixing stays where the Step 2 diagnosis and Step 3 scope assessment still live. That context is what lets a finding that contradicts a deliberate decision get a reply instead of a commit — a context-free subagent would just implement whatever the reviewer said. pr-review.yml's gate widens to accept bess-agent alongside the repo owner so the loop can request its own review. Stages 1-3 and 5 stay owner-only; Stage 4 is the only one that reviews a diff the session already opened, and the 3-round cap bounds the spend. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kvndqxz48QtpJ2Ub6PGCtB
|
@claude-bot review (Pre-merge gate test from the branch that widens it — under the current default-branch |
|
Pre-merge gate test result: as predicted. The comment above produced run 31872296509 with conclusion That is the stronger of the two possible confirmations: the Remaining verification, owed after merge: one |
|
@claude-bot review |
![bess-manager-claude-bot[bot]](https://avatars.githubusercontent.com/in/3024553?s=60&v=4){kind=small}
|
|
||
| deadline=$(( $(date +%s) + timeout )) | ||
| while [ "$(date +%s)" -lt "$deadline" ]; do | ||
| sleep "$interval" |
There was a problem hiding this comment.
Nit: the loop does an unconditional sleep "$interval" (60s) before the first deadline check:
deadline=$(( $(date +%s) + timeout ))
while [ "$(date +%s)" -lt "$deadline" ]; do
sleep "$interval"If request-pr-review.sh <n> <timeout> is ever called with a timeout shorter than interval (60s), the script will still block for a full 60s before checking, overshooting the requested budget and only then hitting the deadline check and exiting 2. Not reachable via the documented implement-issue Step 11 usage (scripts/request-pr-review.sh <n>, default 900s), so non-blocking — but worth a min(interval, timeout) first sleep or a pre-loop check if the script is ever called with a custom short timeout.
There was a problem hiding this comment.
Review summary
No linked issue — this is a process/tooling PR (automates the implement-issue → Stage 4 review loop), not a bug fix, so "does the fix match the linked issue's root cause" doesn't apply.
Architecture compliance / error handling checklist: N/A — no Python touched (only bash, YAML, markdown).
Minimality: the diff is tightly scoped to the stated goal — new script, one skill section, one workflow if clause, matching doc updates in CLAUDE.md/docs/agents/workflow.md. No scope creep.
Security review of the widened gate (.github/workflows/pr-review.yml):
if: |
(github.event.comment.user.login == github.repository_owner ||
github.event.comment.user.login == 'bess-agent') &&
github.event.issue.pull_request != null &&
contains(github.event.comment.body, '@claude-bot')Verified the added parentheses correctly scope the || before the && chain (GitHub Actions expression precedence checked directly, and confirmed the YAML parses via yaml.safe_load). GitHub logins are globally unique, so 'bess-agent' cannot be spoofed by an unrelated account — the only way to hit this branch is the actual automation identity whose PAT (BESS_AGENT_TOKEN) is already trusted elsewhere in this repo (scripts/gh-agent.sh). The reviewer job still checks out the default branch, not the PR head, so this doesn't open a path to running attacker-controlled code from a malicious PR. No new injection surface found in scripts/request-pr-review.sh — all interpolated values ($pr, $since) are script/git-derived, not attacker input.
Self-consistency check: .claude/skills/implement-issue/SKILL.md's Step renumbering (old Step 11 "Hard constraints" → Step 12, new Step 11 "Independent review loop") is applied consistently everywhere it's referenced (CI-mode table, "Green and mergeable" bullet, "After Merge" section, anti-pattern table, the final step-index table). (I initially misread this against local main instead of the PR branch and thought I'd found a stale reference — re-checked against git show <PR-head>:... and it's correct.)
Tests: no automated test added for request-pr-review.sh, but that matches existing repo precedent — none of scripts/gh-agent.sh, scripts/quality-check.sh, scripts/verify-sandbox.sh, or scripts/worktree-setup.sh have one either. The PR's test plan does show real evidence (jq run against actual PRs #596/#594, bess-agent login confirmed via scripts/gh-agent.sh api user) rather than assertions about untested code.
One nit (posted inline on scripts/request-pr-review.sh:45): the poll loop sleeps 60s before checking the deadline, so a caller passing a custom timeout shorter than the 60s interval would overshoot it. Not reachable through the documented implement-issue Step 11 usage (always called with the 900s default), so non-blocking.
One open question, not blocking: the PR omits a CHANGELOG.md entry with the reasoning "agent tooling with no user-visible effect on the add-on." Repo history isn't fully consistent with that being an automatic exemption — a past release logged "Analysis agent — restructured the @claude-bot analyze pipeline..." and "Agent documentation sync from beta" as CHANGELOG.md entries, i.e. bot-pipeline-only changes have been logged before. Worth confirming this is a deliberate call rather than an oversight, but I don't think it should block merge — CHANGELOG.md's own scope ("notable changes to BESS Battery Manager") plausibly excludes this.
No rule violations found. Approving.
|
@claude-bot review |
The poll loop slept a full interval before its first deadline check, so a caller passing a timeout shorter than the 60s interval overshot its budget before exiting 2. Sleep the lesser of interval and remaining instead. Found by the Stage 4 review on #600. Verified: `request-pr-review.sh 600 5` now exits 2 in 8s (5s timeout + comment post), against ~63s before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kvndqxz48QtpJ2Ub6PGCtB
…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
Automates the loop that is currently driven by hand from a second session: open PR →
@claude-botreview → fix findings → re-review, 2–4 rounds until only nits remain.Summary
implement-issue— "Independent review loop", after Step 10 (watch to green) and before the hard constraints. Requests the Stage 4 review, fixes findings, repeats. Hard cap 3 rounds.scripts/request-pr-review.sh— posts@claude-bot reviewasbess-agentand blocks until a new review lands, printingVERDICT <STATE> <submittedAt> <author>. Exit 2 on a 15-minute timeout, after dumping recentPR Reviewruns.pr-review.ymlgate widened — acceptsbess-agentalongside the repo owner.Why the loop runs in the main session, not a subagent
The waiting is mechanical; the fixing is not. Triaging a finding requires the Step 2 diagnosis and the Step 3 scope assessment to tell a real bug from a reviewer objecting to a decision that was made deliberately. A fresh subagent has neither and will implement whatever the reviewer says — the exact failure
superpowers:receiving-code-reviewexists to prevent.The cost argument for backgrounding applies to polling and narrating through the wait, not to the fixing. So the wait is a
run_in_backgroundbash script — zero tokens while it blocks — and the fixing stays in the session that holds the context.Blast radius on the bot gate
Stages 1–3 and 5 stay owner-only: those spend money on work nobody has asked for yet. Stage 4 is the only stage that reviews a diff the requesting session already opened, and the 3-round cap bounds the spend at ~$1.50–6 per issue.
Test plan
./scripts/quality-check.sh— green (0 errors, 0 warnings).ifexpression renders correctly (checked withyaml.safe_load).jqwas run against real PRs:#596→COMMENTED 2026-08-15T06:59:53Z by johanzander;#594(no reviews) → empty, i.e. the loop keeps waiting rather than false-positiving.bess-agentlogin confirmed as literallybess-agentviascripts/gh-agent.sh api user --jq .login, not assumed.Not yet verified, and cannot be before merge:
issue_commentworkflows always run the copy of the file on the default branch, so the widened gate does not take effect until this merges. Until then abess-agentcomment is silently ignored. See the verification comment below for the pre-merge half of the check.Docs
CLAUDE.mds "only the repo owner can trigger bot commands" and the Stage 4 block indocs/agents/workflow.mdboth record the exception.No
CHANGELOG.mdentry: agent tooling with no user-visible effect on the add-on.🤖 Generated with Claude Code
https://claude.ai/code/session_01Kvndqxz48QtpJ2Ub6PGCtB