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.

Author: johanzander

.claude/skills/implement-issue/SKILL.md

@@ -109,7 +109,7 @@ assume it:**
 |---|---|
 | branch or worktree exists | Step 4 |
 | commits ahead of `origin/main`, RED test in the diff | Step 5–7 |
-| open PR with `## Scope assessment` in the body | Step 9 |
+| an open PR exists for the branch | Step 9 |
 | `gh pr checks` green | Step 10 |
 | a terminal review verdict on the PR | Step 11, mid-loop |
 
@@ -122,8 +122,11 @@ the session, and Step 11 depends on holding it. It is recoverable only because
 this skill already forces it to be written down:
 
 - the Stage 2 `@claude-bot analyze` comment on the issue — the root cause
-- the PR body's `## Scope assessment` and `## Test plan` — the agreed approach
-  and what the test was supposed to discriminate
+- the PR body's `## Test plan`, and its `## Scope assessment` **if the PR came
+  from CI mode** — the agreed approach, and what the test was meant to
+  discriminate. Interactive mode keeps its scope assessment conversational, so
+  that heading is absent on most PRs this skill opens; the PR's existence is
+  what proves Step 9 was reached, not any particular heading
 - the diff itself, and any inline review comments
 
 **If those sources do not reconstruct a coherent diagnosis, STOP and report

.github/workflows/pr-review.yml

@@ -65,20 +65,29 @@ jobs:
               2. Identify the linked issue (if any) and read its analysis.
 
               3. Walk through the review checklist in `.github/claude-bot.md`.
-                 For each finding, post an INLINE comment on the specific
-                 line. Do not bundle everything in one summary comment.
 
-                 Post inline comments with `gh api`, NOT `gh pr review`:
+                 **Submit EXACTLY ONE review per run — the summary in step 4.**
+                 This is a hard constraint, not a style preference.
+                 `scripts/request-pr-review.sh` waits for your verdict, and any
+                 extra submitted review is indistinguishable by state from a
+                 real `COMMENT` verdict. That ambiguity left PR #615 approved
+                 but stuck in draft overnight, and a stray "test permission
+                 check" review was submitted to PR #622 while probing what was
+                 available.
+
+                 So: never call `gh pr review` except once, in step 4. Never
+                 call it to probe permissions — it submits.
+
+                 Prefer inline comments on the specific lines, via `gh api`:
                    gh api repos/johanzander/bess-manager/pulls/${{ github.event.issue.number }}/comments \
                      -f body='...' -f commit_id='<head sha>' -f path='<file>' -F line=<n>
 
-                 This matters for automation. `gh pr review` SUBMITS a review,
-                 so using it for inline notes puts an extra `COMMENTED` review
-                 on the PR before your summary. `scripts/request-pr-review.sh`
-                 waits for a verdict, and a stray `COMMENTED` is
-                 indistinguishable by state from a real COMMENT verdict — that
-                 ambiguity left PR #615 approved but stuck in draft. Submit
-                 exactly ONE review per run: the summary in step 4.
+                 `gh api` may be unavailable to you — it is permission-gated in
+                 this repo, and past runs could not use it. If it fails, DO NOT
+                 fall back to `gh pr review` for notes. Fold the findings into
+                 the step 4 summary instead, each with an explicit `file:line`
+                 and the real code quoted. A complete summary review is a fine
+                 outcome; a second submitted review is not.
 
               4. End with a single summary review using `gh pr review`:
                    - APPROVE  — no blockers, only nits or none

backend/tests/test_request_pr_review.py

