From 6c2d97c8dddb37ad5f5bd688f4ef979cd0814deb Mon Sep 17 00:00:00 2001 From: Derek Misler Date: Thu, 10 Sep 2026 18:07:11 +0000 Subject: [PATCH 1/4] fix: substitute {pr} in posting-format.md and log html_url MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit posting-format.md used {pr} in the gh api URL, but {pr} is not a gh CLI template variable — only {owner} and {repo} are. The literal string {pr} produced a 404 on first attempt, forcing the agent to retry with an explicit URL. The retry only logged {id, state} (no html_url), so the pullrequestreview-[0-9]+ grep in action.yml missed it and posted a spurious '⚠️ Review did not complete' notice even though the real review had already been posted. Two changes: 1. Replace {pr} with $PR_NUMBER in posting-format.md. The action already resolves the PR number into steps.resolve-context.outputs.pr-number; expose it as PR_NUMBER in the 'Copy reference files' env block and substitute it via sed alongside __PR_HEAD_SHA__. Add a validation guard so a rendered template containing {pr} fails the step. 2. Pipe the gh api response through jq '{id, state, html_url}' so html_url (which contains pullrequestreview-XXXXXXX) is always logged, making the completion grep robust even on retries. --- review-pr/action.yml | 6 +++++- review-pr/agents/refs/posting-format.md | 3 ++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/review-pr/action.yml b/review-pr/action.yml index d7cb730..c21866c 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,8 +881,11 @@ 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 + 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 -q '{pr}' /tmp/refs/posting-format.md || \ [ "$(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" diff --git a/review-pr/agents/refs/posting-format.md b/review-pr/agents/refs/posting-format.md index cb947b0..80e38f9 100644 --- a/review-pr/agents/refs/posting-format.md +++ b/review-pr/agents/refs/posting-format.md @@ -118,7 +118,8 @@ jq -n \ --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_NUMBER/reviews --input - \ + | jq '{id, state, html_url}' ``` The `` marker MUST be on its own line, separated by a blank line From 141796812d557d44174a02e2c7484b6ae3bb15d2 Mon Sep 17 00:00:00 2001 From: Derek Misler Date: Thu, 10 Sep 2026 18:22:10 +0000 Subject: [PATCH 2/4] fix: restore {pr} in posting-format.md so sed bakes in the real number MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit replaced {pr} with $PR_NUMBER in posting-format.md, making the sed substitution a no-op. At agent runtime PR_NUMBER is not in the run-review step's env, so $PR_NUMBER expanded to empty, producing pulls//reviews → 404, reproducing the original bug. Option A: keep {pr} in posting-format.md as the placeholder; the sed substitution in action.yml's 'Copy reference files' step bakes the actual PR number in at render time, before the agent ever sees the file. The {pr} validation guard now has teeth — it fires whenever the sed substitution fails to replace the placeholder. Also broaden the validation error message to cover all failure conditions (unreplaced placeholder or missing SHA), not just the SHA check. --- review-pr/action.yml | 2 +- review-pr/agents/refs/posting-format.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/review-pr/action.yml b/review-pr/action.yml index c21866c..8409fb3 100644 --- a/review-pr/action.yml +++ b/review-pr/action.yml @@ -888,7 +888,7 @@ runs: grep -q '{pr}' /tmp/refs/posting-format.md || \ [ "$(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 80e38f9..cf36c77 100644 --- a/review-pr/agents/refs/posting-format.md +++ b/review-pr/agents/refs/posting-format.md @@ -118,7 +118,7 @@ jq -n \ --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_NUMBER/reviews --input - \ +| gh api repos/{owner}/{repo}/pulls/{pr}/reviews --input - \ | jq '{id, state, html_url}' ``` From b513bd77693958fadd4f8df119c78e61e7ca3a9d Mon Sep 17 00:00:00 2001 From: Derek Misler Date: Thu, 10 Sep 2026 19:42:23 +0000 Subject: [PATCH 3/4] fix: validate PR_NUMBER, add pipefail, broaden {pr} guard, extend tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four improvements from code review (aheritier): 1. Validate PR_NUMBER before substitution: add a numeric guard (`[[ $PR_NUMBER =~ ^[0-9]+$ ]]`) in the 'Copy reference files' step, mirroring the existing SHA guard. Prevents empty or injected values from reaching the sed substitution and the staged template. 2. Add `set -o pipefail` before the posting pipeline in posting-format.md so a `gh api` 404 propagates as a non-zero exit instead of being silently swallowed by the trailing `jq`. 3. Broaden the {pr} guard from `grep -q '{pr}' posting-format.md` to `grep -rq '{pr}' /tmp/refs/` so a future {pr} in any other staged ref file is also caught. 4. Extend the runCopyReference test table with three PR_NUMBER cases: empty, non-numeric, and shell metacharacters — all must exit 1. Pass PR_NUMBER through the test helper's env block so the existing SHA cases continue to pass with the default '5929'. --- review-pr/action.yml | 6 ++- review-pr/agents/refs/posting-format.md | 1 + .../__tests__/workflow-security.test.ts | 41 +++++++++++++------ 3 files changed, 34 insertions(+), 14 deletions(-) diff --git a/review-pr/action.yml b/review-pr/action.yml index 8409fb3..40feced 100644 --- a/review-pr/action.yml +++ b/review-pr/action.yml @@ -881,11 +881,15 @@ runs: echo "::error::Selected PR head SHA is invalid; refusing to stage review posting" exit 1 fi + 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 -q '{pr}' /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 failed validation (unreplaced placeholder or missing SHA)" diff --git a/review-pr/agents/refs/posting-format.md b/review-pr/agents/refs/posting-format.md index cf36c77..298b463 100644 --- a/review-pr/agents/refs/posting-format.md +++ b/review-pr/agents/refs/posting-format.md @@ -112,6 +112,7 @@ 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" \ diff --git a/src/resolve-trigger-context/__tests__/workflow-security.test.ts b/src/resolve-trigger-context/__tests__/workflow-security.test.ts index 5c67eed..f69bc1e 100644 --- a/src/resolve-trigger-context/__tests__/workflow-security.test.ts +++ b/src/resolve-trigger-context/__tests__/workflow-security.test.ts @@ -591,7 +591,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 +617,7 @@ function runCopyReference(headSha: string, template: string): ReturnType { }); 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('binds immutable review inputs before the snapshot and derives posting from its output', () => { From a9d1f630ab76890f43fd2d98e75e84363a0643e9 Mon Sep 17 00:00:00 2001 From: Derek Misler Date: Thu, 10 Sep 2026 20:01:51 +0000 Subject: [PATCH 4/4] fix: substitute {pr} in pr-review-reply.yaml at staging time The reply agent template (pr-review-reply.yaml) used {pr} in the gh api URL for posting inline replies. {pr} is not a gh CLI template variable, so every reply attempt paid a guaranteed 404 before the agent retried with an explicit URL. Add a 'Stage reply agent' step to review-pr/reply/action.yml that renders a copy of pr-review-reply.yaml to /tmp/pr-review-reply.yaml with {pr} substituted for the actual PR number, then point the run-reply step at the rendered copy. If pr-number is missing or non-numeric the step emits a warning and copies the unrendered template (fail-open, matching the existing reply path behaviour). Pass pr-number from steps.feedback.outputs.pr-number at the workflow call site in review-pr.yml. Add a runStageReplyAgent test helper and four it.each cases (valid, empty, non-numeric, shell metacharacters) to workflow-security.test.ts. Closes #112 (consolidated into #111). --- .github/workflows/review-pr.yml | 1 + review-pr/reply/action.yml | 19 ++++- .../__tests__/workflow-security.test.ts | 70 +++++++++++++++++++ 3 files changed, 89 insertions(+), 1 deletion(-) 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/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 f69bc1e..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, @@ -636,6 +637,51 @@ function runCopyReference( } } +function replyActionStepRun(name: string): string { + const action = parseDocument( + readFileSync(resolve(root, 'review-pr/reply/action.yml'), 'utf8'), + ).toJS() as Action; + const matches = action.runs?.steps?.filter((candidate) => 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; @@ -1347,6 +1393,30 @@ describe('fork workflow security regressions', () => { 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', () => { const action = readFileSync(resolve(root, 'review-pr/action.yml'), 'utf8'); const snapshot = action.slice(