diff --git a/.github/workflows/claude-pr-review.yml b/.github/workflows/claude-pr-review.yml index 6dff399..90ca503 100644 --- a/.github/workflows/claude-pr-review.yml +++ b/.github/workflows/claude-pr-review.yml @@ -91,7 +91,15 @@ jobs: ref: main token: ${{ steps.app-token.outputs.token }} path: .github-workflows - sparse-checkout: docs/claude-pr-review-prompt.md + # The two files this workflow reads out of the central repo, both from `main` -- the + # deployed unit is whatever main holds, the same way every consumer repo gets it. + # Non-cone mode, so these are gitignore-style patterns matching the two paths exactly. + # A path missing here does not fail the checkout; it fails later as a file-not-found in + # the step that reads it, and for the context step that means continue-on-error swallows + # it into an empty . + sparse-checkout: | + docs/claude-pr-review-prompt.md + scripts/gather-review-context.sh sparse-checkout-cone-mode: false - name: Load review prompt @@ -133,405 +141,21 @@ jobs: # fetch what it needs itself when the block is empty. Without that line the prompt # would be telling it not to re-fetch context it never received. continue-on-error: true - # Explicitly, because the default for a run block is `bash -e {0}` -- no pipefail. - # This step is mostly `gh ... | jq` pipelines, and gh writes its error body to - # stdout, so without pipefail a failed fetch feeds its own error text to jq and the - # block renders whatever jq makes of it instead of the guarded fallback sentence. - # tests/context-step-test.sh runs the extracted script under the same shell. + # The work is a script rather than an inline block because Actions parses a `run:` block + # as one template expression and refuses any over 21,000 characters. Inline, this one was + # 20,545 -- about six comment lines of headroom -- and the change that crossed the line + # stopped every review in the org. The script sets its own -e and pipefail, which is what + # `shell: bash` supplied before, and tests/context-step-test.sh runs that same file rather + # than a copy scraped back out of this one. shell: bash - run: | - PR_NUMBER=${{ github.event.pull_request.number }} - REPO=${{ github.repository }} - # Caps. The median PR reviewed across the org is 161 changed lines and the largest - # in a week was 3,448, so 3,000 patch lines covers the corpus; the cap exists so - # one generated-file PR cannot blow up the prompt. - DIFF_MAX=3000 - SINCE_MAX=2000 - LOG_WINDOW=120 - # Byte budgets, split across the step's two outputs rather than applied to one of - # them. threads is its own output written before the context file, so a cap that - # only measured the context bounded nothing: 400 inline comments rendered 1.1 MB of - # threads on their own. The total here is deliberately far below any plausible - # runner limit -- the largest PR reviewed across the org in a week rendered about - # 150 KB -- because the runner accounts for output size in UTF-16, so a byte count - # here is not the number it checks against. - THREADS_MAX_BYTES=100000 - CTX_MAX_BYTES=200000 - # One job log can be mostly a single line: LOG_WINDOW counts lines and a CI log - # line has no length limit, so a base64 or JSON dump next to the first error marker - # would otherwise consume the whole context ahead of the diff. - LOG_MAX_BYTES=40000 - CTX="${RUNNER_TEMP}/pr-context.md" - : > "$CTX" - - # gh refuses a raw-text body containing ANSI colour unless told to allow escape - # sequences, and a diff or a job log earns an escape byte from any file holding - # terminal output -- this repository's own job-log fixtures do. That refusal and - # its --allow-escape-sequences opt-out arrived together in gh 2.97.0 as a security - # fix; ubuntu-latest ships 2.96.0, where the flag is an unknown-flag error and the - # refusal does not exist either. So every raw fetch tries the flag and falls back - # to the bare call: on 2.96 the first attempt fails and the second succeeds, on - # 2.97+ the first succeeds. Pinning either form breaks on the other, and the - # runner image updates weekly. - # head -c against a *file*, never a pipe: `sed ... | head -c` closes the pipe early - # and SIGPIPE takes the producer down under pipefail, which is the shape that has - # already cost this step its error window once. - cap_file() { - if [ "$(wc -c < "$1" | tr -d " ")" -gt "$2" ]; then - head -c "$2" "$1" > "$1.cut" - mv "$1.cut" "$1" - echo "($3)" >> "$1" - fi - } - - fetch_raw() { - RAW_OUT=$1 - shift - gh "$@" --allow-escape-sequences > "$RAW_OUT" 2>/dev/null && return 0 - gh "$@" > "$RAW_OUT" 2>/dev/null - } - - # Count distinct commits already reviewed, never review state: the org ruleset - # sets dismiss_stale_reviews_on_push, so a push flips a prior APPROVED to - # DISMISSED and a state filter stops matching it. Inline comments each create - # their own COMMENTED review sharing the round's commit_id, so unique commit_id - # == round count, +/-1 when a push lands mid-round and splits it across two SHAs. - # Coupled to the reviewer's login: if that ever changes the count silently drops - # to 0 and every round looks like the first, hence the warning below. - CYCLE_JQ='[.[][] | select(.user.login == "claude[bot]") | .commit_id] | unique | length' - # Only consulted when CYCLE is 0; see the warning below. Kept in its own variable - # so tests/review-cycle-test.sh can assert it against the fixtures. - DRIFT_JQ='any(.[][]; .user.type == "Bot")' - # Never fail the review over the cycle number; degrade to 1, but say so. gh - # writes its error body to stdout, so an unguarded pipe into jq aborts the step - # under `bash -e` and skips the failure-notification step below. - # CTX_WARNINGS collects the degradations the *model* has to know about, as opposed - # to the ones only an operator cares about. The distinction is whether the fallback - # is blank or is an assertion: "Could not read the diff." is visibly missing data, - # but "REVIEW CYCLE: 1" and "No prior review comments." are claims, and a failed - # read makes them false ones. - WARN_FILE="${RUNNER_TEMP}/ctx-warnings.md" - : > "$WARN_FILE" - if ! REVIEWS=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/reviews" --paginate); then - echo "::warning::Could not read prior reviews; treating this as review cycle 1." - { - echo "- The prior reviews could not be read, so the REVIEW CYCLE number in this" - echo " prompt may be wrong: it defaults to 1. If this is not really your first" - echo " review, treat the cycle ladder as unknown, and do not take the cycle" - echo " number as evidence that nothing was raised before." - } >> "$WARN_FILE" - REVIEWS='' - fi - CYCLE=$(printf '%s' "$REVIEWS" | jq -s "$CYCLE_JQ" 2>/dev/null) || CYCLE='' - if [ -z "$CYCLE" ]; then - echo "::warning::Could not parse prior reviews; treating this as review cycle 1." - CYCLE=0 - elif [ "$CYCLE" -eq 0 ] && printf '%s' "$REVIEWS" \ - | jq -e -s "$DRIFT_JQ" >/dev/null 2>&1; then - # claude[bot] is the only bot that submits reviews across the org (598 of 598 - # sampled), so bot reviews that the login filter did not count mean the - # reviewer's identity moved and the counter has silently pinned at 1. - echo "::warning::Bot reviews exist but none matched the reviewer login; the review cycle counter is stale." - fi - echo "review_cycle=$((CYCLE + 1))" >> $GITHUB_OUTPUT - - # Same guard as the counter above: unguarded `gh api | jq` aborts the step, and a - # failure here *skips* the review step, so the notify step's failure check never - # fires and the PR gets no review and no explanation. - COMMENTS_OK=1 - if ! COMMENTS=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/comments" --paginate); then - COMMENTS_OK=0 - echo "::warning::Could not read prior review comments; reviewing without them." - { - echo "- The prior inline review comments could not be read. That block is empty" - echo " because the fetch failed, not because there were none. Do not conclude" - echo " that no feedback was given; read the threads with gh pr view before" - echo " re-raising anything." - } >> "$WARN_FILE" - COMMENTS='' - fi - # "No prior review comments." is only true when the fetch worked and returned - # none. Saying it after a failed fetch is the same false claim as an empty CI block - # reading as a green one, and it is the claim the cycle ladder acts on. - if [ "$COMMENTS_OK" -eq 0 ]; then - THREADS='Unavailable: the prior inline review comments could not be read. This block is empty because the fetch failed, not because there were none.' - else - THREADS=$(printf '%s' "$COMMENTS" | jq -s -r ' - (add // []) | sort_by(.created_at) | - if length == 0 then "No prior review comments." - else .[] | - "---", - "Author: \(.user.login)", - "File: \(.path)", - (if .line then "Line: \(.line)" else empty end), - (if .in_reply_to_id then "Reply to #\(.in_reply_to_id)" else "Thread #\(.id)" end), - "", - ((.body // "")[0:3000]) - end - ') || THREADS='Unavailable: the prior inline review comments could not be parsed.' - fi - - # The prompt wraps both blocks below in and - # and tells the reviewer to treat their contents as data. A PR body, a diff hunk, - # or a CI log containing the closing tag ends the block early, and everything the - # author wrote after it lands *outside* the marked region, where it reads as - # prompt. The tags are fixed strings, so neutralising them is complete: there is - # no other spelling the model parses as the same delimiter. - # perl, not sed: this has to be case-insensitive and whitespace-tolerant, and BSD - # sed has no case-insensitive substitute flag, so a sed version would either be a - # GNU-only `I` flag or twenty spelled-out character classes. perl ships on every - # runner image. ``, `` and `< / pr_context foo="1">` all - # read as the same delimiter to a model, so matching the shape is the only version - # of this that is not walked around by whitespace. - strip_block_tags() { - perl -pe 's{< \s* /? \s* (?: pr_context | prior_review_comments ) [^>]* >}{[block tag removed]}gix' - } - - THREADS_FILE="${RUNNER_TEMP}/threads.md" - printf '%s\n' "$THREADS" > "$THREADS_FILE" - cap_file "$THREADS_FILE" "$THREADS_MAX_BYTES" \ - "prior review comments truncated at ${THREADS_MAX_BYTES} bytes; read the rest with gh pr view" - - DELIMITER="REVIEW_CONTEXT_$(openssl rand -hex 16)" - { - echo "threads<<${DELIMITER}" - strip_block_tags < "$THREADS_FILE" - echo "${DELIMITER}" - } >> $GITHUB_OUTPUT - - # Title and body reach the shell through env, never an Actions expression - # interpolation: both are attacker-controlled text and would otherwise be spliced - # into this script. - # - # That expression syntax cannot be written out inside this run block, not even in a - # comment. Actions parses those delimiters in the block's *string value*, comments - # included, and an empty pair is a syntax error that makes the whole workflow - # unparseable -- no jobs, no required check, every PR in the org blocked behind - # "Please close and reopen the PR to trigger this workflow". A YAML comment outside - # a block scalar is safe, because the YAML parser strips it before Actions looks. - # First in the file on purpose: the byte cap keeps the head, so anything the - # reviewer must not miss has to be above the blocks that can grow. - if [ -s "$WARN_FILE" ]; then - { - echo "## Context warnings" - cat "$WARN_FILE" - echo - } >> "$CTX" - fi - - { - echo "## Pull request" - echo "Title: ${PR_TITLE}" - echo "Base branch: ${BASE_REF}" - echo "Head SHA: ${HEAD_SHA}" - echo - echo "### Description" - if [ -n "${PR_BODY}" ]; then printf '%s\n' "${PR_BODY}"; else echo "(no description)"; fi - } >> "$CTX" - - # Each block: read, project, and fall back to a sentence saying what is missing. - # A missing block must read as missing, never as "there are no commits". - COMMITS_JQ='[.[][]] | if length == 0 then "No commits reported." else map("\(.sha[0:8]) \(.commit.message | split("\n")[0])") | join("\n") end' - if COMMITS_JSON=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/commits" --paginate); then - COMMITS=$(printf '%s' "$COMMITS_JSON" | jq -s -r "$COMMITS_JQ" 2>/dev/null) \ - || COMMITS="Could not parse commits." - else - echo "::warning::Could not read commits." - COMMITS="Could not read commits." - fi - { echo; echo "## Commits"; printf '%s\n' "$COMMITS"; } >> "$CTX" - - # status carries added/modified/removed/renamed, which the raw patch does not spell - # out for renames, and the per-file counts let the reviewer budget its reading. - # Every field defaulted: a payload missing .additions would otherwise render - # "+null", and the reviewer quotes these numbers back in review comments. - FILES_JQ='[.[][]] | if length == 0 then "No changed files reported." else "\(length) files, +\([.[].additions // 0] | add) -\([.[].deletions // 0] | add)", (.[] | "\(.status // "unknown") +\(.additions // 0)/-\(.deletions // 0) \(.filename // "(unnamed file)")") end' - if FILES_JSON=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/files" --paginate); then - FILES=$(printf '%s' "$FILES_JSON" | jq -s -r "$FILES_JQ" 2>/dev/null) \ - || FILES="Could not parse changed files." - else - echo "::warning::Could not read changed files." - FILES="Could not read changed files." - fi - { echo; echo "## Changed files"; printf '%s\n' "$FILES"; } >> "$CTX" - - # The reviewer cannot run tests -- no dependencies are installed and the allowlist - # would refuse anyway -- but CI already ran them. Whether they passed is the one - # fact it was asserting without evidence. - CHECKS_JQ='(.statusCheckRollup // []) | if length == 0 then "No checks reported." else map(if .__typename == "CheckRun" then "\(.conclusion // .status // "UNKNOWN") \(.workflowName // "") / \(.name // "(unnamed check)")" else "\(.state // "UNKNOWN") \(.context // "status")" end) | sort | join("\n") end' - # Actions check runs carry the job id in detailsUrl; scan rather than capture so a - # non-Actions check with no job id drops out instead of erroring. - FAILING_JOBS_JQ='[(.statusCheckRollup // [])[] | select(.__typename == "CheckRun") | select((.conclusion // "") | test("FAILURE|TIMED_OUT|ACTION_REQUIRED")) | (.detailsUrl // "") | [scan("/job/([0-9]+)")] | flatten | .[0] // empty] | unique | .[0:3] | join(" ")' - if ROLLUP=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json statusCheckRollup); then - CHECKS=$(printf '%s' "$ROLLUP" | jq -r "$CHECKS_JQ" 2>/dev/null) \ - || CHECKS="Could not parse checks." - JOB_IDS=$(printf '%s' "$ROLLUP" | jq -r "$FAILING_JOBS_JQ" 2>/dev/null) || JOB_IDS='' - else - echo "::warning::Could not read check status." - CHECKS="Could not read check status." - JOB_IDS='' - fi - { - echo - echo "## CI checks as of $(date -u +%Y-%m-%dT%H:%M:%SZ)" - echo "This workflow runs on the same push as the rest of CI, so checks are often" - echo "still queued or in progress here. A check that is not reported as passing" - echo "has not passed yet -- it has not necessarily failed." - echo - printf '%s\n' "$CHECKS" - } >> "$CTX" - # Two windows, not a tail. Across five real failed job logs the informative text - # sat immediately above the first ##[error] in four of them (a rustfmt diff, an - # npm parity error, a docker push failure, a build error body). In the fifth -- - # a Django suite whose later steps kept running -- "FAILED (failures=1)" was 670 - # lines above ##[error] and a tail returned docker cleanup, so the summary lines - # get collected separately from wherever they landed. - LOG_SUMMARY_RE='FAILED \(|FAIL: |ERROR: |test result: FAILED|panicked at|Tests:.*failed|Ran [0-9]+ tests?' - for JOB_ID in $JOB_IDS; do - JOB_LOG="${RUNNER_TEMP}/job-${JOB_ID}.log" - { echo; echo "### Failing job ${JOB_ID}"; } >> "$CTX" - if ! fetch_raw "$JOB_LOG" api "repos/${REPO}/actions/jobs/${JOB_ID}/logs"; then - echo "(log unavailable)" >> "$CTX" - continue - fi - SUMMARY=$(grep -E "$LOG_SUMMARY_RE" "$JOB_LOG" | tail -n 20) || SUMMARY='' - if [ -n "$SUMMARY" ]; then - EXCERPT="${RUNNER_TEMP}/job-${JOB_ID}-summary.txt" - printf '%s\n' "$SUMMARY" > "$EXCERPT" - cap_file "$EXCERPT" "$LOG_MAX_BYTES" "summary truncated" - { echo "Summary lines:"; cat "$EXCERPT"; echo; } >> "$CTX" - fi - # The *first* error marker: later steps in the same job add their own, and the - # failing step's is the one with the cause above it. - # - # -m1 rather than `| head -1`: with pipefail, head closing the pipe after one - # line sends grep SIGPIPE, grep exits 141, and the guard below swallows it as - # "no error marker" -- so the window silently becomes a 120-line tail. Whether - # it fires depends on how much grep has buffered, so it misses the small logs - # and hits the ones with a marker per diagnostic (tsc, clippy, eslint), which - # are exactly the logs where the first-error window is worth the most. -m1 stops - # grep at the first match and drops the pipe stage that made the race possible. - ERR_LINE=$(grep -n -m1 '##\[error\]' "$JOB_LOG" | cut -d: -f1) || ERR_LINE='' - if [ -n "$ERR_LINE" ]; then - START=$((ERR_LINE - LOG_WINDOW + 1)) - if [ "$START" -lt 1 ]; then START=1; fi - EXCERPT="${RUNNER_TEMP}/job-${JOB_ID}-window.txt" - sed -n "${START},${ERR_LINE}p" "$JOB_LOG" > "$EXCERPT" - cap_file "$EXCERPT" "$LOG_MAX_BYTES" \ - "log excerpt truncated at ${LOG_MAX_BYTES} bytes" - { - echo "Log lines ${START}-${ERR_LINE}, ending at the first error:" - cat "$EXCERPT" - } >> "$CTX" - else - EXCERPT="${RUNNER_TEMP}/job-${JOB_ID}-tail.txt" - tail -n "$LOG_WINDOW" "$JOB_LOG" > "$EXCERPT" - cap_file "$EXCERPT" "$LOG_MAX_BYTES" \ - "log excerpt truncated at ${LOG_MAX_BYTES} bytes" - { echo "Last ${LOG_WINDOW} log lines:"; cat "$EXCERPT"; } >> "$CTX" - fi - done - - # The diff since the reviewer's own last round. REVIEWS is already in hand for the - # cycle counter, and the last commit_id it submitted against is exactly the base - # for "what changed since I looked". Ordered by submitted_at, not array order, - # because inline comments and the round's verdict are separate review objects. - LAST_REVIEW_JQ='[.[][] | select(.user.login == "claude[bot]") | select(.submitted_at != null) | {commit_id, submitted_at}] | sort_by(.submitted_at) | last | (.commit_id // "")' - LAST_SHA=$(printf '%s' "$REVIEWS" | jq -s -r "$LAST_REVIEW_JQ" 2>/dev/null) || LAST_SHA='' - if [ -n "$LAST_SHA" ] && [ "$LAST_SHA" != "null" ] && [ "$LAST_SHA" != "$HEAD_SHA" ]; then - SINCE_FILE="${RUNNER_TEMP}/since-last-review.diff" - # The compare API, not git: the checkout is fetch-depth 1, so no base branch and - # no prior commit exists locally to diff against. - # - # Ask for the JSON first and only use the diff when the comparison is a clean - # fast-forward. compare/A...B is three-dot, so it diffs from the *merge base* of - # the two, which equals "since A" only while the branch has done nothing but gain - # commits. After a rebase or a squash-and-force-push the old SHA usually stays - # reachable, so this call succeeds and returns the whole PR plus anything the - # rebase pulled in from upstream -- under a heading that says the opposite. A - # reviewer trusting that heading re-raises issues the author already settled, and - # SINCE_MAX can drop the part that genuinely is new. status is "ahead" only for - # the fast-forward case; "diverged" and "behind" fall through to the message. - COMPARE_STATUS_JQ='.status // "unknown"' - SINCE_STATUS=$(gh api "repos/${REPO}/compare/${LAST_SHA}...${HEAD_SHA}" 2>/dev/null \ - | jq -r "$COMPARE_STATUS_JQ" 2>/dev/null) || SINCE_STATUS='unknown' - if [ "$SINCE_STATUS" = "ahead" ] \ - && fetch_raw "$SINCE_FILE" api "repos/${REPO}/compare/${LAST_SHA}...${HEAD_SHA}" \ - -H "Accept: application/vnd.github.diff"; then - # awk, not `wc -l`: wc pads its count with spaces on BSD and the number - # is interpolated into the notice below, not just compared. - SINCE_LINES=$(awk 'END {print NR}' "$SINCE_FILE") - { - echo - echo "## Diff since your last review (${LAST_SHA} to ${HEAD_SHA})" - head -n "$SINCE_MAX" "$SINCE_FILE" - if [ "$SINCE_LINES" -gt "$SINCE_MAX" ]; then - echo "(truncated: first ${SINCE_MAX} of ${SINCE_LINES} lines)" - fi - } >> "$CTX" - else - { - echo - echo "## Diff since your last review" - echo "Unavailable: ${LAST_SHA} does not fast-forward to ${HEAD_SHA}" - echo "(comparison status: ${SINCE_STATUS})." - echo "The branch was rebased or force-pushed, so there is no meaningful" - echo "\"since last review\" diff. Review the full diff below instead, and" - echo "read the prior review comments to see what was already raised." - } >> "$CTX" - fi - fi - - DIFF_FILE="${RUNNER_TEMP}/pr.diff" - if fetch_raw "$DIFF_FILE" pr diff "$PR_NUMBER" --repo "$REPO"; then - DIFF_LINES=$(awk 'END {print NR}' "$DIFF_FILE") - { - echo - echo "## Full diff" - # A heading with nothing under it is a claim, and the wrong one: a fetch that - # succeeded with no body is not the same fact as a PR with no changes, and the - # prompt has just told the reviewer not to re-fetch what it was given. - if [ "$DIFF_LINES" -eq 0 ]; then - echo "The diff came back empty. That is unusual for a pull request; treat it" - echo "as missing rather than as \"nothing changed\" and run gh pr diff." - else - head -n "$DIFF_MAX" "$DIFF_FILE" - if [ "$DIFF_LINES" -gt "$DIFF_MAX" ]; then - echo "(truncated: first ${DIFF_MAX} of ${DIFF_LINES} lines; run gh pr diff for the rest)" - fi - fi - } >> "$CTX" - else - echo "::warning::Could not read the diff." - { echo; echo "## Full diff"; echo "Could not read the diff; run gh pr diff."; } >> "$CTX" - fi - - # Issue comments, not the pull comments above: the PR conversation is a separate - # endpoint from the inline review threads, and only the threads were ever passed. - ISSUE_COMMENTS_JQ='[.[][]] | if length == 0 then "No PR conversation comments." else sort_by(.created_at) | map("--- \(.user.login) at \(.created_at)\n\((.body // "")[0:3000])") | join("\n") end' - if CONVO_JSON=$(gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" --paginate); then - CONVO=$(printf '%s' "$CONVO_JSON" | jq -s -r "$ISSUE_COMMENTS_JQ" 2>/dev/null) \ - || CONVO="Could not parse PR conversation comments." - else - echo "::warning::Could not read PR conversation comments." - CONVO="Could not read PR conversation comments." - fi - { echo; echo "## PR conversation"; printf '%s\n' "$CONVO"; } >> "$CTX" - - # Last resort against an unbounded block -- the per-block caps above should keep - # the file far below this, so hitting it means one of them regressed. - if [ "$(wc -c < "$CTX" | tr -d " ")" -gt "$CTX_MAX_BYTES" ]; then - echo "::warning::Review context exceeded ${CTX_MAX_BYTES} bytes and was truncated." - fi - cap_file "$CTX" "$CTX_MAX_BYTES" "context truncated at ${CTX_MAX_BYTES} bytes" - - CTX_DELIMITER="PR_CONTEXT_$(openssl rand -hex 16)" - { - echo "pr_context<<${CTX_DELIMITER}" - strip_block_tags < "$CTX" - echo "${CTX_DELIMITER}" - } >> $GITHUB_OUTPUT + run: bash .github-workflows/scripts/gather-review-context.sh + # Every value the script reads arrives through env:, never interpolated into it. + # PR_TITLE and PR_BODY were already here for that reason -- they are attacker + # controlled -- and PR_NUMBER and REPO joined them when the script moved out of this + # file, because out there is no interpolation step to interpolate into. env: + PR_NUMBER: ${{ github.event.pull_request.number }} + REPO: ${{ github.repository }} GH_TOKEN: ${{ github.token }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} BASE_REF: ${{ github.event.pull_request.base.ref }} @@ -544,13 +168,24 @@ jobs: # # Reduce execution log -- needs steps.review.outputs.execution_file, empty when skipped # Upload tool usage -- needs steps.tool-usage.outcome == 'success', which is 'skipped' - # Notify on failure -- needs outcome 'failure' or 'cancelled', and this is 'skipped' + # Notify on failure -- carries its own !inputs.dry_run, because it can now also fire + # on a context failure rather than only on a review failure # # So a smoke run posts no review, no comment and no artifact. Everything before this step # still runs against the live API: the app token, the cross-repo prompt checkout, and the # nine context reads with the job's real permissions. + # + # steps.context.outcome, because the context step is continue-on-error and therefore fails + # green. Anything that stops the script running -- a checkout that does not deliver it, a + # renamed path, a sparse pattern that stops matching -- leaves pr_context, threads and + # review_cycle unset, and an empty context is not a neutral one: a blank REVIEW CYCLE and an + # empty prior-comments block read as cycle 1 with nothing raised before. That is a false + # statement to the model, of exactly the kind the script's own guards exist to prevent, and + # it would reach every consumer repo at once. No context, no review. - uses: anthropics/claude-code-action@v1 - if: github.event.pull_request.user.login != 'dependabot[bot]' && !inputs.dry_run + if: >- + github.event.pull_request.user.login != 'dependabot[bot]' && !inputs.dry_run + && steps.context.outcome == 'success' id: review continue-on-error: true with: @@ -643,8 +278,15 @@ jobs: # (~100 review runs/week org-wide); the question is days-old, not quarters. retention-days: 14 + # Also on a context failure, which now skips the review rather than feeding it an empty + # context. Without this clause that path is the silent one: no review, no comment, green + # check -- the shape of failure this workflow keeps being bitten by. !inputs.dry_run because + # a smoke run reaches this step with the review skipped and must never write. - name: Notify on review failure - if: github.event.pull_request.user.login != 'dependabot[bot]' && (steps.review.outcome == 'failure' || steps.review.outcome == 'cancelled') - run: gh pr comment ${{ github.event.pull_request.number }} --body "Automated review unavailable (Claude step failed). Please review manually." + if: >- + github.event.pull_request.user.login != 'dependabot[bot]' && !inputs.dry_run + && (steps.review.outcome == 'failure' || steps.review.outcome == 'cancelled' + || steps.context.outcome == 'failure') + run: gh pr comment ${{ github.event.pull_request.number }} --body "Automated review unavailable (the review step failed, or the context it needs could not be gathered). Please review manually." env: GH_TOKEN: ${{ github.token }} diff --git a/scripts/gather-review-context.sh b/scripts/gather-review-context.sh new file mode 100755 index 0000000..2404ff5 --- /dev/null +++ b/scripts/gather-review-context.sh @@ -0,0 +1,417 @@ +#!/usr/bin/env bash +# +# Gathers the pull request context that the Claude review prompt reads. The "Gather review +# context" step of .github/workflows/claude-pr-review.yml runs this file, which it checks out of +# hotdata-dev/github-workflows alongside the prompt document. +# +# It is a file rather than a `run:` block because Actions parses a `run:` block as one template +# expression and refuses any over 21,000 characters. This script is 20 KB. Inline it left roughly +# 450 characters of headroom -- about six comment lines -- and the change that crossed the line +# stopped every review in the org with "Invalid workflow file". The same parse is what makes an +# empty expression delimiter anywhere in an inline block, a shell comment included, an outage; +# that was the one before it. Neither hazard exists out here. Actions never parses this file, +# bash does, and bash has no opinion about what is in a comment. +# +# The step passes PR_NUMBER, REPO, GH_TOKEN, HEAD_SHA, BASE_REF, PR_TITLE and PR_BODY in through +# `env:`. Nothing is interpolated into this script, so no pull-request-controlled text can arrive +# here as code. +# +# -e and pipefail, which is what the step's `shell: bash` used to supply: this is mostly +# `gh ... | jq` pipelines, and gh writes its error body to stdout, so without pipefail a failed +# fetch feeds its own error text to jq and the block renders whatever jq makes of it instead of +# the guarded fallback sentence. Not -u -- the script was written without one and does not +# assume it. +set -eo pipefail + +: "${PR_NUMBER:?the calling step must set PR_NUMBER}" +: "${REPO:?the calling step must set REPO}" + +# Caps. The median PR reviewed across the org is 161 changed lines and the largest +# in a week was 3,448, so 3,000 patch lines covers the corpus; the cap exists so +# one generated-file PR cannot blow up the prompt. +DIFF_MAX=3000 +SINCE_MAX=2000 +LOG_WINDOW=120 +# Byte budgets, split across the step's two outputs rather than applied to one of +# them. threads is its own output written before the context file, so a cap that +# only measured the context bounded nothing: 400 inline comments rendered 1.1 MB of +# threads on their own. The total here is deliberately far below any plausible +# runner limit -- the largest PR reviewed across the org in a week rendered about +# 150 KB -- because the runner accounts for output size in UTF-16, so a byte count +# here is not the number it checks against. +THREADS_MAX_BYTES=100000 +CTX_MAX_BYTES=200000 +# One job log can be mostly a single line: LOG_WINDOW counts lines and a CI log +# line has no length limit, so a base64 or JSON dump next to the first error marker +# would otherwise consume the whole context ahead of the diff. +LOG_MAX_BYTES=40000 +CTX="${RUNNER_TEMP}/pr-context.md" +: > "$CTX" + +# gh refuses a raw-text body containing ANSI colour unless told to allow escape +# sequences, and a diff or a job log earns an escape byte from any file holding +# terminal output -- this repository's own job-log fixtures do. That refusal and +# its --allow-escape-sequences opt-out arrived together in gh 2.97.0 as a security +# fix; ubuntu-latest ships 2.96.0, where the flag is an unknown-flag error and the +# refusal does not exist either. So every raw fetch tries the flag and falls back +# to the bare call: on 2.96 the first attempt fails and the second succeeds, on +# 2.97+ the first succeeds. Pinning either form breaks on the other, and the +# runner image updates weekly. +# head -c against a *file*, never a pipe: `sed ... | head -c` closes the pipe early +# and SIGPIPE takes the producer down under pipefail, which is the shape that has +# already cost this step its error window once. +cap_file() { + if [ "$(wc -c < "$1" | tr -d " ")" -gt "$2" ]; then + head -c "$2" "$1" > "$1.cut" + mv "$1.cut" "$1" + echo "($3)" >> "$1" + fi +} + +fetch_raw() { + RAW_OUT=$1 + shift + gh "$@" --allow-escape-sequences > "$RAW_OUT" 2>/dev/null && return 0 + gh "$@" > "$RAW_OUT" 2>/dev/null +} + +# Count distinct commits already reviewed, never review state: the org ruleset +# sets dismiss_stale_reviews_on_push, so a push flips a prior APPROVED to +# DISMISSED and a state filter stops matching it. Inline comments each create +# their own COMMENTED review sharing the round's commit_id, so unique commit_id +# == round count, +/-1 when a push lands mid-round and splits it across two SHAs. +# Coupled to the reviewer's login: if that ever changes the count silently drops +# to 0 and every round looks like the first, hence the warning below. +CYCLE_JQ='[.[][] | select(.user.login == "claude[bot]") | .commit_id] | unique | length' +# Only consulted when CYCLE is 0; see the warning below. Kept in its own variable +# so tests/review-cycle-test.sh can assert it against the fixtures. +DRIFT_JQ='any(.[][]; .user.type == "Bot")' +# Never fail the review over the cycle number; degrade to 1, but say so. gh +# writes its error body to stdout, so an unguarded pipe into jq aborts the step +# under `bash -e` and skips the failure-notification step below. +# CTX_WARNINGS collects the degradations the *model* has to know about, as opposed +# to the ones only an operator cares about. The distinction is whether the fallback +# is blank or is an assertion: "Could not read the diff." is visibly missing data, +# but "REVIEW CYCLE: 1" and "No prior review comments." are claims, and a failed +# read makes them false ones. +WARN_FILE="${RUNNER_TEMP}/ctx-warnings.md" +: > "$WARN_FILE" +if ! REVIEWS=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/reviews" --paginate); then + echo "::warning::Could not read prior reviews; treating this as review cycle 1." + { + echo "- The prior reviews could not be read, so the REVIEW CYCLE number in this" + echo " prompt may be wrong: it defaults to 1. If this is not really your first" + echo " review, treat the cycle ladder as unknown, and do not take the cycle" + echo " number as evidence that nothing was raised before." + } >> "$WARN_FILE" + REVIEWS='' +fi +CYCLE=$(printf '%s' "$REVIEWS" | jq -s "$CYCLE_JQ" 2>/dev/null) || CYCLE='' +if [ -z "$CYCLE" ]; then + echo "::warning::Could not parse prior reviews; treating this as review cycle 1." + CYCLE=0 +elif [ "$CYCLE" -eq 0 ] && printf '%s' "$REVIEWS" \ + | jq -e -s "$DRIFT_JQ" >/dev/null 2>&1; then + # claude[bot] is the only bot that submits reviews across the org (598 of 598 + # sampled), so bot reviews that the login filter did not count mean the + # reviewer's identity moved and the counter has silently pinned at 1. + echo "::warning::Bot reviews exist but none matched the reviewer login; the review cycle counter is stale." +fi +echo "review_cycle=$((CYCLE + 1))" >> $GITHUB_OUTPUT + +# Same guard as the counter above: unguarded `gh api | jq` aborts the step, and a +# failure here *skips* the review step, so the notify step's failure check never +# fires and the PR gets no review and no explanation. +COMMENTS_OK=1 +if ! COMMENTS=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/comments" --paginate); then + COMMENTS_OK=0 + echo "::warning::Could not read prior review comments; reviewing without them." + { + echo "- The prior inline review comments could not be read. That block is empty" + echo " because the fetch failed, not because there were none. Do not conclude" + echo " that no feedback was given; read the threads with gh pr view before" + echo " re-raising anything." + } >> "$WARN_FILE" + COMMENTS='' +fi +# "No prior review comments." is only true when the fetch worked and returned +# none. Saying it after a failed fetch is the same false claim as an empty CI block +# reading as a green one, and it is the claim the cycle ladder acts on. +if [ "$COMMENTS_OK" -eq 0 ]; then + THREADS='Unavailable: the prior inline review comments could not be read. This block is empty because the fetch failed, not because there were none.' +else +THREADS=$(printf '%s' "$COMMENTS" | jq -s -r ' + (add // []) | sort_by(.created_at) | + if length == 0 then "No prior review comments." + else .[] | + "---", + "Author: \(.user.login)", + "File: \(.path)", + (if .line then "Line: \(.line)" else empty end), + (if .in_reply_to_id then "Reply to #\(.in_reply_to_id)" else "Thread #\(.id)" end), + "", + ((.body // "")[0:3000]) + end +') || THREADS='Unavailable: the prior inline review comments could not be parsed.' +fi + +# The prompt wraps both blocks below in and +# and tells the reviewer to treat their contents as data. A PR body, a diff hunk, +# or a CI log containing the closing tag ends the block early, and everything the +# author wrote after it lands *outside* the marked region, where it reads as +# prompt. The tags are fixed strings, so neutralising them is complete: there is +# no other spelling the model parses as the same delimiter. +# perl, not sed: this has to be case-insensitive and whitespace-tolerant, and BSD +# sed has no case-insensitive substitute flag, so a sed version would either be a +# GNU-only `I` flag or twenty spelled-out character classes. perl ships on every +# runner image. ``, `` and `< / pr_context foo="1">` all +# read as the same delimiter to a model, so matching the shape is the only version +# of this that is not walked around by whitespace. +strip_block_tags() { + perl -pe 's{< \s* /? \s* (?: pr_context | prior_review_comments ) [^>]* >}{[block tag removed]}gix' +} + +THREADS_FILE="${RUNNER_TEMP}/threads.md" +printf '%s\n' "$THREADS" > "$THREADS_FILE" +cap_file "$THREADS_FILE" "$THREADS_MAX_BYTES" \ + "prior review comments truncated at ${THREADS_MAX_BYTES} bytes; read the rest with gh pr view" + +DELIMITER="REVIEW_CONTEXT_$(openssl rand -hex 16)" +{ + echo "threads<<${DELIMITER}" + strip_block_tags < "$THREADS_FILE" + echo "${DELIMITER}" +} >> $GITHUB_OUTPUT + +# Title and body reach the shell through env, never an Actions expression +# interpolation: both are attacker-controlled text and would otherwise be spliced +# into this script. +# +# That expression syntax cannot be written out inside this run block, not even in a +# comment. Actions parses those delimiters in the block's *string value*, comments +# included, and an empty pair is a syntax error that makes the whole workflow +# unparseable -- no jobs, no required check, every PR in the org blocked behind +# "Please close and reopen the PR to trigger this workflow". A YAML comment outside +# a block scalar is safe, because the YAML parser strips it before Actions looks. +# First in the file on purpose: the byte cap keeps the head, so anything the +# reviewer must not miss has to be above the blocks that can grow. +if [ -s "$WARN_FILE" ]; then + { + echo "## Context warnings" + cat "$WARN_FILE" + echo + } >> "$CTX" +fi + +{ + echo "## Pull request" + echo "Title: ${PR_TITLE}" + echo "Base branch: ${BASE_REF}" + echo "Head SHA: ${HEAD_SHA}" + echo + echo "### Description" + if [ -n "${PR_BODY}" ]; then printf '%s\n' "${PR_BODY}"; else echo "(no description)"; fi +} >> "$CTX" + +# Each block: read, project, and fall back to a sentence saying what is missing. +# A missing block must read as missing, never as "there are no commits". +COMMITS_JQ='[.[][]] | if length == 0 then "No commits reported." else map("\(.sha[0:8]) \(.commit.message | split("\n")[0])") | join("\n") end' +if COMMITS_JSON=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/commits" --paginate); then + COMMITS=$(printf '%s' "$COMMITS_JSON" | jq -s -r "$COMMITS_JQ" 2>/dev/null) \ + || COMMITS="Could not parse commits." +else + echo "::warning::Could not read commits." + COMMITS="Could not read commits." +fi +{ echo; echo "## Commits"; printf '%s\n' "$COMMITS"; } >> "$CTX" + +# status carries added/modified/removed/renamed, which the raw patch does not spell +# out for renames, and the per-file counts let the reviewer budget its reading. +# Every field defaulted: a payload missing .additions would otherwise render +# "+null", and the reviewer quotes these numbers back in review comments. +FILES_JQ='[.[][]] | if length == 0 then "No changed files reported." else "\(length) files, +\([.[].additions // 0] | add) -\([.[].deletions // 0] | add)", (.[] | "\(.status // "unknown") +\(.additions // 0)/-\(.deletions // 0) \(.filename // "(unnamed file)")") end' +if FILES_JSON=$(gh api "repos/${REPO}/pulls/${PR_NUMBER}/files" --paginate); then + FILES=$(printf '%s' "$FILES_JSON" | jq -s -r "$FILES_JQ" 2>/dev/null) \ + || FILES="Could not parse changed files." +else + echo "::warning::Could not read changed files." + FILES="Could not read changed files." +fi +{ echo; echo "## Changed files"; printf '%s\n' "$FILES"; } >> "$CTX" + +# The reviewer cannot run tests -- no dependencies are installed and the allowlist +# would refuse anyway -- but CI already ran them. Whether they passed is the one +# fact it was asserting without evidence. +CHECKS_JQ='(.statusCheckRollup // []) | if length == 0 then "No checks reported." else map(if .__typename == "CheckRun" then "\(.conclusion // .status // "UNKNOWN") \(.workflowName // "") / \(.name // "(unnamed check)")" else "\(.state // "UNKNOWN") \(.context // "status")" end) | sort | join("\n") end' +# Actions check runs carry the job id in detailsUrl; scan rather than capture so a +# non-Actions check with no job id drops out instead of erroring. +FAILING_JOBS_JQ='[(.statusCheckRollup // [])[] | select(.__typename == "CheckRun") | select((.conclusion // "") | test("FAILURE|TIMED_OUT|ACTION_REQUIRED")) | (.detailsUrl // "") | [scan("/job/([0-9]+)")] | flatten | .[0] // empty] | unique | .[0:3] | join(" ")' +if ROLLUP=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json statusCheckRollup); then + CHECKS=$(printf '%s' "$ROLLUP" | jq -r "$CHECKS_JQ" 2>/dev/null) \ + || CHECKS="Could not parse checks." + JOB_IDS=$(printf '%s' "$ROLLUP" | jq -r "$FAILING_JOBS_JQ" 2>/dev/null) || JOB_IDS='' +else + echo "::warning::Could not read check status." + CHECKS="Could not read check status." + JOB_IDS='' +fi +{ + echo + echo "## CI checks as of $(date -u +%Y-%m-%dT%H:%M:%SZ)" + echo "This workflow runs on the same push as the rest of CI, so checks are often" + echo "still queued or in progress here. A check that is not reported as passing" + echo "has not passed yet -- it has not necessarily failed." + echo + printf '%s\n' "$CHECKS" +} >> "$CTX" +# Two windows, not a tail. Across five real failed job logs the informative text +# sat immediately above the first ##[error] in four of them (a rustfmt diff, an +# npm parity error, a docker push failure, a build error body). In the fifth -- +# a Django suite whose later steps kept running -- "FAILED (failures=1)" was 670 +# lines above ##[error] and a tail returned docker cleanup, so the summary lines +# get collected separately from wherever they landed. +LOG_SUMMARY_RE='FAILED \(|FAIL: |ERROR: |test result: FAILED|panicked at|Tests:.*failed|Ran [0-9]+ tests?' +for JOB_ID in $JOB_IDS; do + JOB_LOG="${RUNNER_TEMP}/job-${JOB_ID}.log" + { echo; echo "### Failing job ${JOB_ID}"; } >> "$CTX" + if ! fetch_raw "$JOB_LOG" api "repos/${REPO}/actions/jobs/${JOB_ID}/logs"; then + echo "(log unavailable)" >> "$CTX" + continue + fi + SUMMARY=$(grep -E "$LOG_SUMMARY_RE" "$JOB_LOG" | tail -n 20) || SUMMARY='' + if [ -n "$SUMMARY" ]; then + EXCERPT="${RUNNER_TEMP}/job-${JOB_ID}-summary.txt" + printf '%s\n' "$SUMMARY" > "$EXCERPT" + cap_file "$EXCERPT" "$LOG_MAX_BYTES" "summary truncated" + { echo "Summary lines:"; cat "$EXCERPT"; echo; } >> "$CTX" + fi + # The *first* error marker: later steps in the same job add their own, and the + # failing step's is the one with the cause above it. + # + # -m1 rather than `| head -1`: with pipefail, head closing the pipe after one + # line sends grep SIGPIPE, grep exits 141, and the guard below swallows it as + # "no error marker" -- so the window silently becomes a 120-line tail. Whether + # it fires depends on how much grep has buffered, so it misses the small logs + # and hits the ones with a marker per diagnostic (tsc, clippy, eslint), which + # are exactly the logs where the first-error window is worth the most. -m1 stops + # grep at the first match and drops the pipe stage that made the race possible. + ERR_LINE=$(grep -n -m1 '##\[error\]' "$JOB_LOG" | cut -d: -f1) || ERR_LINE='' + if [ -n "$ERR_LINE" ]; then + START=$((ERR_LINE - LOG_WINDOW + 1)) + if [ "$START" -lt 1 ]; then START=1; fi + EXCERPT="${RUNNER_TEMP}/job-${JOB_ID}-window.txt" + sed -n "${START},${ERR_LINE}p" "$JOB_LOG" > "$EXCERPT" + cap_file "$EXCERPT" "$LOG_MAX_BYTES" \ + "log excerpt truncated at ${LOG_MAX_BYTES} bytes" + { + echo "Log lines ${START}-${ERR_LINE}, ending at the first error:" + cat "$EXCERPT" + } >> "$CTX" + else + EXCERPT="${RUNNER_TEMP}/job-${JOB_ID}-tail.txt" + tail -n "$LOG_WINDOW" "$JOB_LOG" > "$EXCERPT" + cap_file "$EXCERPT" "$LOG_MAX_BYTES" \ + "log excerpt truncated at ${LOG_MAX_BYTES} bytes" + { echo "Last ${LOG_WINDOW} log lines:"; cat "$EXCERPT"; } >> "$CTX" + fi +done + +# The diff since the reviewer's own last round. REVIEWS is already in hand for the +# cycle counter, and the last commit_id it submitted against is exactly the base +# for "what changed since I looked". Ordered by submitted_at, not array order, +# because inline comments and the round's verdict are separate review objects. +LAST_REVIEW_JQ='[.[][] | select(.user.login == "claude[bot]") | select(.submitted_at != null) | {commit_id, submitted_at}] | sort_by(.submitted_at) | last | (.commit_id // "")' +LAST_SHA=$(printf '%s' "$REVIEWS" | jq -s -r "$LAST_REVIEW_JQ" 2>/dev/null) || LAST_SHA='' +if [ -n "$LAST_SHA" ] && [ "$LAST_SHA" != "null" ] && [ "$LAST_SHA" != "$HEAD_SHA" ]; then + SINCE_FILE="${RUNNER_TEMP}/since-last-review.diff" + # The compare API, not git: the checkout is fetch-depth 1, so no base branch and + # no prior commit exists locally to diff against. + # + # Ask for the JSON first and only use the diff when the comparison is a clean + # fast-forward. compare/A...B is three-dot, so it diffs from the *merge base* of + # the two, which equals "since A" only while the branch has done nothing but gain + # commits. After a rebase or a squash-and-force-push the old SHA usually stays + # reachable, so this call succeeds and returns the whole PR plus anything the + # rebase pulled in from upstream -- under a heading that says the opposite. A + # reviewer trusting that heading re-raises issues the author already settled, and + # SINCE_MAX can drop the part that genuinely is new. status is "ahead" only for + # the fast-forward case; "diverged" and "behind" fall through to the message. + COMPARE_STATUS_JQ='.status // "unknown"' + SINCE_STATUS=$(gh api "repos/${REPO}/compare/${LAST_SHA}...${HEAD_SHA}" 2>/dev/null \ + | jq -r "$COMPARE_STATUS_JQ" 2>/dev/null) || SINCE_STATUS='unknown' + if [ "$SINCE_STATUS" = "ahead" ] \ + && fetch_raw "$SINCE_FILE" api "repos/${REPO}/compare/${LAST_SHA}...${HEAD_SHA}" \ + -H "Accept: application/vnd.github.diff"; then + # awk, not `wc -l`: wc pads its count with spaces on BSD and the number + # is interpolated into the notice below, not just compared. + SINCE_LINES=$(awk 'END {print NR}' "$SINCE_FILE") + { + echo + echo "## Diff since your last review (${LAST_SHA} to ${HEAD_SHA})" + head -n "$SINCE_MAX" "$SINCE_FILE" + if [ "$SINCE_LINES" -gt "$SINCE_MAX" ]; then + echo "(truncated: first ${SINCE_MAX} of ${SINCE_LINES} lines)" + fi + } >> "$CTX" + else + { + echo + echo "## Diff since your last review" + echo "Unavailable: ${LAST_SHA} does not fast-forward to ${HEAD_SHA}" + echo "(comparison status: ${SINCE_STATUS})." + echo "The branch was rebased or force-pushed, so there is no meaningful" + echo "\"since last review\" diff. Review the full diff below instead, and" + echo "read the prior review comments to see what was already raised." + } >> "$CTX" + fi +fi + +DIFF_FILE="${RUNNER_TEMP}/pr.diff" +if fetch_raw "$DIFF_FILE" pr diff "$PR_NUMBER" --repo "$REPO"; then + DIFF_LINES=$(awk 'END {print NR}' "$DIFF_FILE") + { + echo + echo "## Full diff" + # A heading with nothing under it is a claim, and the wrong one: a fetch that + # succeeded with no body is not the same fact as a PR with no changes, and the + # prompt has just told the reviewer not to re-fetch what it was given. + if [ "$DIFF_LINES" -eq 0 ]; then + echo "The diff came back empty. That is unusual for a pull request; treat it" + echo "as missing rather than as \"nothing changed\" and run gh pr diff." + else + head -n "$DIFF_MAX" "$DIFF_FILE" + if [ "$DIFF_LINES" -gt "$DIFF_MAX" ]; then + echo "(truncated: first ${DIFF_MAX} of ${DIFF_LINES} lines; run gh pr diff for the rest)" + fi + fi + } >> "$CTX" +else + echo "::warning::Could not read the diff." + { echo; echo "## Full diff"; echo "Could not read the diff; run gh pr diff."; } >> "$CTX" +fi + +# Issue comments, not the pull comments above: the PR conversation is a separate +# endpoint from the inline review threads, and only the threads were ever passed. +ISSUE_COMMENTS_JQ='[.[][]] | if length == 0 then "No PR conversation comments." else sort_by(.created_at) | map("--- \(.user.login) at \(.created_at)\n\((.body // "")[0:3000])") | join("\n") end' +if CONVO_JSON=$(gh api "repos/${REPO}/issues/${PR_NUMBER}/comments" --paginate); then + CONVO=$(printf '%s' "$CONVO_JSON" | jq -s -r "$ISSUE_COMMENTS_JQ" 2>/dev/null) \ + || CONVO="Could not parse PR conversation comments." +else + echo "::warning::Could not read PR conversation comments." + CONVO="Could not read PR conversation comments." +fi +{ echo; echo "## PR conversation"; printf '%s\n' "$CONVO"; } >> "$CTX" + +# Last resort against an unbounded block -- the per-block caps above should keep +# the file far below this, so hitting it means one of them regressed. +if [ "$(wc -c < "$CTX" | tr -d " ")" -gt "$CTX_MAX_BYTES" ]; then + echo "::warning::Review context exceeded ${CTX_MAX_BYTES} bytes and was truncated." +fi +cap_file "$CTX" "$CTX_MAX_BYTES" "context truncated at ${CTX_MAX_BYTES} bytes" + +CTX_DELIMITER="PR_CONTEXT_$(openssl rand -hex 16)" +{ + echo "pr_context<<${CTX_DELIMITER}" + strip_block_tags < "$CTX" + echo "${CTX_DELIMITER}" +} >> $GITHUB_OUTPUT diff --git a/tests/context-step-test.sh b/tests/context-step-test.sh index 6874eb9..7dbd2b1 100755 --- a/tests/context-step-test.sh +++ b/tests/context-step-test.sh @@ -6,61 +6,56 @@ # check, and until it was made continue-on-error a non-zero exit here skipped the review # *and* the notify step, leaving the PR with no review and no explanation. # -# `bash -e -o pipefail` is what the runner uses, and it is unforgiving of the shapes this -# script is full of: `grep | tail` finding nothing, `$(( ))` on an empty variable, a `[ ]` -# test as the last command of a branch. Each of those aborts the step. So the script is run -# here exactly as the runner runs it, with the failure modes injected: an endpoint that -# 404s, a log with no error marker, a diff past the truncation cap. +# -e and pipefail are unforgiving of the shapes this script is full of: `grep | tail` finding +# nothing, `$(( ))` on an empty variable, a `[ ]` test as the last command of a branch. Each of +# those aborts the step. So the script is invoked the way the workflow invokes it -- plain +# `bash `, no flags from outside, so the `set -eo pipefail` inside the script is itself +# under test -- with the failure modes injected: an endpoint that 404s, a log with no error +# marker, a diff past the truncation cap. set -euo pipefail cd "$(dirname "$0")/.." WORKFLOW=.github/workflows/claude-pr-review.yml +CONTEXT_SCRIPT=scripts/gather-review-context.sh WORK=$(mktemp -d) trap 'rm -rf "$WORK"' EXIT failures=0 -# The step script, extracted from the workflow rather than copied: everything indented -# inside its `run: |` block, dedented, with the two ${{ }} expressions replaced by the -# variables the stub reads. Anything else interpolated into this script would be missed -# here, which is itself worth knowing -- ${{ }} in a run block is how shell injection gets -# in, and the env: block is where PR title and body are deliberately kept. -extract_step() { - awk ' - /^ - name: Gather review context$/ { in_step = 1 } - in_step && /^ run: \|$/ { in_run = 1; next } - in_run && /^ [a-z]/ { exit } - in_run { sub(/^ /, ""); print } - ' "$WORKFLOW" -} +# The script the workflow runs, run directly. It used to be scraped out of a `run: |` block and +# rewritten by sed, because that is where it lived; now it is a file, so there is no extraction +# to drift and no copy to diverge from the shipped thing. +if [ ! -f "$CONTEXT_SCRIPT" ]; then + echo "FAIL: $CONTEXT_SCRIPT is missing" >&2 + exit 1 +fi +if [ "$(wc -l < "$CONTEXT_SCRIPT")" -lt 100 ]; then + echo "FAIL: $CONTEXT_SCRIPT is only $(wc -l < "$CONTEXT_SCRIPT") lines" >&2 + exit 1 +fi -# One definition, shared by the two sed patterns and the grep below, so the thing being -# substituted and the thing being forbidden cannot drift apart. -EXPR_OPEN="\${$(printf '%s' '{')" -extract_step \ - | sed -e "s/${EXPR_OPEN} github.event.pull_request.number }}/\"\$STUB_PR\"/g" \ - -e "s/${EXPR_OPEN} github.repository }}/\"\$STUB_REPO\"/g" \ - > "$WORK/step.sh" - -if [ "$(wc -l < "$WORK/step.sh")" -lt 100 ]; then - echo "FAIL: extracted step script is only $(wc -l < "$WORK/step.sh") lines; the awk" \ - "extraction no longer matches the workflow" >&2 +# The workflow must actually run it. A green suite over an orphaned script is the failure this +# guards against: the file would be exercised here and never reached in production. +if ! grep -qF "$CONTEXT_SCRIPT" "$WORKFLOW"; then + echo "FAIL: $WORKFLOW does not reference $CONTEXT_SCRIPT" >&2 exit 1 fi -# No Actions expression delimiter may survive anywhere in the extracted script -- not in -# code, and not in a comment either. The comment exemption this check used to carry is what -# shipped a broken workflow to every repo in the org: a shell comment reading "never a -# ${OPEN} interpolation" parses as an *empty expression*, which Actions rejects outright, so -# the workflow never started, no required check ever reported, and every PR in the org sat -# behind "Please close and reopen the PR to trigger this workflow". bash does not care what -# is in a comment; the Actions expression parser does. -if grep -q "$EXPR_OPEN" "$WORK/step.sh"; then - echo "FAIL: an Actions expression delimiter survives in the extracted step script." >&2 - echo " Either this test needs to substitute it, or -- if it is inside a comment --" >&2 - echo " the comment has to stop spelling the delimiter out." >&2 - grep -n "$EXPR_OPEN" "$WORK/step.sh" >&2 + +# No Actions expression delimiter may appear in the script. Out here it is inert -- Actions never +# parses this file -- but an interpolation is the one way pull-request text could reach the script +# as code, and the delimiter appearing at all would mean someone had put the script back under the +# template parser, where an empty pair is an outage. That is what shipped a broken workflow to +# every repo in the org: a shell comment reading "never a ${OPEN} interpolation" parses as an +# *empty expression*, which Actions rejects outright, so the workflow never started, no required +# check ever reported, and every PR in the org sat behind "Please close and reopen the PR to +# trigger this workflow". +EXPR_OPEN="\${$(printf '%s' '{')" +if grep -q "$EXPR_OPEN" "$CONTEXT_SCRIPT"; then + echo "FAIL: an Actions expression delimiter appears in $CONTEXT_SCRIPT." >&2 + echo " Values reach this script through the step's env:, never by interpolation." >&2 + grep -n "$EXPR_OPEN" "$CONTEXT_SCRIPT" >&2 exit 1 fi @@ -201,8 +196,8 @@ run_step() { RUNNER_TEMP="$WORK/rt" \ GITHUB_OUTPUT="$WORK/out.txt" \ STUB_FIXTURES="$PWD/tests/fixtures" \ - STUB_PR=172 \ - STUB_REPO=hotdata-dev/dlthubworker \ + PR_NUMBER=172 \ + REPO=hotdata-dev/dlthubworker \ STUB_JOB_LOG="${STUB_JOB_LOG:-job-log-django.txt}" \ GH_VERSION="${GH_VERSION:-2.96}" \ COMPARE_STATUS="${COMPARE_STATUS:-ahead}" \ @@ -214,7 +209,7 @@ run_step() { BASE_REF=main \ PR_TITLE="${PR_TITLE:-feat(filesystem): continuous sync}" \ PR_BODY="${PR_BODY:-Adds a watermark. \`\$(touch /tmp/pwned)\` and \${{ github.token }} are literal text here.}" \ - bash --noprofile --norc -eo pipefail "$WORK/step.sh" > "$WORK/step.out" 2>&1 + bash "$CONTEXT_SCRIPT" > "$WORK/step.out" 2>&1 STEP_STATUS=$? set -e awk '/^pr_context< -- pull a single-quoted jq program out of the workflow +# extract_jq -- pull a single-quoted jq program out of the shipped shell extract_jq() { - local name=$1 prog - prog=$(sed -n "s/^ *$name='\(.*\)'\$/\1/p" "$WORKFLOW") - if [ -z "$prog" ]; then - echo "FAIL: no $name='...' assignment found in $WORKFLOW" >&2 + local name=$1 prog found=() src + for src in "${JQ_SOURCES[@]}"; do + if [ -n "$(sed -n "s/^ *$name='\(.*\)'\$/\1/p" "$src")" ]; then + found+=("$src") + fi + done + if [ "${#found[@]}" -eq 0 ]; then + echo "FAIL: no $name='...' assignment found in ${JQ_SOURCES[*]}" >&2 + exit 1 + fi + if [ "${#found[@]}" -gt 1 ]; then + echo "FAIL: $name is assigned in more than one of ${found[*]}" >&2 exit 1 fi + prog=$(sed -n "s/^ *$name='\(.*\)'\$/\1/p" "${found[0]}") if [ "$(printf '%s\n' "$prog" | wc -l)" -ne 1 ]; then - echo "FAIL: more than one $name assignment in $WORKFLOW:" >&2 + echo "FAIL: more than one $name assignment in ${found[0]}:" >&2 printf '%s\n' "$prog" >&2 exit 1 fi diff --git a/tests/workflow-lint-test.sh b/tests/workflow-lint-test.sh index 703dad4..88f13ed 100755 --- a/tests/workflow-lint-test.sh +++ b/tests/workflow-lint-test.sh @@ -26,6 +26,7 @@ cd "$(dirname "$0")/.." failures=0 WORKFLOW_FILE=.github/workflows/claude-pr-review.yml TESTS_FILE=.github/workflows/tests.yml +CONTEXT_SCRIPT=scripts/gather-review-context.sh # The delimiter, assembled rather than written, so this file does not trip its own scan. OPEN="\${$(printf '%s' '{')" @@ -131,19 +132,13 @@ fi # # The table is endpoint-shape to permission. It is deliberately coarse; the point is that # adding a new API call to the step forces a decision about its permission. -step_script=$(awk ' - /^ - name: Gather review context$/ { in_step = 1 } - in_step && /^ run: \|$/ { in_run = 1; next } - in_run && /^ [a-z]/ { exit } - in_run { print } -' "$WORKFLOW_FILE") -# Fail loudly if the extraction drifted. check_permission returns early when the pattern is -# absent from the script, so an empty step_script silently turns all six checks into no-ops -- -# in the one file whose purpose is catching a permission that is silently missing. (declared -# fails safe: empty means every check reports FAIL.) +step_script=$(cat "$CONTEXT_SCRIPT" 2>/dev/null || true) +# Fail loudly if the script went missing or shrank to nothing. check_permission returns early +# when the pattern is absent from the script, so an empty step_script silently turns all six +# checks into no-ops -- in the one file whose purpose is catching a permission that is silently +# missing. (declared fails safe: empty means every check reports FAIL.) if [ "$(printf '%s\n' "$step_script" | wc -l)" -lt 100 ]; then - echo "FAIL the context-step extraction no longer matches $WORKFLOW_FILE;" \ - "the permission table proves nothing" + echo "FAIL $CONTEXT_SCRIPT is missing or too short; the permission table proves nothing" failures=$((failures + 1)) fi declared=$(awk '/^ permissions:$/ { p = 1; next } p && /^ [a-z-]+:/ { print $1 } p && /^ [a-z]/ { exit }' \ @@ -167,6 +162,31 @@ check_permission "statusCheckRollup" statuses "the StatusContext half of the CI check_permission "/compare/" contents "the since-last-review comparison" check_permission "/pulls/" pull-requests "the PR reads" +# The context step is continue-on-error, so everything it can fail at -- a checkout that does not +# deliver the script, a bad path, a rename that stops matching the sparse pattern -- leaves the run +# green with pr_context, threads and review_cycle all unset. Ungated, the review step then runs on +# that: a blank REVIEW CYCLE and an empty prior-comments block read as cycle 1 with nothing raised +# before, which is a false statement rather than a missing one, and it reaches every consumer repo +# at once. The script's own guards exist to stop exactly that claim, and they cannot help if the +# script never ran. So the review must be gated on the context step having succeeded. +# The condition is a folded block, so collect its continuation lines too: everything indented +# past the `if:` key, up to the next key of the step. +review_gate=$(awk '/^ - uses: anthropics\/claude-code-action/ { found = 1; next } + found && /^ if:/ { print; in_if = 1; next } + in_if && /^ / { print; next } + in_if { exit }' "$WORKFLOW_FILE") +if [ -z "$review_gate" ]; then + echo "FAIL could not find the review step's if: in $WORKFLOW_FILE; this check proves nothing" + failures=$((failures + 1)) +elif ! printf '%s\n' "$review_gate" | grep -q "steps\.context\.outcome == 'success'"; then + echo "FAIL the review step does not require the context step to have succeeded, so a failed" + echo " context read sends the model an empty context that reads as a clean cycle 1:" + printf '%s\n' "$review_gate" | sed 's/^/ /' + failures=$((failures + 1)) +else + echo "ok the review step runs only when the context step succeeded" +fi + # The table above forces a new API call in the context step to declare its permission on the # review job. That does nothing for the smoke job in tests.yml, which calls the review workflow # and has to grant the same set by hand: a caller cannot give a reusable workflow more than it