@@ -0,0 +1,191 @@
+"""Tests for scripts/request-pr-review.sh — the Step 11 verdict wait.
+
+This file exists because the same correctness bug survived THREE review rounds.
+Verification was `quality-check.sh` plus `bash -n`, neither of which exercises
+the decision path, so each "fix" was asserted rather than demonstrated. The
+decision is: given some reviews and a workflow-run state, what does this script
+report?
+
+`gh` and `scripts/gh-agent.sh` are shimmed on PATH, so nothing here touches
+GitHub. `interval` is driven down via REVIEW_POLL_INTERVAL so a test costs
+milliseconds rather than a minute.
+"""
+
+import json
+import os
+import stat
+import subprocess
+from pathlib import Path
+
+import pytest
+
+REPO_ROOT = Path(__file__).resolve().parents[2]
+SCRIPT = REPO_ROOT / "scripts" / "request-pr-review.sh"
+
+
+def _write(path: Path, body: str) -> None:
+    path.write_text(body)
+    path.chmod(path.stat().st_mode | stat.S_IEXEC)
+
+
+@pytest.fixture
+def bin_dir(tmp_path: Path) -> Path:
+    d = tmp_path / "bin"
+    d.mkdir()
+    # The trigger comment goes through gh-agent.sh; make it a no-op.
+    (d / "scripts").mkdir(parents=True, exist_ok=True)
+    return d
+
+
+def _gh(bin_dir: Path, reviews: list, run_state: str) -> None:
+    """A `gh` answering the two queries the script makes.
+
+    `reviews` is returned for `pr view --json reviews`; `run_state` drives
+    `run list`, which is how the script decides whether the reviewer is still
+    working.
+    """
+    runs = {
+        "running": [{"status": "in_progress", "conclusion": None}],
+        "finished": [{"status": "completed", "conclusion": "success"}],
+        "failed": [{"status": "completed", "conclusion": "failure"}],
+        "none": [],
+    }[run_state]
+    # createdAt must sort after the script's `since`, which it computes at start.
+    for r in runs:
+        r["createdAt"] = "2099-01-01T00:00:00Z"
+
+    # The shim must APPLY --jq, like real gh does. An earlier version echoed the
+    # raw JSON and the script happily reported it as a verdict — the shim has to
+    # be faithful about the part under test, which here is the jq filter.
+    (bin_dir / "reviews.json").write_text(json.dumps({"reviews": reviews}))
+    (bin_dir / "runs.json").write_text(json.dumps(runs))
+
+    _write(
+        bin_dir / "gh",
+        f"""#!/bin/sh
+# Pull the --jq filter out of the argument list, then apply it to the fixture.
+filter=''
+prev=''
+for a in "$@"; do
+  if [ "$prev" = "--jq" ]; then filter="$a"; fi
+  prev="$a"
+done
+
+case "$*" in
+  *'pr view'*)  src='{bin_dir}/reviews.json' ;;
+  *'run list'*) src='{bin_dir}/runs.json' ;;
+  *'pr comment'*) exit 0 ;;
+  *) echo "unexpected gh: $*" >&2; exit 1 ;;
+esac
+
+if [ -n "$filter" ]; then
+  jq -r "$filter" < "$src"
+else
+  cat "$src"
+fi
+""",
+    )
+
+
+def _review(state: str, at: str = "2099-01-01T00:00:01Z", body: str = "x") -> dict:
+    return {"state": state, "submittedAt": at, "body": body, "author": {"login": "bot"}}
+
+
+def _run(bin_dir: Path, timeout: int = 2) -> subprocess.CompletedProcess:
+    env = dict(
+        os.environ,
+        PATH=f"{bin_dir}:{os.environ['PATH']}",
+        REVIEW_POLL_INTERVAL="1",
+    )
+    return subprocess.run(
+        ["bash", str(SCRIPT), "622", str(timeout)],
+        capture_output=True,
+        text=True,
+        env=env,
+        cwd=REPO_ROOT,
+    )
+
+
+def test_approved_returns_immediately(bin_dir: Path) -> None:
+    _gh(bin_dir, [_review("APPROVED")], "running")
+    proc = _run(bin_dir)
+    assert proc.returncode == 0
+    assert "VERDICT APPROVED" in proc.stdout
+
+
+def test_changes_requested_returns_immediately(bin_dir: Path) -> None:
+    _gh(bin_dir, [_review("CHANGES_REQUESTED")], "running")
+    proc = _run(bin_dir)
+    assert proc.returncode == 0
+    assert "VERDICT CHANGES_REQUESTED" in proc.stdout
+
+
+def test_commented_while_running_is_not_a_verdict(bin_dir: Path) -> None:
+    """The bug, three rounds running.
+
+    The bot posts an early permission-check comment and keeps working for
+    minutes. Returning that COMMENTED makes Step 11 see a non-APPROVED verdict
+    and skip `gh pr ready` — how #615 sat approved-but-draft overnight. On #622
+    the stub landed at 12:15:14 and the real CHANGES_REQUESTED at 12:16:39.
+    """
+    _gh(
+        bin_dir,
+        [_review("COMMENTED", body="test permission check - ignore")],
+        "running",
+    )
+    proc = _run(bin_dir)
+
+    assert proc.returncode == 2
+    assert "VERDICT" not in proc.stdout
+    assert "still running" in proc.stderr
+
+
+def test_commented_after_the_run_finished_is_the_verdict(bin_dir: Path) -> None:
+    """The opposite failure. `pr-review.yml` lists COMMENT as one of three final
+    verdicts, so once the run is over a COMMENTED last word IS the answer —
+    swallowing it made the script report "no summary" while findings sat on the
+    PR."""
+    _gh(bin_dir, [_review("COMMENTED")], "finished")
+    proc = _run(bin_dir)
+
+    assert proc.returncode == 0
+    assert "VERDICT COMMENTED" in proc.stdout
+
+
+def test_a_failed_run_reports_at_once_instead_of_waiting(bin_dir: Path) -> None:
+    """A dead run and a thinking one are both silence if you only poll reviews.
+    #623's run died on `Reached maximum number of turns (60)` and the wait
+    continued for 16 minutes."""
+    _gh(bin_dir, [], "failed")
+    proc = _run(bin_dir, timeout=60)
+
+    assert proc.returncode == 2
+    assert "FAILED" in proc.stderr
+    assert "not a slow one" in proc.stderr
+
+
+def test_no_run_at_all_is_reported_as_a_trigger_fault(bin_dir: Path) -> None:
+    """#619 failed this way twice: the trigger never reached the workflow, which
+    needs a different response from a stalled review."""
+    _gh(bin_dir, [], "none")
+    proc = _run(bin_dir)
+
+    assert proc.returncode == 2
+    assert "No PR Review run started at all" in proc.stderr
+    assert "actor gate" in proc.stderr
+
+
+def test_a_decisive_verdict_wins_over_an_earlier_commented(bin_dir: Path) -> None:
+    """Ordering, not recency of any state: the stub is older, the verdict newer."""
+    _gh(
+        bin_dir,
+        [
+            _review("COMMENTED", at="2099-01-01T00:00:01Z"),
+            _review("CHANGES_REQUESTED", at="2099-01-01T00:00:02Z"),
+        ],
+        "running",
+    )
+    proc = _run(bin_dir)
+
+    assert proc.returncode == 0
+    assert "VERDICT CHANGES_REQUESTED" in proc.stdout