Skip to content
Closed
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
19 changes: 12 additions & 7 deletions .github/scripts/pr-triage-act.sh
Original file line number Diff line number Diff line change
Expand Up @@ -176,9 +176,9 @@ review_data() {
# ----------------------------------------------------------------------
# Helpers — evaluation status
# ----------------------------------------------------------------------
eval_status_state() {
eval_status_data() {
gh api "repos/$REPO/statuses/$HEAD_SHA" \
--jq '[.[] | select(.context == "evaluation-status")] | (sort_by(.created_at) | last) | .state // "pending"'
--jq '[.[] | select(.context == "evaluation-status")] | (sort_by(.created_at) | last) | [(.state // "pending"), (.description // "")] | @tsv'
}

eval_run_exists_for_head() {
Expand Down Expand Up @@ -309,8 +309,8 @@ if [ -z "$STATE" ]; then
RV=$(review_data)
REVIEW_DECISION=$(jq -r '.reviewDecision // ""' <<<"$RV")
UNRESOLVED=$(jq -r '[.reviewThreads.nodes[] | select(.isResolved == false)] | length' <<<"$RV")
EVAL_STATE=$(eval_status_state)
log "reviewDecision=$REVIEW_DECISION unresolved_threads=$UNRESOLVED eval_status=$EVAL_STATE"
IFS=$'\t' read -r EVAL_STATE EVAL_DESCRIPTION < <(eval_status_data)
log "reviewDecision=$REVIEW_DECISION unresolved_threads=$UNRESOLVED eval_status=$EVAL_STATE eval_description='$EVAL_DESCRIPTION'"

# Malicious scan precedence (non-bot, untrusted, no marker for current head).
# Match either the orchestrator-posted dispatched marker (source of truth) or
Expand Down Expand Up @@ -478,7 +478,12 @@ EOF
)
else
# ready-for-review: resolve CODEOWNERS for changed paths
local files_json owners_str
local files_json owners_str eval_message
if [ "$EVAL_DESCRIPTION" = "No skills to evaluate" ]; then
eval_message="No skill evaluation was required for \`$HEAD_SHA_SHORT\`."
else
eval_message="Evaluation passed for \`$HEAD_SHA_SHORT\`."
fi
# NB: --paginate runs --jq per page, so '[.[] | .filename]' would emit one JSON array
# per page. Emit one filename per line, then slurp into a single JSON array.
files_json=$(gh api --paginate "repos/$REPO/pulls/$PR_NUMBER/files" --jq '.[] | .filename' \
Expand All @@ -499,7 +504,7 @@ EOF
fi
body=$(cat <<EOF
<!-- pr-triage:fingerprint=maintainer-ping/B:$HEAD_SHA_SHORT:$TODAY -->
Evaluation passed for \`$HEAD_SHA_SHORT\`. No CODEOWNERS entry matched the changed paths; cc $MERGE_APPROVERS_TEAM — please review.
$eval_message No CODEOWNERS entry matched the changed paths; cc $MERGE_APPROVERS_TEAM — please review.
EOF
)
else
Expand All @@ -514,7 +519,7 @@ EOF
fi
body=$(cat <<EOF
<!-- pr-triage:fingerprint=maintainer-ping/A:$HEAD_SHA_SHORT:$TODAY -->
Evaluation passed for \`$HEAD_SHA_SHORT\`. cc $owners_str — please review.
$eval_message cc $owners_str — please review.
EOF
)
fi
Expand Down
31 changes: 26 additions & 5 deletions .github/workflows/evaluation-run.yml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ on:
required: false
type: string
default: ''
evaluation_tools_ref:
description: 'PR SHA containing eng/evaluation-tools manifests to smoke-test'
required: false
type: string
default: ''
secrets:
# Passed by the caller via an explicit named mapping (see the vally-evaluate
# job in evaluation.yml). A reusable workflow does not inherit the caller's
Expand Down Expand Up @@ -370,6 +375,15 @@ jobs:
global.json
LICENSE

- name: Checkout evaluated Vally manifests
if: steps.find-evals.outputs.has_evals == 'true' && inputs.evaluation_tools_ref != ''
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
with:
ref: ${{ inputs.evaluation_tools_ref }}
path: _evaluation-tools-source
persist-credentials: false
sparse-checkout: eng/evaluation-tools

- name: Configure NuGet for evaluations
if: steps.find-evals.outputs.has_evals == 'true'
run: |
Expand All @@ -390,16 +404,23 @@ jobs:

- name: Install vally and Copilot CLI
if: steps.find-evals.outputs.has_evals == 'true'
# Install the trusted, pinned toolchain. The direct Copilot CLI invocation
# probes a token before Vally starts expensive trials, while Vally uses the
# same CLI runtime through @github/copilot-sdk.
# For an evaluation-tools infrastructure PR, install only the package
# manifests from the bound PR commit. All adapter and validator code stays
# on the trusted workflow commit.
env:
EVALUATION_TOOLS_REF: ${{ inputs.evaluation_tools_ref }}
working-directory: ${{ runner.temp }}
run: |
printf '' > "$RUNNER_TEMP/evaluation.npmrc"
rm -rf "$RUNNER_TEMP/evaluation-tools"
mkdir -p "$RUNNER_TEMP/evaluation-tools"
cp "$RUNNER_TEMP/trusted-validator-src/eng/evaluation-tools/package.json" \
"$RUNNER_TEMP/trusted-validator-src/eng/evaluation-tools/package-lock.json" \
if [ -n "$EVALUATION_TOOLS_REF" ]; then
EVALUATION_TOOLS_SOURCE="$GITHUB_WORKSPACE/_evaluation-tools-source/eng/evaluation-tools"
else
EVALUATION_TOOLS_SOURCE="$RUNNER_TEMP/trusted-validator-src/eng/evaluation-tools"
fi
cp "$EVALUATION_TOOLS_SOURCE/package.json" \
"$EVALUATION_TOOLS_SOURCE/package-lock.json" \
"$RUNNER_TEMP/evaluation-tools/"
npm ci \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we avoid installing PR-controlled manifests in a job that later runs the installed Copilot/Vally binaries with a real Copilot PAT? Previously these dependencies came from the trusted workflow commit; this change makes them PR-controlled, so a modified dependency could receive or exfiltrate the selected token when executed later.

Could we keep PAT-backed execution on trusted dependencies, or run this PR dependency smoke test without reusable secrets?

Suggested by Copilot.

--prefix "$RUNNER_TEMP/evaluation-tools" \
Expand Down
17 changes: 17 additions & 0 deletions .github/workflows/evaluation.yml
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ jobs:
# changes here. Documentation files don't affect evaluation.
$hasInfraChanges = $changedFiles |
Where-Object {
($_ -match '^eng/evaluation-tools/') -or
($_ -match '^eng/vally-adapter/') -or
($_ -match '^dotnet-skills\.experiment\.yaml$') -or
$_ -match '^\.github/workflows/(evaluation|evaluation-run)\.yml$'
Expand Down Expand Up @@ -256,6 +257,7 @@ jobs:

$hasInfraChanges = $changedFiles |
Where-Object {
($_ -match '^eng/evaluation-tools/') -or
($_ -match '^eng/vally-adapter/') -or
($_ -match '^dotnet-skills\.experiment\.yaml$') -or
$_ -match '^\.github/workflows/(evaluation|evaluation-run)\.yml$'
Expand Down Expand Up @@ -735,6 +737,7 @@ jobs:
entries: ${{ steps.find.outputs.entries }}
has_entries: ${{ steps.find.outputs.has_entries }}
is_infra: ${{ steps.find.outputs.is_infra }}
evaluation_tools_ref: ${{ steps.find.outputs.evaluation_tools_ref }}
plugins: ${{ steps.find.outputs.plugins }}
has_plugins: ${{ steps.find.outputs.has_plugins }}
steps:
Expand Down Expand Up @@ -820,6 +823,7 @@ jobs:
run: |
$entries = @()
$plugins = @()
echo "evaluation_tools_ref=" >> $env:GITHUB_OUTPUT

# Build matrix entries for a full-plugin evaluation, sharding the
# plugin's skills by the optional `executionShard:` top-level tag
Expand Down Expand Up @@ -909,8 +913,13 @@ jobs:
# Check if any changed files are in infrastructure paths. The linter
# (skill-validator) is a separate workflow, so eng/skill-validator/
# changes are NOT infra changes here.
$hasEvaluationToolChanges = $changedFiles |
Where-Object { $_ -match '^eng/evaluation-tools/' } |
Select-Object -First 1

$hasInfraChanges = $changedFiles |
Where-Object {
($_ -match '^eng/evaluation-tools/') -or
($_ -match '^eng/vally-adapter/') -or
($_ -match '^dotnet-skills\.experiment\.yaml$') -or
$_ -match '^\.github/workflows/(evaluation|evaluation-run)\.yml$'
Expand All @@ -922,6 +931,13 @@ jobs:
Where-Object { $_ -match '^(?:plugins/([^/]+)/skills|tests/([^/]+))/([^/]+)/' } |
Select-Object -First 1

if ($hasEvaluationToolChanges) {
# Run Vally from the exact PR commit whose package manifests changed.
# The reusable workflow keeps all other executable tooling on the
# trusted workflow commit.
echo "evaluation_tools_ref=$head" >> $env:GITHUB_OUTPUT
}

if ($hasInfraChanges -and -not $hasSkillChanges) {
echo "is_infra=true" >> $env:GITHUB_OUTPUT
# Infra-only: evaluate a small random subset of plugins to keep
Expand Down Expand Up @@ -1225,6 +1241,7 @@ jobs:
with:
entries: ${{ needs.discover.outputs.entries }}
head_sha: ${{ needs.gate.outputs.head_sha }}
evaluation_tools_ref: ${{ needs.discover.outputs.evaluation_tools_ref }}
# Forward ONLY the Copilot PAT pool across the workflow_call boundary. A
# reusable workflow does not inherit the caller's org/repo secrets, so the
# pool would read empty on this path unless it is passed explicitly. We map
Expand Down
10 changes: 10 additions & 0 deletions docs/design/pr-triage-workflows.md
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,16 @@ Order of evaluation; first match wins:

Trusted = `OWNER` / `MEMBER` / `COLLABORATOR`. Bots are short-circuited as trusted.

The maintainer ping reads both the `evaluation-status` state and description.
It says "Evaluation passed" only after an evaluation run. When the successful
status means that no skill evaluation was required, the ping states that instead.

Infrastructure-only changes normally evaluate two randomly selected eligible
plugins. Changes under `eng/evaluation-tools/`, including Vally version updates,
use the same two-plugin smoke test and install the package manifests from the
exact PR commit. Other executable evaluation tooling remains pinned to the
trusted workflow commit.

## Cool-down and idempotency

Each ping variant writes a hidden HTML marker into its comment. The worker
Expand Down
60 changes: 58 additions & 2 deletions eng/evaluation/test_token_failover.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
WORKFLOW = REPO_ROOT / ".github" / "workflows" / "evaluation-run.yml"
CALLER_WORKFLOW = REPO_ROOT / ".github" / "workflows" / "evaluation.yml"
TEST_WORKFLOW = REPO_ROOT / ".github" / "workflows" / "evaluation-workflow-tests.yml"
TRIAGE_SCRIPT = REPO_ROOT / ".github" / "scripts" / "pr-triage-act.sh"
STEP_NAME = "Select available Copilot token from pool"
GIT_BASH = Path(os.environ.get("ProgramFiles", r"C:\Program Files")) / "Git" / "bin" / "bash.exe"
BASH = str(GIT_BASH) if os.name == "nt" and GIT_BASH.exists() else "bash"
Expand Down Expand Up @@ -263,11 +264,19 @@ def test_eval_discovery_precedes_tool_install_and_token_selection(self) -> None:
install_script,
)
self.assertIn(
'"$RUNNER_TEMP/trusted-validator-src/eng/evaluation-tools/package.json"',
'"$EVALUATION_TOOLS_SOURCE/package.json"',
install_script,
)
self.assertIn(
'"$RUNNER_TEMP/trusted-validator-src/eng/evaluation-tools/package-lock.json"',
'"$EVALUATION_TOOLS_SOURCE/package-lock.json"',
install_script,
)
self.assertIn(
'EVALUATION_TOOLS_SOURCE="$GITHUB_WORKSPACE/_evaluation-tools-source/eng/evaluation-tools"',
install_script,
)
self.assertIn(
'EVALUATION_TOOLS_SOURCE="$RUNNER_TEMP/trusted-validator-src/eng/evaluation-tools"',
install_script,
)
self.assertIn("npm ci", install_script)
Expand Down Expand Up @@ -307,6 +316,53 @@ def test_evaluation_tool_manifest_has_secretless_smoke_test(self) -> None:
smoke_script,
)

def test_evaluation_tool_changes_use_infrastructure_smoke_evaluation(self) -> None:
caller_text = CALLER_WORKFLOW.read_text(encoding="utf-8")
self.assertEqual(caller_text.count("($_ -match '^eng/evaluation-tools/') -or"), 3)

caller = yaml.safe_load(caller_text)
discover = caller["jobs"]["discover"]
self.assertEqual(
discover["outputs"]["evaluation_tools_ref"],
"${{ steps.find.outputs.evaluation_tools_ref }}",
)
find_script = next(
step["run"]
for step in discover["steps"]
if step.get("name") == "Find skills to evaluate"
)
self.assertIn("$hasEvaluationToolChanges", find_script)
self.assertIn('echo "evaluation_tools_ref=$head"', find_script)
self.assertIn(
"Get-Random -Count ([Math]::Min(2, $allPlugins.Count))",
find_script,
)
self.assertEqual(
caller["jobs"]["evaluate"]["with"]["evaluation_tools_ref"],
"${{ needs.discover.outputs.evaluation_tools_ref }}",
)

workflow = yaml.safe_load(WORKFLOW.read_text(encoding="utf-8"))
workflow_call = workflow.get("on", workflow.get(True))["workflow_call"]
self.assertIn("evaluation_tools_ref", workflow_call["inputs"])
steps = {
step.get("name"): step
for step in workflow["jobs"]["vally-evaluate"]["steps"]
}
checkout = steps["Checkout evaluated Vally manifests"]
self.assertEqual(checkout["with"]["ref"], "${{ inputs.evaluation_tools_ref }}")
self.assertEqual(checkout["with"]["sparse-checkout"], "eng/evaluation-tools")

def test_triage_distinguishes_skipped_and_completed_evaluations(self) -> None:
script = TRIAGE_SCRIPT.read_text(encoding="utf-8")
self.assertIn('(.description // "")', script)
self.assertIn(
'if [ "$EVAL_DESCRIPTION" = "No skills to evaluate" ]',
script,
)
self.assertIn("No skill evaluation was required", script)
self.assertIn("Evaluation passed for", script)

def test_fork_checkout_is_blocked_and_adapter_code_is_trusted(self) -> None:
workflow = yaml.safe_load(WORKFLOW.read_text(encoding="utf-8"))
steps = workflow["jobs"]["vally-evaluate"]["steps"]
Expand Down
5 changes: 5 additions & 0 deletions eng/skill-validator/src/docs/InvestigatingResults.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@

> **⚠️ Skill evaluations now run on the Vally harness.** As of the Vally migration, the LLM eval pipeline (`evaluation.yml`) no longer uses `skill-validator evaluate`; it runs Vally via `eng/vally-adapter/` and uploads `vally-results-*` artifacts. For investigating current eval failures, use the guide at `eng/vally-adapter/InvestigatingResults.md` in the repository root instead. This document describes the legacy `skill-validator evaluate` schema and is retained for historical results and reference. (The `skill-validator check` **linter** is unaffected and still runs via `skill-check.yml`.)

> Vally dependency updates under `eng/evaluation-tools/` run an infrastructure
> smoke evaluation against two randomly selected eligible plugins. The package
> manifests come from the exact PR commit; other executable evaluation tooling
> remains pinned to the trusted workflow commit.

This guide is intended primarily for AI agents investigating skill evaluation failures, though humans will find it useful too. It documents the `results.json` schema, common failure patterns, and recommended fixes.

## Using this guide with an AI agent
Expand Down
5 changes: 5 additions & 0 deletions eng/vally-adapter/InvestigatingResults.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ Evaluations run through Vally (`@microsoft/vally-cli`): every skill's `tests/<pl

> Note: the linter (`skill-validator check`) is a **separate** workflow (`skill-check.yml`) and is unrelated to these eval results.

> Vally dependency updates under `eng/evaluation-tools/` trigger an
> infrastructure smoke evaluation of two randomly selected eligible plugins.
> That run installs the package manifests from the exact PR commit while keeping
> the adapter, validator, and workflow logic on the trusted workflow commit.

## Using this guide with an AI agent

When an evaluation has failures, the PR comment includes a ready-to-use prompt — copy it to your AI agent. The agent downloads the artifacts, reads this guide, analyzes the `results.json` files, and suggests fixes.
Expand Down
Loading