diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index c9c10d8..3d32195 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -394,4 +394,8 @@ jobs: if: always() env: FINDINGS: ${{ steps.review.outputs.structured_output }} + # For naming the real cause when there is no output — a reviewer + # killed by an API error (usage limit, expired key) says so in the + # execution log's result entry. + EXECUTION_FILE: ${{ steps.review.outputs.execution_file }} run: node .claude-review/tooling/review/check-review-threshold.mjs diff --git a/review/check-review-threshold.mjs b/review/check-review-threshold.mjs index c01a116..ac71bc0 100644 --- a/review/check-review-threshold.mjs +++ b/review/check-review-threshold.mjs @@ -17,12 +17,64 @@ * — would go on suppressing findings from every later push. */ +import { readFileSync } from 'node:fs'; + const BLOCKING = new Set(['BLOCKER', 'HIGH', 'MEDIUM']); +/** + * Workflow commands are line-oriented, and every field below is written by the + * model. A newline in a summary ends the annotation and hands what follows to + * the runner as a fresh line — so a finding whose text happens to contain + * `::error::`, or `::stop-commands::`, is a finding that writes the log rather + * than appearing in it. The mundane version of the same bug is more likely: + * a summary with a line break in it silently loses everything after it. + * + * `maxLength: 200` in the schema bounds how much text arrives, not which bytes, + * and nothing validates the payload against that schema before this script + * reads it anyway. These are GitHub's own escapes: `%` first, or it would + * re-escape the escapes. + */ +const escapeData = (value) => + String(value).replace(/%/g, '%25').replace(/\r/g, '%0D').replace(/\n/g, '%0A'); + +/** Property values additionally end at `:` or `,`, which separate the properties. */ +const escapeProperty = (value) => escapeData(value).replace(/:/g, '%3A').replace(/,/g, '%2C'); + const raw = process.env.FINDINGS?.trim(); +/** + * When the reviewer died before producing output, the execution log usually + * names why — a usage-limit or auth API error beats the generic guidance + * below, which sends the reader hunting through the run for a cause the + * result entry already states. + */ +const terminalApiError = () => { + const file = process.env.EXECUTION_FILE?.trim(); + if (!file) return null; + try { + const messages = JSON.parse(readFileSync(file, 'utf8')); + const result = messages.findLast((m) => m?.type === 'result'); + if (result?.terminal_reason === 'api_error' || result?.api_error_status) { + return String(result.result || `API error (status ${result.api_error_status})`); + } + } catch { + // Unreadable log: fall through to the generic message. + } + return null; +}; + /** Anything that is not a clean, parseable result is a failure, never a pass. */ if (!raw) { + const apiError = terminalApiError(); + if (apiError) { + console.log(`::error::the review never ran — ${escapeData(apiError)}`); + console.log( + 'The reviewer terminated on an API error before producing output, so there is ' + + 'nothing to score and this fails closed. This is not a finding in the PR: fix ' + + 'the API-side condition (usage limit, expired key, outage) and re-run the job.' + ); + process.exit(1); + } console.log('::error::the review produced no structured output'); console.log( 'A review that reports nothing must not read as a review that found nothing, ' + @@ -56,25 +108,6 @@ try { process.exit(1); } -/** - * Workflow commands are line-oriented, and every field below is written by the - * model. A newline in a summary ends the annotation and hands what follows to - * the runner as a fresh line — so a finding whose text happens to contain - * `::error::`, or `::stop-commands::`, is a finding that writes the log rather - * than appearing in it. The mundane version of the same bug is more likely: - * a summary with a line break in it silently loses everything after it. - * - * `maxLength: 200` in the schema bounds how much text arrives, not which bytes, - * and nothing validates the payload against that schema before this script - * reads it anyway. These are GitHub's own escapes: `%` first, or it would - * re-escape the escapes. - */ -const escapeData = (value) => - String(value).replace(/%/g, '%25').replace(/\r/g, '%0D').replace(/\n/g, '%0A'); - -/** Property values additionally end at `:` or `,`, which separate the properties. */ -const escapeProperty = (value) => escapeData(value).replace(/:/g, '%3A').replace(/,/g, '%2C'); - const blocking = findings.filter((f) => BLOCKING.has(f.severity)); for (const f of findings) {