From 2a1a1cbc6397de3386d3d7b4e9c01957b3ec7e26 Mon Sep 17 00:00:00 2001 From: eldad-caura Date: Thu, 30 Jul 2026 11:40:30 +0300 Subject: [PATCH] ci(review): cap review spend with --max-budget-usd MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This repo's review pipeline invoked the CLI with no spend ceiling, so one runaway exploration could bill without bound. It is also the org's one PUBLIC repo, which is where that matters most. Default is $10.00 per review. The org-membership gate already stops a fork pull request from triggering a review at all. What it does not do is bound what a member's large pull request costs, and sibling repos on the shared pipeline have billed $6.11 on a two-file diff — the ceiling is the missing half of that control, not a replacement. Treat it as a runaway guard, not a budget target. The CLI checks it BETWEEN turns, so a run can overshoot by roughly one turn, and on a single hung turn it never fires at all — the job timeout is the only bound there. Sized above observed spend deliberately: the expensive reviews are the ones that find real defects, so capping near the average would truncate exactly the runs worth paying for. The failure branch had to change too, or the ceiling would have been the least diagnosable outcome in the script. It discarded the CLI's stdout and posted "check workflow logs" above a log that held nothing, and budget exhaustion exits 1 exactly like a crash — so a ceiling hit would have looked like a broken pipeline. It now logs the first 2000 chars of stdout (`VAR=$(cmd)` keeps it even when cmd fails, and claude reports auth, quota and budget failures there rather than on stderr) and branches on the machine-readable marker to post a comment naming the ceiling and the spend. Verified rather than assumed: - --max-budget-usd is present in `claude --help` on 2.1.159, the version this workflow pins in both install steps. - The validator, driven through the real script under bash 5.2.21 (what ubuntu-latest runs): 10.00, 10, .5 and 2.50 pass; 0, 00, 0.0, .0, 0.00, .00, `.`, abc, 10,00, $10, 1., -1 and 1e3 are rejected. An empty value takes the default, since bash `:-` treats unset and empty alike. - All three runtime paths, with a stubbed CLI and gh: exhaustion posts "reached the $7.50 spend ceiling after $7.61 without finishing"; an ordinary crash posts the exit code and logs the payload; success posts the review unchanged, so the happy path is untouched. Shape and value are checked separately. Shape allows `.50` the way the CLI does; value is checked arithmetically because a zero ceiling is accepted by the CLI and fails every review on its first turn, and spelling "zero" as a regex means enumerating 0, 00, 0.0, .0, 0.00 and .00 — where the no-leading-digit forms are the ones easily missed. `jq -e` rather than `grep -q` for the marker: grep stops reading on a match, the upstream printf takes SIGPIPE, and pipefail then turns a MATCH into a non-zero pipeline. jq drains stdin. No RESULT="" declaration, unlike caura-ai/.github: that guard exists there because a CLI smoke test calls the failure helper before RESULT is assigned. Every $RESULT reference here follows the assignment, so there is nothing for nounset to trip on. Checked rather than copied across. No workflow change needed — both invocation sites pass env explicitly, so the script-side default applies without touching them. Signed-off-by: eldad-caura --- .github/scripts/claude_pr_review.sh | 48 +++++++++++++++++++++++++++-- 1 file changed, 46 insertions(+), 2 deletions(-) diff --git a/.github/scripts/claude_pr_review.sh b/.github/scripts/claude_pr_review.sh index 2350c9c53..efd9a9143 100755 --- a/.github/scripts/claude_pr_review.sh +++ b/.github/scripts/claude_pr_review.sh @@ -20,10 +20,35 @@ # REVIEW_PROMPT review instructions + output format # Optional env: # MODEL model id (default: claude-sonnet-4-6) +# MAX_BUDGET_USD per-review spend ceiling, passed to --max-budget-usd (default 10.00) set -euo pipefail MODEL="${MODEL:-claude-sonnet-4-6}" +# A ceiling so one runaway review cannot bill without bound. The reviewer reads the repo +# across turns to judge a diff, which is what makes it useful and also what makes an +# unbounded run possible; sibling repos on the org's shared pipeline have billed $6.11 on a +# two-file diff. The org-membership gate above stops a fork pull request from triggering a +# review at all, but it does not bound what a member's large pull request costs. +# +# A runaway guard, not a budget target. The CLI checks the ceiling BETWEEN turns, so a run +# can overshoot it by roughly one turn, and on a single hung turn it never fires at all — +# the job timeout is the only bound there. Sized above observed spend on purpose: the +# expensive reviews are the ones that find real defects, so capping near the average would +# truncate exactly the runs worth paying for. +MAX_BUDGET_USD="${MAX_BUDGET_USD:-10.00}" +# Shape, then value. Shape does not require a leading digit, so `.50` is accepted the way the +# CLI accepts it, while `.`, `1.`, `10,00`, `$10`, `-1` and `1e3` are rejected. Value is +# checked arithmetically rather than with a second pattern: a zero ceiling is accepted by the +# CLI and makes every review fail on its first turn — which reads as a broken pipeline rather +# than a bad setting — and spelling "zero" as a regex means enumerating 0, 00, 0.0, .0, 0.00 +# and .00, where the no-leading-digit forms are the easy ones to miss. +if ! [[ "$MAX_BUDGET_USD" =~ ^[0-9]*\.?[0-9]+$ ]] \ + || ! awk -v v="$MAX_BUDGET_USD" 'BEGIN { exit !(v + 0 > 0) }'; then + echo "::error::MAX_BUDGET_USD must be a positive decimal number, got '${MAX_BUDGET_USD}'" >&2 + exit 1 +fi + post() { gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" -f body="$1" >/dev/null; } DIFF=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}" -H "Accept: application/vnd.github.diff") @@ -41,8 +66,27 @@ Review the PR diff provided on stdin. Review ONLY the changed lines. If after a # No 2>&1: claude's stderr (warnings/progress) must not contaminate the JSON on # stdout, or jq would parse garbage and yield an empty review. A non-zero exit is # still caught below; stderr goes to the workflow log for debugging. -RESULT=$(printf '%s' "$DIFF" | claude --print --model "$MODEL" --output-format json "$PROMPT") || { - post "⚠️ Claude Code review failed (check workflow logs)." +RESULT=$(printf '%s' "$DIFF" | claude --print --model "$MODEL" --output-format json \ + --max-budget-usd "$MAX_BUDGET_USD" \ + "$PROMPT") || { + CLAUDE_EXIT=$? + # `VAR=$(cmd)` keeps cmd's stdout even when cmd fails, and claude reports several failures + # (auth, quota, and budget exhaustion) there rather than on stderr. This branch used to + # discard it and post "check workflow logs" above a log that held nothing — which would have + # made a ceiling hit the least diagnosable outcome in the script. + echo "Claude exited ${CLAUDE_EXIT}. First 2000 chars of its stdout:" >&2 + printf '%s\n' "${RESULT:0:2000}" >&2 + # Budget exhaustion is an EXPECTED outcome carrying a machine-readable marker, and it exits + # 1 exactly like a crash, so without this branch the ceiling would surface as a bare exit + # code and read as a broken pipeline. `jq -e` rather than `grep -q`: grep stops reading on + # match, the upstream printf takes SIGPIPE, and pipefail then turns a MATCH into a non-zero + # pipeline. jq drains stdin. + if printf '%s' "$RESULT" | jq -e '.subtype == "error_max_budget_usd"' >/dev/null 2>&1; then + SPENT=$(printf '%s' "$RESULT" | jq -r '.total_cost_usd // "unknown"' 2>/dev/null || echo unknown) + post "⚠️ Claude Code review reached the \$${MAX_BUDGET_USD} spend ceiling after \$${SPENT} without finishing. Split the PR, or raise \`MAX_BUDGET_USD\` on the workflow step." + exit 1 + fi + post "⚠️ Claude Code review failed: exit ${CLAUDE_EXIT} (see workflow logs)." exit 1 }