Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 44 additions & 23 deletions .github/scripts/pr-triage-act.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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' \
Expand All @@ -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 #<n> @ <sha7>"). A new push changes <sha7>, 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
}

Expand Down Expand Up @@ -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
}

# ----------------------------------------------------------------------
Expand Down
85 changes: 64 additions & 21 deletions .github/workflows/evaluation.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand All @@ -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-<pr_number>` 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-<pr_number>` 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:
Expand Down Expand Up @@ -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: >-
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Comment thread
JanKrivanek marked this conversation as resolved.
else
PR_NUMBER='${{ github.event.pull_request.number }}'
fi
Expand Down Expand Up @@ -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
Expand All @@ -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:
Expand Down
5 changes: 4 additions & 1 deletion .github/workflows/pr-triage.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
Expand Down
23 changes: 16 additions & 7 deletions docs/design/pr-triage-workflows.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -21,19 +21,28 @@ flowchart TD
Cron["cron: every hour"] --> Batch["pr-triage-batch.yml<br/>(orchestrator)"]
Batch -->|workflow_dispatch| Worker["pr-triage.yml<br/>(per-PR worker)"]
Batch -->|workflow_dispatch| Scan["pr-malicious-scan.agent.lock.yml<br/>(per-PR scanner)"]
Worker -->|adds 'evaluate-now' label| Eval["evaluation.yml<br/>(existing)"]
Worker -->|workflow_dispatch: pr_number| Eval["evaluation.yml<br/>(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 #<n> @ <sha7>`) for idempotency.

## State machine (worker)

Expand Down