diff --git a/.github/scripts/pr-triage-act.sh b/.github/scripts/pr-triage-act.sh index 41732125ef..9e50a0273a 100644 --- a/.github/scripts/pr-triage-act.sh +++ b/.github/scripts/pr-triage-act.sh @@ -183,24 +183,23 @@ eval_status_state() { eval_run_exists_for_head() { # Decide whether a *real* evaluation has already run (or is running) for the - # current head_sha. Filtering by event alone is not reliable: every PR push - # produces placeholder status runs (`pr-status` on pull_request, and + # current head, so we never trigger a duplicate. + # + # Path 1 — runs whose head_sha == the PR head. These come from the /evaluate + # comment (issue_comment) and from a human applying the evaluate-now label + # (pull_request_target). Filtering by event alone is not reliable: every PR + # push also produces placeholder status runs (`pr-status` on pull_request, and # `fork-pr-status` on pull_request_target — the latter concludes "success", - # not "skipped"), so an event/conclusion filter would count those and we'd - # never apply the evaluate-now label. See + # not "skipped"), so an event/conclusion filter would count those. See # https://github.com/dotnet/skills/pull/703 and - # https://github.com/dotnet/skills/pull/720 for the original repro. - # - # The reliable discriminator is whether the run actually executed the `gate` - # job (the /evaluate and evaluate-now label entry points) or the `discover` - # job (manual/scheduled runs). In placeholder status runs both of those jobs - # are present but conclusion=="skipped"; in a real evaluation at least one of - # them is non-skipped (success/failure, or null while still in progress). - local run_ids + # https://github.com/dotnet/skills/pull/720 for the original repro. The + # reliable discriminator is whether the run actually executed the `gate` job + # or the `discover` job: in placeholder runs both are present but + # conclusion=="skipped"; in a real evaluation at least one is non-skipped + # (success/failure, or null while still in progress). + local run_ids id real run_ids=$(gh api --paginate "repos/$REPO/actions/workflows/evaluation.yml/runs?head_sha=$HEAD_SHA" \ --jq '.workflow_runs[].id') - [ -z "$run_ids" ] && return 1 - local id real for id in $run_ids; do real=$(gh api --paginate "repos/$REPO/actions/runs/$id/jobs" \ --jq '[.jobs[] | select((.name == "gate" or .name == "discover") and .conclusion != "skipped")] | length' \ @@ -209,6 +208,18 @@ eval_run_exists_for_head() { return 0 fi done + # Path 2 — runs this worker dispatched via workflow_dispatch. Those execute + # against the default branch, so their head_sha is main's HEAD (not the PR + # head) and Path 1 cannot see them. Match them instead by the deterministic + # run name evaluation.yml derives from the pr_number/head_sha dispatch inputs + # ("Evaluate PR # @ "). A new push changes , so a stale run for + # an older head never masks a head that still needs evaluation. The 100 + # most-recent dispatch runs are ample given the hourly triage cadence. + local dispatched + dispatched=$(gh api "repos/$REPO/actions/workflows/evaluation.yml/runs?event=workflow_dispatch&per_page=100" \ + --jq ".workflow_runs[] | select(.display_title == \"Evaluate PR #$PR_NUMBER @ $HEAD_SHA_SHORT\") | .id" \ + | head -n 1) + [ -n "$dispatched" ] && return 0 return 1 } @@ -379,21 +390,31 @@ ping_age_gate_ok() { # ---------------------------------------------------------------------- do_eval_trigger() { if eval_run_exists_for_head; then - log "eval-trigger: a workflow run already exists for $HEAD_SHA_SHORT — skipping" - return - fi - if has_label "evaluate-now"; then - log "eval-trigger: evaluate-now label already present — skipping" + log "eval-trigger: an evaluation run already exists for $HEAD_SHA_SHORT — skipping" return fi if [ "$DRY_RUN" = "true" ]; then - log "[DRY_RUN] would add label 'evaluate-now'" + log "[DRY_RUN] would dispatch evaluation.yml for PR #$PR_NUMBER @ $HEAD_SHA_SHORT" summary " - action: eval-trigger (DRY_RUN)" return fi - gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label "evaluate-now" >/dev/null - log "eval-trigger: added 'evaluate-now' label" - summary " - action: eval-trigger" + # Trigger evaluation by dispatching evaluation.yml directly. We deliberately do + # NOT add the `evaluate-now` label here: label events emitted by this workflow's + # GITHUB_TOKEN do not start new workflow runs (GitHub's recursion guard), so the + # pull_request_target:[labeled] entry point never fires for the bot. + # workflow_dispatch is exempt from that guard, so it does fire. The label + # remains a valid *human* entry point and is still honoured by evaluation.yml. + # The head_sha (short) feeds evaluation.yml's run name, which is the idempotency + # key eval_run_exists_for_head matches on (Path 2). + if gh workflow run evaluation.yml --repo "$REPO" \ + -f pr_number="$PR_NUMBER" \ + -f head_sha="$HEAD_SHA_SHORT" >/dev/null; then + log "eval-trigger: dispatched evaluation.yml for PR #$PR_NUMBER @ $HEAD_SHA_SHORT" + summary " - action: eval-trigger (dispatched evaluation.yml)" + else + echo "::warning::eval-trigger: failed to dispatch evaluation.yml for PR #$PR_NUMBER" >&2 + summary " - action: eval-trigger (dispatch FAILED)" + fi } # ---------------------------------------------------------------------- diff --git a/.github/workflows/evaluation.yml b/.github/workflows/evaluation.yml index e18a0e9ab8..e1532e5c01 100644 --- a/.github/workflows/evaluation.yml +++ b/.github/workflows/evaluation.yml @@ -25,23 +25,45 @@ # - Secret access: only users with write+ permission can trigger evaluation name: evaluation +# For a triage/manual single-PR dispatch, surface the PR + head in the run name. +# This run name also doubles as the idempotency key the PR-triage worker matches +# on (eval_run_exists_for_head in pr-triage-act.sh): a workflow_dispatch run's +# head_sha is the default branch — not the PR head — so the run name is how the +# worker recognises that an evaluation has already been dispatched for this head. +# For all other events the expression yields '' and GitHub uses the default name. +run-name: ${{ inputs.pr_number != '' && format('Evaluate PR #{0} @ {1}', inputs.pr_number, inputs.head_sha) || '' }} + on: # Manual trigger for one-off deploys (e.g., AGENTVIZ SPA update) and # targeted re-runs of a single plugin without committing to main. + # The pr_number/head_sha inputs are also the PR-triage worker's entry point: + # the worker dispatches this workflow directly (workflow_dispatch is exempt + # from GitHub's GITHUB_TOKEN recursion guard) instead of relying on the + # `evaluate-now` label, which never fires when applied by the bot. workflow_dispatch: inputs: plugin: description: "Specific plugin to evaluate (leave blank for all)" type: string required: false + pr_number: + description: "PR number to evaluate. Routes the dispatch through the gate/PR pipeline instead of a full/plugin run." + type: string + required: false + head_sha: + description: "Short head SHA of the PR (informational; used for the run name and triage idempotency). The gate re-fetches the live head." + type: string + required: false # Same-repo PRs: post initial status pull_request: # Fork PRs: post initial status (runs from base branch for security). - # Also receives label events: the `evaluate-now` label is the second - # entry point into this pipeline (alongside `/evaluate`), driven by the - # PR triage workflow. + # Also receives `labeled` events: a human applying the `evaluate-now` label + # is one entry point into this pipeline (alongside `/evaluate`). The triage + # worker runs as github-actions[bot] and cannot use the label (label events + # emitted by GITHUB_TOKEN do not start workflows), so it uses the + # workflow_dispatch pr_number input above instead. pull_request_target: types: [opened, synchronize, reopened, labeled] @@ -54,10 +76,11 @@ on: - cron: '0 0 * * *' # Once daily at midnight UTC concurrency: - # Both /evaluate (issue_comment) and the `evaluate-now` label - # (pull_request_target labeled) share the same `eval-` group so - # a race between them collapses to a single run for the PR. - group: ${{ github.workflow }}-${{ (github.event_name == 'issue_comment' && startsWith(github.event.comment.body, '/evaluate')) && format('eval-{0}', github.event.issue.number) || (github.event_name == 'pull_request_target' && github.event.action == 'labeled' && github.event.label.name == 'evaluate-now') && format('eval-{0}', github.event.pull_request.number) || (github.event_name == 'issue_comment' && format('eval-noop-{0}-{1}', github.event.issue.number, github.event.comment.id)) || (github.event_name == 'pull_request' && format('eval-status-{0}', github.event.pull_request.number)) || (github.event_name == 'pull_request_target' && format('eval-fork-status-{0}', github.event.pull_request.number)) || github.run_id }} + # /evaluate (issue_comment), the human-applied `evaluate-now` label + # (pull_request_target labeled), and the triage worker's workflow_dispatch + # (pr_number input) all share the same `eval-` group, so a race + # between any of them collapses to a single run for the PR. + group: ${{ github.workflow }}-${{ (github.event_name == 'issue_comment' && startsWith(github.event.comment.body, '/evaluate')) && format('eval-{0}', github.event.issue.number) || (github.event_name == 'pull_request_target' && github.event.action == 'labeled' && github.event.label.name == 'evaluate-now') && format('eval-{0}', github.event.pull_request.number) || (github.event_name == 'workflow_dispatch' && inputs.pr_number != '') && format('eval-{0}', inputs.pr_number) || (github.event_name == 'issue_comment' && format('eval-noop-{0}-{1}', github.event.issue.number, github.event.comment.id)) || (github.event_name == 'pull_request' && format('eval-status-{0}', github.event.pull_request.number)) || (github.event_name == 'pull_request_target' && format('eval-fork-status-{0}', github.event.pull_request.number)) || github.run_id }} cancel-in-progress: true env: @@ -209,12 +232,15 @@ jobs: # ========================================================================== # GATE JOB - # Validate evaluation trigger. Two entry points: + # Validate evaluation trigger. Three entry points: # 1. /evaluate comment (issue_comment) — original human path - # 2. evaluate-now label (pull_request_target labeled) — driven by - # pr-triage worker (or applied manually by a maintainer) - # Both must come from a trusted actor (write+ on the repo, or the - # workflow's own github-actions[bot] identity for the label path). + # 2. evaluate-now label (pull_request_target labeled) — a human applying + # the label (the bot cannot: GITHUB_TOKEN label events don't start runs) + # 3. workflow_dispatch with pr_number — the pr-triage worker's path. It runs + # as github-actions[bot] and dispatches this workflow directly, since + # workflow_dispatch is exempt from the GITHUB_TOKEN recursion guard. + # All must come from a trusted actor (write+ on the repo, or the workflow's + # own github-actions[bot] identity for paths 2 and 3). # ========================================================================== gate: if: >- @@ -224,6 +250,9 @@ jobs: (github.event_name == 'pull_request_target' && github.event.action == 'labeled' && github.event.label.name == 'evaluate-now') + || + (github.event_name == 'workflow_dispatch' && + inputs.pr_number != '') runs-on: ubuntu-latest permissions: contents: read @@ -246,9 +275,11 @@ jobs: else ACTOR='${{ github.event.sender.login }}' fi - # The triage worker applies the evaluate-now label as github-actions[bot]. - # That identity has no entry in /collaborators/* but is implicitly trusted - # because only this workflow's own GITHUB_TOKEN can act under it. + # The triage worker dispatches this workflow as github-actions[bot] + # (workflow_dispatch). A human may instead apply the evaluate-now label, + # which arrives under that human's identity. The bot identity has no + # entry in /collaborators/* but is implicitly trusted because only this + # repository's own GITHUB_TOKEN can act under it. if [[ "$ACTOR" == "github-actions[bot]" ]]; then echo "Actor is github-actions[bot] — trusted by construction" exit 0 @@ -264,9 +295,19 @@ jobs: id: pr env: GH_TOKEN: ${{ github.token }} + # Read the dispatch input via env (not direct interpolation) so the + # numeric validation below runs before the value reaches any gh api + # call — a crafted input cannot inject shell or path traversal. + PR_NUMBER_INPUT: ${{ inputs.pr_number }} run: | if [[ "${{ github.event_name }}" == "issue_comment" ]]; then PR_NUMBER='${{ github.event.issue.number }}' + elif [[ "${{ github.event_name }}" == "workflow_dispatch" ]]; then + PR_NUMBER="$PR_NUMBER_INPUT" + if [[ ! "$PR_NUMBER" =~ ^[0-9]+$ ]]; then + echo "::error::workflow_dispatch input pr_number='$PR_NUMBER' must be a positive integer" + exit 1 + fi else PR_NUMBER='${{ github.event.pull_request.number }}' fi @@ -296,15 +337,17 @@ jobs: -X POST -f content='eyes' || true - name: Remove evaluate-now label - if: github.event_name == 'pull_request_target' + if: github.event_name == 'pull_request_target' || github.event_name == 'workflow_dispatch' continue-on-error: true env: GH_TOKEN: ${{ github.token }} run: | - # Remove the label as the first thing we do so the trigger is - # consumed and re-applying re-fires. Removal is performed via the - # workflow's own GITHUB_TOKEN; per GitHub's recursion rules, events - # emitted by GITHUB_TOKEN do not trigger new workflow runs. + # For the label entry point, removing the label consumes the trigger so + # re-applying re-fires. For the workflow_dispatch entry point there is no + # label to consume, but we still strip any stale human-applied + # evaluate-now label so it doesn't linger. Removal uses the workflow's + # own GITHUB_TOKEN; per GitHub's recursion rules, label events emitted by + # GITHUB_TOKEN do not start new workflow runs. gh pr edit "${{ steps.pr.outputs.pr_number }}" \ --repo "${{ github.repository }}" \ --remove-label "evaluate-now" || true @@ -328,7 +371,7 @@ jobs: needs: gate if: >- always() && - (needs.gate.result == 'success' || github.event_name == 'schedule' || github.event_name == 'workflow_dispatch') && + (needs.gate.result == 'success' || github.event_name == 'schedule' || (github.event_name == 'workflow_dispatch' && inputs.pr_number == '')) && (github.event_name != 'schedule' || github.repository == 'dotnet/skills') runs-on: ubuntu-latest permissions: diff --git a/.github/workflows/pr-triage.yml b/.github/workflows/pr-triage.yml index 2188c2491f..80ea0c0958 100644 --- a/.github/workflows/pr-triage.yml +++ b/.github/workflows/pr-triage.yml @@ -39,7 +39,10 @@ permissions: pull-requests: write issues: write statuses: read - actions: read + # actions: write lets the worker dispatch evaluation.yml via `gh workflow run` + # (the GITHUB_TOKEN-exempt trigger that replaces the bot-applied `evaluate-now` + # label). It also covers the runs/jobs reads in eval_run_exists_for_head. + actions: write concurrency: group: pr-triage-${{ inputs.pr_number }} diff --git a/docs/design/pr-triage-workflows.md b/docs/design/pr-triage-workflows.md index 339c8db8d6..4be66c9bbb 100644 --- a/docs/design/pr-triage-workflows.md +++ b/docs/design/pr-triage-workflows.md @@ -8,7 +8,7 @@ Three GitHub Actions workflows keep open PRs moving without manual nudging: calls. - **`pr-triage.yml`** — per-PR worker (`workflow_dispatch`). Re-validates the PR's state, reconciles a single `pr-state/*` label, and performs at most one - of: trigger evaluation (via the `evaluate-now` label), ping the author, or + of: trigger evaluation (by dispatching `evaluation.yml`), ping the author, or ping maintainers. Cool-down (default 4 days) is enforced via marker comments. - **`pr-malicious-scan.agent.md`** — per-PR malicious-code scanner (gh-aw). Static diff review for untrusted contributors. Reports findings as @@ -21,19 +21,28 @@ flowchart TD Cron["cron: every hour"] --> Batch["pr-triage-batch.yml
(orchestrator)"] Batch -->|workflow_dispatch| Worker["pr-triage.yml
(per-PR worker)"] Batch -->|workflow_dispatch| Scan["pr-malicious-scan.agent.lock.yml
(per-PR scanner)"] - Worker -->|adds 'evaluate-now' label| Eval["evaluation.yml
(existing)"] + Worker -->|workflow_dispatch: pr_number| Eval["evaluation.yml
(existing)"] Worker -->|adds pr-state/* label| PR[("PR")] Worker -->|posts ping comment| PR Scan -->|code-scanning alert + comment| PR - PR -.->|labeled: evaluate-now| Eval + PR -.->|human adds label: evaluate-now| Eval ``` ## Entry points into `evaluation.yml` -The existing `/evaluate` slash command continues to work. In addition, applying -the **`evaluate-now`** label fires evaluation via `pull_request_target [labeled]`. -Both paths share a per-PR concurrency group so a race collapses to a single run. -The label is consumed (removed) by the `gate` job so reapplying re-fires. +Three entry points feed the `gate` job, all sharing a per-PR concurrency group +so a race collapses to a single run: + +1. The existing **`/evaluate`** slash command (`issue_comment`) — humans. +2. The **`evaluate-now`** label (`pull_request_target [labeled]`) — humans. The + `gate` job consumes (removes) the label so reapplying re-fires. +3. **`workflow_dispatch`** with a `pr_number` input — the triage worker. The + worker runs as `github-actions[bot]`, and label events emitted by + `GITHUB_TOKEN` do **not** start workflows (GitHub's recursion guard), so the + bot cannot use entry point 2. `workflow_dispatch` is exempt from that guard, + so the worker dispatches `evaluation.yml` directly. A dispatched run's + `head_sha` is the default branch (not the PR head), so the worker matches the + run by `evaluation.yml`'s run name (`Evaluate PR # @ `) for idempotency. ## State machine (worker)