diff --git a/.github/workflows/arm-api-review.lock.yml b/.github/workflows/arm-api-review.lock.yml index 22ce65e33f50..0d55de94937c 100644 --- a/.github/workflows/arm-api-review.lock.yml +++ b/.github/workflows/arm-api-review.lock.yml @@ -1,4 +1,4 @@ -# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"dbc4a4bcf44913026b742f3374995a32c7f79172f0192676338a68451c749308","body_hash":"4c92627abe9eab5cd6657b8a78c79ade836a9af4f97cf2dc3e33dc9cb7eec358","compiler_version":"v0.86.2","strict":true,"agent_id":"copilot","agent_model":"gpt-5.6-sol?effort=high","detection_agent_id":"copilot","detection_agent_model":"gpt-5.6-sol?effort=high","engine_versions":{"copilot":"1.0.79"}} +# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"abec1bffb3995c8bc5ea29c0b14653bbbabb26a77b4c975bb4f69f23b01280fc","body_hash":"cb0f2e92399e5be203fc6aef3f6b833929d8c6f1b9ca4779965cdc6cd2fc770e","compiler_version":"v0.86.2","strict":true,"agent_id":"copilot","agent_model":"gpt-5.6-sol?effort=high","detection_agent_id":"copilot","detection_agent_model":"gpt-5.6-sol?effort=high","engine_versions":{"copilot":"1.0.79"}} # gh-aw-manifest: {"version":1,"secrets":["GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GITHUB_TOKEN"],"actions":[{"repo":"actions/cache/restore","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/cache/save","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/checkout","sha":"3d3c42e5aac5ba805825da76410c181273ba90b1","version":"v7.0.1"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"373c709c69115d41ff229c7e5df9f8788daa9553","version":"v9"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/github-script","sha":"d746ffe35508b1917358783b479e04febd2b8f71","version":"v9.0.0"},{"repo":"actions/setup-node","sha":"820762786026740c76f36085b0efc47a31fe5020","version":"v7.0.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"},{"repo":"github/gh-aw-actions/setup","sha":"6aab9e5b5c91c615506061f09bedd81a23babe3c","version":"v0.86.2"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.27.44","digest":"sha256:0d727725c737b58c7bdf51f640cffb928385ec46517e0917c7f1a02f1bada8b4","pinned_image":"ghcr.io/github/gh-aw-firewall/agent:0.27.44@sha256:0d727725c737b58c7bdf51f640cffb928385ec46517e0917c7f1a02f1bada8b4"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.44","digest":"sha256:b50fbadba138f6e9aba94aca09711335c489bb3b15861220cb66f6092e042dc7","pinned_image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.44@sha256:b50fbadba138f6e9aba94aca09711335c489bb3b15861220cb66f6092e042dc7"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.27.44","digest":"sha256:83e48bbe12c634be8c228a576832fe45f66c529ac3659db92bddbcf2eeb6d627","pinned_image":"ghcr.io/github/gh-aw-firewall/squid:0.27.44@sha256:83e48bbe12c634be8c228a576832fe45f66c529ac3659db92bddbcf2eeb6d627"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.4.9","digest":"sha256:e5a1569aeaf41820fa7bdee3e94468cae448133cdbf00119ad24f5b74db1ab9f","pinned_image":"ghcr.io/github/gh-aw-mcpg:v0.4.9@sha256:e5a1569aeaf41820fa7bdee3e94468cae448133cdbf00119ad24f5b74db1ab9f"},{"image":"ghcr.io/github/gh-aw-node","digest":"sha256:0d9f1fb5fd6610c0ac1f5194a38e45a8a1e81f8a390d5142d8e4e6f26a4b3196","pinned_image":"ghcr.io/github/gh-aw-node@sha256:0d9f1fb5fd6610c0ac1f5194a38e45a8a1e81f8a390d5142d8e4e6f26a4b3196"},{"image":"ghcr.io/github/github-mcp-server:v1.9.0","digest":"sha256:881b53d6f75f69bdbc1b5b10fc2f1361717c19054143b3a8529fb5c32061a50e","pinned_image":"ghcr.io/github/github-mcp-server:v1.9.0@sha256:881b53d6f75f69bdbc1b5b10fc2f1361717c19054143b3a8529fb5c32061a50e"}],"has_pull_request_target":true} # This file was automatically generated by gh-aw (v0.86.2). DO NOT EDIT. To debug this workflow, load the skill at https://github.com/github/gh-aw/blob/main/debug.md # @@ -44,12 +44,14 @@ # - actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 # - actions/cache/save@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 # v6.1.0 # - actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 +# - actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 (source v7) # - actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 # - actions/github-script@373c709c69115d41ff229c7e5df9f8788daa9553 # v9 # - actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 # - actions/github-script@d746ffe35508b1917358783b479e04febd2b8f71 # v9.0.0 # - actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 # - actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 +# - actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 (source v7) # - github/gh-aw-actions/setup@6aab9e5b5c91c615506061f09bedd81a23babe3c # v0.86.2 # # Container images used: @@ -69,12 +71,14 @@ on: - created # permissions: # Permissions applied to pre-activation job # pull-requests: read + # statuses: write pull_request_target: forks: - "*" types: - opened - synchronize + - reopened - labeled - ready_for_review # roles: # Roles processed as role check in pre-activation job @@ -88,7 +92,26 @@ on: # name: Resolve target pull request # uses: actions/github-script@v9 # with: - # script: "const value = process.env.TARGET_PR_NUMBER ?? \"\";\nif (!/^[1-9]\\d*$/.test(value)) {\n throw new Error(`Invalid or missing pull request number: ${JSON.stringify(value)}`);\n}\n\nif (context.eventName === \"issue_comment\" && !context.payload.issue?.pull_request) {\n throw new Error(`Issue #${value} is not a pull request`);\n}\n\nconst pullNumber = Number(value);\nif (!Number.isSafeInteger(pullNumber)) {\n throw new Error(`Pull request number is outside the safe integer range: ${value}`);\n}\n\nconst { data: pull } = await github.rest.pulls.get({\n ...context.repo,\n pull_number: pullNumber,\n});\ncore.setOutput(\"target_pr_number\", String(pull.number));\n" + # script: "const value = process.env.TARGET_PR_NUMBER ?? \"\";\nif (!/^[1-9]\\d*$/.test(value)) {\n throw new Error(`Invalid or missing pull request number: ${JSON.stringify(value)}`);\n}\n\nif (context.eventName === \"issue_comment\" && !context.payload.issue?.pull_request) {\n throw new Error(`Issue #${value} is not a pull request`);\n}\n\nconst pullNumber = Number(value);\nif (!Number.isSafeInteger(pullNumber)) {\n throw new Error(`Pull request number is outside the safe integer range: ${value}`);\n}\n\nconst { data: pull } = await github.rest.pulls.get({\n ...context.repo,\n pull_number: pullNumber,\n});\nif (!/^[0-9a-f]{40}$/i.test(pull.head.sha)) {\n throw new Error(`Invalid pull request head SHA: ${JSON.stringify(pull.head.sha)}`);\n}\nconst runAttempt = process.env.GITHUB_RUN_ATTEMPT ?? \"\";\nif (!/^[1-9]\\d*$/.test(runAttempt)) {\n throw new Error(`Invalid workflow run attempt: ${JSON.stringify(runAttempt)}`);\n}\ncore.setOutput(\"target_pr_number\", String(pull.number));\ncore.setOutput(\"target_head_sha\", pull.head.sha);\ncore.setOutput(\"run_attempt\", runAttempt);\n" + # - if: steps.check_membership.outputs.is_team_member == 'true' + # name: Prepare ARM semantic review correlation artifact + # run: "mkdir -p \"$RUNNER_TEMP/arm-semantic-review\"\n: > \"$RUNNER_TEMP/arm-semantic-review/empty.txt\"\n" + # shell: bash + # - if: steps.check_membership.outputs.is_team_member == 'true' + # name: Upload ARM semantic review correlation + # uses: actions/upload-artifact@v7 + # with: + # if-no-files-found: error + # name: head-sha=${{ steps.resolve_target_pr.outputs.target_head_sha }};issue-number=${{ steps.resolve_target_pr.outputs.target_pr_number }} + # overwrite: true + # path: ${{ runner.temp }}/arm-semantic-review/empty.txt + # - env: + # TARGET_HEAD_SHA: ${{ steps.resolve_target_pr.outputs.target_head_sha }} + # if: steps.check_membership.outputs.is_team_member == 'true' + # name: Set ARM semantic review pending + # uses: actions/github-script@v9 + # with: + # script: "const runAttempt = process.env.GITHUB_RUN_ATTEMPT ?? \"\";\nif (!/^[1-9]\\d*$/.test(runAttempt)) {\n throw new Error(`Invalid workflow run attempt: ${JSON.stringify(runAttempt)}`);\n}\nawait github.rest.repos.createCommitStatus({\n ...context.repo,\n sha: process.env.TARGET_HEAD_SHA,\n state: \"pending\",\n context: \"ARM Semantic Review\",\n description: \"ARM API semantic review is pending\",\n target_url:\n `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/` +\n `${context.runId}/attempts/${runAttempt}`,\n});\n" workflow_dispatch: inputs: aw_context: @@ -116,6 +139,7 @@ jobs: needs.pre_activation.outputs.activated == 'true' && (github.event_name == 'workflow_dispatch' || (github.event_name == 'pull_request_target' && (github.event.action == 'opened' || github.event.action == 'synchronize' || + github.event.action == 'reopened' || github.event.action == 'ready_for_review') && github.event.pull_request.draft == false && contains(github.event.pull_request.labels.*.name, 'WaitForARMFeedback')) || @@ -332,9 +356,11 @@ jobs: GH_AW_GITHUB_REPOSITORY: ${{ github.repository }} GH_AW_GITHUB_RUN_ID: ${{ github.run_id }} GH_AW_GITHUB_WORKSPACE: ${{ github.workspace }} + GH_AW_NEEDS_PRE_ACTIVATION_OUTPUTS_RUN_ATTEMPT: ${{ needs.pre_activation.outputs.run_attempt }} + GH_AW_NEEDS_PRE_ACTIVATION_OUTPUTS_TARGET_HEAD_SHA: ${{ needs.pre_activation.outputs.target_head_sha }} GH_AW_NEEDS_PRE_ACTIVATION_OUTPUTS_TARGET_PR_NUMBER: ${{ needs.pre_activation.outputs.target_pr_number }} GH_AW_PROMPT_CONTENT_0000: "\n" - GH_AW_PROMPT_CONTENT_0001: "\nTools: add_comment(max:4), create_pull_request_review_comment(max:50), submit_pull_request_review, reply_to_pull_request_review_comment(max:50), resolve_pull_request_review_thread(max:50), add_labels(max:3), remove_labels(max:3), missing_tool, missing_data, noop\n" + GH_AW_PROMPT_CONTENT_0001: "\nTools: add_comment(max:4), create_pull_request_review_comment(max:50), submit_pull_request_review, reply_to_pull_request_review_comment(max:50), resolve_pull_request_review_thread(max:50), add_labels(max:3), remove_labels(max:3), missing_tool, missing_data, noop, record_arm_semantic_review\n" GH_AW_PROMPT_CONTENT_0002: "\n" GH_AW_PROMPT_CONTENT_0003: "\nThe following GitHub context information is available for this workflow:\n{{#if github.actor}}\n- **actor**: __GH_AW_GITHUB_ACTOR__\n{{/if}}\n{{#if github.repository}}\n- **repository**: __GH_AW_GITHUB_REPOSITORY__\n{{/if}}\n{{#if github.workspace}}\n- **workspace**: __GH_AW_GITHUB_WORKSPACE__\n{{/if}}\n{{#if github.event.issue.number || (github.aw.context.item_type == 'issue' && github.aw.context.item_number)}}\n- **issue-number**: #__GH_AW_EXPR_802A9F6A__\n{{/if}}\n{{#if github.event.discussion.number || (github.aw.context.item_type == 'discussion' && github.aw.context.item_number)}}\n- **discussion-number**: #__GH_AW_EXPR_1A3A194A__\n{{/if}}\n{{#if github.event.pull_request.number || (github.aw.context.item_type == 'pull_request' && github.aw.context.item_number)}}\n- **pull-request-number**: #__GH_AW_EXPR_463A214A__\n{{/if}}\n{{#if github.event.comment.id || github.aw.context.comment_id}}\n- **comment-id**: __GH_AW_EXPR_FF1D34CE__\n{{/if}}\n{{#if github.run_id}}\n- **workflow-run-id**: __GH_AW_GITHUB_RUN_ID__\n{{/if}}\n\n\n" GH_AW_PROMPT_CONTENT_0004: "\n" @@ -359,6 +385,8 @@ jobs: GH_AW_ENGINE_ID: "copilot" GH_AW_GITHUB_EVENT_NAME: ${{ github.event_name }} GH_AW_GITHUB_REPOSITORY: ${{ github.repository }} + GH_AW_NEEDS_PRE_ACTIVATION_OUTPUTS_RUN_ATTEMPT: ${{ needs.pre_activation.outputs.run_attempt }} + GH_AW_NEEDS_PRE_ACTIVATION_OUTPUTS_TARGET_HEAD_SHA: ${{ needs.pre_activation.outputs.target_head_sha }} GH_AW_NEEDS_PRE_ACTIVATION_OUTPUTS_TARGET_PR_NUMBER: ${{ needs.pre_activation.outputs.target_pr_number }} with: script: | @@ -381,6 +409,8 @@ jobs: GH_AW_GITHUB_WORKSPACE: ${{ github.workspace }} GH_AW_MCP_CLI_SERVERS_LIST: "- `github` β€” run `github --help` to see available tools\n- `safeoutputs` β€” run `safeoutputs --help` to see available tools" GH_AW_NEEDS_PRE_ACTIVATION_OUTPUTS_ACTIVATED: ${{ needs.pre_activation.outputs.activated }} + GH_AW_NEEDS_PRE_ACTIVATION_OUTPUTS_RUN_ATTEMPT: ${{ needs.pre_activation.outputs.run_attempt }} + GH_AW_NEEDS_PRE_ACTIVATION_OUTPUTS_TARGET_HEAD_SHA: ${{ needs.pre_activation.outputs.target_head_sha }} GH_AW_NEEDS_PRE_ACTIVATION_OUTPUTS_TARGET_PR_NUMBER: ${{ needs.pre_activation.outputs.target_pr_number }} with: script: | @@ -404,6 +434,8 @@ jobs: GH_AW_GITHUB_WORKSPACE: process.env.GH_AW_GITHUB_WORKSPACE, GH_AW_MCP_CLI_SERVERS_LIST: process.env.GH_AW_MCP_CLI_SERVERS_LIST, GH_AW_NEEDS_PRE_ACTIVATION_OUTPUTS_ACTIVATED: process.env.GH_AW_NEEDS_PRE_ACTIVATION_OUTPUTS_ACTIVATED, + GH_AW_NEEDS_PRE_ACTIVATION_OUTPUTS_RUN_ATTEMPT: process.env.GH_AW_NEEDS_PRE_ACTIVATION_OUTPUTS_RUN_ATTEMPT, + GH_AW_NEEDS_PRE_ACTIVATION_OUTPUTS_TARGET_HEAD_SHA: process.env.GH_AW_NEEDS_PRE_ACTIVATION_OUTPUTS_TARGET_HEAD_SHA, GH_AW_NEEDS_PRE_ACTIVATION_OUTPUTS_TARGET_PR_NUMBER: process.env.GH_AW_NEEDS_PRE_ACTIVATION_OUTPUTS_TARGET_PR_NUMBER } }); @@ -555,9 +587,9 @@ jobs: mkdir -p "${RUNNER_TEMP}/gh-aw/safeoutputs" mkdir -p /tmp/gh-aw/safeoutputs mkdir -p /tmp/gh-aw/mcp-logs/safeoutputs - cat > "${RUNNER_TEMP}/gh-aw/safeoutputs/config.json" << 'GH_AW_SAFE_OUTPUTS_CONFIG_193a4741548b2c28_EOF' - {"add_comment":{"max":4,"target":"${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}"},"add_labels":{"max":3,"target":"${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}"},"create_pull_request_review_comment":{"max":50,"side":"RIGHT","target":"${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}"},"create_report_incomplete_issue":{},"missing_data":{},"missing_tool":{},"noop":{"max":1,"report-as-issue":"true"},"remove_labels":{"max":3,"target":"${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}"},"reply_to_pull_request_review_comment":{"max":50,"target":"${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}"},"report_incomplete":{},"resolve_pull_request_review_thread":{"max":50},"submit_pull_request_review":{"footer":"if-body","max":1,"target":"${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}"}} - GH_AW_SAFE_OUTPUTS_CONFIG_193a4741548b2c28_EOF + cat > "${RUNNER_TEMP}/gh-aw/safeoutputs/config.json" << 'GH_AW_SAFE_OUTPUTS_CONFIG_ade4c9f8f4c77740_EOF' + {"add_comment":{"max":4,"target":"${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}"},"add_labels":{"max":3,"target":"${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}"},"create_pull_request_review_comment":{"max":50,"side":"RIGHT","target":"${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}"},"create_report_incomplete_issue":{},"missing_data":{},"missing_tool":{},"noop":{"max":1,"report-as-issue":"true"},"record-arm-semantic-review":{"description":"Validate and publish the ARM semantic review result for auto-signoff","inputs":{"blocking_count":{"default":null,"description":"Number of verified, currently applicable Blocking findings after reconciliation","required":true,"type":"string"},"completeness":{"default":null,"description":"Whether the intended review completed reliably","options":["complete","incomplete"],"required":true,"type":"choice"},"incomplete_reason":{"default":null,"description":"Why the review is incomplete, or none when complete","options":["none","critic-unavailable","required-evidence-unavailable","tool-failure","stale-sha","discussion-data-unavailable"],"required":true,"type":"choice"},"scope":{"default":null,"description":"Whether the full PR or only a scoped subset was reviewed","options":["full","scoped"],"required":true,"type":"choice"}},"output":"ARM semantic review status published"},"remove_labels":{"max":3,"target":"${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}"},"reply_to_pull_request_review_comment":{"max":50,"target":"${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}"},"report_incomplete":{},"resolve_pull_request_review_thread":{"max":50},"submit_pull_request_review":{"footer":"if-body","max":1,"target":"${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}"}} + GH_AW_SAFE_OUTPUTS_CONFIG_ade4c9f8f4c77740_EOF - name: Generate Safe Outputs Tools env: GH_AW_TOOLS_META_JSON: | @@ -572,7 +604,56 @@ jobs: "submit_pull_request_review": " CONSTRAINTS: Maximum 1 review(s) can be submitted. Target: ${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}." }, "repo_params": {}, - "dynamic_tools": [] + "dynamic_tools": [ + { + "description": "Validate and publish the ARM semantic review result for auto-signoff", + "inputSchema": { + "additionalProperties": false, + "properties": { + "blocking_count": { + "description": "Number of verified, currently applicable Blocking findings after reconciliation", + "type": "string" + }, + "completeness": { + "description": "Whether the intended review completed reliably", + "enum": [ + "complete", + "incomplete" + ], + "type": "string" + }, + "incomplete_reason": { + "description": "Why the review is incomplete, or none when complete", + "enum": [ + "none", + "critic-unavailable", + "required-evidence-unavailable", + "tool-failure", + "stale-sha", + "discussion-data-unavailable" + ], + "type": "string" + }, + "scope": { + "description": "Whether the full PR or only a scoped subset was reviewed", + "enum": [ + "full", + "scoped" + ], + "type": "string" + } + }, + "required": [ + "blocking_count", + "completeness", + "incomplete_reason", + "scope" + ], + "type": "object" + }, + "name": "record_arm_semantic_review" + } + ] } GH_AW_VALIDATION_JSON: | { @@ -1169,6 +1250,7 @@ jobs: - activation - agent - detection + - record_arm_semantic_review - safe_outputs if: > always() && (needs.agent.result != 'skipped' || needs.activation.outputs.lockdown_check_failed == 'true' || @@ -1177,8 +1259,10 @@ jobs: runs-on: ubuntu-slim permissions: actions: read + contents: read issues: write pull-requests: write + statuses: write concurrency: group: "gh-aw-conclusion-arm-api-review" cancel-in-progress: false @@ -1205,6 +1289,23 @@ jobs: GH_AW_INFO_VERSION: "1.0.79" GH_AW_INFO_AWF_VERSION: "v0.27.44" GH_AW_INFO_ENGINE_ID: "copilot" + - if: needs.record_arm_semantic_review.result != 'success' + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 (source v7) + with: + persist-credentials: false + sparse-checkout: | + .github + continue-on-error: true + - name: Resolve unpublished ARM semantic review status + if: needs.record_arm_semantic_review.result != 'success' + uses: actions/github-script@d746ffe35508b1917358783b479e04febd2b8f71 # v9.0.0 + with: + retries: 3 + script: | + const { finalizeUnpublishedArmSemanticReview } = + await import('${{ github.workspace }}/.github/workflows/src/arm-auto-signoff/arm-semantic-review-workflow.ts'); + return await finalizeUnpublishedArmSemanticReview({ github, context, core }); + continue-on-error: true - name: Download agent output artifact id: download-agent-output continue-on-error: true @@ -1435,6 +1536,7 @@ jobs: GH_AW_DETECTION_CONCLUSION: ${{ needs.detection.outputs.detection_conclusion }} GH_AW_DETECTION_REASON: ${{ needs.detection.outputs.detection_reason }} GH_AW_SAFE_OUTPUT_MESSAGES: "{\"footer\":\"\\u003e πŸ” *ARM API review by [{workflow_name}]({run_url})*\",\"runStarted\":\"πŸ” [{workflow_name}]({run_url}) is reviewing this PR for ARM API compliance…\",\"runSuccess\":\"πŸ” [{workflow_name}]({run_url}) completed the ARM API review. βœ…\",\"runFailure\":\"πŸ” [{workflow_name}]({run_url}) {status}. ❌\"}" + GH_AW_SAFE_OUTPUT_JOBS: "{\"record_arm_semantic_review\":\"\"}" with: github-token: ${{ secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }} script: | @@ -1715,6 +1817,7 @@ jobs: github.event_name == 'workflow_dispatch' || (github.event_name == 'pull_request_target' && (github.event.action == 'opened' || github.event.action == 'synchronize' || + github.event.action == 'reopened' || github.event.action == 'ready_for_review') && github.event.pull_request.draft == false && contains(github.event.pull_request.labels.*.name, 'WaitForARMFeedback')) || @@ -1729,15 +1832,18 @@ jobs: runs-on: ubuntu-slim permissions: pull-requests: read + statuses: write env: GH_AW_RUNTIME_FEATURES: ${{ vars.GH_AW_RUNTIME_FEATURES }} outputs: activated: ${{ steps.check_membership.outputs.is_team_member == 'true' }} matched_command: '' resolve_target_pr_result: ${{ steps.resolve_target_pr.outcome }} + run_attempt: ${{ steps.resolve_target_pr.outputs.run_attempt }} setup-parent-span-id: ${{ steps.setup.outputs.parent-span-id || steps.setup.outputs.span-id }} setup-span-id: ${{ steps.setup.outputs.span-id }} setup-trace-id: ${{ steps.setup.outputs.trace-id }} + target_head_sha: ${{ steps.resolve_target_pr.outputs.target_head_sha }} target_pr_number: ${{ steps.resolve_target_pr.outputs.target_pr_number }} steps: - name: Setup Scripts @@ -1790,7 +1896,89 @@ jobs: ...context.repo, pull_number: pullNumber, }); + if (!/^[0-9a-f]{40}$/i.test(pull.head.sha)) { + throw new Error(`Invalid pull request head SHA: ${JSON.stringify(pull.head.sha)}`); + } + const runAttempt = process.env.GITHUB_RUN_ATTEMPT ?? ""; + if (!/^[1-9]\d*$/.test(runAttempt)) { + throw new Error(`Invalid workflow run attempt: ${JSON.stringify(runAttempt)}`); + } core.setOutput("target_pr_number", String(pull.number)); + core.setOutput("target_head_sha", pull.head.sha); + core.setOutput("run_attempt", runAttempt); + - name: Prepare ARM semantic review correlation artifact + if: steps.check_membership.outputs.is_team_member == 'true' + run: | + mkdir -p "$RUNNER_TEMP/arm-semantic-review" + : > "$RUNNER_TEMP/arm-semantic-review/empty.txt" + shell: bash + - name: Upload ARM semantic review correlation + if: steps.check_membership.outputs.is_team_member == 'true' + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 (source v7) + with: + if-no-files-found: error + name: head-sha=${{ steps.resolve_target_pr.outputs.target_head_sha }};issue-number=${{ steps.resolve_target_pr.outputs.target_pr_number }} + overwrite: true + path: ${{ runner.temp }}/arm-semantic-review/empty.txt + - name: Set ARM semantic review pending + if: steps.check_membership.outputs.is_team_member == 'true' + uses: actions/github-script@373c709c69115d41ff229c7e5df9f8788daa9553 # v9 + env: + TARGET_HEAD_SHA: ${{ steps.resolve_target_pr.outputs.target_head_sha }} + with: + script: | + const runAttempt = process.env.GITHUB_RUN_ATTEMPT ?? ""; + if (!/^[1-9]\d*$/.test(runAttempt)) { + throw new Error(`Invalid workflow run attempt: ${JSON.stringify(runAttempt)}`); + } + await github.rest.repos.createCommitStatus({ + ...context.repo, + sha: process.env.TARGET_HEAD_SHA, + state: "pending", + context: "ARM Semantic Review", + description: "ARM API semantic review is pending", + target_url: + `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/` + + `${context.runId}/attempts/${runAttempt}`, + }); + + record_arm_semantic_review: + needs: + - agent + - detection + if: > + (!cancelled()) && needs.agent.result != 'skipped' && contains(needs.agent.outputs.output_types, 'record_arm_semantic_review') && + (needs.detection.outputs.detection_success == 'true') + runs-on: ubuntu-slim + permissions: + actions: read + contents: read + pull-requests: read + statuses: write + steps: + - name: Download agent output artifact + continue-on-error: true + uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + with: + name: agent + path: ${{ runner.temp }}/gh-aw/safe-jobs/ + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 (source v7) + env: + GH_AW_AGENT_OUTPUT: ${{ runner.temp }}/gh-aw/safe-jobs/agent_output.json + with: + persist-credentials: false + sparse-checkout: | + .github + - name: Publish ARM semantic review status + uses: actions/github-script@d746ffe35508b1917358783b479e04febd2b8f71 # v9.0.0 + env: + GH_AW_AGENT_OUTPUT: ${{ runner.temp }}/gh-aw/safe-jobs/agent_output.json + with: + retries: 3 + script: | + const { default: publishArmSemanticReviewStatus } = + await import('${{ github.workspace }}/.github/workflows/src/arm-auto-signoff/arm-semantic-review-workflow.ts'); + return await publishArmSemanticReviewStatus({ github, context, core }); safe_outputs: needs: @@ -1884,6 +2072,7 @@ jobs: GH_AW_ALLOWED_DOMAINS: "api.business.githubcopilot.com,api.enterprise.githubcopilot.com,api.github.com,api.githubcopilot.com,api.individual.githubcopilot.com,api.snapcraft.io,archive.ubuntu.com,azure.archive.ubuntu.com,crl.geotrust.com,crl.globalsign.com,crl.identrust.com,crl.sectigo.com,crl.thawte.com,crl.usertrust.com,crl.verisign.com,crl3.digicert.com,crl4.digicert.com,crls.ssl.com,github.com,host.docker.internal,json-schema.org,json.schemastore.org,keyserver.ubuntu.com,ocsp.digicert.com,ocsp.geotrust.com,ocsp.globalsign.com,ocsp.identrust.com,ocsp.sectigo.com,ocsp.ssl.com,ocsp.thawte.com,ocsp.usertrust.com,ocsp.verisign.com,packagecloud.io,packages.cloud.google.com,packages.microsoft.com,ppa.launchpad.net,raw.githubusercontent.com,registry.npmjs.org,s.symcb.com,s.symcd.com,security.ubuntu.com,telemetry.enterprise.githubcopilot.com,ts-crl.ws.symantec.com,ts-ocsp.ws.symantec.com,www.googleapis.com" GITHUB_SERVER_URL: ${{ github.server_url }} GITHUB_API_URL: ${{ github.api_url }} + GH_AW_SAFE_OUTPUT_JOBS: "{\"record_arm_semantic_review\":\"\"}" GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG: "{\"add_comment\":{\"max\":4,\"target\":\"${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}\"},\"add_labels\":{\"max\":3,\"target\":\"${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}\"},\"create_pull_request_review_comment\":{\"max\":50,\"side\":\"RIGHT\",\"target\":\"${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}\"},\"create_report_incomplete_issue\":{},\"missing_data\":{},\"missing_tool\":{},\"noop\":{\"max\":1,\"report-as-issue\":\"true\"},\"remove_labels\":{\"max\":3,\"target\":\"${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}\"},\"reply_to_pull_request_review_comment\":{\"max\":50,\"target\":\"${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}\"},\"report_incomplete\":{},\"resolve_pull_request_review_thread\":{\"max\":50},\"submit_pull_request_review\":{\"footer\":\"if-body\",\"max\":1,\"target\":\"${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}\"}}" with: github-token: ${{ secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }} diff --git a/.github/workflows/arm-api-review.md b/.github/workflows/arm-api-review.md index 1317b7100912..fac27e7ef25f 100644 --- a/.github/workflows/arm-api-review.md +++ b/.github/workflows/arm-api-review.md @@ -22,7 +22,7 @@ on: # fork PRs are reviewed only after a maintainer applies the label or runs # `/arm-review`. pull_request_target: - types: [opened, synchronize, labeled, ready_for_review] + types: [opened, synchronize, reopened, labeled, ready_for_review] forks: ["*"] issue_comment: types: [created] @@ -34,6 +34,7 @@ on: type: string permissions: pull-requests: read + statuses: write steps: - name: Resolve target pull request id: resolve_target_pr @@ -60,7 +61,53 @@ on: ...context.repo, pull_number: pullNumber, }); + if (!/^[0-9a-f]{40}$/i.test(pull.head.sha)) { + throw new Error(`Invalid pull request head SHA: ${JSON.stringify(pull.head.sha)}`); + } + const runAttempt = process.env.GITHUB_RUN_ATTEMPT ?? ""; + if (!/^[1-9]\d*$/.test(runAttempt)) { + throw new Error(`Invalid workflow run attempt: ${JSON.stringify(runAttempt)}`); + } core.setOutput("target_pr_number", String(pull.number)); + core.setOutput("target_head_sha", pull.head.sha); + core.setOutput("run_attempt", runAttempt); + - name: Prepare ARM semantic review correlation artifact + if: steps.check_membership.outputs.is_team_member == 'true' + shell: bash + run: | + mkdir -p "$RUNNER_TEMP/arm-semantic-review" + : > "$RUNNER_TEMP/arm-semantic-review/empty.txt" + - name: Upload ARM semantic review correlation + if: steps.check_membership.outputs.is_team_member == 'true' + uses: actions/upload-artifact@v7 + with: + # One artifact carries both values to save an upload step. `;` separates the + # `key=value` pairs; `parseWorkflowRunArtifactInputs` in context.ts reads each pair. + name: "head-sha=${{ steps.resolve_target_pr.outputs.target_head_sha }};issue-number=${{ steps.resolve_target_pr.outputs.target_pr_number }}" + path: "${{ runner.temp }}/arm-semantic-review/empty.txt" + if-no-files-found: error + overwrite: true + - name: Set ARM semantic review pending + if: steps.check_membership.outputs.is_team_member == 'true' + uses: actions/github-script@v9 + env: + TARGET_HEAD_SHA: ${{ steps.resolve_target_pr.outputs.target_head_sha }} + with: + script: | + const runAttempt = process.env.GITHUB_RUN_ATTEMPT ?? ""; + if (!/^[1-9]\d*$/.test(runAttempt)) { + throw new Error(`Invalid workflow run attempt: ${JSON.stringify(runAttempt)}`); + } + await github.rest.repos.createCommitStatus({ + ...context.repo, + sha: process.env.TARGET_HEAD_SHA, + state: "pending", + context: "ARM Semantic Review", + description: "ARM API semantic review is pending", + target_url: + `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/` + + `${context.runId}/attempts/${runAttempt}`, + }); # Only users with write access (or above) may trigger the workflow. With # `forks: ["*"]` enabled above, this `roles` gate is the primary guard that # keeps externally-authored fork PRs from auto-triggering a review and blocks @@ -80,7 +127,42 @@ on: jobs: pre-activation: outputs: + run_attempt: ${{ steps.resolve_target_pr.outputs.run_attempt }} + target_head_sha: ${{ steps.resolve_target_pr.outputs.target_head_sha }} target_pr_number: ${{ steps.resolve_target_pr.outputs.target_pr_number }} + + # Pre-activation posts a Pending `ARM Semantic Review` status, but the record job + # only runs when the agent emitted a semantic item and threat detection passed. + # The conclusion job runs after every other job regardless of their results, so it + # resolves a Pending status that this exact run still owns. That keeps a failed, + # cancelled, or noop run from leaving the PR waiting forever. The step reads no + # agent output; it takes the head SHA from the trusted correlation artifact. + # It is skipped when the record job succeeded, since that job already published. + # Pre-steps run before the built-in conclusion steps, so `continue-on-error` + # keeps a failure here from blocking gh-aw's own failure reporting. + conclusion: + permissions: + contents: read + statuses: write + pre-steps: + - uses: actions/checkout@v7 + if: needs.record_arm_semantic_review.result != 'success' + continue-on-error: true + with: + persist-credentials: false + sparse-checkout: | + .github + - name: Resolve unpublished ARM semantic review status + if: needs.record_arm_semantic_review.result != 'success' + continue-on-error: true + uses: actions/github-script@v9.0.0 + with: + # Retry transient API errors so a one-off failure does not leave the status Pending. + retries: 3 + script: | + const { finalizeUnpublishedArmSemanticReview } = + await import('${{ github.workspace }}/.github/workflows/src/arm-auto-signoff/arm-semantic-review-workflow.ts'); + return await finalizeUnpublishedArmSemanticReview({ github, context, core }); # Gate at the trigger level so the expensive agent job never starts for # ineligible events. Label / draft / comment gating that used to live in a # custom github-script step is expressed here declaratively; the remaining @@ -91,6 +173,7 @@ if: > (github.event_name == 'pull_request_target' && (github.event.action == 'opened' || github.event.action == 'synchronize' || + github.event.action == 'reopened' || github.event.action == 'ready_for_review') && github.event.pull_request.draft == false && contains(github.event.pull_request.labels.*.name, 'WaitForARMFeedback')) || @@ -209,6 +292,60 @@ safe-outputs: remove-labels: max: 3 target: "${{ github.event.pull_request.number || github.event.issue.number || github.event.inputs.pr_number }}" + jobs: + record-arm-semantic-review: + description: "Validate and publish the ARM semantic review result for auto-signoff" + runs-on: ubuntu-slim + if: needs.detection.outputs.detection_success == 'true' + permissions: + actions: read + contents: read + pull-requests: read + statuses: write + inputs: + scope: + description: "Whether the full PR or only a scoped subset was reviewed" + required: true + type: choice + options: ["full", "scoped"] + completeness: + description: "Whether the intended review completed reliably" + required: true + type: choice + options: ["complete", "incomplete"] + incomplete_reason: + description: "Why the review is incomplete, or none when complete" + required: true + type: choice + options: + [ + "none", + "critic-unavailable", + "required-evidence-unavailable", + "tool-failure", + "stale-sha", + "discussion-data-unavailable", + ] + blocking_count: + description: "Number of verified, currently applicable Blocking findings after reconciliation" + required: true + type: string + steps: + - uses: actions/checkout@v7 + with: + persist-credentials: false + sparse-checkout: | + .github + - name: Publish ARM semantic review status + uses: actions/github-script@v9.0.0 + with: + # Retry transient API errors so a one-off failure does not leave the status Pending. + retries: 3 + script: | + const { default: publishArmSemanticReviewStatus } = + await import('${{ github.workspace }}/.github/workflows/src/arm-auto-signoff/arm-semantic-review-workflow.ts'); + return await publishArmSemanticReviewStatus({ github, context, core }); + output: "ARM semantic review status published" noop: # Threat detection uses the same pinned model as the primary review so all ARM # API Reviewer experiences move together. Keep both literals identical; see @@ -238,7 +375,9 @@ reconciliation throughout. - Repository: `${{ github.repository }}` - Trigger event: `${{ github.event_name }}` +- **Authoritative workflow attempt:** `${{ needs.pre_activation.outputs.run_attempt }}` - **Authoritative target pull request:** `#${{ needs.pre_activation.outputs.target_pr_number }}` +- **Authoritative target head SHA:** `${{ needs.pre_activation.outputs.target_head_sha }}` The target above was parsed, validated as a pull request, and canonicalized via the GitHub Pull Requests API before agent execution. Use it for every GitHub @@ -291,11 +430,12 @@ Exactly **three** things differ, by design: reviewer has selected in VS Code, and pinning one would simply fail for anyone without access to it. Expect wording and emphasis to vary between the two paths. The rules above are what keep the substance the same. -- **Human queue advancement after a clean review.** This unattended workflow is - advisory and leaves `WaitForARMFeedback` unchanged when it publishes no - Blocking finding. A human-approved interactive review removes - `WaitForARMFeedback` because a human reviewer explicitly completed the review. - Both paths remove it when they add `ARMChangesRequested`. +- **Human queue advancement after a clean review.** This unattended workflow + leaves `WaitForARMFeedback` unchanged after a clean review, but removes a + stale `ARMChangesRequested` label when the full review completes reliably. + A human-approved interactive review removes `WaitForARMFeedback` because a + human reviewer explicitly completed the review. Both paths remove + `WaitForARMFeedback` when they add `ARMChangesRequested`. **Repository coverage.** This workflow exists in **both** repositories, `Azure/azure-rest-api-specs` and `Azure/azure-rest-api-specs-pr`, and the two @@ -366,8 +506,8 @@ already guaranteed, before this agent starts, that: or is the explicitly allowlisted `github-actions[bot]` repository automation. This replaces any manual collaborator check. Do **not** re-verify permissions. -- The event is eligible: an automated `opened` / `synchronize` run only reaches - the agent when the PR is not a draft and already carries the +- The event is eligible: an automated `opened` / `synchronize` / `reopened` run + only reaches the agent when the PR is not a draft and already carries the `WaitForARMFeedback` label; `ready_for_review` follows the same label gate; a `labeled` run only fires when that exact label is added to a non-draft PR; an `issue_comment` run only fires for a PR comment whose body is exactly @@ -391,7 +531,8 @@ read-only `github` toolset. If any check fails, act as directed and stop. `pull_request_read(method: "get")`, call `report_incomplete` and stop. Do not call `noop` for target-resolution or infrastructure failures. Pin the returned `head.sha` immediately and use that session SHA for all subsequent PR-head - file reads. + file reads. The pinned SHA must equal the authoritative target head SHA above. + If it does not, call `report_incomplete` and stop. 2. **`skip-arm-review` label** β€” call `pull_request_read(method: "get")` and inspect the labels. If the PR carries `skip-arm-review`, call `noop` and stop (opt-out). From the same response, capture the exact label names matching @@ -462,6 +603,25 @@ read-only `github` toolset. If any check fails, act as directed and stop. Only when checks 1–3 pass should you proceed to the Review Workflow below. Check 4 sets the review scope; it never stops the review. +Track two values for the final semantic result: + +- `scope` is `full` unless the size cap or file-list truncation limits coverage; + otherwise it is `scoped`. +- `completeness` is `complete` only when the intended review and Critic + verification finish normally. Otherwise it is `incomplete`. +- `incomplete_reason` is `none` when completeness is `complete`. When incomplete, + use the most specific reason: `critic-unavailable` after all Critic retries fail; + `discussion-data-unavailable` when comments, reviews, or threads cannot be + fetched completely; `stale-sha` when the pinned review target moved or became + unreachable; `tool-failure` when a required tool operation failed; otherwise + `required-evidence-unavailable`. + +Before any `report_incomplete` stop after the target PR and head SHA are known, +also call `record_arm_semantic_review` with the current scope, +`completeness: incomplete`, the most specific `incomplete_reason` above, and the +number of verified Blocking findings known to remain applicable at that point. +Use `"0"` if the failure occurs before finding reconciliation. + ## Review Workflow ### Step 1: Fetch PR Metadata and Changed Files @@ -709,7 +869,8 @@ considered-and-declined candidates, graph status, and iteration number. context the PR happened to go through; see [Review context parity](#review-context-parity). Because nothing verified these findings, the Step 7 label rules withhold `ARMChangesRequested` for such - a run. + a run. Record `completeness: incomplete` with + `incomplete_reason: critic-unavailable`. - If the Critic reports that the session SHA moved or is unreachable, do not post findings or mutate threads. Restart once against the new head SHA; if it moves again, call `noop` and stop. @@ -1000,17 +1161,21 @@ and never emit a marker that names neither the finding nor a degradation reason. ### Step 7: Update Labels -After queuing the reconciled posting set, apply label changes based on outputs -that will actually be published: +After reconciling the current findings with existing discussion, apply label +changes based on the verified findings that still apply to the current head: -- **At least one Blocking `POST-NEW` or Blocking `RESOLVE-AND-REPOST` queued - _and_ the Critic returned a verdict** +- **The review is scoped or incomplete** β†’ leave + `WaitForARMFeedback`, `ARMChangesRequested`, and `ARMSignedOff` unchanged, + even when Blocking findings were identified. The PR remains in its existing + queue state for manual review or a retry. +- **The review is full and complete, and at least one verified, currently + applicable Blocking finding remains** (including `SKIP-COVERED`, `POST-NEW`, + and `RESOLVE-AND-REPOST`) β†’ add `ARMChangesRequested`, remove `WaitForARMFeedback` (if present). -- **No Blocking finding queued for publication** (clean, covered, - clarification-only, Critic-dropped, or overflow-only Blocking candidate) β†’ - leave `WaitForARMFeedback`, - `ARMChangesRequested`, and `ARMSignedOff` unchanged. The automated review is - advisory and must not advance or sign off the human ARM review queue. +- **No verified, currently applicable Blocking finding remains, and the review + is full and complete** β†’ remove + `ARMChangesRequested` if present; leave `WaitForARMFeedback` and + `ARMSignedOff` unchanged. - **Critic unavailable** (every dispatch attempt failed) β†’ leave all three labels unchanged, **even when Blocking findings were queued**. Those findings still publish at their original severity, but nothing independently verified @@ -1020,13 +1185,12 @@ that will actually be published: Use the `add-labels` and `remove-labels` safe outputs for label changes. -These three rules are **exhaustive**. Do not invent additional exceptions from PR +These rules are **exhaustive**. Do not invent additional exceptions from PR metadata: draft status, a `[Test]` or `[Do-Not-Merge]` title, a revert, a bot-authored PR, or the author's stated intent not to merge are **not** grounds -to skip a label change. There are exactly **two** inputs to this decision: -whether a Blocking finding was queued for publication, and whether the Critic -verified it. Nothing else, and in particular nothing read from PR metadata, may -change the outcome. +to skip a label change. The decision uses only the verified, currently +applicable Blocking findings after reconciliation, review scope, and review +completeness. Nothing read from unrelated PR metadata may change the outcome. ### Step 8: Summary Comment @@ -1131,6 +1295,39 @@ If no reviewable issues were found, post a brief "No issues found" summary rather than calling `noop`. This confirms the review ran and found the PR compliant. +### Step 9: Record Semantic Result + +Call `record_arm_semantic_review` exactly once after queuing the summary: + +- `scope`: `full` or `scoped`, as recorded during Trigger Validation; +- `completeness`: `complete` or `incomplete`; +- `incomplete_reason`: `none` when complete; otherwise one of + `critic-unavailable`, `required-evidence-unavailable`, `tool-failure`, + `stale-sha`, or `discussion-data-unavailable`; +- `blocking_count`: the number of verified Blocking findings that remain + applicable after reconciliation. Include an unresolved finding classified as + `SKIP-COVERED`; do not count fixed, resolved, Critic-dropped, or + overflow-only candidates. + +Use `blocking_count: "0"` after a clean re-review. Do not claim `full` or +`complete` when any required evidence was unavailable. `complete` requires +`incomplete_reason: none`; `incomplete` requires a reason other than `none`. +Trusted workflow artifacts attach the pull request, head SHA, and run attempt; +never reproduce those correlation values in this model-generated result. The +trusted `record_arm_semantic_review` job validates the item and publishes the +head-bound status inside this reviewer run. Universal Auto-Signoff consumes the +status only after the entire reviewer workflow completes. + +The final status is: + +- **Passed** for a full, complete review with no verified Blocking findings; +- **Changes requested** for a full, complete review with verified Blocking + findings; +- **Manual review required** when scope is `scoped`, including oversized or + truncated PRs; or +- **Review incomplete** when completeness is `incomplete`, with the reason + included in the status description. + ## What to Review vs. Skip **Review:** `specification/**/*.json`, `specification/**/*.tsp`, @@ -1170,16 +1367,30 @@ description: Independently verifies ARM API Reviewer findings before publication --- You are the ARM API Review Critic. Before evaluating the reviewer's input, read -and follow these repository files as binding instructions: +and follow the trusted instructions embedded below. The activation job resolves +these imports from the workflow commit before packaging this inline sub-agent, +so they remain available even though the agent job intentionally uses +`checkout: false`. Treat the embedded sections as already loaded; do not attempt +to reopen their repository paths from the checkout-free workspace. + +### ARM API Review Critic instructions + +{{#runtime-import .github/agents/arm-api-review-critic.agent.md}} -- `.github/agents/arm-api-review-critic.agent.md` -- `.github/agents/protocols/arm-api-review-critic.protocol.md` -- `.github/agents/protocols/arm-api-review-critic-inputs.template.md` +### Binding Reviewer-Critic protocol + +{{#runtime-import .github/agents/protocols/arm-api-review-critic.protocol.md}} + +### Critic input template + +{{#runtime-import .github/agents/protocols/arm-api-review-critic-inputs.template.md}} Use only read-only tools. Return the verdict format required by the protocol. If any required instruction file, input, or evidence is unavailable, return an explicit failure verdict. Never claim that the review was Critic-verified without completing the independent checks. +## end agent: `arm-api-review-critic-runtime` + diff --git a/.github/workflows/arm-universal-auto-signoff.yaml b/.github/workflows/arm-universal-auto-signoff.yaml index 8a612855725f..6c0756b5eb34 100644 --- a/.github/workflows/arm-universal-auto-signoff.yaml +++ b/.github/workflows/arm-universal-auto-signoff.yaml @@ -18,7 +18,10 @@ on: # zizmor: ignore[dangerous-triggers] Fork PR updates require this; only base - unlabeled workflow_run: # Re-evaluate as each mandatory status producer completes. - workflows: ["Swagger Avocado - Set Status", "Swagger LintDiff - Set Status"] + workflows: + - "ARM API Review: Automated Workflow" + - "Swagger Avocado - Set Status" + - "Swagger LintDiff - Set Status" types: [completed] permissions: @@ -91,6 +94,13 @@ jobs: name: "ARMAutoSignedOff-Test" value: "${{ fromJson(steps.get-label-action.outputs.result).labelActions['ARMAutoSignedOff-Test'] == 'add' }}" + - if: fromJson(steps.get-label-action.outputs.result).labelActions['ARMManualSignoffRequired'] == 'add' + name: Upload artifact for ARMManualSignoffRequired label + uses: ./.github/actions/add-label-artifact + with: + name: "ARMManualSignoffRequired" + value: "true" + # The label updater uses these trusted artifacts to associate this workflow run with its PR. - if: | always() && diff --git a/.github/workflows/src/arm-auto-signoff/README.md b/.github/workflows/src/arm-auto-signoff/README.md new file mode 100644 index 000000000000..aa1ee9b7d1b3 --- /dev/null +++ b/.github/workflows/src/arm-auto-signoff/README.md @@ -0,0 +1,57 @@ +# ARM Semantic Review and Universal Auto-Signoff + +The ARM API Reviewer is an AI agent, so auto-signoff does not trust it directly. Its verdict is +turned into one commit status, `ARM Semantic Review`, on the reviewed SHA. Universal Auto-Signoff +reads that status alongside `Swagger LintDiff` and `Swagger Avocado`. + +Universal runs in parallel with the legacy ARM Auto SignOff workflows, which remain the +production path. It only manages the pilot label `ARMAutoSignedOff-Test` and never changes +`ARMSignedOff`. + +## Flow + +```text +arm-api-review (reviewer) + pre_activation post status PENDING, upload one head-sha=;issue-number= artifact + agent review the PR, request record_arm_semantic_review + record_arm_semantic_review validate the result, post SUCCESS / FAILURE / ERROR + conclusion if no result was posted, turn this run's PENDING into ERROR + +arm-universal-auto-signoff re-check the PR is open at the evaluated head, read the newest status + per context on that SHA + labels, decide the label action +update-labels add or remove the label +``` + +The model proposes; trusted code validates and publishes. Any doubt ends as "not Passed". + +## Signoff policy + +```text +ARM Semantic Review passed for the current PR head +AND Swagger LintDiff and Swagger Avocado passed for the current PR head +AND labels and approvals allow signoff += add ARMAutoSignedOff-Test +``` + +## Status contract + +| State | Description starts with | Adds `ARMManualSignoffRequired` | +| --------- | -------------------------- | -------------------------------- | +| `pending` | `ARM API semantic review` | No | +| `success` | `Passed:` | No | +| `failure` | `Changes requested:` | No | +| `error` | `Manual review required: ` | Yes (scoped or oversized PR) | +| `error` | `Review incomplete: ` | No (transient, a re-run can fix) | + +Only `success` can lead to signoff. A status is bound to its SHA, so a result for a commit that is +no longer the PR head is never read for the new head. + +## Files + +| File | Role | +| ------------------------------------------------------------------------ | ------------------------------------------------------------------- | +| [arm-semantic-review.ts](./arm-semantic-review.ts) | Pure policy: validate the result, map it to a status, read statuses | +| [arm-semantic-review-workflow.ts](./arm-semantic-review-workflow.ts) | Publish the status; resolve an unpublished one | +| [arm-universal-auto-signoff.ts](./arm-universal-auto-signoff.ts) | Read statuses and labels, decide label actions | +| [arm-api-review.md](../../arm-api-review.md) | Reviewer workflow source (the `.lock.yml` is generated from it) | +| [arm-universal-auto-signoff.yaml](../../arm-universal-auto-signoff.yaml) | Universal workflow | diff --git a/.github/workflows/src/arm-auto-signoff/arm-semantic-review-workflow.ts b/.github/workflows/src/arm-auto-signoff/arm-semantic-review-workflow.ts new file mode 100644 index 000000000000..0175d1ef7276 --- /dev/null +++ b/.github/workflows/src/arm-auto-signoff/arm-semantic-review-workflow.ts @@ -0,0 +1,290 @@ +import { readFile } from "node:fs/promises"; +import { CommitStatusState, PER_PAGE_MAX } from "../../../shared/src/github.ts"; +import { getWorkflowRunArtifactInputs } from "../context.ts"; +import type { GitHubScriptArgs } from "../github.ts"; +import { + addSpecificationChanges, + ARM_SEMANTIC_REVIEW_STATUS, + checkPullRequestSize, + evaluateSemanticReview, + getLatestSemanticReviewStatus, + MANUAL_REVIEW_DESCRIPTION_PREFIX, + parseSemanticReviewResult, + REVIEW_INCOMPLETE_DESCRIPTION_PREFIX, + SemanticReviewCompletion, + SemanticReviewScope, + SPECIFICATION_OVER_LIMITS_REASON, + type CommitStatus, + type SpecificationTotals, + type EvaluatedSemanticReview, +} from "./arm-semantic-review.ts"; + +/* + * I/O for the `ARM Semantic Review` commit status. These functions run in trusted reviewer jobs + * (`record_arm_semantic_review` and the `conclusion` job) and treat the model's output as + * untrusted data. The PR number, head SHA, and run attempt always come from the run's own + * artifacts and environment, never from the model. + * + * GitHub caps commit status descriptions at 140 characters. Every description published here is a + * short fixed string, so none can exceed it. + */ + +/** Returned when the run has no trusted PR/SHA correlation and no status was published. */ +const EMPTY_RESULT = { + headSha: "", + issueNumber: 0, + statusPublished: false, +}; + +/** + * Builds the `target_url` of this run and attempt. It is published on every status this run + * writes and is how a later reader tells whether a status belongs to this exact run, which is what + * lets the finalizer avoid overwriting a newer run. + * + * Reads the attempt from `GITHUB_RUN_ATTEMPT`, which GitHub Actions always sets. + */ +function getRunStatusUrl( + { serverUrl, runId }: GitHubScriptArgs["context"], + owner: string, + repo: string, +) { + return `${serverUrl}/${owner}/${repo}/actions/runs/${runId}/attempts/${process.env.GITHUB_RUN_ATTEMPT}`; +} + +/** + * Independently verifies that the PR is small enough for the reviewer to have covered all of it, + * because the model's own `scope` is untrusted. The rules live in `arm-semantic-review.ts`; this + * does the API calls. It costs one `pulls.get` call, which is all most PRs need. Only when the + * PR's totals are over the limits does it list the files, page by page, and it stops as soon as + * the `specification/` changes are over the limits. + * + * @returns A short fixed reason when a human must review, or `undefined` when the PR is within + * the limits. The reason is published in the status, so it must never include model output. + */ +async function getOversizeReason( + github: GitHubScriptArgs["github"], + core: GitHubScriptArgs["core"], + owner: string, + repo: string, + issueNumber: number, +): Promise { + const { data: pullRequest } = await github.rest.pulls.get({ + owner, + repo, + pull_number: issueNumber, + }); + core.info( + `PR #${issueNumber} size: ${pullRequest.changed_files} files, ` + + `+${pullRequest.additions} -${pullRequest.deletions} lines`, + ); + + const size = checkPullRequestSize(pullRequest); + if (size.outcome === "manual-review") { + core.info(`Cannot be proven within the limits: ${size.reason}`); + return size.reason; + } + if (size.outcome === "within") { + core.info( + "Within the limits overall, so the specification/ subset is too: no file list needed", + ); + return undefined; + } + + core.info("Over the limits overall: counting only the specification/ changes"); + const totals: SpecificationTotals = { files: 0, lines: 0 }; + let pages = 0; + let overLimit = false; + await github.paginate( + github.rest.pulls.listFiles, + { owner, repo, pull_number: issueNumber, per_page: PER_PAGE_MAX }, + (response, done) => { + pages++; + overLimit = addSpecificationChanges(totals, response.data); + core.info( + ` page ${pages}: ${totals.files} specification/ files, ${totals.lines} lines so far`, + ); + if (overLimit) { + core.info(" already over the limits, so the remaining pages are not needed"); + done(); + } + return []; + }, + ); + + if (overLimit) { + return SPECIFICATION_OVER_LIMITS_REASON; + } + core.info( + `specification/ changes are within the limits: ${totals.files} files, ${totals.lines} lines`, + ); + return undefined; +} +/** + * Writes one `ARM Semantic Review` commit status on the reviewed SHA. + * + * @param targetUrl The `target_url` identifying this run and attempt. + * @param status The state and description to publish. + */ +async function publishStatus( + { github, core }: Pick, + owner: string, + repo: string, + headSha: string, + targetUrl: string, + status: EvaluatedSemanticReview, +): Promise { + core.info(`${ARM_SEMANTIC_REVIEW_STATUS}: ${status.state} - ${status.description}`); + await github.rest.repos.createCommitStatus({ + owner, + repo, + sha: headSha, + state: status.state, + context: ARM_SEMANTIC_REVIEW_STATUS, + description: status.description, + target_url: targetUrl, + }); +} + +/** + * Validates one trusted ARM reviewer safe output and publishes its head-bound status. + * This runs inside the reviewer workflow, before workflow_run consumers are triggered. + * + * Steps: read the trusted `head-sha` and `issue-number` artifacts; parse and validate the model's + * result; independently check the PR's size; map the evidence to a status; publish it. Any failure + * while validating or reading the PR is published as `Review incomplete` with a fixed description, + * and the details go to the run log. + * + * The status is bound to the reviewed SHA, so a result for a commit that is no longer the PR head + * is harmless: Universal Auto-Signoff reads only the current head's statuses. + * + * Requires `GH_AW_AGENT_OUTPUT` (the path to `agent_output.json`) in the environment. + * + * @returns The correlation used and whether a status was published. Nothing is published when the + * run has no trusted PR/SHA correlation. + * @throws If the trusted artifacts conflict with each other. + */ +export default async function publishArmSemanticReviewStatus({ + github, + context, + core, +}: GitHubScriptArgs): Promise<{ + headSha: string; + issueNumber: number; + statusPublished: boolean; +}> { + const { owner, repo } = context.repo; + const { headSha, issueNumber } = await getWorkflowRunArtifactInputs({ + github, + core, + owner, + repo, + runId: context.runId, + }); + if (!headSha || !(issueNumber > 0)) { + core.info("The reviewer run has no trusted PR/SHA correlation; status is unchanged"); + return EMPTY_RESULT; + } + core.info(`Publishing for PR #${issueNumber} at ${headSha}`); + + let status: EvaluatedSemanticReview; + try { + const agentOutput = JSON.parse( + await readFile(process.env.GH_AW_AGENT_OUTPUT ?? "", "utf8"), + ) as unknown; + const result = parseSemanticReviewResult(agentOutput); + core.info( + `Reviewer reported: scope=${result.reviewScope}, completeness=${result.completion}, ` + + `incomplete reason=${result.incompleteReason}, Blocking findings=${result.blockingCount}`, + ); + + status = evaluateSemanticReview(result); + if ( + result.completion === SemanticReviewCompletion.Complete && + result.reviewScope === SemanticReviewScope.Full + ) { + // Only a claim of a full, complete review can authorize signoff, so only that claim needs + // verifying. A scoped or incomplete review (the oversized PRs, where the check is the most + // expensive) is already withheld. + const reason = await getOversizeReason(github, core, owner, repo, issueNumber); + if (reason) { + core.info(`The reviewer claimed a full review, but ${reason}: manual review required`); + status = { + state: CommitStatusState.ERROR, + description: `${MANUAL_REVIEW_DESCRIPTION_PREFIX}${reason}`, + }; + } + } else { + core.info("The reviewer did not claim a full, complete review: skipping the size check"); + } + } catch (error) { + // Any failure here, including a transient GitHub API error, is reported as incomplete rather + // than as a manual-review hold, so a re-run can still produce a passing result. The details go + // to the run log: raw error text can include runner paths and must not reach a public status. + core.warning(error instanceof Error ? error.message : String(error)); + status = { + state: CommitStatusState.ERROR, + description: `${REVIEW_INCOMPLETE_DESCRIPTION_PREFIX}result could not be validated`, + }; + } + + await publishStatus( + { github, core }, + owner, + repo, + headSha, + getRunStatusUrl(context, owner, repo), + status, + ); + return { headSha, issueNumber, statusPublished: true }; +} + +/** + * Resolves this run's Pending status when the record job never published a result: the agent + * failed, took a noop path, was cancelled, or threat detection did not pass. It reads no agent + * output and only acts when this exact run and attempt still owns the newest status, so a newer + * run's status is never overwritten. The resulting status is Review incomplete, never a + * manual-review hold, because these outcomes are not a property of the PR. + * + * @returns Whether a status was published. + * @throws If the trusted artifacts conflict with each other. + */ +export async function finalizeUnpublishedArmSemanticReview({ + github, + context, + core, +}: GitHubScriptArgs): Promise<{ statusPublished: boolean }> { + const { owner, repo } = context.repo; + const { headSha } = await getWorkflowRunArtifactInputs({ + github, + core, + owner, + repo, + runId: context.runId, + }); + if (!headSha) { + core.info("The reviewer run has no trusted head SHA; status is unchanged"); + return { statusPublished: false }; + } + + const targetUrl = getRunStatusUrl(context, owner, repo); + core.info(`Checking whether ${targetUrl} left a Pending status on ${headSha}`); + const statuses: CommitStatus[] = await github.paginate( + github.rest.repos.listCommitStatusesForRef, + { owner, repo, ref: headSha, per_page: PER_PAGE_MAX }, + ); + const latestStatus = getLatestSemanticReviewStatus(statuses); + core.info( + `Latest ${ARM_SEMANTIC_REVIEW_STATUS}: ${latestStatus?.state ?? "none"} ` + + `(${latestStatus?.target_url ?? "no url"})`, + ); + if (latestStatus?.state !== CommitStatusState.PENDING || latestStatus.target_url !== targetUrl) { + core.info("This run does not own a Pending status; status is unchanged"); + return { statusPublished: false }; + } + + await publishStatus({ github, core }, owner, repo, headSha, targetUrl, { + state: CommitStatusState.ERROR, + description: `${REVIEW_INCOMPLETE_DESCRIPTION_PREFIX}reviewer did not publish a result`, + }); + return { statusPublished: true }; +} diff --git a/.github/workflows/src/arm-auto-signoff/arm-semantic-review.ts b/.github/workflows/src/arm-auto-signoff/arm-semantic-review.ts new file mode 100644 index 000000000000..21ef7e7d675a --- /dev/null +++ b/.github/workflows/src/arm-auto-signoff/arm-semantic-review.ts @@ -0,0 +1,346 @@ +import { CommitStatusState } from "../../../shared/src/github.ts"; +import { byDate, invert } from "../../../shared/src/sort.ts"; + +/* + * Pure policy for the `ARM Semantic Review` commit status: how the ARM API Reviewer's structured + * result is validated, how it maps to a status, and how the status is read back by consumers. + * This file makes no GitHub API calls; `arm-semantic-review-workflow.ts` does the I/O. + */ + +/** The commit status context that carries the semantic review result for a SHA. */ +export const ARM_SEMANTIC_REVIEW_STATUS = "ARM Semantic Review"; + +/* + * Both outcomes below are published as a commit status in the "error" state. The description + * prefix is the only thing that tells them apart: a manual-review hold is deterministic (scope or + * size limits) and adds a sticky human label, while an incomplete review is a transient failure + * that a re-run can fix, so it never adds that label. + */ + +/** Description prefix of an `error` status that requires a human reviewer (scoped or oversized). */ +export const MANUAL_REVIEW_DESCRIPTION_PREFIX = "Manual review required: "; + +/** Description prefix of an `error` status for a review that did not finish or could not be read. */ +export const REVIEW_INCOMPLETE_DESCRIPTION_PREFIX = "Review incomplete: "; + +/** + * What a consumer concludes from the latest `ARM Semantic Review` status. Only `Passed` can + * contribute to auto-signoff. + */ +export const SemanticReviewOutcome = Object.freeze({ + Passed: "passed", + ChangesRequested: "changes-requested", + ManualReviewRequired: "manual-review-required", + ReviewIncomplete: "review-incomplete", + Pending: "pending", +}); +export type SemanticReviewOutcome = + (typeof SemanticReviewOutcome)[keyof typeof SemanticReviewOutcome]; + +/** How much of the PR the reviewer covered: the whole PR, or a size-limited subset. */ +export const SemanticReviewScope = Object.freeze({ + Full: "full", + Scoped: "scoped", +}); +export type SemanticReviewScope = (typeof SemanticReviewScope)[keyof typeof SemanticReviewScope]; + +/** Whether the reviewer and the Critic finished normally. */ +export const SemanticReviewCompletion = Object.freeze({ + Complete: "complete", + Incomplete: "incomplete", +}); +export type SemanticReviewCompletion = + (typeof SemanticReviewCompletion)[keyof typeof SemanticReviewCompletion]; + +/** + * Why a review is incomplete, or `none` when it is complete. The set is closed so that model + * output can never place free text in a public status description. + */ +export const SemanticReviewIncompleteReason = Object.freeze({ + None: "none", + CriticUnavailable: "critic-unavailable", + RequiredEvidenceUnavailable: "required-evidence-unavailable", + ToolFailure: "tool-failure", + StaleSha: "stale-sha", + DiscussionDataUnavailable: "discussion-data-unavailable", +}); +export type SemanticReviewIncompleteReason = + (typeof SemanticReviewIncompleteReason)[keyof typeof SemanticReviewIncompleteReason]; + +/** The validated form of the reviewer's `record_arm_semantic_review` item. */ +export type SemanticReviewResult = { + /** Verified Blocking findings still applicable after reconciliation. */ + blockingCount: number; + reviewScope: SemanticReviewScope; + completion: SemanticReviewCompletion; + incompleteReason: SemanticReviewIncompleteReason; +}; + +/** The fields of a commit status (`repos.listCommitStatusesForRef`) that this feature reads. */ +export type CommitStatus = { + context: string; + state: string; + description?: string | null; + target_url?: string | null; + updated_at: string; +}; + +/** The commit status state and description to publish for a validated result. */ +export type EvaluatedSemanticReview = { + state: CommitStatusState; + description: string; +}; + +/** Narrows to a plain object, excluding `null` and arrays. */ +function isRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +/** + * Extracts and validates exactly one semantic result from gh-aw's agent output + * (`agent_output.json`). The PR, head SHA, and run attempt are never read from the model's + * output; the caller takes them from trusted artifacts. + * + * Every field the model supplies is checked against a closed set, so an unknown value can never + * fall through toward a passing status. + * + * @param agentOutput The parsed contents of `agent_output.json`. + * @returns The validated result. + * @throws If the output has no `items` array, does not contain exactly one + * `record_arm_semantic_review` item, has a malformed or unknown field, or reports a complete + * review that carries an incomplete reason. + */ +export function parseSemanticReviewResult(agentOutput: unknown): SemanticReviewResult { + if (!isRecord(agentOutput) || !Array.isArray(agentOutput.items)) { + throw new Error("ARM API Reviewer output is missing its items array"); + } + + const results = (agentOutput.items as unknown[]).filter( + (item): item is Record => + isRecord(item) && item.type === "record_arm_semantic_review", + ); + if (results.length !== 1) { + throw new Error(`Expected one ARM semantic review result, found ${results.length}`); + } + + const result = results[0]; + if (typeof result.blocking_count !== "string" || !/^(0|[1-9]\d*)$/.test(result.blocking_count)) { + throw new Error( + `Invalid ARM semantic review Blocking count: '${String(result.blocking_count)}'`, + ); + } + if (result.scope !== SemanticReviewScope.Full && result.scope !== SemanticReviewScope.Scoped) { + throw new Error(`Invalid ARM semantic review scope: '${String(result.scope)}'`); + } + if ( + result.completeness !== SemanticReviewCompletion.Complete && + result.completeness !== SemanticReviewCompletion.Incomplete + ) { + throw new Error(`Invalid ARM semantic review completion: '${String(result.completeness)}'`); + } + if ( + result.incomplete_reason !== SemanticReviewIncompleteReason.None && + result.incomplete_reason !== SemanticReviewIncompleteReason.CriticUnavailable && + result.incomplete_reason !== SemanticReviewIncompleteReason.RequiredEvidenceUnavailable && + result.incomplete_reason !== SemanticReviewIncompleteReason.ToolFailure && + result.incomplete_reason !== SemanticReviewIncompleteReason.StaleSha && + result.incomplete_reason !== SemanticReviewIncompleteReason.DiscussionDataUnavailable + ) { + throw new Error( + `Invalid ARM semantic review incomplete reason: '${String(result.incomplete_reason)}'`, + ); + } + // A complete review must not carry a failure reason, or it could be read as a pass. + if ( + result.completeness === SemanticReviewCompletion.Complete && + result.incomplete_reason !== SemanticReviewIncompleteReason.None + ) { + throw new Error( + `ARM semantic review completion 'complete' is inconsistent with ` + + `incomplete reason '${result.incomplete_reason}'`, + ); + } + + return { + blockingCount: Number(result.blocking_count), + reviewScope: result.scope, + completion: result.completeness, + incompleteReason: result.incomplete_reason, + }; +} + +/** + * Maps a validated result to the commit status to publish. The first matching rule wins: + * incomplete, then scoped, then Blocking findings, then Passed. `success` is only reachable for + * a full, complete review with no Blocking findings. + * + * @param result A result returned by `parseSemanticReviewResult`. + * @returns The state and description to publish. + */ +export function evaluateSemanticReview(result: SemanticReviewResult): EvaluatedSemanticReview { + // Completeness and scope are checked first: Changes requested applies only to + // full, complete reviews. An incomplete or scoped review never authorizes signoff + // regardless of the blocking count. + if (result.completion === SemanticReviewCompletion.Incomplete) { + return { + state: CommitStatusState.ERROR, + description: `${REVIEW_INCOMPLETE_DESCRIPTION_PREFIX}${result.incompleteReason}`, + }; + } + if (result.reviewScope === SemanticReviewScope.Scoped) { + return { + state: CommitStatusState.ERROR, + description: `${MANUAL_REVIEW_DESCRIPTION_PREFIX}scoped review requires manual signoff`, + }; + } + if (result.blockingCount > 0) { + return { + state: CommitStatusState.FAILURE, + description: `Changes requested: ${result.blockingCount} Blocking finding(s)`, + }; + } + return { + state: CommitStatusState.SUCCESS, + description: "Passed: full review completed with no Blocking findings", + }; +} + +/* + * Automated-review size limits. They mirror the cap in the reviewer prompt (`arm-api-review.md`, + * Trigger Validation); keep both in step. A PR above either limit is only partly reviewed, so a + * human must review it. The model's own `scope` is untrusted, so these are checked independently. + */ +const MAX_SPECIFICATION_FILES = 50; +const MAX_SPECIFICATION_LINES = 5_000; + +// GitHub's file-listing API returns at most this many files for a PR and does not say when it cut +// the list off, so a PR this large cannot be proven fully covered. +const MAX_LISTED_FILES = 3_000; + +/** The aggregate counters on a pull request object (`pulls.get`). */ +export type PullRequestCounts = { + changed_files: number; + additions: number; + deletions: number; +}; + +/** One entry of the pull request file list (`pulls.listFiles`). */ +export type ChangedFile = { + filename: string; + /** Set only for a renamed or moved file: the path it had before the PR. */ + previous_filename?: string; + additions: number; + deletions: number; +}; + +/** Running totals of the `specification/` changes seen so far. */ +export type SpecificationTotals = { files: number; lines: number }; + +/** The status reason for a PR whose `specification/` changes exceed the limits. */ +export const SPECIFICATION_OVER_LIMITS_REASON = `specification/ changes exceed ${MAX_SPECIFICATION_FILES} files or ${MAX_SPECIFICATION_LINES} lines`; + +/** What the PR's aggregate counters alone say about its size. */ +export type SizeCheck = + | { outcome: "within" } + | { outcome: "manual-review"; reason: string } + | { outcome: "list-files" }; + +/** + * First, cheapest step of the size check: decide from the PR's aggregate counters alone. + * + * - Zero changed files means GitHub could not compute the diff, and 3,000 or more means its file + * list is cut off. Neither can prove coverage, so a human must review. + * - If the whole PR is within the limits, so is its `specification/` subset: nothing more to check. + * - Otherwise the PR may still be fine (for example many docs files and few specification + * files), so the caller must list the files and count only `specification/` changes. + * + * The reasons are fixed strings because they are published in the status; never add model output. + */ +export function checkPullRequestSize(pullRequest: PullRequestCounts): SizeCheck { + const { changed_files, additions, deletions } = pullRequest; + if (changed_files < 1) { + return { outcome: "manual-review", reason: "GitHub could not compute the PR's size" }; + } + if (changed_files >= MAX_LISTED_FILES) { + return { + outcome: "manual-review", + reason: `PR has ${MAX_LISTED_FILES} or more changed files`, + }; + } + if ( + changed_files <= MAX_SPECIFICATION_FILES && + additions + deletions <= MAX_SPECIFICATION_LINES + ) { + return { outcome: "within" }; + } + return { outcome: "list-files" }; +} + +/** + * Second step of the size check, run once per page of the file list: add that page's + * `specification/` changes to the running `totals`. A renamed or moved file counts when its old or + * its new path is under `specification/`, so a file moved out of it is not missed. + * + * Totals only grow, so once they are over the limits the answer is final and a caller can stop + * reading pages. + * + * @returns `true` when the totals now exceed the limits. + */ +export function addSpecificationChanges( + totals: SpecificationTotals, + files: ChangedFile[], +): boolean { + for (const file of files) { + if ( + file.filename.startsWith("specification/") || + file.previous_filename?.startsWith("specification/") + ) { + totals.files++; + totals.lines += file.additions + file.deletions; + } + } + return totals.files > MAX_SPECIFICATION_FILES || totals.lines > MAX_SPECIFICATION_LINES; +} +/** + * Interprets a commit status as a semantic review outcome. An `error` status is a manual-review + * hold only when its description starts with `MANUAL_REVIEW_DESCRIPTION_PREFIX`; any other + * `error` is an incomplete review. + * + * @param status The latest `ARM Semantic Review` status, if any. + * @returns The outcome, or `undefined` when there is no status or its state is unrecognized. + */ +export function getSemanticReviewOutcome( + status: Pick | undefined, +): SemanticReviewOutcome | undefined { + if (!status) { + return undefined; + } + if (status.state === CommitStatusState.PENDING) { + return SemanticReviewOutcome.Pending; + } + if (status.state === CommitStatusState.SUCCESS) { + return SemanticReviewOutcome.Passed; + } + if (status.state === CommitStatusState.FAILURE) { + return SemanticReviewOutcome.ChangesRequested; + } + if (status.state === CommitStatusState.ERROR) { + return status.description?.startsWith(MANUAL_REVIEW_DESCRIPTION_PREFIX) + ? SemanticReviewOutcome.ManualReviewRequired + : SemanticReviewOutcome.ReviewIncomplete; + } + return undefined; +} + +/** + * Selects the newest `ARM Semantic Review` status. A SHA can hold many statuses for the same + * context (pending, then a result, then a re-run), and only the newest applies. + * + * @param statuses All commit statuses for a SHA. + * @returns The newest matching status, or `undefined` when there is none. + */ +export function getLatestSemanticReviewStatus(statuses: CommitStatus[]): CommitStatus | undefined { + return statuses + .filter((status) => status.context.toLowerCase() === ARM_SEMANTIC_REVIEW_STATUS.toLowerCase()) + .sort(invert(byDate((status) => status.updated_at)))[0]; +} diff --git a/.github/workflows/src/arm-auto-signoff/arm-universal-auto-signoff.ts b/.github/workflows/src/arm-auto-signoff/arm-universal-auto-signoff.ts index 879859c9b965..b9ae9f14a13b 100644 --- a/.github/workflows/src/arm-auto-signoff/arm-universal-auto-signoff.ts +++ b/.github/workflows/src/arm-auto-signoff/arm-universal-auto-signoff.ts @@ -2,32 +2,80 @@ import { inspect } from "node:util"; import { CommitStatusState, PER_PAGE_MAX } from "../../../shared/src/github.ts"; import { byDate, invert } from "../../../shared/src/sort.ts"; import { extractInputs } from "../context.ts"; -import type { Core, GitHub, GitHubScriptArgs } from "../github.ts"; +import type { Core, GitHub, GitHubScriptArgs, WebhookEvent } from "../github.ts"; import { LabelAction } from "../label.ts"; import { ArmAutoSignoffLabel } from "./arm-auto-signoff-labels.ts"; +import { + getLatestSemanticReviewStatus, + getSemanticReviewOutcome, + SemanticReviewOutcome, +} from "./arm-semantic-review.ts"; +/** Deterministic commit statuses that must be `success`, in addition to the semantic review. */ const requiredStatusNames = ["Swagger LintDiff", "Swagger Avocado"]; +/** + * The labels this workflow manages, each with the action to take. `ARMAutoSignedOff-Test` records + * the pilot decision. `ARMManualSignoffRequired` is added (and never removed here) when a human + * must review. `ARMSignedOff` is deliberately not managed: the pilot never changes it. + */ export type ManagedLabelActions = { "ARMAutoSignedOff-Test": LabelAction; + ARMManualSignoffRequired: LabelAction; }; +/** Returns a label-action set that changes nothing. */ function createNoneLabelActions(): ManagedLabelActions { return { [ArmAutoSignoffLabel.ArmAutoSignedOffTest]: LabelAction.None, + [ArmAutoSignoffLabel.ArmManualSignoffRequired]: LabelAction.None, }; } /* v8 ignore start */ +/** + * Workflow entry point. Resolves the PR and head SHA from the triggering event (see + * `extractInputs`), logs the correlation for diagnosis, and returns the label actions to apply. + * + * @returns The correlated head SHA and PR number, which the workflow uploads as artifacts on + * `workflow_run` triggers so `Update Labels` can find the PR, and the label actions to apply. + */ export default async function getLabelAction({ github, context, core }: GitHubScriptArgs): Promise<{ headSha: string; issueNumber: number; labelActions: ManagedLabelActions; }> { + const workflowRun = + context.eventName === "workflow_run" + ? (context.payload as WebhookEvent<"workflow-run">).workflow_run + : undefined; + core.info( + `Universal auto-signoff trigger: ${JSON.stringify({ + eventName: context.eventName, + action: context.payload.action, + workflowRun: workflowRun + ? { + id: workflowRun.id, + name: workflowRun.name, + path: workflowRun.path, + event: workflowRun.event, + conclusion: workflowRun.conclusion, + } + : undefined, + })}`, + ); const { owner, repo, head_sha, issue_number } = await extractInputs(github, context, core); + core.info( + `Universal auto-signoff correlation: ${JSON.stringify({ + owner, + repo, + issueNumber: Number.isSafeInteger(issue_number) ? issue_number : "missing", + headSha: head_sha || "missing", + })}`, + ); - return await getLabelActionImpl({ + const result = await getLabelActionImpl({ owner, repo, head_sha, @@ -35,9 +83,20 @@ export default async function getLabelAction({ github, context, core }: GitHubSc github, core, }); + core.info(`Universal auto-signoff output: ${JSON.stringify(result)}`); + return result; } /* v8 ignore stop */ +/** + * Decides label actions for a PR head. It first re-reads the PR and takes no action when it is + * closed or its head is no longer `head_sha`, so a stale event cannot change current labels. + * Otherwise it reads the PR's labels and the head SHA's commit statuses and defers to + * `getDesiredLabelActions`. + * + * @returns The correlation passed in and the label actions; no action when correlation is missing + * or stale. + */ export async function getLabelActionImpl({ owner, repo, @@ -60,7 +119,11 @@ export async function getLabelActionImpl({ }; if (!Number.isInteger(issue_number) || issue_number <= 0 || !head_sha) { - core.info("Missing pull request number or head SHA"); + core.info( + `Universal auto-signoff no-op: missing correlation ` + + `(issueNumber=${Number.isSafeInteger(issue_number) ? issue_number : "missing"}, ` + + `headSha=${head_sha || "missing"})`, + ); return noneResult; } @@ -71,7 +134,10 @@ export async function getLabelActionImpl({ pull_number: issue_number, }); if (pullRequest.state !== "open" || pullRequest.head.sha !== head_sha) { - core.info("Pull request is closed or its head SHA has changed"); + core.info( + `Universal auto-signoff no-op: pull request state/head mismatch ` + + `(state=${pullRequest.state}, expectedHead=${head_sha}, currentHead=${pullRequest.head.sha})`, + ); return noneResult; } @@ -84,8 +150,9 @@ export async function getLabelActionImpl({ }) ).map((label) => label.name); const hasAutoSignoff = labelNames.includes(ArmAutoSignoffLabel.ArmAutoSignedOffTest); + core.info(`Current pull request labels: ${JSON.stringify([...labelNames].sort())}`); - const desiredAction = await getDesiredLabelAction({ + const labelActions = await getDesiredLabelActions({ owner, repo, head_sha, @@ -94,16 +161,29 @@ export async function getLabelActionImpl({ github, core, }); + core.info(`Universal auto-signoff label actions: ${JSON.stringify(labelActions)}`); return { ...noneResult, - labelActions: { - [ArmAutoSignoffLabel.ArmAutoSignedOffTest]: desiredAction, - }, + labelActions, }; } -async function getDesiredLabelAction({ +/** + * Applies the auto-signoff policy for one head SHA. In order: + * + * 1. Not ready for ARM review (`ARMReview` missing or `NotReadyForARMReview` present): no signoff. + * 2. `ARM Semantic Review` is not `Passed`: no signoff. Only a manual-review hold + * (`Manual review required`) also adds `ARMManualSignoffRequired`; a pending, failed, or + * incomplete review adds nothing, because a re-run can still pass. + * 3. Labels block signoff (`ARMChangesRequested`, `ARMManualSignoffRequired`, or an unapproved + * `SuppressionReviewRequired`): no signoff. + * 4. Any required status (`Swagger LintDiff`, `Swagger Avocado`) is not `success`: no signoff. + * 5. Otherwise add `ARMAutoSignedOff-Test`. + * + * Whenever signoff is withheld, an existing `ARMAutoSignedOff-Test` is removed. + */ +async function getDesiredLabelActions({ owner, repo, head_sha, @@ -119,17 +199,25 @@ async function getDesiredLabelAction({ hasAutoSignoff: boolean; github: GitHub; core: Core; -}): Promise { - const labelsAllowSignoff = - labelNames.includes("ARMReview") && - !labelNames.includes("NotReadyForARMReview") && - !labelNames.includes(ArmAutoSignoffLabel.ArmManualSignoffRequired) && - (!labelNames.includes("SuppressionReviewRequired") || - labelNames.includes("Approved-Suppression")); +}): Promise { + const labelActions = createNoneLabelActions(); + const hasArmReview = labelNames.includes("ARMReview"); + const hasNotReadyForArmReview = labelNames.includes("NotReadyForARMReview"); + const isReadyForArmReview = hasArmReview && !hasNotReadyForArmReview; + core.info( + `ARM review readiness: ${JSON.stringify({ + hasArmReview, + hasNotReadyForArmReview, + isReadyForArmReview, + })}`, + ); - if (!labelsAllowSignoff) { - core.info("Labels do not meet requirements for universal auto-signoff"); - return hasAutoSignoff ? LabelAction.Remove : LabelAction.None; + if (!isReadyForArmReview) { + core.info("Pull request is not ready for ARM review"); + labelActions[ArmAutoSignoffLabel.ArmAutoSignedOffTest] = hasAutoSignoff + ? LabelAction.Remove + : LabelAction.None; + return labelActions; } const statuses: import("@octokit/plugin-rest-endpoint-methods").RestEndpointMethodTypes["repos"]["listCommitStatusesForRef"]["response"]["data"] = @@ -140,6 +228,72 @@ async function getDesiredLabelAction({ per_page: PER_PAGE_MAX, }); + const latestSemanticReviewStatus = getLatestSemanticReviewStatus(statuses); + const semanticReviewOutcome = getSemanticReviewOutcome(latestSemanticReviewStatus); + core.info( + `Latest ARM Semantic Review status: ${JSON.stringify({ + state: latestSemanticReviewStatus?.state ?? "missing", + description: latestSemanticReviewStatus?.description ?? "missing", + targetUrl: latestSemanticReviewStatus?.target_url ?? "missing", + updatedAt: latestSemanticReviewStatus?.updated_at ?? "missing", + outcome: semanticReviewOutcome ?? "missing", + })}`, + ); + + if (semanticReviewOutcome !== SemanticReviewOutcome.Passed) { + if (semanticReviewOutcome === SemanticReviewOutcome.ManualReviewRequired) { + const hasManualSignoffRequired = labelNames.includes( + ArmAutoSignoffLabel.ArmManualSignoffRequired, + ); + core.info( + `ARM semantic review requires manual signoff; ` + + `${ArmAutoSignoffLabel.ArmManualSignoffRequired} ` + + `${hasManualSignoffRequired ? "is already present" : "will be added"}`, + ); + if (!hasManualSignoffRequired) { + labelActions[ArmAutoSignoffLabel.ArmManualSignoffRequired] = LabelAction.Add; + } + } else { + core.info( + `ARM semantic review has not passed: ${semanticReviewOutcome ?? "missing"}` + + (latestSemanticReviewStatus?.description + ? ` (${latestSemanticReviewStatus.description})` + : ""), + ); + } + labelActions[ArmAutoSignoffLabel.ArmAutoSignedOffTest] = hasAutoSignoff + ? LabelAction.Remove + : LabelAction.None; + return labelActions; + } + + const hasArmChangesRequested = labelNames.includes("ARMChangesRequested"); + const hasManualSignoffRequired = labelNames.includes( + ArmAutoSignoffLabel.ArmManualSignoffRequired, + ); + const hasSuppressionReviewRequired = labelNames.includes("SuppressionReviewRequired"); + const hasApprovedSuppression = labelNames.includes("Approved-Suppression"); + const labelsAllowSignoff = + !hasArmChangesRequested && + !hasManualSignoffRequired && + (!hasSuppressionReviewRequired || hasApprovedSuppression); + core.info( + `Auto-signoff label eligibility: ${JSON.stringify({ + hasArmChangesRequested, + hasManualSignoffRequired, + hasSuppressionReviewRequired, + hasApprovedSuppression, + labelsAllowSignoff, + })}`, + ); + if (!labelsAllowSignoff) { + core.info("Labels do not meet requirements for universal auto-signoff"); + labelActions[ArmAutoSignoffLabel.ArmAutoSignedOffTest] = hasAutoSignoff + ? LabelAction.Remove + : LabelAction.None; + return labelActions; + } + for (const statusName of requiredStatusNames) { // A status context may appear more than once; only the most recently updated result applies. const matchingStatuses = statuses @@ -150,10 +304,16 @@ async function getDesiredLabelAction({ core.info(`${statusName}: ${latestStatus?.state ?? "missing"}`); if (latestStatus?.state !== CommitStatusState.SUCCESS) { core.info(`Required status '${statusName}' did not succeed`); - return hasAutoSignoff ? LabelAction.Remove : LabelAction.None; + labelActions[ArmAutoSignoffLabel.ArmAutoSignedOffTest] = hasAutoSignoff + ? LabelAction.Remove + : LabelAction.None; + return labelActions; } } core.info(`Universal auto-signoff pilot requirements met: ${inspect(requiredStatusNames)}`); - return hasAutoSignoff ? LabelAction.None : LabelAction.Add; + labelActions[ArmAutoSignoffLabel.ArmAutoSignedOffTest] = hasAutoSignoff + ? LabelAction.None + : LabelAction.Add; + return labelActions; } diff --git a/.github/workflows/src/context.ts b/.github/workflows/src/context.ts index 98b71d637d1d..60549c600fba 100644 --- a/.github/workflows/src/context.ts +++ b/.github/workflows/src/context.ts @@ -15,6 +15,15 @@ export type RestEndpointMethodTypes = /** * Extracts inputs from context based on event name and properties. * run_id is only defined for "workflow_run:completed" events. + * + * For "workflow_run:completed", `workflow_run.event` is the event that triggered the completed + * (upstream) run, and decides where the head SHA and PR number come from: + * - "pull_request": the payload only (fork code could forge artifacts). + * - "pull_request_target": the payload, overridden by the upstream run's trusted artifacts. + * - "issue_comment", "workflow_run", "check_run", "workflow_dispatch": artifacts only. + * + * `head_sha` may be "" and `issue_number` may be NaN when they can't be determined. + * @throws If artifacts conflict with each other or with the event, or the event is unsupported. */ export async function extractInputs( github: GitHub, @@ -36,6 +45,12 @@ export async function extractInputs( if (context.eventName === "workflow_run") { const payload = context.payload as WebhookEvent<"workflow-run">; workflowRunEvent = payload.workflow_run?.event; + core.info(` payload.workflow_run.id: ${payload.workflow_run?.id ?? "undefined"}`); + core.info(` payload.workflow_run.name: ${payload.workflow_run?.name ?? "undefined"}`); + core.info(` payload.workflow_run.path: ${payload.workflow_run?.path ?? "undefined"}`); + core.info( + ` payload.workflow_run.conclusion: ${payload.workflow_run?.conclusion ?? "undefined"}`, + ); } core.info(` payload.workflow_run.event: ${workflowRunEvent}`); @@ -115,8 +130,8 @@ export async function extractInputs( } else if (context.eventName === "workflow_run" && context.payload.action === "completed") { const payload = context.payload as WebhookEvent<"workflow-run", "completed">; - let issue_number = NaN; - let head_sha = ""; + let issue_number: number; + let head_sha: string; if ( payload.workflow_run.event === "pull_request" || @@ -131,9 +146,8 @@ export async function extractInputs( // For pull_request, do NOT attempt to extract the issue number from an artifact, since this could be modified // in a fork PR. // - // For pull_request_target, it might be safe to extract the issue number from an artifact, since the workflow runs - // on the target branch and can be trusted. But it should also be unnecessary, since we should be able extract - // the issue number from the payload itself, just like pull_request. + // For pull_request_target, the workflow runs from the base branch, so its artifacts are trusted and + // override the SHA and are checked against the PR number below. const pullRequest = payload.workflow_run.pull_requests?.find((pr) => pr !== null); if (pullRequest) { @@ -207,57 +221,42 @@ export async function extractInputs( ); } } + + if (payload.workflow_run.event === "pull_request_target") { + const artifactInputs = await getWorkflowRunArtifactInputs({ + github, + core, + ...getRepositoryInfo(payload.workflow_run.repository), + runId: payload.workflow_run.id, + }); + if (artifactInputs.headSha) { + // The artifact can be newer than the SHA that triggered the run if a push landed in between. + head_sha = artifactInputs.headSha; + } + if (artifactInputs.issueNumber) { + if (issue_number && issue_number !== artifactInputs.issueNumber) { + throw new Error( + `Workflow artifacts reference PR ${artifactInputs.issueNumber}, but the event references PR ${issue_number}`, + ); + } + issue_number = artifactInputs.issueNumber; + } + } } else if ( payload.workflow_run.event === "issue_comment" || payload.workflow_run.event == "workflow_run" || - payload.workflow_run.event == "check_run" + payload.workflow_run.event == "check_run" || + payload.workflow_run.event == "workflow_dispatch" ) { - // Attempt to extract issue number from artifact. This can be trusted, because it was uploaded from a workflow that is trusted, - // because "issue_comment" and "workflow_run" only trigger on workflows in the default branch. - const artifacts = await github.paginate(github.rest.actions.listWorkflowRunArtifacts, { + // These artifacts can be trusted because these event types run workflows from the default branch. + const artifactInputs = await getWorkflowRunArtifactInputs({ + github, + core, ...getRepositoryInfo(payload.workflow_run.repository), - run_id: payload.workflow_run.id, - per_page: PER_PAGE_MAX, + runId: payload.workflow_run.id, }); - - const artifactNames = artifacts.map((a) => a.name); - - core.info(`artifactNames: ${JSON.stringify(artifactNames)}`); - - for (const artifactName of artifactNames) { - // If artifactName has format "head-sha=valid-full-sha", set head_sha=value - // Else, if artifactName has format "head-sha=other-string", warn and set head_sha="" - // Else, if artifactName has format "issue-number=positive-integer", set issue_number=value - // Else, if artifactName has format "issue-number=other-string", warn and set issue_number=NaN - // - Workflows should probably only set "issue-number" to positive integers, but sometimes set it to "null" - // Else, if artifactName does not start with "head-sha=" or "issue-number=", ignore it - const firstEquals = artifactName.indexOf("="); - if (firstEquals !== -1) { - const key = artifactName.substring(0, firstEquals); - if (key === "head-sha") { - const value = artifactName.substring(firstEquals + 1); - if (isFullGitSha(value)) { - head_sha = value; - } else { - // Producers must ensure they only set head-sha to valid full git SHA - throw new Error(`head-sha is not a valid full git SHA: '${value}'`); - } - continue; - } else if (key === "issue-number") { - const value = artifactName.substring(firstEquals + 1); - const parsedValue = Number.parseInt(value); - if (parsedValue > 0) { - issue_number = parsedValue; - } else { - // TODO: Consider throwing instead of warning. May need to handle `issue-number=null|undefined`, - // but invalid integers should throw. - core.info(`Invalid issue-number: '${value}' parsed to '${parsedValue}'`); - issue_number = NaN; - } - continue; - } - } - } + head_sha = artifactInputs.headSha; + issue_number = artifactInputs.issueNumber; if (!head_sha) { core.info( `Could not find 'head-sha' artifact, which is required to associate the triggering workflow run with the head SHA of a PR`, @@ -316,6 +315,86 @@ export async function extractInputs( return inputs; } +/** + * Reads the `head-sha=` and `issue-number=` pairs from the artifact names of a workflow + * run. Only call this for runs whose artifacts come from trusted code (see `extractInputs`). + * @returns `headSha` ("" if absent) and `issueNumber` (NaN if absent or invalid). + * @throws If the artifacts hold an invalid or conflicting head SHA, or conflicting PR numbers. + */ +export async function getWorkflowRunArtifactInputs({ + github, + core, + owner, + repo, + runId, +}: { + github: GitHub; + core: Core; + owner: string; + repo: string; + runId: number; +}): Promise<{ headSha: string; issueNumber: number }> { + const artifacts = await github.paginate(github.rest.actions.listWorkflowRunArtifacts, { + owner, + repo, + run_id: runId, + per_page: PER_PAGE_MAX, + }); + const artifactNames = artifacts.map((artifact) => artifact.name); + core.info(`artifactNames: ${JSON.stringify(artifactNames)}`); + return parseWorkflowRunArtifactInputs(artifactNames, core); +} + +/** + * Parses `head-sha=` and `issue-number=` from artifact names; other names are ignored. + * A producer may carry both in one artifact by joining the pairs with `;`, for example + * `head-sha=;issue-number=`, which saves an upload step. Names without `;` are a single + * pair, so existing single-value artifacts parse unchanged. + * @returns `headSha` ("" if absent) and `issueNumber` (NaN if absent or invalid). + * @throws If a head SHA is invalid, or two head SHAs or two PR numbers conflict. + */ +export function parseWorkflowRunArtifactInputs( + artifactNames: string[], + core: Core, +): { headSha: string; issueNumber: number } { + let headSha = ""; + let issueNumber = NaN; + for (const artifactName of artifactNames.flatMap((name) => name.split(";"))) { + const firstEquals = artifactName.indexOf("="); + if (firstEquals === -1) { + continue; + } + + const key = artifactName.substring(0, firstEquals); + const value = artifactName.substring(firstEquals + 1); + if (key === "head-sha") { + if (!isFullGitSha(value)) { + throw new Error(`head-sha is not a valid full git SHA: '${value}'`); + } + if (headSha && headSha.toLowerCase() !== value.toLowerCase()) { + throw new Error(`Conflicting head-sha artifacts: '${headSha}' and '${value}'`); + } + headSha = value; + } else if (key === "issue-number") { + // Stricter than parseInt, which would accept "12abc". + const parsedValue = /^[1-9]\d*$/.test(value) ? Number(value) : NaN; + if (Number.isSafeInteger(parsedValue)) { + if (Number.isSafeInteger(issueNumber) && issueNumber !== parsedValue) { + throw new Error( + `Conflicting issue-number artifacts: '${issueNumber}' and '${parsedValue}'`, + ); + } + issueNumber = parsedValue; + } else { + core.info(`Invalid issue-number artifact: '${value}'`); + issueNumber = NaN; + } + } + } + + return { headSha, issueNumber }; +} + function getRepositoryInfo( repository: { name?: string; owner?: { login?: string } | null } | null | undefined, ): { diff --git a/.github/workflows/test/arm-api-review-workflow.test.ts b/.github/workflows/test/arm-api-review-workflow.test.ts index 14b1c2d0273a..76b7b4ba46a6 100644 --- a/.github/workflows/test/arm-api-review-workflow.test.ts +++ b/.github/workflows/test/arm-api-review-workflow.test.ts @@ -3,6 +3,7 @@ import { parse } from "yaml"; import { join } from "node:path"; import { beforeAll, describe, expect, it, vi } from "vitest"; import { runInNewContext } from "node:vm"; +import { fullGitSha } from "../../shared/test/examples.ts"; // cspell:ignore REPOST vally @@ -59,7 +60,7 @@ function createResolverHarness( github: { rest: { pulls: { get } } }, context, core: { setOutput }, - process: { env: { TARGET_PR_NUMBER: value } }, + process: { env: { GITHUB_RUN_ATTEMPT: "1", TARGET_PR_NUMBER: value } }, }), }; } @@ -179,6 +180,7 @@ describe("ARM API review workflow", () => { expect(source).toContain(`TARGET_PR_NUMBER: ${TARGET_EXPRESSION}`); expect(source).toContain("const { data: pull } = await github.rest.pulls.get({"); expect(source).toContain('core.setOutput("target_pr_number", String(pull.number))'); + expect(source).toContain('core.setOutput("run_attempt", runAttempt)'); expect(source).toContain( "target_pr_number: ${{ steps.resolve_target_pr.outputs.target_pr_number }}", ); @@ -209,7 +211,7 @@ describe("ARM API review workflow", () => { describe("target PR resolver", () => { it("resolves a workflow-dispatch target and publishes the canonical PR number", async () => { const harness = createResolverHarness(resolverScript, { value: "44499" }); - harness.get.mockResolvedValue({ data: { number: 44499 } }); + harness.get.mockResolvedValue({ data: { number: 44499, head: { sha: fullGitSha } } }); await harness.run(); @@ -220,6 +222,8 @@ describe("ARM API review workflow", () => { pull_number: 44499, }); expect(harness.setOutput).toHaveBeenCalledWith("target_pr_number", "44499"); + expect(harness.setOutput).toHaveBeenCalledWith("target_head_sha", fullGitSha); + expect(harness.setOutput).toHaveBeenCalledWith("run_attempt", "1"); }); it("accepts an issue-shaped pull request comment payload", async () => { @@ -228,7 +232,7 @@ describe("ARM API review workflow", () => { eventName: "issue_comment", payload: { issue: { pull_request: { url: "https://api.github.com/pulls/44499" } } }, }); - harness.get.mockResolvedValue({ data: { number: 44499 } }); + harness.get.mockResolvedValue({ data: { number: 44499, head: { sha: fullGitSha } } }); await harness.run(); @@ -283,6 +287,8 @@ describe("ARM API review workflow", () => { expect(source).toContain("github.event.comment.body == '/arm-review'"); expect(source).not.toContain("contains(github.event.comment.body, '/arm-review')"); + expect(source).toContain("types: [opened, synchronize, reopened, labeled, ready_for_review]"); + expect(source).toContain("github.event.action == 'reopened'"); expect(source).toContain('bots: ["github-actions[bot]"]'); expect(source).toContain("status-comment: true"); expect(source).toContain(`run-name: "ARM API Review #${TARGET_EXPRESSION}`); @@ -290,6 +296,8 @@ describe("ARM API review workflow", () => { expect(compiled).toContain('GH_AW_ALLOWED_BOTS: "github-actions[bot]"'); expect(compiled).toContain("- name: Add comment with workflow run link"); expect(compiled).toContain("- name: Update reaction comment with completion status"); + expect(compiled).toContain(" - reopened"); + expect(compiled).toContain("github.event.action == 'reopened'"); expect(compiled).toContain(`run-name: "ARM API Review #${TARGET_EXPRESSION}`); const statusCommentStart = compiled.indexOf("- name: Add comment with workflow run link"); @@ -313,12 +321,27 @@ describe("ARM API review workflow", () => { it("wires the mandatory ARM Critic as an inline runtime subagent", async () => { const [source, compiled] = await readWorkflowFiles(); + const criticRuntime = source.slice( + source.indexOf("## agent: `arm-api-review-critic-runtime`"), + source.indexOf("## end agent: `arm-api-review-critic-runtime`") + + "## end agent: `arm-api-review-critic-runtime`".length, + ); + const criticRuntimeImports = [ + ".github/agents/arm-api-review-critic.agent.md", + ".github/agents/protocols/arm-api-review-critic.protocol.md", + ".github/agents/protocols/arm-api-review-critic-inputs.template.md", + ]; expect(source).toContain( "## agent: `arm-api-review-critic-runtime`\n---\ndescription: Independently verifies ARM API Reviewer findings before publication\n---", ); expect(source).toContain("dispatch the inline\n`arm-api-review-critic-runtime` subagent"); - expect(source).toContain("`.github/agents/arm-api-review-critic.agent.md`"); + expect(criticRuntime).toContain("Treat the embedded sections as already loaded"); + for (const importPath of criticRuntimeImports) { + expect(criticRuntime).toContain(`{{#runtime-import ${importPath}}}`); + await expect(readFile(join(ROOT, importPath), "utf8")).resolves.not.toHaveLength(0); + } + expect(criticRuntime).toContain("## end agent: `arm-api-review-critic-runtime`"); expect(source).toContain("Never claim that the review was Critic-verified"); expect(compiled).toContain("- name: Restore inline sub-agents from activation artifact"); expect(compiled).toContain('GH_AW_SUB_AGENT_DIR: ".github/agents"'); @@ -444,15 +467,20 @@ describe("ARM API review workflow", () => { it("keeps the agent read-only and preserves the human queue after a clean review", async () => { const [source, compiled] = await readWorkflowFiles(); + const collapsed = collapseWhitespace(source); - expect(source).toContain("permissions:\n pull-requests: read\n steps:"); expect(source).toContain( - "**At least one Blocking `POST-NEW` or Blocking `RESOLVE-AND-REPOST` queued", + "permissions:\n pull-requests: read\n statuses: write\n steps:", + ); + expect(source).toContain( + "**The review is full and complete, and at least one verified, currently", ); - expect(source).toContain("_and_ the Critic returned a verdict**"); - expect(source).toContain("**No Blocking finding queued for publication**"); + expect(collapsed).toContain("including `SKIP-COVERED`, `POST-NEW`, and `RESOLVE-AND-REPOST`"); expect(source).toContain( - "clean, covered,\n clarification-only, Critic-dropped, or overflow-only", + "**No verified, currently applicable Blocking finding remains, and the review", + ); + expect(collapsed).toContain( + "remove `ARMChangesRequested` if present; leave `WaitForARMFeedback`", ); expect(source).not.toContain("**Blocking findings found**"); @@ -478,6 +506,7 @@ describe("ARM API review workflow", () => { expect(agentJob).not.toContain("issues: write"); expect(agentJob).not.toContain("pull-requests: write"); expect(preActivationJob).toContain("pull-requests: read"); + expect(preActivationJob).toContain("statuses: write"); expect(preActivationJob).not.toContain("issues: write"); expect(safeOutputsJob).toContain("issues: write"); expect(safeOutputsJob).toContain("pull-requests: write"); @@ -533,19 +562,102 @@ describe("ARM API review posting reliability", () => { it("leaves no metadata-driven exception to the ARMChangesRequested label rule", async () => { const source = collapseWhitespace(await readFile(join(ROOT, SOURCE_FILE), "utf8")); - expect(source).toContain("These three rules are **exhaustive**."); + expect(source).toContain("These rules are **exhaustive**."); expect(source).toContain( - "draft status, a `[Test]` or `[Do-Not-Merge]` title, a revert, a bot-authored PR, or the author's stated intent not to merge are **not** grounds to skip a label change", + "The review is scoped or incomplete** β†’ leave `WaitForARMFeedback`, `ARMChangesRequested`, and `ARMSignedOff` unchanged", ); - // The decision now has exactly two inputs, not one: a Blocking finding must - // be queued AND the Critic must have verified it. PR metadata still may not - // influence the outcome. expect(source).toContain( - "There are exactly **two** inputs to this decision: whether a Blocking finding was queued for publication, and whether the Critic verified it.", + "No verified, currently applicable Blocking finding remains, and the review is full and complete", ); expect(source).toContain( - "Nothing else, and in particular nothing read from PR metadata, may change the outcome.", + "draft status, a `[Test]` or `[Do-Not-Merge]` title, a revert, a bot-authored PR, or the author's stated intent not to merge are **not** grounds to skip a label change", + ); + expect(source).toContain( + "The decision uses only the verified, currently applicable Blocking findings after reconciliation, review scope, and review completeness.", + ); + expect(source).toContain("Nothing read from unrelated PR metadata may change the outcome."); + }); + + it("publishes one head-bound semantic review status from the trusted safe-output job", async () => { + const [source, compiled] = await readWorkflowFiles(); + const collapsed = collapseWhitespace(source); + + expect(source).toContain("name: Set ARM semantic review pending"); + // One upload carries both correlation values; the old per-value steps must not return. + expect(source).toContain("name: Upload ARM semantic review correlation"); + expect(source).not.toContain("name: Upload ARM semantic review head SHA"); + expect(source).not.toContain("name: Upload ARM semantic review issue number"); + expect(compiled).toContain( + "head-sha=${{ steps.resolve_target_pr.outputs.target_head_sha }};issue-number=${{ steps.resolve_target_pr.outputs.target_pr_number }}", + ); + expect(source).toContain('context: "ARM Semantic Review"'); + expect(source).toContain("record-arm-semantic-review:"); + expect(source).toContain("scope:"); + expect(source).toContain("completeness:"); + expect(source).toContain("### Step 9: Record Semantic Result"); + expect(collapsed).toContain( + "Call `record_arm_semantic_review` exactly once after queuing the summary", + ); + expect(compiled).toContain("record_arm_semantic_review"); + expect(compiled).toContain("ARM Semantic Review"); + expect(compiled).toContain("Publish ARM semantic review status"); + expect(compiled).not.toContain("Upload ARM semantic review receipt"); + const semanticJob = compiled.slice( + compiled.indexOf("\n record_arm_semantic_review:\n"), + compiled.indexOf("\n safe_outputs:\n"), + ); + const semanticConfig = source.slice( + source.indexOf(" record-arm-semantic-review:\n"), + source.indexOf(" noop:\n"), + ); + expect(semanticJob).toContain("name: Publish ARM semantic review status"); + expect(semanticJob).toContain("actions: read"); + expect(semanticJob).toContain("statuses: write"); + expect(semanticJob).not.toContain("TARGET_PR_NUMBER"); + expect(semanticJob).toContain("permissions:"); + expect(semanticConfig).not.toContain("issue_number:"); + expect(semanticConfig).not.toContain("head_sha:"); + expect(semanticConfig).not.toContain("run_attempt:"); + expect(semanticConfig).not.toContain('"degraded"'); + expect(semanticConfig).toContain("incomplete_reason:"); + expect(semanticConfig).toContain('"critic-unavailable"'); + expect(semanticConfig).toContain('"discussion-data-unavailable"'); + expect(collapsed).toContain( + "Trusted workflow artifacts attach the pull request, head SHA, and run attempt", + ); + expect(collapsed).toContain( + "The trusted `record_arm_semantic_review` job validates the item and publishes the head-bound status inside this reviewer run", + ); + expect(collapsed).toContain( + "Universal Auto-Signoff consumes the status only after the entire reviewer workflow completes", + ); + }); + + it("resolves an unpublished Pending status from the conclusion job", async () => { + const [source, compiled] = await readWorkflowFiles(); + const conclusionStart = compiled.indexOf("\n conclusion:\n"); + const conclusionJob = compiled.slice( + conclusionStart, + compiled.indexOf("\n detection:\n", conclusionStart), + ); + const finalizerStep = conclusionJob.indexOf( + "name: Resolve unpublished ARM semantic review status", + ); + + expect(source).toContain("finalizeUnpublishedArmSemanticReview"); + expect(conclusionStart).toBeGreaterThan(-1); + expect(conclusionJob).toContain("statuses: write"); + expect(finalizerStep).toBeGreaterThan(-1); + // The finalizer is a pre-step, so it must not be able to block gh-aw's own + // failure reporting that runs after it. + expect(conclusionJob.slice(finalizerStep, finalizerStep + 700)).toContain( + "continue-on-error: true", + ); + // The record job already published when it succeeded, so the finalizer's reads are skipped. + expect(conclusionJob.slice(finalizerStep, finalizerStep + 200)).toContain( + "if: needs.record_arm_semantic_review.result != 'success'", ); + expect(conclusionJob.indexOf("name: Process no-op messages")).toBeGreaterThan(finalizerStep); }); it("withholds ARMChangesRequested when the Critic could not verify the findings", async () => { @@ -749,11 +861,10 @@ describe("ARM API review consistency and hardening", () => { it("gives the label rules a single unambiguous outcome per case", async () => { const source = collapseWhitespace(await readFile(join(ROOT, SOURCE_FILE), "utf8")); - // Rule 1 and the Critic-unavailable rule both matched the case - // (Blocking queued AND Critic unavailable) with opposite outcomes and no - // stated precedence. Rule 1 now carries the Critic condition itself. + // Unverified findings from a Critic-unavailable run do not satisfy the + // verified-finding rule, so the explicit Critic-unavailable rule remains authoritative. expect(source).toContain( - "**At least one Blocking `POST-NEW` or Blocking `RESOLVE-AND-REPOST` queued _and_ the Critic returned a verdict**", + "**The review is full and complete, and at least one verified, currently applicable Blocking finding remains**", ); }); diff --git a/.github/workflows/test/arm-auto-signoff/arm-semantic-review.test.ts b/.github/workflows/test/arm-auto-signoff/arm-semantic-review.test.ts new file mode 100644 index 000000000000..653a026fe236 --- /dev/null +++ b/.github/workflows/test/arm-auto-signoff/arm-semantic-review.test.ts @@ -0,0 +1,857 @@ +import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { describe, expect, it } from "vitest"; +import { CommitStatusState, PER_PAGE_MAX } from "../../../shared/src/github.ts"; +import { + ARM_SEMANTIC_REVIEW_STATUS, + evaluateSemanticReview, + getSemanticReviewOutcome, + addSpecificationChanges, + checkPullRequestSize, + parseSemanticReviewResult, + SemanticReviewCompletion, + SemanticReviewIncompleteReason, + SemanticReviewOutcome, + SemanticReviewScope, + type ChangedFile, + type SemanticReviewResult, + type SpecificationTotals, +} from "../../src/arm-auto-signoff/arm-semantic-review.ts"; +import publishArmSemanticReviewStatus, { + finalizeUnpublishedArmSemanticReview, +} from "../../src/arm-auto-signoff/arm-semantic-review-workflow.ts"; +import type { GitHubScriptArgs } from "../../src/github.ts"; +import { createMockCore, createMockGithub } from "../mocks.ts"; + +const owner = "Azure"; +const repo = "azure-rest-api-specs"; +const issueNumber = 123; +const headSha = "0123456789abcdef0123456789abcdef01234567"; +const runId = 456; +const runAttempt = 1; +const runUrl = "https://github.com/Azure/azure-rest-api-specs/actions/runs/456"; + +const passedResult: SemanticReviewResult = { + blockingCount: 0, + reviewScope: SemanticReviewScope.Full, + completion: SemanticReviewCompletion.Complete, + incompleteReason: SemanticReviewIncompleteReason.None, +}; + +function agentOutput(result: SemanticReviewResult = passedResult) { + return { + items: [ + { + type: "record_arm_semantic_review", + blocking_count: String(result.blockingCount), + scope: result.reviewScope, + completeness: result.completion, + incomplete_reason: result.incompleteReason, + }, + ], + }; +} + +const smallPullRequest = { number: issueNumber, changed_files: 1, additions: 10, deletions: 5 }; + +function createPublisherGithub() { + const github = createMockGithub(); + github.rest.actions.listWorkflowRunArtifacts.mockResolvedValue({ + data: { + artifacts: [{ name: `head-sha=${headSha}` }, { name: `issue-number=${issueNumber}` }], + }, + }); + // Default: small PR well within automated review limits + github.rest.pulls.get.mockResolvedValue({ data: smallPullRequest }); + return github; +} + +async function runPublisher({ + output = agentOutput(), + includeOutput = true, + setOutputPath = true, + github = createPublisherGithub(), +}: { + output?: unknown; + includeOutput?: boolean; + setOutputPath?: boolean; + github?: ReturnType; +} = {}) { + const directory = await mkdtemp(join(tmpdir(), "arm-semantic-review-")); + const outputPath = join(directory, "agent_output.json"); + const previousOutputPath = process.env.GH_AW_AGENT_OUTPUT; + const previousRunAttempt = process.env.GITHUB_RUN_ATTEMPT; + try { + if (includeOutput) { + await writeFile(outputPath, JSON.stringify(output), "utf8"); + } + if (setOutputPath) { + process.env.GH_AW_AGENT_OUTPUT = outputPath; + } else { + delete process.env.GH_AW_AGENT_OUTPUT; + } + process.env.GITHUB_RUN_ATTEMPT = String(runAttempt); + const core = createMockCore(); + const result = await publishArmSemanticReviewStatus({ + github, + context: { + repo: { owner, repo }, + runId, + serverUrl: "https://github.com", + }, + core, + } as unknown as GitHubScriptArgs); + return { github, result, core }; + } finally { + if (previousOutputPath === undefined) { + delete process.env.GH_AW_AGENT_OUTPUT; + } else { + process.env.GH_AW_AGENT_OUTPUT = previousOutputPath; + } + if (previousRunAttempt === undefined) { + delete process.env.GITHUB_RUN_ATTEMPT; + } else { + process.env.GITHUB_RUN_ATTEMPT = previousRunAttempt; + } + await rm(directory, { recursive: true, force: true }); + } +} + +describe("parseSemanticReviewResult", () => { + it("parses one valid result", () => { + expect(parseSemanticReviewResult(agentOutput())).toEqual(passedResult); + }); + + it("ignores model-supplied identifiers", () => { + const output = agentOutput(); + Object.assign(output.items[0], { + head_sha: "675718552ce00736684ed7a184bab", + issue_number: "999", + run_attempt: "99", + }); + + expect(parseSemanticReviewResult(output)).toEqual(passedResult); + }); + + it("rejects missing or duplicate semantic results", () => { + expect(() => parseSemanticReviewResult({ items: [] })).toThrow( + "Expected one ARM semantic review result, found 0", + ); + expect(() => + parseSemanticReviewResult({ + items: [ + ...agentOutput().items, + ...agentOutput({ ...passedResult, blockingCount: 1 }).items, + ], + }), + ).toThrow("Expected one ARM semantic review result, found 2"); + expect(() => parseSemanticReviewResult({})).toThrow("missing its items array"); + }); + + it.each([ + { field: "blocking_count", value: "-1", message: "Invalid ARM semantic review Blocking count" }, + { field: "blocking_count", value: 0, message: "Invalid ARM semantic review Blocking count" }, + { field: "scope", value: "partial", message: "Invalid ARM semantic review scope" }, + { field: "completeness", value: "unknown", message: "Invalid ARM semantic review completion" }, + { + field: "incomplete_reason", + value: "unknown", + message: "Invalid ARM semantic review incomplete reason", + }, + ])("rejects an invalid $field", ({ field, value, message }) => { + expect(() => + parseSemanticReviewResult({ items: [{ ...agentOutput().items[0], [field]: value }] }), + ).toThrow(message); + }); + + it("rejects a complete review that carries an incomplete reason", () => { + expect(() => + parseSemanticReviewResult({ + items: [ + { + ...agentOutput().items[0], + incomplete_reason: SemanticReviewIncompleteReason.CriticUnavailable, + }, + ], + }), + ).toThrow("completion 'complete' is inconsistent"); + }); +}); +describe("evaluateSemanticReview", () => { + it("passes when a full, complete review has no Blocking findings", () => { + expect(evaluateSemanticReview(passedResult)).toEqual({ + state: CommitStatusState.SUCCESS, + description: "Passed: full review completed with no Blocking findings", + }); + }); + + it("requests changes when Blocking findings remain", () => { + expect(evaluateSemanticReview({ ...passedResult, blockingCount: 2 })).toEqual({ + state: CommitStatusState.FAILURE, + description: "Changes requested: 2 Blocking finding(s)", + }); + }); + + it("requires manual review when the semantic review is scoped", () => { + const result = { ...passedResult, reviewScope: SemanticReviewScope.Scoped }; + const description = "Manual review required: scoped review requires manual signoff"; + + expect(evaluateSemanticReview(result)).toEqual({ + state: CommitStatusState.ERROR, + description, + }); + }); + + it.each([ + SemanticReviewIncompleteReason.CriticUnavailable, + SemanticReviewIncompleteReason.RequiredEvidenceUnavailable, + SemanticReviewIncompleteReason.ToolFailure, + SemanticReviewIncompleteReason.StaleSha, + SemanticReviewIncompleteReason.DiscussionDataUnavailable, + ])("includes incomplete reason %s in the status", (incompleteReason) => { + const result = { + ...passedResult, + completion: SemanticReviewCompletion.Incomplete, + incompleteReason, + }; + + expect(evaluateSemanticReview(result)).toEqual({ + state: CommitStatusState.ERROR, + description: `Review incomplete: ${incompleteReason}`, + }); + }); + + it.each([ + { + name: "scoped with blockers", + result: { ...passedResult, reviewScope: SemanticReviewScope.Scoped, blockingCount: 2 }, + description: "Manual review required: scoped review requires manual signoff", + }, + { + name: "incomplete with blockers", + result: { + ...passedResult, + completion: SemanticReviewCompletion.Incomplete, + incompleteReason: SemanticReviewIncompleteReason.CriticUnavailable, + blockingCount: 1, + }, + description: "Review incomplete: critic-unavailable", + }, + ])( + "requires manual review rather than changes requested for $name", + ({ result, description }) => { + expect(evaluateSemanticReview(result)).toEqual({ + state: CommitStatusState.ERROR, + description, + }); + }, + ); + + it("maps commit status states back to semantic outcomes", () => { + expect(getSemanticReviewOutcome({ state: "pending" })).toBe(SemanticReviewOutcome.Pending); + expect(getSemanticReviewOutcome({ state: "success" })).toBe(SemanticReviewOutcome.Passed); + expect(getSemanticReviewOutcome({ state: "failure" })).toBe( + SemanticReviewOutcome.ChangesRequested, + ); + expect(getSemanticReviewOutcome({ state: "error" })).toBe( + SemanticReviewOutcome.ReviewIncomplete, + ); + expect( + getSemanticReviewOutcome({ + state: "error", + description: "Review incomplete: tool-failure", + }), + ).toBe(SemanticReviewOutcome.ReviewIncomplete); + expect( + getSemanticReviewOutcome({ + state: "error", + description: "Manual review required: scoped review requires manual signoff", + }), + ).toBe(SemanticReviewOutcome.ManualReviewRequired); + expect(getSemanticReviewOutcome(undefined)).toBeUndefined(); + expect(getSemanticReviewOutcome({ state: "unexpected" })).toBeUndefined(); + }); +}); + +describe("checkPullRequestSize", () => { + it.each([ + { name: "a small PR", counts: [1, 10, 5], expected: { outcome: "within" } }, + { name: "exactly 50 files", counts: [50, 50, 0], expected: { outcome: "within" } }, + { name: "exactly 5,000 lines", counts: [1, 3_000, 2_000], expected: { outcome: "within" } }, + { name: "51 files", counts: [51, 51, 0], expected: { outcome: "list-files" } }, + { name: "5,001 lines", counts: [1, 5_001, 0], expected: { outcome: "list-files" } }, + { + // GitHub reports zero counters when the diff is too large for it to compute. + name: "zero reported files", + counts: [0, 0, 0], + expected: { outcome: "manual-review", reason: "GitHub could not compute the PR's size" }, + }, + { + name: "one file under GitHub's listing cap", + counts: [2_999, 3_000, 0], + expected: { outcome: "list-files" }, + }, + { + // GitHub lists at most 3,000 files and does not say when it cut the list off. + name: "GitHub's listing cap", + counts: [3_000, 3_000, 0], + expected: { outcome: "manual-review", reason: "PR has 3000 or more changed files" }, + }, + ])("$name", ({ counts: [changed_files, additions, deletions], expected }) => { + expect(checkPullRequestSize({ changed_files, additions, deletions })).toEqual(expected); + }); +}); + +describe("addSpecificationChanges", () => { + const spec = (name: string, lines = 1): ChangedFile => ({ + filename: `specification/foo/${name}.json`, + additions: lines, + deletions: 0, + }); + const other = (name: string): ChangedFile => ({ + filename: `documentation/${name}.md`, + additions: 100, + deletions: 0, + }); + const renamed = (from: string, to: string): ChangedFile => ({ + filename: to, + previous_filename: from, + additions: 0, + deletions: 0, + }); + const times = (count: number, build: (i: number) => T) => + Array.from({ length: count }, (_, i) => build(i)); + const add = (files: ChangedFile[]) => { + const totals: SpecificationTotals = { files: 0, lines: 0 }; + return { over: addSpecificationChanges(totals, files), totals }; + }; + + it("is false at exactly the limits and true just over", () => { + expect(add(times(50, (i) => spec(`f${i}`))).over).toBe(false); + expect(add(times(51, (i) => spec(`f${i}`))).over).toBe(true); + expect(add([spec("big", 5_000)]).over).toBe(false); + expect(add([spec("big", 5_001)]).over).toBe(true); + }); + + it("does not count files or lines outside specification/", () => { + const { over, totals } = add([spec("a"), ...times(500, (i) => other(`d${i}`))]); + + expect(over).toBe(false); + expect(totals).toEqual({ files: 1, lines: 1 }); + }); + + it("accumulates across pages", () => { + const totals: SpecificationTotals = { files: 0, lines: 0 }; + + expect( + addSpecificationChanges( + totals, + times(30, (i) => spec(`a${i}`)), + ), + ).toBe(false); + expect( + addSpecificationChanges( + totals, + times(21, (i) => spec(`b${i}`)), + ), + ).toBe(true); + expect(totals.files).toBe(51); + }); + + it.each([ + { name: "moved into specification/", from: "eng", to: "specification/foo" }, + { name: "moved out of specification/", from: "specification/foo", to: "archive" }, + { name: "renamed within specification/", from: "specification/foo", to: "specification/bar" }, + ])("counts a file $name once", ({ from, to }) => { + const { over, totals } = add( + times(51, (i) => renamed(`${from}/f${i}.json`, `${to}/f${i}.json`)), + ); + + expect(over).toBe(true); + expect(totals.files).toBe(51); + }); + + it("ignores a rename that never touches specification/", () => { + const { over, totals } = add(times(51, (i) => renamed(`eng/a${i}.json`, `docs/a${i}.json`))); + + expect(over).toBe(false); + expect(totals.files).toBe(0); + }); +}); +describe("publishArmSemanticReviewStatus", () => { + it("publishes Passed for a clean reviewer result", async () => { + const { github, result } = await runPublisher(); + + expect(result).toEqual({ headSha, issueNumber, statusPublished: true }); + expect(github.rest.repos.createCommitStatus).toHaveBeenCalledWith({ + owner, + repo, + sha: headSha, + state: CommitStatusState.SUCCESS, + context: ARM_SEMANTIC_REVIEW_STATUS, + description: "Passed: full review completed with no Blocking findings", + target_url: `${runUrl}/attempts/${runAttempt}`, + }); + expect(github.rest.actions.listWorkflowRunArtifacts).toHaveBeenCalledOnce(); + expect(github.rest.actions.listWorkflowRunArtifacts).toHaveBeenCalledWith({ + owner, + repo, + run_id: runId, + per_page: PER_PAGE_MAX, + }); + }); + + it("publishes Changes requested for Blocking findings", async () => { + const { github } = await runPublisher({ + output: agentOutput({ ...passedResult, blockingCount: 3 }), + }); + + expect(github.rest.repos.createCommitStatus).toHaveBeenCalledWith( + expect.objectContaining({ + state: CommitStatusState.FAILURE, + description: "Changes requested: 3 Blocking finding(s)", + }), + ); + }); + + it("publishes Manual review required for a scoped clean review", async () => { + const { github } = await runPublisher({ + output: agentOutput({ ...passedResult, reviewScope: SemanticReviewScope.Scoped }), + }); + + expect(github.rest.repos.createCommitStatus).toHaveBeenCalledWith( + expect.objectContaining({ + state: CommitStatusState.ERROR, + description: "Manual review required: scoped review requires manual signoff", + }), + ); + }); + + describe("PR size limits", () => { + const OVER_LIMITS = + "Manual review required: specification/ changes exceed 50 files or 5000 lines"; + const spec = (name: string, lines = 1) => ({ + filename: `specification/foo/${name}.json`, + additions: lines, + deletions: 0, + }); + const doc = (name: string) => ({ + filename: `documentation/${name}.md`, + additions: 1, + deletions: 0, + }); + const times = (count: number, build: (i: number) => T) => + Array.from({ length: count }, (_, i) => build(i)); + + /** Publishes for a PR with the given totals; `files` is what `pulls.listFiles` returns. */ + async function publish( + totals: { changed_files: number; additions: number; deletions: number }, + files: unknown[] = [], + ) { + const github = createPublisherGithub(); + github.rest.pulls.get.mockResolvedValue({ data: { number: issueNumber, ...totals } }); + github.rest.pulls.listFiles.mockResolvedValue({ data: files }); + return (await runPublisher({ github })).github; + } + const published = (github: ReturnType) => + github.rest.repos.createCommitStatus.mock.calls[0][0] as { + state: string; + description: string; + }; + const publishedState = (github: ReturnType) => + published(github).state; + const publishedDescription = (github: ReturnType) => + published(github).description; + + it.each([ + { name: "a small PR", totals: { changed_files: 1, additions: 10, deletions: 5 } }, + { name: "exactly 50 files", totals: { changed_files: 50, additions: 50, deletions: 0 } }, + { + name: "exactly 5,000 lines", + totals: { changed_files: 1, additions: 3_000, deletions: 2_000 }, + }, + ])("does not list files for $name", async ({ totals }) => { + const github = await publish(totals); + + expect(github.rest.pulls.listFiles).not.toHaveBeenCalled(); + expect(publishedState(github)).toBe(CommitStatusState.SUCCESS); + }); + + it.each([ + { + name: "zero reported files", + changed_files: 0, + reason: "GitHub could not compute the PR's size", + }, + // GitHub lists at most 3,000 files and does not say when it truncated the list. + { + name: "3,000 or more files", + changed_files: 3_000, + reason: "PR has 3000 or more changed files", + }, + ])( + "requires manual review without listing files for $name", + async ({ changed_files, reason }) => { + const github = await publish({ changed_files, additions: 0, deletions: 0 }); + + expect(github.rest.pulls.listFiles).not.toHaveBeenCalled(); + expect(publishedDescription(github)).toBe(`Manual review required: ${reason}`); + }, + ); + + it("lists the files of the trusted PR when the totals are over the limits", async () => { + const github = await publish({ changed_files: 51, additions: 51, deletions: 0 }, [spec("a")]); + + expect(github.rest.pulls.listFiles).toHaveBeenCalledWith( + expect.objectContaining({ owner, repo, pull_number: issueNumber }), + ); + }); + + it("passes when only files outside specification/ push the PR over the limits", async () => { + const files = [spec("a"), ...times(60, (i) => doc(`d${i}`))]; + const github = await publish({ changed_files: 61, additions: 61, deletions: 0 }, files); + + expect(publishedState(github)).toBe(CommitStatusState.SUCCESS); + }); + + it.each([ + { name: "too many specification files", files: times(51, (i) => spec(`f${i}`)) }, + { name: "too many specification lines", files: [spec("big", 5_001)] }, + ])("requires manual review for $name", async ({ files }) => { + const additions = files.reduce((total, file) => total + file.additions, 0); + const github = await publish({ changed_files: 51, additions, deletions: 0 }, files); + + expect(publishedDescription(github)).toBe(OVER_LIMITS); + }); + + it("overrides a Changes requested result when the PR is over the limits", async () => { + // The model claimed a full review of an oversized PR, so its Blocking count is not trusted. + const github = createPublisherGithub(); + github.rest.pulls.get.mockResolvedValue({ + data: { number: issueNumber, changed_files: 51, additions: 51, deletions: 0 }, + }); + github.rest.pulls.listFiles.mockResolvedValue({ data: times(51, (i) => spec(`f${i}`)) }); + + const { github: g } = await runPublisher({ + github, + output: agentOutput({ ...passedResult, blockingCount: 2 }), + }); + expect(publishedDescription(g)).toBe(OVER_LIMITS); + }); + + it.each([ + { + name: "a scoped review", + result: { ...passedResult, reviewScope: SemanticReviewScope.Scoped }, + description: "Manual review required: scoped review requires manual signoff", + }, + { + name: "an incomplete review", + result: { + ...passedResult, + completion: SemanticReviewCompletion.Incomplete, + incompleteReason: SemanticReviewIncompleteReason.ToolFailure, + }, + description: "Review incomplete: tool-failure", + }, + ])( + "skips the size check for $name, which already withholds signoff", + async ({ result, description }) => { + const github = createPublisherGithub(); + github.rest.pulls.get.mockResolvedValue({ + data: { number: issueNumber, changed_files: 500, additions: 9_000, deletions: 0 }, + }); + + const { github: g } = await runPublisher({ github, output: agentOutput(result) }); + expect(g.rest.pulls.get).not.toHaveBeenCalled(); + expect(g.rest.pulls.listFiles).not.toHaveBeenCalled(); + expect(publishedDescription(g)).toBe(description); + }, + ); + + it("logs each step of the size decision", async () => { + const github = createPublisherGithub(); + github.rest.pulls.get.mockResolvedValue({ + data: { number: issueNumber, changed_files: 61, additions: 61, deletions: 0 }, + }); + github.rest.pulls.listFiles.mockResolvedValue({ + data: [spec("a"), ...times(60, (i) => doc(`d${i}`))], + }); + + const { core } = await runPublisher({ github }); + const logged = core.info.mock.calls.map(([message]) => String(message)); + expect(logged).toEqual( + expect.arrayContaining([ + expect.stringContaining("Reviewer reported: scope=full, completeness=complete"), + expect.stringContaining("size: 61 files, +61 -0 lines"), + expect.stringContaining("counting only the specification/ changes"), + expect.stringContaining("page 1: 1 specification/ files, 1 lines so far"), + expect.stringContaining("specification/ changes are within the limits: 1 files, 1 lines"), + ]), + ); + }); + + describe("paginating the file list", () => { + const PAGE_SIZE = 100; + const page = (start: number, count: number, prefix: string) => + Array.from({ length: count }, (_, i) => ({ + filename: `${prefix}/f${start + i}.json`, + additions: 1, + deletions: 0, + })); + + /** Serves `pages` through paginate's map function, honouring `done()` like Octokit does. */ + function withPages(pages: ReturnType[], changedFiles: number) { + const github = createPublisherGithub(); + github.rest.pulls.get.mockResolvedValue({ + data: { + number: issueNumber, + changed_files: changedFiles, + additions: changedFiles, + deletions: 0, + }, + }); + const requested: number[] = []; + const originalPaginate = github.paginate; + type MapPage = (response: { data: unknown[] }, done: () => void) => unknown[]; + github.paginate = (async (fn: unknown, params: unknown, mapFn?: MapPage) => { + if (fn !== github.rest.pulls.listFiles || !mapFn) { + return originalPaginate(fn as never, params as never); + } + const result: unknown[] = []; + let stopped = false; + for (const [index, data] of pages.entries()) { + requested.push(index); + result.push(...mapFn({ data }, () => (stopped = true))); + if (stopped) break; + } + return result; + }) as typeof github.paginate; + return { github, requested }; + } + + it("stops after the first page when its specification/ files already exceed the limits", async () => { + const { github, requested } = withPages( + [ + page(0, PAGE_SIZE, "specification/foo"), + page(PAGE_SIZE, PAGE_SIZE, "specification/foo"), + page(2 * PAGE_SIZE, 50, "specification/foo"), + ], + 250, + ); + + const { github: g } = await runPublisher({ github }); + expect(requested).toEqual([0]); + expect(publishedDescription(g)).toBe(OVER_LIMITS); + }); + + it("keeps paginating while the specification/ files seen are still within the limits", async () => { + // 250 files, but only 10 are under specification/, so no page can settle the answer early. + const { github, requested } = withPages( + [ + page(0, PAGE_SIZE, "documentation"), + page(PAGE_SIZE, PAGE_SIZE, "documentation"), + [...page(2 * PAGE_SIZE, 40, "documentation"), ...page(0, 10, "specification/foo")], + ], + 250, + ); + + const { github: g } = await runPublisher({ github }); + expect(requested).toEqual([0, 1, 2]); + expect(publishedState(g)).toBe(CommitStatusState.SUCCESS); + }); + }); + }); + it.each([ + { + name: "missing agent output", + options: { includeOutput: false }, + logged: "ENOENT", + }, + { + name: "missing semantic item", + options: { output: { items: [] } }, + logged: "Expected one ARM semantic review result, found 0", + }, + { + name: "invalid Blocking count", + options: { + output: { + items: [{ ...agentOutput().items[0], blocking_count: "not-a-number" }], + }, + }, + logged: "Invalid ARM semantic review Blocking count", + }, + ])("publishes Review incomplete for $name", async ({ options, logged }) => { + const { github, core } = await runPublisher(options); + + expect(github.rest.repos.createCommitStatus).toHaveBeenCalledWith( + expect.objectContaining({ + state: CommitStatusState.ERROR, + description: "Review incomplete: result could not be validated", + }), + ); + // The details go to the run log, not the public status description. + expect(core.warning).toHaveBeenCalledWith(expect.stringContaining(logged)); + }); + + it("publishes Review incomplete when the output path is not set", async () => { + const { github } = await runPublisher({ setOutputPath: false }); + + expect(github.rest.repos.createCommitStatus).toHaveBeenCalledWith( + expect.objectContaining({ + state: CommitStatusState.ERROR, + description: "Review incomplete: result could not be validated", + }), + ); + }); + + it("logs a thrown value that is not an Error", async () => { + const github = createPublisherGithub(); + github.rest.pulls.get.mockRejectedValue("plain string failure"); + + const { github: g, core } = await runPublisher({ github }); + expect(g.rest.repos.createCommitStatus).toHaveBeenCalledWith( + expect.objectContaining({ state: CommitStatusState.ERROR }), + ); + expect(core.warning).toHaveBeenCalledWith("plain string failure"); + }); + + it("publishes Review incomplete without exposing the error when the PR cannot be read", async () => { + const github = createPublisherGithub(); + github.rest.pulls.get.mockRejectedValue(new Error("boom: /home/runner/work/_temp/secret")); + + const { github: g, core } = await runPublisher({ github }); + expect(g.rest.repos.createCommitStatus).toHaveBeenCalledWith( + expect.objectContaining({ + state: CommitStatusState.ERROR, + description: "Review incomplete: result could not be validated", + }), + ); + expect(core.warning).toHaveBeenCalledWith(expect.stringContaining("boom")); + }); + + it("does nothing when trusted PR/SHA correlation is missing", async () => { + const github = createPublisherGithub(); + github.rest.actions.listWorkflowRunArtifacts.mockResolvedValue({ + data: { artifacts: [] }, + }); + + const execution = await runPublisher({ github }); + expect(execution.result).toEqual({ + headSha: "", + issueNumber: 0, + statusPublished: false, + }); + expect(github.rest.repos.createCommitStatus).not.toHaveBeenCalled(); + }); +}); + +describe("finalizeUnpublishedArmSemanticReview", () => { + const ownedPending = { + context: ARM_SEMANTIC_REVIEW_STATUS, + state: CommitStatusState.PENDING, + target_url: `${runUrl}/attempts/${runAttempt}`, + updated_at: "2026-01-01T00:00:00Z", + }; + + async function runFinalizer(statuses: unknown[], github = createPublisherGithub()) { + github.rest.repos.listCommitStatusesForRef.mockResolvedValue({ data: statuses }); + const previousRunAttempt = process.env.GITHUB_RUN_ATTEMPT; + process.env.GITHUB_RUN_ATTEMPT = String(runAttempt); + try { + const result = await finalizeUnpublishedArmSemanticReview({ + github, + context: { repo: { owner, repo }, runId, serverUrl: "https://github.com" }, + core: createMockCore(), + } as unknown as GitHubScriptArgs); + return { github, result }; + } finally { + if (previousRunAttempt === undefined) { + delete process.env.GITHUB_RUN_ATTEMPT; + } else { + process.env.GITHUB_RUN_ATTEMPT = previousRunAttempt; + } + } + } + + it("resolves a Pending status owned by this run as Review incomplete", async () => { + const { github, result } = await runFinalizer([ownedPending]); + + expect(result).toEqual({ statusPublished: true }); + expect(github.rest.repos.createCommitStatus).toHaveBeenCalledWith({ + owner, + repo, + sha: headSha, + state: CommitStatusState.ERROR, + context: ARM_SEMANTIC_REVIEW_STATUS, + description: "Review incomplete: reviewer did not publish a result", + target_url: `${runUrl}/attempts/${runAttempt}`, + }); + }); + + it("never publishes a manual-review hold for an unpublished result", async () => { + const { github } = await runFinalizer([ownedPending]); + + const status = github.rest.repos.createCommitStatus.mock.calls[0]?.[0] as { + description: string; + }; + expect(getSemanticReviewOutcome({ state: "error", description: status.description })).toBe( + SemanticReviewOutcome.ReviewIncomplete, + ); + }); + + it.each([ + { name: "there is no status", statuses: [] }, + { + name: "the record job already published a result", + statuses: [ + { + ...ownedPending, + state: CommitStatusState.SUCCESS, + updated_at: "2026-01-01T00:00:01Z", + }, + ownedPending, + ], + }, + { + name: "a newer run owns the newest status", + statuses: [ + { + ...ownedPending, + target_url: "https://github.com/Azure/azure-rest-api-specs/actions/runs/789/attempts/1", + updated_at: "2026-01-01T00:00:01Z", + }, + ownedPending, + ], + }, + { + name: "a newer attempt of this run owns the newest status", + statuses: [ + { + ...ownedPending, + target_url: `${runUrl}/attempts/2`, + updated_at: "2026-01-01T00:00:01Z", + }, + ownedPending, + ], + }, + ])("leaves the status unchanged when $name", async ({ statuses }) => { + const { github, result } = await runFinalizer(statuses); + + expect(result).toEqual({ statusPublished: false }); + expect(github.rest.repos.createCommitStatus).not.toHaveBeenCalled(); + }); + + it("leaves the status unchanged when the run has no trusted head SHA", async () => { + const github = createPublisherGithub(); + github.rest.actions.listWorkflowRunArtifacts.mockResolvedValue({ data: { artifacts: [] } }); + + const { result } = await runFinalizer([ownedPending], github); + + expect(result).toEqual({ statusPublished: false }); + expect(github.rest.repos.listCommitStatusesForRef).not.toHaveBeenCalled(); + expect(github.rest.repos.createCommitStatus).not.toHaveBeenCalled(); + }); +}); diff --git a/.github/workflows/test/arm-auto-signoff/arm-universal-auto-signoff.test.ts b/.github/workflows/test/arm-auto-signoff/arm-universal-auto-signoff.test.ts index fd8135fb89ec..7d44eae90df4 100644 --- a/.github/workflows/test/arm-auto-signoff/arm-universal-auto-signoff.test.ts +++ b/.github/workflows/test/arm-auto-signoff/arm-universal-auto-signoff.test.ts @@ -1,3 +1,5 @@ +import { readFile } from "node:fs/promises"; +import { join } from "node:path"; import { describe, expect, it } from "vitest"; import { CommitStatusState } from "../../../shared/src/github.ts"; import { ArmAutoSignoffLabel } from "../../src/arm-auto-signoff/arm-auto-signoff-labels.ts"; @@ -10,11 +12,19 @@ const owner = "TestOwner"; const repo = "TestRepo"; const issueNumber = 123; const headSha = "abc123"; +const GITHUB_ROOT = join(import.meta.dirname, "..", "..", ".."); -const successfulStatuses = [ +const semanticPassed = { + context: "ARM Semantic Review", + state: CommitStatusState.SUCCESS, + description: "Passed: full review completed with no Blocking findings", + updated_at: "2026-01-01", +}; +const deterministicStatuses = [ { context: "Swagger LintDiff", state: CommitStatusState.SUCCESS, updated_at: "2026-01-01" }, { context: "Swagger Avocado", state: CommitStatusState.SUCCESS, updated_at: "2026-01-01" }, ]; +const successfulStatuses = [semanticPassed, ...deterministicStatuses]; function createMockGithub({ labelNames = [], @@ -23,7 +33,7 @@ function createMockGithub({ pullRequestState = "open", }: { labelNames?: string[]; - statuses?: { context: string; state: string; updated_at: string }[]; + statuses?: { context: string; state: string; description?: string; updated_at: string }[]; currentHeadSha?: string; pullRequestState?: "open" | "closed"; } = {}) { @@ -38,7 +48,6 @@ function createMockGithub({ data: labelNames.map((name) => ({ name })), }); github.rest.repos.listCommitStatusesForRef.mockResolvedValue({ data: statuses }); - return github; } @@ -54,7 +63,35 @@ function run(github: ReturnType) { } describe("getLabelActionImpl", () => { - it("adds the pilot label when the universal requirements pass", async () => { + it("logs a no-op when an indirect trigger has no PR correlation", async () => { + const loggingCore = createMockCore(); + const github = createMockGithub(); + + const result = await getLabelActionImpl({ + owner, + repo, + head_sha: "", + issue_number: Number.NaN, + github, + core: loggingCore, + }); + + expect(result).toEqual({ + headSha: "", + issueNumber: Number.NaN, + labelActions: { + [ArmAutoSignoffLabel.ArmAutoSignedOffTest]: LabelAction.None, + [ArmAutoSignoffLabel.ArmManualSignoffRequired]: LabelAction.None, + }, + }); + expect(github.rest.pulls.get).not.toHaveBeenCalled(); + expect(loggingCore.info).toHaveBeenCalledWith( + "Universal auto-signoff no-op: missing correlation " + + "(issueNumber=missing, headSha=missing)", + ); + }); + + it("adds the pilot label when all universal requirements pass", async () => { const github = createMockGithub({ labelNames: ["ARMReview"] }); await expect(run(github)).resolves.toEqual({ @@ -62,6 +99,7 @@ describe("getLabelActionImpl", () => { issueNumber, labelActions: { [ArmAutoSignoffLabel.ArmAutoSignedOffTest]: LabelAction.Add, + [ArmAutoSignoffLabel.ArmManualSignoffRequired]: LabelAction.None, }, }); }); @@ -75,7 +113,32 @@ describe("getLabelActionImpl", () => { expect(result.labelActions[ArmAutoSignoffLabel.ArmAutoSignedOffTest]).toBe(LabelAction.None); }); - it("removes the pilot label when manual signoff is required", async () => { + it.each([ + { name: "ARMReview is missing", labelNames: [] as string[] }, + { name: "NotReadyForARMReview is present", labelNames: ["ARMReview", "NotReadyForARMReview"] }, + ])("does not read statuses and removes the pilot label when $name", async ({ labelNames }) => { + const github = createMockGithub({ + labelNames: [...labelNames, ArmAutoSignoffLabel.ArmAutoSignedOffTest], + }); + + const result = await run(github); + expect(result.labelActions).toEqual({ + [ArmAutoSignoffLabel.ArmAutoSignedOffTest]: LabelAction.Remove, + [ArmAutoSignoffLabel.ArmManualSignoffRequired]: LabelAction.None, + }); + expect(github.rest.repos.listCommitStatusesForRef).not.toHaveBeenCalled(); + }); + + it("takes no action when the PR is not ready and has no pilot label", async () => { + const github = createMockGithub({ labelNames: [] }); + + const result = await run(github); + expect(result.labelActions).toEqual({ + [ArmAutoSignoffLabel.ArmAutoSignedOffTest]: LabelAction.None, + [ArmAutoSignoffLabel.ArmManualSignoffRequired]: LabelAction.None, + }); + }); + it("treats a manual signoff label as a hard stop without removing it", async () => { const github = createMockGithub({ labelNames: [ "ARMReview", @@ -85,13 +148,279 @@ describe("getLabelActionImpl", () => { }); const result = await run(github); - expect(result.labelActions[ArmAutoSignoffLabel.ArmAutoSignedOffTest]).toBe(LabelAction.Remove); + expect(result.labelActions).toEqual({ + [ArmAutoSignoffLabel.ArmAutoSignedOffTest]: LabelAction.Remove, + [ArmAutoSignoffLabel.ArmManualSignoffRequired]: LabelAction.None, + }); + }); + + describe("auto-signoff workflow correlation", () => { + it("re-evaluates after the ARM API Reviewer publishes its status", async () => { + const workflow = await readFile( + join(GITHUB_ROOT, "workflows", "arm-universal-auto-signoff.yaml"), + "utf8", + ); + + expect(workflow).toContain('"ARM API Review: Automated Workflow"'); + expect(workflow).not.toContain("ARM Semantic Review - Set Status"); + }); + + it("publishes correlation artifacts only for workflow_run triggers", async () => { + const workflow = await readFile( + join(GITHUB_ROOT, "workflows", "arm-universal-auto-signoff.yaml"), + "utf8", + ); + const headArtifact = workflow.indexOf("name: Upload artifact with head SHA"); + const issueArtifact = workflow.indexOf("name: Upload artifact with issue number"); + + expect(headArtifact).toBeGreaterThan(-1); + expect(issueArtifact).toBeGreaterThan(headArtifact); + expect(workflow.slice(headArtifact - 180, headArtifact)).toContain( + "github.event_name == 'workflow_run'", + ); + expect(workflow.slice(issueArtifact - 180, issueArtifact)).toContain( + "github.event_name == 'workflow_run'", + ); + }); + }); + + it("does not sign off while ARMChangesRequested is present", async () => { + const github = createMockGithub({ + labelNames: ["ARMReview", "ARMChangesRequested", ArmAutoSignoffLabel.ArmAutoSignedOffTest], + }); + + const result = await run(github); + expect(result.labelActions).toEqual({ + [ArmAutoSignoffLabel.ArmAutoSignedOffTest]: LabelAction.Remove, + [ArmAutoSignoffLabel.ArmManualSignoffRequired]: LabelAction.None, + }); + }); + + it.each([ + { + name: "missing", + status: undefined, + }, + { + name: "pending", + status: { + context: "ARM Semantic Review", + state: CommitStatusState.PENDING, + description: "ARM API semantic review is pending", + updated_at: "2026-01-01", + }, + }, + { + name: "changes requested", + status: { + context: "ARM Semantic Review", + state: CommitStatusState.FAILURE, + description: "Changes requested: 1 Blocking finding(s)", + updated_at: "2026-01-01", + }, + }, + ])( + "does not sign off or require manual signoff when semantic review is $name", + async ({ status }) => { + const github = createMockGithub({ + labelNames: ["ARMReview"], + statuses: status ? [status, ...deterministicStatuses] : deterministicStatuses, + }); + + const result = await run(github); + expect(result.labelActions).toEqual({ + [ArmAutoSignoffLabel.ArmAutoSignedOffTest]: LabelAction.None, + [ArmAutoSignoffLabel.ArmManualSignoffRequired]: LabelAction.None, + }); + }, + ); + + it("does not add manual signoff when semantic review is incomplete", async () => { + const github = createMockGithub({ + labelNames: ["ARMReview"], + statuses: [ + { + context: "ARM Semantic Review", + state: CommitStatusState.ERROR, + description: "Review incomplete: workflow concluded with failure", + updated_at: "2026-01-01", + }, + ...deterministicStatuses, + ], + }); + + const result = await run(github); + expect(result.labelActions).toEqual({ + [ArmAutoSignoffLabel.ArmAutoSignedOffTest]: LabelAction.None, + [ArmAutoSignoffLabel.ArmManualSignoffRequired]: LabelAction.None, + }); + }); + + it("logs the reason when semantic review is incomplete", async () => { + const loggingCore = createMockCore(); + const github = createMockGithub({ + labelNames: ["ARMReview"], + statuses: [ + { + context: "ARM Semantic Review", + state: CommitStatusState.ERROR, + description: "Review incomplete: tool-failure", + updated_at: "2026-01-01", + }, + ...deterministicStatuses, + ], + }); + + await getLabelActionImpl({ + owner, + repo, + head_sha: headSha, + issue_number: issueNumber, + github, + core: loggingCore, + }); + + expect(loggingCore.info).toHaveBeenCalledWith( + "ARM semantic review has not passed: review-incomplete (Review incomplete: tool-failure)", + ); + }); + + it("logs the reason when semantic review requested changes", async () => { + const loggingCore = createMockCore(); + const github = createMockGithub({ + labelNames: ["ARMReview"], + statuses: [ + { + context: "ARM Semantic Review", + state: CommitStatusState.FAILURE, + description: "Changes requested: 2 Blocking finding(s)", + updated_at: "2026-01-01", + }, + ...deterministicStatuses, + ], + }); + + await getLabelActionImpl({ + owner, + repo, + head_sha: headSha, + issue_number: issueNumber, + github, + core: loggingCore, + }); + + expect(loggingCore.info).toHaveBeenCalledWith( + "ARM semantic review has not passed: changes-requested (Changes requested: 2 Blocking finding(s))", + ); + }); + + it("logs a missing semantic review status", async () => { + const loggingCore = createMockCore(); + const github = createMockGithub({ + labelNames: ["ARMReview"], + statuses: deterministicStatuses, + }); + + await getLabelActionImpl({ + owner, + repo, + head_sha: headSha, + issue_number: issueNumber, + github, + core: loggingCore, + }); + + expect(loggingCore.info).toHaveBeenCalledWith("ARM semantic review has not passed: missing"); + }); + + it("logs why an existing manual signoff label produces no new action", async () => { + const loggingCore = createMockCore(); + const github = createMockGithub({ + labelNames: ["ARMReview", ArmAutoSignoffLabel.ArmManualSignoffRequired], + statuses: [ + { + context: "ARM Semantic Review", + state: CommitStatusState.ERROR, + description: "Manual review required: PR exceeds automated review size limits", + updated_at: "2026-01-01", + }, + ...deterministicStatuses, + ], + }); + + const result = await getLabelActionImpl({ + owner, + repo, + head_sha: headSha, + issue_number: issueNumber, + github, + core: loggingCore, + }); + + expect(result.labelActions).toEqual({ + [ArmAutoSignoffLabel.ArmAutoSignedOffTest]: LabelAction.None, + [ArmAutoSignoffLabel.ArmManualSignoffRequired]: LabelAction.None, + }); + expect(loggingCore.info).toHaveBeenCalledWith( + expect.stringContaining('"isReadyForArmReview":true'), + ); + expect(loggingCore.info).toHaveBeenCalledWith( + expect.stringContaining( + '"description":"Manual review required: PR exceeds automated review size limits"', + ), + ); + expect(loggingCore.info).toHaveBeenCalledWith( + "ARM semantic review requires manual signoff; ARMManualSignoffRequired is already present", + ); + expect(loggingCore.info).toHaveBeenCalledWith( + 'Universal auto-signoff label actions: {"ARMAutoSignedOff-Test":"none","ARMManualSignoffRequired":"none"}', + ); + }); + + it("adds manual signoff when automated coverage was scoped", async () => { + const github = createMockGithub({ + labelNames: ["ARMReview"], + statuses: [ + { + context: "ARM Semantic Review", + state: CommitStatusState.ERROR, + description: "Manual review required: automated review was scoped", + updated_at: "2026-01-01", + }, + ...deterministicStatuses, + ], + }); + + const result = await run(github); + expect(result.labelActions).toEqual({ + [ArmAutoSignoffLabel.ArmAutoSignedOffTest]: LabelAction.None, + [ArmAutoSignoffLabel.ArmManualSignoffRequired]: LabelAction.Add, + }); + }); + + it("adds manual signoff for scoped coverage even when changes were requested", async () => { + const github = createMockGithub({ + labelNames: ["ARMReview", "ARMChangesRequested"], + statuses: [ + { + context: "ARM Semantic Review", + state: CommitStatusState.ERROR, + description: "Manual review required: automated review was scoped", + updated_at: "2026-01-01", + }, + ...deterministicStatuses, + ], + }); + + const result = await run(github); + expect(result.labelActions[ArmAutoSignoffLabel.ArmManualSignoffRequired]).toBe(LabelAction.Add); }); - it("removes the pilot label while a mandatory status is pending", async () => { + it("removes the pilot label while a deterministic status is pending", async () => { const github = createMockGithub({ labelNames: ["ARMReview", ArmAutoSignoffLabel.ArmAutoSignedOffTest], statuses: [ + semanticPassed, { context: "Swagger LintDiff", state: CommitStatusState.SUCCESS, updated_at: "2026-01-01" }, { context: "Swagger Avocado", state: CommitStatusState.PENDING, updated_at: "2026-01-01" }, ], @@ -101,7 +430,7 @@ describe("getLabelActionImpl", () => { expect(result.labelActions[ArmAutoSignoffLabel.ArmAutoSignedOffTest]).toBe(LabelAction.Remove); }); - it("removes the pilot label when a suppression lacks approval", async () => { + it("removes the pilot label when suppression approval is missing", async () => { const github = createMockGithub({ labelNames: [ "ARMReview", diff --git a/.github/workflows/test/context.test.ts b/.github/workflows/test/context.test.ts index df38e667edbe..0ff31d1628ff 100644 --- a/.github/workflows/test/context.test.ts +++ b/.github/workflows/test/context.test.ts @@ -221,6 +221,192 @@ describe("extractInputs", () => { }, ); + it("workflow_run:completed:pull_request_target uses trusted correlation artifacts", async () => { + const github = createMockGithub(); + github.rest.actions.listWorkflowRunArtifacts.mockResolvedValue({ + data: { + artifacts: [{ name: `head-sha=${fullGitSha}` }, { name: "issue-number=123" }], + }, + }); + const context = { + eventName: "workflow_run", + payload: { + action: "completed", + workflow_run: { + event: "pull_request_target", + head_sha: "payload-sha", + id: 456, + repository: { + name: "TestRepoName", + owner: { + login: "TestRepoOwnerLogin", + }, + }, + pull_requests: [{ number: 123 }], + }, + }, + }; + + await expect(extractInputs(github, context, createMockCore())).resolves.toEqual({ + owner: "TestRepoOwnerLogin", + repo: "TestRepoName", + head_sha: fullGitSha, + issue_number: 123, + run_id: 456, + }); + expect(github.rest.actions.listWorkflowRunArtifacts).toHaveBeenCalledWith({ + owner: "TestRepoOwnerLogin", + repo: "TestRepoName", + run_id: 456, + per_page: PER_PAGE_MAX, + }); + }); + + it("workflow_run:completed:pull_request_target keeps the payload SHA when artifacts are missing", async () => { + const github = createMockGithub(); + github.rest.actions.listWorkflowRunArtifacts.mockResolvedValue({ + data: { artifacts: [] }, + }); + const context = { + eventName: "workflow_run", + payload: { + action: "completed", + workflow_run: { + event: "pull_request_target", + head_sha: "payload-sha", + id: 456, + repository: { + name: "TestRepoName", + owner: { + login: "TestRepoOwnerLogin", + }, + }, + pull_requests: [{ number: 123 }], + }, + }, + }; + + await expect(extractInputs(github, context, createMockCore())).resolves.toEqual({ + owner: "TestRepoOwnerLogin", + repo: "TestRepoName", + head_sha: "payload-sha", + issue_number: 123, + run_id: 456, + }); + }); + + it("workflow_run:completed:workflow_dispatch uses trusted correlation artifacts", async () => { + const github = createMockGithub(); + github.rest.actions.listWorkflowRunArtifacts.mockResolvedValue({ + data: { + artifacts: [{ name: `head-sha=${fullGitSha}` }, { name: "issue-number=123" }], + }, + }); + const context = { + eventName: "workflow_run", + payload: { + action: "completed", + workflow_run: { + event: "workflow_dispatch", + head_sha: "default-branch-sha", + id: 456, + repository: { + name: "TestRepoName", + owner: { + login: "TestRepoOwnerLogin", + }, + }, + pull_requests: [], + }, + }, + }; + + await expect(extractInputs(github, context, createMockCore())).resolves.toEqual({ + owner: "TestRepoOwnerLogin", + repo: "TestRepoName", + head_sha: fullGitSha, + issue_number: 123, + run_id: 456, + }); + }); + + it("workflow_run:completed:workflow_dispatch does not fall back to the payload SHA when artifacts are missing", async () => { + const github = createMockGithub(); + github.rest.actions.listWorkflowRunArtifacts.mockResolvedValue({ + data: { artifacts: [] }, + }); + const context = { + eventName: "workflow_run", + payload: { + action: "completed", + workflow_run: { + event: "workflow_dispatch", + head_sha: "default-branch-sha", + id: 456, + repository: { + name: "TestRepoName", + owner: { + login: "TestRepoOwnerLogin", + }, + }, + pull_requests: [], + }, + }, + }; + + await expect(extractInputs(github, context, createMockCore())).resolves.toEqual({ + owner: "TestRepoOwnerLogin", + repo: "TestRepoName", + head_sha: "", + issue_number: NaN, + run_id: 456, + }); + expect(github.rest.pulls.get).not.toHaveBeenCalled(); + }); + + it("rejects conflicting workflow correlation artifacts", async () => { + const github = createMockGithub(); + const context = { + eventName: "workflow_run", + payload: { + action: "completed", + workflow_run: { + event: "workflow_dispatch", + head_sha: "default-branch-sha", + id: 456, + repository: { + name: "TestRepoName", + owner: { + login: "TestRepoOwnerLogin", + }, + }, + pull_requests: [], + }, + }, + }; + + github.rest.actions.listWorkflowRunArtifacts.mockResolvedValue({ + data: { + artifacts: [ + { name: `head-sha=${fullGitSha}` }, + { name: "head-sha=abcdefabcdefabcdefabcdefabcdefabcdefabcd" }, + ], + }, + }); + await expect(extractInputs(github, context, createMockCore())).rejects.toThrow( + "Conflicting head-sha artifacts", + ); + + github.rest.actions.listWorkflowRunArtifacts.mockResolvedValue({ + data: { + artifacts: [{ name: "issue-number=123" }, { name: "issue-number=456" }], + }, + }); + await expect(extractInputs(github, context, createMockCore())).rejects.toThrow( + "Conflicting issue-number artifacts", + ); + }); + it.each(["repository", "head_repository"])( "rejects a workflow run with a null %s owner", async (missingOwner) => { @@ -467,6 +653,57 @@ describe("extractInputs", () => { }); }); + it("workflow_run:completed:workflow_run reads both values from one combined artifact", async () => { + const github = createMockGithub(); + github.rest.actions.listWorkflowRunArtifacts.mockResolvedValue({ + data: { + artifacts: [{ name: `head-sha=${fullGitSha};issue-number=123` }, { name: "unrelated=1" }], + }, + }); + + const context = { + eventName: "workflow_run", + payload: { + action: "completed", + workflow_run: { + event: "workflow_run", + head_sha: "def456", + id: 456, + repository: { name: "TestRepoName", owner: { login: "TestRepoOwnerLogin" } }, + }, + }, + }; + + await expect(extractInputs(github, context, createMockCore())).resolves.toEqual({ + owner: "TestRepoOwnerLogin", + repo: "TestRepoName", + head_sha: fullGitSha, + issue_number: 123, + run_id: 456, + }); + + // A combined artifact is validated exactly like separate ones. + github.rest.actions.listWorkflowRunArtifacts.mockResolvedValue({ + data: { artifacts: [{ name: "head-sha=not-full-git-sha;issue-number=123" }] }, + }); + await expect(extractInputs(github, context, createMockCore())).rejects.toThrow( + "head-sha is not a valid full git SHA", + ); + + // It also conflicts with a separate artifact that disagrees. + github.rest.actions.listWorkflowRunArtifacts.mockResolvedValue({ + data: { + artifacts: [ + { name: `head-sha=${fullGitSha};issue-number=123` }, + { name: "issue-number=456" }, + ], + }, + }); + await expect(extractInputs(github, context, createMockCore())).rejects.toThrow( + "Conflicting issue-number artifacts", + ); + }); + it("workflow_run:completed:unsupported", async () => { const context = { eventName: "workflow_run", diff --git a/.github/workflows/test/mocks.ts b/.github/workflows/test/mocks.ts index ea08d797907a..1142002d9326 100644 --- a/.github/workflows/test/mocks.ts +++ b/.github/workflows/test/mocks.ts @@ -22,9 +22,14 @@ function createMockGithubImpl() { paginate: async ( func: (input: T) => Promise<{ data: Array | Record> }>, params: T, + mapFn?: (response: { data: Array | Record> }, done: () => void) => U[], ) => { // Assume all test data fits in single page - const data = (await func(params)).data; + const response = await func(params); + if (mapFn) { + return mapFn(response, () => {}); + } + const data = response.data; // Simulate normalization performed by real impl return Array.isArray(data) ? data : data[Object.keys(data)[0]]; @@ -51,6 +56,7 @@ function createMockGithubImpl() { }, pulls: { get: vi.fn(), + listFiles: vi.fn().mockResolvedValue({ data: [] }), }, repos: { createCommitStatus: vi.fn(),