diff --git a/.github/workflows/claude-review.yml b/.github/workflows/claude-review.yml index 6dd58dece..2e0d1f11f 100644 --- a/.github/workflows/claude-review.yml +++ b/.github/workflows/claude-review.yml @@ -16,7 +16,7 @@ on: workflow_call: inputs: review-skill: - description: Name of the local review skill to invoke (a .claude/skills/ in the caller repo). It just needs to output the review; this workflow captures that as structured output and publishes it. + description: Name of the local review skill to invoke (a .claude/skills/ in the caller repo). It just needs to output the review as its final message; this workflow reads that from the execution log and publishes it. type: string default: pr-review max-turns: @@ -293,13 +293,14 @@ jobs: /${{ env.REVIEW_SKILL }} ${{ steps.progress.outputs.review_depth }} - Return the complete Markdown review as the `review` field of your final - structured output โ€” that is the deliverable, which this workflow publishes to - the PR. Do not post to GitHub or write any files yourself; you have no write - access, so the attempt only costs turns. Do not add a "๐Ÿค– Review generated - with โ€ฆ" sign-off or any attribution footer: that line belongs to whoever posts - the comment, and here that is this workflow, not you โ€” your skill adds it only - when it posts the comment itself, which it is not doing here. + Your final message is the deliverable: make it the complete Markdown review and + nothing else โ€” no preamble, no "here is the review", no trailing sign-off. This + workflow reads that final message and publishes it to the PR. Do not post to + GitHub or write any files yourself; you have no write access, so the attempt only + costs turns. Do not add a "๐Ÿค– Review generated with โ€ฆ" sign-off or any attribution + footer: that line belongs to whoever posts the comment, and here that is this + workflow, not you โ€” your skill adds it only when it posts the comment itself, + which it is not doing here. A `[TURN BUDGET]` counter is injected after each tool call, showing turns used and the soft limit. Finishing the review is the priority โ€” once you reach @@ -308,30 +309,25 @@ jobs: budget, note in the review that it was capped at the turn budget and its quality may be affected. # The allowlist is the real control (the prompt is only a hint): reads only, no - # write tool at all. The review is handed back as structured output (--json-schema - # below), which the workflow publishes โ€” so the agent has no filesystem or GitHub - # write path. Worst case a hijacked run returns a misleading review (a human reads - # it); it can't touch files, post to GitHub, or leak the token. --add-dir grants - # read of the head worktree. Subagents inherit this list (verified; see - # anthropics/claude-code#27661), which the high-effort fan-out relies on; - # `Task` itself gates nothing. + # write tool at all. The review is handed back as the agent's final message, which + # the workflow reads from the execution log and publishes โ€” so the agent has no + # filesystem or GitHub write path. Worst case a hijacked run returns a misleading + # review (a human reads it); it can't touch files, post to GitHub, or leak the + # token. --add-dir grants read of the head worktree. Subagents inherit this list + # (verified; see anthropics/claude-code#27661), which the high-effort fan-out + # relies on; `Task` itself gates nothing. # # Unlisted Bash is denied only because the runner has no sandbox (no bubblewrap # on ubuntu-latest). Setting allowed_non_write_users (to review external PRs) # installs the sandbox and starts auto-approving sandboxable Bash, with # ./pr-head in the working dir โ€” re-check "never execute PR code" before then. # - # --json-schema forces the final message to be {review: ""}, exposed as - # steps.claude.outputs.structured_output. A file handoff would instead need the - # Write tool, which no allowlist entry grants without also loosening the sandbox. - # # --model: the review model; subagents inherit it. claude_args: | --model claude-${{ env.REVIEW_MODEL }} --effort ${{ env.REASONING_EFFORT }} --add-dir pr-head --allowedTools "Read,Grep,Glob,Task,Bash(git diff:*),Bash(git show:*),Bash(git log:*),Bash(git merge-base:*),Bash(gh pr view:*)" - --json-schema '{"type":"object","additionalProperties":false,"required":["review"],"properties":{"review":{"type":"string","description":"The complete Markdown review to publish as the PR comment."}}}' --max-turns ${{ env.MAX_TURNS }} # `always()` so an agent failure, the turn hard-stop, a timeout and a cancellation all @@ -345,15 +341,16 @@ jobs: COMMENT_ID: ${{ steps.progress.outputs.comment_id }} REVIEW_DEPTH: ${{ steps.progress.outputs.review_depth }} EXECUTION_FILE: ${{ steps.claude.outputs.execution_file }} - STRUCTURED_OUTPUT: ${{ steps.claude.outputs.structured_output }} + CLAUDE_OUTCOME: ${{ steps.claude.outcome }} with: script: | const fs = require('fs'); - // What the run cost, from the action's own execution log: an array of SDK - // messages whose last entry is the `result` summary. Best effort โ€” the file is - // absent or half-written if the agent died, and this step must still publish. - const readStats = () => { + // The run's result entry from the action's execution log โ€” an array of SDK + // messages whose last entry is the `result` summary. Carries both the cost/turn + // stats and `.result`, the agent's final message (the review). Best effort โ€” the + // file is absent or half-written if the agent died, and this step must still publish. + const readResult = () => { const file = process.env.EXECUTION_FILE; if (!file || !fs.existsSync(file)) { return null; @@ -363,7 +360,7 @@ jobs: const last = Array.isArray(log) ? log[log.length - 1] : null; return last && last.type === 'result' ? last : null; } catch (error) { - core.warning(`Could not read execution stats: ${error.message}`); + core.warning(`Could not read the execution result: ${error.message}`); return null; } }; @@ -380,15 +377,15 @@ jobs: return [titleCase(family), version.join('.')].filter(Boolean).join(' '); }; - const stats = readStats(); + const runResult = readResult(); const facts = [ `${formatModel(process.env.REVIEW_MODEL)} (${titleCase(process.env.REASONING_EFFORT)})`, `\`${process.env.REVIEW_DEPTH}\` review depth`, - stats && `${stats.num_turns} turns`, - stats && formatDuration(stats.duration_ms), - stats && - typeof stats.total_cost_usd === 'number' && - `$${stats.total_cost_usd.toFixed(2)}`, + runResult && `${runResult.num_turns} turns`, + runResult && formatDuration(runResult.duration_ms), + runResult && + typeof runResult.total_cost_usd === 'number' && + `$${runResult.total_cost_usd.toFixed(2)}`, ].filter(Boolean); // The review carries no attribution of its own โ€” the skill adds that only when @@ -399,17 +396,54 @@ jobs: `[run](${process.env.RUN_URL})`, ].join(' ยท ')}_`; - // The review comes back as the action's schema-validated structured output - // {review: ""}, so it is well-formed by construction; on a failed or - // aborted run it is empty ({}) and falls through to the "no report" failure below. - let report = (JSON.parse(process.env.STRUCTURED_OUTPUT || '{}').review || '').trim(); - const limit = 65536 - footer.length; // GitHub's comment body limit - if (report.length > limit) { + // The review is the agent's final message, `.result` of the result entry. A run + // that died leaves its error text in that same field, so it is a review only + // when the transcript says the agent got that far. + const finalMessage = ( + typeof runResult?.result === 'string' ? runResult.result : '' + ).trim(); + + // The transcript decides whether a review exists; the step's outcome only + // colours it. The action throws โ€” so the step reads `failure` โ€” even when the + // agent returned a COMPLETE review and merely overran --max-turns, and the turn + // budget above aims runs at that boundary, so resting this on the step outcome + // would throw away good reviews on a path we deliberately steer into. This + // conjunction is the action's own success test, minus that turns check; it + // still excludes the `subtype: success` + `is_error` runs, which is the case + // that publishing raw `.result` would otherwise turn into a fake review. + const produced = + runResult?.subtype === 'success' && runResult.is_error !== true; + const stepFailed = process.env.CLAUDE_OUTCOME !== 'success'; + + const truncate = (value) => { + const limit = 65536 - footer.length; // GitHub's comment body limit const notice = '\n\n_Report truncated to fit a comment._'; - report = report.slice(0, limit - notice.length) + notice; - } + return value.length > limit + ? value.slice(0, limit - notice.length) + notice + : value; + }; - const body = `${report || 'The review finished without producing a report.'}${footer}`; + const report = produced ? truncate(finalMessage) : ''; + + // Only the reason's first line, capped, and fenced as code: the rest is + // arbitrary SDK text that can carry endpoint and account identifiers, and it + // would otherwise render as Markdown (a leading `#` or `-` becomes markup). + // A run stopped by the turn cap leaves no text at all, so fall back to the + // subtype โ€” `error_max_turns` tells a maintainer what to change. + const reason = + finalMessage.split('\n', 1)[0].slice(0, 200).replace(/`/g, "'") || + (!produced && runResult?.subtype ? runResult.subtype : ''); + const problem = reason + ? `The review run failed: \`${reason}\`` + : 'The review finished without producing a report.'; + + // A review that arrived despite a failed step overran the turn budget or hit a + // post-completion error; it is worth publishing, but not silently. + const banner = + report && stepFailed + ? '> [!WARNING]\n> The run did not exit cleanly, so this review may be incomplete.\n\n' + : ''; + const body = `${banner}${report || problem}${footer}`; await core.summary .addHeading('Claude review', 3) @@ -430,13 +464,13 @@ jobs: } if (!report) { - core.setFailed('Review run produced no report'); + core.setFailed(problem); } - # TEMPORARY (debugging): upload the agent's full execution transcript so we can see - # what the review actually produced โ€” e.g. why the structured `review` field came - # back as "= PLACEHOLDER =" instead of the review. Short retention; remove once the - # review handoff is settled. + # TEMPORARY (debugging): upload the agent's full execution transcript. It is the only + # way to see why a run returned no review, and the same file carries the review, so a + # malformed one is diagnosable only from here. Short retention; remove once the review + # handoff is settled. - name: Upload execution log (debug) if: always() && steps.claude.outputs.execution_file uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1