From 3feb5757f5f43950ab5d13749a5c146c8cd0257e Mon Sep 17 00:00:00 2001 From: Gavin Lucas Date: Fri, 23 Oct 2026 22:47:49 +0100 Subject: [PATCH 1/2] Read the whole change in one review, and fail a review that did not Reviews of the two largest pull requests in quaffed came back partial: each said it had split the change into parallel passes that had not reported back, and wrote up only what it had read itself. The session is not allowed to start other agents, so those passes cannot have run; both runs logged refused tool calls, though without full output which calls they were is not shown. Agent and Workflow are now disallowed, so the session is not offered them and reads the change itself, and the prompt says so. A partial review still posted "approve", so the review check was green over files nobody reviewed. The structured output now carries `complete`, and a review that did not cover every file in the range is posted headed "incomplete", with the verdict on what it read, and fails the run with no marker, so the next request reviews the range again. A missing field counts as incomplete, since it cannot be told apart. Tests run the posting step as it ships against a stand-in for gh, and read the session's arguments; four of the five fail against the old workflow. Signed-off-by: Gavin Lucas --- .github/workflows/claude-review.yaml | 33 ++++++++- README.md | 23 ++++-- tests/test_claude_review.py | 106 +++++++++++++++++++++++++++ 3 files changed, 151 insertions(+), 11 deletions(-) create mode 100644 tests/test_claude_review.py diff --git a/.github/workflows/claude-review.yaml b/.github/workflows/claude-review.yaml index 255019f..8431783 100644 --- a/.github/workflows/claude-review.yaml +++ b/.github/workflows/claude-review.yaml @@ -329,12 +329,16 @@ jobs: # The workflow token, not the Claude GitHub App: Claude needs no GitHub access here, # so the App does not have to be installed for the review to run. github_token: ${{ github.token }} + # Agent and Workflow hand work to other sessions, which a review here cannot wait on or be + # allowed to start. Offered them, a review of a large change can plan to split itself + # into passes and then return a review of only the part it read itself. Not offered + # them, it reads the whole change, which the 60-minute timeout leaves room for. claude_args: | --effort ${{ steps.range.outputs.effort }} ${{ steps.range.outputs.model != '' && format('--model {0}', steps.range.outputs.model) || '' }} --allowedTools "Read,Grep,Glob,Bash(git diff:*),Bash(git log:*),Bash(git show:*)" - --disallowedTools "Edit,Write,NotebookEdit,WebFetch,WebSearch" - --json-schema '{"type":"object","properties":{"verdict":{"type":"string","enum":["approve","changes_requested"]},"review":{"type":"string"}},"required":["verdict","review"]}' + --disallowedTools "Edit,Write,NotebookEdit,WebFetch,WebSearch,Agent,Workflow" + --json-schema '{"type":"object","properties":{"verdict":{"type":"string","enum":["approve","changes_requested"]},"complete":{"type":"boolean"},"review":{"type":"string"}},"required":["verdict","complete","review"]}' prompt: | You are reviewing pull request #${{ inputs.pr-number }} in ${{ github.repository }}. The pull request head is checked out at ${{ steps.pr.outputs.head }}. @@ -348,6 +352,9 @@ jobs: and review against them. They are written for assistants doing the work as well as reviewing it; apply the review rules, and judge the change against the rest. + Read the whole change yourself: there are no other agents to hand parts of it to, and + the job allows an hour. + For a full review: cover everything the pull request changes, with depth over speed. Read the surrounding code, not only the diff, and look at failure paths, at tests that assert the wrong thing, and at documentation the change makes untrue. The aim is that @@ -393,7 +400,10 @@ jobs: manufacture findings; if there is nothing to raise, say so. Return `verdict` as `approve` when nothing you found should block a merge, otherwise - `changes_requested`, and `review` as the review body in GitHub-flavoured Markdown. The + `changes_requested`, and `review` as the review body in GitHub-flavoured Markdown. + Return `complete` as `true` only if you reviewed every file the range changes; if you + did not, return `false` and list in the review the files you did not reach. A review + that is not complete fails the check, so the gap is seen rather than passed over. The comment's title and the verdict are added above the body for you, so start the body with its first section and give it no title of its own. Do not post anything to GitHub yourself. @@ -479,6 +489,23 @@ jobs: exit 1 fi + # A review that did not cover the whole range is posted, so its findings are not lost, + # but fails the run and carries no marker, so the next request reviews the same range + # again. Anything but an explicit `true` counts as not complete: a check that cannot + # tell must not pass. + if ! jq -e '.complete == true' >/dev/null <<<"$REVIEW_JSON"; then + { + printf '## Claude review: incomplete\n\n' + printf 'This %s review did not cover every file in `%s..%s`, so it does not stand for a review of the range, and its verdict on what it read was %s. Comment `/claude-review full` for another.\n\n' \ + "$MODE" "${FROM:0:12}" "${HEAD_SHA:0:12}" "${verdict//_/ }" + jq -r '.review' <<<"$REVIEW_JSON" + printf '\n\n---\n_%s review of `%s..%s`, incomplete. [Run](%s)._\n' "$MODE" "${FROM:0:12}" "${HEAD_SHA:0:12}" "$RUN_URL" + } > "$body" + gh pr comment "$PR" --repo "$GITHUB_REPOSITORY" --body-file "$body" + echo "::error::The review did not cover every file in the range; it is posted as incomplete and the range will be reviewed again." + exit 1 + fi + case "$verdict" in approve) title='Claude review: approve' ;; *) title='Claude review: changes requested' ;; diff --git a/README.md b/README.md index 7dfb57a..66e15cb 100644 --- a/README.md +++ b/README.md @@ -72,8 +72,15 @@ inputs are at the top of `.github/workflows/claude-review.yaml`. then worse than nothing; a record of what was done belongs in the commit message or pull request. - **What Claude can do.** Read the checkout and run `git diff`, `git log` and `git show`. It - returns a verdict, approve or changes requested, and a review body as structured output, and a - later step posts them, so the session never holds a token that can write to GitHub. + returns a verdict, approve or changes requested, whether the review covered every file in the + range, and a review body as structured output, and a later step posts them, so the session + never holds a token that can write to GitHub. It is not offered the tools that hand work to + other agents, so it reads the whole change itself rather than planning to split a large one + into passes and returning a review of only the part it read. +- **An incomplete review fails.** A review that did not cover every file is posted, headed + `Claude review: incomplete` with the verdict on what it read, and fails the run, so the check + is never green over a part nobody reviewed. It carries no marker, so the next request reviews + the same range again. - **One at a time.** Runs for the same pull request queue on the job, so only real requests reach the queue. A running review is never cancelled. Of two waiting, the newer one runs, and the range it covers starts at the last commit a posted review recorded, so commits the @@ -280,12 +287,12 @@ Each release also gets its full tag, such as `v2.0.0`, which never moves. Everything here runs in other repositories' CI, so a change reaches them as soon as its release moves the major tag. This repository's own CI runs the action-pins action from the commit under -review, lints every workflow and action with actionlint, runs the Slack workflows' posting steps -against stand-ins for `curl` and `gh` and the action-pins check over sample references -(`tests/`), and reviews each pull request with the pull request's own version of the Claude -review, so a change to the review reviews itself. The hygiene check does not run on this -repository: it expects the instruction layer and detail layer of a product repository, which -this one does not have. +review, lints every workflow and action with actionlint, runs the Slack workflows' and the Claude +review's posting steps against stand-ins for `curl` and `gh`, and the action-pins check over +sample references (`tests/`), and reviews each pull request with the pull request's own version +of the Claude review, so a change to the review reviews itself. The hygiene check does not run +on this repository: it expects the instruction layer and detail layer of a product repository, +which this one does not have. To run the tests locally, from the root of a clone: diff --git a/tests/test_claude_review.py b/tests/test_claude_review.py new file mode 100644 index 0000000..65aa920 --- /dev/null +++ b/tests/test_claude_review.py @@ -0,0 +1,106 @@ +"""Behaviour of the Claude review's posting step, and what the review session is offered. + +The step's script is read out of the workflow file and run under bash with the environment +GitHub would give it, against a stand-in for `gh` that keeps the comment it was asked to post. +So what is tested is the script that ships, not a copy of it. What this cannot test: the GitHub +expressions that fill the environment, and the review session itself. +""" + +import json +import os +import pathlib +import shlex +import stat +import subprocess +import textwrap + +import pytest +import yaml + +ROOT = pathlib.Path(__file__).resolve().parent.parent +WORKFLOW = ROOT / ".github" / "workflows" / "claude-review.yaml" +HEAD = "a" * 40 + +# Keeps the file `gh pr comment` was handed as the comment it would post. +GH = textwrap.dedent("""\ + #!/usr/bin/env bash + while [ $# -gt 0 ]; do + case "$1" in + --body-file) cp "$2" "$STUB_DIR/comment.md"; shift ;; + esac + shift + done +""") + + +def review_step(name): + document = yaml.safe_load(WORKFLOW.read_text()) + (step,) = [s for s in document["jobs"]["review"]["steps"] if s.get("name") == name] + return step + + +@pytest.fixture +def stubs(tmp_path): + gh = tmp_path / "gh" + gh.write_text(GH) + gh.chmod(gh.stat().st_mode | stat.S_IXUSR) + return tmp_path + + +def post(stubs, review): + env = { + "PATH": f"{stubs}:{os.environ['PATH']}", + "STUB_DIR": str(stubs), + "TMPDIR": str(stubs), + "PR": "7", + "GITHUB_REPOSITORY": "L337-org/example", + "REVIEW_JSON": json.dumps(review), + "EXECUTION_FILE": "", + "HEAD_SHA": HEAD, + "MODE": "full", + "FROM": "b" * 40, + "COUNT": "0", + "TRIGGER": "other", + "RUN_URL": "https://github.com/L337-org/example/actions/runs/1", + } + result = subprocess.run( + ["bash", "-c", review_step("Post the review")["run"]], env=env, capture_output=True, + text=True, timeout=30, + ) + comment = stubs / "comment.md" + return result, comment.read_text() if comment.exists() else None + + +def test_a_complete_review_is_posted_with_its_marker(stubs): + result, comment = post(stubs, {"verdict": "approve", "complete": True, "review": "Fine."}) + assert result.returncode == 0, result.stderr + assert comment.startswith("## Claude review: approve\n") + assert "Fine." in comment + assert f"reviewed-sha={HEAD} depth=full" in comment + + +@pytest.mark.parametrize("review", [ + {"verdict": "approve", "complete": False, "review": "Read half of it."}, + {"verdict": "changes_requested", "complete": False, "review": "Read half of it."}, + # A session that leaves the field out cannot be told from one that skipped files. + {"verdict": "approve", "review": "Read half of it."}, +]) +def test_an_incomplete_review_is_posted_but_fails_and_carries_no_marker(stubs, review): + result, comment = post(stubs, review) + assert result.returncode == 1 + assert "::error::The review did not cover every file in the range" in result.stdout + assert comment.startswith("## Claude review: incomplete\n") + assert f"its verdict on what it read was {review['verdict'].replace('_', ' ')}" in comment + assert "Read half of it." in comment + assert "reviewed-sha=" not in comment + + +def test_the_session_cannot_hand_work_to_other_agents_and_must_say_whether_it_finished(): + arguments = shlex.split(review_step("Review")["with"]["claude_args"].replace("\n", " ")) + disallowed = arguments[arguments.index("--disallowedTools") + 1].split(",") + assert {"Agent", "Workflow"} <= set(disallowed) + allowed = arguments[arguments.index("--allowedTools") + 1].split(",") + assert not {"Agent", "Task", "Workflow"} & set(allowed) + schema = json.loads(arguments[arguments.index("--json-schema") + 1]) + assert schema["properties"]["complete"] == {"type": "boolean"} + assert "complete" in schema["required"] From cbc1f5d61dbfaa71df71867be9b17b35ba43b50d Mon Sep 17 00:00:00 2001 From: Gavin Lucas Date: Fri, 23 Oct 2026 22:53:30 +0100 Subject: [PATCH 2/2] Count a marker only where the workflow writes it, and bound incomplete rounds An incomplete review carried no marker of its own, so the parser, which took the last marker anywhere in a comment, would read one quoted in its text as the review's: a quoted marker for the head would answer the next request "already reviewed", and a quoted paused marker would count as a notice already posted, so automatic reviews would stop silently. The review text comes from a session the pull request's author can steer. A reviewed or paused marker now counts only at the very end of a comment, where this workflow writes it. An incomplete review also escaped the bound on automatic rounds, which counted only marked reviews, so a large pull request that never finishes would be reviewed in full on every push. It now ends with its own marker, which counts push-triggered ones towards the bound and moves no range. The README names the two tools disallowed rather than all tools that reach other agents, the job's timeout is no longer restated in the prompt or a comment, and the incomplete notice offers a delta request as well as a full one. Tests run the range step as it ships against stand-ins for gh and git; four of the new ones fail against the previous commit. Signed-off-by: Gavin Lucas --- .github/workflows/claude-review.yaml | 40 +++++++----- README.md | 14 ++-- tests/test_claude_review.py | 98 ++++++++++++++++++++++++++++ 3 files changed, 130 insertions(+), 22 deletions(-) diff --git a/.github/workflows/claude-review.yaml b/.github/workflows/claude-review.yaml index 8431783..f0efd67 100644 --- a/.github/workflows/claude-review.yaml +++ b/.github/workflows/claude-review.yaml @@ -233,28 +233,35 @@ jobs: comments = [json.loads(line) for line in f if line.strip()] # Only this workflow's own comments carry markers, so one typed by anyone else cannot - # steer the next review. The last marker in a comment is the one this workflow - # appended; an earlier one would be text quoted inside the review. + # steer the next review. A marker counts only at the very end of a comment, where this + # workflow writes it: one anywhere else is text quoted inside a review, which a review + # with no marker of its own - an incomplete one - would otherwise let count. MARKER = re.compile(r"") + r"(?: depth=(full|delta))?(?: trigger=(push|other))? -->\s*\Z") + INCOMPLETE = re.compile(r"\s*\Z") PAUSED = "" OWN = "github-actions[bot]" - reviews, paused_at = [], [] + reviews, incomplete, paused_at = [], [], [] for i, c in enumerate(comments): if c["user"]["login"] != OWN: continue - found = MARKER.findall(c["body"]) - if found: - sha, depth, trigger = found[-1] + if found := MARKER.search(c["body"]): + sha, depth, trigger = found.groups() # Markers from before depth and trigger were recorded were all full reviews. reviews.append({"i": i, "sha": sha, "depth": depth or "full", "trigger": trigger or "other", "body": c["body"]}) - elif PAUSED in c["body"]: + elif found := INCOMPLETE.search(c["body"]): + incomplete.append({"i": i, "trigger": found.group(1)}) + elif c["body"].rstrip().endswith(PAUSED): paused_at.append(i) last = reviews[-1] if reviews else None last_full = max((r["i"] for r in reviews if r["depth"] == "full"), default=-1) - pushes_since_full = sum(1 for r in reviews if r["i"] > last_full and r["trigger"] == "push") + # An incomplete review reviewed nothing, so it moves no range, but a push-triggered one + # still counts towards the bound: a large pull request that never finishes would + # otherwise be reviewed in full on every push. + pushes_since_full = sum(1 for r in reviews + incomplete + if r["i"] > last_full and r["trigger"] == "push") already_paused = any(i > last_full for i in paused_at) def is_ancestor(a, b): @@ -332,7 +339,7 @@ jobs: # Agent and Workflow hand work to other sessions, which a review here cannot wait on or be # allowed to start. Offered them, a review of a large change can plan to split itself # into passes and then return a review of only the part it read itself. Not offered - # them, it reads the whole change, which the 60-minute timeout leaves room for. + # them, it reads the whole change. claude_args: | --effort ${{ steps.range.outputs.effort }} ${{ steps.range.outputs.model != '' && format('--model {0}', steps.range.outputs.model) || '' }} @@ -352,8 +359,7 @@ jobs: and review against them. They are written for assistants doing the work as well as reviewing it; apply the review rules, and judge the change against the rest. - Read the whole change yourself: there are no other agents to hand parts of it to, and - the job allows an hour. + Read the whole change yourself: there are no other agents to hand parts of it to. For a full review: cover everything the pull request changes, with depth over speed. Read the surrounding code, not only the diff, and look at failure paths, at tests that @@ -490,16 +496,18 @@ jobs: fi # A review that did not cover the whole range is posted, so its findings are not lost, - # but fails the run and carries no marker, so the next request reviews the same range - # again. Anything but an explicit `true` counts as not complete: a check that cannot - # tell must not pass. + # but fails the run and carries no reviewed marker, so the next request reviews the same + # range again. Its own marker counts it towards the bound on automatic rounds. + # Anything but an explicit `true` counts as not complete: a check that cannot tell must + # not pass. if ! jq -e '.complete == true' >/dev/null <<<"$REVIEW_JSON"; then { printf '## Claude review: incomplete\n\n' - printf 'This %s review did not cover every file in `%s..%s`, so it does not stand for a review of the range, and its verdict on what it read was %s. Comment `/claude-review full` for another.\n\n' \ + printf 'This %s review did not cover every file in `%s..%s`, so it does not stand for a review of the range, and its verdict on what it read was %s. Comment `/claude-review` to review the same range again, or `/claude-review full` for a full review.\n\n' \ "$MODE" "${FROM:0:12}" "${HEAD_SHA:0:12}" "${verdict//_/ }" jq -r '.review' <<<"$REVIEW_JSON" printf '\n\n---\n_%s review of `%s..%s`, incomplete. [Run](%s)._\n' "$MODE" "${FROM:0:12}" "${HEAD_SHA:0:12}" "$RUN_URL" + printf '\n\n' "$TRIGGER" } > "$body" gh pr comment "$PR" --repo "$GITHUB_REPOSITORY" --body-file "$body" echo "::error::The review did not cover every file in the range; it is posted as incomplete and the range will be reviewed again." diff --git a/README.md b/README.md index 66e15cb..c38bb76 100644 --- a/README.md +++ b/README.md @@ -62,7 +62,8 @@ inputs are at the top of `.github/workflows/claude-review.yaml`. A request by comment always runs, and a full review restarts the count. - **Where the last review stopped.** Each review comment ends with a marker naming the commit it reviewed, the depth, and whether a push started it. Only this workflow's own comments are - read for it. A force-push that drops that commit from the branch falls back to a full review. + read for it, and a marker counts only at the end of one, so one quoted in a review's text does + not. A force-push that drops that commit from the branch falls back to a full review. A request by comment for a commit already reviewed is answered without running one; a push of one only says so in the log, because nobody asked. - **Recorded state.** Every review checks what the change adds to documentation and comments @@ -74,13 +75,14 @@ inputs are at the top of `.github/workflows/claude-review.yaml`. - **What Claude can do.** Read the checkout and run `git diff`, `git log` and `git show`. It returns a verdict, approve or changes requested, whether the review covered every file in the range, and a review body as structured output, and a later step posts them, so the session - never holds a token that can write to GitHub. It is not offered the tools that hand work to - other agents, so it reads the whole change itself rather than planning to split a large one - into passes and returning a review of only the part it read. + never holds a token that can write to GitHub. It is not offered `Agent` or `Workflow`, the + tools that start other agents, so it reads the whole change itself rather than planning to + split a large one into passes and returning a review of only the part it read. - **An incomplete review fails.** A review that did not cover every file is posted, headed `Claude review: incomplete` with the verdict on what it read, and fails the run, so the check - is never green over a part nobody reviewed. It carries no marker, so the next request reviews - the same range again. + is never green over a part nobody reviewed. It carries no reviewed marker, so the next request + reviews the same range again, but a marker of its own counts it towards the bound on automatic + rounds when a push started it. - **One at a time.** Runs for the same pull request queue on the job, so only real requests reach the queue. A running review is never cancelled. Of two waiting, the newer one runs, and the range it covers starts at the last commit a posted review recorded, so commits the diff --git a/tests/test_claude_review.py b/tests/test_claude_review.py index 65aa920..ae5c081 100644 --- a/tests/test_claude_review.py +++ b/tests/test_claude_review.py @@ -104,3 +104,101 @@ def test_the_session_cannot_hand_work_to_other_agents_and_must_say_whether_it_fi schema = json.loads(arguments[arguments.index("--json-schema") + 1]) assert schema["properties"]["complete"] == {"type": "boolean"} assert "complete" in schema["required"] + + +# Answers `gh api` with the comments in $STUB_COMMENTS, one JSON object a line, as the step's +# `--jq '.[]'` would. +GH_COMMENTS = textwrap.dedent("""\ + #!/usr/bin/env bash + printf '%s' "$STUB_COMMENTS" +""") + +# `git merge-base` gives the base; `--is-ancestor` says yes. +GIT = textwrap.dedent("""\ + #!/usr/bin/env bash + if [ "$1" = merge-base ] && [ "$2" = --is-ancestor ]; then exit 0; fi + if [ "$1" = merge-base ]; then echo "$STUB_BASE"; exit 0; fi + exit 1 +""") + +BASE = "c" * 40 + + +def own(body, user="github-actions[bot]"): + return {"user": {"login": user}, "body": body, "author_association": "NONE", + "created_at": "2026-10-23T00:00:00Z"} + + +def decide(tmp_path, comments, push=False, limit=10): + for name, script in (("gh", GH_COMMENTS), ("git", GIT)): + path = tmp_path / name + path.write_text(script) + path.chmod(path.stat().st_mode | stat.S_IXUSR) + output = tmp_path / "output" + output.write_text("") + env = { + "PATH": f"{tmp_path}:{os.environ['PATH']}", + "TMPDIR": str(tmp_path), + "STUB_COMMENTS": "\n".join(json.dumps(c) for c in comments), + "STUB_BASE": BASE, + "GITHUB_OUTPUT": str(output), + "GITHUB_REPOSITORY": "L337-org/example", + "PR": "7", + "HEAD_SHA": HEAD, + "BASE_REF": "main", + "DEPTH": "auto", + "PUSH_TRIGGERED": "true" if push else "false", + "MAX_AUTO_REVIEWS": str(limit), + "FULL_EFFORT": "high", + "DELTA_EFFORT": "medium", + "FULL_MODEL": "opus", + "DELTA_MODEL": "sonnet", + } + result = subprocess.run( + ["bash", "-c", review_step("Read earlier rounds, and decide the depth and range")["run"]], + env=env, capture_output=True, text=True, timeout=30, + ) + assert result.returncode == 0, result.stderr + lines = output.read_text().splitlines() + return dict(line.split("=", 1) for line in lines if "=" in line and "<<" not in line) + + +def test_a_review_at_the_head_leaves_nothing_to_review(tmp_path): + marker = f"" + review = own(f"## Claude review: approve\n\nFine.\n\n{marker}\n") + assert decide(tmp_path, [review])["mode"] == "none" + + +def test_a_marker_quoted_in_an_incomplete_review_is_not_read_as_one(tmp_path): + quoted = (f"Quoting " + "and from the workflow.\n") + body = ("## Claude review: incomplete\n\n" + quoted + + "\n\n") + outputs = decide(tmp_path, [own(body)]) + assert outputs["mode"] == "full" + assert outputs["from"] == BASE + + +def test_a_paused_marker_quoted_mid_review_is_not_a_pause_notice(tmp_path): + reviews = [own(f"Round {n}.\n\n\n") for n in range(2)] + quoting = own("## Claude review: incomplete\n\nIt says here.\n" + "\n\n") + # The bound is reached; with no pause notice posted yet, the run posts one. + assert decide(tmp_path, [*reviews, quoting], push=True, limit=3)["mode"] == "paused" + + +def test_push_triggered_incomplete_reviews_count_towards_the_bound(tmp_path): + incomplete = own("## Claude review: incomplete\n\nRead half.\n" + "\n\n") + assert decide(tmp_path, [incomplete] * 2, push=True, limit=3)["mode"] == "full" + assert decide(tmp_path, [incomplete] * 3, push=True, limit=3)["mode"] == "paused" + # Asked for by comment, they do not count. + asked = own("## Claude review: incomplete\n\nRead half.\n" + "\n\n") + assert decide(tmp_path, [asked] * 3, push=True, limit=3)["mode"] == "full" + + +def test_an_incomplete_review_ends_with_its_own_marker(stubs): + _, comment = post(stubs, {"verdict": "approve", "complete": False, "review": "Half."}) + assert comment.rstrip().endswith("")