diff --git a/.github/workflows/review-pr.yml b/.github/workflows/review-pr.yml index 0518e20..521c857 100644 --- a/.github/workflows/review-pr.yml +++ b/.github/workflows/review-pr.yml @@ -976,6 +976,7 @@ jobs: with: thread-context: ${{ steps.thread.outputs.prompt }} comment-id: ${{ steps.feedback.outputs.comment-id }} + pr-number: ${{ steps.feedback.outputs.pr-number }} anthropic-api-key: ${{ env.ANTHROPIC_API_KEY_FROM_SSM || secrets.ANTHROPIC_API_KEY }} openai-api-key: ${{ env.OPENAI_API_KEY_FROM_SSM || secrets.OPENAI_API_KEY }} google-api-key: ${{ secrets.GOOGLE_API_KEY }} diff --git a/review-pr/action.yml b/review-pr/action.yml index d7cb730..40feced 100644 --- a/review-pr/action.yml +++ b/review-pr/action.yml @@ -873,6 +873,7 @@ runs: env: ACTION_PATH: ${{ github.action_path }} PR_HEAD_SHA: ${{ steps.pr-info.outputs.head-sha }} + PR_NUMBER: ${{ steps.resolve-context.outputs.pr-number }} run: | mkdir -p /tmp/refs cp "$ACTION_PATH"/agents/refs/*.md /tmp/refs/ @@ -880,11 +881,18 @@ runs: echo "::error::Selected PR head SHA is invalid; refusing to stage review posting" exit 1 fi - sed "s/__PR_HEAD_SHA__/$PR_HEAD_SHA/g" "$ACTION_PATH/agents/refs/posting-format.md" > /tmp/refs/posting-format.md + if ! [[ "$PR_NUMBER" =~ ^[0-9]+$ ]]; then + echo "::error::Resolved PR number is invalid; refusing to stage review posting" + exit 1 + fi + sed -e "s/__PR_HEAD_SHA__/$PR_HEAD_SHA/g" \ + -e "s/{pr}/$PR_NUMBER/g" \ + "$ACTION_PATH/agents/refs/posting-format.md" > /tmp/refs/posting-format.md if grep -q '__PR_HEAD_SHA__\|\$PR_HEAD_SHA' /tmp/refs/posting-format.md || \ + grep -rq '{pr}' /tmp/refs/ || \ [ "$(grep -o -- '--arg commit_id' /tmp/refs/posting-format.md | wc -l | tr -d ' ')" != 1 ] || \ ! grep -q -- "--arg commit_id \"$PR_HEAD_SHA\"" /tmp/refs/posting-format.md; then - echo "::error::Rendered posting template does not contain exactly one selected immutable SHA" + echo "::error::Rendered posting template failed validation (unreplaced placeholder or missing SHA)" exit 1 fi echo "posting-reference=/tmp/refs/posting-format.md" >> "$GITHUB_OUTPUT" diff --git a/review-pr/agents/refs/posting-format.md b/review-pr/agents/refs/posting-format.md index cb947b0..298b463 100644 --- a/review-pr/agents/refs/posting-format.md +++ b/review-pr/agents/refs/posting-format.md @@ -112,13 +112,15 @@ echo "Posting review with $(jq length /tmp/review_comments.json) inline comment( # The composite action replaces __PR_HEAD_SHA__ with the validated immutable review snapshot # before the agent runs. This command must contain the selected literal SHA. +set -o pipefail jq -n \ --arg body "$REVIEW_BODY" \ --arg event "COMMENT" \ --arg commit_id "__PR_HEAD_SHA__" \ --slurpfile comments /tmp/review_comments.json \ '{body: $body, event: $event, commit_id: $commit_id, comments: $comments[0]}' \ -| gh api repos/{owner}/{repo}/pulls/{pr}/reviews --input - +| gh api repos/{owner}/{repo}/pulls/{pr}/reviews --input - \ + | jq '{id, state, html_url}' ``` The `` marker MUST be on its own line, separated by a blank line diff --git a/review-pr/reply/action.yml b/review-pr/reply/action.yml index 88d44d9..eb819ab 100644 --- a/review-pr/reply/action.yml +++ b/review-pr/reply/action.yml @@ -12,6 +12,10 @@ inputs: comment-id: description: "ID of the triggering comment (for failure notification reaction)" required: false + pr-number: + description: "Pull request number (used to render the reply agent template)" + required: false + default: "" anthropic-api-key: description: "Anthropic API key" required: false @@ -73,6 +77,19 @@ runs: pr-review-memory-${{ github.repository }}-reply- pr-review-memory-${{ github.repository }}- + - name: Stage reply agent + shell: bash + env: + ACTION_PATH: ${{ github.action_path }} + PR_NUMBER: ${{ inputs.pr-number }} + run: | + if ! [[ "$PR_NUMBER" =~ ^[0-9]+$ ]]; then + echo "::warning::pr-number input is missing or non-numeric — reply agent will use unrendered {pr} placeholder" + cp "$ACTION_PATH/../agents/pr-review-reply.yaml" /tmp/pr-review-reply.yaml + else + sed -e "s/{pr}/$PR_NUMBER/g" "$ACTION_PATH/../agents/pr-review-reply.yaml" > /tmp/pr-review-reply.yaml + fi + - name: Run reply agent id: run-reply continue-on-error: true @@ -80,7 +97,7 @@ runs: env: ACTION_PATH: ${{ github.action_path }} with: - agent: ${{ env.ACTION_PATH }}/../agents/pr-review-reply.yaml + agent: /tmp/pr-review-reply.yaml prompt: ${{ inputs.thread-context }} timeout: "300" anthropic-api-key: ${{ inputs.anthropic-api-key }} diff --git a/src/resolve-trigger-context/__tests__/workflow-security.test.ts b/src/resolve-trigger-context/__tests__/workflow-security.test.ts index 5c67eed..f081b96 100644 --- a/src/resolve-trigger-context/__tests__/workflow-security.test.ts +++ b/src/resolve-trigger-context/__tests__/workflow-security.test.ts @@ -6,6 +6,7 @@ import { chmodSync, cpSync, existsSync, + mkdirSync, mkdtempSync, readFileSync, rmSync, @@ -591,7 +592,11 @@ function summaryRun(): string { return actionStepRun('Post clean summary'); } -function runCopyReference(headSha: string, template: string): ReturnType { +function runCopyReference( + headSha: string, + template: string, + prNumber = '5929', +): ReturnType { const directory = mkdtempSync(resolve(tmpdir(), 'docker-agent-copy-reference-')); const actionPath = resolve(directory, 'action'); const refs = resolve(actionPath, 'agents/refs'); @@ -613,6 +618,7 @@ function runCopyReference(headSha: string, template: string): ReturnType candidate.name === name) ?? []; + if (matches.length !== 1 || !matches[0].run) throw new Error(`Expected one ${name} run body`); + return matches[0].run; +} + +function runStageReplyAgent( + prNumber: string, + template: string, +): { result: ReturnType; staged: string | null } { + const directory = mkdtempSync(resolve(tmpdir(), 'docker-agent-stage-reply-')); + // ACTION_PATH is the reply action dir; the step references $ACTION_PATH/../agents/ + const actionPath = resolve(directory, 'reply'); + const agentsDir = resolve(directory, 'agents'); + const stagedPath = '/tmp/pr-review-reply.yaml'; + const backupPath = resolve(directory, 'pr-review-reply.yaml.bak'); + try { + mkdirSync(actionPath, { recursive: true }); + mkdirSync(agentsDir, { recursive: true }); + writeFileSync(resolve(agentsDir, 'pr-review-reply.yaml'), template); + writeFileSync(resolve(directory, 'run.sh'), replyActionStepRun('Stage reply agent')); + if (existsSync(stagedPath)) cpSync(stagedPath, backupPath); + const result = spawnSync( + '/bin/bash', + ['--noprofile', '--norc', '-e', '-o', 'pipefail', resolve(directory, 'run.sh')], + { + env: testEnvironment({ + ACTION_PATH: actionPath, + PR_NUMBER: prNumber, + }), + encoding: 'utf8', + }, + ); + const staged = existsSync(stagedPath) ? readFileSync(stagedPath, 'utf8') : null; + return { result, staged }; + } finally { + if (existsSync(backupPath)) cpSync(backupPath, stagedPath); + else if (existsSync(stagedPath)) rmSync(stagedPath); + rmSync(directory, { recursive: true, force: true }); + } +} + type SummaryInvocation = { skipReason?: string; exitCode?: string; @@ -1313,23 +1364,57 @@ describe('fork workflow security regressions', () => { }); it.each([ - ['valid immutable SHA', 'a'.repeat(40), 'jq -n --arg commit_id "__PR_HEAD_SHA__"'], - ['empty SHA', '', 'jq -n --arg commit_id "__PR_HEAD_SHA__"'], - ['non-hex SHA', 'g'.repeat(40), 'jq -n --arg commit_id "__PR_HEAD_SHA__"'], - ['short SHA', 'a'.repeat(39), 'jq -n --arg commit_id "__PR_HEAD_SHA__"'], - ['long SHA', 'a'.repeat(41), 'jq -n --arg commit_id "__PR_HEAD_SHA__"'], - ['unresolved template', 'a'.repeat(40), 'jq -n --arg commit_id "$PR_HEAD_SHA"'], - ['zero commit arguments', 'a'.repeat(40), 'jq -n --arg body "review"'], + ['valid immutable SHA', 'a'.repeat(40), 'jq -n --arg commit_id "__PR_HEAD_SHA__"', '5929'], + ['empty SHA', '', 'jq -n --arg commit_id "__PR_HEAD_SHA__"', '5929'], + ['non-hex SHA', 'g'.repeat(40), 'jq -n --arg commit_id "__PR_HEAD_SHA__"', '5929'], + ['short SHA', 'a'.repeat(39), 'jq -n --arg commit_id "__PR_HEAD_SHA__"', '5929'], + ['long SHA', 'a'.repeat(41), 'jq -n --arg commit_id "__PR_HEAD_SHA__"', '5929'], + ['unresolved template', 'a'.repeat(40), 'jq -n --arg commit_id "$PR_HEAD_SHA"', '5929'], + ['zero commit arguments', 'a'.repeat(40), 'jq -n --arg body "review"', '5929'], [ 'multiple commit arguments', 'a'.repeat(40), 'jq -n --arg commit_id "__PR_HEAD_SHA__" --arg commit_id "x"', + '5929', ], - ])('executes Copy reference files staging preflight for %s', (_name, sha, template) => { - const result = runCopyReference(sha, template); - expect(result.status, result.stderr).toBe( - template === 'jq -n --arg commit_id "__PR_HEAD_SHA__"' && /^[a-f0-9]{40}$/i.test(sha) ? 0 : 1, - ); + ['empty PR number', 'a'.repeat(40), 'jq -n --arg commit_id "__PR_HEAD_SHA__"', ''], + ['non-numeric PR number', 'a'.repeat(40), 'jq -n --arg commit_id "__PR_HEAD_SHA__"', 'abc'], + [ + 'PR number with shell metacharacters', + 'a'.repeat(40), + 'jq -n --arg commit_id "__PR_HEAD_SHA__"', + '111; echo INJECTED', + ], + ])('executes Copy reference files staging preflight for %s', (_name, sha, template, prNumber) => { + const result = runCopyReference(sha, template, prNumber); + const validSha = /^[a-f0-9]{40}$/i.test(sha); + const validPr = /^[0-9]+$/.test(prNumber); + const validTemplate = template === 'jq -n --arg commit_id "__PR_HEAD_SHA__"'; + expect(result.status, result.stderr).toBe(validSha && validPr && validTemplate ? 0 : 1); + }); + + it.each([ + ['valid PR number', '5929', 'gh api repos/{owner}/{repo}/pulls/{pr}/comments --input -'], + ['empty PR number', '', 'gh api repos/{owner}/{repo}/pulls/{pr}/comments --input -'], + ['non-numeric PR number', 'abc', 'gh api repos/{owner}/{repo}/pulls/{pr}/comments --input -'], + [ + 'PR number with shell metacharacters', + '111; echo INJECTED', + 'gh api repos/{owner}/{repo}/pulls/{pr}/comments --input -', + ], + ])('stages reply agent with {pr} substitution for %s', (_name, prNumber, template) => { + const { result, staged } = runStageReplyAgent(prNumber, template); + const validPr = /^[0-9]+$/.test(prNumber); + if (validPr) { + expect(result.status, result.stderr).toBe(0); + expect(staged).not.toBeNull(); + expect(staged).not.toContain('{pr}'); + expect(staged).toContain(prNumber); + } else { + // Invalid PR number: step exits 0 with a warning, copies unrendered template + expect(result.status, result.stderr).toBe(0); + expect(result.stdout).toMatch(/warning/i); + } }); it('binds immutable review inputs before the snapshot and derives posting from its output', () => {