diff --git a/.claude/hooks/check-worktree-path.sh b/.claude/hooks/check-worktree-path.sh index f73700af..26c9d607 100755 --- a/.claude/hooks/check-worktree-path.sh +++ b/.claude/hooks/check-worktree-path.sh @@ -53,4 +53,45 @@ if [ "$target_root" != "$session_root" ]; then exit 0 fi +# --------------------------------------------------------------------------- +# Second check: is this session in a worktree at all? +# +# CLAUDE.md's rule is unconditional -- "Never edit any file on main, even a +# one-line doc fix" -- and it is prose, so it has to be REMEMBERED at the moment +# of the first edit. That is precisely when it isn't: a session that opens as a +# question ("why does X happen?") and drifts into implementing never re-evaluates +# a rule it had no reason to consider at the start. Six live sessions currently +# sit in the main checkout for exactly that legitimate read-only reason. +# +# The cost is not hypothetical. PR #619's branch carries a merge of itself -- +# `Merge remote-tracking branch 'origin/fix/...' into fix/...` -- because two +# writers worked the same branch from different bases and diverged for fifteen +# hours. The reviewer reviewed one line three times while the other line, based +# on a commit from 08:09, never saw a single verdict. +# +# So the check is mechanical and fires at the transition itself: the first +# Edit/Write IS the moment a question becomes an implementation. A linked +# worktree has its own git dir under the main one, so `--git-dir` and +# `--git-common-dir` differ there and are identical in the main checkout. That +# is a path comparison, the only shape docs/agents/rules.md sanctions for this +# hook -- it never has to guess what a command string will touch. +# +# Sibling checkouts (`../bess-manager-feature/`) are linked worktrees too, so +# they pass: this enforces "work in a worktree", not "work under .claude/". +git_dir=$(git rev-parse --absolute-git-dir 2>/dev/null || true) +common_dir=$(cd "$(git rev-parse --git-common-dir 2>/dev/null || echo .)" 2>/dev/null && pwd || true) + +if [ -n "$git_dir" ] && [ -n "$common_dir" ] && [ "$git_dir" = "$common_dir" ]; then + branch=$(git rev-parse --abbrev-ref HEAD 2>/dev/null || echo "?") + reason="BLOCKED: this session is in the MAIN checkout ('${session_root}', branch '${branch}'), not a worktree, so editing '${target_path}' would write straight into the shared checkout. CLAUDE.md: work in a worktree before ANY edit -- unconditional, including a one-line doc fix. If this session started as a question and has become implementation work, that is the common path here and this is the moment to switch: call EnterWorktree (never \`git worktree add\`, which the sandbox denies), then redo the edit there. The main checkout stays read-only: questions, \`gh\`, the backlog, and dispatch all work fine from it." + jq -n --arg reason "$reason" '{ + hookSpecificOutput: { + hookEventName: "PreToolUse", + permissionDecision: "deny", + permissionDecisionReason: $reason + } + }' + exit 0 +fi + echo '{"continue": true}' diff --git a/.claude/skills/implement-issue/SKILL.md b/.claude/skills/implement-issue/SKILL.md index 0713113e..cb67e728 100644 --- a/.claude/skills/implement-issue/SKILL.md +++ b/.claude/skills/implement-issue/SKILL.md @@ -121,6 +121,30 @@ Re-enter at the **earliest incomplete** step and run forward normally. A PR carrying `CHANGES_REQUESTED` re-enters at Step 11's `CHANGES_REQUESTED` branch; one carrying `APPROVED` needs only `gh pr ready`. +**A verdict alone does not say how far Step 11 got — compare it against the +last push.** The loop is request → verdict → fix → push → request, so the same +`CHANGES_REQUESTED` means two different things depending on which side of it +HEAD sits: + +| Newest verdict vs newest commit | The dead session had | +|---|---| +| verdict **newer** than last commit | received the findings and not yet acted on them — resume by fixing them | +| last commit **newer** than verdict | already fixed and pushed — resume by requesting the next round | + +```bash +gh pr view --json reviews,commits --jq \ + '{verdict: ([.reviews[] | select(.state=="APPROVED" or .state=="CHANGES_REQUESTED")] + | sort_by(.submittedAt) | last | .submittedAt), + pushed: ([.commits[].committedDate] | sort | last)}' +``` + +Getting this backwards is what happened on #619: four `@claude-bot review` +comments, two paid verdicts, and one diff that never changed between them, +because "have I acted on this yet" was held in a session that had died. +`request-pr-review.sh` now refuses the illegal round rather than trusting the +caller to have checked, but read it here too — the answer also tells you *what +to do*, which the script cannot. + **Rehydrate the diagnosis before touching code.** Step 2's analysis died with the session, and Step 11 depends on holding it. It is recoverable only because this skill already forces it to be written down: @@ -609,6 +633,30 @@ verdict lands, printing `VERDICT ` (exit 2 on a 15-minute timeout, after dumping recent `PR Review` runs). +**Exit 1 means the round was illegal and no review was requested — read the +message, do not retry.** The script checks, before posting anything, whether +asking can possibly help: + +| Refusal | What it means | The actual next move | +|---|---|---| +| unconsumed `CHANGES_REQUESTED` | the newest verdict is newer than the last push, so it describes the diff as it stands | address the findings and push; then the next round is legal | +| already `APPROVED` | approved on the current diff, nothing pushed since | re-check mergeability, then `gh pr ready` | +| 3 decisive rounds | the cap below, enforced rather than remembered | hand the outstanding findings to the user verbatim | +| gate unreadable | `gh` failed, so legality was never established | re-run; it costs nothing, a needless round costs a paid review | + +This exists because **the cap and "have I acted on the last verdict" are the +two pieces of loop state a dead session takes with it**, and asking again is +the one move a confused loop can always make. On #619 that produced four +requests, zero consumed verdicts, and two paid reviews of byte-identical code. +Both facts are already on the PR — decisive review count, and whether a commit +follows the newest verdict — so the script reads them instead of trusting the +caller to remember. + +`--allow-unconsumed` overrides the first row, for the one case this step +sanctions: the finding was wrong, and you have replied on the PR saying why +rather than pushing. It is a flag so that "the reviewer is mistaken" is a +decision you take deliberately, not the path a stalled loop slides into. + **`COMMENTED` is ambiguous, and the script resolves it for you — don't second-guess it.** `pr-review.yml` allows three final verdicts (`APPROVE`, `REQUEST_CHANGES`, `COMMENT`), so a real `COMMENT` verdict is possible and diff --git a/backend/tests/test_pr_state.py b/backend/tests/test_pr_state.py new file mode 100644 index 00000000..4c9a7989 --- /dev/null +++ b/backend/tests/test_pr_state.py @@ -0,0 +1,334 @@ +"""Tests for scripts/pr-state.sh — the derived fleet-state read. + +The decision under test is: given a PR's reviews, commits, mergeability and +checks, what state is it in and whose turn is it? That is the whole value of the +script, so it is what gets exercised. `gh` is shimmed on PATH; nothing here +touches GitHub. + +The classification that motivated the script is `test_619_shape`: a PR that is +BOTH conflicted and carrying an unaddressed CHANGES_REQUESTED. Reporting only +the conflict — which an earlier ordering did — hides two blocking reviews behind +a mechanical merge. +""" + +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" / "pr-state.sh" + +GREEN = [{"name": "Fast tests", "status": "COMPLETED", "conclusion": "SUCCESS"}] + + +def _requested(at: str) -> dict: + """The Stage 4 trigger comment, which is the only thing that starts a review.""" + return {"body": "@claude-bot review", "createdAt": at} + + +def _pr( + number: int = 1, + *, + reviews: list | None = None, + commits: list | None = None, + comments: list | None = None, + checks: list | None = None, + mergeable: str = "MERGEABLE", + merge_state: str = "CLEAN", + draft: bool = True, +) -> dict: + return { + "number": number, + "title": f"pr {number}", + "isDraft": draft, + "mergeable": mergeable, + "mergeStateStatus": merge_state, + "headRefName": f"branch-{number}", + "updatedAt": "2026-08-01T00:00:00Z", + "reviews": reviews or [], + "commits": commits or [{"committedDate": "2026-08-02T00:00:00Z"}], + # Default: a request AFTER the default push, so tests that are not about + # the request feed land on the reviewer rather than the dispatcher. + "comments": ( + comments if comments is not None else [_requested("2026-08-03T00:00:00Z")] + ), + "statusCheckRollup": GREEN if checks is None else checks, + } + + +def _review(state: str, at: str) -> dict: + return {"state": state, "submittedAt": at} + + +def _rows(out: str) -> str: + """Just the classification rows. + + The script's closing note names `needs-fix` and `needs-refresh` while + explaining that liveness is not guessed, so a negative assertion against the + whole of stdout matches the explanation rather than a classification. + """ + return out.split("Liveness (")[0] + + +@pytest.fixture +def run(tmp_path: Path): + """Shim `gh pr list` with a fixture and return the script's stdout.""" + outside_a_repo = tmp_path / "not-a-repo" + outside_a_repo.mkdir() + + def _run(prs: list[dict]) -> str: + bin_dir = tmp_path / "bin" + bin_dir.mkdir(exist_ok=True) + (bin_dir / "prs.json").write_text(json.dumps(prs)) + gh = bin_dir / "gh" + gh.write_text(f"""#!/bin/sh +# Faithful about the part under test: return the fixture for --json, and let the +# script's own jq do the classifying. +cat '{bin_dir}/prs.json' +""") + gh.chmod(gh.stat().st_mode | stat.S_IEXEC) + proc = subprocess.run( + ["bash", str(SCRIPT)], + capture_output=True, + text=True, + env=dict(os.environ, PATH=f"{bin_dir}:{os.environ['PATH']}"), + # NOT the repo: the script's local-writer section walks real + # worktrees and fetches, which would make every classifier test slow, + # networked, and dependent on whatever branches happen to exist. + # Outside a checkout it takes the documented skip path instead, which + # `test_the_local_writer_check_skips_loudly` pins. + cwd=outside_a_repo, + ) + assert proc.returncode == 0, proc.stderr + return proc.stdout + + return _run + + +def test_619_shape_reports_the_findings_not_just_the_conflict(run) -> None: + """PR #619 as GitHub reported it: conflicted, CI green on a head that has + not moved since 07:34, and two CHANGES_REQUESTED at 09:40 and 10:55. + + The blocking fact is the unaddressed findings — the conflict is mechanical + and belongs to `sweep-prs`. An ordering that put the conflict first called + this `needs-refresh` and lost both reviews. + """ + out = run( + [ + _pr( + 619, + reviews=[ + _review("COMMENTED", "2026-08-17T09:39:21Z"), + _review("CHANGES_REQUESTED", "2026-08-17T09:40:12Z"), + _review("CHANGES_REQUESTED", "2026-08-17T10:55:21Z"), + ], + commits=[{"committedDate": "2026-08-17T07:34:00Z"}], + mergeable="CONFLICTING", + merge_state="DIRTY", + ) + ] + ) + + assert "needs-fix" in out + assert "[executor]" in out + assert "UNCONSUMED" in out + # The conflict is still reported, as a flag rather than as the headline. + assert "+conflicted" in out + + +def test_a_push_after_the_verdict_is_not_needs_fix(run) -> None: + """The same verdict means the opposite thing once HEAD moves past it: the + findings were acted on, so it is no longer the executor's turn.""" + out = run( + [ + _pr( + 1, + reviews=[_review("CHANGES_REQUESTED", "2026-08-01T00:00:00Z")], + commits=[{"committedDate": "2026-08-02T00:00:00Z"}], + comments=[_requested("2026-08-03T00:00:00Z")], + ) + ] + ) + + assert "awaiting-review" in out + assert "[reviewer]" in out + assert "needs-fix" not in _rows(out) + + +def test_a_review_never_requested_is_the_dispatchers_turn_not_the_reviewers( + run, +) -> None: + """The Stage 4 bot only acts when triggered by an `@claude-bot review` + comment, so "green with no verdict" has two completely different meanings + and only one of them is the reviewer's. + + Measured on the live fleet: #637 and #635 had NEVER been asked, and #620, + #619, #614 and #490 had all been pushed to after their last request. All six + were being reported as `awaiting-review [reviewer]` — parked on someone who + had not been asked and was never going to act. Six of eleven open PRs. + """ + out = run([_pr(637, comments=[])]) + + assert "needs-review-request" in out + assert "[dispatcher]" in out + assert "NEVER been requested" in out + assert "[reviewer]" not in _rows(out) + + +def test_a_push_after_the_last_request_owes_a_new_round(run) -> None: + """The subtler half of the same bug. A request exists, so the feed is not + empty — but it predates the code, so the bot already returned its verdict on + a diff that no longer exists. #620 requested at 17:40 and pushed at 18:33.""" + out = run( + [ + _pr( + 620, + commits=[{"committedDate": "2026-08-17T18:33:05Z"}], + comments=[_requested("2026-08-17T17:40:53Z")], + ) + ] + ) + + assert "needs-review-request" in out + assert "[dispatcher]" in out + assert "round owed" in out + + +def test_a_request_newer_than_the_push_really_is_the_reviewers_turn(run) -> None: + """The gate must not label everything the dispatcher's job: once the request + postdates the code, waiting on the bot is genuinely the correct state.""" + out = run( + [ + _pr( + 3, + commits=[{"committedDate": "2026-08-02T00:00:00Z"}], + comments=[_requested("2026-08-04T00:00:00Z")], + ) + ] + ) + + assert "awaiting-review" in out + assert "[reviewer]" in out + assert "needs-review-request" not in _rows(out) + + +def test_approved_and_still_draft_is_the_maintainers_turn(run) -> None: + """#615 sat in this state overnight with only the merge left to do. It is + nobody's bug and nobody's review — it is a flag someone has to flip.""" + out = run( + [ + _pr( + 2, + reviews=[_review("APPROVED", "2026-08-03T00:00:00Z")], + commits=[{"committedDate": "2026-08-02T00:00:00Z"}], + ) + ] + ) + + assert "awaiting-ready" in out + assert "[maintainer]" in out + + +def test_a_commented_placeholder_is_not_a_verdict(run) -> None: + """The bot posts permission-check and inline-note reviews as COMMENTED. + Counting those as verdicts is how #615 was misread; this classifier looks + only at APPROVED/CHANGES_REQUESTED.""" + out = run( + [ + _pr( + 7, + reviews=[_review("COMMENTED", "2026-08-09T00:00:00Z")], + commits=[{"committedDate": "2026-08-02T00:00:00Z"}], + ) + ] + ) + + assert "not returned a verdict yet" in out + assert "UNCONSUMED" not in out + + +def test_a_conflict_outranks_ci_because_a_conflicted_pr_has_no_run(run) -> None: + """`sweep-prs` found two PRs sitting conflicted with nobody aware, because a + CONFLICTING PR creates no workflow run at all and so presents as "CI never + fired". An empty check list must not read as green.""" + out = run([_pr(5, mergeable="CONFLICTING", merge_state="DIRTY", checks=[])]) + + assert "needs-refresh" in out + assert "[sweep]" in out + + +def test_unknown_mergeability_is_never_reported_as_clean(run) -> None: + """`mergeable` is computed LAZILY: the first query on a cold PR returns + UNKNOWN and only then triggers the computation. + + This was measured, not theorised. A first live fleet run classified #167 and + #619 with no conflict flag; once earlier queries had warmed them, the + identical command returned `needs-refresh` for both. They were CONFLICTING + throughout. Treating UNKNOWN as "not conflicted" therefore hides exactly the + stale PRs this script exists to surface — the same trap `sweep-prs` + documents and retries for. + + The script retries while anything is UNKNOWN. This pins the fallback: if it + still is, say so rather than falling through to the clean branch. + """ + out = run([_pr(9, mergeable="UNKNOWN", merge_state="UNKNOWN")]) + + assert "UNKNOWN" in _rows(out) + assert "re-run" in _rows(out) + + +def test_red_ci_belongs_to_the_executor(run) -> None: + out = run( + [ + _pr( + 3, + checks=[ + { + "name": "Algorithm tests", + "status": "COMPLETED", + "conclusion": "FAILURE", + } + ], + ) + ] + ) + + assert "needs-fix" in out + assert "Algorithm tests" in out + + +def test_running_ci_is_nobodys_turn(run) -> None: + """Distinct from needs-fix: there is nothing to do but wait, and a state + read that says "act" here produces churn.""" + out = run( + [_pr(4, checks=[{"name": "E2E", "status": "IN_PROGRESS", "conclusion": None}])] + ) + + assert "in-flight" in out + + +def test_liveness_is_reported_as_unknown_rather_than_guessed(run) -> None: + """The whole point. A fresh session previously answered "is anyone working + on this" from `git worktree list` and `claude agents --json` — local signals + that see nothing when the executor is a container or a GitHub Action, and + that reported #619's REVIEWER timestamp as the agent's last activity. + """ + out = run([_pr(1)]) + + assert "not a GitHub fact and is not" in out + assert "guessed here" in out + + +def test_the_local_writer_check_skips_loudly_outside_a_checkout(run) -> None: + """The script is meant to run against a container fleet too, where no + worktree exists. A silent skip there would read as "no divergence found", + which is the same silent-cap failure `sweep-prs` warns about — so the skip + has to say that it proves nothing.""" + out = run([_pr(1)]) + + assert "SKIPPED" in out + assert "says nothing about writers" in out diff --git a/backend/tests/test_pr_state_divergence.py b/backend/tests/test_pr_state_divergence.py new file mode 100644 index 00000000..73f6c04b --- /dev/null +++ b/backend/tests/test_pr_state_divergence.py @@ -0,0 +1,154 @@ +"""Tests for pr-state.sh's local-writer section — the two-writers detector. + +This is the one thing GitHub cannot tell you. A branch with two writers looks +completely normal through the API: the divergence exists only between a local +checkout and the remote, and it collapses into an ordinary merge commit the +moment somebody reconciles it. + +PR #619 is why this exists. One writer took the branch at 08:09 and worked from +that base; another pushed 23031e78 at 09:34. The reviewer reviewed 23031e78 +three times, twice with blocking findings, while the first line never held that +commit at all. Fifteen hours later it landed as `Merge remote-tracking branch +'origin/fix/...' into fix/...` — a branch merged into itself. + +So the scenario below is built for real: a bare "origin", two clones that both +commit to one branch, and the assertion that the script names it. +""" + +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" / "pr-state.sh" + +GIT_ENV = { + "PATH": "/usr/bin:/bin:/usr/local/bin:/opt/homebrew/bin", + "GIT_CONFIG_GLOBAL": "/dev/null", + "GIT_CONFIG_SYSTEM": "/dev/null", + "GIT_AUTHOR_NAME": "t", + "GIT_AUTHOR_EMAIL": "t@t", + "GIT_COMMITTER_NAME": "t", + "GIT_COMMITTER_EMAIL": "t@t", +} + +BRANCH = "fix/issue-592-vpp-idle-at-floor" + + +def _git(cwd: Path, *args: str) -> str: + proc = subprocess.run( + ["git", *args], + cwd=cwd, + check=True, + capture_output=True, + text=True, + env={**GIT_ENV, "HOME": str(cwd)}, + ) + return proc.stdout.strip() + + +def _commit(repo: Path, name: str) -> None: + (repo / name).write_text(name) + _git(repo, "add", name) + _git(repo, "commit", "-q", "-m", name) + + +@pytest.fixture +def two_writers(tmp_path: Path) -> Path: + """A checkout whose branch has diverged from its own remote. + + Writer B pushes; writer A, which branched earlier, commits locally without + pulling. That is #619's shape exactly. + """ + origin = tmp_path / "origin.git" + origin.mkdir() + _git(origin, "init", "-q", "--bare", "-b", "main") + + writer_b = tmp_path / "writer-b" + _git(tmp_path, "clone", "-q", str(origin), "writer-b") + _commit(writer_b, "seed") + _git(writer_b, "push", "-q", "origin", "main") + _git(writer_b, "checkout", "-q", "-b", BRANCH) + _commit(writer_b, "from-b") + _git(writer_b, "push", "-q", "origin", BRANCH) + + writer_a = tmp_path / "writer-a" + _git(tmp_path, "clone", "-q", str(origin), "writer-a") + _git(writer_a, "checkout", "-q", "-b", BRANCH, "--no-track", "origin/main") + _commit(writer_a, "from-a") + # A never pulled B's push, so the two lines have no common tip. + return writer_a + + +def _run(cwd: Path, tmp_path: Path, prs: list[dict]) -> str: + bin_dir = tmp_path / "bin" + bin_dir.mkdir(exist_ok=True) + (bin_dir / "prs.json").write_text(json.dumps(prs)) + gh = bin_dir / "gh" + gh.write_text(f"#!/bin/sh\ncat '{bin_dir}/prs.json'\n") + gh.chmod(gh.stat().st_mode | stat.S_IEXEC) + + proc = subprocess.run( + ["bash", str(SCRIPT)], + capture_output=True, + text=True, + cwd=cwd, + env=dict(os.environ, PATH=f"{bin_dir}:{os.environ['PATH']}"), + ) + assert proc.returncode == 0, proc.stderr + return proc.stdout + + +def _pr(number: int, branch: str) -> dict: + return { + "number": number, + "title": f"pr {number}", + "isDraft": True, + "mergeable": "MERGEABLE", + "mergeStateStatus": "CLEAN", + "headRefName": branch, + "updatedAt": "2026-08-17T00:00:00Z", + "reviews": [], + "commits": [{"committedDate": "2026-08-17T00:00:00Z"}], + "comments": [ + {"body": "@claude-bot review", "createdAt": "2026-08-18T00:00:00Z"} + ], + "statusCheckRollup": [ + {"name": "Fast tests", "status": "COMPLETED", "conclusion": "SUCCESS"} + ], + } + + +def test_two_writers_on_one_branch_are_named(two_writers: Path, tmp_path: Path) -> None: + """The #619 shape. Local has a commit the remote lacks AND the remote has a + commit local lacks — which cannot happen with a single writer.""" + out = _run(two_writers, tmp_path, [_pr(619, BRANCH)]) + + assert "DIVERGED" in out + assert "TWO WRITERS" in out + assert "#619" in out + + +def test_a_branch_in_sync_is_not_flagged(two_writers: Path, tmp_path: Path) -> None: + """Guards against a detector that shouts on every branch — which would get + it ignored, the way a CONFLICTING PR with no checks got ignored.""" + _git(two_writers, "fetch", "-q", "origin") + _git(two_writers, "reset", "-q", "--hard", f"origin/{BRANCH}") + out = _run(two_writers, tmp_path, [_pr(619, BRANCH)]) + + assert "DIVERGED" not in out + assert "in sync" in out + + +def test_a_branch_with_no_open_pr_is_not_reported( + two_writers: Path, tmp_path: Path +) -> None: + """The fleet has ~47 worktrees and most have no open PR. Listing them all + would bury the one line that matters.""" + out = _run(two_writers, tmp_path, [_pr(999, "some/other-branch")]) + + assert BRANCH not in out.split("Local writers")[-1] diff --git a/backend/tests/test_request_pr_review.py b/backend/tests/test_request_pr_review.py index 04073b6d..7ab6266c 100644 --- a/backend/tests/test_request_pr_review.py +++ b/backend/tests/test_request_pr_review.py @@ -57,12 +57,21 @@ def env_file(tmp_path: Path) -> Path: return p -def _gh(bin_dir: Path, reviews: list, run_state: str) -> None: +def _gh( + bin_dir: Path, reviews: list, run_state: str, commits: list | None = None +) -> 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. + + `commits` feeds the precondition gate, which compares the newest verdict + against the newest push to decide whether the last verdict was consumed. + It defaults to a commit NEWER than `_review`'s default `submittedAt`, i.e. + "the last verdict was acted on" — the state every pre-gate test was + implicitly written against. A test that wants to exercise the gate passes + a commit older than its reviews. """ # `unknown` is not a run shape but a FAILURE to read one: `gh run list` # exits non-zero, which is what a network blip or rate limit looks like. @@ -84,7 +93,11 @@ def _gh(bin_dir: Path, reviews: list, run_state: str) -> None: # 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})) + if commits is None: + commits = [{"committedDate": "2099-06-01T00:00:00Z"}] + (bin_dir / "reviews.json").write_text( + json.dumps({"reviews": reviews, "commits": commits}) + ) (bin_dir / "runs.json").write_text(json.dumps(runs)) _write( @@ -124,7 +137,10 @@ def _review(state: str, at: str = "2099-01-01T00:00:01Z", body: str = "x") -> di def _run( - bin_dir: Path, env_file: Path, timeout: int = 2 + bin_dir: Path, + env_file: Path, + timeout: int = 2, + flags: list[str] | None = None, ) -> subprocess.CompletedProcess: env = dict( os.environ, @@ -133,7 +149,7 @@ def _run( BESS_ENV_FILE=str(env_file), ) return subprocess.run( - ["bash", str(SCRIPT), "622", str(timeout)], + ["bash", str(SCRIPT), *(flags or []), "622", str(timeout)], capture_output=True, text=True, env=env, @@ -286,3 +302,142 @@ def test_a_decisive_verdict_wins_over_an_earlier_commented( assert proc.returncode == 0 assert "VERDICT CHANGES_REQUESTED" in proc.stdout + + +# The gate prints this immediately before posting the trigger comment, so its +# absence is proof the trigger path was never reached — a refusal that still +# posted `@claude-bot review` would have spent the round it was refusing. +TRIGGERED = "Requesting review" + +# --- The precondition gate: is another round legal at all? --- +# +# Every test below is PR #619's timeline. Four `@claude-bot review` comments, +# two paid verdicts, one diff that never changed between them. Step 11's prose +# already forbade it; prose could not enforce it because the round count and +# "did I act on the last verdict" lived in a session that died. + +OLD_PUSH = [{"committedDate": "2098-01-01T00:00:00Z"}] +"""A push OLDER than `_review`'s default verdict, i.e. verdict not yet acted on.""" + + +def test_an_unconsumed_changes_requested_refuses_the_next_round( + bin_dir: Path, env_file: Path +) -> None: + """#619 at 10:50. A CHANGES_REQUESTED landed at 09:40, HEAD had not moved + since 07:35, and the loop asked for another review anyway — collecting a + second, different blocking finding on byte-identical code.""" + _gh(bin_dir, [_review("CHANGES_REQUESTED")], "finished", commits=OLD_PUSH) + proc = _run(bin_dir, env_file) + + assert proc.returncode == 1 + assert TRIGGERED not in proc.stdout + assert "unconsumed CHANGES_REQUESTED" in proc.stderr + assert "address them and push" in proc.stderr + + +def test_an_approved_verdict_on_the_current_diff_is_not_re_reviewed( + bin_dir: Path, env_file: Path +) -> None: + """The other half of the same mistake: re-reviewing an approval cannot + improve on it, and the next move is `gh pr ready` — which is exactly what + #615 failed to reach, sitting approved-but-draft overnight.""" + _gh(bin_dir, [_review("APPROVED")], "finished", commits=OLD_PUSH) + proc = _run(bin_dir, env_file) + + assert proc.returncode == 1 + assert TRIGGERED not in proc.stdout + assert "already APPROVED" in proc.stderr + assert "gh pr ready" in proc.stderr + + +def test_a_push_after_the_verdict_makes_the_next_round_legal( + bin_dir: Path, env_file: Path +) -> None: + """The gate must not seize up on the normal path. A verdict that has been + acted on — findings fixed, commit pushed — is what earns the next round.""" + _gh( + bin_dir, + [_review("CHANGES_REQUESTED", at="2099-01-01T00:00:01Z")], + "finished", + commits=[{"committedDate": "2099-03-01T00:00:00Z"}], + ) + proc = _run(bin_dir, env_file) + + assert proc.returncode == 0 + assert TRIGGERED in proc.stdout + + +def test_allow_unconsumed_is_the_reviewer_was_wrong_escape_hatch( + bin_dir: Path, env_file: Path +) -> None: + """Step 11 sanctions one case with no push: the finding was wrong and you + replied on the PR saying why. A flag makes that a decision someone takes, + rather than the default a stalled loop falls into.""" + _gh(bin_dir, [_review("CHANGES_REQUESTED")], "finished", commits=OLD_PUSH) + proc = _run(bin_dir, env_file, flags=["--allow-unconsumed"]) + + assert proc.returncode == 0 + assert TRIGGERED in proc.stdout + + +def test_the_round_cap_is_enforced_here_not_remembered( + bin_dir: Path, env_file: Path +) -> None: + """Step 11 caps the loop at 3 rounds. A resumed session cannot remember how + many have happened, but the PR knows: count the decisive reviews. All three + here are consumed (a push follows them), so the cap is what refuses.""" + _gh( + bin_dir, + [ + _review("CHANGES_REQUESTED", at="2099-01-01T00:00:01Z"), + _review("CHANGES_REQUESTED", at="2099-01-01T00:00:02Z"), + _review("CHANGES_REQUESTED", at="2099-01-01T00:00:03Z"), + ], + "finished", + commits=[{"committedDate": "2099-03-01T00:00:00Z"}], + ) + proc = _run(bin_dir, env_file) + + assert proc.returncode == 1 + assert TRIGGERED not in proc.stdout + assert "3 decisive review rounds" in proc.stderr + + +def test_commented_reviews_do_not_count_toward_the_cap_or_block_a_round( + bin_dir: Path, env_file: Path +) -> None: + """The bot's placeholder and permission-check reviews are `COMMENTED`. #619 + collected one ("test-permission-check-only, will be replaced") 60 seconds + before the real verdict. Counting those would refuse legal rounds and + exhaust the cap on noise — the gate looks only at decisive states.""" + _gh( + bin_dir, + [ + _review("COMMENTED", body="test-permission-check-only"), + _review("COMMENTED", body="Inline notes below; summary to follow."), + _review("COMMENTED", body="more notes"), + ], + "finished", + commits=OLD_PUSH, + ) + proc = _run(bin_dir, env_file) + + assert proc.returncode == 0 + assert TRIGGERED in proc.stdout + + +def test_an_unreadable_gate_refuses_rather_than_spending_a_round( + bin_dir: Path, env_file: Path +) -> None: + """Symmetry with the run-state rule above: "I could not tell" must not + license the expensive action. A needless round costs a paid review of an + unchanged diff; re-running this command costs nothing.""" + _write( + bin_dir / "gh", + "#!/bin/sh\necho 'simulated gh failure' >&2\nexit 1\n", + ) + proc = _run(bin_dir, env_file) + + assert proc.returncode == 1 + assert TRIGGERED not in proc.stdout + assert "never determined" in proc.stderr diff --git a/backend/tests/test_worktree_hook.py b/backend/tests/test_worktree_hook.py new file mode 100644 index 00000000..74203c53 --- /dev/null +++ b/backend/tests/test_worktree_hook.py @@ -0,0 +1,146 @@ +"""Tests for .claude/hooks/check-worktree-path.sh. + +The hook is the only mechanical enforcement of "work in a worktree before ANY +edit". CLAUDE.md has stated that rule unconditionally for a long time and it +still gets skipped, because prose has to be REMEMBERED at the moment of the +first edit — and that is exactly the moment a session which opened as a question +has no reason to reconsider it. + +The damage is on the record. PR #619's branch carries a merge of itself +(`Merge remote-tracking branch 'origin/fix/...' into fix/...`) because two +writers worked the same branch from different bases and diverged for fifteen +hours; the reviewer reviewed one line three times while the other, based on a +commit from 08:09, never saw a verdict. + +So these tests pin the decision the hook makes, against real git checkouts built +in tmp_path rather than the developer's own layout. +""" + +import json +import subprocess +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[2] +HOOK = REPO_ROOT / ".claude" / "hooks" / "check-worktree-path.sh" + + +def _git(cwd: Path, *args: str) -> None: + subprocess.run( + ["git", *args], + cwd=cwd, + check=True, + capture_output=True, + env={ + "PATH": "/usr/bin:/bin:/usr/local/bin:/opt/homebrew/bin", + "HOME": str(cwd), + "GIT_CONFIG_GLOBAL": "/dev/null", + "GIT_CONFIG_SYSTEM": "/dev/null", + "GIT_AUTHOR_NAME": "t", + "GIT_AUTHOR_EMAIL": "t@t", + "GIT_COMMITTER_NAME": "t", + "GIT_COMMITTER_EMAIL": "t@t", + }, + ) + + +@pytest.fixture +def repo(tmp_path: Path) -> Path: + """A real main checkout on `main` with one commit.""" + main = tmp_path / "main-checkout" + main.mkdir() + _git(main, "init", "-q", "-b", "main") + (main / "seed.txt").write_text("seed\n") + _git(main, "add", "seed.txt") + _git(main, "commit", "-q", "-m", "seed") + return main + + +@pytest.fixture +def worktree(repo: Path, tmp_path: Path) -> Path: + """A linked worktree of that checkout — the sanctioned place to edit.""" + wt = tmp_path / "linked-worktree" + _git(repo, "worktree", "add", "-q", "-b", "feat/x", str(wt)) + return wt + + +def _decide(cwd: Path, target: Path) -> dict: + """Run the hook as Claude Code would, and return its decision.""" + proc = subprocess.run( + ["bash", str(HOOK)], + cwd=cwd, + input=json.dumps({"tool_input": {"file_path": str(target)}}), + capture_output=True, + text=True, + ) + assert proc.returncode == 0, proc.stderr + return json.loads(proc.stdout) + + +def _denial(decision: dict) -> str | None: + hook_out = decision.get("hookSpecificOutput") + if not hook_out or hook_out.get("permissionDecision") != "deny": + return None + return hook_out["permissionDecisionReason"] + + +def test_editing_from_the_main_checkout_is_blocked(repo: Path) -> None: + """The gap this closes. The pre-existing check only fired when the target + was a DIFFERENT checkout, so an edit made from the main checkout to a file + in the main checkout — the exact shape of a question-session that drifted + into implementing — sailed through.""" + reason = _denial(_decide(repo, repo / "seed.txt")) + + assert reason is not None + assert "MAIN checkout" in reason + # The remedy has to be in the message: an agent that is blocked without + # being told the next move retries or works around it. + assert "EnterWorktree" in reason + + +def test_the_message_names_the_branch_and_the_read_only_escape(repo: Path) -> None: + """Blocking is only half the job. The main checkout has legitimate + read-only uses — six live sessions sit there asking questions, running + `gh`, and dispatching — so the denial must say what still works, or it + reads as "this session is useless here".""" + reason = _denial(_decide(repo, repo / "seed.txt")) + + assert reason is not None + assert "branch 'main'" in reason + assert "read-only" in reason + + +def test_editing_inside_a_linked_worktree_is_allowed(worktree: Path) -> None: + """The rule must not block the sanctioned path, or it just gets disabled.""" + decision = _decide(worktree, worktree / "seed.txt") + + assert _denial(decision) is None + assert decision.get("continue") is True + + +def test_a_new_file_in_a_worktree_is_allowed(worktree: Path) -> None: + """Resolution goes via the parent directory so a not-yet-created file still + lands in the right checkout — pinned because most edits during + implementation create files.""" + decision = _decide(worktree, worktree / "brand-new.py") + + assert _denial(decision) is None + + +def test_cross_checkout_edits_are_still_blocked(repo: Path, worktree: Path) -> None: + """Regression guard on the original purpose: a stale absolute path from an + earlier turn writing into a different checkout.""" + reason = _denial(_decide(worktree, repo / "seed.txt")) + + assert reason is not None + assert "DIFFERENT checkout" in reason + + +def test_outside_a_git_repo_the_hook_stays_out_of_the_way(tmp_path: Path) -> None: + """The hook governs this repo's worktree discipline, not the filesystem.""" + plain = tmp_path / "not-a-repo" + plain.mkdir() + decision = _decide(plain, plain / "file.txt") + + assert _denial(decision) is None diff --git a/scripts/pr-state.sh b/scripts/pr-state.sh new file mode 100755 index 00000000..c5f9582c --- /dev/null +++ b/scripts/pr-state.sh @@ -0,0 +1,228 @@ +#!/usr/bin/env bash +# +# What state is each open PR in, and WHOSE TURN is it? +# +# Answers "is someone working on this, is it stalled, blocked, in progress, or +# waiting on review" in one command, from GitHub facts only. +# +# Usage: +# scripts/pr-state.sh # every open PR +# scripts/pr-state.sh 619 # one PR +# +# WHY THIS EXISTS. The question was previously answered by a fresh session +# running half a dozen ad-hoc shell commands and inferring from local signals -- +# `git worktree list`, `claude agents --json`, HEAD age. That is expensive (a +# cold Opus context per question, see CLAUDE.md Cost Discipline) and it is wrong +# in a specific way: those signals are LOCAL. An executor running in a container +# or a GitHub Action leaves no worktree and no local session, so every PR reads +# as unowned. +# +# Asked about #619 it reported "no one is working on it, session idle since +# 10:55". 10:55 was the REVIEWER BOT's review timestamp -- `updatedAt` on the +# PR, bumped by someone else entirely. The last executor action was a review +# request at 10:50; the last code action was four hours earlier. The inference +# conflated "something happened on this PR" with "the agent is alive". +# +# So this derives state from what GitHub durably knows, and reports the one thing +# it cannot know as unknown rather than guessing it from a worktree. +# +# THE ONE THING NOT DERIVABLE is liveness: GitHub knows a PR's content state, +# never whether a process is currently working on it. What it does know is +# strictly more useful for deciding what to do -- whether the ball is with the +# executor, the reviewer, or the maintainer. A PR whose newest verdict is newer +# than its newest push is waiting on a code change no matter who is or is not +# alive, so `needs-fix` is actionable without resolving liveness at all. +# +# `sweep-prs` remains the tool that ACTS (merge main, prune worktrees). This one +# only reads, so it is safe to run anywhere, including against a fleet whose +# worktrees live on another machine. +set -euo pipefail + +pr_filter="${1:-}" + +# `reviewDecision` is deliberately not used: it does not distinguish a verdict +# that has been acted on from one that has not, which is the distinction that +# decides whose turn it is. Compute it from reviews vs commits instead. +fields='number,title,isDraft,mergeable,mergeStateStatus,headRefName,reviews,commits,statusCheckRollup,updatedAt,comments' + +# `commits` is what bounds this, and the bound is GitHub's, not a preference. +# `--json commits` expands each commit's authors connection, so gh's cost +# estimate is limit x commits x authors: at --limit 100 that is 100 x 100 x 100 = +# 1,000,000 possible nodes and the query is REJECTED outright -- +# +# GraphQL: By the time this query traverses to the authors connection, it is +# requesting up to 1,000,000 possible nodes which exceeds the maximum limit of +# 500,000. +# +# Found by running this against a real fleet; the fixture tests could not see it. +# 30 keeps the worst case at 300,000, inside the ceiling. There is no cheaper +# field for "when did HEAD last move" -- `gh pr list --json` offers no +# last-commit date, and a review's own commit SHA is REST-only (`gh api`, which +# is on CLAUDE.md's ask list). +PR_LIMIT=30 + +fetch() { + if [ -n "$pr_filter" ]; then + gh pr view "$pr_filter" --json "$fields" | jq -c '[.]' + else + gh pr list --state open --limit "$PR_LIMIT" --json "$fields" + fi +} + +# `mergeable` is computed LAZILY, and this is the single most dangerous thing +# about reading it. The first query on a cold PR returns UNKNOWN *and* triggers +# the computation, so a one-shot pass reports UNKNOWN for precisely the stale +# PRs worth finding -- and if UNKNOWN is treated as "not conflicted", every +# conflicted PR in a cold fleet reads as fine. +# +# Measured here while building this: a first fleet run classified #167 and #619 +# as `needs-fix`/CI-red with no conflict flag; after the queries above had +# warmed them, the identical command returned `needs-refresh` for both. Both +# were CONFLICTING the whole time. `sweep-prs` documents the same trap and +# retries for the same reason. +# +# So: ask again while anything is UNKNOWN, and if it still is, SAY SO rather +# than letting it fall through to the not-conflicted branch. +for _ in 1 2 3; do + raw=$(fetch) + [ "$(echo "$raw" | jq '[.[] | select(.mergeable == "UNKNOWN")] | length')" -eq 0 ] && break + sleep 3 +done + +if [ -z "$pr_filter" ] && [ "$(echo "$raw" | jq 'length')" -eq "$PR_LIMIT" ]; then + # No silent caps (sweep-prs): a truncated fleet must not read as a clean one. + echo "WARNING: hit the ${PR_LIMIT}-PR ceiling — older open PRs were NOT" >&2 + echo "classified. Pass a PR number to inspect one directly." >&2 +fi + +# A quoted heredoc, not a single-quoted argument: the program below contains +# apostrophes, and `jq -r '...'` cannot hold them. +read -r -d '' classify <<'JQ' || true + def verdict: + [.reviews[] | select(.state == "APPROVED" or .state == "CHANGES_REQUESTED")] + | sort_by(.submittedAt) | last; + def pushed: [.commits[].committedDate] | sort | last; + # The Stage 4 bot only ever acts when triggered by this comment, so its + # absence is the difference between "waiting on the reviewer" and "nobody + # has asked the reviewer". + def requested: + [.comments[]? | select(.body | test("@claude-bot review")) | .createdAt] + | sort | last; + def failing: + [.statusCheckRollup[]? | select(.conclusion == "FAILURE") | .name]; + def pending: + [.statusCheckRollup[]? | select(.status == "IN_PROGRESS" or .status == "QUEUED")] + | length; + + .[] + | . as $p + | (verdict) as $v + | (pushed) as $push + | (.mergeable == "CONFLICTING" or .mergeStateStatus == "DIRTY") as $conflicted + # UNKNOWN survived the retries above. It is NOT "not conflicted" -- it is "we + # never found out", and it must never render as a clean PR. + | (.mergeable == "UNKNOWN") as $mergeUnknown + | (requested) as $req + | ($v != null and $v.submittedAt > $push) as $unconsumed + + # Order matters, and it is ordered by WHAT THE DIFF STILL OWES rather than by + # what happened most recently. + # + # An unconsumed CHANGES_REQUESTED outranks a conflict. Both belong to the + # executor, but the findings are about the code while the conflict is + # mechanical, and a PR that reported only its conflict would hide two blocking + # reviews. #619 is exactly that shape -- conflicted AND carrying two + # unaddressed verdicts -- so the conflict is reported alongside, never instead. + # + # Conflict then outranks CI, because a CONFLICTING PR gets no workflow run at + # all (see sweep-prs): testing CI first reports "no checks" and calls a + # conflicted PR green. + | (if $unconsumed and $v.state == "CHANGES_REQUESTED" + then ["needs-fix", "executor", + "UNCONSUMED changes-requested (\($v.submittedAt)); no push since \($push)"] + elif $conflicted + then ["needs-refresh", "sweep", "conflicts with main; no CI runs until merged"] + elif (failing | length) > 0 + then ["needs-fix", "executor", "CI red: " + (failing | join(", "))] + elif pending > 0 + then ["in-flight", "-", "CI running (\(pending) checks)"] + elif $unconsumed + then ["awaiting-ready", "maintainer", + "approved (\($v.submittedAt)) and still a draft"] + elif ($req == null or $req < $push) + then ["needs-review-request", "dispatcher", + (if $req == null + then "green, but a review has NEVER been requested" + else "pushed \($push) after the last request (\($req)); round owed" + end)] + else ["awaiting-review", "reviewer", + "requested \($req); the bot has not returned a verdict yet"] end) + as [$state, $owner, $why] + + | "#\($p.number) \(if $p.isDraft then "draft" else "ready" end) \($state) [\($owner)]" + + (if $conflicted and $state != "needs-refresh" then " (+conflicted)" else "" end) + + (if $mergeUnknown then " (+mergeability UNKNOWN — re-run)" else "" end) + + "\n \($p.title[0:72])\n \($why)\n" +JQ + +echo "$raw" | jq -r "$classify" + +# --- Local join: is more than one writer on this branch? -------------------- +# +# GitHub cannot answer this. A branch with two writers looks entirely normal +# through the API -- the divergence exists only between a local checkout and the +# remote, and it collapses into an ordinary merge the moment someone reconciles. +# +# #619 is the worked example. One writer took the branch at 08:09 and worked +# from that base all day; another pushed 23031e78 at 09:34. The reviewer +# reviewed 23031e78 three times, twice with blocking findings, while the first +# line never held that commit at all. Fifteen hours later it landed as +# `Merge remote-tracking branch 'origin/fix/...' into fix/...` -- a branch merged +# into itself, which is the fingerprint of exactly this. +# +# `git rev-list --left-right` would have caught it at 09:34. +# +# Skipped LOUDLY when there is no checkout to compare: this script is also meant +# to run against a container fleet, where no worktree exists and a silent skip +# would read as "no divergence found". +echo "" +if ! git rev-parse --git-dir >/dev/null 2>&1; then + echo "Local writer check SKIPPED — not inside a git checkout, so no worktree" + echo "could be compared against its remote. This says nothing about writers." +else + git fetch origin --quiet 2>/dev/null || true + echo "Local writers (worktrees whose branch has an open PR):" + git worktree list --porcelain | + awk '/^worktree /{w=$2} /^branch /{sub("refs/heads/","",$2); print w"\t"$2}' | + while IFS="$(printf '\t')" read -r wt branch; do + pr=$(echo "$raw" | jq -r --arg b "$branch" \ + '.[] | select(.headRefName == $b) | .number') + [ -z "$pr" ] && continue + counts=$(git rev-list --left-right --count \ + "refs/remotes/origin/${branch}...${branch}" 2>/dev/null || true) + if [ -z "$counts" ]; then + echo " #${pr} ${branch} — never pushed (no remote-tracking ref)" + continue + fi + behind=$(echo "$counts" | cut -f1) + ahead=$(echo "$counts" | cut -f2) + if [ "$behind" -gt 0 ] && [ "$ahead" -gt 0 ]; then + echo " #${pr} ${branch}" + echo " *** DIVERGED: ${ahead} local / ${behind} remote — TWO WRITERS ***" + echo " ${wt}" + elif [ "$ahead" -gt 0 ]; then + echo " #${pr} ${branch} — ${ahead} unpushed commit(s)" + elif [ "$behind" -gt 0 ]; then + echo " #${pr} ${branch} — ${behind} behind its own remote" + else + echo " #${pr} ${branch} — in sync" + fi + done +fi + +cat <<'EOF' +Liveness (is an agent working RIGHT NOW) is not a GitHub fact and is not +guessed here. `needs-fix`, `needs-refresh` and `needs-review-request` are +actionable regardless: each names an action the pipeline still owes, which no +amount of waiting produces. +EOF diff --git a/scripts/request-pr-review.sh b/scripts/request-pr-review.sh index 456d05c8..33781da9 100755 --- a/scripts/request-pr-review.sh +++ b/scripts/request-pr-review.sh @@ -58,13 +58,58 @@ # review at all — that removes the ambiguity at its source. The run-state check # is what keeps this correct for older PRs and if the bot regresses. # +# THE UNCONSUMED-VERDICT PROBLEM. Everything above concerns waiting for a +# verdict. This block concerns whether asking for one is legal at all. +# +# Step 11's contract is a cycle: request -> verdict -> fix -> push -> request. +# The round count and "have I acted on the last verdict yet" lived only in the +# agent's head, so a session that died, timed out, or simply lost the thread +# re-entered the loop by doing the one thing it could always do -- ask again. +# +# Observed on PR #619, verbatim: +# 06:55 @claude-bot review (no run -- actor gate, see `none` below) +# 07:12 @claude-bot review (no run) +# 09:33 @claude-bot review +# 09:40 CHANGES_REQUESTED two blocking findings +# 10:50 @claude-bot review <-- HEAD unchanged since 07:35 +# 10:55 CHANGES_REQUESTED a different blocking finding +# Four requests, zero verdicts consumed, one byte-identical diff, and two paid +# review rounds spent re-reviewing code nobody had touched. Step 11's prose +# already forbids this ("Fix the blockers ... Then start the next round") and +# already caps rounds at 3. Prose was not the enforcement mechanism, because the +# state it reasons about did not survive the session. +# +# It does not need to. Both facts are already on the PR and are read here +# instead of remembered: +# rounds so far = decisive reviews (APPROVED/CHANGES_REQUESTED) on the PR +# consumed? = is the newest commit newer than the newest verdict? +# A verdict newer than the last push is a verdict about the current diff, so the +# only legal next move is to change the diff. Asking again cannot help. +# +# `--allow-unconsumed` is the escape hatch for the one case Step 11 does +# sanction: the reviewer was wrong, you replied on the PR saying why, and no +# push was warranted. It is a flag rather than the default so that "the reviewer +# is mistaken" has to be a decision someone makes, not the path of least +# resistance a stalled loop falls into. +# +# committedDate is the authored date, not the push date. Those diverge under +# rebase and cherry-pick; this repo merges the target branch instead of rebasing +# (CLAUDE.md, Release Workflow), so on any branch this script is pointed at they +# agree to within seconds of the push. +# # Exit codes: # 0 a verdict landed; verdict on stdout # 2 timed out waiting (recent PR Review runs dumped for diagnosis) -# 1 usage/precondition error +# 1 usage/precondition error, INCLUDING an illegal round (see above) set -euo pipefail -pr="${1:?usage: request-pr-review.sh [timeout-seconds]}" +allow_unconsumed=0 +if [ "${1:-}" = "--allow-unconsumed" ]; then + allow_unconsumed=1 + shift +fi + +pr="${1:?usage: request-pr-review.sh [--allow-unconsumed] [timeout-seconds]}" timeout="${2:-900}" # REVIEW_POLL_INTERVAL is a test seam (see BESS_ENV_FILE in gh-agent.sh for the # same shape): the decision logic is what needs exercising, not the waiting, and @@ -100,6 +145,65 @@ review_run_state() { repo_root=$(git rev-parse --show-toplevel) cd "$repo_root" +# --- Is another round legal at all? (see THE UNCONSUMED-VERDICT PROBLEM) --- +# +# One `gh pr view`, deliberately not `gh api`: that is on CLAUDE.md's ask list, +# and this script is run with run_in_background, where a permission prompt is an +# indefinite stall rather than a question anyone sees. +gate=$(gh pr view "$pr" --json reviews,commits --jq ' + ([.reviews[] + | select(.state == "APPROVED" or .state == "CHANGES_REQUESTED")] + | sort_by(.submittedAt)) as $decisive + | ($decisive | last) as $latest + | ([.commits[].committedDate] | sort | last) as $pushed + | "\($decisive | length) \($latest.state // "none")" + + " \($latest.submittedAt // "-") \($pushed // "-")" + ' 2>/dev/null || echo "") + +if [ -z "$gate" ] && [ "$allow_unconsumed" -eq 0 ]; then + echo "Could not read PR #${pr}'s reviews and commits, so whether another" >&2 + echo "round is legal was never determined. Refusing rather than guessing." >&2 + echo "A needless round costs a paid review of an unchanged diff; re-running" >&2 + echo "this command costs nothing. --allow-unconsumed overrides." >&2 + exit 1 +fi + +if [ -n "$gate" ] && [ "$allow_unconsumed" -eq 0 ]; then + read -r rounds latest_state latest_at last_push <<<"$gate" + + # A verdict newer than the last push is a verdict about the diff as it + # stands. Asking again re-reviews the same bytes -- #619 did this twice. + if [ "$latest_state" != "none" ] && [ "$last_push" != "-" ] && + [[ "$latest_at" > "$last_push" ]]; then + if [ "$latest_state" = "APPROVED" ]; then + echo "PR #${pr} is already APPROVED (${latest_at}) on the current" >&2 + echo "diff -- nothing has been pushed since. Another round cannot" >&2 + echo "improve on an approval. Step 11's next move is 'gh pr ready'," >&2 + echo "after re-checking mergeability." >&2 + else + echo "PR #${pr} has an unconsumed CHANGES_REQUESTED (${latest_at})" >&2 + echo "and HEAD has not moved since (last commit ${last_push})." >&2 + echo "The findings are still outstanding, so the only move that can" >&2 + echo "change the answer is to address them and push. Requesting" >&2 + echo "another review here is what burned two paid rounds on #619." >&2 + echo "" >&2 + echo "If the reviewer is wrong, reply on the PR saying why and pass" >&2 + echo "--allow-unconsumed to request the next round deliberately." >&2 + fi + exit 1 + fi + + # Step 11's cap, enforced here because a resumed session cannot remember it. + if [ "$rounds" -ge 3 ]; then + echo "PR #${pr} already has ${rounds} decisive review rounds. Step 11" >&2 + echo "caps this at 3: three rounds of disagreement means the reviewer" >&2 + echo "and the author disagree about the design, not about a bug, and a" >&2 + echo "fourth will not settle it. Hand the outstanding findings to the" >&2 + echo "user verbatim instead." >&2 + exit 1 + fi +fi + # Reviews strictly newer than this are the ones this run triggered. since=$(date -u +%Y-%m-%dT%H:%M:%SZ)