diff --git a/.github/workflows/claude-review.yaml b/.github/workflows/claude-review.yaml index 255019f..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): @@ -329,12 +336,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. 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 +359,8 @@ 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. + 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 +406,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 +495,25 @@ 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 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` 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." + 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..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 @@ -72,8 +73,16 @@ 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 `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 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 @@ -280,12 +289,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..ae5c081 --- /dev/null +++ b/tests/test_claude_review.py @@ -0,0 +1,204 @@ +"""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"] + + +# 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("")