Skip to content

Require explicit /evaluate for all PRs, add message to that effect #363

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
danmoseley wants to merge 3 commits into from
Closed

Require explicit /evaluate for all PRs, add message to that effect #363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bff8af6
Require explicit /evaluate for all PRs
Mar 14, 2026
b4bcffc
Address review feedback: permissions, concurrency, detection, pinning
Mar 14, 2026
751aadb
Handle infra changes in /evaluate discover step
Mar 14, 2026
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
108 changes: 64 additions & 44 deletions .github/workflows/evaluation-fork-pr.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,28 @@
# Evaluation workflow for fork PRs.
# Evaluation workflow for pull requests.
#
# Fork PRs cannot access repository secrets via the `pull_request` trigger.
# This workflow uses `issue_comment` so that a maintainer can trigger evaluation
# by posting "/evaluate" on a fork PR.
# All PRs require an explicit `/evaluate` command to trigger evaluation.
# This avoids automatic evaluation runs on every push, reducing token consumption.
#
# The `pull_request_target` trigger handles initial status posting for fork PRs:
# The `pull_request_target` trigger handles initial status posting for all PRs:
# it runs discovery (from the base branch, safe) and posts a commit status so
# the required "evaluation-status" check is never stuck as "Expected".
Comment thread
danmoseley marked this conversation as resolved.
#
# Security model:
# - Workflow YAML: always from the base branch (enforced by issue_comment
# and pull_request_target triggers)
# - Validator binary: built from the base branch checkout (trusted)
# - Skill/test content: checked out from the fork PR (untrusted data, read-only)
# - Skill/test content: checked out from the PR head (untrusted for forks, read-only)
# - Secret access: only users with write+ permission can trigger evaluation
name: evaluation (fork PRs)
name: evaluation (PRs)

on:
issue_comment:
types: [created]
pull_request_target:
Comment thread
danmoseley marked this conversation as resolved.
types: [opened, synchronize, reopened]

concurrency:
group: ${{ github.event_name == 'issue_comment' && (startsWith(github.event.comment.body, '/evaluate') && format('eval-fork-{0}', github.event.issue.number) || format('eval-fork-{0}-noop-{1}', github.event.issue.number, github.event.comment.id)) || format('eval-fork-status-{0}', github.event.pull_request.number) }}
group: ${{ github.event_name == 'issue_comment' && (startsWith(github.event.comment.body, '/evaluate') && format('eval-pr-{0}', github.event.issue.number) || format('eval-pr-{0}-noop-{1}', github.event.issue.number, github.event.comment.id)) || format('eval-pr-status-{0}', github.event.pull_request.number) }}
cancel-in-progress: true

# pull-requests: write is required at the workflow level because the reusable
Expand All @@ -35,23 +35,24 @@ permissions:
statuses: write

jobs:
# Post initial evaluation commit status for fork PRs so the required check
# Post initial evaluation commit status for all PRs so the required check
# is never stuck as "Expected". Posts success (no skills) or pending (needs /evaluate).
# Uses pull_request_target so it runs with base repo permissions.
fork-status:
pr-status:
if: >-
github.event_name == 'pull_request_target' &&
github.event.pull_request.head.repo.full_name != github.repository
github.event_name == 'pull_request_target'
runs-on: ubuntu-latest
permissions:
contents: read
pull-requests: write
statuses: write
steps:
- name: Checkout base branch
Comment thread
danmoseley marked this conversation as resolved.
uses: actions/checkout@v6
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6
with:
ref: ${{ github.event.pull_request.base.sha }}
fetch-depth: 0
persist-credentials: false

- name: Fetch PR head (metadata only)
run: git fetch origin +refs/pull/${{ github.event.pull_request.number }}/head
Expand All @@ -66,7 +67,7 @@ jobs:
$changedFiles = git diff --name-only --diff-filter=ACMR $mergeBase $head

$hasSkillChanges = $changedFiles |
Where-Object { $_ -match '^(plugins/[^/]+/skills|tests/[^/]+)/[^/]+/' } |
Where-Object { $_ -match '^(plugins/[^/]+/skills|tests/[^/]+)/[^/]+/|^(\.github/workflows/evaluation(-run|-fork-pr)?\.yml$|eng/skill-validator/)' } |
Comment thread
danmoseley marked this conversation as resolved.
Select-Object -First 1

