fix: scope the review-run check to its own PR, and survive a transient 503 (#639)

Two failures in `request-pr-review.sh`, both observed live, both of which cost
a paid review round.

WRONG RUN. `review_run_state()` took the newest `PR Review` run created after
the trigger, with no filter for which PR it belonged to. `pr-review.yml`
triggers on `issue_comment`, which fires for comments on ISSUES too, and those
runs are gated out and complete as `skipped` in about ten seconds. So any
comment posted anywhere in the repo during a review produced a newer run that
was `completed` and not `success` -- read as `failed`.

Measured on #636: a routine PO comment on issue #441 at 21:13:07 made the
script abandon the review, which went on to APPROVE at 21:15:45. The caller was
told the run was broken while it was still thinking, and the natural response
-- re-request -- would have spent a second review round on a review already in
flight.

`displayTitle` carries the PR title for a run triggered on that PR, so it is
the discriminator. The title is read once before the loop and matched via
`--arg`, not string interpolation: a title containing a quote would otherwise
break the filter silently.

TRANSIENT FAILURE. `set -e` turned a single 503 on the verdict read into a
fatal exit 1 -- AFTER the trigger comment had posted, so a re-run again spent a
second round on a live review. GitHub returned 503s for roughly ninety minutes
on 2026-08-17 and this fired twice. Both review reads now tolerate a failed
call and retry on the next poll, which is what `review_run_state` already did
via its `unknown` state.

Both bug tests were verified to fail against the pre-fix script -- the second
reproducing the exact exit 1 with the 503 on stderr -- while
`test_this_prs_own_failed_run_is_still_reported` passes both before and after,
which is what proves the scoping does not swallow a genuine failure.

No CHANGELOG entry: agent tooling, no user-visible effect.


Claude-Session: https://claude.ai/code/session_012LExo6fcbup75vtc9NfoAR

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: johanzander

backend/tests/test_request_pr_review.py

@@ -57,7 +57,16 @@ def env_file(tmp_path: Path) -> Path:
     return p
 
 
-def _gh(bin_dir: Path, reviews: list, run_state: str) -> None:
+PR_TITLE = "fix: the pr under review"
+
+
+def _gh(
+    bin_dir: Path,
+    reviews: list,
+    run_state: str,
+    extra_runs: list | None = None,
+    title: str = PR_TITLE,
+) -> None:
     """A `gh` answering the two queries the script makes.
 
     `reviews` is returned for `pr view --json reviews`; `run_state` drives
@@ -78,14 +87,24 @@ def _gh(bin_dir: Path, reviews: list, run_state: str) -> None:
         }[run_state]
     )
     # createdAt must sort after the script's `since`, which it computes at start.
+    # displayTitle is what scopes a run to THIS PR — a run carrying any other
+    # title belongs to a different PR (or to an issue comment, which spawns a
+    # gated-out `skipped` run) and must be ignored.
     for r in runs:
         r["createdAt"] = "2099-01-01T00:00:00Z"
+        r.setdefault("displayTitle", title)
+
+    # `extra_runs` are NEWER runs belonging to something else. Real gh returns
+    # newest-first, so they go in front — which is exactly how they used to be
+    # mistaken for this PR's run.
+    runs = (extra_runs or []) + runs
 
     # 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))
+    (bin_dir / "title.json").write_text(json.dumps({"title": title}))
 
     _write(
         bin_dir / "gh",
@@ -99,6 +118,7 @@ def _gh(bin_dir: Path, reviews: list, run_state: str) -> None:
 done
 
 case "$*" in
+  *'pr view'*'--json title'*) src='{bin_dir}/title.json' ;;
   *'pr view'*)  src='{bin_dir}/reviews.json' ;;
   *'run list'*)
       if [ "{int(runs_fail)}" = "1" ]; then
@@ -286,3 +306,96 @@ def test_a_decisive_verdict_wins_over_an_earlier_commented(
 
     assert proc.returncode == 0
     assert "VERDICT CHANGES_REQUESTED" in proc.stdout
+
+
+def _foreign_run(title: str, conclusion: str = "skipped") -> dict:
+    """A newer `PR Review` run belonging to something else.
+
+    `pr-review.yml` triggers on `issue_comment`, which fires for comments on
+    ISSUES as well as PRs. Those runs are gated out and complete as `skipped`
+    within about ten seconds, so any comment posted anywhere in the repo while a
+    review is running produces one of these.
+    """
+    return {
+        "status": "completed",
+        "conclusion": conclusion,
+        "createdAt": "2099-01-01T00:00:30Z",
+        "displayTitle": title,
+    }
+
+
+def test_a_newer_run_for_a_different_pr_is_not_this_review(
+    bin_dir: Path, env_file: Path
+) -> None:
+    """The bug that cost a real review round. A routine PO comment on issue
+    #441 spawned a gated-out `skipped` run, which was newer than #636's and so
+    was read as "this review failed" — while #636 went on to APPROVE two
+    minutes later. The caller abandoned a review that was still thinking.
+    """
+    _gh(
+        bin_dir,
+        [],
+        "running",
+        extra_runs=[_foreign_run("Question: Is it always counting with 15 minutes?")],
+    )
+    proc = _run(bin_dir, env_file, timeout=2)
+
+    # Times out waiting, which is correct: the review is still running.
+    assert proc.returncode == 2
+    assert "FAILED without submitting a verdict" not in proc.stderr
+
+
+def test_this_prs_own_failed_run_is_still_reported(
+    bin_dir: Path, env_file: Path
+) -> None:
+    """The scoping must not swallow a genuine failure — that is the other half
+    of the same rule, and the reason a fixed grace window was replaced by
+    asking the run in the first place."""
+    _gh(
+        bin_dir,
+        [],
+        "failed",
+        extra_runs=[_foreign_run("some unrelated issue")],
+    )
+    proc = _run(bin_dir, env_file, timeout=2)
+
+    assert proc.returncode == 2
+    assert "FAILED without submitting a verdict" in proc.stderr
+
+
+def test_a_transient_api_failure_does_not_kill_the_poll(
+    bin_dir: Path, env_file: Path
+) -> None:
+    """`set -e` used to turn one 503 on the verdict read into a fatal exit 1,
+    AFTER the trigger comment had posted — so re-running spent a second paid
+    review round on a review already in flight. GitHub returned 503s for about
+    ninety minutes on 2026-08-17 and this fired twice.
+
+    The shim fails `pr view --json reviews` once, then serves normally.
+    """
+    _gh(bin_dir, [_review("APPROVED")], "running")
+
+    # Wrap the shim: first reviews read fails, subsequent ones succeed.
+    gh = bin_dir / "gh"
+    real = gh.read_text()
+    (bin_dir / "gh-real").write_text(real)
+    (bin_dir / "gh-real").chmod(0o755)
+    _write(
+        gh,
+        f"""#!/bin/sh
+case "$*" in
+  *'pr view'*'--json reviews'*)
+      if [ ! -f {bin_dir}/tripped ]; then
+          touch {bin_dir}/tripped
+          echo "HTTP 503: no server is currently available" >&2
+          exit 1
+      fi ;;
+esac
+exec {bin_dir}/gh-real "$@"
+""",
+    )
+
+    proc = _run(bin_dir, env_file, timeout=6)
+
+    assert proc.returncode == 0, proc.stderr
+    assert "VERDICT APPROVED" in proc.stdout

scripts/request-pr-review.sh

@@ -87,19 +87,48 @@ interval="${REVIEW_POLL_INTERVAL:-60}"
 # reviews, so a crashed review burned the full timeout. On #623 that cost 16
 # minutes of waiting on a run that had already failed with "Reached maximum
 # number of turns (60)".
+#
+# THE RUN MUST BE THIS PR'S. Selecting the newest run by time alone was wrong,
+# and wrong in the direction that costs a review round: `pr-review.yml` triggers
+# on `issue_comment`, which fires for comments on ISSUES too, not only PRs.
+# Those runs are gated out and complete as `skipped` within about ten seconds.
+#
+# So any comment posted anywhere in the repo while a review is running produces
+# a newer `PR Review` run that is `completed` and not `success` -- which this
+# function read as `failed`. Measured on #636: a routine PO comment on issue
+# #441 at 21:13:07 made the script abandon a review that went on to APPROVE at
+# 21:15:45. The caller was told the run was broken while it was still thinking.
+#
+# `displayTitle` carries the PR title for a run triggered on that PR, so it is
+# the discriminator. The title is fetched once, before the loop, and matched
+# through `--arg` rather than string-interpolated -- a title containing a quote
+# would otherwise break the filter.
 review_run_state() {
     gh run list --workflow "PR Review" --limit 20 \
-        --json status,conclusion,createdAt \
-        --jq "[ .[] | select(.createdAt > \"${since}\") ] | first
-              | if . == null then \"none\"
-                elif .status != \"completed\" then \"running\"
-                elif .conclusion == \"success\" then \"finished\"
-                else \"failed\" end" 2>/dev/null || echo "unknown"
+        --json status,conclusion,createdAt,displayTitle 2>/dev/null \
+      | jq -r --arg since "$since" --arg title "$pr_title" '
+            [ .[]
+              | select(.createdAt > $since)
+              | select(.displayTitle == $title) ] | first
+            | if . == null then "none"
+              elif .status != "completed" then "running"
+              elif .conclusion == "success" then "finished"
+              else "failed" end' 2>/dev/null || echo "unknown"
 }
 
 repo_root=$(git rev-parse --show-toplevel)
 cd "$repo_root"
 
+# The PR title, used to tell THIS PR's review run from any other `PR Review`
+# run that happens to be newer. Fetched before `since` so a slow call cannot
+# push the window past a review that lands immediately.
+pr_title=$(gh pr view "$pr" --json title --jq .title)
+if [ -z "$pr_title" ]; then
+    echo "Could not read PR #${pr} title; refusing to poll without a way to" >&2
+    echo "tell its review run from anyone elses." >&2
+    exit 2
+fi
+
 # Reviews strictly newer than this are the ones this run triggered.
 since=$(date -u +%Y-%m-%dT%H:%M:%SZ)
 
@@ -118,12 +147,19 @@ while [ "$(date +%s)" -lt "$deadline" ]; do
     fi
 
     # A decisive verdict wins immediately, whenever it appears.
+    #
+    # A FAILED READ IS NOT A RESULT, and `set -e` used to turn one into a fatal
+    # error: a single transient 503 on this call killed the script with exit 1
+    # AFTER the trigger comment had already posted, so re-running it spent a
+    # second paid review round on a review already in flight. GitHub returned
+    # 503s for roughly ninety minutes on 2026-08-17 and this fired twice.
+    # Swallowing the failure costs one wasted poll; the next iteration retries.
     verdict=$(gh pr view "$pr" --json reviews \
         --jq "[.reviews[]
                | select(.submittedAt > \"${since}\")
                | select(.state == \"APPROVED\" or .state == \"CHANGES_REQUESTED\")]
               | last | select(. != null)
-              | \"\(.state) \(.submittedAt) \(.author.login)\"")
+              | \"\(.state) \(.submittedAt) \(.author.login)\"" 2>/dev/null) || verdict=""
 
     if [ -n "$verdict" ]; then
         echo "VERDICT ${verdict}"
@@ -142,12 +178,13 @@ while [ "$(date +%s)" -lt "$deadline" ]; do
         exit 2
     fi
 
+    # Same transient-failure tolerance as the verdict read above.
     commented=$(gh pr view "$pr" --json reviews \
         --jq "[.reviews[]
                | select(.submittedAt > \"${since}\")
                | select(.state == \"COMMENTED\")]
               | last | select(. != null)
-              | \"\(.state) \(.submittedAt) \(.author.login)\"")
+              | \"\(.state) \(.submittedAt) \(.author.login)\"" 2>/dev/null) || commented=""
 
     if [ -n "$commented" ]; then
         if [ "$state" = "running" ] || [ "$state" = "unknown" ]; then