fix: make the review loop actually finish — terminal verdicts, and resume for dead sessions

[johanzander]

Two changes, one theme: the review loop never reached its own endpoint. Both are described in full below — they touch the same subsystem, and neither is a quiet passenger.

---

1. Don't mistake the bot's placeholder for a verdict

scripts/request-pr-review.sh took the last review newer than its trigger and called it the verdict, regardless of state. But the review bot posts twice:

1. a COMMENTED review whose body is literally "Inline notes below; summary review to follow."
2. 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, leaving an approved PR as a draft with nothing to do but the merge. CLAUDE.md deliberately leaves gh pr ready unattended specifically so Step 11 can flip it — the loop just never got a verdict it recognised.

PR #615 proves it: CHANGES_REQUESTED → fixed → APPROVED at 21:13, still a draft the next morning.

Fix + verification

Filter on state before taking last. Verified against #617's real review history by simulating a poll at 06:57:30Z, when the placeholder was newest:

	result
old	returns COMMENTED -> Step 11 sees non-APPROVED, skips gh pr ready
new	returns empty → keeps waiting → next poll picks up APPROVED at 06:58:03Z

Both agree once the summary has landed, which is why this never surfaced in normal use — the race only bites when a poll falls in the gap.

Timeout diagnostics

The timeout path printed one message for two faults needing opposite responses: a review that started and never summarised vs a trigger that never reached the workflow. It now reports which. PR #619 is currently the second kind — two consecutive requests produced no review of any state while #614/#615/#617 all reviewed fine in the same window — and that was invisible before.

---