if ($hasSkillChanges) {
Expand All @@ -81,7 +82,7 @@ jobs:
run: |
if [[ "${{ steps.discover.outputs.has_skills }}" == "true" ]]; then
STATE="pending"
DESC="Fork PR — post /evaluate to trigger evaluation"
DESC="Post /evaluate to trigger evaluation"
else
STATE="success"
DESC="No skills to evaluate"
Comment thread
danmoseley marked this conversation as resolved.
Comment thread
danmoseley marked this conversation as resolved.
Expand All @@ -93,7 +94,16 @@ jobs:
-f description="$DESC" \
-f target_url="${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}"

# Validate the trigger: must be a /evaluate comment on a fork PR from a user
- name: Post /evaluate reminder comment
if: github.event.action == 'opened' && steps.discover.outputs.has_skills == 'true'
env:
GH_TOKEN: ${{ github.token }}
run: |
gh pr comment "${{ github.event.pull_request.number }}" \
--repo "${{ github.repository }}" \
--body "This PR changes skills, tests, or evaluation infrastructure. A maintainer can post \`/evaluate\` to trigger evaluation."

# Validate the trigger: must be a /evaluate comment on a PR from a user
# with write+ permissions.
gate:
if: >-
Expand Down Expand Up @@ -129,13 +139,7 @@ jobs:
PR_NUMBER=${{ github.event.issue.number }}
PR_DATA=$(gh api "repos/${{ github.repository }}/pulls/${PR_NUMBER}")
HEAD_SHA=$(echo "$PR_DATA" | jq -r '.head.sha')
HEAD_REPO=$(echo "$PR_DATA" | jq -r '.head.repo.full_name')
BASE_REPO=$(echo "$PR_DATA" | jq -r '.base.repo.full_name')
BASE_SHA=$(echo "$PR_DATA" | jq -r '.base.sha')
if [[ "$HEAD_REPO" == "$BASE_REPO" ]]; then
echo "::error::This command is only for fork PRs. Same-repo PRs are evaluated automatically."
exit 1
fi
echo "PR #${PR_NUMBER}: head=${HEAD_SHA} base=${BASE_SHA}"
echo "head_sha=${HEAD_SHA}" >> $GITHUB_OUTPUT
echo "base_sha=${BASE_SHA}" >> $GITHUB_OUTPUT
Expand Down Expand Up @@ -184,33 +188,49 @@ jobs:
$head = (git rev-parse origin/pr-head)
$changedFiles = git diff --name-only --diff-filter=ACMR $base $head

# Check if any changed files are in infrastructure paths
$hasInfraChanges = $changedFiles |
Where-Object { $_ -match '^(\.github/workflows/evaluation(-run|-fork-pr)?\.yml$|eng/skill-validator/)' } |
Select-Object -First 1

# We need the PR content to check for SKILL.md files — use a worktree
git worktree add /tmp/pr-content origin/pr-head 2>$null

# Extract unique plugin/skill pairs from changed files
$changedPairs = @($changedFiles |
Where-Object { $_ -match '^(?:plugins/([^/]+)/skills|tests/([^/]+))/([^/]+)/' } |
ForEach-Object {
$p = if ($Matches[1]) { $Matches[1] } else { $Matches[2] }
"$p/$($Matches[3])"
} |
Sort-Object -Unique)
if ($hasInfraChanges) {
# Infra changes can affect any evaluation — evaluate all plugins
Write-Host "Infrastructure changes detected, evaluating all plugins"
$plugins = @(Get-ChildItem -Path "/tmp/pr-content/plugins" -Directory |
Where-Object { (Test-Path (Join-Path $_.FullName "skills")) -and (Test-Path (Join-Path "/tmp/pr-content/tests" $_.Name)) } |
Select-Object -ExpandProperty Name)
$entries = @($plugins | ForEach-Object {
@{ name = $_; plugin = $_; skills_path = "plugins/$_/skills" }
})
} else {
# Extract unique plugin/skill pairs from changed files
$changedPairs = @($changedFiles |
Where-Object { $_ -match '^(?:plugins/([^/]+)/skills|tests/([^/]+))/([^/]+)/' } |
ForEach-Object {
$p = if ($Matches[1]) { $Matches[1] } else { $Matches[2] }
"$p/$($Matches[3])"
} |
Sort-Object -Unique)

# Filter to skills that have a SKILL.md and a tests directory (check in PR content)
$entries = @($changedPairs | ForEach-Object {
$parts = $_ -split '/'
$plugin = $parts[0]
$skill = $parts[1]
$skillMd = Join-Path "/tmp/pr-content" "plugins" $plugin "skills" $skill "SKILL.md"
$testsDir = Join-Path "/tmp/pr-content" "tests" $plugin
if ((Test-Path $skillMd) -and (Test-Path $testsDir)) {
@{
name = "$plugin--$skill"
plugin = $plugin
skills_path = "plugins/$plugin/skills/$skill"
# Filter to skills that have a SKILL.md and a tests directory (check in PR content)
$entries = @($changedPairs | ForEach-Object {
$parts = $_ -split '/'
$plugin = $parts[0]
$skill = $parts[1]
$skillMd = Join-Path "/tmp/pr-content" "plugins" $plugin "skills" $skill "SKILL.md"
$testsDir = Join-Path "/tmp/pr-content" "tests" $plugin
if ((Test-Path $skillMd) -and (Test-Path $testsDir)) {
@{
name = "$plugin--$skill"
plugin = $plugin
skills_path = "plugins/$plugin/skills/$skill"
}
}
}
} | Where-Object { $_ })
} | Where-Object { $_ })
}

git worktree remove /tmp/pr-content --force 2>$null

Expand Down
125 changes: 17 additions & 108 deletions .github/workflows/evaluation.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,28 @@
name: evaluation

on:
pull_request:

schedule:
- cron: '0 */3 * * *'

concurrency:
group: ${{ github.workflow }}-${{ github.event_name == 'pull_request' && github.ref || github.run_id }}
group: ${{ github.workflow }}-schedule
cancel-in-progress: true

env:
DASHBOARD_RETENTION_DAYS: 14

permissions:
contents: write
pull-requests: write

jobs:
discover:
# Skip fork PRs (handled by evaluation-fork-pr.yml) and scheduled runs
# outside the canonical repo. The schedule guard uses an explicit repo
# name (not just the fork flag) because evaluation requires
# COPILOT_GITHUB_TOKEN* secrets that only exist in dotnet/skills.
if: >-
(github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository) &&
(github.event_name != 'schedule' || github.repository == 'dotnet/skills')
# Only run scheduled evaluations in the canonical repo (requires
# COPILOT_GITHUB_TOKEN* secrets that only exist in dotnet/skills).
if: github.repository == 'dotnet/skills'
runs-on: ubuntu-latest
outputs:
entries: ${{ steps.find.outputs.entries }}
has_entries: ${{ steps.find.outputs.has_entries }}
is_infra: ${{ steps.find.outputs.is_infra }}
plugins: ${{ steps.find.outputs.plugins }}
has_plugins: ${{ steps.find.outputs.has_plugins }}
steps:
Expand All @@ -43,68 +35,13 @@ jobs:
- name: Find skills to evaluate
id: find
run: |
$entries = @()
$plugins = @()

if ("${{ github.event_name }}" -eq "pull_request") {
# PR: detect individual changed skills
$base = "${{ github.event.pull_request.base.sha }}"
$head = "${{ github.event.pull_request.head.sha }}"
$mergeBase = git merge-base $base $head
$changedFiles = git diff --name-only --diff-filter=ACMR $mergeBase $head

# Check if any changed files are in infrastructure paths (evaluation workflows or eng/skill-validator/)
$hasInfraChanges = $changedFiles |
Where-Object { $_ -match '^(\.github/workflows/evaluation(-run|-fork-pr)?\.yml$|eng/skill-validator/)' } |
Select-Object -First 1

if ($hasInfraChanges) {
# Infra changes can affect any evaluation — evaluate all plugins
Write-Host "Infrastructure changes detected, evaluating all plugins"
echo "is_infra=true" >> $env:GITHUB_OUTPUT
$plugins = @(Get-ChildItem -Path "plugins" -Directory |
Where-Object { (Test-Path (Join-Path $_.FullName "skills")) -and (Test-Path (Join-Path "tests" $_.Name)) } |
Select-Object -ExpandProperty Name)
$entries = @($plugins | ForEach-Object {
@{ name = $_; plugin = $_; skills_path = "plugins/$_/skills" }
})
} else {
# Extract unique plugin/skill pairs from changed files under plugins/*/skills/*/ or tests/*/*/
$changedPairs = @($changedFiles |
Where-Object { $_ -match '^(?:plugins/([^/]+)/skills|tests/([^/]+))/([^/]+)/' } |
ForEach-Object {
$p = if ($Matches[1]) { $Matches[1] } else { $Matches[2] }
"$p/$($Matches[3])"
} |
Sort-Object -Unique)

# Filter to skills that have a SKILL.md and a tests directory
$entries = @($changedPairs | ForEach-Object {
$parts = $_ -split '/'
$plugin = $parts[0]
$skill = $parts[1]
$skillMd = Join-Path "plugins" $plugin "skills" $skill "SKILL.md"
$testsDir = Join-Path "tests" $plugin
if ((Test-Path $skillMd) -and (Test-Path $testsDir)) {
@{
name = "$plugin--$skill"
plugin = $plugin
skills_path = "plugins/$plugin/skills/$skill"
}
}
} | Where-Object { $_ })

$plugins = @($entries | ForEach-Object { $_.plugin } | Sort-Object -Unique)
}
} else {
# Schedule: evaluate all plugins with skills and tests
$plugins = @(Get-ChildItem -Path "plugins" -Directory |
Where-Object { (Test-Path (Join-Path $_.FullName "skills")) -and (Test-Path (Join-Path "tests" $_.Name)) } |
Select-Object -ExpandProperty Name)
$entries = @($plugins | ForEach-Object {
@{ name = $_; plugin = $_; skills_path = "plugins/$_/skills" }
})
}
# Schedule: evaluate all plugins with skills and tests
$plugins = @(Get-ChildItem -Path "plugins" -Directory |
Where-Object { (Test-Path (Join-Path $_.FullName "skills")) -and (Test-Path (Join-Path "tests" $_.Name)) } |
Select-Object -ExpandProperty Name)
$entries = @($plugins | ForEach-Object {
@{ name = $_; plugin = $_; skills_path = "plugins/$_/skills" }
})

# Output entries for evaluate matrix
if (-not $entries -or $entries.Count -eq 0) {
Expand Down Expand Up @@ -135,13 +72,12 @@ jobs:
uses: ./.github/workflows/evaluation-run.yml
with:
entries: ${{ needs.discover.outputs.entries }}
runs: ${{ needs.discover.outputs.is_infra == 'true' && '1' || (github.ref == 'refs/heads/main' && '5' || '3') }}
# Infra changes and scheduled runs evaluate all plugins in parallel — reduce
# per-job concurrency to avoid API rate limits and timeouts from contention.
parallel-skills: ${{ (needs.discover.outputs.is_infra == 'true' || github.event_name == 'schedule') && '2' || '5' }}
parallel-scenarios: ${{ (needs.discover.outputs.is_infra == 'true' || github.event_name == 'schedule') && '3' || '5' }}
parallel-runs: ${{ (needs.discover.outputs.is_infra == 'true' || github.event_name == 'schedule') && '3' || '5' }}
pr-number: ${{ github.event.pull_request.number || '' }}
runs: '5'
# Scheduled runs evaluate all plugins in parallel — reduce per-job
# concurrency to avoid API rate limits and timeouts from contention.
parallel-skills: '2'
parallel-scenarios: '3'
parallel-runs: '3'
secrets:
COPILOT_GITHUB_TOKEN: ${{ secrets.COPILOT_GITHUB_TOKEN }}
COPILOT_GITHUB_TOKEN_2: ${{ secrets.COPILOT_GITHUB_TOKEN_2 }}
Expand All @@ -152,33 +88,6 @@ jobs:
COPILOT_GITHUB_TOKEN_7: ${{ secrets.COPILOT_GITHUB_TOKEN_7 }}
COPILOT_GITHUB_TOKEN_8: ${{ secrets.COPILOT_GITHUB_TOKEN_8 }}

# Plain check-run job that can be required in the GitHub ruleset.
# For fork PRs, discover is skipped (fork guard) so this job is also skipped;
# the fork-status job in evaluation-fork-pr.yml posts a commit status instead.
evaluation-status:
needs: [discover, run-evaluation]
if: always() && github.event_name == 'pull_request' && needs.discover.result != 'skipped'
runs-on: ubuntu-latest
permissions: {}
steps:
- name: Check evaluation result
run: |
DISCOVER_RESULT="${{ needs.discover.result }}"
HAS_ENTRIES="${{ needs.discover.outputs.has_entries }}"
EVAL_RESULT="${{ needs.run-evaluation.result }}"

if [[ "$DISCOVER_RESULT" != "success" ]]; then
echo "::error::Discovery failed ($DISCOVER_RESULT)"
exit 1
elif [[ "$HAS_ENTRIES" != "true" ]]; then
echo "No skills to evaluate"
elif [[ "$EVAL_RESULT" == "success" ]]; then
echo "Evaluation passed"
else
echo "::error::Evaluation did not pass ($EVAL_RESULT)"
exit 1
fi

publish-benchmark:
needs: [discover, run-evaluation]
if: github.ref == 'refs/heads/main' && needs.discover.outputs.has_plugins == 'true'
Expand Down