fix: guard git push by shape so feature-branch pushes run unattended - #620
Draft
johanzander wants to merge 2 commits into
Draft
fix: guard git push by shape so feature-branch pushes run unattended#620johanzander wants to merge 2 commits into
johanzander wants to merge 2 commits into
Conversation
The blanket `Bash(git push *)` ask rule stalled every autonomous run at `implement-issue` Step 9 and at every `sweep-prs` push. It was justified in `quality-check.sh` by the claim that a marker at an arbitrary argument position is unreachable by a prefix glob — and that claim was false. The matcher in the same file is `re.fullmatch` with `*` -> `.*`, which spans spaces, so `Bash(git push *--force*)` matches `git push origin main --force`. The greedy-globs note further down said exactly that; the two comments contradicted each other for four review rounds and the blanket rule was built on the wrong one. The earlier enumeration leaked because it was prefix-ANCHORED (`Bash(git push --force*)`), which really cannot reach position 3. The `*marker*` spelling can. Push is now guarded per shape: force in any position, refspec `+`, ref deletion, `--mirror`/`--prune`/`--tags`, release tags, and anything naming main/master/beta in either the `origin main` or `HEAD:main` spelling. A push naming a feature branch runs unattended. Verified against every push spelling the gate already pinned plus real branch names from this repo: 20 dangerous shapes still guarded, 20 read-only and feature-branch shapes unattended, 0 holes and 0 false prompts. `git push --force-with-lease` still asks, deliberately. - `MUST_BE_GUARDED` gains the colon-refspec forms a `* main` pattern cannot see (`HEAD:main`, `HEAD:refs/heads/main`, `:main`, `HEAD:beta`), plus `--force-with-lease`, `-f`, `--mirror` and `--tags`. - `MUST_NOT_BE_GUARDED` gains the feature-branch pushes and `maintenance-cleanup`, which pins the `main`-substring trap that would return if the protected-ref rules were loosened to `* main*`. - `REQUIRED["ask"]` no longer names the push rules: 52 pattern names would recreate the presence-check failure mode this gate exists to catch. The command strings hold it instead. - `gh api` keeps its blanket rule — any `-f`/`-F`/`-X` turns a read into a mutation, so its safe subset has no lexical marker. CLAUDE.md's Permissions section asserted the opposite in three places and is corrected to match.
#617 landed the gh release verb split, which touches the same three files. Both conflicts were in lists that each PR appends to: - CLAUDE.md's escapes-to-GitHub row: keep this branch's per-shape push clause and main's `delete-asset` addition. - quality-check.sh MUST_NOT_BE_GUARDED: keep both blocks, the feature-branch pushes and the read-only gh release verbs. Also pins `gh release download v9.9.0`, which #617's review flagged as claimed-unattended in its body and CLAUDE.md but pinned by no command string. Behaviour was already correct; only the verification was missing -- the same "rule that looks right and matches nothing" failure this list exists to catch. Gate after resolution: 95 command shapes checked, 20 require deny, 25 must stay unattended, Errors: 0.
Collaborator
|
@claude-bot review |
![bess-manager-claude-bot[bot]](https://avatars.githubusercontent.com/in/3024553?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Summary
This PR replaces the blanket Bash(git push *) ask rule with a per-shape enumeration (force, delete, mirror/prune/tags, release tags, protected-branch names). The stated goal — closing the false-prompt-that-stalls-autonomous-runs problem while keeping main/master/beta protected — is legitimate, and the PR does the right diligence: it cites matches()'s actual regex semantics, runs scripts/quality-check.sh's pinned command-string gate, and documents the reasoning at length in CLAUDE.md.
However, I verified the new enumeration against real git push semantics (not just the pinned test strings) and found two more real holes in the same category this PR exists to close — i.e. exactly the "leaked, twice" failure mode the PR's own description warns about, now a third time, in the PR meant to fix it.
Findings
1. git push origin refs/heads/main runs unattended — CONFIRMED, blocking
.claude/settings.json lines 103–115 (ask) key every protected-branch guard off a literal space ("Bash(git push * main)") or colon ("Bash(git push *:main)", "Bash(git push *:refs/heads/main*)") immediately preceding the branch name. But git push origin refs/heads/main is a completely ordinary, single-sided refspec — the character before main is /, not a space or colon — and it pushes local main straight to remote main.
I verified this against a real repo (not just the regex):
$ git push origin refs/heads/main --dry-run -v
To ../remote.git
2412ebb..9866033 main -> main
And against the PR's own matches() function (scripts/quality-check.sh:396-399), none of the 50 MUST_BE_GUARDED strings, nor any of the 25 ask push patterns, match git push origin refs/heads/main (same for refs/heads/master, refs/heads/beta).
Needs a source-side *refs/heads/main* / *refs/heads/master* / *refs/heads/beta* pattern (mirroring the destination-side *:refs/heads/main* ones already present) in both ask and MUST_BE_GUARDED.
2. git push --all (and git push --all origin) runs unattended — CONFIRMED, blocking
The same ask block guards --mirror, --prune, --tags (.claude/settings.json:97-99) but not --all. --all pushes every local branch, including main, to the remote.
Verified against a real repo:
$ git push --all origin --dry-run -v
To ../remote.git
2412ebb..9866033 main -> main
* [new branch] some-feature -> some-feature
Confirmed via the matcher: no ask/deny pattern matches git push --all or git push --all origin.
Checklist
- Matches a linked issue's root cause? N/A — this is a self-initiated infra PR (no linked GitHub issue), with root cause well-documented in the PR body itself and traceable to the false claim in the pre-existing
quality-check.shcomment. - Would the added test catch a regression? Partially.
scripts/quality-check.sh'sMUST_BE_GUARDED/MUST_NOT_BE_GUARDEDgate is a good mechanism and correctly pins every scenario it enumerates (verified: ran it on the PR branch, green, 95 shapes). But it only proves "the strings we thought of are covered" — it does not prove "no valid git-push spelling reaches a protected branch," which is the actual property this PR wants. The two holes above show the enumeration is still incomplete after two prior rounds of the same mistake. - Minimal / no scope creep? Yes — the diff is tightly scoped to the push-permission shapes plus matching docs/gate updates.
- Rule violations? None from
docs/agents/rules.md(no Python/architecture rules apply to this config/docs/bash-script change).
Recommendation
Add the two missing shapes (refs/heads/<protected-branch> as a bare source-side ref, and --all) to ask and MUST_BE_GUARDED, and re-run scripts/quality-check.sh. Given this is the third leak of the same enumeration category in this repo's history, it's also worth asking — per the PR's own "Fitness of approach" self-critique — whether a normalization step (resolve the refspec's effective destination ref via git rev-parse/git for-each-ref semantics before matching, rather than pattern-matching the raw command string) would close this class of gap for good rather than requiring a fourth round of enumeration next time it's found. Not blocking this PR by itself, but worth a follow-up issue.
🤖 Generated with Claude Code — PR Review Bot
Owner
Author
Prerequisite is now in place — this PR should shrink
So Why that matters for this PRReview found two more holes (
Correct. Prefix globs cannot express that property, so enumerating protected-ref spellings will keep leaking. Agreed directionDelete the protected-ref half of the enumeration and keep only the shapes that are lexically unambiguous and not otherwise prevented:
Net effect: a much smaller pattern set, and the half that has failed three times stops existing. Still to do here
The |
johanzander
added a commit
that referenced
this pull request
Aug 17, 2026
`implement-issue` is used for TODO.md items and refactors, not only for issues, so a draft PR with no linked issue is normal rather than a defect. The pass reported "no issue references this PR; finish it by hand", which left every self-directed PR with no owner in the loop -- exactly how #620, #622 and #623 all ended up driven by hand today. No flag distinguishes the two cases: GitHub numbers issues and PRs from ONE sequence per repo, so a bare number is already unambiguous and Step 0 can resolve whichever it is. An earlier draft of this used `--pr <n>`; that distinction carries no information. Where an issue IS linked it is still named, because it carries the diagnosis. Where none is, the PR number is the handle, and a strong one: it holds the branch, the diff, the scope assessment and the review verdict, which is everything Step 0 reads. Step 0 accepting a PR number is a matching change to implement-issue's SKILL.md, which lives on the fix/review-verdict-placeholder branch (#622) where Step 0 was added. Both have to land for the loop to cover this case.
johanzander
added a commit
that referenced
this pull request
Aug 17, 2026
This skill is used for TODO.md items and for refactors that never had an issue, so "issue number" was too narrow a contract. Step 0 already keys off observable state; a PR is simply another entry point to it, and the stronger one -- it carries the branch, the diff, the `## Scope assessment` and the review verdict, which is everything Step 0 reads. No flag is needed. GitHub numbers issues and PRs from ONE sequence per repository, so a bare number is unambiguous: try `gh pr view <n>`, fall back to `gh issue view <n>`. An earlier draft used `--pr <n>`; that distinction carries no information. Why it matters beyond tidiness: scripts/backlog-rhythm.sh hands unfinished drafts back to this skill, and for a PR with no linked issue it had nothing to hand -- it reported "no issue references this PR; finish it by hand". That left every self-directed PR with no owner in the loop, which is how #620, #622 and #623 all ended up driven by hand in one session. Where an issue IS linked, nothing changes: it is still read for the diagnosis. Where none is, Step 2's root cause comes from the maintainer's own framing rather than a Stage 2 comment, and Step 9 records it in the PR body as usual.
johanzander
added a commit
that referenced
this pull request
Aug 17, 2026
* fix: make backlog grooming reflect what actually blocks an issue The digest misclassified enough of the board that grooming could not be trusted, and every misclassification pushed work in the same direction: towards looking more ready than it was. Measured against the live board, Ready went 1 -> 0 and In Progress 5 -> 1. Five defects, each traced to a real item. 1. `analyzed` was tested BEFORE any wait, so an analysed-but-blocked item reported Ready. #96 was labelled `analyzed`, prioritised P2, carried no blocking label, and still could not be built because its approach was undecided. It read as dispatchable, a session was dispatched at it, and that session deadlocked on three design questions. Waits now outrank `analyzed`. 2. The board's `Awaiting` field was never read. 17 items have it set (`discussion` x11, `reporter` x6) and the digest derived its own value from labels instead, so recorded grooming had no effect on anything. The field is now authoritative, with `awaiting_source` and `awaiting_suggested` exposed so an unset field can be reconciled rather than silently invented. 3. `awaiting: discussion` was returned for ANY human comment, which is not a blocker -- thanks, a "me too" and a follow-up question all pushed an item to Analysis. Only a recorded wait or a blocking label does that now. 4. A worktree left on disk pinned its issue to In Progress forever. #593, #571, #542 and #466 all reported In Progress while their PRs (#618, #579, #591, #517) had merged. Staleness is decided by comparing the worktree's own branch against merged PRs -- an exact match, deliberately not the fuzzy issue-number match used to associate a worktree with an issue. 5. `blocked` did not fail Ready, so #571 reported Ready for Dev while labelled `blocked`. Definition of Ready criterion 5 now holds, including an unresolved `Blocked by #N`. Ready for Dev also finally requires a Priority, which the design always specified and the code deferred "until a board exists". It exists. New: `last_comment` {author, days, is_reporter, is_bot}. Without it the digest could not represent the transition that matters most to grooming -- the reporter answering us. A count and a date cannot tell that from a nudge we posted, which is why #621 crossed the Definition of Ready line unnoticed. REJECTED while building this: mapping a merged PR to Done. It reclassified 7 open issues (#118, #120, #403 among them) as finished, and it contradicts this project's rule that beta PRs omit `Closes #N` until graduation -- an open issue with a merged fix is the NORMAL state. `merged_pr` is reported; it moves no column, and a test pins that. Merged PR bodies are reduced to their closing references before reaching jq; passing 200 of them through --argjson overflows the argument list. Tests: 25 pass. The `gh` shim now branches on the full argument string, because `pr list` is called twice with different `--state` values and matching only the subcommand returned the open list for both -- which would have made every open PR look merged. Fixtures gained `createdAt` on comments, which real gh always sends and whose absence failed `strptime` rather than testing anything. `_run` now surfaces the digest's stderr instead of a bare "exit status 5". * fix: only a still-open Blocked by #N fails Ready Review of #623 found a real gap, and it contradicted an explicit claim: both the commit message and SKILL.md said this enforced an "unresolved" blocker, while `blocked_by` was a pure text scan that never checked whether the blocker was still open. A `Blocked by #N` line is never edited out of an issue body once N lands, so that scan pins the item out of Ready for Dev permanently. That is the same failure this script exists to fix, pointing the other way: an item reading wrong relative to its real state. `$issues` is already the open-issue list, so membership decides it with no extra API call. `blocked_by` keeps the raw parse so the reference stays visible; the new `blocked_by_open` is the subset that actually blocks, and `$blocked` reads that. Also pins the precedence question the review asked me to confirm rather than guess at: a recorded wait DOES outrank a live worktree in `column`, because unsettled scope must not read as progress. The risk is hiding active undelivered code, so the worktree stays reported on the item — the wait changes the column, not the evidence. Now tested and documented in the reconciliation table instead of being implied. 27 tests pass. * feat: a Rhythm pass that carries work from incoming to a ready PR Every follow-up rule in the backlog skill had been written down and NONE had ever fired. The 14-day reporter chase, the 28-day park, the reporter-replied re-check, the stale-worktree handoff: all decoration, because each needed a model to notice it and nothing scheduled one. So the noticing is deterministic now and lives in scripts/backlog-rhythm.sh. Every rule is a comparison over the digest — no judgement, no tokens. A quiet backlog prints "RHYTHM: nothing due." for the cost of one process, which is what makes it worth running on a timer at all. The PO agent is needed only to ACT, and only when something is due. The pass covers BOTH halves of the path to an approvable PR: Issue side — recheck_ready, nudge_reporter, park, surface_discussion, set_awaiting, set_priority, triage_labels, dispatchable. PR side — and this is the half that actually hands the maintainer something: mark_ready approved but still a draft <- the finish line awaiting_maintainer approved and out of draft request_review draft with no review at all rework changes requested resolve_conflict CONFLICTING (produces no CI run, so it reads as "checks never fired" and nobody investigates) Those two states were invisible in practice. #615 and #617 sat APPROVED and still drafts overnight with nothing left but the merge; #619 was never reviewed at all. Nothing was watching either transition. Ordering is load-bearing. PR actions come first because they are closest to the finish line, and recheck_ready outranks the chases: nudging someone who has already replied is the worst output this pass could produce. Quiet time is measured from the LAST COMMENT, not updatedAt — a label change or a board move bumps updatedAt, so an issue nobody has spoken on for a month would look active and never age into a chase. A bare COMMENTED review is not treated as a verdict, because the review bot posts its inline notes as one before the summary. Against the live board the pass finds 30 due actions, including PR #490 awaiting the maintainer, #162 park (quiet 54d), three stale worktrees and three conflicted PRs. RHYTHM_DIGEST_FILE / RHYTHM_PRS_FILE are test seams, the same shape as BESS_ENV_FILE in gh-agent.sh. 16 tests pin the rules, including that a quiet backlog is a noop and that a reply beats the chase. Still not wired to a schedule — that is the invocation, not the logic, and it is deliberately a separate step. * refactor: hand unfinished PRs back to implement-issue instead of duplicating it The previous commit built request_review / mark_ready / rework into the Rhythm pass, which is a second copy of implement-issue Step 11. That contradicts the argument used to put resume in Step 0 rather than in a separate skill: two copies of one review loop means one of them goes stale. It also mis-diagnosed the symptom. #615 and #617 did not sit APPROVED-but-draft because nothing was watching for that state; they sat there because the sessions that owned them exited before Step 11 finished. The fix belongs where the loop already lives. So every unfinished draft now resolves to ONE action, `resume_implementation`, carrying the issue number so the handoff is directly runnable. Step 0 re-enters at the earliest incomplete step, whether the PR needs a first review, a rework, or just the ready flag it never got. Two fleet-level exceptions stay in the pass, because implement-issue deliberately does not widen to them: `awaiting_maintainer` (report only) and `resolve_conflict` (sweep-prs). Adds the stalled-work rule this was missing: a LIVE worktree with no session behind it is an implementation that stopped mid-flight -- the machine restarted, the session was killed, or the agent exited between steps. Nothing picked these up, and an audit found 34 such worktrees, 8 holding real unpushed commits and one with 32. Against the live board it finds #466 and #602. `pr == null` guards that rule so work with a PR is reported once, by the PR branch, rather than twice. It is always a RESUME, never a restart: Step 4 branches fresh from origin/main and would delete commits that exist nowhere else. The detail string says so, and a test pins it. SKILL.md gains the reasons a future pass must not re-learn this: do not drive the review loop here; a session reporting `working` may have written nothing (three dispatches produced zero writes in one day while reporting healthy state); and read `claude agents --json` unsandboxed, since ~/.claude/jobs is sandbox-denied and a sandboxed listing returned 1 session where the truth was 17. 19 rhythm tests, 27 digest tests, gate green. * fix: match Blocked by #N per line, and drop dead code the refactor left Review of #623 found a real misclassification path in a PR whose whole point is eliminating them. `blocked_by` was a free `scan` over the issue body, so it matched the substring regardless of what preceded it. "not blocked by #500 anymore" and "no longer blocked by #500" both registered as live blockers -- and those are the natural way to update an issue once its blocker resolves, so the false positive fired exactly when the blocker was GONE. The item would be pinned out of Ready for Dev permanently. The severity is new, not latent: on main `blocked_by` was extracted and never fed into `column()`, so a bad parse was inert. Gating `column()` on it is what gave it teeth. Matched per LINE and anchored to the line start now, optionally bulleted, which is the convention the skill documents ("a `Blocked by #N` line in the issue body"). Anchoring rejects the negations without a blacklist that would only cover the phrasings someone happened to think of. It also fixes the reviewer's third point: an untriaged issue merely mentioning a blocker in prose no longer moves Backlog -> Analysis with no human triage behind it. Four tests: the bulleted form still counts, three negated phrasings do not, and an incidental mid-sentence mention does not reclassify. Also from the same review: - `human_comments` became dead code when `awaiting` stopped deriving `discussion` from comment activity. Removed, and the stale comment on `comments:` that still described that mechanism is corrected. - SKILL.md claimed the board's custom-field JSON shape was "confirmed" where the previous text had explicitly said unconfirmed, without showing the evidence. It was verified live; the command and its result are now recorded, with a note that the tests fabricate that shape and so cannot prove it. 49 tests pass (27 digest + 19 rhythm, plus the 3 new negation cases). * fix: resume a PR by its own number when no issue is linked `implement-issue` is used for TODO.md items and refactors, not only for issues, so a draft PR with no linked issue is normal rather than a defect. The pass reported "no issue references this PR; finish it by hand", which left every self-directed PR with no owner in the loop -- exactly how #620, #622 and #623 all ended up driven by hand today. No flag distinguishes the two cases: GitHub numbers issues and PRs from ONE sequence per repo, so a bare number is already unambiguous and Step 0 can resolve whichever it is. An earlier draft of this used `--pr <n>`; that distinction carries no information. Where an issue IS linked it is still named, because it carries the diagnosis. Where none is, the PR number is the handle, and a strong one: it holds the branch, the diff, the scope assessment and the review verdict, which is everything Step 0 reads. Step 0 accepting a PR number is a matching change to implement-issue's SKILL.md, which lives on the fix/review-verdict-placeholder branch (#622) where Step 0 was added. Both have to land for the loop to cover this case.
johanzander
added a commit
that referenced
this pull request
Aug 17, 2026
…sume for dead sessions (#622) * fix: wait for a terminal review verdict, not the bot's placeholder `request-pr-review.sh` took the LAST review newer than its trigger and called it the verdict. The review bot posts its inline notes first, as a COMMENTED review whose body is "Inline notes below; summary review to follow.", then submits the real APPROVED/CHANGES_REQUESTED summary seconds later. Measured on PR #617: placeholder at 06:57:13Z, APPROVED at 06:58:03Z — 50 seconds apart. Any poll landing in that window returned COMMENTED. `implement-issue` Step 11 then saw a non-APPROVED verdict and skipped `gh pr ready`, so an approved PR stayed a draft with nothing left to do but the merge. PR #615 sat that way overnight: CHANGES_REQUESTED, fixed, APPROVED at 21:13, still a draft the next morning. Filter on state BEFORE taking `last`, so only APPROVED or CHANGES_REQUESTED ends the wait. Verified against #617's real review history by simulating a poll at 06:57:30Z, when the placeholder was the newest review: the old expression returns COMMENTED, the new one returns empty and keeps waiting. The timeout path also conflated two opposite faults that printed the same message — a review that started and never summarised, versus a trigger that never reached the workflow. It now reports which one happened. PR #619 is currently the second kind, and that was invisible before. Step 11's own text told the agent to act on `COMMENTED`, so it is corrected to match; a bare COMMENTED can no longer reach the caller at all. * feat: resume an issue whose session died mid-flight (implement-issue Step 0) Sessions die mid-issue routinely and nothing picked them up. A fleet audit found 34 worktrees whose sessions had exited: 8 with real unpushed commits and no PR (one with 32 commits), plus three PRs sitting green-or-reviewed with no owner left. #615 was APPROVED and still a draft the next morning; #614 carried CHANGES_REQUESTED with nobody to act on it. `sweep-prs` refuses that job by design, so the work simply stopped. This lives in `implement-issue` rather than a new skill because the loop that acts on review feedback is Step 11 and already lives here. A second skill would duplicate it, and duplicating a review loop is how one of them goes stale. Step 0 keys off state observable from OUTSIDE the dead session — branch, worktree, commits, PR body sections, CI status, review verdict — and re-enters at the earliest incomplete step. The one thing that dies with the session is Step 2's diagnosis, which Step 11 depends on holding; it is recoverable only because this skill already forces it to be written down (the Stage 2 analyze comment, and the PR body's `## Scope assessment` and `## Test plan`). When those do not reconstruct a coherent approach, Step 0 STOPS rather than re-diagnosing on top of commits encoding decisions it cannot see. Hard rules, each from an observed failure: - never run Step 4's fresh-from-origin/main worktree when a branch for the issue already has commits — that deletes them - never reset or force-push a resumed branch; its commits are the only copy - check for a live session unscoped AND unsandboxed: a sandboxed `claude agents --json` returned 1 session where the truth was 17, because ~/.claude/jobs is sandbox-denied, so every other session read as dead - treat uncommitted tracked changes as unfinished work; WIP-commit first - if the same issue has died twice, say so and stop CI mode gets a Step 0 row too: Stage 3 is re-triggered by hand, so a second `@claude-bot fix` on an issue that already has a has-fix-pr PR is a resume, not a restart, and must not open a second PR. * fix: resolve an ambiguous COMMENTED review by time, and stop emitting it Review of #622 found the previous commit's fix incomplete, and it was right. `pr-review.yml:75` documents COMMENT as a legitimate FINAL verdict ("questions/observations only"), submitted with `gh pr review --comment`, which produces the same state == "COMMENTED" as the bot's inline-notes placeholder. Treating every COMMENTED as non-terminal therefore swallowed a real COMMENT verdict: the loop waited out the full timeout and reported "never submitted a summary" while a summary with findings sat on the PR. That over-generalised "the placeholder is COMMENTED" into "COMMENTED is always the placeholder". Fixed at the source and in the consumer. Source: pr-review.yml step 3 permitted `gh pr review` for inline notes, and that is what submits the extra review. It now requires `gh api .../pulls/N/comments`, so exactly ONE review is submitted per run -- the step 4 summary. No placeholder means no ambiguity. Consumer: the script no longer decides by state alone, and deliberately does NOT parse the placeholder's body -- that text is bot-generated prose with no contract behind it. APPROVED/CHANGES_REQUESTED return immediately; a COMMENTED-only state is held `grace` seconds (180, against observed placeholder-to-summary gaps of 16s on #622 and 50s on #617) to let a summary supersede it, and is returned as the verdict if none does. The grace window is what keeps this correct for reviews already on older PRs and if the bot regresses. Verified against #622's real review history: with both reviews visible the decisive branch returns CHANGES_REQUESTED and grace is never entered; in a window containing only the placeholder the COMMENTED branch finds it while the decisive branch is empty, so grace holds. The timeout message is also now correct rather than merely different: a COMMENTED-only run can no longer reach it, so reaching it means no review of any state was submitted -- a trigger fault, which is what #619 hit twice. SKILL.md's Step 11 said "It will never hand you COMMENTED"; a COMMENTED that now reaches the caller IS the verdict, so it is documented as carrying findings and not earning the ready flag, same as CHANGES_REQUESTED. * feat: Step 0 resolves a bare number to an issue OR a pull request This skill is used for TODO.md items and for refactors that never had an issue, so "issue number" was too narrow a contract. Step 0 already keys off observable state; a PR is simply another entry point to it, and the stronger one -- it carries the branch, the diff, the `## Scope assessment` and the review verdict, which is everything Step 0 reads. No flag is needed. GitHub numbers issues and PRs from ONE sequence per repository, so a bare number is unambiguous: try `gh pr view <n>`, fall back to `gh issue view <n>`. An earlier draft used `--pr <n>`; that distinction carries no information. Why it matters beyond tidiness: scripts/backlog-rhythm.sh hands unfinished drafts back to this skill, and for a PR with no linked issue it had nothing to hand -- it reported "no issue references this PR; finish it by hand". That left every self-directed PR with no owner in the loop, which is how #620, #622 and #623 all ended up driven by hand in one session. Where an issue IS linked, nothing changes: it is still read for the diagnosis. Where none is, Step 2's root cause comes from the maintainer's own framing rather than a Stage 2 comment, and Step 9 records it in the PR body as usual. * fix: decide a COMMENTED review by run state, and test the decision Third round on the same finding, and the previous two "fixes" were asserted rather than demonstrated -- verification was quality-check.sh plus `bash -n`, neither of which executes the decision path. So this commit changes the mechanism AND adds the missing tests. The grace window was the wrong instrument, and the review named the reason: it competed against the ORIGINAL deadline instead of extending it, so a COMMENTED first seen in the last `grace` seconds of the window could never satisfy the `elapsed >= grace` branch -- the loop exited first and reported "no review landed", which was false. Observed live on #622: the stub landed 12:15:14, the real CHANGES_REQUESTED 12:16:39, and the script returned the stub. Sizing it differently would not have helped. The gap that matters is not placeholder-to-summary (16s on #622, 50s on #617) but placeholder-to-END-OF-RUN: the bot posts an early permission-check comment within a couple of minutes and works for five to eight more. No constant is both short enough to return a real COMMENT promptly and long enough never to pre-empt a summary. So ask the run instead. `review_run_state` reads the PR Review workflow run started since the trigger: running -> a COMMENTED decides nothing; keep waiting finished -> a COMMENTED last word IS the verdict, per pr-review.yml's own three-verdict contract (APPROVE / REQUEST_CHANGES / COMMENT) failed -> report at once; do not burn the timeout on a dead run none -> the trigger never reached the workflow, a different fault That last one matters as much as the first. A dead run and a thinking one are both silence if you only poll for reviews, which is how #623's run -- already failed on "Reached maximum number of turns (60)" -- was waited on for 16 minutes. Also from this review round: - pr-review.yml no longer REQUIRES `gh api` for inline comments. The reviewer reported that `gh api` is permission-gated and unavailable to it, and that it probed with `gh pr review` to find out -- which submits, and is where the stray "test permission check" reviews came from. The prompt now states the one hard rule (submit exactly ONE review, never probe with it), prefers `gh api` for inline notes, and says to fold findings into the summary with file:line when it is unavailable, rather than falling back to a second review. - SKILL.md Step 0 keyed a resume signal on `## Scope assessment`, which only CI mode writes into the PR body. An interactive-mode PR never carries it, so the row would not match for most PRs this skill opens. It now keys on the PR existing, which is what actually proves Step 9 was reached. 7 new tests, REVIEW_POLL_INTERVAL added as their seam. Two of them are the discriminating pair: identical reviews (COMMENTED only), opposite outcomes, differing only in run state -- so the decision is provably driven by the new signal and not by timing. The shim applies `--jq` like real gh does; an earlier version echoed raw JSON and the script reported it as a verdict. * fix: give the verdict tests their own env file so they pass in CI The new test file could not pass in CI, and the review caught it with the failing run: `Fast tests` was red on this PR while local `quality-check.sh` was green. Reproduced both sides before fixing. Cause: request-pr-review.sh posts its trigger through scripts/gh-agent.sh, which reads a real BESS_AGENT_TOKEN and exits 1 before `gh` is reached. Shimming `gh` on PATH does not help -- gh-agent.sh is invoked by a repo-relative path, not looked up on PATH. Worse, it resolves its env file from the MAIN checkout (`dirname $(git rev-parse --git-common-dir)`), so this worktree having no `.env` of its own was irrelevant: the developer's real token was read anyway. CI provisions no `.env` and no such secret, so the suite failed unconditionally there. The abandoned `(d / "scripts").mkdir(...)` in the fixture was an attempt at this that did nothing. Fixed with the seam gh-agent.sh already documents for exactly this (`BESS_ENV_FILE`, "a seam tests use to point at a fixture .env instead"), so no production code changes: each test now supplies its own env file carrying a dummy token. Also adds a test that PINS that dependency, because the fix is otherwise invisible and could be dropped again silently: point the seam at an empty file and the script must fail before polling, naming the missing token. Worth recording how I nearly mis-verified this: my first attempt set BESS_ENV_FILE from OUTSIDE pytest and saw the tests still pass, which looked like proof of CI-safety. It was not -- the test sets that variable in the subprocess env, so it overrides any outer value and the experiment could not fail. The in-test pin above is the version that can actually discriminate. Full backend suite: 507 passed. * fix: an unreadable run state must not promote a placeholder to a verdict Review of #622 found a real correctness bug, reproduced against the shipped script: `review_run_state` falls back to `unknown` whenever `gh run list` itself fails -- network blip, rate limit, transient auth error -- and the COMMENTED branch tested only `state = running`, so `unknown` fell through to the else and reported the bot's placeholder as the verdict. That re-opens the exact race this script exists to close, gated on API flakiness instead of timing. "I could not tell whether the reviewer is still working" must never mean "it finished". It now waits, same as `running`: waiting costs one more poll, and a genuine COMMENT verdict still returns as soon as the state resolves. A test drives it, with the shim making `run list` exit 1 rather than returning a run shape -- which is what the failure actually looks like. That test also exposed a second defect in the same path: the timeout branch ends with `gh run list ... >&2` for diagnostics, and under `set -e` a failing `gh` -- precisely the `unknown` case -- aborted the script with exit 1 instead of the exit 2 that means "no verdict". Diagnostics must not decide the exit code, so it is `|| true` now. Also fixes the doc/implementation mismatch the review flagged: SKILL.md still described the 180s hold from the superseded commit, which would actively mislead the next reader about how Step 11 decides. It now describes the run-state mechanism, including that an unreadable state waits. 9 tests. Worth noting the `pr-review.yml` change from this PR is already working: this round the bot folded its findings into a single summary review and posted no stray placeholder.
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.
Problem
The blanket
Bash(git push *)ask rule stalled every autonomous run —implement-issueStep 9 and everysweep-prspush. In this session it blocked a routine sweep three times before the work could continue.scripts/quality-check.shjustified the blanket rule like this:That claim is false, and the proof is in the same file.
matches()is:*becomes.*, which spans spaces — soBash(git push *--force*)matchesgit push origin main --forceperfectly well. The greedy-globs note ~140 lines further down says exactly this. The two comments contradicted each other, and the blanket rule was built on the wrong one.Why the earlier attempt really leaked: it was prefix-anchored (
Bash(git push --force*)), which genuinely cannot reach argument position 3. The*marker*spelling can. That distinction is the whole fix.Change
Push is guarded per shape: force in any position, refspec
+, ref deletion,--mirror/--prune/--tags, release tags, and anything namingmain/master/betain either theorigin mainorHEAD:mainspelling. A push naming a feature branch runs unattended.Branch protection already refuses the case the blanket prompt was standing in for, so it cost an autonomous run and bought nothing.
Verification
Every push spelling the gate already pinned, plus real branch names from this repo:
git push origin main --force/--force-with-lease/-f origin maingit push origin HEAD:fix/issue-592-vpp-idle-at-floorgit push origin +beta-release-9.9git push origin feat/phase4c-charge-commandsgit push origin --delete release-9.9,:maingit push -u origin fix/issue-604-signed-pair-aliasesgit push origin v9.9.0,--tags,--mirrorgit push origin worktree-po-followupsgit push origin HEAD:main,HEAD:refs/heads/main,mastergit push origin maintenance-cleanup← themain-substring trapgit push beta main,HEAD:beta,beta-release-*-tmpgit -C .claude/worktrees/x push origin HEAD:fix/…git push, and all of the above viagit -C …/git -c …0 holes, 0 false prompts.
./scripts/quality-check.shgreen: "Permission surface intact (85 command shapes checked, 20 require deny, 20 must stay unattended)",Errors: 0, full suite 1992 passed / 125 frontend.The push that created this PR ran without a prompt — the change is demonstrated on itself.
Gate changes
MUST_BE_GUARDEDgains the colon-refspec forms a* mainpattern cannot see (HEAD:main,HEAD:refs/heads/main,:main,HEAD:beta), plus--force-with-lease,-f,--mirror,--tags.MUST_NOT_BE_GUARDEDgains the feature-branch pushes andmaintenance-cleanup. That last one pins the substring trap that would reappear if the protected-ref rules were loosened to* main*.REQUIRED["ask"]no longer names the push rules. 52 pattern names would recreate the presence-check failure mode this gate exists to catch; the command strings hold it instead.Deliberately unchanged
gh apikeeps its blanket rule. It is not separable this way: any-f,-For-Xturns a read into a mutation, so no lexical marker isolates the safe subset. If it stalls runs, the fix is an allow-list of read-only paths, not a shape guard.--force-with-leasestill asks. A lease protects against a concurrent writer, not against wrong local history.Docs
CLAUDE.md's Permissions section asserted the opposite in three places ("Everygit pushasks", "guarded bluntly, and that is deliberate", and the--force-with-leaserationale). All three are corrected, including the false "prefix globbing cannot reach it" premise.Follow-ups not in this PR
scripts/quality-check.sh:120— the mypy check degrades to aWARNINGwhenorigin/maincan't be resolved, and warnings exit 0, so it printsErrors: 0for a run that type-checked nothing. Found by the fix: enforce mypy on changed files, make the PR review bot always submit a verdict #614 review; being fixed there.gh release downloadis claimed unattended in docs but has noMUST_NOT_BE_GUARDEDstring pinning it. Behaviour is correct, verification is missing.🤖 Generated with Claude Code