feat: enforce review-loop legality and derive fleet state from GitHub - #638
Closed
johanzander wants to merge 4 commits into
Closed
feat: enforce review-loop legality and derive fleet state from GitHub#638johanzander wants to merge 4 commits into
johanzander wants to merge 4 commits into
Conversation
Step 11's review loop kept two facts only in the agent's head — how many rounds had happened, and whether the last verdict had been acted on. A session that died, timed out, or lost the thread re-entered the loop by doing the one thing always available: asking for another review. PR #619 is the worked example. Four `@claude-bot review` comments, two paid CHANGES_REQUESTED verdicts with different blocking findings, and one byte-identical diff between them (HEAD unchanged since 07:35): 06:55 request (no run — actor gate) 07:12 request (no run) 09:33 request 09:40 CHANGES_REQUESTED 10:50 request <-- nothing pushed since 07:35 10:55 CHANGES_REQUESTED Step 11's prose already forbade this and already capped rounds at 3. Prose was not the enforcement mechanism, because the state it reasons about did not survive the session. It does not need to: both facts are already on the PR. request-pr-review.sh now refuses an illegal round before posting anything — decisive review count for the cap, and newest-verdict vs newest-commit for "was it consumed". --allow-unconsumed is the escape hatch for the one case Step 11 sanctions, where the finding was wrong and you replied on the PR instead of pushing. An unreadable gate refuses rather than spending a round, matching the existing rule that "I could not tell" must never license the expensive action. pr-state.sh answers "is anyone working on this, is it stalled, blocked, in progress, or waiting on review" in one command. That question was being answered by a fresh session running half a dozen ad-hoc commands over LOCAL signals — git worktree list, claude agents --json, HEAD age — which see nothing when the executor is a container or an Action. Asked about #619 that approach reported "no one is working on it, idle since 10:55", but 10:55 was the REVIEWER's timestamp; the last code action was four hours earlier. Liveness is reported as not-derivable rather than guessed, because the states that matter (needs-fix, needs-refresh) name a change the diff still owes regardless of who is alive. Ordering note: an unconsumed CHANGES_REQUESTED outranks a conflict. Both belong to the executor, but reporting only the conflict — as an earlier ordering did — hides blocking reviews behind a mechanical merge, which is exactly #619's shape. Complements #631, which taught Step 0 and Step 11 to read the comment feed: that governs WHAT to read, this governs whether another round is legal at all. Both scripts' decision paths are covered by tests seen to fail under mutation: disabling the gate reddens 4 of the 7 new review tests, and putting the conflict check first reddens the #619 classification. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fixture tests could not see either of these. Both turned up on the first live run. 1. GraphQL node limit. `--json commits` expands each commit's authors connection, so gh's cost estimate is limit x commits x authors. At --limit 100 that is 1,000,000 possible nodes and the query is rejected outright, so the command produced no output at all. --limit 30 keeps the worst case at 300,000. There is no cheaper field for "when did HEAD last move": `gh pr list --json` has no last-commit date, and a review's own commit SHA is REST-only. The cap is announced when hit rather than silently truncating, per sweep-prs. 2. Lazy `mergeable`, which is the dangerous one. The first query on a cold PR returns UNKNOWN *and* only then triggers the computation, so treating UNKNOWN as "not conflicted" hides precisely the stale PRs this script exists to surface. Measured: a first fleet run classified #167 and #619 with no conflict flag; once earlier queries had warmed them, the identical command returned `needs-refresh` for both. They were CONFLICTING the whole time. sweep-prs documents this trap and retries for the same reason; this reintroduced it. Now: re-ask while anything is UNKNOWN, and if it survives, render it as "(+mergeability UNKNOWN — re-run)" rather than letting it fall through to the clean branch. Verified by mutation: forcing $mergeUnknown to false reddens test_unknown_mergeability_is_never_reported_as_clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…iewer" The Stage 4 bot only ever acts when triggered by an `@claude-bot review` comment. So "green, no verdict yet" has two completely different meanings, and only one of them belongs to the reviewer: request NEWER than last push -> the bot is genuinely working: reviewer request OLDER, or absent -> nobody has asked: DISPATCHER Collapsing those parked six of eleven open PRs on someone who had not been asked and was never going to act. Measured on the live fleet: #637, #635 never requested at all #620 requested 17:40:53, pushed 18:33:05 #619 requested 10:50:40, pushed 21:53:00 #614 requested 06:55:38, pushed 07:08:20 #490 requested 08-15 13:41, pushed 08-16 15:30 Every one had been reported as `awaiting-review [reviewer]`. After this change the same fleet shows zero PRs waiting on the reviewer and zero waiting on the maintainer — the pipeline owes an action on all of them. That is the failure this whole branch is about, seen from the other side. The gate stops a confused loop asking too often; this surfaces the loop that stopped asking at all. Both are the same lost state — whether the last verdict has been consumed — and both are recoverable from the PR rather than from a session that died. `comments` joins the field set for this. It fits inside the GraphQL node budget at --limit 30, which is already bounded by `commits`. Verified by mutation: disabling the request-feed check reddens test_a_review_never_requested_is_the_dispatchers_turn_not_the_reviewers and test_a_push_after_the_last_request_owes_a_new_round. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… branch Two halves of the same failure: work landing outside a worktree, and nobody noticing when it does. ## The hook CLAUDE.md has said "never edit any file on main, even a one-line doc fix" unconditionally for a long time, and it keeps being skipped. The reason is structural, not carelessness: it is prose, so it has to be REMEMBERED at the moment of the first edit — and that is exactly the moment a session which opened as a question has no reason to reconsider it. Six live sessions currently sit in the main checkout for perfectly good read-only reasons; nothing catches the one that quietly starts editing. check-worktree-path.sh already guarded CROSS-checkout edits and passed same-checkout ones, so main-to-main sailed through. It now also refuses any edit made from the main checkout, detected by --git-dir equalling --git-common-dir. That is a path comparison, the only shape docs/agents/rules.md sanctions here — it never guesses what a command will touch. Linked worktrees and sibling checkouts both differ, so both still work; the rule is "be in a worktree", not "be under .claude/". The denial names the remedy (EnterWorktree) and says what the main checkout still does — questions, gh, backlog, dispatch — because a block without a next move gets worked around. Residual gap, stated plainly: this governs Edit/Write/NotebookEdit. A Bash `sed -i` still writes. Guarding that would mean parsing command strings, which rules.md forbids for this hook and which has produced false positives here four times. ## The detector pr-state.sh gains a local-writer section. GitHub cannot see this: a branch with two writers looks normal through the API, because the divergence exists only between a local checkout and the remote and it collapses into an ordinary merge the moment someone reconciles. #619 is the worked example. One writer took the branch at 08:09 and worked from that base; another pushed 23031e7 at 09:34. The reviewer reviewed 23031e7 three times, twice with blocking findings, while the first line never held that commit. Fifteen hours later it landed as `Merge remote-tracking branch 'origin/fix/...' into fix/...` — a branch merged into itself, which is the fingerprint. `git rev-list --left-right` would have caught it at 09:34. Run against the live fleet it also surfaces the precursor state: #437 is 5 commits behind its own remote and #614 is 3, so a commit in either worktree diverges immediately. Skipped LOUDLY outside a checkout, since this script is also meant for a container fleet where a silent skip would read as "no divergence found". Verified by mutation: disabling the main-checkout guard reddens 2 of the 6 hook tests. The divergence detector is tested against a real two-clone scenario, not fixtures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Open
6 tasks
4 tasks
Collaborator
|
Superseded by #658 (merged 2026-08-20) — the issue/PR state machine there replaces this gate. Closing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two pieces of
implement-issueStep 11's loop state lived only in the agent's head — how many rounds had happened, and whether the last verdict had been acted on. A session that died, timed out, or lost the thread re-entered the loop by doing the one thing always available to it: asking for another review.scripts/request-pr-review.shnow refuses an illegal round before posting anything.scripts/pr-state.sh(new) answers "is anyone working on this, is it stalled, blocked, in progress, or waiting on review" in one command, from GitHub facts only.Root cause
PR #619 is the worked example. Four
@claude-bot reviewcomments, two paidCHANGES_REQUESTEDverdicts with different blocking findings, and one byte-identical diff between them —headRefOidnever moved after 07:35, and all three reviews carry the samecommit_id:@claude-bot review(no run — actor gate)@claude-bot review(no run)@claude-bot review@claude-bot review← nothing pushed since 07:35Step 11's prose already forbade this ("Fix the blockers … Then start the next round") and already capped rounds at 3. Prose was not the enforcement mechanism, because the state it reasons about did not survive the session.
It does not need to. Both facts are already on the PR:
rounds so far= decisive reviews (APPROVED/CHANGES_REQUESTED)consumed?= is the newest commit newer than the newest verdict?A verdict newer than the last push is a verdict about the current diff, so the only legal next move is to change the diff. Asking again cannot help.
The gate
Exit 1, before the trigger comment is posted:
CHANGES_REQUESTEDAPPROVEDon this diffgh pr ready--allow-unconsumedis the escape hatch for the one case Step 11 sanctions: the finding was wrong and you replied on the PR rather than pushing. A flag, so "the reviewer is mistaken" is a decision someone takes, not the path a stalled loop slides into.An unreadable gate refuses rather than spending a round — the same stance the script already takes on an unreadable run state ("I could not tell" must never license the expensive action).
pr-state.shThat question was previously answered by a fresh session running half a dozen ad-hoc commands over local signals —
git worktree list,claude agents --json, HEAD age. Those see nothing when the executor is a container or a GitHub Action, so every PR reads as unowned.Asked about #619, that approach reported "no one is working on it, session idle since 10:55." 10:55 was the reviewer bot's timestamp —
updatedAt, bumped by someone else. The last executor action was the 10:50 request; the last code action was four hours earlier. It conflated "something happened on this PR" with "the agent is alive". It also contradictedsweep-prs's own rule that idle means owned.On #619's real data this now prints one line:
Liveness is reported as not-derivable rather than guessed. GitHub knows a PR's content state, never whether a process is working on it — and the states that matter (
needs-fix,needs-refresh) name a change the diff still owes regardless of who is alive.Ordering: an unconsumed
CHANGES_REQUESTEDoutranks a conflict. Both belong to the executor, but reporting only the conflict hides blocking reviews behind a mechanical merge — exactly #619's shape. Conflict still outranks CI, because aCONFLICTINGPR gets no run at all and would otherwise read as green.sweep-prsremains the tool that acts; this one only reads, so it is safe to run against a fleet whose worktrees live on another machine.Relationship to #631
Complementary, and they touch adjacent lines in the same file. #631 taught Step 0 and Step 11 to read the comment feed — that governs what to read. This governs whether another round is legal at all.
Test plan
./scripts/quality-check.sh— Errors: 0, Warnings: 0bash -nclean on both scriptsEvidence the tests discriminate
Both decision paths were mutated and watched to redden:
if false; thenon both guards) → 4 failures:test_an_unconsumed_changes_requested_refuses_the_next_round,test_an_approved_verdict_on_the_current_diff_is_not_re_reviewed,test_the_round_cap_is_enforced_here_not_remembered,test_an_unreadable_gate_refuses_rather_than_spending_a_round. The remaining three new tests stayed green by design — they assert the gate permits legal rounds, so they guard against over-blocking rather than under-blocking.pr-state.sh→test_619_shape_reports_the_findings_not_just_the_conflictfails, reportingneeds-refresh [sweep]and losing both blocking reviews.Scope assessment
Local. The gate is a precondition block in a script that already owns the request/verdict decision; no caller signature changes (
--allow-unconsumedis optional and prepended).pr-state.shis additive and read-only. No production code touched —core/,backend/app.pyand the frontend are untouched.Workaround check: nothing here routes around an ordering, timing or dependency problem. The gate reads state that already exists rather than introducing a new trigger, flag-to-sequence-things, or second construction site. No fallback and no silent degradation — an unreadable gate fails loudly.
No
CHANGELOG.mdentry, matching the precedent for agent-tooling commits (#594, #587, #581, #576, #573 all skip it); it tracks user-facing BESS Manager changes.Not covered
pr-state.shis exercised against fixtures, not liveghoutput. The field names come from #619's real payload, but a futureghschema change would not be caught here.🤖 Generated with Claude Code