diff --git a/.github/scripts/resolve_integration_test_target.js b/.github/scripts/resolve_integration_test_target.js new file mode 100644 index 00000000000..af57b8df040 --- /dev/null +++ b/.github/scripts/resolve_integration_test_target.js @@ -0,0 +1,170 @@ +// Copyright (c) Microsoft. All rights reserved. + +const DECISIVE_REVIEW_STATES = new Set(['APPROVED', 'CHANGES_REQUESTED', 'DISMISSED']); +const SHA_PATTERN = /^[0-9a-f]{40}$/; +const BRANCH_PATTERN = /^[a-zA-Z0-9_./-]+$/; + +function assertValidSha(sha, description) { + if (!SHA_PATTERN.test(sha)) { + throw new Error(`GitHub returned an invalid ${description} SHA.`); + } +} + +function hasWritePermission(permissionData) { + return permissionData.user?.permissions?.push === true + || ['admin', 'maintain', 'write'].includes(permissionData.permission); +} + +function latestDecisiveReviews(reviews) { + const latestByReviewer = new Map(); + const sortedReviews = [...reviews].sort((left, right) => { + const submittedComparison = (left.submitted_at || '').localeCompare(right.submitted_at || ''); + return submittedComparison || Number(left.id) - Number(right.id); + }); + + for (const review of sortedReviews) { + const state = review.state?.toUpperCase(); + const reviewer = review.user?.login?.toLowerCase(); + if (reviewer && DECISIVE_REVIEW_STATES.has(state)) { + latestByReviewer.set(reviewer, review); + } + } + + return latestByReviewer; +} + +async function resolvePullRequest({ github, context, core, prNumber, requiredApprovals }) { + if (!/^[0-9]+$/.test(prNumber)) { + throw new Error('Invalid PR number. Only numeric values are allowed.'); + } + + const pullNumber = Number(prNumber); + const { data: pullRequest } = await github.rest.pulls.get({ + ...context.repo, + pull_number: pullNumber, + }); + + if (pullRequest.state !== 'open') { + throw new Error(`PR #${pullNumber} is not open (state: ${pullRequest.state}).`); + } + + const headSha = pullRequest.head.sha; + const baseSha = pullRequest.base.sha; + assertValidSha(headSha, 'PR head'); + assertValidSha(baseSha, 'PR base'); + + const reviews = await github.paginate(github.rest.pulls.listReviews, { + ...context.repo, + pull_number: pullNumber, + per_page: 100, + }); + const latestReviews = latestDecisiveReviews(reviews); + const author = pullRequest.user?.login?.toLowerCase(); + const approvalCandidates = [...latestReviews.entries()] + .filter(([, review]) => review.state.toUpperCase() === 'APPROVED') + .filter(([, review]) => review.commit_id === headSha) + .filter(([reviewer]) => reviewer !== author); + + const approvedMaintainers = []; + for (const [reviewer] of approvalCandidates) { + const { data: permissionData } = await github.rest.repos.getCollaboratorPermissionLevel({ + ...context.repo, + username: reviewer, + }); + if (hasWritePermission(permissionData)) { + approvedMaintainers.push(reviewer); + } else { + core.info(`Ignoring approval from ${reviewer}: reviewer does not have write permission.`); + } + } + + if (approvedMaintainers.length < requiredApprovals) { + throw new Error( + `PR #${pullNumber} head ${headSha} requires ${requiredApprovals} approvals from unique ` + + `write-capable maintainers; found ${approvedMaintainers.length}.`, + ); + } + + core.info( + `PR #${pullNumber} head ${headSha} approved by: ${approvedMaintainers.join(', ')}.`, + ); + return { + baseRef: baseSha, + checkoutRef: headSha, + description: `PR #${pullNumber}`, + }; +} + +async function resolveBranch({ github, context, core, branch }) { + if (!BRANCH_PATTERN.test(branch)) { + throw new Error( + 'Invalid branch name. Only alphanumeric characters, hyphens, underscores, dots, and slashes ' + + 'are allowed.', + ); + } + + const [{ data: repository }, { data: targetBranch }] = await Promise.all([ + github.rest.repos.get(context.repo), + github.rest.repos.getBranch({ ...context.repo, branch }), + ]); + const { data: baseBranch } = await github.rest.repos.getBranch({ + ...context.repo, + branch: repository.default_branch, + }); + + const checkoutRef = targetBranch.commit.sha; + const baseRef = baseBranch.commit.sha; + assertValidSha(checkoutRef, 'branch head'); + assertValidSha(baseRef, 'default branch'); + core.info(`Branch ${branch} resolved to immutable commit ${checkoutRef}.`); + + return { + baseRef, + checkoutRef, + description: `branch ${branch}`, + }; +} + +/** + * Resolve a manually requested integration-test target to an immutable commit. + * + * Pull requests must have fresh approvals from two unique write-capable + * maintainers for the exact head commit. Branches are limited to branches in + * the base repository and are pinned to their current commit. + */ +async function resolveIntegrationTestTarget({ + github, + context, + core, + prNumber = '', + branch = '', + requiredApprovals = 2, +}) { + const normalizedPrNumber = prNumber.trim(); + const normalizedBranch = branch.trim(); + + if (normalizedPrNumber && normalizedBranch) { + throw new Error('Please provide either a PR number or a branch name, not both.'); + } + if (!normalizedPrNumber && !normalizedBranch) { + throw new Error('Please provide either a PR number or a branch name.'); + } + + if (normalizedPrNumber) { + return resolvePullRequest({ + github, + context, + core, + prNumber: normalizedPrNumber, + requiredApprovals, + }); + } + return resolveBranch({ + github, + context, + core, + branch: normalizedBranch, + }); +} + +module.exports = resolveIntegrationTestTarget; diff --git a/.github/tests/test_resolve_integration_test_target.js b/.github/tests/test_resolve_integration_test_target.js new file mode 100644 index 00000000000..3fe91a1e56e --- /dev/null +++ b/.github/tests/test_resolve_integration_test_target.js @@ -0,0 +1,212 @@ +// Copyright (c) Microsoft. All rights reserved. + +/** + * Tests for resolve_integration_test_target.js. + * + * Run with: node --test .github/tests/test_resolve_integration_test_target.js + */ + +const { describe, it } = require('node:test'); +const assert = require('node:assert/strict'); + +const resolveIntegrationTestTarget = require('../scripts/resolve_integration_test_target.js'); + +const HEAD_SHA = 'a'.repeat(40); +const BASE_SHA = 'b'.repeat(40); + +function review({ + id, + login, + state = 'APPROVED', + commitId = HEAD_SHA, + submittedAt = `2026-07-13T00:00:${String(id).padStart(2, '0')}Z`, +}) { + return { + id, + state, + commit_id: commitId, + submitted_at: submittedAt, + user: { login }, + }; +} + +function createMocks({ + pullState = 'open', + pullAuthor = 'contributor', + reviews = [], + permissions = {}, +} = {}) { + const core = { + infoMessages: [], + info(message) { + this.infoMessages.push(message); + }, + }; + const context = { + repo: { owner: 'microsoft', repo: 'agent-framework' }, + }; + const github = { + paginate: async () => reviews, + rest: { + pulls: { + get: async () => ({ + data: { + state: pullState, + user: { login: pullAuthor }, + head: { sha: HEAD_SHA }, + base: { sha: BASE_SHA }, + }, + }), + listReviews: async () => {}, + }, + repos: { + get: async () => ({ data: { default_branch: 'main' } }), + getBranch: async ({ branch }) => ({ + data: { commit: { sha: branch === 'main' ? BASE_SHA : HEAD_SHA } }, + }), + getCollaboratorPermissionLevel: async ({ username }) => ({ + data: permissions[username] || { + permission: 'read', + user: { permissions: { push: false } }, + }, + }), + }, + }, + }; + + return { core, context, github }; +} + +const WRITE_PERMISSION = { + permission: 'write', + user: { permissions: { push: true } }, +}; + +describe('input validation', () => { + it('rejects missing and conflicting targets', async () => { + const mocks = createMocks(); + await assert.rejects( + () => resolveIntegrationTestTarget(mocks), + /provide either a PR number or a branch name/, + ); + await assert.rejects( + () => resolveIntegrationTestTarget({ ...mocks, prNumber: '1', branch: 'feature' }), + /not both/, + ); + }); + + it('rejects invalid PR numbers and branch names', async () => { + const mocks = createMocks(); + await assert.rejects( + () => resolveIntegrationTestTarget({ ...mocks, prNumber: '1;echo' }), + /Invalid PR number/, + ); + await assert.rejects( + () => resolveIntegrationTestTarget({ ...mocks, branch: 'feature branch' }), + /Invalid branch name/, + ); + }); +}); + +describe('pull request resolution', () => { + it('pins an open PR with two fresh write-capable approvals', async () => { + const mocks = createMocks({ + reviews: [ + review({ id: 1, login: 'maintainer-one' }), + review({ id: 2, login: 'maintainer-two' }), + ], + permissions: { + 'maintainer-one': WRITE_PERMISSION, + 'maintainer-two': WRITE_PERMISSION, + }, + }); + + const result = await resolveIntegrationTestTarget({ ...mocks, prNumber: '123' }); + + assert.deepEqual(result, { + baseRef: BASE_SHA, + checkoutRef: HEAD_SHA, + description: 'PR #123', + }); + }); + + it('rejects closed PRs', async () => { + const mocks = createMocks({ pullState: 'closed' }); + await assert.rejects( + () => resolveIntegrationTestTarget({ ...mocks, prNumber: '123' }), + /is not open/, + ); + }); + + it('ignores stale, self, and read-only approvals', async () => { + const mocks = createMocks({ + reviews: [ + review({ id: 1, login: 'stale', commitId: 'c'.repeat(40) }), + review({ id: 2, login: 'contributor' }), + review({ id: 3, login: 'reader' }), + review({ id: 4, login: 'maintainer' }), + ], + permissions: { + contributor: WRITE_PERMISSION, + reader: { permission: 'read', user: { permissions: { push: false } } }, + maintainer: WRITE_PERMISSION, + }, + }); + + await assert.rejects( + () => resolveIntegrationTestTarget({ ...mocks, prNumber: '123' }), + /found 1/, + ); + }); + + it('uses each reviewer latest decisive review and ignores later comments', async () => { + const mocks = createMocks({ + reviews: [ + review({ id: 1, login: 'changes-requested' }), + review({ id: 2, login: 'changes-requested', state: 'CHANGES_REQUESTED' }), + review({ id: 3, login: 'maintainer-one' }), + review({ id: 4, login: 'maintainer-one', state: 'COMMENTED' }), + review({ id: 5, login: 'maintainer-two' }), + ], + permissions: { + 'changes-requested': WRITE_PERMISSION, + 'maintainer-one': WRITE_PERMISSION, + 'maintainer-two': WRITE_PERMISSION, + }, + }); + + const result = await resolveIntegrationTestTarget({ ...mocks, prNumber: '123' }); + assert.equal(result.checkoutRef, HEAD_SHA); + }); + + it('does not count a dismissed approval', async () => { + const mocks = createMocks({ + reviews: [ + review({ id: 1, login: 'dismissed', state: 'DISMISSED' }), + review({ id: 2, login: 'maintainer' }), + ], + permissions: { + dismissed: WRITE_PERMISSION, + maintainer: WRITE_PERMISSION, + }, + }); + + await assert.rejects( + () => resolveIntegrationTestTarget({ ...mocks, prNumber: '123' }), + /found 1/, + ); + }); +}); + +describe('branch resolution', () => { + it('pins base-repository branches and their comparison base to SHAs', async () => { + const mocks = createMocks(); + const result = await resolveIntegrationTestTarget({ ...mocks, branch: 'feature/test' }); + + assert.deepEqual(result, { + baseRef: BASE_SHA, + checkoutRef: HEAD_SHA, + description: 'branch feature/test', + }); + }); +}); diff --git a/.github/workflows/dotnet-integration-tests.yml b/.github/workflows/dotnet-integration-tests.yml index 4a10694bda5..18f7cdfd6c0 100644 --- a/.github/workflows/dotnet-integration-tests.yml +++ b/.github/workflows/dotnet-integration-tests.yml @@ -9,16 +9,31 @@ on: workflow_call: inputs: checkout-ref: - description: "Git ref to checkout (e.g., refs/pull/123/head)" + description: "Immutable commit SHA to check out" required: true type: string + secrets: + AZURE_CLIENT_ID: + required: true + AZURE_TENANT_ID: + required: true + AZURE_SUBSCRIPTION_ID: + required: true + AZUREAI__ENDPOINT: + required: true + COPILOT_GITHUB_TOKEN: + required: true + OPENAI__APIKEY: + required: true permissions: contents: read - id-token: write jobs: dotnet-integration-tests: + permissions: + contents: read + id-token: write strategy: fail-fast: false matrix: diff --git a/.github/workflows/integration-tests-manual.yml b/.github/workflows/integration-tests-manual.yml index d3d617fa686..ee766abb693 100644 --- a/.github/workflows/integration-tests-manual.yml +++ b/.github/workflows/integration-tests-manual.yml @@ -3,7 +3,7 @@ # Go to Actions → "Integration Tests (Manual)" → Run workflow → enter a PR number or branch name. # # It calls dedicated integration-only workflows (dotnet-integration-tests and python-integration-tests), -# passing a ref so they check out and test the correct code. +# passing an immutable commit SHA so they check out and test the approved code. # Changed paths are detected here so only the relevant test suites run. # @@ -26,7 +26,6 @@ on: permissions: contents: read pull-requests: read - id-token: write concurrency: group: integration-tests-manual-${{ github.event.inputs.pr-number || github.event.inputs.branch }} @@ -38,67 +37,50 @@ jobs: runs-on: ubuntu-latest outputs: checkout-ref: ${{ steps.resolve.outputs.checkout-ref }} + base-ref: ${{ steps.resolve.outputs.base-ref }} dotnet-changes: ${{ steps.detect-changes.outputs.dotnet }} python-changes: ${{ steps.detect-changes.outputs.python }} steps: - - name: Resolve checkout ref + - name: Check out trusted workflow helpers + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6 + with: + ref: ${{ github.sha }} + persist-credentials: false + sparse-checkout: .github/scripts + + - name: Resolve and authorize checkout ref id: resolve + uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8 + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + script: | + const resolveIntegrationTestTarget = require( + './.github/scripts/resolve_integration_test_target.js' + ); + const target = await resolveIntegrationTestTarget({ + github, + context, + core, + prNumber: process.env.PR_NUMBER, + branch: process.env.BRANCH, + }); + core.setOutput('checkout-ref', target.checkoutRef); + core.setOutput('base-ref', target.baseRef); + core.info(`Running integration tests for ${target.description} at ${target.checkoutRef}.`); env: - GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} PR_NUMBER: ${{ github.event.inputs.pr-number }} BRANCH: ${{ github.event.inputs.branch }} - REPO: ${{ github.repository }} - run: | - if [ -n "$PR_NUMBER" ] && [ -n "$BRANCH" ]; then - echo "::error::Please provide either a PR number or a branch name, not both." - exit 1 - fi - - if [ -z "$PR_NUMBER" ] && [ -z "$BRANCH" ]; then - echo "::error::Please provide either a PR number or a branch name." - exit 1 - fi - - if [ -n "$PR_NUMBER" ]; then - if ! echo "$PR_NUMBER" | grep -Eq '^[0-9]+$'; then - echo "::error::Invalid PR number. Only numeric values are allowed." - exit 1 - fi - - PR_DATA=$(gh pr view "$PR_NUMBER" --repo "$REPO" --json state) - PR_STATE=$(echo "$PR_DATA" | jq -r '.state') - - if [ "$PR_STATE" != "OPEN" ]; then - echo "::error::PR #$PR_NUMBER is not open (state: $PR_STATE)" - exit 1 - fi - - echo "checkout-ref=refs/pull/$PR_NUMBER/head" >> "$GITHUB_OUTPUT" - echo "Running integration tests for PR #$PR_NUMBER" - else - if ! echo "$BRANCH" | grep -Eq '^[a-zA-Z0-9_./-]+$'; then - echo "::error::Invalid branch name. Only alphanumeric characters, hyphens, underscores, dots, and slashes are allowed." - exit 1 - fi - - echo "checkout-ref=$BRANCH" >> "$GITHUB_OUTPUT" - echo "Running integration tests for branch $BRANCH" - fi - name: Detect changed paths id: detect-changes env: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} - PR_NUMBER: ${{ github.event.inputs.pr-number }} - BRANCH: ${{ github.event.inputs.branch }} + BASE_REF: ${{ steps.resolve.outputs.base-ref }} + CHECKOUT_REF: ${{ steps.resolve.outputs.checkout-ref }} REPO: ${{ github.repository }} run: | - if [ -n "$PR_NUMBER" ]; then - CHANGED_FILES=$(gh pr diff "$PR_NUMBER" --repo "$REPO" --name-only) - else - # For branches, compare against main using the GitHub API - CHANGED_FILES=$(gh api "repos/$REPO/compare/main...$BRANCH" --jq '.files[].filename') - fi + CHANGED_FILES=$(gh api "repos/$REPO/compare/$BASE_REF...$CHECKOUT_REF" \ + --jq '.files[].filename') DOTNET_CHANGES=false PYTHON_CHANGES=false @@ -113,22 +95,41 @@ jobs: echo "dotnet=$DOTNET_CHANGES" >> "$GITHUB_OUTPUT" echo "python=$PYTHON_CHANGES" >> "$GITHUB_OUTPUT" - echo "Detected changes — dotnet: $DOTNET_CHANGES, python: $PYTHON_CHANGES" + echo "Detected changes; dotnet: $DOTNET_CHANGES, python: $PYTHON_CHANGES" dotnet-integration-tests: name: .NET Integration Tests needs: resolve-ref if: needs.resolve-ref.outputs.dotnet-changes == 'true' + permissions: + contents: read + id-token: write uses: ./.github/workflows/dotnet-integration-tests.yml with: checkout-ref: ${{ needs.resolve-ref.outputs.checkout-ref }} - secrets: inherit + secrets: + AZURE_CLIENT_ID: ${{ secrets.AZURE_CLIENT_ID }} + AZURE_TENANT_ID: ${{ secrets.AZURE_TENANT_ID }} + AZURE_SUBSCRIPTION_ID: ${{ secrets.AZURE_SUBSCRIPTION_ID }} + AZUREAI__ENDPOINT: ${{ secrets.AZUREAI__ENDPOINT }} + COPILOT_GITHUB_TOKEN: ${{ secrets.COPILOT_GITHUB_TOKEN }} + OPENAI__APIKEY: ${{ secrets.OPENAI__APIKEY }} python-integration-tests: name: Python Integration Tests needs: resolve-ref if: needs.resolve-ref.outputs.python-changes == 'true' + permissions: + contents: read + id-token: write uses: ./.github/workflows/python-integration-tests.yml with: checkout-ref: ${{ needs.resolve-ref.outputs.checkout-ref }} - secrets: inherit + secrets: + ANTHROPIC_API_KEY: ${{ secrets.ANTHROPIC_API_KEY }} + AZURE_CLIENT_ID: ${{ secrets.AZURE_CLIENT_ID }} + AZURE_TENANT_ID: ${{ secrets.AZURE_TENANT_ID }} + AZURE_SUBSCRIPTION_ID: ${{ secrets.AZURE_SUBSCRIPTION_ID }} + COPILOT_GITHUB_TOKEN: ${{ secrets.COPILOT_GITHUB_TOKEN }} + FOUNDRY_MODELS_API_KEY: ${{ secrets.FOUNDRY_MODELS_API_KEY }} + OPENAI__APIKEY: ${{ secrets.OPENAI__APIKEY }} diff --git a/.github/workflows/python-integration-tests.yml b/.github/workflows/python-integration-tests.yml index 8f6d6bf1265..d522d0c3c5f 100644 --- a/.github/workflows/python-integration-tests.yml +++ b/.github/workflows/python-integration-tests.yml @@ -13,13 +13,27 @@ on: workflow_call: inputs: checkout-ref: - description: "Git ref to checkout (e.g., refs/pull/123/head)" + description: "Immutable commit SHA to check out" required: true type: string + secrets: + ANTHROPIC_API_KEY: + required: true + AZURE_CLIENT_ID: + required: true + AZURE_TENANT_ID: + required: true + AZURE_SUBSCRIPTION_ID: + required: true + COPILOT_GITHUB_TOKEN: + required: true + FOUNDRY_MODELS_API_KEY: + required: false + OPENAI__APIKEY: + required: true permissions: contents: read - id-token: write env: UV_CACHE_DIR: /tmp/.uv-cache @@ -99,6 +113,9 @@ jobs: # Azure OpenAI integration tests python-tests-azure-openai: name: Python Integration Tests - Azure OpenAI + permissions: + contents: read + id-token: write runs-on: ubuntu-latest environment: integration timeout-minutes: 60 @@ -260,6 +277,9 @@ jobs: # Azure Functions + Durable Task integration tests python-tests-functions: name: Python Integration Tests - Functions + permissions: + contents: read + id-token: write runs-on: ubuntu-latest environment: integration timeout-minutes: 60 @@ -324,6 +344,9 @@ jobs: # Foundry integration tests python-tests-foundry: name: Python Integration Tests - Foundry + permissions: + contents: read + id-token: write runs-on: ubuntu-latest environment: integration timeout-minutes: 60 @@ -378,6 +401,9 @@ jobs: # Foundry Hosting integration tests python-tests-foundry-hosting: name: Python Integration Tests - Foundry Hosting + permissions: + contents: read + id-token: write runs-on: ubuntu-latest environment: integration timeout-minutes: 60