From f74ff51d00c9f1cd8dbac30a22e109d17b527731 Mon Sep 17 00:00:00 2001 From: Abhitej John Date: Tue, 18 Aug 2026 11:58:36 -0700 Subject: [PATCH] Run smoke evaluations for Vally updates Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 03c6d29f-0826-4c56-b97f-92c3169d204e --- .github/scripts/pr-triage-act.sh | 19 +++--- .github/workflows/evaluation-run.yml | 31 ++++++++-- .github/workflows/evaluation.yml | 17 ++++++ docs/design/pr-triage-workflows.md | 10 ++++ eng/evaluation/test_token_failover.py | 60 ++++++++++++++++++- .../src/docs/InvestigatingResults.md | 5 ++ eng/vally-adapter/InvestigatingResults.md | 5 ++ 7 files changed, 133 insertions(+), 14 deletions(-) diff --git a/.github/scripts/pr-triage-act.sh b/.github/scripts/pr-triage-act.sh index 9e50a0273a..1c1e6070fb 100644 --- a/.github/scripts/pr-triage-act.sh +++ b/.github/scripts/pr-triage-act.sh @@ -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() { @@ -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 @@ -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' \ @@ -499,7 +504,7 @@ EOF fi body=$(cat < -✅ 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 @@ -514,7 +519,7 @@ EOF fi body=$(cat < -✅ Evaluation passed for \`$HEAD_SHA_SHORT\`. cc $owners_str — please review. +✅ $eval_message cc $owners_str — please review. EOF ) fi diff --git a/.github/workflows/evaluation-run.yml b/.github/workflows/evaluation-run.yml index afffdaca48..c028fd2ff5 100644 --- a/.github/workflows/evaluation-run.yml +++ b/.github/workflows/evaluation-run.yml @@ -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 @@ -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: | @@ -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 \ --prefix "$RUNNER_TEMP/evaluation-tools" \ diff --git a/.github/workflows/evaluation.yml b/.github/workflows/evaluation.yml index aaea263e7b..d2d2ffcc78 100644 --- a/.github/workflows/evaluation.yml +++ b/.github/workflows/evaluation.yml @@ -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$' @@ -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$' @@ -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: @@ -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 @@ -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$' @@ -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 @@ -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 diff --git a/docs/design/pr-triage-workflows.md b/docs/design/pr-triage-workflows.md index 1e7d7bc311..cb61bf8b50 100644 --- a/docs/design/pr-triage-workflows.md +++ b/docs/design/pr-triage-workflows.md @@ -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 diff --git a/eng/evaluation/test_token_failover.py b/eng/evaluation/test_token_failover.py index 568993aa9a..4c726f0d13 100644 --- a/eng/evaluation/test_token_failover.py +++ b/eng/evaluation/test_token_failover.py @@ -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" @@ -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) @@ -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"] diff --git a/eng/skill-validator/src/docs/InvestigatingResults.md b/eng/skill-validator/src/docs/InvestigatingResults.md index 16be0f3a5e..9a7aa45da1 100644 --- a/eng/skill-validator/src/docs/InvestigatingResults.md +++ b/eng/skill-validator/src/docs/InvestigatingResults.md @@ -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 diff --git a/eng/vally-adapter/InvestigatingResults.md b/eng/vally-adapter/InvestigatingResults.md index 9ee62d7470..272450274f 100644 --- a/eng/vally-adapter/InvestigatingResults.md +++ b/eng/vally-adapter/InvestigatingResults.md @@ -6,6 +6,11 @@ Evaluations run through Vally (`@microsoft/vally-cli`): every skill's `tests/ 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.