From 1782fb20782e5e139e6aed6eedf05771bbfb0b9f Mon Sep 17 00:00:00 2001 From: Jakob Heuser Date: Tue, 25 Aug 2026 22:56:29 -0700 Subject: [PATCH] chore(ci): fail the on-demand review when it posts nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../claude-code-review-on-demand.yml | 148 ++++++++++++++++++ 1 file changed, 148 insertions(+) diff --git a/.github/workflows/claude-code-review-on-demand.yml b/.github/workflows/claude-code-review-on-demand.yml index 379ce152..22431b3f 100644 --- a/.github/workflows/claude-code-review-on-demand.yml +++ b/.github/workflows/claude-code-review-on-demand.yml @@ -211,6 +211,7 @@ jobs: # Note: claude-code-action adds its own 👀 reaction to the triggering # comment, so there's no explicit reaction step here. - name: Run Claude Code Review + id: review uses: anthropics/claude-code-action@d40ddef4c030e508327d6e35a9c45f3368482c50 # v1 with: claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} @@ -247,3 +248,150 @@ jobs: # output — the workflow may use privileged tools, the model may not. claude_args: | --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Read" + + # A review that posts NOTHING must not report success. + # + # MEASURED on PR #182 (run 32933651692): the 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"), + # `No buffered inline comments` was logged, 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 PR reads as reviewed when nothing read it. + # + # `is_error` is NOT a sufficient gate — it was false in that very run. + # The load-bearing signals are in the 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 reported separately + # because a denial is how a review dies quietly: the tool it needs is not + # on the allowlist, it cannot say so anywhere a human will look, and it + # stops. + # + # This step reads only the run's own counters — never the model's output. + # `show_full_output: true` would answer the same question, but this is a + # PUBLIC repo and that dumps text the model produced while ingesting an + # untrusted diff into a world-readable log. Counters are not attacker- + # controlled; model prose is. + # + # `if: always()` so this still runs when the action itself fails, and the + # summary records what happened either way. + - name: Verify the review actually ran + if: always() + env: + EXECUTION_FILE: ${{ steps.review.outputs.execution_file }} + run: | + if [ -z "$EXECUTION_FILE" ] || [ ! -f "$EXECUTION_FILE" ]; then + echo "::error::No execution file from the review action; cannot confirm a review ran." + exit 1 + fi + + python3 - "$EXECUTION_FILE" <<'PYEOF' + import json, os, sys + + path = sys.argv[1] + with open(path) as fh: + text = fh.read() + + # The action has written both a JSON array of messages and JSONL, + # depending on version. Accept either rather than pinning a shape. + result = None + try: + parsed = json.loads(text) + messages = parsed if isinstance(parsed, list) else [parsed] + except json.JSONDecodeError: + messages = [] + for line in text.splitlines(): + line = line.strip() + if not line: + continue + try: + messages.append(json.loads(line)) + except json.JSONDecodeError: + continue + + for message in messages: + if isinstance(message, dict) and message.get("type") == "result": + result = message + + if result is None: + print("::error::Execution file holds no result record; cannot confirm a review ran.") + sys.exit(1) + + turns = result.get("num_turns") + denials = result.get("permission_denials_count") or 0 + is_error = result.get("is_error") + cost = result.get("total_cost_usd") + + summary = ( + f"num_turns={turns} permission_denials={denials} " + f"is_error={is_error} cost_usd={cost}" + ) + print(summary) + + summary_path = os.environ.get("GITHUB_STEP_SUMMARY") + if summary_path: + with open(summary_path, "a") as fh: + fh.write(f"### Review execution\n\n`{summary}`\n") + + failed = False + + if is_error: + print("::error::The review action reported an error.") + failed = True + + # The core guard. A review that took no turns posted nothing. + if not turns: + print( + "::error::The review took 0 turns and therefore posted no " + "review. Treating this as a failure so it is not mistaken " + "for a clean pass." + ) + failed = True + + if denials: + # Name the denied TOOLS when the record carries them. Tool names + # are structured data the runner produced, not model prose, so + # printing them is safe on a public repo where dumping the full + # output would not be. This is what makes the next occurrence + # self-diagnosing instead of needing a local re-run. + denied_tools = [] + records = result.get("permission_denials") + if not isinstance(records, list): + records = [ + m for m in messages + if isinstance(m, dict) + and "denial" in str(m.get("type", "")).lower() + ] + for record in records or []: + if not isinstance(record, dict): + continue + # Never print the tool INPUT: it can quote the untrusted diff. + name = ( + record.get("tool_name") + or record.get("tool") + or record.get("name") + ) + if name and name not in denied_tools: + denied_tools.append(str(name)) + + detail = ( + f" Denied tool(s): {', '.join(denied_tools)}." + if denied_tools + else " The record does not name them; add the tool to" + " --allowedTools once identified." + ) + # Loud even when the review otherwise succeeded: a partial review + # missing a tool is still a review that could not see everything. + print( + f"::error::The review hit {denials} permission denial(s)." + f"{detail} A tool it needed is not on the allowlist, so its" + " findings may be incomplete." + ) + if denied_tools and summary_path: + with open(summary_path, "a") as fh: + fh.write(f"\nDenied tools: `{', '.join(denied_tools)}`\n") + failed = True + + sys.exit(1 if failed else 0) + PYEOF