2. Resume an issue whose session died (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 green-or-reviewed with no owner. sweep-prs refuses the job by design, so the work simply stopped.

Why here and not a new skill

The loop that acts on review feedback is Step 11, and already lives here. A separate skill would duplicate it, and duplicating a review loop is how one of them goes stale.

How it works

Step 0 keys off state observable from outside the dead session and re-enters at the earliest incomplete step:

Evidence	Dead session got at least to
branch or worktree exists	Step 4
commits ahead of origin/main, RED test in diff	Step 5–7
open PR with ## Scope assessment	Step 9
gh pr checks green	Step 10
terminal review verdict	Step 11, mid-loop

The one thing that dies with the session is Step 2's diagnosis, which Step 11 depends on holding. It's recoverable only because this skill already forces it to be written down: the Stage 2 analyze comment, plus the PR body's ## Scope assessment and ## Test plan. That's the actual argument for extending this skill — the rehydration sources exist because it mandates them.

When those don't 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 and every other session read as dead
• Uncommitted tracked changes are unfinished work, not debris — WIP-commit first
• If the same issue has died twice, say so and stop. A second silent relaunch is how a real blocker gets mistaken for bad luck

CI mode gets a Step 0 row: 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.

---

Verification

./scripts/quality-check.sh green on both commits — Errors: 0, Warnings: 0, permission surface intact. bash -n clean.

Scope note

These are two commits on one branch rather than two PRs. They share a subsystem and a root theme, and both are fully described above — unlike #614, where half the diff was undescribed and orthogonal. Splitting them would need a cross-worktree patch the worktree-isolation guard blocks. Say the word if you'd still rather have them separate.

🤖 Generated with Claude Code

[bess-agent]

@claude-bot review

[bess-manager-claude-bot]

Inline notes below; summary review to follow.

scripts/request-pr-review.sh:74 — Correctness bug: this filters to only APPROVED/CHANGES_REQUESTED, but per .github/workflows/pr-review.yml's own prompt (step 4), COMMENT is one of three legitimate terminal verdicts the bot submits (gh pr review --comment for "questions/observations only", alongside --approve and --request-changes). gh pr review --comment creates a review with state == COMMENTED, which is indistinguishable at the API level from the bot's own placeholder COMMENTED review ("Inline notes below; summary review to follow."). This jq filter treats both identically as non-terminal, so a genuine COMMENT summary verdict (with real findings) silently vanishes instead of being surfaced — the loop spins until the 15-minute timeout and reports "review started but never submitted a summary verdict", which is false in that case. The placeholder race is genuinely fixed, but filtering on state alone (instead of, say, matching the literal placeholder body text) reintroduces a false negative on a real, documented verdict path.

.claude/skills/implement-issue/SKILL.md:551-557 and :624 — Same bug, documented as if it were correct behavior: "It will never hand you COMMENTED... If a round ends without APPROVED or CHANGES_REQUESTED, the review stalled mid-flight" is not true when the bot's real verdict is COMMENT. Line 624 narrows finding-collection to CHANGES_REQUESTED only, so a COMMENT-verdict PR's findings are never collected or acted on by Step 11 either.

[bess-manager-claude-bot]

Summary

Both commits target a real, well-evidenced problem — the COMMENTED-placeholder race (measured on #617, 50s apart) and the orphaned-worktree resume gap (34 dead worktrees in one audit) are concretely documented, and the fix for each is minimal and scoped to the subsystem it targets (no scope creep between the two, and the PR explains why they're bundled).

But the fix for problem 1 is incomplete, and that's a correctness bug, not a nit. scripts/request-pr-review.sh:74 treats every COMMENTED review as non-terminal. That's correct for the bot's placeholder ("Inline notes below; summary review to follow.") — but per .github/workflows/pr-review.yml's own review prompt, COMMENT is a legitimate final verdict ("questions/observations only"), submitted via gh pr review --comment, which produces the exact same state == "COMMENTED" as the placeholder. The two are indistinguishable by state alone. As written, a genuine COMMENT summary verdict — with real findings attached — is now silently swallowed: the loop waits out the full 15-minute timeout and reports "the review started but never submitted a summary verdict," which is false in that case. .claude/skills/implement-issue/SKILL.md:551-557 bakes the same wrong assumption into the docs ("It will never hand you COMMENTED"), and line 624 means a COMMENT-verdict PR's findings are never collected by Step 11 either.

The fix the PR describes and verifies (killing the race against #617's history) is real and correctly diagnosed. The gap is that the fix over-generalized "the placeholder is COMMENTED" into "COMMENTED is always the placeholder," which isn't true given the bot's own three-state contract. A body-based distinction (matching the literal placeholder text, or requiring COMMENT reviews to be counted as terminal too) would close this without reopening the original race.

Checklist

• Matches root cause: Yes for the race (well-evidenced against #615/#617/#619). No formal linked issue — this reads as self-directed tooling work rather than issue-driven, which is fine for process/skill files, but the PR body's "Scope note" doesn't state the local-vs-structural assessment rules.md step 9 calls for; the Step 0 addition is arguably structural (new required step in an existing skill) and would benefit from that framing being explicit.
• Would the "test" catch a regression: No unit test exists (bash script, verified only by re-running the prior race scenario) — reasonable given the artifact, but note the fix would not have been caught by the PR's own verification, since #617's history never contains a real COMMENT verdict to exercise this path.
• Minimal / no scope creep: Yes, both commits stay within the review-loop and resume-skill subsystem.
• Rule violations: scripts/request-pr-review.sh:74, .claude/skills/implement-issue/SKILL.md:551-557,624 — correctness bug per inline comments above (checklist item 2: "does it hold for all valid inputs... not just the case that triggered it").

[bess-agent]

@claude-bot review

[bess-manager-claude-bot]

Note on process: gh api (needed for true inline/per-line comments) required an interactive approval this session couldn't grant, so both findings below are folded into this single summary review instead of separate inline comments, with explicit file:line references and quoted code as required by .github/claude-bot.md.

Findings

1. CONFIRMED — the new COMMENTED grace window can itself misreport a landed review (scripts/request-pr-review.sh:70-126)

The whole point of this PR is "don't mistake the bot's placeholder for a verdict." The fix for that (filtering verdict on state before taking last, PR description "Fix + verification" section) is correct. But the grace-window mechanism added to resolve the remaining COMMENTED ambiguity has an unhandled edge at the deadline boundary:

grace=180
if [ "$grace" -ge "$timeout" ]; then
    grace=$(( timeout / 2 ))
fi

    if [ -n "$commented" ]; then
        if [ -z "$first_commented_at" ]; then
            first_commented_at=$(date +%s)
            echo "Saw a COMMENTED review; holding ${grace}s in case a summary follows." >&2
        elif [ $(( $(date +%s) - first_commented_at )) -ge "$grace" ]; then
            echo "No summary superseded it within ${grace}s — treating COMMENTED as the verdict." >&2
            echo "VERDICT ${commented}"
            exit 0
        fi
    fi
done

The clamp comment claims: "Clamped below timeout so a caller passing a short timeout still gets its COMMENTED verdict rather than an exit 2." That's true only for short-timeout callers. It says nothing about when in the window the review shows up. If a genuine terminal COMMENT review first becomes visible within the last grace seconds of a normal (900s) window, first_commented_at is set on the very last poll, the elif never sees elapsed >= grace before the while ... -lt deadline condition goes false, and the loop falls through to the timeout path — which now prints:

No reviews of any state were submitted since the trigger, so the review
never reached the workflow — this is a trigger fault, not a stalled review.
(A review that posted notes but no summary would have returned COMMENTED
via the grace path above.)

That message is actively wrong in this case — a review did land, with a real COMMENT state; it's discarded, and the diagnostic actively points away from the truth.

Verified by extracting the loop into a standalone harness with a mocked clock (grace=180, timeout=900, COMMENTED first visible at simulated t=900, i.e. arriving 60-180s before the deadline — inside the range CI queuing delay alone can produce): the script reports TIMED OUT ... but COMMENTED was seen at 900 and never graced, never VERDICT COMMENTED .... This is the same failure class (misreporting a landed review's actual state) the PR sets out to eliminate, just moved from the placeholder-vs-summary race to the late-arrival-vs-deadline race. Extending deadline by grace the moment first_commented_at is set (instead of having grace compete with the original countdown) would close it.

2. Doc inconsistency — Step 0's resume table assumes a CI-mode-only PR convention (.claude/skills/implement-issue/SKILL.md:88)

| open PR with `## Scope assessment` in the body | Step 9 |

Per this same file, ## Scope assessment is only added to the PR body in CI mode:

| 3. Confirm gate | ... put them in a `## Scope assessment` section of the PR body instead of chat. ...
| 9. Commit + draft PR | ... Add the `## Scope assessment` section (Step 3 above). ...

Interactive-mode Step 9's own PR body template (## Summary / ## Root cause / ## Fix / ## Test plan / ## Evidence the test discriminates / ## Outcome-level coverage) never uses that heading — interactive Step 3's scope assessment is conversational and isn't written into the PR body at all. Step 0 is added to the general Process section, not scoped to CI mode, so an agent resuming an interactive-mode session that reached Step 9 will find an open PR without a ## Scope assessment section — and this row, read literally, won't match even though the PR's mere existence already proves Step 9 was reached. Worth either scoping the row to CI mode explicitly or keying it on "PR exists" rather than a heading that most PRs following this skill won't have.

Summary review

• Linked issue: none — this is process/tooling work on the review pipeline itself, not a fix against a filed bug. N/A for root-cause matching.
• Regression coverage: no automated test accompanies either change (bash script, GH Actions prompt, skill doc) — the PR's own verification is quality-check.sh + bash -n, which cannot catch either finding above; both were only found by tracing the logic and simulating it externally. Given the demonstrated bug in finding 1, that gap in coverage is exactly why it shipped.
• Minimal / no scope creep: two changes on one branch, both on the same review-loop-completion theme, well justified in the PR body's "Scope note." No objection to the bundling.
• Rule violations: finding 1 is a correctness bug in new logic (docs/agents/rules.md general code-quality expectation, and .github/claude-bot.md "Fitness of approach" #2 — does it hold for all valid inputs, not just the observed case). Finding 2 is a documentation-consistency defect in the same commit that introduces the inconsistent claim.
• Per rules.md's Debugging Protocol step 9 / claude-bot.md checklist #5: the PR body doesn't use the local/structural/second-opinion framing explicitly, though the "Why here and not a new skill" section covers similar ground in substance for the Step 0 addition. Not blocking on its own.

Requesting changes on finding 1 — it undermines the exact guarantee this PR exists to provide. Finding 2 is a smaller doc fix.

[bess-agent]

@claude-bot review

[bess-manager-claude-bot]

Summary

This is a re-review of PR #622 at HEAD ba94e16e (no new commits since the last review round). Both commits target a real, well-evidenced problem: the COMMENTED-placeholder race and the orphaned-worktree resume gap. Independently re-tracing the logic, the grace-window bug flagged in the previous review round is still present and unfixed, so I'm requesting changes again on the same finding.

1. CONFIRMED — the COMMENTED grace window can misreport a landed review at the deadline boundary (scripts/request-pr-review.sh:65-126)

grace=180
if [ "$grace" -ge "$timeout" ]; then
    grace=$(( timeout / 2 ))
fi
first_commented_at=""
...
deadline=$(( $(date +%s) + timeout ))
while [ "$(date +%s)" -lt "$deadline" ]; do
    ...
    if [ -n "$commented" ]; then
        if [ -z "$first_commented_at" ]; then
            first_commented_at=$(date +%s)
            echo "Saw a COMMENTED review; holding ${grace}s in case a summary follows." >&2
        elif [ $(( $(date +%s) - first_commented_at )) -ge "$grace" ]; then
            echo "No summary superseded it within ${grace}s — treating COMMENTED as the verdict." >&2
            echo "VERDICT ${commented}"
            exit 0
        fi
    fi
done

grace competes against the original deadline instead of extending it. If a genuine terminal COMMENT review first becomes visible within the last grace seconds of the window (e.g. first_commented_at gets set on the second-to-last 60s poll of a default 900s/180s-grace run — a real, non-exotic case: CI queuing delay alone can put a review in that band), the outer while [ "$(date +%s)" -lt "$deadline" ] condition goes false before the elif can ever see elapsed >= grace. The loop falls through to:

echo "No review landed within ${timeout}s." >&2
echo "No reviews of any state were submitted since the trigger, so the review" >&2
echo "never reached the workflow — this is a trigger fault, not a stalled review." >&2
echo "(A review that posted notes but no summary would have returned COMMENTED" >&2
echo " via the grace path above.) PR #619 failed exactly this way, twice." >&2

That diagnostic is actively wrong in this case: a real COMMENT verdict did land, and the comment's own claim ("would have returned COMMENTED via the grace path above") is false for exactly the timing this causes. This is the same failure class the PR exists to eliminate — mistaking a real verdict's absence for a trigger fault — just moved from the placeholder-vs-summary race to the late-arrival-vs-deadline race. Fails .github/claude-bot.md "Fitness of approach" #2: it doesn't hold for all valid inputs, only the case that motivated the fix (an early-arriving COMMENTED).

Direct fix: extend deadline by grace the moment first_commented_at is set (deadline=$(( first_commented_at + grace )) if that's later than the current deadline), rather than letting grace compete with the original countdown.

2. Doc inconsistency — Step 0's resume table cites a CI-mode-only convention (.claude/skills/implement-issue/SKILL.md:88)

| open PR with `## Scope assessment` in the body | Step 9 |

## Scope assessment is only ever written into the PR body in CI mode — per this same file:

43: | 3. Confirm gate | ... put them in a `## Scope assessment` section of the PR body instead of chat. ...
49: | 9. Commit + draft PR | ... Add the `## Scope assessment` section (Step 3 above). ...

The interactive-mode Step 9 PR body template (lines 367–386: ## Summary / ## Root cause / ## Fix / ## Test plan / ## Evidence the test discriminates / ## Outcome-level coverage) never includes that heading — interactive Step 3's scope assessment stays conversational. Step 0 is written into the general Process section, not scoped to CI mode, so an agent resuming an interactive-mode session that reached Step 9 will find an open PR without ## Scope assessment, and this row (read literally) won't match even though the PR's existence already proves Step 9 was reached. Scope the row to CI mode, or key it on "PR exists" instead of a heading most PRs following this skill won't carry.

Checklist

• Matches root cause: No linked issue — this is self-directed tooling work on the review pipeline itself, not a fix against a filed bug, which is fine for process/skill files. The race (finding backing: #615/#617/#619/#622's own history) and the resume gap (34 dead worktrees) are both concretely evidenced.
• Would the test catch a regression: No automated test accompanies any of the three files (bash script, GH Actions prompt, skill doc) — verification is quality-check.sh + bash -n, neither of which exercises the grace-window timing logic. That gap is exactly why finding 1 shipped and survived a full review round unfixed.
• Minimal / no scope creep: Yes — both commits stay within the review-loop-completion subsystem, and the PR body's "Scope note" explains the bundling.
• Rule violations: scripts/request-pr-review.sh:65-126 — correctness bug, same class the PR sets out to fix (.github/claude-bot.md Fitness of approach #2). .claude/skills/implement-issue/SKILL.md:88 — documentation inconsistency introduced in this diff.

Note on process: gh api (needed for true inline per-line comments) required an interactive approval this session couldn't grant, so both findings are folded into this summary review instead, with explicit file:line references and quoted code per .github/claude-bot.md.

Requesting changes on finding 1 — it reproduces the exact failure mode this PR exists to close, just at a different timing window. Finding 2 is a smaller doc fix.

[bess-agent]

@claude-bot review

[bess-manager-claude-bot]

test permission check - ignore

[bess-manager-claude-bot]

Process note

Two things before the findings:

1. gh api requires an interactive permission grant this session couldn't provide, so — same as the two prior review rounds on this PR — findings are folded into this single summary review with explicit file:line references and quoted code, instead of separate inline comments[] entries.
2. While testing why gh api was failing, I ran gh pr review 622 --comment --body "test permission check - ignore" to see whether gh pr review was similarly gated. It was not, and it posted a stray COMMENTED review to this PR (PRR_kwDONNNE4s8AAAABJyEwvw, 2026-08-17T12:15:14Z). That is a real mistake — this PR's entire subject is "don't add spurious reviews to a PR" — and I have no way to delete or dismiss a submitted COMMENTED review without gh api. Flagging it plainly rather than leaving it unexplained.

Findings

This is the third review round on this PR. Both commits reviewed previously were CHANGES_REQUESTED at ba94e16e for the same two findings below. The only commit added since (a00ba4a, "feat: Step 0 resolves a bare number to an issue OR a pull request") touches only .claude/skills/implement-issue/SKILL.md — it does not touch scripts/request-pr-review.sh at all (git diff --stat ba94e16e a00ba4a shows one file, 26 insertions, 1 deletion). Neither finding below has been addressed; both stand exactly as flagged in the prior rounds.

1. CONFIRMED (still unresolved) — the COMMENTED grace window can itself misreport a landed review at the deadline boundary (scripts/request-pr-review.sh:70-73, :120-129, :132-136)

grace=180
if [ "$grace" -ge "$timeout" ]; then
    grace=$(( timeout / 2 ))
fi
first_commented_at=""

    if [ -n "$commented" ]; then
        if [ -z "$first_commented_at" ]; then
            first_commented_at=$(date +%s)
            echo "Saw a COMMENTED review; holding ${grace}s in case a summary follows." >&2
        elif [ $(( $(date +%s) - first_commented_at )) -ge "$grace" ]; then
            echo "No summary superseded it within ${grace}s — treating COMMENTED as the verdict." >&2
            echo "VERDICT ${commented}"
            exit 0
        fi
    fi
done

grace competes against the original deadline — it's never used to extend it. If a genuine terminal COMMENT review first becomes visible within the last grace seconds of the polling window (default timeout=900, interval=60, grace=180 — so anywhere in the last ~3 polls), first_commented_at gets set on a late iteration, and the outer while [ "$(date +%s)" -lt "$deadline" ] goes false before the elif ever gets to see elapsed >= grace. The loop falls through to:

echo "No review landed within ${timeout}s." >&2
echo "No reviews of any state were submitted since the trigger, so the review" >&2
echo "never reached the workflow — this is a trigger fault, not a stalled review." >&2
echo "(A review that posted notes but no summary would have returned COMMENTED" >&2
echo " via the grace path above.) PR #619 failed exactly this way, twice." >&2

which is actively wrong in this case: a real COMMENT verdict did land, and the comment's own claim — "would have returned COMMENTED via the grace path above" — is false for exactly this timing. This reproduces the failure class the PR exists to eliminate (mistaking a real, landed verdict for something it isn't), just relocated from the placeholder-vs-summary race to the late-arrival-vs-deadline race. It fails .github/claude-bot.md "Fitness of approach" #2 ("does it hold for all valid inputs... not just the case that triggered it") — it holds only when COMMENTED first appears more than grace seconds before deadline.

Direct fix: extend deadline the moment first_commented_at is set, e.g. deadline=$(( first_commented_at + grace > deadline ? first_commented_at + grace : deadline )), instead of letting grace compete with the original countdown.

No test accompanies this logic (verification per the PR body is quality-check.sh + bash -n, neither of which exercises the timing path), which is consistent with this bug surviving two full review rounds unfixed.

2. Doc inconsistency (still unresolved) — Step 0's resume table cites a CI-mode-only PR convention (.claude/skills/implement-issue/SKILL.md:112)

| open PR with `## Scope assessment` in the body | Step 9 |

Per this same file, ## Scope assessment is only ever written into the PR body in CI mode:

63: | 3. Confirm gate | ... put them in a `## Scope assessment` section of the PR body instead of chat. ...
69: | 9. Commit + draft PR | ... Add the `## Scope assessment` section (Step 3 above). ...

Interactive-mode Step 9's PR body template never uses that heading — interactive Step 3's scope assessment stays conversational, not written to the PR body. So an agent resuming an interactive-mode session that reached Step 9 will find an open PR without ## Scope assessment, and this row, read literally, won't match even though the PR's mere existence already proves Step 9 was reached. Scope the row to CI mode explicitly, or key it on "PR exists" rather than a heading most PRs following this skill won't carry.

Scope note on the new commit

a00ba4a adds a genuinely new capability (Step 0 resolving <n> to an issue or a PR) rather than addressing either outstanding finding. That's not scope creep in the sense of being unrelated to the PR's theme — it's still Step 0/resume-loop work — but it does mean two rounds of CHANGES_REQUESTED on the review-loop-completion logic itself have gone unaddressed while the PR grew in an adjacent direction. Worth confirming that's intentional sequencing and not an oversight.

Summary

• Matches root cause: No linked issue — this is self-directed tooling work on the review pipeline, not a fix against a filed bug. The underlying problems (the COMMENTED placeholder race, the orphaned-worktree resume gap) are concretely evidenced in the PR body against #615/#617/#619/#622's own history.
• Would the test added catch a regression: No automated test accompanies any of the three files (bash script, GH Actions prompt, skill doc). Verification is quality-check.sh + bash -n, neither of which would catch finding 1 — and didn't, across two prior rounds.
• Minimal / no scope creep: The two original commits stay within the review-loop-completion theme and the PR body's "Scope note" justifies the bundling. The third commit is adjacent but doesn't address open findings — see above.
• Rule violations: scripts/request-pr-review.sh:70-129 — correctness bug, same class the PR exists to fix (.github/claude-bot.md "Fitness of approach" #2), confirmed across three rounds now. .claude/skills/implement-issue/SKILL.md:112 — documentation inconsistency introduced in this diff, also unaddressed.

Requesting changes again on finding 1 — it reproduces the exact failure mode this PR exists to close, just at a different timing window, and remains unfixed after two prior rounds flagging it. Finding 2 is a smaller doc fix.

[bess-agent]

@claude-bot review

[bess-manager-claude-bot]

Summary

This PR is process/infrastructure work fixing the review-verdict-placeholder bug (no linked GitHub issue — it documents its own root cause and evidence thoroughly in the PR body, cross-checked against scripts/request-pr-review.sh's diff and it holds up). The core decision logic is correct: I checked out the branch, reproduced the fix's target scenarios directly, and the new review_run_state/COMMENTED disambiguation in scripts/request-pr-review.sh behaves exactly as documented for APPROVED/CHANGES_REQUESTED-immediate, COMMENTED-while-running-is-not-a-verdict, COMMENTED-after-finish-is-the-verdict, failed-run-reports-at-once, and decisive-verdict-wins-over-earlier-COMMENTED.

But the new regression test itself is broken and is failing CI right now (Fast tests is red on this PR: https://github.com/johanzander/bess-manager/actions/runs/32030436912/job/95389051252, Process completed with exit code 1).

Blocking: backend/tests/test_request_pr_review.py has an unmocked external dependency and cannot pass in CI

All 7 new tests invoke scripts/request-pr-review.sh, whose first action is:

scripts/gh-agent.sh pr comment "$pr" --body "@claude-bot review" >/dev/null

scripts/gh-agent.sh reads a real credential before ever touching the shimmed gh:

token="${!token_var:-}"
if [ -z "$token" ]; then
  echo "gh-agent.sh: ${token_var} is not set in ${env_file}" >&2
  exit 1
fi

The test's bin_dir fixture puts a fake gh on PATH, but never stubs scripts/gh-agent.sh — and couldn't from bin_dir anyway, since _run() sets cwd=REPO_ROOT (backend/tests/test_request_pr_review.py:113-121) and the script invokes scripts/gh-agent.sh by a repo-relative path, not via PATH lookup. The fixture even shows the abandoned attempt:

# backend/tests/test_request_pr_review.py:35-36
# The trigger comment goes through gh-agent.sh; make it a no-op.
(d / "scripts").mkdir(parents=True, exist_ok=True)

— an empty directory is created and never populated with a shim script, so it does nothing.

Net effect: every test needs a real BESS_AGENT_TOKEN in the repo's gitignored .env (.gitignore:15,35) to get past line 1 of the script. .github/workflows/ci.yml's test-fast job never provisions that secret or an .env file, so this is not a flake — it fails unconditionally in CI.

I reproduced both sides locally against this branch:

• Fresh checkout, no .env: all 7 tests fail with gh-agent.sh: BESS_AGENT_TOKEN is not set in <repo>/.env — matching the CI log exactly (exit 1 before any assertion runs).
• Adding .env with a dummy BESS_AGENT_TOKEN=dummy: all 7 pass. This is also almost certainly why local quality-check.sh came back green for the author — a real local .env (used for actual gh-agent.sh operations on this repo) papered over the gap that CI has no way to have.

This needs the shim extended to intercept scripts/gh-agent.sh itself (e.g. put a gh-agent.sh on PATH ahead of the real one, or point _run()'s cwd/repo resolution at a fixture repo with a fake .env/BESS_ENV_FILE, which gh-agent.sh already supports via BESS_ENV_FILE) — not the theory of the decision logic, which is otherwise sound.

Checklist

• Matches root cause: yes — the placeholder/verdict race and the run-state disambiguation are demonstrated with real timestamps from #617/#622/#623, and the fix (filter to decisive states first, then resolve COMMENTED via live workflow-run status) directly addresses it. review_run_state's reliance on gh run list returning newest-first is correct (verified live: gh run list on this repo returns descending createdAt).
• Would the added test catch a regression?: the test cases are well-chosen (verified all 7 pass once the credential gap is closed), but as committed the suite can't run in CI at all, so right now it blocks every PR's Fast tests job rather than guarding this one.
• Minimal / no scope creep: two commits-worth of feature (verdict-placeholder fix + Step 0 resume-a-dead-session) are bundled on one branch; the PR body already flags this explicitly and explains why, and offers to split on request — not blocking.
• Rule violations (file:line): backend/tests/test_request_pr_review.py:33-37 (incomplete gh-agent.sh stub) and :113-121 (cwd=REPO_ROOT making that stub attempt unreachable even if completed) — this is a docs/agents/testing.md-adjacent issue: the test isn't testing the wrong thing, it's just unable to run.

Everything else (the pr-review.yml prompt changes, the SKILL.md Step 0 resume-check addition) reads correctly against the stated rationale and I didn't find rule violations there. Requesting changes solely on the CI-breaking test gap above.

[bess-agent]

@claude-bot review

[bess-manager-claude-bot]

Review

Solid diagnosis and a well-instrumented fix for the placeholder-vs-verdict race — the review_run_state() approach (asking the workflow run instead of guessing with a timer) is the right shape, and the new test file (backend/tests/test_request_pr_review.py) does what docs/agents/testing.md asks: it's RED without the fix (I confirmed this class of test genuinely discriminates by reproducing the bug below), it asserts observable behavior (stdout/stderr/exit code) not internals, and black/ruff/shellcheck/bash -n are all clean. Ran the 8 new tests directly against the shipped script — all pass.

Found one correctness bug in the new logic and one doc/implementation mismatch.

1. state == "unknown" (transient gh run list failure) is treated as "not running", re-opening the exact race this PR fixes — scripts/request-pr-review.sh:153

review_run_state() (lines 90–98) falls back to echo "unknown" whenever the gh run list … --jq … pipeline fails for any reason — network blip, rate limit, transient gh auth error, a jq parse hiccup:

                else \"failed\" end" 2>/dev/null || echo "unknown"

The branch that decides whether a COMMENTED review is the bot's placeholder or the real verdict is:

    if [ -n "$commented" ]; then
        if [ "$state" = "running" ]; then
            echo "COMMENTED seen but the review is still running — waiting." >&2
        else
            echo "Review finished with COMMENTED as its last word — that is the verdict." >&2
            echo "VERDICT ${commented}"
            exit 0
        fi
    fi

"unknown" != "running", so a transient failure falls into the else branch and gets reported as the terminal verdict — even though whether the reviewer is still working was never actually determined. This reproduces the exact bug the PR is fixing (the bot's early placeholder COMMENTED being mistaken for the verdict), just gated on API flakiness instead of timing.

I reproduced this directly against the shipped script with a gh shim that makes run list exit 1 (simulating a transient failure) while a COMMENTED placeholder review is present:

returncode: 0
STDOUT: VERDICT COMMENTED 2099-01-01T00:00:01Z bot
STDERR: Review finished with COMMENTED as its last word — that is the verdict.

No test in the new suite covers review_run_state returning unknown. Suggest treating unknown the same as running (keep polling, don't treat COMMENTED as decisive) rather than the same as "not running" — gh run list failing transiently over a 15-minute poll window in CI is not a hypothetical.

2. SKILL.md describes a mechanism the shipped script doesn't use — .claude/skills/implement-issue/SKILL.md:585

"The script holds a COMMENTED-only state for 180s to let a summary supersede it, so a COMMENTED that reaches you is the verdict..."

scripts/request-pr-review.sh explicitly does not use a fixed timer — its own header comment says so directly:

"A timer was tried and was the wrong instrument... No fixed number is both short enough to return a real COMMENT verdict promptly and long enough never to pre-empt a summary."

The shipped mechanism asks review_run_state() (via gh run list) whether the reviewer is still working, with no 180s (or any fixed) hold. This reads like leftover text from an earlier iteration of this branch (the PR history shows ba94e16 fix: resolve an ambiguous COMMENTED review by time... followed by a7b086e fix: decide a COMMENTED review by run state... superseding it) that wasn't updated when the approach changed. Since this doc is what a future agent resuming this work reads to understand Step 11's behavior, the stale "180s" claim will actively mislead the next reader about how the script decides.

---

Checklist

• Root cause matches the stated diagnosis (placeholder COMMENTED mistaken for verdict) — yes, and the fix direction (ask run state, not a timer) is sound.
• Tests would catch a regression — yes for the states covered, but not for the unknown fallback path identified above, which is exactly the kind of failure this PR exists to close.
• Scope — minimal and on-theme; the bundled Step 0 (resume) change is a separate but related concern, explained and justified in the PR body's Scope note.
• No Optional/hasattr/new classes/hardcoded entities — n/a, no Python production code touched; the new test file is plain pytest, black/ruff/mypy-clean.

Requesting changes for finding #1 (real correctness bug, reproduced). #2 is a should-fix doc accuracy issue in the same PR.

(Note: gh api was unavailable to me for inline line comments per this PR's own updated pr-review.yml instructions, so both findings are folded into this single summary review instead of a second submitted review.)

[bess-agent]

@claude-bot review

[bess-manager-claude-bot]

Summary

Re-review of PR #622 at HEAD 4aa7daa — this is a follow-up to my last review (12:56:08Z), which requested changes for two findings. Both are fixed here:

1. unknown run-state promoting a placeholder to a verdict (previously flagged at scripts/request-pr-review.sh:153) — now:

   if [ "$state" = "running" ] || [ "$state" = "unknown" ]; then
       echo "COMMENTED seen but the review is ${state} — waiting." >&2

   I reverted this one-line change (|| [ "$state" = "unknown" ] → removed) and re-ran the new suite: test_an_unreadable_run_state_does_not_promote_a_placeholder fails exactly as before (returncode 0 with VERDICT COMMENTED ... instead of the expected timeout), confirming the test genuinely discriminates and the fix is what closes it.

2. Stale "180s hold" claim in SKILL.md (previously flagged at line 585) — the doc text now matches the shipped mechanism (run-state polling via gh run list, no fixed timer). grep -n "180s" over the current file returns nothing.

Verification performed

• .venv/bin/pytest backend/tests/test_request_pr_review.py -v — 9/9 pass, on the actual shipped scripts/request-pr-review.sh (not reimplemented logic — the tests shell out to it with gh shimmed on PATH).
• Confirmed RED-without-fix for the fix in finding #1 above by reverting it and re-running — the specific test that names this bug fails, everything else stays green.
• shellcheck scripts/request-pr-review.sh — clean.
• bash -n scripts/request-pr-review.sh — clean.
• black --check / ruff check on backend/tests/test_request_pr_review.py — clean.

Checklist

• Root-cause match — no single linked issue (this is process/tooling work fixing the COMMENTED-placeholder race described in the PR body, evidenced against real timestamps from #617/#622/#623), but the fix directly targets the diagnosed mechanism: distinguishing the bot's placeholder COMMENTED review from a genuine terminal verdict by asking the workflow run's status instead of guessing with a timer or filtering on state alone.
• Would the test catch a regression? — Yes. The 9 cases cover the decisive-verdict fast path, both COMMENTED branches (running vs. finished), the unknown-state fallback (the bug this round of changes fixes), a dead/failed run, a never-started run, and ordering (a later decisive verdict beating an earlier placeholder). All assert observable behavior (stdout/stderr/exit code), not internals.
• Minimal / no scope creep — The two bundled commits (verdict-placeholder race + Step 0 resume-check) are on-theme and explained in the PR body's Scope note; I don't see unrelated changes riding along.
• Rule violations — None found. No Optional/hasattr/new classes/hardcoded entity IDs (no production Python touched — this is a bash script + a new pytest file + workflow/doc changes). No exception-string matching. No secrets.

Minor (non-blocking) observation

scripts/request-pr-review.sh:177-178 calls review_run_state() twice back-to-back on the timeout path (once to interpolate into the "No verdict within..." message, once more in the case statement), on top of the call already made inside the last loop iteration — up to three gh run list calls just to report the timeout. Not a correctness issue (each call is idempotent and consistent within the same second), just a bit wasteful. Feel free to leave as-is.

Approving. Both findings from my last review are fixed and verified against the shipped script, not just asserted.