Skip to content

Commit 3363036

Browse files
committed
chore(ci): fail the on-demand review when it posts nothing
MEASURED on PR #182 (run 32933651692): the review action exited `success` with `is_error: false` having done no work at all — `num_turns: 0`, `permission_denials_count: 4`, `total_cost_usd: 1.076`. The tracking comment kept its placeholder ("I'll analyze this and get back to you"), the run logged `No buffered inline comments`, and the PR received zero inline comments. The job went green. That is the worst failure this workflow can have. A silent no-op is indistinguishable from a clean review, so a pull request reads as reviewed when nothing read it, and the cost is paid either way. `is_error` cannot gate this; it was false in that very run. The load-bearing signal is in the action's execution file: `num_turns` counts the model's completed turns, and a review that never took a turn cannot have posted anything. `permission_denials_count` is failed on separately, because a denial is how a review dies quietly — the tool it needs is not on the allowlist, it has nowhere to say so, and it stops. A denial with turns is also failed: a review that lost a tool is a review that could not see everything. The step names the denied tools when the record carries them. Tool names are structured data the runner produced, so printing them is safe here; the tool INPUT is never printed, because it can quote the untrusted diff. That is what makes the next occurrence self-diagnosing rather than a log excavation. `show_full_output: true` would answer the same question and is deliberately not used: this is a public repo and it dumps text the model produced while ingesting an untrusted diff into a world-readable log. Counters are not attacker controlled; model prose is. Both execution-file shapes the action has written (a JSON array of messages, and JSONL) are accepted, rather than pinning a shape it may change. Verified against six fixtures including the real record from run 32933651692: the failing shape, a healthy run, denials-with-turns, a file with no result record, a missing file, and an unset variable.
1 parent a2939c8 commit 3363036

1 file changed

Lines changed: 148 additions & 0 deletions

File tree

