Skip to content

Commit e135b8e

Browse files
kovtcharovOvtcharov
andauthored
fix(ci): post PR reviews from the workflow instead of hoping the model does (#2719)
The Claude reviewer has posted **nothing since 2026-07-27 08:34 UTC** while every `pr-review` job reported success — 84 PRs opened in that window, **50 already merged unreviewed**, each having burned ~\$1.80 of Opus writing a review no one ever saw. The cause is that posting was never part of the harness: the prompt asked the model to finish by running `gh pr comment` itself, and when that last call stops happening the job still exits clean, because the only health check asks "did Claude error?" and never "did a comment appear?". After this change Claude writes the review to a file and the workflow posts it, and a review that fails to materialise turns the job red instead of green. The trigger was #2506 bumping `claude-code-action` v1.0.178 → v1.0.183 (the last review landed 4m25s before it merged; nothing else touched review CI in that window). That bump carries no comment-posting changes of its own — only a bundled runtime bump — which is exactly why a harness that depends on the model remembering a shell command should not be restored as-is. Scoped deliberately: posting is gated on the new `comment_file` input, so `issue-handler`, `pr-comment` and `auto-fix` keep their existing in-prompt posting and cannot double-post. `pr-rereview` opts out of `require_comment` because `REVIEW.md` makes silence correct there. > [!IMPORTANT] > `pull_request_target` reads workflows from the **base** branch, so this PR is still reviewed by the old, broken `main` copy — its own silence proves nothing. The fix only takes effect once merged. ## Test plan - [x] `pytest tests/unit/test_claude_review_posting.py` — 6 passed. Locks the invariant: neither review prompt may hand the model a `gh … comment … --body-file` line, and both jobs must set `comment_file`. - [x] Posting step exercised locally across all six branches: review written → posts; missing file + `require_comment` → **exit 1**; empty file + `require_comment` → **exit 1**; missing file without it → clean no-op; issue event → `gh issue comment`. - [x] Both workflows parse (`yaml.safe_load`). - [ ] After merge: close/reopen one PR and confirm a review comment appears, and that the run is red if it doesn't. - [ ] After merge: backfill reviews on the ~32 still-open unreviewed PRs. --------- Co-authored-by: Ovtcharov <kovtchar@amd.com>
1 parent efd3812 commit e135b8e

3 files changed

Lines changed: 142 additions & 6 deletions

File tree

.github/workflows/claude-run.yml

Lines changed: 45 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,11 @@
1212
# errored) writes execution_file and is NOT retried. The retried case is thus a
1313
# hard crash with no output, overwhelmingly the pre-Claude install ENOENT (no
1414
# comment posted). See the attempt-2 step for the one vanishingly-rare residual.
15-
# 3. artifact-based verification: the action only sets execution_file once Claude
15+
# 3. posting: when a caller passes `comment_file`, Claude WRITES its comment to that
16+
# path and a workflow step POSTS it. Callers must not ask the model to run
17+
# `gh pr comment` itself — a skipped final tool call is invisible (green job, no
18+
# comment). With `require_comment: true` an absent file FAILS the job instead.
19+
# 4. artifact-based verification: the action only sets execution_file once Claude
1620
# ran to completion, and is_error flags a mid-flight error. Either gap emits a
1721
# loud ::warning:: (the step outcome is unreliable — continue-on-error masks it
1822
# and an install crash exits before outcome is set).
@@ -58,6 +62,16 @@ on:
5862
required: false
5963
type: boolean
6064
default: false
65+
comment_file:
66+
description: "Path Claude writes its comment to (e.g. /tmp/review.md). When set, a workflow step posts it — the model must NOT post it itself. Empty keeps the legacy in-prompt `gh` posting."
67+
required: false
68+
type: string
69+
default: ""
70+
require_comment:
71+
description: "With comment_file set: true fails the job when Claude produced no comment (pr-review — a review is mandatory); false treats an absent file as legitimate silence (pr-rereview)."
72+
required: false
73+
type: boolean
74+
default: false
6175

6276
jobs:
6377
run:
@@ -345,6 +359,35 @@ jobs:
345359
prompt: ${{ inputs.prompt }}
346360
claude_args: ${{ inputs.claude_args }}
347361

362+
# --- Post the comment Claude wrote ---
363+
# Posting must not be the model's own final `gh` call — when that call doesn't
364+
# happen the job still goes green with nothing published, which is undetectable.
365+
- name: Post Claude's comment
366+
if: "!cancelled() && inputs.comment_file != ''"
367+
env:
368+
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
369+
COMMENT_FILE: ${{ inputs.comment_file }}
370+
REQUIRE_COMMENT: ${{ inputs.require_comment }}
371+
IS_PR: ${{ github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review_comment' }}
372+
NUMBER: ${{ (github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review_comment') && github.event.pull_request.number || github.event.issue.number }}
373+
RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}
374+
run: |
375+
set -euo pipefail
376+
if [ ! -s "$COMMENT_FILE" ]; then
377+
if [ "$REQUIRE_COMMENT" = "true" ]; then
378+
echo "::error title=Claude produced no review::Expected the review at $COMMENT_FILE; it is missing or empty. Claude ran but published nothing — this is a harness failure, NOT 'the PR was clean' (a clean PR still gets an Approve comment). Log: $RUN_URL"
379+
exit 1
380+
fi
381+
echo "No comment at $COMMENT_FILE — nothing to post (silence is a valid outcome for this job)."
382+
exit 0
383+
fi
384+
if [ "$IS_PR" = "true" ]; then
385+
gh pr comment "$NUMBER" --body-file "$COMMENT_FILE"
386+
else
387+
gh issue comment "$NUMBER" --body-file "$COMMENT_FILE"
388+
fi
389+
echo "✅ Posted $(wc -c < "$COMMENT_FILE") bytes to #$NUMBER"
390+
348391
# --- Verify a result was actually produced ---
349392
# Don't trust step outcome (continue-on-error masks it; an install crash exits
350393
# before it's set). Check the real artifact from whichever attempt ran. !cancelled()
@@ -366,5 +409,5 @@ jobs:
366409
if [ "$ok" = "true" ]; then
367410
echo "✅ Claude $KIND completed (log: $EXEC_FILE)"
368411
else
369-
echo "::warning title=Claude $KIND did not complete::No $KIND was posted to $TARGET after 2 attempts — the Claude action crashed or errored before finishing (often the upstream install-phase ENOENT flake). Re-run: $RUN_URL"
412+
echo "::warning title=Claude $KIND did not complete::Claude did not finish the $KIND for $TARGET after 2 attempts — the action crashed or errored before completing (often the upstream install-phase ENOENT flake). Re-run: $RUN_URL"
370413
fi

.github/workflows/claude.yml

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -133,15 +133,25 @@ jobs:
133133
# Run the gaia-testing skill to produce real-world evidence before the review.
134134
# Gated inside claude-run.yml to same-repo PRs touching a testable surface — never fork code.
135135
generate_evidence: true
136+
# Claude writes the review here; claude-run.yml posts it. A review is mandatory, so an
137+
# empty/absent file fails the job rather than passing silently.
138+
comment_file: /tmp/review.md
139+
require_comment: true
136140
prompt: |
137141
Your review POLICY — correctness-first severity, the nit cap, skip rules, and
138142
length caps — lives in the review rubric. Read it from `/tmp/review-rubric.md`
139143
(fetched from the base branch) and follow it exactly. If that file is absent or
140144
empty, fall back to the inline policy in this prompt and note at the top of your
141145
review that the rubric was unavailable. Do NOT add a `custom_instructions:` input —
142146
it was removed from the action API and is ignored.
143-
Then post one review comment in the TWO-part format defined under "Output format — TWO parts" below: a visible human summary on top (verdict + headline issues in plain words), with the per-severity issues, `file:line`, and ```suggestion blocks tucked into the collapsed `<details>` block.
144-
Post the review via: `gh pr comment ${{ github.event.pull_request.number }} --body-file /tmp/review.md` (write body to /tmp/review.md first).
147+
Then write one review comment in the TWO-part format defined under "Output format — TWO parts" below: a visible human summary on top (verdict + headline issues in plain words), with the per-severity issues, `file:line`, and ```suggestion blocks tucked into the collapsed `<details>` block.
148+
149+
HOW YOUR REVIEW SHIPS — write it to `/tmp/review.md`. That file IS the review: a
150+
workflow step posts it after you exit. Do NOT run `gh pr comment` (or any other
151+
`gh` posting command) yourself — that would double-post. Writing `/tmp/review.md`
152+
is the ONE required output of this job; finishing without it fails the run, so
153+
write it even when the PR is clean (a clean PR gets an Approve comment, not silence)
154+
and write it BEFORE you run low on turns.
145155
146156
### Real-world evidence — a REQUIRED section, and your verdict must depend on it
147157
ALWAYS include a `### Real-world evidence` heading in the VISIBLE part of your comment (after
@@ -399,6 +409,10 @@ jobs:
399409
with:
400410
# Re-review fork-PR pushes too; same trade-off as pr-review (see file header).
401411
allowed_non_write_users: "*"
412+
# Claude writes the re-review here; claude-run.yml posts it. require_comment stays false —
413+
# REVIEW.md makes silence the correct default for a re-review, so no file means no comment.
414+
comment_file: /tmp/rereview.md
415+
require_comment: false
402416
prompt: |
403417
You are doing a LIGHTWEIGHT re-review of a push to an already-reviewed GAIA pull
404418
request — NOT a full review. The first pr-review already covered this PR in depth.
@@ -433,8 +447,10 @@ jobs:
433447
the PR. This is intentionally STRICTER than the full review's nit cap: a re-review
434448
posts ZERO nits (the rubric's nit budget is the ceiling for the first review only).
435449
436-
If you do comment, write it to /tmp/rereview.md first, then:
437-
`gh pr comment ${{ github.event.pull_request.number }} --body-file /tmp/rereview.md`
450+
If — and ONLY if — you have something worth flagging, write it to `/tmp/rereview.md`.
451+
A workflow step posts that file after you exit; do NOT run `gh pr comment` yourself
452+
(that would double-post). If you have nothing to flag, write NO file and exit — that
453+
is the expected outcome for most pushes.
438454
439455
## Output format — TWO parts (mandatory)
440456
Structure the comment in two parts:
Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
# Copyright(C) 2025-2026 Advanced Micro Devices, Inc. All rights reserved.
2+
# SPDX-License-Identifier: MIT
3+
4+
"""Guards the PR-review posting contract in .github/workflows/.
5+
6+
Claude WRITES its review to a file; a workflow step POSTS it. When the model was
7+
asked to run `gh pr comment` itself, a skipped final tool call published nothing
8+
while the job stayed green — 50 PRs merged unreviewed before anyone noticed.
9+
These tests fail if that arrangement is reintroduced.
10+
"""
11+
12+
import re
13+
from pathlib import Path
14+
15+
import pytest
16+
17+
yaml = pytest.importorskip("yaml")
18+
19+
WORKFLOWS = Path(__file__).resolve().parents[2] / ".github" / "workflows"
20+
REVIEW_JOBS = ("pr-review", "pr-rereview")
21+
22+
# `gh pr comment N --body-file X` / `gh issue comment ...` — the executable posting
23+
# form. A bare "do NOT run `gh pr comment`" prohibition has no --body-file and is fine.
24+
POSTING_CMD = re.compile(r"gh\s+(?:pr|issue)\s+comment\b[^\n]*--body-file")
25+
26+
27+
def _load(name):
28+
return yaml.safe_load((WORKFLOWS / name).read_text(encoding="utf-8"))
29+
30+
31+
@pytest.fixture(scope="module")
32+
def claude():
33+
return _load("claude.yml")
34+
35+
36+
@pytest.fixture(scope="module")
37+
def runner():
38+
return _load("claude-run.yml")
39+
40+
41+
@pytest.mark.parametrize("job,require", [("pr-review", True), ("pr-rereview", False)])
42+
def test_review_jobs_delegate_posting_to_the_workflow(claude, job, require):
43+
"""Both review jobs hand a file to claude-run.yml instead of posting inline."""
44+
with_ = claude["jobs"][job]["with"]
45+
assert with_.get("comment_file"), f"{job} must set comment_file"
46+
# pr-review: a review is mandatory, so no file is a failure. pr-rereview:
47+
# REVIEW.md makes silence correct, so no file is a legitimate no-op.
48+
assert with_.get("require_comment", False) is require
49+
50+
51+
@pytest.mark.parametrize("job", REVIEW_JOBS)
52+
def test_review_prompts_never_tell_the_model_to_post(claude, job):
53+
"""The model must not be handed a posting command — that path fails silently."""
54+
prompt = claude["jobs"][job]["with"]["prompt"]
55+
found = POSTING_CMD.search(prompt)
56+
assert found is None, (
57+
f"{job}'s prompt tells the model to post: {found.group(0)!r}. "
58+
"Posting belongs to claude-run.yml's 'Post Claude's comment' step; "
59+
"a model-issued post is invisible when it doesn't happen."
60+
)
61+
62+
63+
def test_runner_posts_and_is_gated_on_comment_file(runner):
64+
steps = runner["jobs"]["run"]["steps"]
65+
post = next((s for s in steps if s.get("name") == "Post Claude's comment"), None)
66+
assert post is not None, "claude-run.yml lost its posting step"
67+
assert "inputs.comment_file != ''" in post["if"], (
68+
"posting must be gated on comment_file so callers that still post inline "
69+
"(issue-handler, pr-comment) don't double-post"
70+
)
71+
assert POSTING_CMD.search(post["run"]), "posting step no longer posts anything"
72+
73+
74+
def test_runner_declares_the_posting_inputs(runner):
75+
inputs = runner[True]["workflow_call"]["inputs"] # PyYAML reads `on:` as True
76+
assert inputs["comment_file"]["default"] == ""
77+
assert inputs["require_comment"]["default"] is False

0 commit comments

Comments
 (0)