From 214b06a0c7a3f7041989301cdf9fe5f8752b1a7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arnaud=20He=CC=81ritier?= Date: Mon, 17 Aug 2026 22:47:41 +0200 Subject: [PATCH 1/7] fix(e2e): remove privileged fork PR execution --- .github/workflows/test-e2e-trigger.yml | 31 --------- .github/workflows/test-e2e.yml | 87 +++++--------------------- AGENTS.md | 3 +- 3 files changed, 16 insertions(+), 105 deletions(-) delete mode 100644 .github/workflows/test-e2e-trigger.yml diff --git a/.github/workflows/test-e2e-trigger.yml b/.github/workflows/test-e2e-trigger.yml deleted file mode 100644 index c86c447..0000000 --- a/.github/workflows/test-e2e-trigger.yml +++ /dev/null @@ -1,31 +0,0 @@ -# Copyright The Docker Agent Action authors -# SPDX-License-Identifier: Apache-2.0 - -name: Test E2E Trigger -on: - pull_request: - types: [opened, synchronize, reopened, ready_for_review] - branches: [main] - -permissions: {} - -jobs: - save-context: - runs-on: ubuntu-latest - steps: - - name: Save event context - env: - PR_NUMBER: ${{ github.event.pull_request.number }} - PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} - run: | - mkdir -p context - printf '%s' "${{ github.event_name }}" > context/event_name.txt - printf '%s' "$PR_NUMBER" > context/pr_number.txt - printf '%s' "$PR_HEAD_SHA" > context/pr_head_sha.txt - - - name: Upload context - uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 - with: - name: e2e-test-context - path: context/ - retention-days: 1 diff --git a/.github/workflows/test-e2e.yml b/.github/workflows/test-e2e.yml index a63c65f..22eb6b2 100644 --- a/.github/workflows/test-e2e.yml +++ b/.github/workflows/test-e2e.yml @@ -6,15 +6,13 @@ name: Test Docker Agent Action on: push: branches: [main] - workflow_run: - workflows: ["Test E2E Trigger"] - types: [completed] + pull_request: branches: [main] workflow_dispatch: inputs: pr_number: description: 'PR number to run mention-reply E2E tests against' - required: true + required: false type: string permissions: @@ -26,15 +24,13 @@ jobs: runs-on: ubuntu-latest if: | github.event_name == 'push' || - github.event_name == 'workflow_dispatch' || - github.event.workflow_run.conclusion == 'success' + github.event_name == 'pull_request' || + github.event_name == 'workflow_dispatch' permissions: contents: read steps: - name: Checkout code uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - with: - ref: ${{ github.event_name == 'workflow_run' && github.event.workflow_run.head_sha || '' }} - name: Run output extraction tests run: | @@ -47,15 +43,13 @@ jobs: runs-on: ubuntu-latest if: | github.event_name == 'push' || - github.event_name == 'workflow_dispatch' || - github.event.workflow_run.conclusion == 'success' + github.event_name == 'pull_request' || + github.event_name == 'workflow_dispatch' permissions: contents: read steps: - name: Checkout code uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - with: - ref: ${{ github.event_name == 'workflow_run' && github.event.workflow_run.head_sha || '' }} - name: Run job summary tests run: | @@ -68,15 +62,13 @@ jobs: runs-on: ubuntu-latest if: | github.event_name == 'push' || - github.event_name == 'workflow_dispatch' || - github.event.workflow_run.conclusion == 'success' + github.event_name == 'pull_request' || + github.event_name == 'workflow_dispatch' permissions: contents: read steps: - name: Checkout code uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - with: - ref: ${{ github.event_name == 'workflow_run' && github.event.workflow_run.head_sha || '' }} - name: Run release caller-permissions tests run: | @@ -84,66 +76,18 @@ jobs: chmod +x test-release-caller-permissions.sh ./test-release-caller-permissions.sh - resolve-context: - name: Resolve PR Context - runs-on: ubuntu-latest - if: github.event.workflow_run.conclusion == 'success' - permissions: - contents: read - id-token: write - actions: read - outputs: - pr-number: ${{ steps.read.outputs.pr-number }} - pr-head-sha: ${{ steps.read.outputs.pr-head-sha }} - steps: - - name: Setup credentials - uses: docker/docker-agent-action/setup-credentials@b08367e7e9a471822464be2bb9fdaa1c0fb6061b # v2.0.4 - - - name: Verify token for cross-run artifact download - shell: bash - run: | - if [ -z "$GITHUB_APP_TOKEN" ]; then - echo "::error::GITHUB_APP_TOKEN is not set. setup-credentials may have failed." - echo "::error::Cross-run artifact download requires a token with actions:read scope." - exit 1 - fi - - - name: Download trigger context - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 - with: - name: e2e-test-context - path: /tmp/context - run-id: ${{ github.event.workflow_run.id }} - github-token: ${{ env.GITHUB_APP_TOKEN }} - - - name: Read context - id: read - shell: bash - run: | - if [ ! -f /tmp/context/pr_number.txt ]; then - echo "::error::pr_number.txt missing from artifact" - exit 1 - fi - echo "pr-number=$(cat /tmp/context/pr_number.txt)" >> $GITHUB_OUTPUT - if [ -f /tmp/context/pr_head_sha.txt ]; then - echo "pr-head-sha=$(cat /tmp/context/pr_head_sha.txt)" >> $GITHUB_OUTPUT - fi - test-pirate-agent: name: Pirate Agent Test runs-on: ubuntu-latest - needs: [resolve-context] if: | - always() && - needs.resolve-context.result == 'success' + github.event_name == 'push' || + github.event_name == 'workflow_dispatch' permissions: contents: read id-token: write steps: - name: Checkout code uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - with: - ref: refs/pull/${{ needs.resolve-context.outputs.pr-number }}/head - name: Setup pnpm uses: pnpm/action-setup@8912a9102ac27614460f54aedde9e1e7f9aec20d # v6.0.5 @@ -219,18 +163,15 @@ jobs: test-invalid-agent: name: Invalid Agent Test runs-on: ubuntu-latest - needs: [resolve-context] if: | - always() && - needs.resolve-context.result == 'success' + github.event_name == 'push' || + github.event_name == 'workflow_dispatch' permissions: contents: read id-token: write steps: - name: Checkout code uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - with: - ref: refs/pull/${{ needs.resolve-context.outputs.pr-number }}/head - name: Setup pnpm uses: pnpm/action-setup@8912a9102ac27614460f54aedde9e1e7f9aec20d # v6.0.5 @@ -278,7 +219,7 @@ jobs: test-mention-reply-toplevel: name: Mention Reply (Top-Level) E2E Test runs-on: ubuntu-latest - if: github.event_name == 'workflow_dispatch' + if: github.event_name == 'workflow_dispatch' && inputs.pr_number != '' permissions: contents: read id-token: write @@ -418,7 +359,7 @@ jobs: test-mention-reply-inline: name: Mention Reply (Inline) E2E Test runs-on: ubuntu-latest - if: github.event_name == 'workflow_dispatch' + if: github.event_name == 'workflow_dispatch' && inputs.pr_number != '' permissions: contents: read id-token: write diff --git a/AGENTS.md b/AGENTS.md index cbf8971..60a9637 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -195,7 +195,8 @@ The action runs untrusted input (PR titles, bodies, comments, diffs) through an | Workflow | Purpose | | --------------------------------- | -------------------------------------------------------------------- | | `test.yml` | Unit + integration tests on push/PR. | -| `test-e2e.yml` | End-to-end action invocation against a real agent. | +| `test-e2e.yml` | Secretless E2E coverage on PRs; credential-bearing scenarios only on trusted main pushes or manual dispatch. | +| `test-e2e-reviewer.yml` | Dispatch-only E2E workflow for the PR reviewer. | | `release.yml` | Publishes tagged releases (must include a built `dist/`). | | `review-pr.yml` | **Reusable workflow** consumers call as `docker/docker-agent-action/.github/workflows/review-pr.yml@vā€¦`. | | `self-review-pr.yml` + `-trigger.yml` | Dogfooding: the repo reviews its own PRs. | From 4bbe7a707f84e9751f1170aea788e0f712e3d9a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arnaud=20He=CC=81ritier?= Date: Mon, 17 Aug 2026 22:50:15 +0200 Subject: [PATCH 2/7] feat(security): add trusted trigger-context resolver --- .github/workflows/review-pr.yml | 4 +- .../__tests__/resolve-trigger-context.test.ts | 553 ++++++++++++++++++ src/resolve-trigger-context/index.ts | 88 +++ .../resolve-trigger-context.ts | 282 +++++++++ tsup.config.ts | 1 + 5 files changed, 926 insertions(+), 2 deletions(-) create mode 100644 src/resolve-trigger-context/__tests__/resolve-trigger-context.test.ts create mode 100644 src/resolve-trigger-context/index.ts create mode 100644 src/resolve-trigger-context/resolve-trigger-context.ts diff --git a/.github/workflows/review-pr.yml b/.github/workflows/review-pr.yml index ef22996..df197df 100644 --- a/.github/workflows/review-pr.yml +++ b/.github/workflows/review-pr.yml @@ -112,12 +112,12 @@ jobs: trigger-event: ${{ steps.read.outputs.event-name }} pr-number: ${{ steps.read.outputs.pr-number }} pr-head-sha: ${{ steps.read.outputs.pr-head-sha }} + comment-has-mention: ${{ steps.read.outputs.comment-has-mention }} + comment-is-review-cmd: ${{ steps.read.outputs.comment-is-review-cmd }} requested-reviewer: ${{ steps.read.outputs.requested-reviewer }} comment-json: ${{ steps.read.outputs.comment-json }} comment-author: ${{ steps.read.outputs.comment-author }} comment-in-reply-to-id: ${{ steps.read.outputs.comment-in-reply-to-id }} - comment-has-mention: ${{ steps.read.outputs.comment-has-mention }} - comment-is-review-cmd: ${{ steps.read.outputs.comment-is-review-cmd }} comment-author-type: ${{ steps.read.outputs.comment-author-type }} steps: # The trigger run concludes success even when its save-context job was diff --git a/src/resolve-trigger-context/__tests__/resolve-trigger-context.test.ts b/src/resolve-trigger-context/__tests__/resolve-trigger-context.test.ts new file mode 100644 index 0000000..6fa75c5 --- /dev/null +++ b/src/resolve-trigger-context/__tests__/resolve-trigger-context.test.ts @@ -0,0 +1,553 @@ +// Copyright The Docker Agent Action authors +// SPDX-License-Identifier: Apache-2.0 + +import { mkdirSync, mkdtempSync, readFileSync, rmSync, statSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const { getWorkflowRun, listPulls, getPull, getReviewComment, paginate, MockOctokit } = vi.hoisted( + () => { + const getWorkflowRun = vi.fn(); + const listPulls = vi.fn(); + const getPull = vi.fn(); + const getReviewComment = vi.fn(); + const paginate = vi.fn(); + class MockOctokit { + rest = { + actions: { getWorkflowRun }, + pulls: { list: listPulls, get: getPull, getReviewComment }, + }; + paginate = paginate; + } + return { getWorkflowRun, listPulls, getPull, getReviewComment, paginate, MockOctokit }; + }, +); + +vi.mock('@octokit/rest', () => ({ Octokit: MockOctokit })); + +import { + main, + resolverOutputs, + writeCanonicalContext, + writeCanonicalContextFile, +} from '../index.js'; +import { resolveTriggerContext } from '../resolve-trigger-context.js'; + +const repository = 'docker/docker-agent-action'; +const sha = 'a'.repeat(40); +const pr = { + number: 42, + head: { sha, repo: { full_name: 'external/repo' } }, + base: { sha: 'b'.repeat(40), ref: 'main' }, + user: { login: 'external' }, +}; + +const canonicalContext = { + event: 'pull_request_review_comment' as const, + runId: 123, + runHeadSha: sha, + actor: 'external', + pullRequest: { + number: 42, + headSha: sha, + baseSha: 'b'.repeat(40), + baseRef: 'main', + author: 'external', + }, + comment: { + id: 5, + author: 'external', + authorType: 'User', + body: '@docker-agent review this', + inReplyToId: null, + pullRequestUrl: `https://api.github.com/repos/${repository}/pulls/42`, + }, +}; + +describe('canonical context staging', () => { + it('writes the canonical context in a randomized private directory', () => { + const root = mkdtempSync(join(tmpdir(), 'runner-temp-')); + try { + const first = writeCanonicalContext(canonicalContext, root); + const second = writeCanonicalContext(canonicalContext, root); + + expect(dirname(first)).not.toBe(dirname(second)); + expect(dirname(first)).not.toBe(root); + expect(first).toMatch(/canonical-trigger-context\.json$/); + expect(statSync(dirname(first)).mode & 0o777).toBe(0o700); + expect(statSync(first).mode & 0o777).toBe(0o600); + expect(JSON.parse(readFileSync(first, 'utf8'))).toEqual(canonicalContext); + } finally { + rmSync(root, { recursive: true, force: true }); + } + }); +}); + +it('creates canonical context files exclusively', () => { + const directory = mkdtempSync(join(tmpdir(), 'canonical-context-')); + try { + writeCanonicalContextFile(directory, canonicalContext); + expect(() => writeCanonicalContextFile(directory, canonicalContext)).toThrow(/EEXIST/); + } finally { + rmSync(directory, { recursive: true, force: true }); + } +}); + +it('requires RUNNER_TEMP for canonical context staging', () => { + expect(() => writeCanonicalContext(canonicalContext, '')).toThrow(/RUNNER_TEMP/); +}); + +it('keeps canonical context staging under the supplied runner temp root', () => { + const root = mkdtempSync(join(tmpdir(), 'runner-temp-')); + try { + const contextPath = writeCanonicalContext(canonicalContext, root); + expect(contextPath.startsWith(`${root}/`)).toBe(true); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + +function run(overrides: Record = {}) { + return { + repository: { id: 1, full_name: repository }, + event: 'pull_request', + status: 'completed', + conclusion: 'success', + actor: { login: 'external' }, + head_sha: sha, + head_branch: 'feature', + head_repository: { full_name: 'external/repo', owner: { login: 'external' } }, + pull_requests: [ + { + number: 42, + head: { sha }, + base: { + repo: { + id: 1, + name: 'docker-agent-action', + url: `https://api.github.com/repos/${repository}`, + }, + }, + }, + ], + ...overrides, + }; +} + +function artifacts(files: Record): string { + const directory = mkdtempSync(join(tmpdir(), 'trigger-context-')); + for (const [name, value] of Object.entries(files)) { + mkdirSync(join(directory, '..'), { recursive: true }); + writeFileSync(join(directory, name), value); + } + return directory; +} + +async function resolve(directory: string) { + return resolveTriggerContext({ + triggerRunId: '123', + repository, + repoToken: 'token', + artifactDirectory: directory, + }); +} + +beforeEach(() => { + vi.clearAllMocks(); + getWorkflowRun.mockResolvedValue({ data: run() }); + getPull.mockResolvedValue({ data: pr }); + listPulls.mockResolvedValue({ data: [pr] }); + paginate.mockResolvedValue([pr]); +}); + +afterEach(() => { + vi.unstubAllEnvs(); +}); + +describe('resolver entrypoint', () => { + it.each([ + ['unset', undefined], + ['empty', ''], + ])('does not fall back to /tmp/context or another implicit directory when TRIGGER_ARTIFACT_DIRECTORY is %s', async (_description, artifactDirectory) => { + vi.stubEnv('GITHUB_APP_TOKEN', 'token'); + vi.stubEnv('GITHUB_TOKEN', ''); + vi.stubEnv('TRIGGER_ARTIFACT_DIRECTORY', artifactDirectory); + + await expect(main()).rejects.toThrow('TRIGGER_ARTIFACT_DIRECTORY is not set'); + expect(getWorkflowRun).not.toHaveBeenCalled(); + }); +}); + +describe('resolveTriggerContext', () => { + it.each([ + [ + 'a threaded live review comment', + { + id: 5, + author: 'external', + authorType: 'User', + body: '@docker-agent review this', + inReplyToId: 1, + pullRequestUrl: `https://api.github.com/repos/${repository}/pulls/42`, + }, + '1', + ], + [ + 'a top-level live review comment', + { + id: 5, + author: 'external', + authorType: 'User', + body: '@docker-agent review this', + inReplyToId: null, + pullRequestUrl: `https://api.github.com/repos/${repository}/pulls/42`, + }, + '', + ], + ['a comment-less pull request', null, ''], + ])('emits only live-derived reply-parent output for %s', (_description, comment, parentOutput) => { + const outputs = resolverOutputs({ + event: comment ? 'pull_request_review_comment' : 'pull_request', + runId: 123, + runHeadSha: sha, + actor: 'external', + pullRequest: { + number: 42, + headSha: sha, + baseSha: 'b'.repeat(40), + baseRef: 'main', + author: 'external', + }, + comment, + }); + + expect(outputs['comment-in-reply-to-id']).toBe(parentOutput); + expect(outputs).not.toHaveProperty('comment-body'); + expect(outputs).not.toHaveProperty('comment-id'); + expect(outputs).not.toHaveProperty('actor'); + }); + + it.each([ + ['a canonical mention', '@docker-agent review this', 'true', 'false'], + ['a canonical review command', '/review @docker-agent', 'true', 'true'], + ['a canonical ordinary comment', 'hello', 'false', 'false'], + ['no canonical comment', null, 'false', 'false'], + ])('derives legacy mention outputs only from %s', (_description, body, mention, reviewCommand) => { + const outputs = resolverOutputs({ + ...canonicalContext, + comment: body === null ? null : { ...canonicalContext.comment, body }, + }); + + expect(outputs['comment-has-mention']).toBe(mention); + expect(outputs['comment-is-review-cmd']).toBe(reviewCommand); + expect(outputs).not.toHaveProperty('comment-body'); + expect(outputs).not.toHaveProperty('comment-id'); + }); + + it('uses the canonical live body rather than a forged artifact body for legacy mention outputs', () => { + const outputs = resolverOutputs({ + ...canonicalContext, + comment: { ...canonicalContext.comment, body: 'live body without a mention' }, + }); + + expect(outputs['comment-has-mention']).toBe('false'); + expect(outputs['comment-is-review-cmd']).toBe('false'); + }); + + it('rejects a workflow run from another repository before PR lookup', async () => { + getWorkflowRun.mockResolvedValue({ data: run({ repository: { full_name: 'other/repo' } }) }); + const directory = artifacts({ 'event_name.txt': 'pull_request' }); + try { + await expect(resolve(directory)).rejects.toThrow(/different repository/); + expect(getPull).not.toHaveBeenCalled(); + expect(listPulls).not.toHaveBeenCalled(); + } finally { + rmSync(directory, { recursive: true }); + } + }); + + it.each([ + ['an incomplete workflow run', { status: 'in_progress' }], + ['an unsuccessful workflow run', { conclusion: 'failure' }], + ])('rejects %s before PR lookup', async (_description, overrides) => { + getWorkflowRun.mockResolvedValue({ data: run(overrides) }); + const directory = artifacts({ 'event_name.txt': 'pull_request' }); + try { + await expect(resolve(directory)).rejects.toThrow(/did not complete successfully/); + expect(getPull).not.toHaveBeenCalled(); + expect(listPulls).not.toHaveBeenCalled(); + } finally { + rmSync(directory, { recursive: true }); + } + }); + + it('rejects an unsupported workflow event before PR lookup', async () => { + getWorkflowRun.mockResolvedValue({ data: run({ event: 'push' }) }); + const directory = artifacts({ 'event_name.txt': 'push' }); + try { + await expect(resolve(directory)).rejects.toThrow(/Unsupported workflow run event/); + expect(getPull).not.toHaveBeenCalled(); + expect(listPulls).not.toHaveBeenCalled(); + } finally { + rmSync(directory, { recursive: true }); + } + }); + + it('rejects a workflow run with no original actor before PR lookup', async () => { + getWorkflowRun.mockResolvedValue({ data: run({ actor: { login: '' } }) }); + const directory = artifacts({ 'event_name.txt': 'pull_request' }); + try { + await expect(resolve(directory)).rejects.toThrow(/no original actor/); + expect(getPull).not.toHaveBeenCalled(); + expect(listPulls).not.toHaveBeenCalled(); + } finally { + rmSync(directory, { recursive: true }); + } + }); + + it('rejects a workflow run with a non-immutable head SHA before PR lookup', async () => { + getWorkflowRun.mockResolvedValue({ data: run({ head_sha: 'not-a-sha' }) }); + const directory = artifacts({ 'event_name.txt': 'pull_request' }); + try { + await expect(resolve(directory)).rejects.toThrow(/workflow run head SHA/); + expect(getPull).not.toHaveBeenCalled(); + expect(listPulls).not.toHaveBeenCalled(); + } finally { + rmSync(directory, { recursive: true }); + } + }); + + it.each([ + [ + 'wrong base repository', + { base: { repo: { id: 2, name: 'repo', url: 'https://api.github.com/repos/other/repo' } } }, + ], + [ + 'malformed base repository', + { base: { repo: { id: 2, name: 'repo', url: 'https://api.github.com/repos/other/repo' } } }, + ], + ])('rejects associated PR metadata with a %s', async (_description, association) => { + getWorkflowRun.mockResolvedValue({ + data: run({ pull_requests: [{ number: 42, ...association }] }), + }); + const directory = artifacts({ 'event_name.txt': 'pull_request' }); + try { + await expect(resolve(directory)).rejects.toThrow(/metadata does not match/); + expect(getPull).not.toHaveBeenCalled(); + } finally { + rmSync(directory, { recursive: true }); + } + }); + + it('selects an advanced live PR head over the workflow-run provenance SHA', async () => { + getPull.mockResolvedValue({ data: { ...pr, head: { ...pr.head, sha: 'b'.repeat(40) } } }); + const directory = artifacts({ 'event_name.txt': 'pull_request' }); + try { + await expect(resolve(directory)).resolves.toMatchObject({ + pullRequest: { headSha: 'b'.repeat(40) }, + headAdvanced: true, + }); + } finally { + rmSync(directory, { recursive: true }); + } + }); + + it('rejects a review comment whose PR differs from the workflow run association', async () => { + getWorkflowRun.mockResolvedValue({ + data: run({ + event: 'pull_request_review_comment', + pull_requests: [ + { + number: 99, + head: { sha }, + base: { + repo: { + id: 1, + name: 'docker-agent-action', + url: `https://api.github.com/repos/${repository}`, + }, + }, + }, + ], + }), + }); + getReviewComment.mockResolvedValue({ + data: { + id: 5, + body: 'live body', + user: { login: 'external', type: 'User' }, + pull_request_url: `https://api.github.com/repos/${repository}/pulls/42`, + }, + }); + const directory = artifacts({ + 'event_name.txt': 'pull_request_review_comment', + 'comment_id.txt': '5', + }); + try { + await expect(resolve(directory)).rejects.toThrow(/does not match workflow run PR/); + expect(getPull).not.toHaveBeenCalled(); + } finally { + rmSync(directory, { recursive: true }); + } + }); + + it('rejects malformed legacy comment JSON', async () => { + getWorkflowRun.mockResolvedValue({ + data: run({ event: 'pull_request_review_comment', pull_requests: [] }), + }); + const directory = artifacts({ + 'event_name.txt': 'pull_request_review_comment', + 'comment.json': '{', + }); + try { + await expect(resolve(directory)).rejects.toThrow(/Legacy comment.json is malformed/); + expect(getReviewComment).not.toHaveBeenCalled(); + } finally { + rmSync(directory, { recursive: true }); + } + }); + + it('uses only trusted workflow metadata and live PR data for pull requests', async () => { + const directory = artifacts({ + 'event_name.txt': 'pull_request', + 'pr_number.txt': '999', + 'pr_head_sha.txt': 'b'.repeat(40), + 'requested_reviewer.txt': 'member', + }); + try { + await expect(resolve(directory)).resolves.toMatchObject({ + pullRequest: { number: 42, headSha: sha }, + }); + expect(getPull).toHaveBeenCalledWith({ + owner: 'docker', + repo: 'docker-agent-action', + pull_number: 42, + }); + } finally { + rmSync(directory, { recursive: true }); + } + }); + + it('falls back to a unique trusted head lookup when no PR is associated', async () => { + getWorkflowRun.mockResolvedValue({ data: run({ pull_requests: [] }) }); + const directory = artifacts({ 'event_name.txt': 'pull_request' }); + try { + await expect(resolve(directory)).resolves.toMatchObject({ pullRequest: { number: 42 } }); + expect(paginate).toHaveBeenCalledWith( + listPulls, + expect.objectContaining({ head: 'external:feature' }), + ); + } finally { + rmSync(directory, { recursive: true }); + } + }); + + it('fails closed when the fallback has zero or multiple matching PRs', async () => { + getWorkflowRun.mockResolvedValue({ data: run({ pull_requests: [] }) }); + paginate.mockResolvedValue([pr, pr]); + const directory = artifacts({ 'event_name.txt': 'pull_request' }); + try { + await expect(resolve(directory)).rejects.toThrow(/ambiguous or empty/); + } finally { + rmSync(directory, { recursive: true }); + } + }); + + it('binds a live review comment to the original actor, not a rerunner', async () => { + getWorkflowRun.mockResolvedValue({ + data: run({ event: 'pull_request_review_comment', pull_requests: [] }), + }); + getReviewComment.mockResolvedValue({ + data: { + id: 5, + body: '@docker-agent review this', + in_reply_to_id: 1, + user: { login: 'external', type: 'User' }, + pull_request_url: `https://api.github.com/repos/${repository}/pulls/42`, + }, + }); + const directory = artifacts({ + 'event_name.txt': 'pull_request_review_comment', + 'comment_id.txt': '5', + }); + try { + await expect(resolve(directory)).resolves.toMatchObject({ + event: 'pull_request_review_comment', + comment: { id: 5, author: 'external', inReplyToId: 1 }, + }); + } finally { + rmSync(directory, { recursive: true }); + } + }); + + it('supports legacy comment.json as a locator without trusting its body or author', async () => { + getWorkflowRun.mockResolvedValue({ + data: run({ event: 'pull_request_review_comment', pull_requests: [] }), + }); + getReviewComment.mockResolvedValue({ + data: { + id: 5, + body: 'live body', + user: { login: 'external', type: 'User' }, + pull_request_url: `https://api.github.com/repos/${repository}/pulls/42`, + }, + }); + const directory = artifacts({ + 'event_name.txt': 'pull_request_review_comment', + 'comment.json': JSON.stringify({ id: 5, body: 'forged body', user: { login: 'member' } }), + }); + try { + await expect(resolve(directory)).resolves.toMatchObject({ + comment: { body: 'live body', author: 'external' }, + }); + } finally { + rmSync(directory, { recursive: true }); + } + }); + + it.each([ + ['a malformed run ID', { triggerRunId: 'bad' }], + ['an artifact event mismatch', {}], + ])('fails closed on %s', async (_description, overrides) => { + const directory = artifacts({ 'event_name.txt': 'pull_request_review_comment' }); + try { + await expect( + resolveTriggerContext({ + triggerRunId: '123', + repository, + repoToken: 'token', + artifactDirectory: directory, + ...overrides, + }), + ).rejects.toThrow(); + } finally { + rmSync(directory, { recursive: true }); + } + }); + + it('rejects forged actor, cross-repository comment URLs, and malformed comment IDs', async () => { + getWorkflowRun.mockResolvedValue({ + data: run({ event: 'pull_request_review_comment', pull_requests: [] }), + }); + getReviewComment.mockResolvedValue({ + data: { + id: 5, + body: '', + user: { login: 'member', type: 'User' }, + pull_request_url: 'https://api.github.com/repos/other/repo/pulls/42', + }, + }); + const directory = artifacts({ + 'event_name.txt': 'pull_request_review_comment', + 'comment_id.txt': 'not-a-number', + }); + try { + await expect(resolve(directory)).rejects.toThrow(/comment ID/); + writeFileSync(join(directory, 'comment_id.txt'), '5'); + await expect(resolve(directory)).rejects.toThrow(/author/); + } finally { + rmSync(directory, { recursive: true }); + } + }); +}); diff --git a/src/resolve-trigger-context/index.ts b/src/resolve-trigger-context/index.ts new file mode 100644 index 0000000..0c71749 --- /dev/null +++ b/src/resolve-trigger-context/index.ts @@ -0,0 +1,88 @@ +// Copyright The Docker Agent Action authors +// SPDX-License-Identifier: Apache-2.0 + +import { chmodSync, mkdtempSync, writeFileSync } from 'node:fs'; +import { join } from 'node:path'; +import * as core from '@actions/core'; +import { + type CanonicalTriggerContext, + resolveTriggerContext, + triggerRoute, +} from './resolve-trigger-context.js'; + +export function resolverOutputs(context: CanonicalTriggerContext): Record { + const comment = context.comment; + return { + 'event-name': context.event, + 'pr-number': String(context.pullRequest.number), + 'pr-head-sha': context.pullRequest.headSha, + 'pr-base-sha': context.pullRequest.baseSha, + 'trigger-route': triggerRoute(context), + // Preserve the legacy workflow-run mention gate until its canonical route + // replacement lands; both values derive exclusively from live context. + 'comment-has-mention': String(comment?.body.includes('@docker-agent') ?? false), + 'comment-is-review-cmd': String(comment?.body.startsWith('/review') ?? false), + 'comment-author': comment?.author ?? '', + 'comment-author-type': comment?.authorType ?? '', + 'comment-in-reply-to-id': + comment?.inReplyToId && comment.inReplyToId > 0 ? String(comment.inReplyToId) : '', + }; +} + +export function writeCanonicalContextFile( + directory: string, + context: CanonicalTriggerContext, +): string { + const outputPath = join(directory, 'canonical-trigger-context.json'); + writeFileSync(outputPath, `${JSON.stringify(context)}\n`, { + encoding: 'utf8', + flag: 'wx', + mode: 0o600, + }); + chmodSync(outputPath, 0o600); + return outputPath; +} + +export function writeCanonicalContext( + context: CanonicalTriggerContext, + runnerTemp: string, +): string { + if (!runnerTemp) throw new Error('RUNNER_TEMP is not set'); + + const directory = mkdtempSync(join(runnerTemp, 'docker-agent-trigger-context-')); + chmodSync(directory, 0o700); + return writeCanonicalContextFile(directory, context); +} + +export async function main(): Promise { + const triggerRunId = process.env.TRIGGER_RUN_ID ?? ''; + const repository = process.env.GITHUB_REPOSITORY ?? ''; + const repoToken = process.env.GITHUB_APP_TOKEN ?? process.env.GITHUB_TOKEN ?? ''; + const artifactDirectory = process.env.TRIGGER_ARTIFACT_DIRECTORY ?? ''; + const runnerTemp = process.env.RUNNER_TEMP ?? ''; + + if (!repoToken) throw new Error('GITHUB_APP_TOKEN is not set'); + if (!artifactDirectory) throw new Error('TRIGGER_ARTIFACT_DIRECTORY is not set'); + const context = await resolveTriggerContext({ + triggerRunId, + repository, + repoToken, + artifactDirectory, + }); + const outputPath = writeCanonicalContext(context, runnerTemp); + + for (const [name, value] of Object.entries({ + ...resolverOutputs(context), + 'canonical-context-path': outputPath, + })) { + core.setOutput(name, value); + } +} + +if (process.argv[1]?.endsWith('resolve-trigger-context.js') && !process.env.VITEST) { + main().catch((error: unknown) => { + core.setFailed( + `Failed to resolve trusted trigger context: ${error instanceof Error ? error.message : String(error)}`, + ); + }); +} diff --git a/src/resolve-trigger-context/resolve-trigger-context.ts b/src/resolve-trigger-context/resolve-trigger-context.ts new file mode 100644 index 0000000..ad40a3a --- /dev/null +++ b/src/resolve-trigger-context/resolve-trigger-context.ts @@ -0,0 +1,282 @@ +// Copyright The Docker Agent Action authors +// SPDX-License-Identifier: Apache-2.0 + +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { Octokit } from '@octokit/rest'; + +export interface TriggerContextInputs { + triggerRunId: string; + repository: string; + repoToken: string; + artifactDirectory: string; +} + +type WorkflowRun = Awaited>['data']; +type WorkflowRunPullRequest = NonNullable[number]; +type PullRequest = Awaited>['data']; +type ReviewComment = Awaited>['data']; + +export type { PullRequest, ReviewComment, WorkflowRun, WorkflowRunPullRequest }; +export type TriggerRoute = 'review' | 'feedback' | 'mention' | 'none'; + +export interface CanonicalPullRequest { + number: number; + headSha: string; + baseSha: string; + baseRef: string; + author: string; +} + +export interface CanonicalComment { + id: number; + author: string; + authorType: string; + body: string; + inReplyToId: number | null; + pullRequestUrl: string; + path: string | null; + line: number | null; + originalLine: number | null; + side: string | null; + startLine: number | null; + startSide: string | null; + diffHunk: string | null; + commitId: string | null; + originalCommitId: string | null; +} + +export interface CanonicalTriggerContext { + event: 'pull_request' | 'pull_request_review_comment'; + runId: number; + runHeadSha: string; + headAdvanced: boolean; + actor: string; + pullRequest: CanonicalPullRequest; + comment: CanonicalComment | null; +} + +function parseRepository(repository: string): [string, string] { + const parts = repository.split('/'); + if (parts.length !== 2 || !parts[0] || !parts[1]) + throw new Error(`Invalid base repository: '${repository}'`); + return [parts[0], parts[1]]; +} + +function requireRunId(value: string): number { + if (!/^\d+$/.test(value)) throw new Error(`Invalid trigger run ID: '${value}'`); + const id = Number(value); + if (!Number.isSafeInteger(id) || id <= 0) throw new Error(`Invalid trigger run ID: '${value}'`); + return id; +} + +function requireSha(value: string | null | undefined, label: string): string { + if (!value || !/^[a-f0-9]{40}$/i.test(value)) + throw new Error(`Invalid ${label}: '${value ?? ''}'`); + return value; +} + +function nullableSha(value: string | null | undefined, label: string): string | null { + return value == null ? null : requireSha(value, label); +} + +function readArtifactHint(artifactDirectory: string, filename: string): string { + try { + return readFileSync(join(artifactDirectory, filename), 'utf8').trim(); + } catch (error: unknown) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return ''; + throw error; + } +} + +function readCommentId(artifactDirectory: string): number { + const direct = readArtifactHint(artifactDirectory, 'comment_id.txt'); + const legacy = + direct || + (() => { + const raw = readArtifactHint(artifactDirectory, 'comment.json'); + if (!raw) return ''; + try { + return String((JSON.parse(raw) as { id?: unknown }).id ?? ''); + } catch { + throw new Error('Legacy comment.json is malformed'); + } + })(); + if (!/^\d+$/.test(legacy) || Number(legacy) <= 0) { + throw new Error('A positive numeric comment ID is required for review-comment triggers'); + } + return Number(legacy); +} + +function assertArtifactEvent(artifactDirectory: string, event: string): void { + const hint = readArtifactHint(artifactDirectory, 'event_name.txt'); + if (hint && hint !== event) + throw new Error(`Artifact event '${hint}' does not match workflow run event '${event}'`); +} + +function assertPrUrl(url: string, repository: string): [string, number] { + const match = /^https:\/\/api\.github\.com\/repos\/([^/]+)\/([^/]+)\/pulls\/(\d+)$/.exec(url); + if (!match || `${match[1]}/${match[2]}` !== repository) { + throw new Error( + 'Review comment belongs to a different repository or has an invalid pull request URL', + ); + } + return [match[1], Number(match[3])]; +} + +function canonicalPullRequest(pr: PullRequest): CanonicalPullRequest { + return { + number: pr.number, + headSha: requireSha(pr.head.sha, 'PR head SHA'), + baseSha: requireSha(pr.base.sha, 'PR base SHA'), + baseRef: pr.base.ref, + author: pr.user?.login ?? '', + }; +} + +function repositoryIdentityMatches( + association: WorkflowRunPullRequest, + run: WorkflowRun, + repository: string, +): boolean { + const base = association.base?.repo; + if (!base) return false; + const expectedUrl = `https://api.github.com/repos/${repository}`; + return ( + (typeof base.id === 'number' && base.id === run.repository?.id) || base.url === expectedUrl + ); +} + +function findAssociatedPr(run: WorkflowRun, repository: string): number | null { + const prs = run.pull_requests ?? []; + if (prs.length > 1) throw new Error('Workflow run is associated with multiple pull requests'); + const pr = prs[0]; + if (!pr) return null; + if ( + !Number.isSafeInteger(pr.number) || + pr.number <= 0 || + !repositoryIdentityMatches(pr, run, repository) + ) { + throw new Error('Workflow run pull request metadata does not match the trusted run'); + } + return pr.number; +} + +function canonicalComment(comment: ReviewComment): CanonicalComment { + return { + id: comment.id, + author: comment.user?.login ?? '', + authorType: comment.user?.type ?? '', + body: comment.body, + inReplyToId: comment.in_reply_to_id ?? null, + pullRequestUrl: comment.pull_request_url, + path: comment.path ?? null, + line: comment.line ?? null, + originalLine: comment.original_line ?? null, + side: comment.side ?? null, + startLine: comment.start_line ?? null, + startSide: comment.start_side ?? null, + diffHunk: comment.diff_hunk ?? null, + commitId: nullableSha(comment.commit_id, 'review comment commit SHA'), + originalCommitId: nullableSha(comment.original_commit_id, 'review comment original commit SHA'), + }; +} + +export function triggerRoute( + context: Pick, +): TriggerRoute { + if (context.event === 'pull_request') return 'review'; + const comment = context.comment; + if (!comment) return 'none'; + if (comment.inReplyToId !== null) return 'feedback'; + if (/@docker-agent(?=[^a-zA-Z0-9_-]|$)/.test(comment.body) && !comment.body.startsWith('/review')) + return 'mention'; + return 'none'; +} + +async function findForkPr( + octokit: Octokit, + run: WorkflowRun, + owner: string, + repo: string, +): Promise { + const headRepository = run.head_repository?.full_name; + const headOwner = run.head_repository?.owner?.login; + const headBranch = run.head_branch; + if (!headRepository || !headOwner || !headBranch) + throw new Error('Workflow run has no pull request locator'); + const candidates = await octokit.paginate(octokit.rest.pulls.list, { + owner, + repo, + head: `${headOwner}:${headBranch}`, + state: 'open', + per_page: 100, + }); + const matches = candidates.filter( + (candidate) => candidate.head.repo?.full_name === headRepository, + ); + if (matches.length !== 1) + throw new Error('Workflow run pull request lookup was ambiguous or empty'); + return matches[0].number; +} + +export async function resolveTriggerContext( + inputs: TriggerContextInputs, +): Promise { + if (!inputs.artifactDirectory) throw new Error('TRIGGER_ARTIFACT_DIRECTORY is not set'); + const runId = requireRunId(inputs.triggerRunId); + const [owner, repo] = parseRepository(inputs.repository); + const octokit = new Octokit({ auth: inputs.repoToken }); + const { data: run } = await octokit.rest.actions.getWorkflowRun({ owner, repo, run_id: runId }); + if (run.repository?.full_name !== inputs.repository) + throw new Error('Workflow run belongs to a different repository'); + if (run.status !== 'completed' || run.conclusion !== 'success') + throw new Error('Workflow run did not complete successfully'); + if (run.event !== 'pull_request' && run.event !== 'pull_request_review_comment') + throw new Error(`Unsupported workflow run event: '${run.event ?? ''}'`); + const event = run.event; + assertArtifactEvent(inputs.artifactDirectory, event); + const runHeadSha = requireSha(run.head_sha, 'workflow run head SHA'); + const actor = run.actor?.login ?? ''; + if (!actor) throw new Error('Workflow run has no original actor'); + + if (event === 'pull_request') { + const associatedPr = findAssociatedPr(run, inputs.repository); + const prNumber = associatedPr ?? (await findForkPr(octokit, run, owner, repo)); + const { data: pr } = await octokit.rest.pulls.get({ owner, repo, pull_number: prNumber }); + const pullRequest = canonicalPullRequest(pr); + return { + event, + runId, + runHeadSha, + headAdvanced: pullRequest.headSha !== runHeadSha, + actor, + pullRequest, + comment: null, + }; + } + + const commentId = readCommentId(inputs.artifactDirectory); + const { data: reviewComment } = await octokit.rest.pulls.getReviewComment({ + owner, + repo, + comment_id: commentId, + }); + if (reviewComment.user?.login !== actor) + throw new Error('Live review comment author does not match original workflow run actor'); + const [, prNumber] = assertPrUrl(reviewComment.pull_request_url, inputs.repository); + const associatedPr = findAssociatedPr(run, inputs.repository); + if (associatedPr !== null && associatedPr !== prNumber) + throw new Error('Review comment PR does not match workflow run PR'); + const { data: pr } = await octokit.rest.pulls.get({ owner, repo, pull_number: prNumber }); + const pullRequest = canonicalPullRequest(pr); + return { + event, + runId, + runHeadSha, + headAdvanced: pullRequest.headSha !== runHeadSha, + actor, + pullRequest, + comment: canonicalComment(reviewComment), + }; +} diff --git a/tsup.config.ts b/tsup.config.ts index 2574834..f99f802 100644 --- a/tsup.config.ts +++ b/tsup.config.ts @@ -35,6 +35,7 @@ const entry = { 'migrate-consumer-refs': src('migrate-consumer-refs'), 'post-mention-reply': src('post-mention-reply'), 'rate-limit': src('rate-limit'), + 'resolve-trigger-context': src('resolve-trigger-context'), 'score-confidence': src('score-confidence'), 'score-risk': src('score-risk'), security: src('security'), From 0851ee9d910837ea49ff0294532562bdf9160bea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arnaud=20He=CC=81ritier?= Date: Mon, 17 Aug 2026 22:51:07 +0200 Subject: [PATCH 3/7] fix(review-pr): canonicalize fork review context and pin review SHA --- .github/workflows/review-pr.yml | 160 +++++++++++++++++------- review-pr/action.yml | 111 +++++++++------- review-pr/agents/pr-review.yaml | 29 ++--- review-pr/agents/refs/posting-format.md | 6 +- src/prepare-review/index.ts | 28 +++++ src/prepare-review/prepare-review.ts | 116 +++++++++++++++++ tsup.config.ts | 1 + 7 files changed, 336 insertions(+), 115 deletions(-) create mode 100644 src/prepare-review/index.ts create mode 100644 src/prepare-review/prepare-review.ts diff --git a/.github/workflows/review-pr.yml b/.github/workflows/review-pr.yml index df197df..988cd2d 100644 --- a/.github/workflows/review-pr.yml +++ b/.github/workflows/review-pr.yml @@ -98,8 +98,9 @@ permissions: # progress is false so an in-flight review/reply is never killed mid-post. # Per-PR request *frequency* is enforced by the rate-limit check below, and the # in-action cache lock (review-pr/action.yml) prevents concurrent reviews; the -# workflow_run/fork path (PR number only in the artifact) falls back to a per-run -# group, where those two mechanisms still bound abuse. +# workflow_run/fork context is server-resolved after the group is evaluated, so +# it falls back to a per-run group; the cache lock still prevents concurrent +# reviews for the resolved PR and bounds abuse. concurrency: group: pr-review-${{ github.event.pull_request.number || github.event.issue.number || inputs.pr-number || github.run_id }}-${{ github.event.comment.id || github.event_name }} cancel-in-progress: false @@ -112,10 +113,16 @@ jobs: trigger-event: ${{ steps.read.outputs.event-name }} pr-number: ${{ steps.read.outputs.pr-number }} pr-head-sha: ${{ steps.read.outputs.pr-head-sha }} + pr-base-sha: ${{ steps.read.outputs.pr-base-sha }} + trigger-route: ${{ steps.read.outputs.trigger-route }} comment-has-mention: ${{ steps.read.outputs.comment-has-mention }} comment-is-review-cmd: ${{ steps.read.outputs.comment-is-review-cmd }} - requested-reviewer: ${{ steps.read.outputs.requested-reviewer }} - comment-json: ${{ steps.read.outputs.comment-json }} + canonical-context-available: ${{ steps.read.outputs.canonical-context-available }} + # Downstream reply jobs run on isolated runners. The resolver uploads the + # server-derived context and consumers select it only by this immutable + # same-run ID, never by artifact name. download-artifact verifies the + # archive digest from GitHub's artifact metadata for this ID. + canonical-context-artifact-id: ${{ steps.canonical-context.outputs.artifact-id }} comment-author: ${{ steps.read.outputs.comment-author }} comment-in-reply-to-id: ${{ steps.read.outputs.comment-in-reply-to-id }} comment-author-type: ${{ steps.read.outputs.comment-author-type }} @@ -165,58 +172,99 @@ jobs: exit 1 fi + - name: Create trigger context directory + id: trigger-context-directory + if: steps.context-exists.outputs.exists == 'true' + shell: bash + env: + RUNNER_TEMP: ${{ runner.temp }} + RUN_ATTEMPT: ${{ github.run_attempt }} + run: | + set -euo pipefail + runner_temp=$(realpath -e "${RUNNER_TEMP:?}") + context_dir=$(mktemp -d "$runner_temp/docker-agent-trigger-context-attempt-${RUN_ATTEMPT:?}.XXXXXX") + chmod 700 "$context_dir" + [ "$(stat -c '%a' "$context_dir")" = 700 ] + echo "path=$context_dir" >> "$GITHUB_OUTPUT" + - name: Download trigger context if: steps.context-exists.outputs.exists == 'true' uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: name: pr-review-context - path: /tmp/context + path: ${{ steps.trigger-context-directory.outputs.path }} run-id: ${{ inputs.trigger-run-id }} github-token: ${{ env.GITHUB_APP_TOKEN }} - - name: Read context + - name: Guard trigger context directory + if: steps.context-exists.outputs.exists == 'true' + shell: bash + env: + RUNNER_TEMP: ${{ runner.temp }} + CONTEXT_DIRECTORY: ${{ steps.trigger-context-directory.outputs.path }} + run: | + set -euo pipefail + runner_temp=$(realpath -e "${RUNNER_TEMP:?}") + [ -n "${CONTEXT_DIRECTORY:-}" ] && [ -d "$CONTEXT_DIRECTORY" ] && [ ! -L "$CONTEXT_DIRECTORY" ] + context_dir=$(realpath -e "$CONTEXT_DIRECTORY") + case "$context_dir" in "$runner_temp"/*) ;; *) exit 1 ;; esac + chmod 700 "$context_dir" + [ "$(stat -c '%a' "$context_dir")" = 700 ] + + - name: Resolve trusted trigger context if: steps.context-exists.outputs.exists == 'true' id: read shell: bash + env: + TRIGGER_RUN_ID: ${{ inputs.trigger-run-id }} + TRIGGER_ARTIFACT_DIRECTORY: ${{ steps.trigger-context-directory.outputs.path }} + RUNNER_TEMP: ${{ runner.temp }} run: | - # Validate required artifact files exist - if [ ! -f /tmp/context/event_name.txt ] || [ ! -f /tmp/context/pr_number.txt ]; then - echo "::error::Required artifact files missing (event_name.txt or pr_number.txt)" - exit 1 + HELPER="$DOCKER_AGENT_ACTION_ROOT/dist/resolve-trigger-context.js" + if [ ! -f "$HELPER" ]; then + echo "canonical-context-available=false" >> "$GITHUB_OUTPUT" + echo "::warning::Skipping workflow_run trigger: the pinned action bundle lacks the trusted trigger-context resolver. Direct event routes remain available." + exit 0 fi + node "$HELPER" + echo "canonical-context-available=true" >> "$GITHUB_OUTPUT" - echo "event-name=$(cat /tmp/context/event_name.txt)" >> $GITHUB_OUTPUT - echo "pr-number=$(cat /tmp/context/pr_number.txt)" >> $GITHUB_OUTPUT - if [ -f /tmp/context/pr_head_sha.txt ]; then - echo "pr-head-sha=$(cat /tmp/context/pr_head_sha.txt)" >> $GITHUB_OUTPUT - fi - if [ -f /tmp/context/requested_reviewer.txt ]; then - echo "requested-reviewer=$(cat /tmp/context/requested_reviewer.txt)" >> $GITHUB_OUTPUT - fi - if [ -f /tmp/context/comment.json ]; then - # Use heredoc output syntax ā€” jq -c compacts JSON but comment bodies - # can still contain literal newlines that break single-line echo. - { - echo 'comment-json<> $GITHUB_OUTPUT - echo "comment-author=$(jq -r '.user.login' /tmp/context/comment.json)" >> $GITHUB_OUTPUT - echo "comment-in-reply-to-id=$(jq -r '.in_reply_to_id // empty' /tmp/context/comment.json)" >> $GITHUB_OUTPUT - echo "comment-has-mention=$(jq -r 'if (.body | contains("@docker-agent")) then "true" else "false" end' /tmp/context/comment.json)" >> $GITHUB_OUTPUT - echo "comment-is-review-cmd=$(jq -r 'if (.body | startswith("/review")) then "true" else "false" end' /tmp/context/comment.json)" >> $GITHUB_OUTPUT - echo "comment-author-type=$(jq -r '.user.type // empty' /tmp/context/comment.json)" >> $GITHUB_OUTPUT - fi + - name: Guard canonical trigger context before upload + if: steps.read.outputs.canonical-context-available == 'true' + shell: bash + env: + RUNNER_TEMP: ${{ runner.temp }} + CANONICAL_CONTEXT: ${{ steps.read.outputs.canonical-context-path }} + run: | + set -euo pipefail + runner_temp=$(realpath -e "${RUNNER_TEMP:?}") + [ -n "${CANONICAL_CONTEXT:-}" ] && [ "$(basename "$CANONICAL_CONTEXT")" = canonical-trigger-context.json ] + [ -f "$CANONICAL_CONTEXT" ] && [ ! -L "$CANONICAL_CONTEXT" ] + parent=$(dirname "$CANONICAL_CONTEXT") + [ ! -L "$parent" ] + resolved_parent=$(realpath -e "$parent") + resolved_file=$(realpath -e "$CANONICAL_CONTEXT") + case "$resolved_parent" in "$runner_temp"/*) ;; *) exit 1 ;; esac + [ "$resolved_file" = "$resolved_parent/canonical-trigger-context.json" ] + [ "$(stat -c '%a' "$resolved_parent")" = 700 ] + [ "$(stat -c '%a' "$resolved_file")" = 600 ] + + # Jobs run on isolated runners. Downstream reply jobs receive only this + # server-derived file selected by its immutable same-run artifact ID. + - name: Upload canonical trigger context + id: canonical-context + if: steps.read.outputs.canonical-context-available == 'true' + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: trusted-trigger-context-${{ github.run_id }}-${{ github.run_attempt }} + path: ${{ steps.read.outputs.canonical-context-path }} + if-no-files-found: error + retention-days: 1 review: needs: [resolve-context] - # The trigger (workflow_run) path additionally gates on the requested reviewer - # captured in the artifact: only a docker-agent request may start a review, - # mirroring the requested_reviewer.login gate on the direct pull_request path. - # Empty is allowed through ā€” it means a legacy trigger artifact (predating - # requested_reviewer.txt) or an auto-review event (opened/ready_for_review). - # This is a cost gate, not authorization: the artifact is never trusted for - # auth, which stays server-side in check-org-membership. + # Triggered workflow_run reviews require canonical server-fetched context. + # Authorization remains server-side in check-org-membership. if: | always() && ( (github.event_name == 'issue_comment' && @@ -231,8 +279,7 @@ jobs: (github.event_name == 'pull_request' && github.event.action != 'review_requested' && github.event.sender.type != 'Bot' && github.event.sender.login != 'docker-agent' && github.event.sender.login != 'docker-agent[bot]') || (github.event_name == 'pull_request' && github.event.action == 'review_requested' && github.event.requested_reviewer.login == 'docker-agent') || inputs.pr-number != '' || - (needs.resolve-context.result == 'success' && needs.resolve-context.outputs.trigger-event == 'pull_request' && - (needs.resolve-context.outputs.requested-reviewer == 'docker-agent' || needs.resolve-context.outputs.requested-reviewer == '')) + (needs.resolve-context.result == 'success' && needs.resolve-context.outputs.canonical-context-available == 'true' && needs.resolve-context.outputs.trigger-event == 'pull_request' && needs.resolve-context.outputs.trigger-route == 'review') ) runs-on: ubuntu-latest # Budget: review agent hard-capped at 2700 s total + feedback processing at @@ -259,6 +306,8 @@ jobs: env: GH_TOKEN: ${{ github.token }} TRIGGER_PR_NUMBER: ${{ needs.resolve-context.outputs.pr-number }} + TRIGGER_PR_HEAD_SHA: ${{ needs.resolve-context.outputs.pr-head-sha }} + TRIGGER_PR_BASE_SHA: ${{ needs.resolve-context.outputs.pr-base-sha }} INPUT_PR_NUMBER: ${{ inputs.pr-number }} EVENT_ISSUE_NUMBER: ${{ github.event.issue.number }} EVENT_PR_NUMBER: ${{ github.event.pull_request.number }} @@ -285,6 +334,24 @@ jobs: fi echo "number=$PR_NUMBER" >> $GITHUB_OUTPUT echo "Resolved PR #$PR_NUMBER" + if [ -n "$TRIGGER_PR_NUMBER" ]; then + PR_HEAD_SHA="$TRIGGER_PR_HEAD_SHA" + PR_BASE_SHA="$TRIGGER_PR_BASE_SHA" + else + PR_SNAPSHOT=$(gh api "repos/${GITHUB_REPOSITORY}/pulls/$PR_NUMBER") + PR_HEAD_SHA=$(jq -r '.head.sha // empty' <<<"$PR_SNAPSHOT") + PR_BASE_SHA=$(jq -r '.base.sha // empty' <<<"$PR_SNAPSHOT") + fi + if ! [[ "$PR_HEAD_SHA" =~ ^[0-9a-fA-F]{40}$ ]]; then + echo "::error::Selected PR head SHA is invalid" + exit 1 + fi + if ! [[ "$PR_BASE_SHA" =~ ^[0-9a-fA-F]{40}$ ]]; then + echo "::error::Selected PR base SHA is invalid" + exit 1 + fi + echo "head-sha=$PR_HEAD_SHA" >> $GITHUB_OUTPUT + echo "base-sha=$PR_BASE_SHA" >> $GITHUB_OUTPUT - name: Check for /review command if: github.event_name == 'issue_comment' @@ -327,6 +394,7 @@ jobs: EVENT_ACTION: ${{ github.event.action }} INPUT_PR_NUMBER: ${{ inputs.pr-number }} RESOLVE_CONTEXT_RESULT: ${{ needs.resolve-context.result }} + CANONICAL_CONTEXT_AVAILABLE: ${{ needs.resolve-context.outputs.canonical-context-available }} IS_REVIEW_COMMAND: ${{ steps.command.outputs.is_review }} run: | # Classify whether this is a user-requested or automatic review. @@ -341,7 +409,7 @@ jobs: elif [ -n "$INPUT_PR_NUMBER" ]; then echo "user_requested=true" >> $GITHUB_OUTPUT echo "šŸŽÆ Trigger type: user-requested (explicit pr-number input)" - elif [ "$RESOLVE_CONTEXT_RESULT" = "success" ]; then + elif [ "$RESOLVE_CONTEXT_RESULT" = "success" ] && [ "$CANONICAL_CONTEXT_AVAILABLE" = "true" ]; then echo "user_requested=true" >> $GITHUB_OUTPUT echo "šŸŽÆ Trigger type: user-requested (trigger path via resolve-context)" else @@ -428,6 +496,7 @@ jobs: uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 env: PR_NUMBER: ${{ steps.pr.outputs.number }} + PR_HEAD_SHA: ${{ steps.pr.outputs.head-sha }} with: github-token: ${{ github.token }} script: | @@ -442,7 +511,7 @@ jobs: owner: context.repo.owner, repo: context.repo.repo, name: 'PR Review', - head_sha: pr.head.sha, + head_sha: process.env.PR_HEAD_SHA, status: 'in_progress', started_at: new Date().toISOString(), details_url: runUrl @@ -458,7 +527,7 @@ jobs: uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: fetch-depth: 0 - ref: refs/pull/${{ steps.pr.outputs.number }}/head + ref: ${{ steps.pr.outputs.head-sha }} - name: Run PR Review if: | @@ -470,7 +539,8 @@ jobs: continue-on-error: true uses: docker/docker-agent-action/review-pr@b08367e7e9a471822464be2bb9fdaa1c0fb6061b # v2.0.4 with: - pr-number: ${{ steps.pr.outputs.number }} + pr-head-sha: ${{ steps.pr.outputs.head-sha }} + pr-base-sha: ${{ steps.pr.outputs.base-sha }} comment-id: ${{ inputs.comment-id || github.event.comment.id }} additional-prompt: ${{ inputs.additional-prompt }} add-prompt-files: ${{ inputs.add-prompt-files }} diff --git a/review-pr/action.yml b/review-pr/action.yml index d95911e..e2eaba0 100644 --- a/review-pr/action.yml +++ b/review-pr/action.yml @@ -14,6 +14,14 @@ inputs: description: "Comment ID for reactions (auto-detected from github.event if not provided)" required: false default: "" + pr-head-sha: + description: "Immutable selected PR head commit SHA. Must be provided with pr-base-sha." + required: false + default: "" + pr-base-sha: + description: "Immutable selected PR base commit SHA. Must be provided with pr-head-sha." + required: false + default: "" additional-prompt: description: "Additional instructions appended to the review (e.g., language-specific patterns, project conventions)" required: false @@ -248,46 +256,17 @@ runs: gh api "repos/$REPO/issues/comments/$COMMENT_ID/reactions" \ -X POST -f content='eyes' || true - - name: Get PR information + - name: Prepare immutable PR snapshot if: steps.lock-check.outputs.skip != 'true' id: pr-info shell: bash env: - GH_TOKEN: ${{ steps.resolve-token.outputs.token }} + ACTION_PATH: ${{ github.action_path }} + GITHUB_TOKEN: ${{ steps.resolve-token.outputs.token }} PR_NUMBER: ${{ steps.resolve-context.outputs.pr-number }} - REPO: ${{ github.repository }} - READ_TOKEN: ${{ github.token }} - run: | - PR_URL="https://github.com/${REPO}/pull/${PR_NUMBER}" - - gh pr view "$PR_URL" --json files -q '.files[].path' > changed_files.txt - gh pr view "$PR_URL" --json title,body,author,baseRefName,headRefName > pr_metadata.json - echo "files_count=$(wc -l < changed_files.txt | tr -d ' ')" >> $GITHUB_OUTPUT - - # Pre-fetch diff for the agent so it doesn't need to call gh pr diff itself. - # Uses github.token (has contents:read) instead of the App token (used for posting reviews). - # Uses the full PR URL instead of just the number ā€” gh can't resolve a bare number - # from a detached HEAD (refs/pull/N/head). - if GH_TOKEN="$READ_TOKEN" gh pr diff "$PR_URL" > pr.diff 2>pr_diff_stderr.txt; then - echo "āœ… Pre-fetched PR diff via gh pr diff ($(wc -l < pr.diff | tr -d ' ') lines)" - rm -f pr_diff_stderr.txt - else - echo "::warning::gh pr diff failed: $(cat pr_diff_stderr.txt 2>/dev/null)" - rm -f pr.diff pr_diff_stderr.txt - - # Fallback: git diff with merge-base (repo is checked out with full history) - MERGE_BASE=$(git merge-base origin/main HEAD 2>/dev/null || git merge-base origin/master HEAD 2>/dev/null || echo "") - if [ -n "$MERGE_BASE" ]; then - if git diff "$MERGE_BASE"...HEAD > pr.diff; then - echo "āœ… Pre-fetched PR diff via git diff ($(wc -l < pr.diff | tr -d ' ') lines)" - else - echo "::warning::git diff failed ā€” agent will fetch diff itself" - rm -f pr.diff - fi - else - echo "::warning::Could not determine merge base ā€” agent will fetch diff itself" - fi - fi + PR_HEAD_SHA: ${{ inputs.pr-head-sha }} + PR_BASE_SHA: ${{ inputs.pr-base-sha }} + run: node "$ACTION_PATH/../dist/prepare-review.js" # Incremental review: when a previous completed review exists, narrow pr.diff # to the commits pushed since it. The original full diff is preserved at @@ -737,6 +716,7 @@ runs: uses: docker/docker-agent-action@b08367e7e9a471822464be2bb9fdaa1c0fb6061b # v2.0.4 env: ACTION_PATH: ${{ github.action_path }} + PR_HEAD_SHA: ${{ steps.pr-info.outputs.head-sha }} with: agent: ${{ env.ACTION_PATH }}/agents/pr-review-feedback.yaml prompt: | @@ -791,6 +771,7 @@ runs: shell: bash env: PR_NUMBER: ${{ steps.resolve-context.outputs.pr-number }} + PR_HEAD_SHA: ${{ steps.pr-info.outputs.head-sha }} EXTRA_PROMPT: ${{ inputs.additional-prompt }} REPO: ${{ github.repository }} CONFIDENCE_SCORE: ${{ steps.resolve-confidence.outputs.score }} @@ -853,11 +834,14 @@ runs: fi echo "---" echo "" + echo "- **Selected review SHA**: \`${PR_HEAD_SHA}\`" + echo "- **Posting command**: \`/tmp/refs/posting-format.md\` contains this exact immutable SHA." + echo "" echo "## Instructions" echo "" echo "Execute the review pipeline:" echo "" - echo "1. **Gather**: Read the pre-fetched \`pr.diff\` file. If missing, run \`gh pr diff https://github.com/${REPO}/pull/${PR_NUMBER} > pr.diff\` (use the full URL, not just the number) so the validator reads the same diff" + echo "1. **Gather**: Read the pre-fetched \`pr.diff\` file. It is derived from the immutable selected base and head commits; do NOT replace it by fetching a live PR diff." echo '2. **Draft**: Delegate to `drafter` agent to generate bug hypotheses' echo '3. **Verify**: For each hypothesis, delegate to `verifier` agent' echo '4. **Post**: Aggregate findings and post review via `gh api`' @@ -883,13 +867,27 @@ runs: } >> $GITHUB_OUTPUT - name: Copy reference files + id: copy-reference-files if: steps.lock-check.outputs.skip != 'true' shell: bash env: ACTION_PATH: ${{ github.action_path }} + PR_HEAD_SHA: ${{ steps.pr-info.outputs.head-sha }} run: | mkdir -p /tmp/refs cp "$ACTION_PATH"/agents/refs/*.md /tmp/refs/ + if ! [[ "$PR_HEAD_SHA" =~ ^[0-9a-fA-F]{40}$ ]]; then + 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 grep -q '__PR_HEAD_SHA__\|\$PR_HEAD_SHA' /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" + exit 1 + fi + echo "posting-reference=/tmp/refs/posting-format.md" >> "$GITHUB_OUTPUT" # Stage the suggestion-block validator where the agent can run it before # posting (the agent's working dir is the consumer repo, not the action). if [ -f "$ACTION_PATH/../dist/validate-suggestions.js" ]; then @@ -1036,7 +1034,13 @@ runs: SKIP_REASON: ${{ steps.lock-check.outputs.skip-reason }} LOCK_AGE: ${{ steps.lock-check.outputs.lock-age }} CHUNK_COUNT: ${{ steps.chunk-diff.outputs.chunk_count }} + PR_HEAD_SHA: ${{ steps.pr-info.outputs.head-sha }} + POSTING_REFERENCE: ${{ steps.copy-reference-files.outputs.posting-reference }} run: | + post_review() { + gh api "repos/$REPOSITORY/pulls/$PR_NUMBER/reviews" --input - + } + REVIEW_URL="https://github.com/$REPOSITORY/pull/$PR_NUMBER" echo "review-url=$REVIEW_URL" >> $GITHUB_OUTPUT TIMEOUT_NOTE="" @@ -1048,7 +1052,19 @@ runs: STATUS="ā­ļø **Review skipped** ā€” another review is already in progress" elif [ -z "$EXIT_CODE" ]; then STATUS="ā­ļø **Review skipped** ā€” agent did not run" - elif [ "$EXIT_CODE" = "124" ]; then + else + if ! [[ "$PR_HEAD_SHA" =~ ^[0-9a-fA-F]{40}$ ]]; then + echo "::error::Selected PR head SHA is invalid; refusing to post a PR review" >&2 + exit 1 + fi + if [ ! -f "$POSTING_REFERENCE" ] || \ + grep -q '__PR_HEAD_SHA__\|\$PR_HEAD_SHA' "$POSTING_REFERENCE" || \ + [ "$(grep -o -- '--arg commit_id' "$POSTING_REFERENCE" | wc -l | tr -d ' ')" != 1 ] || \ + ! grep -q -- "--arg commit_id \"$PR_HEAD_SHA\"" "$POSTING_REFERENCE"; then + echo "::error::Rendered posting reference does not contain exactly one selected immutable SHA" >&2 + exit 1 + fi + if [ "$EXIT_CODE" = "124" ]; then # Timeout (SIGKILL after 2700 s) ā€” provide actionable guidance STATUS="ā±ļø **Review timed out** (exit code: 124, limit: 2700 s)" TIMEOUT_NOTE="- **Exit code:** 124 (SIGKILL ā€” 2700 s timeout)" @@ -1061,10 +1077,9 @@ runs: else TIMEOUT_BODY="ā±ļø **PR Review Timed Out** ā€” The review agent hit the 2700 s time limit. This usually happens on large or complex diffs. Re-request a review from \`docker-agent\` to retry ā€” if it times out again, consider splitting the PR into smaller pieces." fi - if ! jq -n --arg body "$TIMEOUT_BODY" --arg event "COMMENT" \ - '{body: $body, event: $event, comments: []}' \ - | gh api "repos/$REPOSITORY/pulls/$PR_NUMBER/reviews" --input - \ - 2>&1; then + if ! jq -n --arg body "$TIMEOUT_BODY" --arg event "COMMENT" --arg commit_id "$PR_HEAD_SHA" \ + '{body: $body, event: $event, commit_id: $commit_id, comments: []}' \ + | post_review 2>&1; then echo "::warning::Failed to post timeout comment to PR" fi elif [ "$EXIT_CODE" != "0" ]; then @@ -1076,10 +1091,9 @@ runs: STATUS="āš ļø **Review completed with warnings** (exit code: $EXIT_CODE)" else STATUS="āŒ **Review failed** (exit code: $EXIT_CODE)" - if ! jq -n --arg body "āŒ **PR Review Failed** ā€” The review agent encountered an error and could not complete the review. [View logs]($RUN_URL)." --arg event "COMMENT" \ - '{body: $body, event: $event, comments: []}' \ - | gh api "repos/$REPOSITORY/pulls/$PR_NUMBER/reviews" --input - \ - 2>&1; then + if ! jq -n --arg body "āŒ **PR Review Failed** ā€” The review agent encountered an error and could not complete the review. [View logs]($RUN_URL)." --arg event "COMMENT" --arg commit_id "$PR_HEAD_SHA" \ + '{body: $body, event: $event, commit_id: $commit_id, comments: []}' \ + | post_review 2>&1; then echo "::warning::Failed to post fallback comment to PR" fi fi @@ -1102,13 +1116,14 @@ runs: echo "ā„¹ļø Fallback LGTM review already exists ā€” skipping duplicate post" else echo "::warning::Agent exited 0 but no review was posted ā€” posting fallback LGTM review" - jq -n --arg body "šŸŸ¢ **No issues found** ā€” LGTM! [View logs]($RUN_URL)." --arg event "COMMENT" \ - '{body: $body, event: $event, comments: []}' \ - | gh api "repos/$REPOSITORY/pulls/$PR_NUMBER/reviews" --input - 2>&1 || \ + jq -n --arg body "šŸŸ¢ **No issues found** ā€” LGTM! [View logs]($RUN_URL)." --arg event "COMMENT" --arg commit_id "$PR_HEAD_SHA" \ + '{body: $body, event: $event, commit_id: $commit_id, comments: []}' \ + | post_review 2>&1 || \ echo "::warning::Failed to post fallback LGTM review to PR" fi fi fi + fi # Override the default summary with a cleaner one for PR reviews { diff --git a/review-pr/agents/pr-review.yaml b/review-pr/agents/pr-review.yaml index af654e2..0aaadcd 100644 --- a/review-pr/agents/pr-review.yaml +++ b/review-pr/agents/pr-review.yaml @@ -84,15 +84,10 @@ agents: first one that works). The drafter and the suggestion validator both read `pr.diff`, so a fallback MUST redirect into it, not just print to stdout ā€” otherwise the validator sees no diff and strips every suggestion: - 1. Check if `pr.diff` already exists in the working directory (pre-fetched by the CI workflow) - 2. `gh pr diff > pr.diff` using the full PR URL (e.g., `https://github.com/owner/repo/pull/123`). - NEVER use just the number ā€” `gh pr diff ` fails in detached HEAD checkouts. - 3. `git diff $(git merge-base origin/main HEAD)...HEAD > pr.diff` ā€” the repo is checked out - with full history, so this always works as a last resort. - Do NOT try `curl`, `gh repo clone`, or any other method. The three options above are sufficient. - After obtaining the diff, log which method succeeded: + 1. Check that `pr.diff` already exists in the working directory (pre-fetched from the immutable base/head snapshot by the CI workflow). If it is missing, stop and report that the trusted review diff is unavailable; never fetch a live PR diff or substitute another range. + After obtaining the pre-fetched diff, log: ```bash - echo "DIFF_METHOD=pr.diff" # or gh_pr_diff or git_diff (whichever worked) + echo "DIFF_METHOD=trusted_snapshot" echo "DIFF_LINES=$(wc -l < pr.diff)" ``` b. PR metadata is included in the prompt above (title, author, branch, file list, @@ -478,18 +473,12 @@ agents: Some repos lack branch protection; `APPROVE` would bypass human review, `REQUEST_CHANGES` would block merging. The bot provides feedback only. - **Zero-findings example** (use this exact pattern ONLY when the findings list is - empty AND the merged `review_complete` is true ā€” an incomplete review must - instead post the "### āš ļø Review incomplete" body from Decision Rules rule 4, - never a šŸŸ¢ APPROVE body): - ```bash - REVIEW_BODY="### Assessment: šŸŸ¢ APPROVE" - jq -n --arg body "$REVIEW_BODY" --arg event "COMMENT" \ - '{body: $body, event: $event, comments: []}' \ - | gh api repos/{owner}/{repo}/pulls/{pr}/reviews --input - - ``` - Replace `{owner}`, `{repo}`, and `{pr}` with the actual values from the PR URL. - This call is mandatory even when there are zero findings ā€” do not skip it. + **Zero-findings posting** (use only when findings are empty AND the merged + `review_complete` is true ā€” incomplete reviews must instead post the + "### āš ļø Review incomplete" body from Decision Rules rule 4, never a šŸŸ¢ APPROVE body): + set `REVIEW_BODY="### Assessment: šŸŸ¢ APPROVE"` and use the rendered command in + `/tmp/refs/posting-format.md`. It already contains the validated immutable `commit_id`. + This call is mandatory even when there are zero findings. - **Console output mode**: Output markdown (see Console format below). Never call `gh api`. diff --git a/review-pr/agents/refs/posting-format.md b/review-pr/agents/refs/posting-format.md index 639b5eb..cb947b0 100644 --- a/review-pr/agents/refs/posting-format.md +++ b/review-pr/agents/refs/posting-format.md @@ -110,12 +110,14 @@ jq '[.[] | select(.body | length > 0)]' /tmp/review_comments.json > /tmp/review_ && mv /tmp/review_comments.tmp /tmp/review_comments.json echo "Posting review with $(jq length /tmp/review_comments.json) inline comment(s)" -# Use jq to assemble the final payload with proper escaping +# 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. 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, comments: $comments[0]}' \ + '{body: $body, event: $event, commit_id: $commit_id, comments: $comments[0]}' \ | gh api repos/{owner}/{repo}/pulls/{pr}/reviews --input - ``` diff --git a/src/prepare-review/index.ts b/src/prepare-review/index.ts new file mode 100644 index 0000000..30778c7 --- /dev/null +++ b/src/prepare-review/index.ts @@ -0,0 +1,28 @@ +// Copyright The Docker Agent Action authors +// SPDX-License-Identifier: Apache-2.0 + +import * as core from '@actions/core'; +import { prepareReview } from './prepare-review.js'; + +export async function main(): Promise { + const prepared = await prepareReview({ + repository: process.env.GITHUB_REPOSITORY ?? '', + pullNumber: process.env.PR_NUMBER ?? '', + githubToken: process.env.GITHUB_TOKEN ?? '', + workspace: process.env.GITHUB_WORKSPACE ?? process.cwd(), + headSha: process.env.PR_HEAD_SHA || undefined, + baseSha: process.env.PR_BASE_SHA || undefined, + }); + for (const [name, value] of Object.entries({ + 'head-sha': prepared.headSha, + 'base-sha': prepared.baseSha, + 'changed-files': String(prepared.changedFiles), + })) + core.setOutput(name, value); +} + +if (process.argv[1]?.endsWith('prepare-review.js') && !process.env.VITEST) { + main().catch((error: unknown) => + core.setFailed(error instanceof Error ? error.message : String(error)), + ); +} diff --git a/src/prepare-review/prepare-review.ts b/src/prepare-review/prepare-review.ts new file mode 100644 index 0000000..9d35a18 --- /dev/null +++ b/src/prepare-review/prepare-review.ts @@ -0,0 +1,116 @@ +// Copyright The Docker Agent Action authors +// SPDX-License-Identifier: Apache-2.0 + +import { execFileSync } from 'node:child_process'; +import { writeFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { Octokit } from '@octokit/rest'; + +export interface PrepareReviewInputs { + repository: string; + pullNumber: string; + githubToken: string; + workspace: string; + headSha?: string; + baseSha?: string; +} + +export interface PreparedReview { + headSha: string; + baseSha: string; + baseRef: string; + title: string; + body: string; + author: string; + changedFiles: number; +} + +function requireSha(value: string, label: string): string { + if (!/^[a-f0-9]{40}$/i.test(value)) throw new Error(`Invalid ${label}: '${value}'`); + return value; +} + +function parseRepository(value: string): [string, string] { + const [owner, repo, ...extra] = value.split('/'); + if (!owner || !repo || extra.length) throw new Error(`Invalid repository: '${value}'`); + return [owner, repo]; +} + +function requirePullNumber(value: string): number { + if (!/^\d+$/.test(value) || Number(value) <= 0) + throw new Error(`Invalid pull request number: '${value}'`); + return Number(value); +} + +export const GIT_OUTPUT_MAX_BUFFER = 64 * 1024 * 1024; + +function git(workspace: string, args: string[]): string { + try { + return execFileSync('git', args, { + cwd: workspace, + encoding: 'utf8', + maxBuffer: GIT_OUTPUT_MAX_BUFFER, + stdio: ['ignore', 'pipe', 'pipe'], + }); + } catch (error) { + throw new Error(`Git ${args.join(' ')} failed while preparing the immutable PR snapshot`, { + cause: error, + }); + } +} + +function ensureObject(workspace: string, sha: string): void { + try { + git(workspace, ['cat-file', '-e', `${sha}^{commit}`]); + } catch { + try { + git(workspace, ['fetch', '--no-tags', 'origin', sha]); + } catch { + throw new Error( + `Selected commit ${sha} is unavailable. Use actions/checkout with fetch-depth: 0 or fetch that immutable SHA before invoking review-pr.`, + ); + } + git(workspace, ['cat-file', '-e', `${sha}^{commit}`]); + } +} + +export async function prepareReview(inputs: PrepareReviewInputs): Promise { + const [owner, repo] = parseRepository(inputs.repository); + const pullNumber = requirePullNumber(inputs.pullNumber); + const hasHead = Boolean(inputs.headSha); + const hasBase = Boolean(inputs.baseSha); + if (hasHead !== hasBase) throw new Error('pr-head-sha and pr-base-sha must be provided together'); + const octokit = new Octokit({ auth: inputs.githubToken }); + const { data: pr } = await octokit.rest.pulls.get({ owner, repo, pull_number: pullNumber }); + const liveHead = requireSha(pr.head.sha, 'PR head SHA'); + const liveBase = requireSha(pr.base.sha, 'PR base SHA'); + const headSha = hasHead ? requireSha(inputs.headSha ?? '', 'pr-head-sha') : liveHead; + const baseSha = hasBase ? requireSha(inputs.baseSha ?? '', 'pr-base-sha') : liveBase; + + ensureObject(inputs.workspace, baseSha); + ensureObject(inputs.workspace, headSha); + git(inputs.workspace, ['checkout', '--detach', headSha]); + const diff = git(inputs.workspace, ['diff', '--binary', `${baseSha}...${headSha}`]); + const changedFiles = git(inputs.workspace, ['diff', '--name-only', `${baseSha}...${headSha}`]); + writeFileSync(join(inputs.workspace, 'pr.diff'), diff); + writeFileSync(join(inputs.workspace, 'changed_files.txt'), changedFiles); + writeFileSync( + join(inputs.workspace, 'pr_metadata.json'), + `${JSON.stringify({ + title: pr.title, + body: pr.body ?? '', + author: { login: pr.user?.login ?? '' }, + baseRefName: pr.base.ref, + headRefName: pr.head.ref, + })}\n`, + ); + return { + headSha, + baseSha, + baseRef: pr.base.ref, + title: pr.title, + body: pr.body ?? '', + author: pr.user?.login ?? '', + changedFiles: changedFiles.split('\n').filter(Boolean).length, + }; +} diff --git a/tsup.config.ts b/tsup.config.ts index f99f802..9664847 100644 --- a/tsup.config.ts +++ b/tsup.config.ts @@ -34,6 +34,7 @@ const entry = { 'mention-reply': src('mention-reply'), 'migrate-consumer-refs': src('migrate-consumer-refs'), 'post-mention-reply': src('post-mention-reply'), + 'prepare-review': src('prepare-review'), 'rate-limit': src('rate-limit'), 'resolve-trigger-context': src('resolve-trigger-context'), 'score-confidence': src('score-confidence'), From 1553e4fa6aed5fa36652d8090415c66948be8724 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arnaud=20He=CC=81ritier?= Date: Thu, 20 Aug 2026 13:01:44 +0200 Subject: [PATCH 4/7] fix(review-pr): rebuild feedback replies from live context --- .github/workflows/review-pr.yml | 160 ++++++++++++++++++++++++-------- 1 file changed, 123 insertions(+), 37 deletions(-) diff --git a/.github/workflows/review-pr.yml b/.github/workflows/review-pr.yml index 988cd2d..c167364 100644 --- a/.github/workflows/review-pr.yml +++ b/.github/workflows/review-pr.yml @@ -593,7 +593,7 @@ jobs: if: | always() && needs.resolve-context.result != 'failure' && ( (github.event_name == 'pull_request_review_comment' && github.event.comment.in_reply_to_id && github.event.comment.user.login != 'docker-agent' && github.event.comment.user.type != 'Bot') || - (needs.resolve-context.result == 'success' && needs.resolve-context.outputs.trigger-event == 'pull_request_review_comment' && needs.resolve-context.outputs.comment-in-reply-to-id != '' && needs.resolve-context.outputs.comment-author != 'docker-agent' && needs.resolve-context.outputs.comment-author-type != 'Bot' && needs.resolve-context.outputs.comment-author != 'docker-agent[bot]') + (inputs.trigger-run-id != '' && needs.resolve-context.result == 'success' && needs.resolve-context.outputs.canonical-context-available == 'true' && needs.resolve-context.outputs.canonical-context-artifact-id != '' && needs.resolve-context.outputs.trigger-event == 'pull_request_review_comment' && needs.resolve-context.outputs.trigger-route == 'feedback' && needs.resolve-context.outputs.comment-in-reply-to-id != '' && needs.resolve-context.outputs.comment-author != 'docker-agent' && needs.resolve-context.outputs.comment-author-type != 'Bot' && needs.resolve-context.outputs.comment-author != 'docker-agent[bot]') ) runs-on: ubuntu-latest permissions: @@ -601,11 +601,32 @@ jobs: pull-requests: write issues: write id-token: write - actions: read # download cross-run artifacts + actions: read # download same-run canonical context artifacts steps: + - name: Create feedback context directory + id: feedback-context-directory + shell: bash + env: + RUNNER_TEMP: ${{ runner.temp }} + RUN_ATTEMPT: ${{ github.run_attempt }} + run: | + set -euo pipefail + runner_temp=$(realpath -e "${RUNNER_TEMP:?}") + root=$(mktemp -d "$runner_temp/docker-agent-feedback-context-attempt-${RUN_ATTEMPT:?}.XXXXXX") + canonical="$root/canonical" + mkdir "$canonical" + chmod 700 "$root" "$canonical" + [ "$(stat -c '%a' "$root")" = 700 ] && [ "$(stat -c '%a' "$canonical")" = 700 ] + reply_comment=$(mktemp "$root/reply-comment.XXXXXX.json") + comment_body=$(mktemp "$root/comment-body.XXXXXX.txt") + chmod 600 "$reply_comment" "$comment_body" + echo "root=$root" >> "$GITHUB_OUTPUT" + echo "canonical=$canonical" >> "$GITHUB_OUTPUT" + echo "reply-comment=$reply_comment" >> "$GITHUB_OUTPUT" + echo "comment-body=$comment_body" >> "$GITHUB_OUTPUT" - - name: Setup credentials + - name: Setup cross-run credentials if: inputs.trigger-run-id != '' uses: docker/docker-agent-action/setup-credentials@b08367e7e9a471822464be2bb9fdaa1c0fb6061b # v2.0.4 @@ -619,48 +640,110 @@ jobs: exit 1 fi - - name: Download trigger context - # Bypass secret-masking: instead of consuming comment-json from - # resolve-context job outputs (which GitHub Actions silently drops when - # the value looks like a secret), download the artifact directly. - if: inputs.trigger-run-id != '' + - name: Validate canonical context artifact ID + id: canonical-artifact + if: inputs.trigger-run-id != '' && needs.resolve-context.result == 'success' && needs.resolve-context.outputs.canonical-context-available == 'true' && needs.resolve-context.outputs.canonical-context-artifact-id != '' + shell: bash + env: + ARTIFACT_ID: ${{ needs.resolve-context.outputs.canonical-context-artifact-id }} + run: | + if [[ "$ARTIFACT_ID" =~ ^[1-9][0-9]*$ ]]; then + echo "valid=true" >> "$GITHUB_OUTPUT" + else + echo "::error::Canonical context artifact ID is invalid" + exit 1 + fi + + - name: Download canonical trigger context + if: inputs.trigger-run-id != '' && steps.canonical-artifact.outputs.valid == 'true' uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: - name: pr-review-context - path: /tmp/context - run-id: ${{ inputs.trigger-run-id }} - github-token: ${{ env.GITHUB_APP_TOKEN }} + artifact-ids: ${{ needs.resolve-context.outputs.canonical-context-artifact-id }} + path: ${{ steps.feedback-context-directory.outputs.canonical }} + digest-mismatch: error + + - name: Guard downloaded feedback context + if: inputs.trigger-run-id != '' && steps.canonical-artifact.outputs.valid == 'true' + shell: bash + env: + RUNNER_TEMP: ${{ runner.temp }} + CONTEXT_ROOT: ${{ steps.feedback-context-directory.outputs.root }} + CANONICAL_DIRECTORY: ${{ steps.feedback-context-directory.outputs.canonical }} + run: | + set -euo pipefail + runner_temp=$(realpath -e "${RUNNER_TEMP:?}") + [ -d "$CONTEXT_ROOT" ] && [ ! -L "$CONTEXT_ROOT" ] && [ -d "$CANONICAL_DIRECTORY" ] && [ ! -L "$CANONICAL_DIRECTORY" ] + root=$(realpath -e "$CONTEXT_ROOT") + canonical_dir=$(realpath -e "$CANONICAL_DIRECTORY") + case "$root:$canonical_dir" in "$runner_temp"/*:"$root"/canonical) ;; *) exit 1 ;; esac + [ "$(find "$canonical_dir" -maxdepth 1 -type f -name canonical-trigger-context.json | wc -l | tr -d ' ')" = 1 ] + context="$canonical_dir/canonical-trigger-context.json" + [ -f "$context" ] && [ ! -L "$context" ] && [ "$(realpath -e "$context")" = "$canonical_dir/canonical-trigger-context.json" ] + chmod 700 "$root" "$canonical_dir" + chmod 600 "$context" + [ "$(stat -c '%a' "$root")" = 700 ] && [ "$(stat -c '%a' "$canonical_dir")" = 700 ] && [ "$(stat -c '%a' "$context")" = 600 ] - name: Parse comment context id: feedback shell: bash env: - # Direct path (same-repo pull_request_review_comment) + GH_TOKEN: ${{ github.token }} EVENT_COMMENT_JSON: ${{ toJSON(github.event.comment) }} EVENT_PR_NUMBER: ${{ github.event.pull_request.number }} - EVENT_PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} EVENT_NAME: ${{ github.event_name }} + TRIGGER_RUN_ID: ${{ inputs.trigger-run-id }} + CANONICAL_CONTEXT: ${{ steps.feedback-context-directory.outputs.canonical }}/canonical-trigger-context.json + REPLY_COMMENT_FILE: ${{ steps.feedback-context-directory.outputs.reply-comment }} + COMMENT_BODY_FILE: ${{ steps.feedback-context-directory.outputs.comment-body }} run: | - if [ -f /tmp/context/comment.json ]; then - # Trigger path: downloaded artifact (bypasses secret-masking on job outputs) - cp /tmp/context/comment.json /tmp/reply_comment.json - echo "pr-number=$(cat /tmp/context/pr_number.txt || { echo '::error::pr_number.txt missing from artifact'; exit 1; })" >> $GITHUB_OUTPUT - echo "pr-head-sha=$(cat /tmp/context/pr_head_sha.txt 2>/dev/null || echo '')" >> $GITHUB_OUTPUT + if [ -n "$TRIGGER_RUN_ID" ]; then + if ! jq -e --arg run_id "$TRIGGER_RUN_ID" ' + (.runId | tostring) == $run_id and + .event == "pull_request_review_comment" and + (.pullRequest.number | (type == "number" and . > 0)) and + (.pullRequest.headSha | type == "string" and test("^[0-9a-fA-F]{40}$")) and + (.comment | type == "object") and + (.comment.id | type == "number" and . > 0) and + (.comment.inReplyToId | type == "number" and . > 0) + ' "$CANONICAL_CONTEXT" > /dev/null; then + echo "::error::Canonical trigger context is malformed or belongs to a different run" + exit 1 + fi + # Preserve the server-derived anchor so replies retain direct-event fidelity. + jq '.comment | { + id, + body, + in_reply_to_id: .inReplyToId, + path, + line, + original_line: .originalLine, + side, + start_line: .startLine, + start_side: .startSide, + diff_hunk: .diffHunk, + commit_id: .commitId, + original_commit_id: .originalCommitId, + user: {login: .author, type: .authorType} + }' "$CANONICAL_CONTEXT" > "$REPLY_COMMENT_FILE" + echo "pr-number=$(jq -r '.pullRequest.number' "$CANONICAL_CONTEXT")" >> $GITHUB_OUTPUT + PR_HEAD_SHA=$(jq -r '.pullRequest.headSha' "$CANONICAL_CONTEXT") elif [ "$EVENT_NAME" = "pull_request_review_comment" ]; then - # Direct path: comment data from event payload - printf '%s' "$EVENT_COMMENT_JSON" > /tmp/reply_comment.json + printf '%s' "$EVENT_COMMENT_JSON" > "$REPLY_COMMENT_FILE" echo "pr-number=$EVENT_PR_NUMBER" >> $GITHUB_OUTPUT - echo "pr-head-sha=$EVENT_PR_HEAD_SHA" >> $GITHUB_OUTPUT + PR_HEAD_SHA=$(gh api "repos/$GITHUB_REPOSITORY/pulls/$EVENT_PR_NUMBER" --jq '.head.sha') else echo "::error::No comment data available" exit 1 fi - - echo "comment-id=$(jq -r '.id' /tmp/reply_comment.json)" >> $GITHUB_OUTPUT - echo "comment-author=$(jq -r '.user.login' /tmp/reply_comment.json)" >> $GITHUB_OUTPUT - # Use // empty so null in_reply_to_id becomes empty string (not the literal "null") - echo "parent-id=$(jq -r '.in_reply_to_id // empty' /tmp/reply_comment.json)" >> $GITHUB_OUTPUT - jq -r '.body' /tmp/reply_comment.json > /tmp/comment_body.txt + if ! [[ "$PR_HEAD_SHA" =~ ^[0-9a-fA-F]{40}$ ]]; then + echo "::error::Selected PR head SHA is invalid" + exit 1 + fi + echo "pr-head-sha=$PR_HEAD_SHA" >> $GITHUB_OUTPUT + echo "comment-id=$(jq -r '.id' "$REPLY_COMMENT_FILE")" >> $GITHUB_OUTPUT + echo "comment-author=$(jq -r '.user.login' "$REPLY_COMMENT_FILE")" >> $GITHUB_OUTPUT + echo "parent-id=$(jq -r '.inReplyToId // .in_reply_to_id // empty' "$REPLY_COMMENT_FILE")" >> $GITHUB_OUTPUT + jq -r '.body' "$REPLY_COMMENT_FILE" > "$COMMENT_BODY_FILE" - name: Check if reply is to agent comment id: check @@ -806,6 +889,7 @@ jobs: LINE: ${{ steps.check.outputs.line }} TRIGGER_COMMENT_AUTHOR: ${{ steps.feedback.outputs.comment-author }} TRIGGER_COMMENT_ID: ${{ steps.feedback.outputs.comment-id }} + COMMENT_BODY_FILE: ${{ steps.feedback-context-directory.outputs.comment-body }} run: | # Fetch the root comment (fail early if the API call errors) root=$(gh api "repos/$REPO/pulls/comments/$ROOT_ID") || { @@ -818,7 +902,7 @@ jobs: # Uses --paginate to handle PRs with >100 review comments. # Each page is processed by jq independently, then merged with jq -s. # Note: the triggering comment may not appear here due to eventual - # consistency, so we append it from /tmp/comment_body.txt below. + # consistency, so we append it from $COMMENT_BODY_FILE below. all_comments=$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/comments" | \ jq -s --arg root_id "$ROOT_ID" \ '[.[][] | select((.in_reply_to_id | tostring) == $root_id)] | sort_by(.created_at)') || { @@ -853,7 +937,7 @@ jobs: if [ "$reply_count" -gt 0 ]; then for i in $(seq 0 $((reply_count - 1))); do comment_id=$(echo "$all_comments" | jq -r ".[$i].id") || continue - # Skip the triggering comment ā€” we append it from /tmp/comment_body.txt below + # Skip the triggering comment ā€” we append it from $COMMENT_BODY_FILE below if [ "$comment_id" = "$TRIGGER_COMMENT_ID" ]; then continue fi @@ -869,22 +953,22 @@ jobs: done fi - # Always append the triggering comment last ā€” sourced from /tmp/comment_body.txt + # Always append the triggering comment last ā€” sourced from $COMMENT_BODY_FILE # (written by the Parse comment context step; safe for multi-line bodies). echo "[REPLY by @$TRIGGER_COMMENT_AUTHOR] ā† this is the reply you are responding to" - cat /tmp/comment_body.txt + cat "$COMMENT_BODY_FILE" echo "" echo "$DELIM" } >> $GITHUB_OUTPUT - echo "āœ… Built thread context with replies (triggering comment from /tmp/comment_body.txt)" + echo "āœ… Built thread context with replies (triggering comment from $COMMENT_BODY_FILE)" - name: Checkout PR head if: steps.check.outputs.is_agent == 'true' && steps.auth.outputs.authorized == 'true' uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: fetch-depth: 0 - ref: refs/pull/${{ steps.feedback.outputs.pr-number }}/head + ref: ${{ steps.feedback.outputs.pr-head-sha }} - name: Run reply if: steps.check.outputs.is_agent == 'true' && steps.auth.outputs.authorized == 'true' @@ -906,14 +990,16 @@ jobs: - name: Save feedback artifact if: steps.check.outputs.is_agent == 'true' shell: bash + env: + REPLY_COMMENT_FILE: ${{ steps.feedback-context-directory.outputs.reply-comment }} run: | - # /tmp/reply_comment.json was written by the "Parse comment context" step - if [ ! -f /tmp/reply_comment.json ]; then + # $REPLY_COMMENT_FILE was written by the "Parse comment context" step + if [ ! -f "$REPLY_COMMENT_FILE" ]; then echo "::warning::No comment JSON available, skipping feedback capture" exit 0 fi mkdir -p feedback - cp /tmp/reply_comment.json feedback/feedback.json + cp "$REPLY_COMMENT_FILE" feedback/feedback.json # Validate it's parseable JSON before uploading if ! jq empty feedback/feedback.json 2>/dev/null; then echo "::warning::Feedback JSON is malformed, skipping" From adf890cfe9a7d6e8b4e392b5a418647b4c9fd1b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arnaud=20He=CC=81ritier?= Date: Thu, 20 Aug 2026 13:01:44 +0200 Subject: [PATCH 5/7] fix(review-pr): rebuild mention replies from live context --- .github/workflows/review-pr.yml | 182 ++++++++++++++++++--------- src/resolve-trigger-context/index.ts | 4 - 2 files changed, 121 insertions(+), 65 deletions(-) diff --git a/.github/workflows/review-pr.yml b/.github/workflows/review-pr.yml index c167364..a7822bc 100644 --- a/.github/workflows/review-pr.yml +++ b/.github/workflows/review-pr.yml @@ -115,8 +115,6 @@ jobs: pr-head-sha: ${{ steps.read.outputs.pr-head-sha }} pr-base-sha: ${{ steps.read.outputs.pr-base-sha }} trigger-route: ${{ steps.read.outputs.trigger-route }} - comment-has-mention: ${{ steps.read.outputs.comment-has-mention }} - comment-is-review-cmd: ${{ steps.read.outputs.comment-is-review-cmd }} canonical-context-available: ${{ steps.read.outputs.canonical-context-available }} # Downstream reply jobs run on isolated runners. The resolver uploads the # server-derived context and consumers select it only by this immutable @@ -1036,13 +1034,7 @@ jobs: !startsWith(github.event.comment.body, '/review') && github.event.comment.user.login != 'docker-agent' && github.event.comment.user.type != 'Bot') || - (needs.resolve-context.result == 'success' && - needs.resolve-context.outputs.trigger-event == 'pull_request_review_comment' && - needs.resolve-context.outputs.comment-in-reply-to-id == '' && - needs.resolve-context.outputs.comment-has-mention == 'true' && - needs.resolve-context.outputs.comment-is-review-cmd != 'true' && - needs.resolve-context.outputs.comment-author != 'docker-agent' && - needs.resolve-context.outputs.comment-author-type != 'Bot') + (inputs.trigger-run-id != '' && needs.resolve-context.result == 'success' && needs.resolve-context.outputs.canonical-context-available == 'true' && needs.resolve-context.outputs.canonical-context-artifact-id != '' && needs.resolve-context.outputs.trigger-event == 'pull_request_review_comment' && needs.resolve-context.outputs.trigger-route == 'mention' && needs.resolve-context.outputs.comment-in-reply-to-id == '' && needs.resolve-context.outputs.comment-author != 'docker-agent' && needs.resolve-context.outputs.comment-author-type != 'Bot') ) runs-on: ubuntu-latest permissions: @@ -1050,73 +1042,140 @@ jobs: pull-requests: write issues: write id-token: write - actions: read # download cross-run artifacts + actions: read # download same-run canonical context artifacts steps: - - name: Setup credentials + - name: Setup direct credentials uses: docker/docker-agent-action/setup-credentials@b08367e7e9a471822464be2bb9fdaa1c0fb6061b # v2.0.4 - - name: Download trigger context - if: inputs.trigger-run-id != '' + - name: Validate canonical context artifact ID + id: canonical-artifact + if: inputs.trigger-run-id != '' && needs.resolve-context.result == 'success' && needs.resolve-context.outputs.canonical-context-available == 'true' && needs.resolve-context.outputs.canonical-context-artifact-id != '' + shell: bash + env: + ARTIFACT_ID: ${{ needs.resolve-context.outputs.canonical-context-artifact-id }} + run: | + if [[ "$ARTIFACT_ID" =~ ^[1-9][0-9]*$ ]]; then + echo "valid=true" >> "$GITHUB_OUTPUT" + else + echo "::error::Canonical context artifact ID is invalid" + exit 1 + fi + + - name: Create mention context directory + id: mention-context-directory + shell: bash + env: + RUNNER_TEMP: ${{ runner.temp }} + RUN_ATTEMPT: ${{ github.run_attempt }} + run: | + set -euo pipefail + runner_temp=$(realpath -e "${RUNNER_TEMP:?}") + root=$(mktemp -d "$runner_temp/docker-agent-mention-context-attempt-${RUN_ATTEMPT:?}.XXXXXX") + canonical="$root/canonical" + mkdir "$canonical" + mention_event=$(mktemp "$root/mention-event.XXXXXX.json") + chmod 700 "$root" "$canonical" + chmod 600 "$mention_event" + echo "root=$root" >> "$GITHUB_OUTPUT" + echo "canonical=$canonical" >> "$GITHUB_OUTPUT" + echo "mention-event=$mention_event" >> "$GITHUB_OUTPUT" + + - name: Download canonical trigger context + if: inputs.trigger-run-id != '' && steps.canonical-artifact.outputs.valid == 'true' uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: - name: pr-review-context - path: /tmp/context - run-id: ${{ inputs.trigger-run-id }} - github-token: ${{ env.GITHUB_APP_TOKEN }} + artifact-ids: ${{ needs.resolve-context.outputs.canonical-context-artifact-id }} + path: ${{ steps.mention-context-directory.outputs.canonical }} + digest-mismatch: error + + - name: Guard downloaded mention context + if: inputs.trigger-run-id != '' && steps.canonical-artifact.outputs.valid == 'true' + shell: bash + env: + RUNNER_TEMP: ${{ runner.temp }} + CONTEXT_ROOT: ${{ steps.mention-context-directory.outputs.root }} + CANONICAL_DIRECTORY: ${{ steps.mention-context-directory.outputs.canonical }} + run: | + set -euo pipefail + runner_temp=$(realpath -e "${RUNNER_TEMP:?}") + [ -d "$CONTEXT_ROOT" ] && [ ! -L "$CONTEXT_ROOT" ] && [ -d "$CANONICAL_DIRECTORY" ] && [ ! -L "$CANONICAL_DIRECTORY" ] + root=$(realpath -e "$CONTEXT_ROOT") + canonical_dir=$(realpath -e "$CANONICAL_DIRECTORY") + case "$root:$canonical_dir" in "$runner_temp"/*:"$root"/canonical) ;; *) exit 1 ;; esac + [ "$(find "$canonical_dir" -maxdepth 1 -type f -name canonical-trigger-context.json | wc -l | tr -d ' ')" = 1 ] + context="$canonical_dir/canonical-trigger-context.json" + [ -f "$context" ] && [ ! -L "$context" ] && [ "$(realpath -e "$context")" = "$canonical_dir/canonical-trigger-context.json" ] + chmod 700 "$root" "$canonical_dir" + chmod 600 "$context" + [ "$(stat -c '%a' "$root")" = 700 ] && [ "$(stat -c '%a' "$canonical_dir")" = 700 ] && [ "$(stat -c '%a' "$context")" = 600 ] - name: Synthesize mention-reply event context - if: inputs.trigger-run-id != '' + if: inputs.trigger-run-id != '' && steps.canonical-artifact.outputs.valid == 'true' shell: bash + env: + TRIGGER_RUN_ID: ${{ inputs.trigger-run-id }} + CANONICAL_CONTEXT: ${{ steps.mention-context-directory.outputs.canonical }}/canonical-trigger-context.json + MENTION_EVENT_FILE: ${{ steps.mention-context-directory.outputs.mention-event }} run: | - if [ ! -f /tmp/context/comment.json ]; then - echo "::warning::comment.json not found in artifact ā€” cannot synthesize mention event" - exit 0 - fi - PR_NUMBER=$(cat /tmp/context/pr_number.txt 2>/dev/null || echo '') - if [ -z "$PR_NUMBER" ]; then - echo "::warning::pr_number.txt missing or empty ā€” cannot synthesize mention event" - exit 0 + if ! jq -e --arg run_id "$TRIGGER_RUN_ID" ' + (.runId | tostring) == $run_id and + .event == "pull_request_review_comment" and + (.pullRequest.number | (type == "number" and . > 0)) and + (.pullRequest.headSha | type == "string" and test("^[0-9a-fA-F]{40}$")) and + (.comment | type == "object") and + (.comment.id | type == "number" and . > 0) and + (.comment.inReplyToId == null) + ' "$CANONICAL_CONTEXT" > /dev/null; then + echo "::error::Canonical trigger context is malformed or belongs to a different run" + exit 1 fi - PR_HEAD_SHA=$(cat /tmp/context/pr_head_sha.txt 2>/dev/null || echo '') - REPO_NAME="${GITHUB_REPOSITORY##*/}" - REPO_FULL="${GITHUB_REPOSITORY}" - REPO_OWNER="${GITHUB_REPOSITORY_OWNER}" - jq -n \ - --slurpfile comment /tmp/context/comment.json \ - --arg pr_number "$PR_NUMBER" \ - --arg pr_head_sha "$PR_HEAD_SHA" \ - --arg repo_name "$REPO_NAME" \ - --arg repo_full_name "$REPO_FULL" \ - --arg repo_owner "$REPO_OWNER" \ - '{ - action: "created", - pull_request: { - number: ($pr_number | tonumber), - head: { sha: $pr_head_sha } - }, - comment: $comment[0], - repository: { - name: $repo_name, - full_name: $repo_full_name, - owner: { login: $repo_owner } - }, - sender: $comment[0].user - }' > /tmp/mention_event.json - echo "āœ… Synthesized mention-reply event context at /tmp/mention_event.json" + jq '. as $context | { + action: "created", + pull_request: { number: $context.pullRequest.number, head: { sha: $context.pullRequest.headSha } }, + comment: { + id: $context.comment.id, + body: $context.comment.body, + in_reply_to_id: $context.comment.inReplyToId, + path: $context.comment.path, + line: $context.comment.line, + original_line: $context.comment.originalLine, + side: $context.comment.side, + start_line: $context.comment.startLine, + start_side: $context.comment.startSide, + diff_hunk: $context.comment.diffHunk, + commit_id: $context.comment.commitId, + original_commit_id: $context.comment.originalCommitId, + user: { login: $context.comment.author, type: $context.comment.authorType } + }, + repository: { full_name: env.GITHUB_REPOSITORY, name: (env.GITHUB_REPOSITORY | split("/")[1]), owner: { login: env.GITHUB_REPOSITORY_OWNER } }, + sender: { login: $context.actor } + }' "$CANONICAL_CONTEXT" > "$MENTION_EVENT_FILE" + echo "āœ… Synthesized mention-reply event context from the canonical server-fetched context" - name: Resolve event context for mention-reply action id: resolve-event shell: bash + env: + TRIGGER_RUN_ID: ${{ inputs.trigger-run-id }} + MENTION_EVENT_FILE: ${{ steps.mention-context-directory.outputs.mention-event }} run: | - if [ -f /tmp/mention_event.json ]; then - echo "path=/tmp/mention_event.json" >> $GITHUB_OUTPUT - echo "name=pull_request_review_comment" >> $GITHUB_OUTPUT + if [ -n "$TRIGGER_RUN_ID" ]; then + if [ ! -f "$MENTION_EVENT_FILE" ] || [ -L "$MENTION_EVENT_FILE" ]; then + echo "::error::Canonical mention event is unavailable" + exit 1 + fi + echo "path=$MENTION_EVENT_FILE" >> "$GITHUB_OUTPUT" + echo "name=pull_request_review_comment" >> "$GITHUB_OUTPUT" else - echo "path=$GITHUB_EVENT_PATH" >> $GITHUB_OUTPUT - echo "name=$GITHUB_EVENT_NAME" >> $GITHUB_OUTPUT + echo "path=$GITHUB_EVENT_PATH" >> "$GITHUB_OUTPUT" + echo "name=$GITHUB_EVENT_NAME" >> "$GITHUB_OUTPUT" fi + - name: Setup credentials + if: steps.resolve-event.outputs.path != '' + uses: docker/docker-agent-action/setup-credentials@b08367e7e9a471822464be2bb9fdaa1c0fb6061b # v2.0.4 + - name: Run mention-reply handler id: mention-context if: steps.resolve-event.outputs.path != '' @@ -1157,21 +1216,22 @@ jobs: GH_TOKEN: ${{ env.GITHUB_APP_TOKEN || github.token }} REPO: ${{ github.repository }} EVENT_COMMENT_ID: ${{ github.event.comment.id }} + EVENT_PATH: ${{ steps.resolve-event.outputs.path }} OUTCOME: ${{ steps.run-reply.outcome }} - EVENT_NAME: ${{ github.event_name }} + EVENT_NAME: ${{ steps.resolve-event.outputs.name }} run: | if [ "$OUTCOME" != "success" ]; then exit 0 fi COMMENT_ID="$EVENT_COMMENT_ID" - if [ -z "$COMMENT_ID" ] && [ -f /tmp/mention_event.json ]; then - COMMENT_ID=$(jq -r '.comment.id // empty' /tmp/mention_event.json) + if [ -z "$COMMENT_ID" ] && [ -f "$EVENT_PATH" ]; then + COMMENT_ID=$(jq -r '.comment.id // empty' "$EVENT_PATH") fi if [ -z "$COMMENT_ID" ]; then echo "::warning::No comment ID available ā€” skipping completion reaction" exit 0 fi - if [ -f /tmp/mention_event.json ] || [ "$EVENT_NAME" = "pull_request_review_comment" ]; then + if [ "$EVENT_NAME" = "pull_request_review_comment" ]; then gh api "repos/$REPO/pulls/comments/$COMMENT_ID/reactions" \ -X POST -f content='+1' || true else diff --git a/src/resolve-trigger-context/index.ts b/src/resolve-trigger-context/index.ts index 0c71749..5570d06 100644 --- a/src/resolve-trigger-context/index.ts +++ b/src/resolve-trigger-context/index.ts @@ -18,10 +18,6 @@ export function resolverOutputs(context: CanonicalTriggerContext): Record Date: Thu, 20 Aug 2026 13:01:45 +0200 Subject: [PATCH 6/7] docs(security): minimize trigger artifacts and document trust boundary --- .../skills/add-pr-reviewer-to-repo/SKILL.md | 34 +++++++++++++++--- .github/workflows/self-review-pr-trigger.yml | 10 ++---- AGENTS.md | 11 +++++- SECURITY.md | 20 +++++++++-- review-pr/README.md | 36 +++++++++++++------ 5 files changed, 84 insertions(+), 27 deletions(-) diff --git a/.agents/skills/add-pr-reviewer-to-repo/SKILL.md b/.agents/skills/add-pr-reviewer-to-repo/SKILL.md index 66db6e1..982feb4 100644 --- a/.agents/skills/add-pr-reviewer-to-repo/SKILL.md +++ b/.agents/skills/add-pr-reviewer-to-repo/SKILL.md @@ -132,7 +132,7 @@ Replace `@VERSION` with the tag from Step 2 (e.g. `@v2.0.0`). ## 4b. Fork PRs ā€” 2-Workflow Pattern -Fork PRs run under GitHub's security restrictions: `pull_request` and `pull_request_review_comment` events get read-only tokens, no secrets, and no OIDC. The solution is a lightweight "trigger" workflow that saves event context as an artifact; a `workflow_run` handler then picks it up with full permissions. +Fork PRs run under GitHub's security restrictions: `pull_request` and `pull_request_review_comment` events get read-only tokens, no secrets, and no OIDC. The solution is a lightweight "trigger" workflow that saves only untrusted locator hints as an artifact; a `workflow_run` handler then picks it up with full permissions. ### File 1: `.github/workflows/pr-review-trigger.yml` @@ -150,20 +150,23 @@ permissions: {} jobs: save-context: + # A review request for anyone other than docker-agent must not fan out to a review. + if: > + github.event_name != 'pull_request' || + github.event.action != 'review_requested' || + github.event.requested_reviewer.login == 'docker-agent' runs-on: ubuntu-latest steps: - name: Save event context env: PR_NUMBER: ${{ github.event.pull_request.number }} - PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} - COMMENT_JSON: ${{ toJSON(github.event.comment) }} + COMMENT_ID: ${{ github.event.comment.id }} run: | mkdir -p context printf '%s' "${{ github.event_name }}" > context/event_name.txt printf '%s' "$PR_NUMBER" > context/pr_number.txt - printf '%s' "$PR_HEAD_SHA" > context/pr_head_sha.txt if [ "${{ github.event_name }}" = "pull_request_review_comment" ]; then - printf '%s' "$COMMENT_JSON" > context/comment.json + printf '%s' "$COMMENT_ID" > context/comment_id.txt fi - name: Upload context @@ -234,6 +237,24 @@ pull_request_review_comment `issue_comment` always has full permissions regardless of fork status, so `/review` commands and `@docker-agent` mentions bypass the trigger workflow entirely. +### Trigger-artifact upgrade and rollback compatibility + +The trigger artifact is an **untrusted locator**. The reusable workflow fetches all authoritative +PR and comment data from GitHub; it only uses `event_name.txt`, `pr_number.txt`, and (for review +comments) `comment_id.txt` to locate that data. + +| Trigger artifact producer | Reusable workflow consumer | Supported? | Required order / behavior | +| --- | --- | --- | --- | +| New minimized locator artifact | Updated reusable workflow | Yes | Normal target state. The resolver uses the locator files and server-fetches authoritative values. | +| Legacy full-context artifact | Updated reusable workflow | Yes | Safe rollout bridge. The resolver extracts only the comment ID from legacy `comment.json`, then server-fetches authoritative values. | +| New minimized locator artifact | Older reusable workflow | Not guaranteed | Upgrade the reusable workflow **before** minimizing the trigger artifact. Roll back by restoring the legacy artifact format until the consumer is upgraded. | + +For this repository's self-review workflow, the currently pinned `v2.0.4` self-reference predates +`pr-head-sha` and `pr-base-sha`; it does not accept those immutable SHA inputs. After releasing the +updated reusable workflow, bump every internal pin and its version comment before relying on +dogfooding for this path. The current pin must not be treated as coverage of immutable-SHA input +wiring. + --- ## 5. Upgrade Checklist @@ -244,6 +265,9 @@ For repos that already have the workflows, verify each item: - [ ] **All required permissions are present** ā€” `contents: read`, `pull-requests: write`, `issues: write`, `id-token: write`, `actions: write`. Missing any of these causes silent failures or OIDC/artifact errors. Note: missing `actions: write` specifically causes a 403 when the reusable workflow tries to store binary cache or upload/download artifacts (cache write operations require `write`; artifact download requires only `read`). - [ ] **`checks: write` is present** (optional but recommended) ā€” without it the review won't appear as a check run on the PR. - [ ] **Bot-filter `if` condition is correct** ā€” the condition must filter out `docker-agent`, `docker-agent[bot]`, any `Bot` user type, and comments containing `` or ``. A missing or incomplete filter causes infinite review loops. +- [ ] **Fork repos: reviewer-target gate is present** ā€” if `pull_request.review_requested` is enabled, `save-context` must run it only when `github.event.requested_reviewer.login == 'docker-agent'`. A request for a human, team, or other bot must leave `save-context` skipped and must not invoke the privileged `workflow_run` handler; a request for exactly `docker-agent` proceeds. +- [ ] **Fork artifact rollout order is safe** ā€” upgrade the reusable workflow before switching the trigger to the minimized locator artifact. New minimized artifacts with an older reusable workflow are not guaranteed to work; roll back by restoring the legacy artifact format until the consumer is upgraded. +- [ ] **Repository self-review pins are upgraded after release** ā€” the currently pinned `v2.0.4` self-reference predates immutable `pr-head-sha`/`pr-base-sha` inputs. Bump its SHA and version comment after release; do not claim the current dogfood pin exercises immutable-SHA input wiring. - [ ] **Fork repos: trigger workflow has the artifact upload step** ā€” the `actions/upload-artifact` step must be present in `pr-review-trigger.yml`, pinned to a specific commit SHA (not just a tag). Without it the `workflow_run` handler has no artifact to download. - [ ] **Fork repos: `trigger-run-id` input is wired correctly** ā€” must be `${{ github.event_name == 'workflow_run' && format('{0}', github.event.workflow_run.id) || '' }}`. An empty string is safe for `issue_comment` events; the reusable workflow handles both paths. - [ ] **Fork repos: `workflow_run.workflows` array matches the trigger workflow name exactly** ā€” the string `"PR Review - Trigger"` (or whatever you named it) must match the `name:` field in `pr-review-trigger.yml` character-for-character. diff --git a/.github/workflows/self-review-pr-trigger.yml b/.github/workflows/self-review-pr-trigger.yml index 5625311..449904d 100644 --- a/.github/workflows/self-review-pr-trigger.yml +++ b/.github/workflows/self-review-pr-trigger.yml @@ -42,19 +42,13 @@ jobs: - name: Save event context env: PR_NUMBER: ${{ github.event.pull_request.number }} - PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} - REQUESTED_REVIEWER: ${{ github.event.requested_reviewer.login }} - COMMENT_JSON: ${{ toJSON(github.event.comment) }} + COMMENT_ID: ${{ github.event.comment.id }} run: | mkdir -p context printf '%s' "${{ github.event_name }}" > context/event_name.txt printf '%s' "$PR_NUMBER" > context/pr_number.txt - printf '%s' "$PR_HEAD_SHA" > context/pr_head_sha.txt - if [ "${{ github.event_name }}" = "pull_request" ]; then - printf '%s' "$REQUESTED_REVIEWER" > context/requested_reviewer.txt - fi if [ "${{ github.event_name }}" = "pull_request_review_comment" ]; then - printf '%s' "$COMMENT_JSON" > context/comment.json + printf '%s' "$COMMENT_ID" > context/comment_id.txt fi - name: Upload context diff --git a/AGENTS.md b/AGENTS.md index 60a9637..bd93b8c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -56,6 +56,14 @@ Anything else here (workflows under `.github/workflows/`, scripts, tests) exists ā”‚ ā”‚ ā”œā”€ā”€ index.ts # CLI entry ā†’ bundled to dist/incremental-review.js ā”‚ ā”‚ ā”œā”€ā”€ incremental-review.ts # Core planIncrementalReview()/findLastReviewedSha() pure functions. ā”‚ ā”‚ ā””ā”€ā”€ __tests__/ +ā”‚ ā”œā”€ā”€ prepare-review/ # Creates an immutable checked-out PR snapshot and its diff/metadata files. +ā”‚ ā”‚ ā”œā”€ā”€ index.ts # CLI entry ā†’ bundled to dist/prepare-review.js +ā”‚ ā”‚ ā”œā”€ā”€ prepare-review.ts # Resolves immutable base/head SHAs and writes review inputs. +ā”‚ ā”‚ ā””ā”€ā”€ __tests__/ +ā”‚ ā”œā”€ā”€ resolve-trigger-context/ # Server-resolves canonical workflow-run context for protected artifact transfer. +ā”‚ ā”‚ ā”œā”€ā”€ index.ts # CLI entry ā†’ bundled to dist/resolve-trigger-context.js +ā”‚ ā”‚ ā”œā”€ā”€ resolve-trigger-context.ts # Produces canonical context and downstream job outputs. +ā”‚ ā”‚ ā””ā”€ā”€ __tests__/ ā”‚ ā”œā”€ā”€ score-confidence/ # Per-finding confidence scoring for the PR review pipeline. ā”‚ ā”‚ ā”œā”€ā”€ index.ts # CLI entry ā†’ bundled to dist/score-confidence.js ā”‚ ā”‚ ā”œā”€ā”€ score-confidence.ts # Core scoreFinding()/scoreFindings() pure functions + posting policy. @@ -179,7 +187,8 @@ The action runs untrusted input (PR titles, bodies, comments, diffs) through an ### `review-pr` action specifics - Uses a **best-effort cache lock** (`pr-review-lock---*` cache key) to avoid concurrent reviews on the same PR. Completed runs release the lock by saving a `-released` marker cache entry that shadows their lock entry (cache saves work regardless of token scopes; the REST cache DELETE is best-effort cleanup only). The 3600s TTL is a fallback for crashed holders and must stay above the review agent's 2700s wall-clock budget (45 min, enforced by the root action's `total-timeout` across all attempts) so an in-flight review is never treated as stale. Reviews are idempotent so the small race window is acceptable. -- **Memory persistence** uses `actions/cache` keyed by `pr-review-memory---` with prefix-based restore. The DB lives at `${{ github.workspace }}/.cache/pr-review-memory.db`. +- **Memory persistence** uses `actions/cache` keyed by `pr-review-memory---` with prefix-based restore. The review memory database lives at `${{ github.workspace }}/.cache/pr-review-memory.db`. +- **Fork workflow-run private context files** are canonicalized from GitHub API data. Trigger artifacts are untrusted locators only; server-derived PR/comment data and an immutable 40-hex SHA drive authorization, prompts, posting, and checkout. Attempt-specific randomized `runner.temp` roots are `0700`; the resolver exclusively creates canonical JSON at `0600`, and a pre-upload guard verifies containment, non-symlink status, and exact modes. Isolated consumers select the same-run artifact by immutable ID, verify its digest, then restore and verify `0700/0600` because artifact modes are not preserved. Canonical-derived files are exclusively created at `0600` in the same private job root. Never use predictable shared `/tmp` paths for locator, canonical, or derived trigger context; unrelated reviewed runtime temporary files are outside this invariant. The artifact name includes the run ID and run attempt to avoid rerun collisions. If the pinned bundle has no resolver, workflow-run routes skip fail-closed while direct routes continue. - **Feedback loop**: the `reply-to-feedback` job in `.github/workflows/review-pr.yml` (which runs the `pr-review-reply.yaml` agent) uploads a `pr-review-feedback` artifact on every reply via its "Upload feedback artifact" step. The next review run downloads all such artifacts, runs `pr-review-feedback.yaml` to call `add_memory(...)` for each, then deletes the artifacts. - **Bot reply detection** uses HTML markers: `` on review comments, `` on agent replies (including mention-reply responses). **Don't change these strings** ā€” workflows in consumer repos grep for them. - **Copilot-style triggers**: in addition to the original `pull_request_review` / `issue_comment /review` paths, `review-pr.yml` now also fires on: diff --git a/SECURITY.md b/SECURITY.md index f1d1596..cba4127 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -95,8 +95,8 @@ The membership check **fails closed**: if no actor login can be resolved, or the actor is not an org member, the review is skipped. `src/check-org-membership` evaluates the authorization paths in order (the PR author for automatic review, then the trusted requester for `review_requested`), resolving the PR author live -via the API so the directly-wired `pull_request` path verifies the author -instead of an empty comment author. +via the API so the directly-wired `pull_request` path verifies the author instead +of an empty comment author. ### 2. Rate anomalies are detected and throttled @@ -121,6 +121,22 @@ bound request frequency: on completion via a cache marker, 3600 s TTL fallback for crashed runs) prevents concurrent reviews from racing on the same PR. +### 3. Fork trigger artifacts and private trigger-context files + +A fork-controlled trigger artifact can contain only locator hints. The privileged +`workflow_run` route resolves the completed successful run from GitHub, validates +its base repository and immutable 40-hex head SHA, and fetches the PR and review +comment live. The resolver stages server-derived canonical context by exclusively +creating a `0600` JSON file in an attempt-specific randomized `runner.temp` root +with mode `0700`; a pre-upload guard verifies containment, non-symlink status, +and exact modes. Isolated consumers select the same-run artifact by immutable ID +and verify its digest, then restore and verify `0700/0600` because artifact modes +are not preserved. Canonical-derived files are exclusively created at `0600` in +the same private job root. Locator, canonical, and derived trigger context never +use predictable shared `/tmp` paths; this does not constrain unrelated runtime +temporary files. If the pinned bundle has no resolver, workflow-run routes skip +fail-closed while direct routes continue. + ## Security Modules All security logic lives under `src/security/` and is compiled into `dist/security.js` by diff --git a/review-pr/README.md b/review-pr/README.md index 543a965..a24452a 100644 --- a/review-pr/README.md +++ b/review-pr/README.md @@ -44,7 +44,7 @@ That's it. All three events (`pull_request`, `issue_comment`, `pull_request_revi ### Repos that accept fork PRs (2 workflows) -Fork PRs are subject to GitHub's security restrictions: `pull_request` and `pull_request_review_comment` events get **read-only tokens, no secrets, and no OIDC**. To work around this, you need a second "trigger" workflow that saves event context as an artifact, then a `workflow_run` handler picks it up with full permissions. +Fork PRs are subject to GitHub's security restrictions: `pull_request` and `pull_request_review_comment` events get **read-only tokens, no secrets, and no OIDC**. To work around this, you need a second "trigger" workflow that saves only untrusted locator hints as an artifact, then a `workflow_run` handler picks it up with full permissions. **`.github/workflows/pr-review-trigger.yml`** ā€” lightweight, no secrets needed: @@ -70,19 +70,13 @@ jobs: - name: Save event context env: PR_NUMBER: ${{ github.event.pull_request.number }} - PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} - REQUESTED_REVIEWER: ${{ github.event.requested_reviewer.login }} - COMMENT_JSON: ${{ toJSON(github.event.comment) }} + COMMENT_ID: ${{ github.event.comment.id }} run: | mkdir -p context printf '%s' "${{ github.event_name }}" > context/event_name.txt printf '%s' "$PR_NUMBER" > context/pr_number.txt - printf '%s' "$PR_HEAD_SHA" > context/pr_head_sha.txt - if [ "${{ github.event_name }}" = "pull_request" ]; then - printf '%s' "$REQUESTED_REVIEWER" > context/requested_reviewer.txt - fi if [ "${{ github.event_name }}" = "pull_request_review_comment" ]; then - printf '%s' "$COMMENT_JSON" > context/comment.json + printf '%s' "$COMMENT_ID" > context/comment_id.txt fi - name: Upload context @@ -129,6 +123,21 @@ jobs: trigger-run-id: ${{ github.event_name == 'workflow_run' && format('{0}', github.event.workflow_run.id) || '' }} ``` +> **Artifact trust boundary:** The trigger artifact is controlled by fork code and is never trusted for identity, prompt content, PR routing, posting targets, or revisions. The reusable workflow fetches the workflow run, PR, and review comment from GitHub, binds comment events to the original run actor, and checks out the resolved immutable SHA. Locator, canonical, and canonical-derived trigger context never use predictable shared `/tmp` paths: each job uses an attempt-specific randomized `runner.temp` root at `0700`; canonical JSON and derived files are exclusively created at `0600`. The producer verifies containment, non-symlink status, and exact modes before upload. Downstream jobs select the same-run artifact only by immutable ID, verify its digest, and restore and verify `0700/0600` because artifact modes are not preserved. Those jobs never download an artifact by name or read the fork trigger artifact. This invariant does not restrict unrelated runtime temporary files. +> +> **Trigger-artifact upgrade and rollback compatibility:** +> +> | Trigger artifact producer | Reusable workflow consumer | Supported? | Required order / behavior | +> | --- | --- | --- | --- | +> | New minimized locator artifact | Updated reusable workflow | Yes | Normal target state. The resolver uses `event_name.txt`, `pr_number.txt`, and (for review comments) `comment_id.txt` to locate data, then server-fetches authoritative values. | +> | Legacy full-context artifact | Updated reusable workflow | Yes | Safe rollout bridge. The resolver extracts only the comment ID from legacy `comment.json`, then server-fetches authoritative values. | +> | New minimized locator artifact | Older reusable workflow | Not guaranteed | Upgrade the reusable workflow **before** minimizing the trigger artifact. Roll back by restoring the legacy artifact format until the consumer is upgraded. | +> +> The repository's currently pinned `v2.0.4` self-reference predates immutable `pr-head-sha` and +> `pr-base-sha` inputs, so it does not accept them. After releasing the updated reusable workflow, +> bump every internal pin and its version comment before relying on dogfooding for this path. The +> current pin must not be treated as coverage of immutable-SHA input wiring. + #### How the two workflows interact ``` @@ -268,7 +277,7 @@ When using `docker/docker-agent-action/.github/workflows/review-pr.yml`: | Input | Description | Default | | ------------------- | ---------------------------------------------------------------------- | ------- | | `trigger-run-id` | Workflow run ID from `pr-review-trigger.yml` (for `workflow_run` path) | - | -| `pr-number` | PR number override (auto-detected from event or trigger artifact) | - | +| `pr-number` | PR number override (auto-detected from the direct event or server-resolved trigger-run context) | - | | `comment-id` | Comment ID for reactions (auto-detected) | - | | `additional-prompt` | Additional review guidelines | - | | `model` | Model override (e.g., `anthropic/claude-haiku-4-5`) | - | @@ -278,7 +287,10 @@ When using `docker/docker-agent-action/.github/workflows/review-pr.yml`: ### `review-pr` (Composite Action) -PR number and comment ID are auto-detected from `github.event` when not provided. +PR number and comment ID are auto-detected from `github.event` when not provided. The reusable +workflow resolves an immutable base/head snapshot for both direct and `workflow_run` routes; +all review posts use that selected head SHA as `commit_id`, so a force-push cannot retarget a +review after its diff was prepared. > **API Keys:** Provide at least one API key for your preferred provider. You don't need all of them. @@ -286,6 +298,8 @@ PR number and comment ID are auto-detected from `github.event` when not provided | -------------------------- | ---------------------------------------------------------------- | -------- | | `pr-number` | PR number (auto-detected) | No | | `comment-id` | Comment ID for reactions (auto-detected) | No | +| `pr-head-sha` | Selected immutable 40-hex PR head SHA; must be supplied with `pr-base-sha` | No | +| `pr-base-sha` | Selected immutable 40-hex PR base SHA; must be supplied with `pr-head-sha` | No | | `additional-prompt` | Additional review guidelines (appended to built-in instructions) | No | | `model` | Model override (default: `anthropic/claude-sonnet-4-5`) | No | | `anthropic-api-key` | Anthropic API key | No\* | From eca6cadaa0adcea3287b9eb338b66d2430c83f24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arnaud=20He=CC=81ritier?= Date: Thu, 20 Aug 2026 13:01:45 +0200 Subject: [PATCH 7/7] test(security): add workflow regression coverage for AIA-16 --- package.json | 3 +- pnpm-lock.yaml | 10 + .../evals/marlin-event-firing-react-1.json | 2 +- .../agents/evals/marlin-false-positive-1.json | 2 +- .../evals/marlin-identity-ingestion-1.json | 2 +- .../agents/evals/marlin-pii-detection-1.json | 2 +- .../evals/marlin-platform-context-1.json | 2 +- .../evals/marlin-string-kind-fixed-1.json | 2 +- .../__tests__/pr-review-yaml.test.ts | 4 +- .../prepare-review.integration.test.ts | 83 + .../__tests__/prepare-review.test.ts | 202 +++ .../__tests__/resolve-trigger-context.test.ts | 81 +- .../__tests__/workflow-security.test.ts | 1565 +++++++++++++++++ 13 files changed, 1924 insertions(+), 36 deletions(-) create mode 100644 src/prepare-review/__tests__/prepare-review.integration.test.ts create mode 100644 src/prepare-review/__tests__/prepare-review.test.ts create mode 100644 src/resolve-trigger-context/__tests__/workflow-security.test.ts diff --git a/package.json b/package.json index 606cc4e..fbb0f54 100644 --- a/package.json +++ b/package.json @@ -30,7 +30,8 @@ "@types/node": "22.0.0", "tsup": "8.5.1", "typescript": "5.9.3", - "vitest": "4.0.18" + "vitest": "4.0.18", + "yaml": "2.8.2" }, "packageManager": "pnpm@10.26.0", "pnpm": { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 76932f6..4901a22 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -57,6 +57,9 @@ importers: vitest: specifier: 4.0.18 version: 4.0.18(@types/node@22.0.0)(tsx@4.21.0) + yaml: + specifier: 2.8.2 + version: 2.8.2 packages: @@ -1871,6 +1874,11 @@ packages: resolution: {integrity: sha512-si7QWI6zUMq56bESFvagtmzMdGOtoxfR+Sez11Mobfc7tm+VkUckk9bW2UeffTGVUbOksxmSw0AA2gs8g71NCQ==} engines: {node: '>=12'} + yaml@2.8.2: + resolution: {integrity: sha512-mplynKqc1C2hTVYxd0PU2xQAc22TI1vShAYGksCCfxbn/dFwnHTNi1bvYsBTkhdUNtGIf5xNOg938rrSSYvS9A==} + engines: {node: '>= 14.6'} + hasBin: true + zip-stream@6.0.1: resolution: {integrity: sha512-zK7YHHz4ZXpW89AHXUPbQVGKI7uvkd3hzusTdotCg1UxyaVtg0zFJSTfW/Dq5f7OBBVnq6cZIaC8Ti4hb6dtCA==} engines: {node: '>= 14'} @@ -3982,6 +3990,8 @@ snapshots: string-width: 5.1.2 strip-ansi: 7.2.0 + yaml@2.8.2: {} + zip-stream@6.0.1: dependencies: archiver-utils: 5.0.2 diff --git a/review-pr/agents/evals/marlin-event-firing-react-1.json b/review-pr/agents/evals/marlin-event-firing-react-1.json index 191aac4..ecd8ed0 100644 --- a/review-pr/agents/evals/marlin-event-firing-react-1.json +++ b/review-pr/agents/evals/marlin-event-firing-react-1.json @@ -2,7 +2,7 @@ "id": "ed82e8e4-0b29-4a3c-80d8-55284bdfd6c0", "title": "Marlin SDK PageView fired in React render body with wrong timestamp format (run 1)", "evals": { - "setup": "apk add --no-cache github-cli && mkdir -p /tmp/refs && cp /configs/refs/posting-format.md /configs/refs/marlin_v2_producer_code_review.md /tmp/refs/", + "setup": "apk add --no-cache github-cli && mkdir -p /tmp/refs && sed 's/__PR_HEAD_SHA__/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/g' /configs/refs/posting-format.md > /tmp/refs/posting-format.md && cp /configs/refs/marlin_v2_producer_code_review.md /tmp/refs/", "relevance": [ "The agent detects the Marlin SDK import pattern (from \"@docker/data-contracts\") and recognizes this as a Marlin producer PR", "At least one finding identifies that marlin.track() is called directly in the React component render body, not inside a useEffect ā€” this causes the event to fire on every render", diff --git a/review-pr/agents/evals/marlin-false-positive-1.json b/review-pr/agents/evals/marlin-false-positive-1.json index 1636365..813502c 100644 --- a/review-pr/agents/evals/marlin-false-positive-1.json +++ b/review-pr/agents/evals/marlin-false-positive-1.json @@ -2,7 +2,7 @@ "id": "34b3988f-0263-4379-8e4a-63519969d02c", "title": "Non-Marlin analytics library ā€” should not trigger Marlin guide (run 1)", "evals": { - "setup": "apk add --no-cache github-cli && mkdir -p /tmp/refs && cp /configs/refs/posting-format.md /configs/refs/marlin_v2_producer_code_review.md /tmp/refs/", + "setup": "apk add --no-cache github-cli && mkdir -p /tmp/refs && sed 's/__PR_HEAD_SHA__/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/g' /configs/refs/posting-format.md > /tmp/refs/posting-format.md && cp /configs/refs/marlin_v2_producer_code_review.md /tmp/refs/", "relevance": [ "The agent ran 'echo $GITHUB_ACTIONS' before performing the review to detect the output mode", "The agent output the review to the console as formatted markdown instead of posting via gh api", diff --git a/review-pr/agents/evals/marlin-identity-ingestion-1.json b/review-pr/agents/evals/marlin-identity-ingestion-1.json index a6ed040..c57af0e 100644 --- a/review-pr/agents/evals/marlin-identity-ingestion-1.json +++ b/review-pr/agents/evals/marlin-identity-ingestion-1.json @@ -2,7 +2,7 @@ "id": "046121d9-ac09-4f96-9796-cb853564572e", "title": "Marlin SDK Go producer setting ingestor-owned fields, identity mismatch, and UGC truncation (run 1)", "evals": { - "setup": "apk add --no-cache github-cli && mkdir -p /tmp/refs && cp /configs/refs/posting-format.md /configs/refs/marlin_v2_producer_code_review.md /tmp/refs/", + "setup": "apk add --no-cache github-cli && mkdir -p /tmp/refs && sed 's/__PR_HEAD_SHA__/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/g' /configs/refs/posting-format.md > /tmp/refs/posting-format.md && cp /configs/refs/marlin_v2_producer_code_review.md /tmp/refs/", "relevance": [ "The agent detects the Marlin SDK import pattern (github.com/docker/data-contracts/gen/go/docker/marlin/) and recognizes this as a Marlin producer PR", "At least one finding identifies that IpAddress is set by the producer (from r.RemoteAddr) ā€” this is an ingestor-owned field that must never be set by the producer", diff --git a/review-pr/agents/evals/marlin-pii-detection-1.json b/review-pr/agents/evals/marlin-pii-detection-1.json index ba626c2..68cfc6c 100644 --- a/review-pr/agents/evals/marlin-pii-detection-1.json +++ b/review-pr/agents/evals/marlin-pii-detection-1.json @@ -2,7 +2,7 @@ "id": "cb05f5d2-8838-4302-ba78-70b6fb5f75ae", "title": "Marlin SDK AppInvoke analytics with PII risk ā€” err.Error() and command args (run 1)", "evals": { - "setup": "apk add --no-cache github-cli && mkdir -p /tmp/refs && cp /configs/refs/posting-format.md /configs/refs/marlin_v2_producer_code_review.md /tmp/refs/", + "setup": "apk add --no-cache github-cli && mkdir -p /tmp/refs && sed 's/__PR_HEAD_SHA__/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/g' /configs/refs/posting-format.md > /tmp/refs/posting-format.md && cp /configs/refs/marlin_v2_producer_code_review.md /tmp/refs/", "relevance": [ "The agent ran 'echo $GITHUB_ACTIONS' before performing the review to detect the output mode", "The agent output the review to the console as formatted markdown instead of posting via gh api", diff --git a/review-pr/agents/evals/marlin-platform-context-1.json b/review-pr/agents/evals/marlin-platform-context-1.json index 3fc0c21..e841b65 100644 --- a/review-pr/agents/evals/marlin-platform-context-1.json +++ b/review-pr/agents/evals/marlin-platform-context-1.json @@ -2,7 +2,7 @@ "id": "b5394a48-d3f1-480c-9952-3a035e4876c3", "title": "Marlin SDK web event from Node server with raw integer enum and non-UUID account_id (run 1)", "evals": { - "setup": "apk add --no-cache github-cli && mkdir -p /tmp/refs && cp /configs/refs/posting-format.md /configs/refs/marlin_v2_producer_code_review.md /tmp/refs/", + "setup": "apk add --no-cache github-cli && mkdir -p /tmp/refs && sed 's/__PR_HEAD_SHA__/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/g' /configs/refs/posting-format.md > /tmp/refs/posting-format.md && cp /configs/refs/marlin_v2_producer_code_review.md /tmp/refs/", "relevance": [ "The agent detects the Marlin SDK import pattern (from \"@docker/data-contracts\") and recognizes this as a Marlin producer PR", "At least one finding identifies that a WebPageView event (which is a PLATFORM_CONTEXT_WEB event) is being fired from a Node.js server-side Express handler ā€” it should use a PLATFORM_CONTEXT_NODE event instead", diff --git a/review-pr/agents/evals/marlin-string-kind-fixed-1.json b/review-pr/agents/evals/marlin-string-kind-fixed-1.json index b20bb15..3e96e7c 100644 --- a/review-pr/agents/evals/marlin-string-kind-fixed-1.json +++ b/review-pr/agents/evals/marlin-string-kind-fixed-1.json @@ -2,7 +2,7 @@ "id": "2a204370-80fb-4ad5-bea2-75ccb3696e64", "title": "Marlin SDK WebClick with action and elementTag from user input ā€” string_kind and action discriminator violations (run 1)", "evals": { - "setup": "apk add --no-cache github-cli && mkdir -p /tmp/refs && cp /configs/refs/posting-format.md /configs/refs/marlin_v2_producer_code_review.md /tmp/refs/", + "setup": "apk add --no-cache github-cli && mkdir -p /tmp/refs && sed 's/__PR_HEAD_SHA__/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/g' /configs/refs/posting-format.md > /tmp/refs/posting-format.md && cp /configs/refs/marlin_v2_producer_code_review.md /tmp/refs/", "relevance": [ "The agent detects the Marlin SDK import pattern (from \"@docker/data-contracts\") and recognizes this as a Marlin producer PR", "At least one finding identifies that the action field (STRING_KIND_FIXED) receives user-supplied input from req.body.action, which violates string_kind correctness", diff --git a/src/pr-review-agent/__tests__/pr-review-yaml.test.ts b/src/pr-review-agent/__tests__/pr-review-yaml.test.ts index 06749ce..91143ad 100644 --- a/src/pr-review-agent/__tests__/pr-review-yaml.test.ts +++ b/src/pr-review-agent/__tests__/pr-review-yaml.test.ts @@ -344,10 +344,10 @@ describe('root orchestration contracts', () => { it('reserves the zero-findings šŸŸ¢ APPROVE template for complete merges', () => { expect(root).toContain( - 'use this exact pattern ONLY when the findings list is empty AND the merged `review_complete` is true', + 'use only when findings are empty AND the merged `review_complete` is true', ); expect(root).toContain( - 'an incomplete review must instead post the "### āš ļø Review incomplete" body from Decision Rules rule 4, never a šŸŸ¢ APPROVE body', + 'incomplete reviews must instead post the "### āš ļø Review incomplete" body from Decision Rules rule 4, never a šŸŸ¢ APPROVE body', ); }); diff --git a/src/prepare-review/__tests__/prepare-review.integration.test.ts b/src/prepare-review/__tests__/prepare-review.integration.test.ts new file mode 100644 index 0000000..5198175 --- /dev/null +++ b/src/prepare-review/__tests__/prepare-review.integration.test.ts @@ -0,0 +1,83 @@ +// Copyright The Docker Agent Action authors +// SPDX-License-Identifier: Apache-2.0 + +import { execFileSync } from 'node:child_process'; +import { randomBytes } from 'node:crypto'; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +const getPull = vi.hoisted(() => vi.fn()); + +vi.mock('@octokit/rest', () => ({ + Octokit: class { + rest = { pulls: { get: getPull } }; + }, +})); + +import { prepareReview } from '../prepare-review.js'; + +const directories: string[] = []; + +function git(directory: string, args: string[]): string { + return execFileSync('git', args, { cwd: directory, encoding: 'utf8' }); +} + +function createRepository(): { directory: string; baseSha: string; headSha: string } { + const directory = mkdtempSync(join(tmpdir(), 'prepare-review-integration-')); + directories.push(directory); + git(directory, ['init']); + git(directory, ['config', 'user.email', 'test@example.com']); + git(directory, ['config', 'user.name', 'Test User']); + writeFileSync(join(directory, 'large.bin'), Buffer.alloc(32)); + git(directory, ['add', 'large.bin']); + git(directory, ['commit', '-m', 'base']); + const baseSha = git(directory, ['rev-parse', 'HEAD']).trim(); + writeFileSync(join(directory, 'large.bin'), randomBytes(1024 * 1024 + 64 * 1024)); + git(directory, ['add', 'large.bin']); + git(directory, ['commit', '-m', 'large binary change']); + const headSha = git(directory, ['rev-parse', 'HEAD']).trim(); + return { directory, baseSha, headSha }; +} + +afterEach(() => { + vi.clearAllMocks(); + while (directories.length) rmSync(directories.pop() as string, { recursive: true, force: true }); +}); + +describe('prepareReview integration', () => { + it('writes an immutable binary diff larger than one MiB', async () => { + const { directory, baseSha, headSha } = createRepository(); + getPull.mockResolvedValue({ + data: { + head: { sha: headSha, ref: 'feature' }, + base: { sha: baseSha, ref: 'main' }, + title: 'Large binary diff', + body: null, + user: { login: 'contributor' }, + }, + }); + + await expect( + prepareReview({ + repository: 'docker/docker-agent-action', + pullNumber: '42', + githubToken: 'token', + workspace: directory, + headSha, + baseSha, + }), + ).resolves.toMatchObject({ headSha, baseSha, changedFiles: 1 }); + + expect(readFileSync(join(directory, 'pr.diff')).byteLength).toBeGreaterThan(1024 * 1024); + expect(readFileSync(join(directory, 'changed_files.txt'), 'utf8')).toBe('large.bin\n'); + expect(JSON.parse(readFileSync(join(directory, 'pr_metadata.json'), 'utf8'))).toEqual({ + title: 'Large binary diff', + body: '', + author: { login: 'contributor' }, + baseRefName: 'main', + headRefName: 'feature', + }); + }); +}); diff --git a/src/prepare-review/__tests__/prepare-review.test.ts b/src/prepare-review/__tests__/prepare-review.test.ts new file mode 100644 index 0000000..6ecb92c --- /dev/null +++ b/src/prepare-review/__tests__/prepare-review.test.ts @@ -0,0 +1,202 @@ +// Copyright The Docker Agent Action authors +// SPDX-License-Identifier: Apache-2.0 + +import { mkdtempSync, readFileSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +const { execFileSync, getPull, MockOctokit } = vi.hoisted(() => { + const execFileSync = vi.fn(); + const getPull = vi.fn(); + class MockOctokit { + rest = { pulls: { get: getPull } }; + } + return { execFileSync, getPull, MockOctokit }; +}); + +vi.mock('node:child_process', () => ({ execFileSync })); +vi.mock('@octokit/rest', () => ({ Octokit: MockOctokit })); + +import { GIT_OUTPUT_MAX_BUFFER, prepareReview } from '../prepare-review.js'; + +const liveHead = 'a'.repeat(40); +const liveBase = 'b'.repeat(40); +const selectedHead = 'c'.repeat(40); +const selectedBase = 'd'.repeat(40); +const workspaces: string[] = []; + +function workspace(): string { + const value = mkdtempSync(join(tmpdir(), 'prepare-review-')); + workspaces.push(value); + return value; +} + +function configurePull(): void { + getPull.mockResolvedValue({ + data: { + head: { sha: liveHead, ref: 'feature' }, + base: { sha: liveBase, ref: 'main' }, + title: 'Review this', + body: 'Description', + user: { login: 'contributor' }, + }, + }); +} + +function configureGit(existing = new Set()): void { + execFileSync.mockImplementation((_command: string, args: string[]) => { + if (args[0] === 'cat-file') { + const sha = args[2]?.replace('^{commit}', '') ?? ''; + if (!existing.has(sha)) throw new Error('missing object'); + return ''; + } + if (args[0] === 'fetch') { + existing.add(args[3] ?? ''); + return ''; + } + if (args[0] === 'checkout') return ''; + if (args[0] === 'diff' && args[1] === '--binary') return 'diff --git a/file.ts b/file.ts\n'; + if (args[0] === 'diff' && args[1] === '--name-only') return 'file.ts\n'; + throw new Error(`Unexpected git invocation: ${args.join(' ')}`); + }); +} + +function inputs(overrides: Partial[0]> = {}) { + return { + repository: 'docker/docker-agent-action', + pullNumber: '42', + githubToken: 'token', + workspace: workspace(), + ...overrides, + }; +} + +afterEach(() => { + vi.clearAllMocks(); + while (workspaces.length) rmSync(workspaces.pop() as string, { recursive: true, force: true }); +}); + +describe('prepareReview', () => { + it('checks out and diffs the explicit immutable commit pair', async () => { + configurePull(); + configureGit(new Set([selectedBase, selectedHead])); + const request = inputs({ headSha: selectedHead, baseSha: selectedBase }); + + await expect(prepareReview(request)).resolves.toMatchObject({ + headSha: selectedHead, + baseSha: selectedBase, + changedFiles: 1, + }); + + expect(execFileSync).toHaveBeenCalledWith( + 'git', + ['checkout', '--detach', selectedHead], + expect.objectContaining({ cwd: request.workspace }), + ); + expect(execFileSync).toHaveBeenCalledWith( + 'git', + ['diff', '--binary', `${selectedBase}...${selectedHead}`], + expect.objectContaining({ maxBuffer: GIT_OUTPUT_MAX_BUFFER }), + ); + expect(execFileSync).toHaveBeenCalledWith( + 'git', + ['diff', '--name-only', `${selectedBase}...${selectedHead}`], + expect.objectContaining({ maxBuffer: GIT_OUTPUT_MAX_BUFFER }), + ); + expect(readFileSync(join(request.workspace, 'pr.diff'), 'utf8')).toContain('diff --git'); + expect(readFileSync(join(request.workspace, 'changed_files.txt'), 'utf8')).toBe('file.ts\n'); + expect(JSON.parse(readFileSync(join(request.workspace, 'pr_metadata.json'), 'utf8'))).toEqual({ + title: 'Review this', + body: 'Description', + author: { login: 'contributor' }, + baseRefName: 'main', + headRefName: 'feature', + }); + }); + + it('uses the current API commits only when neither immutable SHA is supplied', async () => { + configurePull(); + configureGit(new Set([liveBase, liveHead])); + + await expect(prepareReview(inputs())).resolves.toMatchObject({ + headSha: liveHead, + baseSha: liveBase, + }); + expect(execFileSync).toHaveBeenCalledWith( + 'git', + ['diff', '--binary', `${liveBase}...${liveHead}`], + expect.any(Object), + ); + }); + + it.each([ + { headSha: selectedHead }, + { baseSha: selectedBase }, + ])('rejects an incomplete immutable SHA pair', async (shaInputs) => { + await expect(prepareReview(inputs(shaInputs))).rejects.toThrow( + 'pr-head-sha and pr-base-sha must be provided together', + ); + expect(execFileSync).not.toHaveBeenCalled(); + }); + + it('validates immutable SHA inputs before using Git', async () => { + await expect( + prepareReview(inputs({ headSha: 'not-a-sha', baseSha: selectedBase })), + ).rejects.toThrow("Invalid pr-head-sha: 'not-a-sha'"); + expect(execFileSync).not.toHaveBeenCalled(); + }); + + it('fetches unavailable selected commits by their immutable SHA', async () => { + configurePull(); + configureGit(); + + await prepareReview(inputs({ headSha: selectedHead, baseSha: selectedBase })); + + expect(execFileSync).toHaveBeenCalledWith( + 'git', + ['fetch', '--no-tags', 'origin', selectedBase], + expect.any(Object), + ); + expect(execFileSync).toHaveBeenCalledWith( + 'git', + ['fetch', '--no-tags', 'origin', selectedHead], + expect.any(Object), + ); + }); + + it('preserves Git failure context for immutable diff errors', async () => { + configurePull(); + configureGit(new Set([selectedBase, selectedHead])); + const failure = new Error('stdout maxBuffer length exceeded'); + execFileSync.mockImplementation((_command: string, args: string[]) => { + if (args[0] === 'diff' && args[1] === '--binary') throw failure; + if (args[0] === 'cat-file' || args[0] === 'checkout') return ''; + throw new Error(`Unexpected git invocation: ${args.join(' ')}`); + }); + + await expect( + prepareReview(inputs({ headSha: selectedHead, baseSha: selectedBase })), + ).rejects.toSatisfy((error: unknown) => { + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toContain( + `Git diff --binary ${selectedBase}...${selectedHead} failed`, + ); + expect((error as Error).message).not.toContain('Unable to obtain the selected Git object'); + expect((error as Error & { cause?: unknown }).cause).toBe(failure); + return true; + }); + }); + + it('fails closed when an unavailable commit cannot be fetched', async () => { + configurePull(); + execFileSync.mockImplementation((_command: string, args: string[]) => { + if (args[0] === 'cat-file' || args[0] === 'fetch') throw new Error('unavailable'); + return ''; + }); + + await expect( + prepareReview(inputs({ headSha: selectedHead, baseSha: selectedBase })), + ).rejects.toThrow(`Selected commit ${selectedBase} is unavailable`); + }); +}); diff --git a/src/resolve-trigger-context/__tests__/resolve-trigger-context.test.ts b/src/resolve-trigger-context/__tests__/resolve-trigger-context.test.ts index 6fa75c5..d312cc5 100644 --- a/src/resolve-trigger-context/__tests__/resolve-trigger-context.test.ts +++ b/src/resolve-trigger-context/__tests__/resolve-trigger-context.test.ts @@ -62,6 +62,15 @@ const canonicalContext = { body: '@docker-agent review this', inReplyToId: null, pullRequestUrl: `https://api.github.com/repos/${repository}/pulls/42`, + path: 'src/example.ts', + line: 42, + originalLine: 40, + side: 'RIGHT', + startLine: 41, + startSide: 'RIGHT', + diffHunk: '@@ -40,3 +40,5 @@', + commitId: sha, + originalCommitId: 'b'.repeat(40), }, }; @@ -228,33 +237,6 @@ describe('resolveTriggerContext', () => { expect(outputs).not.toHaveProperty('actor'); }); - it.each([ - ['a canonical mention', '@docker-agent review this', 'true', 'false'], - ['a canonical review command', '/review @docker-agent', 'true', 'true'], - ['a canonical ordinary comment', 'hello', 'false', 'false'], - ['no canonical comment', null, 'false', 'false'], - ])('derives legacy mention outputs only from %s', (_description, body, mention, reviewCommand) => { - const outputs = resolverOutputs({ - ...canonicalContext, - comment: body === null ? null : { ...canonicalContext.comment, body }, - }); - - expect(outputs['comment-has-mention']).toBe(mention); - expect(outputs['comment-is-review-cmd']).toBe(reviewCommand); - expect(outputs).not.toHaveProperty('comment-body'); - expect(outputs).not.toHaveProperty('comment-id'); - }); - - it('uses the canonical live body rather than a forged artifact body for legacy mention outputs', () => { - const outputs = resolverOutputs({ - ...canonicalContext, - comment: { ...canonicalContext.comment, body: 'live body without a mention' }, - }); - - expect(outputs['comment-has-mention']).toBe('false'); - expect(outputs['comment-is-review-cmd']).toBe('false'); - }); - it('rejects a workflow run from another repository before PR lookup', async () => { getWorkflowRun.mockResolvedValue({ data: run({ repository: { full_name: 'other/repo' } }) }); const directory = artifacts({ 'event_name.txt': 'pull_request' }); @@ -454,6 +436,51 @@ describe('resolveTriggerContext', () => { } }); + it('preserves every server-derived inline anchor in canonical review comments', async () => { + getWorkflowRun.mockResolvedValue({ + data: run({ event: 'pull_request_review_comment', pull_requests: [] }), + }); + getReviewComment.mockResolvedValue({ + data: { + id: 5, + body: '@docker-agent review this', + in_reply_to_id: 1, + user: { login: 'external', type: 'User' }, + pull_request_url: `https://api.github.com/repos/${repository}/pulls/42`, + path: 'src/example.ts', + line: 42, + original_line: 40, + side: 'RIGHT', + start_line: 41, + start_side: 'RIGHT', + diff_hunk: '@@ -40,3 +40,5 @@', + commit_id: sha, + original_commit_id: 'b'.repeat(40), + }, + }); + const directory = artifacts({ + 'event_name.txt': 'pull_request_review_comment', + 'comment_id.txt': '5', + }); + try { + await expect(resolve(directory)).resolves.toMatchObject({ + comment: { + path: 'src/example.ts', + line: 42, + originalLine: 40, + side: 'RIGHT', + startLine: 41, + startSide: 'RIGHT', + diffHunk: '@@ -40,3 +40,5 @@', + commitId: sha, + originalCommitId: 'b'.repeat(40), + }, + }); + } finally { + rmSync(directory, { recursive: true }); + } + }); + it('binds a live review comment to the original actor, not a rerunner', async () => { getWorkflowRun.mockResolvedValue({ data: run({ event: 'pull_request_review_comment', pull_requests: [] }), diff --git a/src/resolve-trigger-context/__tests__/workflow-security.test.ts b/src/resolve-trigger-context/__tests__/workflow-security.test.ts new file mode 100644 index 0000000..cefbbab --- /dev/null +++ b/src/resolve-trigger-context/__tests__/workflow-security.test.ts @@ -0,0 +1,1565 @@ +// Copyright The Docker Agent Action authors +// SPDX-License-Identifier: Apache-2.0 + +import { spawnSync } from 'node:child_process'; +import { + chmodSync, + cpSync, + existsSync, + mkdtempSync, + readFileSync, + rmSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { delimiter, resolve } from 'node:path'; +import { describe, expect, it } from 'vitest'; +import { parseDocument } from 'yaml'; +import { resolverOutputs } from '../index.js'; +import type { CanonicalComment, CanonicalTriggerContext } from '../resolve-trigger-context.js'; + +const root = resolve(import.meta.dirname, '../../..'); +const safePath = '/usr/bin:/bin'; + +function testEnvironment(values: Record): NodeJS.ProcessEnv { + return { + HOME: '/tmp', + PATH: safePath, + TMPDIR: tmpdir(), + ...values, + }; +} + +type WorkflowStep = { + name?: string; + id?: string; + if?: string; + uses?: string; + with?: Record; + env?: Record; + run?: string; +}; + +type WorkflowJob = { + if?: string; + outputs?: Record; + permissions?: Record; + steps?: WorkflowStep[]; +}; + +type Workflow = { + on?: Record; + jobs: Record; +}; + +type Action = { + runs?: { steps?: WorkflowStep[] }; +}; + +function parseWorkflow(path: string): Workflow { + return parseDocument(readFileSync(path, 'utf8')).toJS() as Workflow; +} + +const workflow = parseWorkflow(resolve(root, '.github/workflows/review-pr.yml')); +const e2e = parseWorkflow(resolve(root, '.github/workflows/test-e2e.yml')); + +function job(name: string): WorkflowJob { + const result = workflow.jobs[name]; + if (!result) throw new Error(`Missing ${name} job`); + return result; +} + +function step(jobName: string, name: string): WorkflowStep { + const result = job(jobName).steps?.find((candidate) => candidate.name === name); + if (!result) throw new Error(`Missing ${name} step in ${jobName}`); + return result; +} + +function stepIndex(jobName: string, name: string): number { + const result = job(jobName).steps?.findIndex((candidate) => candidate.name === name) ?? -1; + if (result < 0) throw new Error(`Missing ${name} step in ${jobName}`); + return result; +} + +function values(value: unknown): string[] { + if (typeof value === 'string') return [value]; + if (Array.isArray(value)) return value.flatMap(values); + if (value && typeof value === 'object') return Object.values(value).flatMap(values); + return []; +} + +function resolverOutputNames(): Set { + const context: CanonicalTriggerContext = { + event: 'pull_request_review_comment', + runId: 1, + runHeadSha: 'a'.repeat(40), + actor: 'actor', + pullRequest: { + number: 1, + headSha: 'a'.repeat(40), + baseSha: 'b'.repeat(40), + baseRef: 'main', + author: 'author', + }, + comment: canonicalComment({ inReplyToId: 2 }), + }; + return new Set(Object.keys(resolverOutputs(context))); +} + +const canonicalContext: CanonicalTriggerContext = { + event: 'pull_request_review_comment', + runId: 123, + runHeadSha: 'a'.repeat(40), + actor: 'actor', + pullRequest: { + number: 1, + headSha: 'a'.repeat(40), + baseSha: 'b'.repeat(40), + baseRef: 'main', + author: 'author', + }, + comment: { + id: 1, + author: 'author', + authorType: 'User', + body: '@docker-agent', + inReplyToId: null, + pullRequestUrl: 'https://api.github.com/repos/docker/docker-agent-action/pulls/1', + path: 'src/example.ts', + line: 42, + originalLine: 40, + side: 'RIGHT', + startLine: 41, + startSide: 'RIGHT', + diffHunk: '@@ -40,3 +40,5 @@', + commitId: 'a'.repeat(40), + originalCommitId: 'b'.repeat(40), + }, +}; + +const canonicalGate = "needs.resolve-context.outputs.canonical-context-available == 'true'"; + +function extractedProjection(jobName: string, stepName: string): string { + const run = step(jobName, stepName).run ?? ''; + const match = run.match(/\n\s*jq '([\s\S]*?)' "\$CANONICAL_CONTEXT" > "\$/); + if (!match) throw new Error(`Missing canonical jq projection in ${jobName}/${stepName}`); + return match[1]; +} + +function runJq(filter: string, context: CanonicalTriggerContext): unknown { + const result = spawnSync('/usr/bin/jq', [filter], { + input: JSON.stringify(context), + encoding: 'utf8', + env: testEnvironment({ + GITHUB_REPOSITORY: 'docker/docker-agent-action', + GITHUB_REPOSITORY_OWNER: 'docker', + }), + }); + if (result.error) throw result.error; + if (result.status !== 0) throw new Error(result.stderr); + return JSON.parse(result.stdout); +} + +function projectedComment(comment: CanonicalComment) { + return { + id: comment.id, + body: comment.body, + in_reply_to_id: comment.inReplyToId, + path: comment.path, + line: comment.line, + original_line: comment.originalLine, + side: comment.side, + start_line: comment.startLine, + start_side: comment.startSide, + diff_hunk: comment.diffHunk, + commit_id: comment.commitId, + original_commit_id: comment.originalCommitId, + user: { login: comment.author, type: comment.authorType }, + }; +} + +function mutateProjection(filter: string, anchor: string, source: string, kind: 'drop' | 'rename') { + const member = new RegExp(`^(\\s*)${anchor}(?:: ([^\\n]+))?,`, 'm'); + const mutated = filter.replace(member, (_matched, indent: string, value?: string) => { + if (kind === 'drop') return ''; + return `${indent}mutated_${anchor}: ${value ?? `.${source}`},`; + }); + if (mutated === filter) throw new Error(`Could not ${kind} ${anchor}`); + return mutated; +} + +const artifactGate = "needs.resolve-context.outputs.canonical-context-artifact-id != ''"; +const triggerRoute = "inputs.trigger-run-id != ''"; + +function canonicalComment(overrides: Partial = {}): CanonicalComment { + return { + id: 1, + author: 'author', + authorType: 'User', + body: '@docker-agent', + inReplyToId: null, + pullRequestUrl: 'https://api.github.com/repos/docker/docker-agent-action/pulls/1', + path: 'src/example.ts', + line: 42, + originalLine: 40, + side: 'RIGHT', + startLine: 41, + startSide: 'RIGHT', + diffHunk: '@@ -40,3 +40,5 @@', + commitId: 'a'.repeat(40), + originalCommitId: 'b'.repeat(40), + ...overrides, + }; +} + +function routeContext( + comment: CanonicalTriggerContext['comment'], +): Pick { + return { event: 'pull_request_review_comment', comment }; +} + +function productionTriggerRoute( + context: Pick, +): string { + return resolverOutputs({ + ...context, + runId: 1, + runHeadSha: 'a'.repeat(40), + headAdvanced: false, + actor: 'actor', + pullRequest: { + number: 1, + headSha: 'a'.repeat(40), + baseSha: 'b'.repeat(40), + baseRef: 'main', + author: 'author', + }, + })['trigger-route']; +} + +const routingScenarios = [ + { + name: 'threaded reply without a mention', + context: routeContext(canonicalComment({ inReplyToId: 101 })), + route: 'feedback', + }, + { + name: 'threaded reply with a mention', + context: routeContext(canonicalComment({ inReplyToId: 101, body: '@docker-agent' })), + route: 'feedback', + }, + { name: 'top-level mention', context: routeContext(canonicalComment()), route: 'mention' }, + { + name: 'review command', + context: routeContext(canonicalComment({ body: '/review @docker-agent' })), + route: 'none', + }, + { + name: 'ordinary top-level comment', + context: routeContext(canonicalComment({ body: 'hello' })), + route: 'none', + }, +] as const; + +const artifactIdExpression = + '${' + '{ needs.resolve-context.outputs.canonical-context-artifact-id }}'; +const reviewShaExpression = '${' + '{ steps.pr.outputs.head-sha }}'; +const feedbackShaExpression = '${' + '{ steps.feedback.outputs.pr-head-sha }}'; +const githubTokenExpression = '${' + '{ github.token }}'; +const uploadName = + 'trusted-trigger-context-${' + '{ github.run_id }}-${' + '{ github.run_attempt }}'; + +type Expression = + | { kind: 'literal'; value: string | boolean } + | { kind: 'path'; value: string } + | { kind: 'call'; name: 'always' | 'contains' | 'startsWith'; args: Expression[] } + | { kind: 'not'; operand: Expression } + | { kind: 'binary'; operator: '&&' | '||' | '==' | '!='; left: Expression; right: Expression }; + +type ConditionTarget = { + id: string; + expression: string; + job: string; + scope: 'job' | 'step'; + step?: string; +}; + +type AtomicPredicate = { + id: string; + expression: Expression; + category: ConditionCategory; + target: string; +}; + +type ConditionCategory = + | 'direct/workflow route selection' + | 'bot/self guards' + | 'requested-reviewer gate' + | 'resolver-result guard' + | 'canonical-availability gate' + | 'artifact-ID gate' + | 'reply-parent route split' + | 'conjunction/&&ā†’|| weakening'; + +const conditionStepInventory = [ + ['resolve-context', 'Setup credentials'], + ['resolve-context', 'Verify token for cross-run artifact download'], + ['resolve-context', 'Create trigger context directory'], + ['resolve-context', 'Download trigger context'], + ['resolve-context', 'Guard trigger context directory'], + ['resolve-context', 'Resolve trusted trigger context'], + ['resolve-context', 'Guard canonical trigger context before upload'], + ['resolve-context', 'Upload canonical trigger context'], + ['reply-to-feedback', 'Setup cross-run credentials'], + ['reply-to-feedback', 'Verify token for cross-run artifact download'], + ['reply-to-feedback', 'Validate canonical context artifact ID'], + ['reply-to-feedback', 'Download canonical trigger context'], + ['reply-to-feedback', 'Guard downloaded feedback context'], + ['reply-to-mention', 'Validate canonical context artifact ID'], + ['reply-to-mention', 'Download canonical trigger context'], + ['reply-to-mention', 'Guard downloaded mention context'], + ['reply-to-mention', 'Synthesize mention-reply event context'], +] as const; + +function productionConditionTargets(): ConditionTarget[] { + const targets: ConditionTarget[] = [ + 'resolve-context', + 'review', + 'reply-to-feedback', + 'reply-to-mention', + ].map((name) => ({ + id: `job:${name}`, + expression: requiredCondition(job(name).if, `job ${name}`), + job: name, + scope: 'job', + })); + for (const [jobName, stepName] of conditionStepInventory) { + targets.push({ + id: `step:${jobName}/${stepName}`, + expression: requiredCondition(step(jobName, stepName).if, `${jobName}/${stepName}`), + job: jobName, + scope: 'step', + step: stepName, + }); + } + return targets; +} + +function requiredCondition(value: string | undefined, name: string): string { + if (!value?.trim()) throw new Error(`Missing condition for ${name}`); + return value; +} + +function tokenize(expression: string): string[] { + const tokens = expression.match( + /\s*(\|\||&&|==|!=|!|\(|\)|,|'[^']*'|[A-Za-z_][A-Za-z0-9_.-]*)\s*/g, + ); + if (!tokens || tokens.join('').replace(/\s/g, '') !== expression.replace(/\s/g, '')) + throw new Error(`Unsupported condition syntax: ${expression}`); + return tokens.map((token) => token.trim()); +} + +function parseCondition(expression: string): Expression { + const tokens = tokenize(expression); + let index = 0; + const peek = () => tokens[index]; + const take = () => tokens[index++]; + const consume = (token: string) => { + if (peek() !== token) return false; + index++; + return true; + }; + const primary = (): Expression => { + const token = take(); + if (!token) throw new Error('Unexpected end of condition'); + if (token === '(') { + const nested = or(); + if (!consume(')')) throw new Error('Missing closing parenthesis'); + return nested; + } + if (token.startsWith("'")) return { kind: 'literal', value: token.slice(1, -1) }; + if (token === 'true' || token === 'false') return { kind: 'literal', value: token === 'true' }; + if (consume('(')) { + const args: Expression[] = []; + if (peek() !== ')') { + args.push(or()); + while (consume(',')) args.push(or()); + } + if (!consume(')')) throw new Error(`Missing closing parenthesis for ${token}`); + if (!['always', 'contains', 'startsWith'].includes(token)) + throw new Error(`Unsupported function: ${token}`); + if ((token === 'always' && args.length !== 0) || (token !== 'always' && args.length !== 2)) + throw new Error(`Invalid arguments for ${token}`); + return { kind: 'call', name: token as 'always' | 'contains' | 'startsWith', args }; + } + return { kind: 'path', value: token }; + }; + const comparison = (): Expression => { + let left = primary(); + while (peek() === '==' || peek() === '!=') { + left = { kind: 'binary', operator: take() as '==' | '!=', left, right: primary() }; + } + return left; + }; + const unary = (): Expression => (consume('!') ? { kind: 'not', operand: unary() } : comparison()); + const and = (): Expression => { + let left = unary(); + while (consume('&&')) left = { kind: 'binary', operator: '&&', left, right: unary() }; + return left; + }; + const or = (): Expression => { + let left = and(); + while (consume('||')) left = { kind: 'binary', operator: '||', left, right: and() }; + return left; + }; + const result = or(); + if (index !== tokens.length) throw new Error(`Unexpected token: ${tokens[index]}`); + return result; +} + +function evaluateExpression(expression: Expression, bindings: Record): unknown { + switch (expression.kind) { + case 'literal': + return expression.value; + case 'path': + if (!(expression.value in bindings)) throw new Error(`Missing binding: ${expression.value}`); + return bindings[expression.value]; + case 'call': { + const args = expression.args.map((argument) => evaluateExpression(argument, bindings)); + if (expression.name === 'always') return true; + if (expression.name === 'contains') + return String(args[0] ?? '').includes(String(args[1] ?? '')); + return String(args[0] ?? '').startsWith(String(args[1] ?? '')); + } + case 'not': + return !evaluateExpression(expression.operand, bindings); + case 'binary': { + const left = evaluateExpression(expression.left, bindings); + if (expression.operator === '&&') + return Boolean(left) && Boolean(evaluateExpression(expression.right, bindings)); + if (expression.operator === '||') + return Boolean(left) || Boolean(evaluateExpression(expression.right, bindings)); + const right = evaluateExpression(expression.right, bindings); + return expression.operator === '==' ? left === right : left !== right; + } + } +} + +function evaluateCondition(expression: string, bindings: Record): boolean { + return Boolean(evaluateExpression(parseCondition(expression), bindings)); +} + +function renderExpression(expression: Expression): string { + switch (expression.kind) { + case 'literal': + return typeof expression.value === 'string' + ? `'${expression.value}'` + : String(expression.value); + case 'path': + return expression.value; + case 'call': + return `${expression.name}(${expression.args.map(renderExpression).join(', ')})`; + case 'not': + return `!${renderExpression(expression.operand)}`; + case 'binary': + return `(${renderExpression(expression.left)} ${expression.operator} ${renderExpression(expression.right)})`; + } +} + +function categoryFor(expression: Expression): ConditionCategory { + const rendered = renderExpression(expression); + if (expression.kind === 'binary' && expression.operator === '&&') + return 'conjunction/&&ā†’|| weakening'; + if (rendered.includes('requested_reviewer')) return 'requested-reviewer gate'; + if (rendered.includes('needs.resolve-context.result')) return 'resolver-result guard'; + if (rendered.includes('canonical-context-available')) return 'canonical-availability gate'; + if (rendered.includes('canonical-context-artifact-id') || rendered.includes('canonical-artifact')) + return 'artifact-ID gate'; + if (rendered.includes('comment-in-reply-to-id') || rendered.includes('comment.in_reply_to_id')) + return 'reply-parent route split'; + if ( + rendered.includes('comment-author') || + rendered.includes('comment.user.') || + rendered.includes('sender.') + ) + return 'bot/self guards'; + return 'direct/workflow route selection'; +} + +function atomicPredicates( + expression: Expression, + target: string, + path = 'root', +): AtomicPredicate[] { + if (expression.kind === 'binary' && expression.operator === '&&') + return [ + ...(path === 'root' + ? [{ id: `${target}:${path}`, expression, category: categoryFor(expression), target }] + : []), + ...atomicPredicates(expression.left, target, `${path}.left`), + ...atomicPredicates(expression.right, target, `${path}.right`), + ]; + if (expression.kind === 'binary' && expression.operator === '||') + return [ + ...atomicPredicates(expression.left, target, `${path}.left`), + ...atomicPredicates(expression.right, target, `${path}.right`), + ]; + return [{ id: `${target}:${path}`, expression, category: categoryFor(expression), target }]; +} + +function replaceExpression( + expression: Expression, + id: string, + replacement: Expression, + target: string, + path = 'root', +): Expression { + if (`${target}:${path}` === id) return replacement; + if (expression.kind === 'not') + return { + ...expression, + operand: replaceExpression(expression.operand, id, replacement, target, `${path}.operand`), + }; + if (expression.kind === 'binary') + return { + ...expression, + left: replaceExpression(expression.left, id, replacement, target, `${path}.left`), + right: replaceExpression(expression.right, id, replacement, target, `${path}.right`), + }; + if (expression.kind === 'call') + return { + ...expression, + args: expression.args.map((argument, index) => + replaceExpression(argument, id, replacement, target, `${path}.arg${index}`), + ), + }; + return expression; +} + +function mutateAtom(atom: Expression): Expression { + if (atom.kind === 'binary' && atom.operator === '&&') return { ...atom, operator: '||' }; + if (atom.kind === 'binary' && atom.operator === '==') return { ...atom, operator: '!=' }; + if (atom.kind === 'binary' && atom.operator === '!=') return { ...atom, operator: '==' }; + if (atom.kind === 'not') return atom.operand; + return { kind: 'not', operand: atom }; +} + +function canonicalBindings( + route: string, + overrides: Record = {}, +): Record { + return { + 'github.event_name': 'workflow_run', + 'github.event.action': '', + 'github.event.issue.pull_request': false, + 'github.event.comment.in_reply_to_id': false, + 'github.event.comment.user.login': '', + 'github.event.comment.user.type': '', + 'github.event.comment.body': '', + 'github.event.sender.type': '', + 'github.event.sender.login': '', + 'github.event.requested_reviewer.login': '', + 'inputs.pr-number': '', + 'inputs.trigger-run-id': '123', + 'needs.resolve-context.result': 'success', + 'needs.resolve-context.outputs.canonical-context-available': 'true', + 'needs.resolve-context.outputs.canonical-context-artifact-id': '456', + 'needs.resolve-context.outputs.trigger-event': 'pull_request_review_comment', + 'needs.resolve-context.outputs.trigger-route': route, + 'needs.resolve-context.outputs.comment-in-reply-to-id': route === 'feedback' ? '789' : '', + 'needs.resolve-context.outputs.comment-author': 'human', + 'needs.resolve-context.outputs.comment-author-type': 'User', + ...overrides, + }; +} + +function requiredComment(context: CanonicalTriggerContext): CommentContext { + if (!context.comment) throw new Error('Scenario must include a comment'); + return context.comment; +} + +function actionStepRun(name: string): string { + const action = parseDocument( + readFileSync(resolve(root, 'review-pr/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 summaryRun(): string { + return actionStepRun('Post clean summary'); +} + +function runCopyReference(headSha: string, template: string): ReturnType { + const directory = mkdtempSync(resolve(tmpdir(), 'docker-agent-copy-reference-')); + const actionPath = resolve(directory, 'action'); + const refs = resolve(actionPath, 'agents/refs'); + const output = resolve(directory, 'output'); + const originalRefs = '/tmp/refs'; + const backupRefs = resolve(directory, 'refs-backup'); + try { + if (existsSync(originalRefs)) + cpSync(originalRefs, backupRefs, { recursive: true, dereference: false }); + rmSync(originalRefs, { recursive: true, force: true }); + writeFileSync(resolve(directory, 'run.sh'), actionStepRun('Copy reference files')); + writeFileSync(resolve(directory, 'posting-format.md'), template); + cpSync(resolve(root, 'review-pr/agents/refs'), refs, { recursive: true }); + writeFileSync(resolve(refs, 'posting-format.md'), template); + const result = spawnSync( + '/bin/bash', + ['--noprofile', '--norc', '-e', '-o', 'pipefail', resolve(directory, 'run.sh')], + { + env: testEnvironment({ + ACTION_PATH: actionPath, + PR_HEAD_SHA: headSha, + GITHUB_OUTPUT: output, + }), + encoding: 'utf8', + }, + ); + if (result.status === 0) + expect(readFileSync(output, 'utf8')).toContain( + 'posting-reference=/tmp/refs/posting-format.md', + ); + return result; + } finally { + rmSync(originalRefs, { recursive: true, force: true }); + if (existsSync(backupRefs)) + cpSync(backupRefs, originalRefs, { recursive: true, dereference: false }); + rmSync(directory, { recursive: true, force: true }); + } +} + +type SummaryInvocation = { + skipReason?: string; + exitCode?: string; + verboseLog?: string; + chunkCount?: string; + headSha?: string; + postingReference?: string; + dedupCounts?: [number, number]; +}; + +type GhRecord = { args: string; input: string }; + +function runSummary(invocation: SummaryInvocation): { + result: ReturnType; + records: GhRecord[]; + summary: string; + directory: string; +} { + const directory = mkdtempSync(resolve(tmpdir(), 'docker-agent-summary-')); + const output = resolve(directory, 'output'); + const summary = resolve(directory, 'summary'); + const recordsPath = resolve(directory, 'gh-records.jsonl'); + writeFileSync(recordsPath, ''); + writeFileSync(output, ''); + writeFileSync(summary, ''); + const reference = invocation.postingReference ?? resolve(directory, 'posting-format.md'); + const sha = invocation.headSha ?? 'a'.repeat(40); + writeFileSync(resolve(directory, 'summary.sh'), summaryRun()); + if (!invocation.postingReference) { + writeFileSync(reference, `jq -n --arg commit_id "${sha}" '{commit_id: $commit_id}'`); + } + if (invocation.verboseLog !== undefined) + writeFileSync(resolve(directory, 'verbose.log'), invocation.verboseLog); + writeFileSync( + resolve(directory, 'gh'), + `#!/usr/bin/env bash +set -euo pipefail +args="$*" +input="" +if [[ "$args" == *" --input -" ]]; then input=$(cat); fi +printf '%s\\n' "$(jq -cn --arg args "$args" --arg input "$input" '{args: $args, input: $input}')" >> "$GH_RECORDS" +if [[ "$args" == *"/reviews --jq "* ]]; then + if [[ "$args" == *"/issues/"* ]]; then printf '%s\\n' "${invocation.dedupCounts?.[1] ?? 0}"; else printf '%s\\n' "${invocation.dedupCounts?.[0] ?? 0}"; fi +fi +`, + ); + chmodSync(resolve(directory, 'gh'), 0o755); + const result = spawnSync( + '/bin/bash', + [ + '--noprofile', + '--norc', + '-e', + '-o', + 'pipefail', + '-c', + 'source "$1"', + 'bash', + resolve(directory, 'summary.sh'), + ], + { + cwd: directory, + env: testEnvironment({ + PATH: `${directory}${delimiter}${safePath}`, + GH_RECORDS: recordsPath, + GITHUB_OUTPUT: output, + GITHUB_STEP_SUMMARY: summary, + REPOSITORY: 'docker/docker-agent-action', + PR_NUMBER: '88', + RUN_URL: 'https://example.test/run', + SKIP_REASON: invocation.skipReason ?? '', + EXIT_CODE: invocation.exitCode ?? '', + VERBOSE_LOG_FILE: + invocation.verboseLog === undefined ? '' : resolve(directory, 'verbose.log'), + CHUNK_COUNT: invocation.chunkCount ?? '', + LOCK_AGE: '', + ACTION_PATH: directory, + PR_HEAD_SHA: sha, + POSTING_REFERENCE: reference, + }), + encoding: 'utf8', + }, + ); + const records = readFileSync(recordsPath, 'utf8', { flag: 'a+' }) + .trim() + .split('\n') + .filter(Boolean) + .map((line) => JSON.parse(line) as GhRecord); + return { result, records, summary: readFileSync(summary, 'utf8'), directory }; +} + +function reviewCreations(records: GhRecord[]): GhRecord[] { + return records.filter( + (record) => record.args === 'api repos/docker/docker-agent-action/pulls/88/reviews --input -', + ); +} + +function expectReviewPayload(record: GhRecord, sha: string, bodyPrefix: string): void { + expect(JSON.parse(record.input)).toEqual({ + body: expect.stringMatching(new RegExp(`^${bodyPrefix}`)), + event: 'COMMENT', + commit_id: sha, + comments: [], + }); +} + +describe('fork workflow security regressions', () => { + it('connects every resolver producer, job output, and downstream consumer', () => { + const resolveContext = job('resolve-context'); + const resolverStep = step('resolve-context', 'Resolve trusted trigger context'); + const producers = resolverOutputNames(); + expect(resolverStep.run).toContain('canonical-context-available'); + producers.add('canonical-context-available'); + + for (const [name, expression] of Object.entries(resolveContext.outputs ?? {})) { + const match = expression.match(/^\${{\s*steps\.read\.outputs\.([\w-]+)\s*}}$/); + if (match) expect(producers, `${name} must be emitted by the resolver`).toContain(match[1]); + } + + const exposedOutputs = new Set(Object.keys(resolveContext.outputs ?? {})); + const downstreamReferences = new Set(); + for (const [name, candidate] of Object.entries(workflow.jobs)) { + if (name === 'resolve-context') continue; + for (const value of values(candidate)) { + for (const match of value.matchAll(/needs\.resolve-context\.outputs\.([\w-]+)/g)) { + downstreamReferences.add(match[1]); + } + } + } + for (const output of downstreamReferences) { + expect(exposedOutputs, `${output} must be exposed by resolve-context`).toContain(output); + } + }); + + it.each(routingScenarios)('routes $name through the production trigger policy', ({ + context, + route, + }) => { + const actualRoute = productionTriggerRoute(context); + const routeToJob = { feedback: 'reply-to-feedback', mention: 'reply-to-mention' } as const; + + for (const jobName of ['review', 'reply-to-feedback', 'reply-to-mention'] as const) { + const condition = job(jobName).if ?? ''; + const enabled = evaluateCondition(condition, canonicalBindings(actualRoute)); + expect(enabled, `${jobName} for ${actualRoute}`).toBe( + actualRoute !== 'none' && routeToJob[actualRoute as keyof typeof routeToJob] === jobName, + ); + } + expect(actualRoute).toBe(route); + }); + + it('rejects unsupported syntax and kills a real routing mutation', () => { + expect(() => evaluateCondition('unknown()', {})).toThrow('Unsupported function'); + expect(() => + evaluateCondition('github.event_name ==', { 'github.event_name': 'workflow_run' }), + ).toThrow(); + + const review = job('review').if ?? ''; + const target = "needs.resolve-context.outputs.trigger-route == 'review'"; + expect(review).toContain(target); + const mutated = review.replace( + target, + "needs.resolve-context.outputs.trigger-route != 'review'", + ); + const bindings = canonicalBindings('review', { + 'needs.resolve-context.outputs.trigger-event': 'pull_request', + }); + expect(evaluateCondition(review, bindings)).toBe(true); + expect(evaluateCondition(mutated, bindings)).toBe(false); + }); + + it('parses production condition ASTs and kills every categorized mutation with real route scenarios', () => { + const targets = productionConditionTargets(); + expect(targets.map((target) => target.id)).toEqual([ + 'job:resolve-context', + 'job:review', + 'job:reply-to-feedback', + 'job:reply-to-mention', + ...conditionStepInventory.map(([jobName, stepName]) => `step:${jobName}/${stepName}`), + ]); + + const parsed = targets.map((target) => ({ ...target, ast: parseCondition(target.expression) })); + const atoms = parsed.flatMap((target) => atomicPredicates(target.ast, target.id)); + expect(new Set(atoms.map((atom) => atom.id)).size).toBe(atoms.length); + const categories: ConditionCategory[] = [ + 'direct/workflow route selection', + 'bot/self guards', + 'requested-reviewer gate', + 'resolver-result guard', + 'canonical-availability gate', + 'artifact-ID gate', + 'reply-parent route split', + 'conjunction/&&ā†’|| weakening', + ]; + for (const category of categories) + expect( + atoms.filter((atom) => atom.category === category), + `${category} has no production atoms`, + ).not.toHaveLength(0); + + const contexts = { + review: { ...canonicalContext, event: 'pull_request', comment: undefined }, + feedback: { + ...canonicalContext, + comment: canonicalComment({ inReplyToId: 7, body: 'reply' }), + }, + mention: { ...canonicalContext, comment: canonicalComment({ body: '@docker-agent' }) }, + none: { ...canonicalContext, comment: canonicalComment({ body: 'ordinary' }) }, + bot: { + ...canonicalContext, + comment: canonicalComment({ author: 'docker-agent', body: '@docker-agent' }), + }, + } as const; + + const outputMappings = job('resolve-context').outputs ?? {}; + const outputBindings = ( + context: CanonicalTriggerContext, + available = true, + artifactId = '456', + ) => { + const outputs = resolverOutputs(context); + const bindings: Record = {}; + const artifactOutput = '${' + '{ steps.canonical-context.outputs.artifact-id }}'; + for (const [name, mapping] of Object.entries(outputMappings)) { + const resolver = mapping.match(/^\${{\s*steps\.read\.outputs\.([\w-]+)\s*}}$/); + if (resolver) { + if (resolver[1] === 'canonical-context-available') { + bindings[`needs.resolve-context.outputs.${name}`] = available ? 'true' : 'false'; + continue; + } + if (!(resolver[1] in outputs)) throw new Error(`No resolver producer for ${name}`); + bindings[`needs.resolve-context.outputs.${name}`] = outputs[resolver[1]]; + continue; + } + if (mapping === artifactOutput) { + bindings[`needs.resolve-context.outputs.${name}`] = artifactId; + continue; + } + throw new Error(`Unsupported resolve-context output producer: ${name}=${mapping}`); + } + bindings['needs.resolve-context.outputs.canonical-context-available'] = available + ? 'true' + : 'false'; + return bindings; + }; + + const directBindings = (overrides: Record = {}): Record => ({ + 'github.event_name': 'issue_comment', + 'github.event.action': 'created', + 'github.event.issue.pull_request': true, + 'github.event.comment.in_reply_to_id': '', + 'github.event.comment.user.login': 'human', + 'github.event.comment.user.type': 'User', + 'github.event.comment.body': 'ordinary', + 'github.event.sender.type': 'User', + 'github.event.sender.login': 'human', + 'github.event.requested_reviewer.login': '', + 'inputs.pr-number': '', + 'inputs.trigger-run-id': '', + 'needs.resolve-context.result': 'skipped', + 'needs.resolve-context.outputs.canonical-context-available': 'false', + 'needs.resolve-context.outputs.canonical-context-artifact-id': '', + 'needs.resolve-context.outputs.trigger-event': '', + 'needs.resolve-context.outputs.trigger-route': '', + 'needs.resolve-context.outputs.comment-in-reply-to-id': '', + 'needs.resolve-context.outputs.comment-author': '', + 'needs.resolve-context.outputs.comment-author-type': '', + 'steps.context-exists.outputs.exists': 'true', + 'steps.read.outputs.canonical-context-available': 'true', + 'steps.canonical-artifact.outputs.valid': 'true', + ...overrides, + }); + const workflowBindings = ( + context: CanonicalTriggerContext, + overrides: Record = {}, + ) => + directBindings({ + 'github.event_name': 'workflow_run', + 'github.event.action': '', + 'github.event.issue.pull_request': false, + 'inputs.trigger-run-id': '123', + 'needs.resolve-context.result': 'success', + ...outputBindings(context), + ...overrides, + }); + + const scenarios = [ + { + name: 'direct issue protected marker denied', + bindings: directBindings({ 'github.event.comment.body': '' }), + routes: [], + }, + { + name: 'direct issue mention', + bindings: directBindings({ 'github.event.comment.body': '@docker-agent' }), + routes: ['review', 'reply-to-mention'], + }, + { + name: 'direct issue command', + bindings: directBindings({ 'github.event.comment.body': '/review' }), + routes: ['review'], + }, + { name: 'direct issue ordinary', bindings: directBindings(), routes: ['review'] }, + { + name: 'direct pull request automatic', + bindings: directBindings({ + 'github.event_name': 'pull_request', + 'github.event.action': 'synchronize', + }), + routes: ['review'], + }, + { + name: 'direct pull request unsupported action', + bindings: directBindings({ + 'github.event_name': 'pull_request', + 'github.event.action': 'closed', + }), + routes: ['review'], + }, + { + name: 'direct requested reviewer', + bindings: directBindings({ + 'github.event_name': 'pull_request', + 'github.event.action': 'review_requested', + 'github.event.requested_reviewer.login': 'docker-agent', + }), + routes: ['review'], + }, + { + name: 'direct requested reviewer mismatch', + bindings: directBindings({ + 'github.event_name': 'pull_request', + 'github.event.action': 'review_requested', + 'github.event.requested_reviewer.login': 'other', + }), + routes: [], + }, + { + name: 'direct feedback reply', + bindings: directBindings({ + 'github.event_name': 'pull_request_review_comment', + 'github.event.comment.in_reply_to_id': 7, + }), + routes: ['reply-to-feedback'], + }, + { + name: 'direct inline mention', + bindings: directBindings({ + 'github.event_name': 'pull_request_review_comment', + 'github.event.comment.body': '@docker-agent', + }), + routes: ['reply-to-mention'], + }, + { + name: 'direct bot denied', + bindings: directBindings({ + 'github.event.comment.body': '@docker-agent', + 'github.event.comment.user.login': 'docker-agent', + 'github.event.sender.login': 'docker-agent', + }), + routes: [], + }, + { name: 'workflow review', bindings: workflowBindings(contexts.review), routes: ['review'] }, + { + name: 'workflow feedback', + bindings: workflowBindings(contexts.feedback), + routes: ['reply-to-feedback'], + }, + { + name: 'workflow mention', + bindings: workflowBindings(contexts.mention), + routes: ['reply-to-mention'], + }, + { name: 'workflow no route', bindings: workflowBindings(contexts.none), routes: [] }, + { + name: 'workflow unavailable', + bindings: workflowBindings(contexts.mention, { + 'needs.resolve-context.outputs.canonical-context-available': 'false', + }), + routes: [], + }, + { + name: 'workflow no artifact', + bindings: workflowBindings(contexts.mention, { + 'needs.resolve-context.outputs.canonical-context-artifact-id': '', + }), + routes: [], + }, + { + name: 'workflow resolver failure', + bindings: workflowBindings(contexts.mention, { 'needs.resolve-context.result': 'failure' }), + routes: [], + }, + { + name: 'workflow resolver cancelled', + bindings: workflowBindings(contexts.mention, { + 'needs.resolve-context.result': 'cancelled', + 'needs.resolve-context.outputs.canonical-context-available': 'false', + }), + routes: [], + }, + { name: 'workflow bot denied', bindings: workflowBindings(contexts.bot), routes: [] }, + ]; + + const downstreamConditions = new Map( + parsed + .filter((target) => target.scope === 'job' && target.id !== 'job:resolve-context') + .map((target) => [target.job, target.ast]), + ); + for (const scenario of scenarios) { + expect( + Boolean(evaluateExpression(parsed[0].ast, scenario.bindings)), + `${scenario.name}: resolver entry`, + ).toBe(scenario.bindings['inputs.trigger-run-id'] !== ''); + const enabled = ['review', 'reply-to-feedback', 'reply-to-mention'].filter((name) => { + const condition = downstreamConditions.get(name); + if (!condition) throw new Error(`Missing job condition: ${name}`); + return Boolean(evaluateExpression(condition, scenario.bindings)); + }); + expect(enabled, scenario.name).toEqual(scenario.routes); + } + + let killed = 0; + for (const target of parsed) { + for (const atom of atomicPredicates(target.ast, target.id)) { + const mutant = replaceExpression( + target.ast, + atom.id, + mutateAtom(atom.expression), + target.id, + ); + expect(renderExpression(mutant), `${atom.category}/${atom.id} was a no-op`).not.toBe( + renderExpression(target.ast), + ); + const killer = scenarios.find( + (scenario) => + Boolean(evaluateExpression(target.ast, scenario.bindings)) !== + Boolean(evaluateExpression(mutant, scenario.bindings)), + ); + expect( + killer, + `${atom.category}/${atom.id}/${renderExpression(atom.expression)}`, + ).toBeDefined(); + killed++; + } + } + expect(killed).toBe(atoms.length); + }); + + it('executes canonical comment projections and kills every anchor mutation', () => { + const anchors = [ + ['path', 'path'], + ['line', 'line'], + ['original_line', 'originalLine'], + ['side', 'side'], + ['start_line', 'startLine'], + ['start_side', 'startSide'], + ['diff_hunk', 'diffHunk'], + ['commit_id', 'commitId'], + ['original_commit_id', 'originalCommitId'], + ] as const; + const contexts = [ + { ...canonicalContext, comment: { ...canonicalContext.comment, inReplyToId: 1 } }, + { + ...canonicalContext, + comment: { + ...canonicalContext.comment, + inReplyToId: 1, + path: null, + line: null, + originalLine: null, + side: null, + startLine: null, + startSide: null, + diffHunk: null, + commitId: null, + originalCommitId: null, + }, + }, + ]; + const projections = [ + ['feedback', extractedProjection('reply-to-feedback', 'Parse comment context')], + [ + 'mention', + extractedProjection('reply-to-mention', 'Synthesize mention-reply event context'), + ], + ] as const; + let kills = 0; + + for (const [projectionName, projection] of projections) { + for (const context of contexts) { + const expectedComment = projectedComment(requiredComment(context)); + const expected = + projectionName === 'feedback' + ? expectedComment + : expect.objectContaining({ comment: expectedComment }); + expect(runJq(projection, context)).toEqual(expected); + } + for (const [anchor, source] of anchors) { + for (const kind of ['drop', 'rename'] as const) { + const mutant = mutateProjection(projection, anchor, source, kind); + for (const context of contexts) { + const actual = runJq(mutant, context); + const expectedComment = projectedComment(requiredComment(context)); + const expected = + projectionName === 'feedback' + ? expectedComment + : expect.objectContaining({ comment: expectedComment }); + expect(actual).not.toEqual(expected); + kills++; + } + } + } + } + expect(kills).toBe(72); + }); + + it.each([ + { + name: 'normal agent-posted success', + exitCode: '0', + verboseLog: 'pullrequestreview-1', + reads: 0, + body: undefined, + }, + { + name: 'zero-findings already posted', + exitCode: '0', + verboseLog: 'no review', + dedupCounts: [1, 0], + reads: 2, + body: undefined, + }, + { + name: 'timeout with unknown chunks', + exitCode: '124', + verboseLog: 'no review', + chunkCount: '', + reads: 0, + body: 'ā±ļø', + }, + { + name: 'timeout with one chunk', + exitCode: '124', + verboseLog: 'no review', + chunkCount: '1', + reads: 0, + body: 'ā±ļø', + }, + { + name: 'timeout with many chunks', + exitCode: '124', + verboseLog: 'no review', + chunkCount: '2', + reads: 0, + body: 'ā±ļø', + }, + { name: 'non-124 failure', exitCode: '1', verboseLog: 'no review', reads: 0, body: 'āŒ' }, + { + name: 'failure with prior review', + exitCode: '1', + verboseLog: 'pullrequestreview-1', + reads: 0, + body: undefined, + }, + { + name: 'fallback LGTM', + exitCode: '0', + verboseLog: 'no review', + dedupCounts: [0, 0], + reads: 2, + body: 'šŸŸ¢', + }, + { name: 'success without log', exitCode: '0', reads: 0, body: undefined }, + ])('executes the summary $name vector with exact review payload behavior', (vector) => { + const sha = 'a'.repeat(40); + const run = runSummary(vector); + try { + expect(run.result.status, run.result.stderr).toBe(0); + const creations = reviewCreations(run.records); + expect(run.records.filter((record) => record.args.includes(' --jq '))).toHaveLength( + vector.reads, + ); + expect(creations).toHaveLength(vector.body ? 1 : 0); + if (vector.body) expectReviewPayload(creations[0], sha, vector.body); + } finally { + rmSync(run.directory, { recursive: true, force: true }); + } + }); + + it('does not inherit a poisoned PATH when executing the summary harness', () => { + const poison = mkdtempSync(resolve(tmpdir(), 'docker-agent-poisoned-path-')); + const previousPath = process.env.PATH; + try { + writeFileSync(resolve(poison, 'jq'), '#!/bin/sh\necho poisoned >&2\nexit 97\n'); + chmodSync(resolve(poison, 'jq'), 0o755); + process.env.PATH = poison; + + const run = runSummary({ exitCode: '0', verboseLog: 'no review' }); + try { + expect(run.result.status, run.result.stderr).toBe(0); + expect(run.result.stderr).not.toContain('poisoned'); + } finally { + rmSync(run.directory, { recursive: true, force: true }); + } + } finally { + process.env.PATH = previousPath; + rmSync(poison, { recursive: true, force: true }); + } + }); + + it.each([ + ['', 'Selected PR head SHA is invalid'], + ['g'.repeat(40), 'Selected PR head SHA is invalid'], + ['a'.repeat(39), 'Selected PR head SHA is invalid'], + ['a'.repeat(41), 'Selected PR head SHA is invalid'], + ])('executes malformed SHA preflight %s without API access', (headSha, diagnostic) => { + const run = runSummary({ exitCode: '124', verboseLog: 'no review', headSha }); + try { + expect(run.result.status).not.toBe(0); + expect(run.result.stderr).toContain(diagnostic); + expect(run.records).toEqual([]); + } finally { + rmSync(run.directory, { recursive: true, force: true }); + } + }); + + it.each([ + ['missing reference', undefined], + ['retained template marker', 'jq -n --arg commit_id "__PR_HEAD_SHA__"'], + ['retained shell marker', 'jq -n --arg commit_id "$PR_HEAD_SHA"'], + ['zero commit argument', 'jq -n'], + [ + 'multiple commit arguments', + 'jq -n --arg commit_id "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" --arg commit_id "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"', + ], + [ + 'selected/rendered SHA mismatch', + 'jq -n --arg commit_id "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"', + ], + ])('executes malformed posting reference %s without API access', (_name, content) => { + const directory = mkdtempSync(resolve(tmpdir(), 'docker-agent-reference-')); + const reference = resolve(directory, 'posting-format.md'); + if (content) writeFileSync(reference, content); + const run = runSummary({ + exitCode: '124', + verboseLog: 'no review', + postingReference: reference, + }); + try { + expect(run.result.status).not.toBe(0); + expect(run.result.stderr).toContain( + 'Rendered posting reference does not contain exactly one', + ); + expect(run.records).toEqual([]); + } finally { + rmSync(run.directory, { recursive: true, force: true }); + rmSync(directory, { recursive: true, force: true }); + } + }); + + it.each([ + { skipReason: 'concurrent' }, + { exitCode: '' }, + ])('executes benign skip states without requiring preflight inputs', (vector) => { + const directory = mkdtempSync(resolve(tmpdir(), 'docker-agent-skip-')); + const run = runSummary({ + ...vector, + headSha: '', + postingReference: resolve(directory, 'missing'), + }); + try { + expect(run.result.status, run.result.stderr).toBe(0); + expect(run.summary).toContain('Review skipped'); + expect(run.records).toEqual([]); + } finally { + rmSync(run.directory, { recursive: true, force: true }); + rmSync(directory, { recursive: true, force: true }); + } + }); + + 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"'], + [ + 'multiple commit arguments', + 'a'.repeat(40), + 'jq -n --arg commit_id "__PR_HEAD_SHA__" --arg commit_id "x"', + ], + ])('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, + ); + }); + + 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( + action.indexOf('- name: Prepare immutable PR snapshot'), + action.indexOf('- name: Compute incremental review range'), + ); + const summary = action.slice(action.indexOf('- name: Post clean summary')); + expect(snapshot).toContain(`PR_HEAD_SHA: ${'${'}{ inputs.pr-head-sha }}`); + expect(snapshot).toContain(`PR_BASE_SHA: ${'${'}{ inputs.pr-base-sha }}`); + expect(snapshot).not.toContain('steps.pr-info.outputs.head-sha'); + expect(snapshot).not.toContain('POSTING_REFERENCE'); + expect(summary).toContain(`PR_HEAD_SHA: ${'${'}{ steps.pr-info.outputs.head-sha }}`); + }); + + it('keeps resolver output names body-free and shell expressions out of run bodies', () => { + const outputs = resolverOutputNames(); + expect(outputs).toContain('comment-in-reply-to-id'); + expect(outputs).not.toContain('comment-body'); + expect(JSON.stringify(workflow)).not.toContain('steps.read.outputs.comment-body'); + expect(JSON.stringify(workflow)).not.toContain('needs.resolve-context.outputs.comment-body'); + + for (const jobName of ['resolve-context', 'review', 'reply-to-feedback', 'reply-to-mention']) { + for (const candidate of job(jobName).steps ?? []) { + if (!candidate.run) continue; + expect( + candidate.run, + `${jobName}/${candidate.name ?? 'unnamed'} must use env for expressions`, + ).not.toContain('${{'); + } + } + }); + + it('does not run privileged E2E jobs from workflow_run', () => { + expect(e2e.on?.workflow_run).toBeUndefined(); + expect(JSON.stringify(e2e)).not.toContain('test-e2e-trigger.yml'); + }); + + it('gates every workflow-run route on the canonical resolver result', () => { + for (const route of ['review', 'reply-to-feedback', 'reply-to-mention']) { + expect(job(route).if).toContain(canonicalGate); + } + expect(job('review').if).toContain("trigger-route == 'review'"); + const resolvePr = step('review', 'Resolve PR number'); + expect(resolvePr.run).toContain( + `PR_SNAPSHOT=$(gh api "repos/\${GITHUB_REPOSITORY}/pulls/$PR_NUMBER")`, + ); + expect(resolvePr.run).toContain( + 'PR_HEAD_SHA=$(jq -r \'.head.sha // empty\' <<<"$PR_SNAPSHOT")', + ); + expect(resolvePr.run).toContain( + 'PR_BASE_SHA=$(jq -r \'.base.sha // empty\' <<<"$PR_SNAPSHOT")', + ); + expect(resolvePr.run).not.toContain('PR_BASE_SHA=${PR_BASE_SHA:-$(gh api'); + expect(job('reply-to-feedback').if).toContain("trigger-route == 'feedback'"); + expect(job('reply-to-mention').if).toContain("trigger-route == 'mention'"); + expect(job('reply-to-feedback').if).toContain(artifactGate); + expect(job('reply-to-mention').if).toContain(artifactGate); + }); + + it('stages, guards, and transfers canonical context through private runner-temp directories', () => { + const resolver = step('resolve-context', 'Resolve trusted trigger context'); + const locator = step('resolve-context', 'Create trigger context directory'); + const locatorGuard = step('resolve-context', 'Guard trigger context directory'); + const uploadGuard = step('resolve-context', 'Guard canonical trigger context before upload'); + const upload = step('resolve-context', 'Upload canonical trigger context'); + expect(locator.env?.RUNNER_TEMP).toBe('${' + '{ runner.temp }}'); + expect(locator.run).toContain('RUN_ATTEMPT'); + expect(stepIndex('resolve-context', locator.name ?? '')).toBeLessThan( + stepIndex('resolve-context', 'Download trigger context'), + ); + expect(step('resolve-context', 'Download trigger context').with?.path).toBe( + '${' + '{ steps.trigger-context-directory.outputs.path }}', + ); + expect(resolver.env?.TRIGGER_ARTIFACT_DIRECTORY).toBe( + '${' + '{ steps.trigger-context-directory.outputs.path }}', + ); + expect(locatorGuard.run).toContain('realpath'); + expect(locatorGuard.run).toContain("stat -c '%a'"); + expect(stepIndex('resolve-context', locatorGuard.name ?? '')).toBeLessThan( + stepIndex('resolve-context', resolver.name ?? ''), + ); + expect(uploadGuard.run).toContain('canonical-trigger-context.json'); + expect(uploadGuard.run).toContain('[ -f'); + expect(uploadGuard.run).toContain('[ ! -L'); + expect(uploadGuard.run).toContain('realpath'); + expect(uploadGuard.run).toContain("stat -c '%a'"); + expect(stepIndex('resolve-context', resolver.name ?? '')).toBeLessThan( + stepIndex('resolve-context', uploadGuard.name ?? ''), + ); + expect(stepIndex('resolve-context', uploadGuard.name ?? '')).toBeLessThan( + stepIndex('resolve-context', upload.name ?? ''), + ); + expect(upload.with?.path).toBe('${' + '{ steps.read.outputs.canonical-context-path }}'); + expect(JSON.stringify(workflow)).not.toContain('/tmp/context'); + + for (const [route, directory] of [ + ['reply-to-feedback', 'feedback-context-directory'], + ['reply-to-mention', 'mention-context-directory'], + ] as const) { + const createDirectory = step( + route, + route === 'reply-to-feedback' + ? 'Create feedback context directory' + : 'Create mention context directory', + ); + const download = step(route, 'Download canonical trigger context'); + const guard = step( + route, + route === 'reply-to-feedback' + ? 'Guard downloaded feedback context' + : 'Guard downloaded mention context', + ); + expect(createDirectory.if).toBeUndefined(); + expect(createDirectory.env?.RUNNER_TEMP).toBe('${' + '{ runner.temp }}'); + expect(createDirectory.run).toContain('RUN_ATTEMPT'); + expect(createDirectory.run).toContain('mktemp'); + expect(createDirectory.run).toContain('chmod 600'); + expect(download.with?.path).toBe(`\${{ steps.${directory}.outputs.canonical }}`); + expect(guard.run).toContain('realpath'); + expect(guard.run).toContain('[ ! -L'); + expect(guard.run).toContain("stat -c '%a'"); + expect(stepIndex(route, createDirectory.name ?? '')).toBeLessThan( + stepIndex(route, download.name ?? ''), + ); + expect(stepIndex(route, download.name ?? '')).toBeLessThan( + stepIndex(route, guard.name ?? ''), + ); + } + + expect(step('reply-to-feedback', 'Parse comment context').env?.CANONICAL_CONTEXT).toBe( + '${' + + '{ steps.feedback-context-directory.outputs.canonical }}/canonical-trigger-context.json', + ); + expect( + step('reply-to-mention', 'Synthesize mention-reply event context').env?.CANONICAL_CONTEXT, + ).toBe( + '${' + + '{ steps.mention-context-directory.outputs.canonical }}/canonical-trigger-context.json', + ); + }); + + it('selects the canonical artifact by ID, validates it, and only then uses it', () => { + for (const route of ['reply-to-feedback', 'reply-to-mention']) { + const validate = step(route, 'Validate canonical context artifact ID'); + const download = step(route, 'Download canonical trigger context'); + expect(validate.if).toContain(triggerRoute); + expect(validate.if).toContain(canonicalGate); + expect(validate.if).toContain(artifactGate); + expect(download.if).toContain(triggerRoute); + expect(download.if).toContain("steps.canonical-artifact.outputs.valid == 'true'"); + expect(download.with?.['artifact-ids']).toBe(artifactIdExpression); + expect(download.with?.name).toBeUndefined(); + expect(download.with?.['digest-mismatch']).toBe('error'); + expect(stepIndex(route, validate.name ?? '')).toBeLessThan( + stepIndex(route, download.name ?? ''), + ); + } + const feedbackParse = step('reply-to-feedback', 'Parse comment context'); + const mentionSynthesis = step('reply-to-mention', 'Synthesize mention-reply event context'); + expect(feedbackParse.run).toContain('CANONICAL_CONTEXT'); + expect(mentionSynthesis.if).toContain(triggerRoute); + expect(mentionSynthesis.run).toContain('CANONICAL_CONTEXT'); + expect(step('reply-to-mention', 'Guard downloaded mention context').run).toContain( + 'canonical-trigger-context.json', + ); + }); + + it('uses resolved immutable SHAs and never checks out mutable pull refs', () => { + const reviewCheckout = step('review', 'Checkout PR head'); + const feedbackCheckout = step('reply-to-feedback', 'Checkout PR head'); + expect(reviewCheckout.with?.ref).toBe(reviewShaExpression); + expect(feedbackCheckout.with?.ref).toBe(feedbackShaExpression); + expect(JSON.stringify(workflow)).not.toContain('refs/pull/'); + }); + + it('makes feedback context fetching work with the direct-route token before conditional credentials', () => { + const feedback = job('reply-to-feedback'); + const parse = step('reply-to-feedback', 'Parse comment context'); + const check = step('reply-to-feedback', 'Check if reply is to agent comment'); + const credentials = step('reply-to-feedback', 'Setup credentials'); + expect(parse.env?.GH_TOKEN).toBe(githubTokenExpression); + expect(stepIndex('reply-to-feedback', parse.name ?? '')).toBeLessThan( + stepIndex('reply-to-feedback', check.name ?? ''), + ); + expect(stepIndex('reply-to-feedback', check.name ?? '')).toBeLessThan( + stepIndex('reply-to-feedback', credentials.name ?? ''), + ); + expect(credentials.if).toBe("steps.check.outputs.is_agent == 'true'"); + expect(feedback.permissions?.['pull-requests']).toBe('write'); + }); + + it('uses canonical event data for workflow-run mentions and direct event data otherwise', () => { + const mention = job('reply-to-mention'); + const synthesize = step('reply-to-mention', 'Synthesize mention-reply event context'); + const resolveEvent = step('reply-to-mention', 'Resolve event context for mention-reply action'); + const credentials = step('reply-to-mention', 'Setup credentials'); + const handler = step('reply-to-mention', 'Run mention-reply handler'); + const completionReaction = step('reply-to-mention', 'Add completion reaction'); + expect(synthesize.if).toContain(triggerRoute); + expect(synthesize.run).toContain('TRIGGER_RUN_ID'); + expect(synthesize.run).not.toContain('${{'); + expect(resolveEvent.run).toContain('if [ -n "$TRIGGER_RUN_ID" ]'); + expect(resolveEvent.run).toContain('path=$GITHUB_EVENT_PATH'); + expect(credentials.if).toBe("steps.resolve-event.outputs.path != ''"); + expect(handler.if).toBe("steps.resolve-event.outputs.path != ''"); + expect(completionReaction.env?.EVENT_NAME).toBe('${' + '{ steps.resolve-event.outputs.name }}'); + expect(completionReaction.env?.EVENT_NAME).not.toContain('github.event_name'); + expect(completionReaction.env?.EVENT_PATH).toBe('${' + '{ steps.resolve-event.outputs.path }}'); + expect(completionReaction.run).toContain('jq -r \'.comment.id // empty\' "$EVENT_PATH"'); + expect(completionReaction.run).toContain( + 'if [ "$EVENT_NAME" = "pull_request_review_comment" ]; then', + ); + expect(completionReaction.run).toContain('repos/$REPO/pulls/comments/$COMMENT_ID/reactions'); + expect(completionReaction.run).toContain('repos/$REPO/issues/comments/$COMMENT_ID/reactions'); + expect(stepIndex('reply-to-mention', resolveEvent.name ?? '')).toBeLessThan( + stepIndex('reply-to-mention', credentials.name ?? ''), + ); + expect(stepIndex('reply-to-mention', credentials.name ?? '')).toBeLessThan( + stepIndex('reply-to-mention', handler.name ?? ''), + ); + expect(mention.if).toContain("trigger-route == 'mention'"); + }); + + it('uses an attempt-unique, same-run artifact name without selecting artifacts by name', () => { + const upload = step('resolve-context', 'Upload canonical trigger context'); + expect(upload.with?.name).toBe(uploadName); + for (const route of ['reply-to-feedback', 'reply-to-mention']) { + expect(step(route, 'Download canonical trigger context').with?.name).toBeUndefined(); + } + }); +});