‎.github/workflows/claude-code-review-on-demand.yml‎

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,7 @@ jobs:
211211
# Note: claude-code-action adds its own 👀 reaction to the triggering
212212
# comment, so there's no explicit reaction step here.
213213
- name: Run Claude Code Review
214+
id: review
214215
uses: anthropics/claude-code-action@d40ddef4c030e508327d6e35a9c45f3368482c50 # v1
215216
with:
216217
claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }}
@@ -247,3 +248,150 @@ jobs:
247248
# output — the workflow may use privileged tools, the model may not.
248249
claude_args: |
249250
--allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Read"
251+
252+
# A review that posts NOTHING must not report success.
253+
#
254+
# MEASURED on PR #182 (run 32933651692): the action exited `success` with
255+
# `is_error: false` having done no work at all — `num_turns: 0`,
256+
# `permission_denials_count: 4`, `total_cost_usd: 1.076`. The tracking
257+
# comment kept its placeholder ("I'll analyze this and get back to you"),
258+
# `No buffered inline comments` was logged, and the PR received zero
259+
# inline comments. The job went green. That is the worst failure this
260+
# workflow can have: a silent no-op is indistinguishable from a clean
261+
# review, so a PR reads as reviewed when nothing read it.
262+
#
263+
# `is_error` is NOT a sufficient gate — it was false in that very run.
264+
# The load-bearing signals are in the execution file: `num_turns` counts
265+
# the model's completed turns, and a review that never took a turn cannot
266+
# have posted anything. `permission_denials_count` is reported separately
267+
# because a denial is how a review dies quietly: the tool it needs is not
268+
# on the allowlist, it cannot say so anywhere a human will look, and it
269+
# stops.
270+
#
271+
# This step reads only the run's own counters — never the model's output.
272+
# `show_full_output: true` would answer the same question, but this is a
273+
# PUBLIC repo and that dumps text the model produced while ingesting an
274+
# untrusted diff into a world-readable log. Counters are not attacker-
275+
# controlled; model prose is.
276+
#
277+
# `if: always()` so this still runs when the action itself fails, and the
278+
# summary records what happened either way.
279+
- name: Verify the review actually ran
280+
if: always()
281+
env:
282+
EXECUTION_FILE: ${{ steps.review.outputs.execution_file }}
283+
run: |
284+
if [ -z "$EXECUTION_FILE" ] || [ ! -f "$EXECUTION_FILE" ]; then
285+
echo "::error::No execution file from the review action; cannot confirm a review ran."
286+
exit 1
287+
fi
288+
289+
python3 - "$EXECUTION_FILE" <<'PYEOF'
290+
import json, os, sys
291+
292+
path = sys.argv[1]
293+
with open(path) as fh:
294+
text = fh.read()
295+
296+
# The action has written both a JSON array of messages and JSONL,
297+
# depending on version. Accept either rather than pinning a shape.
298+
result = None
299+
try:
300+
parsed = json.loads(text)
301+
messages = parsed if isinstance(parsed, list) else [parsed]
302+
except json.JSONDecodeError:
303+
messages = []
304+
for line in text.splitlines():
305+
line = line.strip()
306+
if not line:
307+
continue
308+
try:
309+
messages.append(json.loads(line))
310+
except json.JSONDecodeError:
311+
continue
312+
313+
for message in messages:
314+
if isinstance(message, dict) and message.get("type") == "result":
315+
result = message
316+
317+
if result is None:
318+
print("::error::Execution file holds no result record; cannot confirm a review ran.")
319+
sys.exit(1)
320+
321+
turns = result.get("num_turns")
322+
denials = result.get("permission_denials_count") or 0
323+
is_error = result.get("is_error")
324+
cost = result.get("total_cost_usd")
325+
326+
summary = (
327+
f"num_turns={turns} permission_denials={denials} "
328+
f"is_error={is_error} cost_usd={cost}"
329+
)
330+
print(summary)
331+
332+
summary_path = os.environ.get("GITHUB_STEP_SUMMARY")
333+
if summary_path:
334+
with open(summary_path, "a") as fh:
335+
fh.write(f"### Review execution\n\n`{summary}`\n")
336+
337+
failed = False
338+
339+
if is_error:
340+
print("::error::The review action reported an error.")
341+
failed = True
342+
343+
# The core guard. A review that took no turns posted nothing.
344+
if not turns:
345+
print(
346+
"::error::The review took 0 turns and therefore posted no "
347+
"review. Treating this as a failure so it is not mistaken "
348+
"for a clean pass."
349+
)
350+
failed = True
351+
352+
if denials:
353+
# Name the denied TOOLS when the record carries them. Tool names
354+
# are structured data the runner produced, not model prose, so
355+
# printing them is safe on a public repo where dumping the full
356+
# output would not be. This is what makes the next occurrence
357+
# self-diagnosing instead of needing a local re-run.
358+
denied_tools = []
359+
records = result.get("permission_denials")
360+
if not isinstance(records, list):
361+
records = [
362+
m for m in messages
363+
if isinstance(m, dict)
364+
and "denial" in str(m.get("type", "")).lower()
365+
]
366+
for record in records or []:
367+
if not isinstance(record, dict):
368+
continue
369+
# Never print the tool INPUT: it can quote the untrusted diff.
370+
name = (
371+
record.get("tool_name")
372+
or record.get("tool")
373+
or record.get("name")
374+
)
375+
if name and name not in denied_tools:
376+
denied_tools.append(str(name))
377+
378+
detail = (
379+
f" Denied tool(s): {', '.join(denied_tools)}."
380+
if denied_tools
381+
else " The record does not name them; add the tool to"
382+
" --allowedTools once identified."
383+
)
384+
# Loud even when the review otherwise succeeded: a partial review
385+
# missing a tool is still a review that could not see everything.
386+
print(
387+
f"::error::The review hit {denials} permission denial(s)."
388+
f"{detail} A tool it needed is not on the allowlist, so its"
389+
" findings may be incomplete."
390+
)
391+
if denied_tools and summary_path:
392+
with open(summary_path, "a") as fh:
393+
fh.write(f"\nDenied tools: `{', '.join(denied_tools)}`\n")
394+
failed = True
395+
396+
sys.exit(1 if failed else 0)
397+
PYEOF

0 commit comments

Comments
 (0)