Skip to content

Read the whole change in one review, and fail a review that did not - #20

Merged
GavinLucas merged 2 commits into
mainfrom
fix/claude-review-partial
Oct 23, 2026
Merged

GavinLucas merged 2 commits into
mainfrom
fix/claude-review-partial

Conversation

@GavinLucas

Copy link
Copy Markdown
Contributor

Why

Full reviews of the two largest pull requests in L337-org/quaffed (#54 and #55) came back partial, and both posted "approve":

  • What each said: it had split the change into parallel passes that had not reported back, and it wrote up only what it had read itself.
  • Why those passes cannot have run: the session is not allowed to start other agents.
  • What the logs show: the runs logged 14 and 4 refused tool calls. The action hides tool calls without show_full_output, so which calls those were is not shown.
  • Size, not chance: every full review of a smaller pull request there came back complete.

The review / Review check was green both times, over files nobody had reviewed.

What changes

  1. No helper agents. Agent and Workflow join --disallowedTools, so the session is not offered them. The prompt says to read the whole change itself, within the job's hour.

    • Checked in @anthropic-ai/claude-agent-sdk@0.3.304, which the pinned action installs: the subagent tool is Agent, the older Task is an alias for it, and Workflow is the multi-agent tool.
  2. An incomplete review fails. The structured output gains a required complete. When it is not true, the review is still posted, but:

    • Heading: Claude review: incomplete, with the verdict on what it read.
    • Check: the run fails with an error.
    • No reviewed-commit marker, so the next request reviews the same range again.

    A missing field counts as incomplete, since it cannot be told apart from a skipped part.

The README's review section and its description of this repository's CI are updated.

Tested

  • New tests in tests/test_claude_review.py. They run the posting step as it ships against a stand-in for gh, and read the session's arguments:
    • a complete review posts with its marker;
    • an incomplete one, or one missing the field, posts headed incomplete, fails, and carries no marker;
    • Agent and Workflow are disallowed, and complete is required in the schema.
  • Shown to fail. Four of the five tests fail against the workflow on main.
  • Local checks. The full suite passes (95 tests). actionlint shows only the SC2016 notes that CI ignores.

Not tested

  • Whether a large review now completes, rather than reporting itself incomplete. That can only be seen on a large pull request after release: quaffed's #55 with /claude-review full once v2 points at the release.
  • This pull request's own review runs with this version of the workflow, so it exercises both changes on a small diff.

Release

After merge, the owner publishes v2.0.2 and moves v2. No caller needs a change.

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 <Gavin@L337.org>
@github-actions

Copy link
Copy Markdown

Claude review: changes requested

Findings

1. Bug: an incomplete review's body can be read as a marker, so a range nobody finished reviewing can count as reviewed. .github/workflows/claude-review.yaml:496-507 with the parser at :238-253.
The range step looks for markers in the whole body of every github-actions[bot] comment, using an unanchored MARKER.findall(c["body"]), and takes the last match. Until now every review comment ended with a marker the workflow appended, so a marker quoted inside the review text could never be the last one. That is the reasoning the comment at :235-237 relies on ("The last marker in a comment is the one this workflow appended; an earlier one would be text quoted inside the review"). An incomplete comment has no appended marker, so any marker-shaped text in its review body becomes the marker. The review text comes from a session whose input the PR author controls, and the prompt gives that session the head SHA.

What goes wrong:

  • If the incomplete review quotes <!-- claude-review reviewed-sha=<head> depth=full -->, a later /claude-review at the same head is answered "already reviewed" with no run. A later push gets a delta from that SHA rather than a full review.
  • Either way, the README's "It carries no marker, so the next request reviews the same range again" (README.md:82-83) and "the check is never green over a part nobody reviewed" (README.md:81-82) stop holding.

The paused marker has the same problem, and it is likely in this repository specifically. :240 and :431 contain <!-- claude-review paused --> verbatim. An incomplete review comment has no marker, so it falls through to the elif PAUSED in c["body"] branch. If such a review quotes either line, the comment counts as a pause notice already posted. When the push bound is next reached, the mode becomes skip and no notice is posted, so automatic reviews stop with nothing on the pull request saying so.

The "did not complete" comment already has the same exposure through the result text it quotes, but before this change no review comment could reach that branch.

Suggested fix: accept a marker only at the end of the body, e.g. re.search(MARKER.pattern + r"\s*$", body), and the same for PAUSED. Alternatively, neutralise <!-- in the review text before posting. A test should feed the range step an incomplete comment that quotes a marker. The tests here only assert that the benign test body carries none (tests/test_claude_review.py:95).

2. Bug: incomplete reviews are not counted towards max-auto-reviews, so the bound on automatic rounds does not apply to them. .github/workflows/claude-review.yaml:256-257, 278-281.
pushes_since_full counts only comments with markers, and an incomplete review has none. The pull requests that come back incomplete are the large ones this change is about. On those, each push starts another full review at the full model and effort. If it comes back incomplete again, it adds nothing to the count.

code-review.yaml:11-14 accepts the loop between a reviewing agent and a coding agent only because "the shared workflow pauses automatic rounds after a bound". An agent that pushes fixes for the findings an incomplete review did list can now loop without limit. The "did not complete" path already had this, but that came from failures, not from a result that may recur on every round.

Suggested fix: put a distinct sentinel on incomplete comments, e.g. <!-- claude-review incomplete trigger=push -->, and have the parser count push-triggered ones towards the bound without treating them as a reviewed SHA.

3. Maintainability: the README says more than the change does about tools that reach other agents. README.md:77-79.
"It is not offered the tools that hand work to other agents" is broader than the change. Only Agent and Workflow are disallowed (:340).

This session runs the pull request's own version of the review, and it is still offered:

  • ListAgents directly;
  • SendMessage, RemoteTrigger, CronCreate and TaskStop as deferred tools;
  • Skill, with a schedule skill for creating cloud agents.

None of them is in --allowedTools, so calls would be refused. But the change's own reasoning is that offering such a tool lets a review plan around it (:332-335). Either add them to --disallowedTools and to the test at tests/test_claude_review.py:102, or narrow the README sentence to the two tools named. The comment at :332 is accurate as written.

4. Maintainability (recorded state): two added lines restate the job timeout.

  • The prompt at .github/workflows/claude-review.yaml:355-356 says "the job allows an hour".
  • The workflow comment at :335 says "which the 60-minute timeout leaves room for".

Both repeat timeout-minutes: 60 at :165 and go stale when it changes. The second also claims that a whole review fits in the timeout, which nothing here measures. The fact lives at :165. Drop the figure from the prompt (e.g. "Read the whole change yourself: there are no other agents to hand parts of it to"). Either drop the clause from the comment, or say why the timeout is set where it is next to :165.

5. Preference: the incomplete comment always suggests /claude-review full. :499.
An incomplete comment carries no marker, so a plain /claude-review after an incomplete delta reviews the same delta range again, at delta cost. Suggesting full in every case pushes towards the most expensive option, on the pull requests that are already too large to finish. Consider suggesting /claude-review, or naming both.

What I checked

  • The whole diff, all three files:
    • the workflow's claude_args, prompt and new posting branch;
    • the README changes;
    • the new tests/test_claude_review.py.
  • The rest of the review job, for how the new comment interacts with what reads it:
    • the gate;
    • the range and marker parser;
    • the pause logic;
    • the no-verdict path.
  • code-review.yaml, as the caller and for its stated bound.
  • tests/test_slack_workflows.py and conftest.py, for the test conventions the new file follows. Running the script with bash -c and no -e matches the existing tests.
  • The new tests by reading them:
    • the parametrised incomplete cases match the script's ${verdict//_/ } text;
    • the shlex split of claude_args copes with the ${{ }} line's quotes;
    • against the old workflow, four of the five would fail, as the commit message says.
  • That --disallowedTools takes effect: neither Agent nor Workflow appears among this session's tools.

What I did not check

  • I did not run the tests or the parser. Running pytest and python3 needed approval here. Finding 1 rests on reading the regex and the parsing loop, not on running them.
  • I did not check against the action's source which tool names its pinned Claude Code version honours, beyond the tool list this session sees.

full review of f9c180d67187..3feb5757f5f4. Run.

…e 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 <Gavin@L337.org>
@GavinLucas

Copy link
Copy Markdown
Contributor Author

All five points are fixed in cbc1f5d. Both bugs were confirmed in the parser before fixing.

  1. Fixed. A reviewed or paused marker counts only at the very end of a comment, where this workflow writes it. One quoted in a review's text no longer counts.
    • Tests feed the range step an incomplete review quoting a reviewed marker for the head. It gives a full review from the base, not "already reviewed".
    • Tests also feed it one quoting the paused marker while the bound is reached. It posts the notice rather than skipping silently.
  2. Fixed. An incomplete review ends with <!-- claude-review incomplete trigger=... -->.
    • Push-triggered ones count towards max-auto-reviews and move no range; ones requested by comment do not count.
    • A test covers both cases.
  3. Fixed by narrowing: the README names Agent and Workflow, the two tools disallowed.
  4. Fixed. The prompt and the comment no longer restate the timeout.
  5. Fixed. The incomplete notice offers /claude-review to review the same range again, or /claude-review full for a full review.

Tests.

  • The range step now runs as it ships, against stand-ins for gh and git.
  • Four of the new tests fail against 3feb575.
  • The suite passes (100 tests), and actionlint is clean apart from the SC2016 notes CI ignores.

@github-actions

Copy link
Copy Markdown

Claude review: approve

Resolved

  • 1 (quoted marker read as a marker): fixed. MARKER and INCOMPLETE are anchored with \s*\Z, and the pause check is rstrip().endswith(PAUSED) (claude-review.yaml:239-255). The workflow writes all three markers last (:437, :525, and the new incomplete printf), so real comments still match. The two quoting tests exercise the range step as it ships.
  • 2 (incomplete reviews not counted): fixed. Push-triggered incomplete comments count in pushes_since_full and do not enter reviews, so they move no range. The new test covers the below-bound, at-bound and comment-triggered cases.
  • 3 (README overstates tools): fixed. It now names Agent and Workflow.
  • 4 (restated timeout): fixed. Both mentions are gone.
  • 5 (suggests full every time): fixed. The notice offers both /claude-review and /claude-review full.

Still open
None.

Declined
None.

New
None. I checked the following:

  • MARKER.sub("", last["body"]) still strips the trailing marker for the previous-review context.
  • Incomplete comments before the last full review are ignored by the i > last_full filter.
  • The last and reviews[0] indexing is unaffected, because incomplete comments are kept out of reviews.
  • The new tests' stand-in gh and git match the commands the step calls.

What I checked: the whole delta across the workflow, the README and the tests, and the range step around it. I did not run the tests or actionlint, so I am relying on the author's report of 100 passing.


delta review of 3feb5757f5f4..cbc1f5d61dbf. Run.

@GavinLucas
GavinLucas merged commit 3d16133 into main Oct 23, 2026
8 checks passed
@GavinLucas
GavinLucas deleted the fix/claude-review-partial branch October 23, 2026 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant