From a7ae2c1c8ef1d056e57972420dc48a750b4d1c05 Mon Sep 17 00:00:00 2001 From: Ronnie Geraghty Date: Wed, 11 Nov 2026 11:23:30 -0800 Subject: [PATCH] Compare changed skills before and after with the native evals Action Require protected-environment approval before paid same-repository runs; hold eval measurements fixed while switching only the target skill source and reuse the Action's verified same-job tooling. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/scripts/validation.test.mjs | 146 +++++++++++++++++++++++++- .github/workflows/evals.yml | 3 +- .github/workflows/skill-evals.yml | 152 ++++++++++++++++++++++++++++ README.md | 4 +- docs/checks-and-evals.md | 30 +++++- 5 files changed, 329 insertions(+), 6 deletions(-) create mode 100644 .github/workflows/skill-evals.yml diff --git a/.github/scripts/validation.test.mjs b/.github/scripts/validation.test.mjs index ba64bdf..ea2e230 100644 --- a/.github/scripts/validation.test.mjs +++ b/.github/scripts/validation.test.mjs @@ -2,7 +2,8 @@ import test from "node:test"; import assert from "node:assert/strict"; import { spawnSync } from "node:child_process"; import { randomUUID } from "node:crypto"; -import { copyFileSync, existsSync, mkdirSync, readFileSync, readdirSync, realpathSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { copyFileSync, existsSync, mkdirSync, mkdtempSync, readFileSync, readdirSync, realpathSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; import { resolve } from "node:path"; import { fileURLToPath, pathToFileURL } from "node:url"; import { parse } from "yaml"; @@ -361,7 +362,7 @@ test("schema validation skips absent local manifests without downloading or inst } }); -test("validation calls CI scripts, and paid eval activation is manual-only", () => { +test("validation stays model-free and the smoke workflow remains manual-only", () => { const validate = readFileSync(resolve(root, ".github/workflows/validate.yml"), "utf8"); assert.doesNotMatch(validate, /node -e|run: \|/); for (const script of ["validate-schema.sh", "check-local-catalog.mjs", "check-remote-pins.mjs"]) { @@ -391,6 +392,147 @@ test("validation calls CI scripts, and paid eval activation is manual-only", () }); }); + test("PR skill evals require approval and use two sequential native Action runs with fixed measurement inputs", () => { + const workflow = parse(readFileSync(resolve(root, ".github/workflows/skill-evals.yml"), "utf8")); + assert.deepEqual(Object.keys(workflow.on), ["pull_request"]); + assert.deepEqual(workflow.on.pull_request.paths, ["plugins/*/skills/**"]); + assert.equal(workflow.jobs.select.if, "github.event.pull_request.head.repo.full_name == github.repository"); + assert.deepEqual(workflow.permissions, { contents: "read" }); + assert.equal(workflow.jobs.select.permissions, undefined); + const job = workflow.jobs.evals; + assert.equal(job.environment, "evals"); + assert.equal(job.strategy["fail-fast"], false); + assert.deepEqual(job.permissions, { contents: "read", "copilot-requests": "write" }); + const actions = job.steps.filter((step) => step.uses?.startsWith("github/evals-action@")); + assert.equal(actions.length, 2); + assert.equal(actions[0].uses, actions[1].uses); + const { "report-id": beforeId, ...before } = actions[0].with; + const { "report-id": afterId, ...after } = actions[1].with; + assert.deepEqual(before, after); + assert.equal(before.command, "run"); + assert.equal(before["eval-paths"], "${{ matrix.evalPath }}"); + assert.equal(before["vally-version"], "0.19.0"); + assert.equal(before.runs, 1); + assert.equal(before.workers, 1); + assert.equal(before["max-retries"], 0); + assert.equal(before["upload-transcripts"], false); + assert.notEqual(beforeId, afterId); + assert.equal(actions[1].if, "${{ !cancelled() && steps.after_source.outcome == 'success' }}"); + assert.equal(job.steps.find((step) => step.id === "after_source").if, + "${{ !cancelled() && steps.before_source.outcome == 'success' }}"); + for (const step of job.steps.filter((step) => step.uses?.startsWith("actions/checkout@"))) { + assert.equal(step.with["persist-credentials"], false); + assert.equal(step.with["fetch-depth"], 2); + assert.equal(step.with.ref, undefined, "HEAD must remain the merge SHA accepted by the Action"); + } + }); + + test("PR selection and sequential source restores compare only eligible target skills and fail closed without approval", (t) => { + const directory = mkdtempSync(resolve(tmpdir(), "skill-eval-workflow-")); + t.after(() => rmSync(directory, { recursive: true, force: true })); + const repository = resolve(directory, "repository"); + const runner = resolve(directory, "runner"); + const hooks = resolve(directory, "hooks"); + for (const path of [repository, runner, hooks]) mkdirSync(path); + const config = resolve(directory, "gitconfig"); + writeFileSync(config, ""); + const env = { + ...process.env, GIT_CONFIG_GLOBAL: config, GIT_CONFIG_NOSYSTEM: "1", + RUNNER_TEMP: runner, GITHUB_OUTPUT: resolve(directory, "outputs"), + GITHUB_STEP_SUMMARY: resolve(directory, "summary.md").replaceAll("\\", "/"), + }; + const git = (...args) => { + const result = spawnSync("git", [ + "-c", "user.name=Fixture", "-c", "user.email=fixture@example.test", + "-c", "commit.gpgsign=false", "-c", `core.hooksPath=${hooks}`, ...args, + ], { cwd: repository, env, encoding: "utf8", timeout: 10000 }); + assert.ifError(result.error); + assert.equal(result.status, 0, result.stderr); + return result.stdout.trim(); + }; + const file = (path, content) => { + const destination = resolve(repository, path); + mkdirSync(resolve(destination, ".."), { recursive: true }); + writeFileSync(destination, content); + }; + const target = "plugins/example/skills/changed"; + git("init", "--initial-branch=main"); + file(`${target}/SKILL.md`, "base skill\n"); + file(`${target}/old.md`, "base-only file\n"); + file("plugins/example/skills/neighbor/SKILL.md", "base neighbor\n"); + file("plugins/example/skills/deleted/SKILL.md", "deleted skill\n"); + file("evals/example/skills/changed/eval.yaml", "fixed measurement\n"); + git("add", "."); git("commit", "-m", "Base fixture"); + git("checkout", "-b", "candidate"); + file(`${target}/SKILL.md`, "head skill\n"); + file(`${target}/new.md`, "head-only file\n"); + rmSync(resolve(repository, `${target}/old.md`)); + rmSync(resolve(repository, "plugins/example/skills/deleted/SKILL.md")); + file("plugins/example/skills/neighbor/SKILL.md", "head neighbor without an eval\n"); + file("plugins/example/skills/added/SKILL.md", "new skill without a baseline\n"); + git("add", "."); git("commit", "-m", "Candidate fixture"); + env.HEAD_SHA = git("rev-parse", "HEAD"); + git("checkout", "main"); + file(`${target}/main.md`, "current-main-only file\n"); + git("add", "."); git("commit", "-m", "Advance main fixture"); + const base = git("rev-parse", "HEAD"); + git("merge", "--no-ff", "candidate", "-m", "Merge fixture"); + env.GITHUB_SHA = git("rev-parse", "HEAD"); + const workflow = parse(readFileSync(resolve(root, ".github/workflows/skill-evals.yml"), "utf8")); + const selection = workflow.jobs.select.steps.find((step) => step.id === "select").run; + const code = selection.match(/<<'JS'\n([\s\S]*)\nJS/)[1]; + const environment = resolve(runner, "evals-environment.json"); + writeFileSync(environment, JSON.stringify({ protection_rules: [] })); + const denied = spawnSync(process.execPath, ["--input-type=module", "-e", code], { + cwd: repository, env, encoding: "utf8", timeout: 10000, + }); + assert.ifError(denied.error); + assert.notEqual(denied.status, 0); + assert.match(denied.stderr, /required maintainer reviewers/); + assert.equal(existsSync(env.GITHUB_OUTPUT), false); + writeFileSync(environment, JSON.stringify({ protection_rules: [{ type: "required_reviewers", reviewers: [{ type: "Team" }] }] })); + const selected = spawnSync(process.execPath, ["--input-type=module", "-e", code], { + cwd: repository, env, encoding: "utf8", timeout: 10000, + }); + assert.ifError(selected.error); + assert.equal(selected.status, 0, selected.stderr); + assert.match(selected.stdout, /No before\/after pair for example\/added/); + assert.match(selected.stdout, /No before\/after pair for example\/deleted/); + assert.match(selected.stdout, /No before\/after pair for example\/neighbor/); + const matrix = JSON.parse(readFileSync(env.GITHUB_OUTPUT, "utf8").split("\n")[0].slice("matrix=".length)); + assert.equal(matrix.include.length, 1); + const item = matrix.include[0]; + assert.equal(item.directory, target); + assert.equal(item.base, base); + assert.equal(item.head, env.HEAD_SHA); + assert.match(item.id, /^skill-[a-f0-9]{12}$/); + for (const [id, source, expected] of [ + ["before_source", base, "base skill\n"], ["after_source", env.HEAD_SHA, "head skill\n"], + ]) { + const step = workflow.jobs.evals.steps.find((entry) => entry.id === id); + const restored = spawnSync("bash", ["-c", step.run], { + cwd: repository, env: { ...env, SOURCE_SHA: source, SKILL_DIR: target }, encoding: "utf8", timeout: 10000, + }); + assert.ifError(restored.error); + assert.equal(restored.status, 0, restored.stderr); + assert.equal(git("rev-parse", "HEAD"), env.GITHUB_SHA); + assert.equal(readFileSync(resolve(repository, `${target}/SKILL.md`), "utf8"), expected); + assert.equal(existsSync(resolve(repository, `${target}/old.md`)), id === "before_source"); + assert.equal(existsSync(resolve(repository, `${target}/new.md`)), id === "after_source"); + assert.equal(existsSync(resolve(repository, `${target}/main.md`)), id === "before_source"); + assert.equal(readFileSync(resolve(repository, "plugins/example/skills/neighbor/SKILL.md"), "utf8"), "head neighbor without an eval\n"); + assert.equal(readFileSync(resolve(repository, "evals/example/skills/changed/eval.yaml"), "utf8"), "fixed measurement\n"); + assert.ok(git("diff", "--name-only", "HEAD").split("\n").every((path) => path.startsWith(`${target}/`))); + } + file("evals/example/skills/changed/eval.yaml", "unexpected measurement change\n"); + const drift = spawnSync("bash", ["-c", workflow.jobs.evals.steps.find((entry) => entry.id === "after_source").run], { + cwd: repository, env: { ...env, SOURCE_SHA: env.HEAD_SHA, SKILL_DIR: target }, encoding: "utf8", timeout: 10000, + }); + assert.ifError(drift.error); + assert.notEqual(drift.status, 0); + assert.match(drift.stdout, /Measurement files changed outside the selected skill/); + }); + for (const [plugin, skillName, readInstruction] of [ ["marketplace-maintainer", "update-remote-pins", "Read its `AGENTS.md` and `marketplace.json`, and this skill's [remote pin rules](references/remote-pins.md)."], ["plugin-authoring", "submit-plugin", "Read its `AGENTS.md` and `CONTRIBUTING.md`."], diff --git a/.github/workflows/evals.yml b/.github/workflows/evals.yml index 732ff84..79a65f1 100644 --- a/.github/workflows/evals.yml +++ b/.github/workflows/evals.yml @@ -1,6 +1,7 @@ name: Evals # Runs the smoke evals with real Copilot sessions, which cost AI credits. -# Start by explicit manual dispatch only; pull requests never start paid runs. +# This smoke workflow starts only by explicit manual dispatch. +# Skill comparisons use the separate approval-gated skill-evals workflow. on: workflow_dispatch: permissions: diff --git a/.github/workflows/skill-evals.yml b/.github/workflows/skill-evals.yml new file mode 100644 index 0000000..ad238eb --- /dev/null +++ b/.github/workflows/skill-evals.yml @@ -0,0 +1,152 @@ +name: Skill evals (before and after) +# Paid runs require approval in the existing, protected evals environment. +on: + pull_request: + paths: + - plugins/*/skills/** +permissions: + contents: read +jobs: + select: + if: github.event.pull_request.head.repo.full_name == github.repository + runs-on: ubuntu-latest + timeout-minutes: 5 + outputs: + matrix: ${{ steps.select.outputs.matrix }} + count: ${{ steps.select.outputs.count }} + steps: + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 + with: + fetch-depth: 2 + persist-credentials: false + - uses: actions/setup-node@249970729cb0ef3589644e2896645e5dc5ba9c38 # v6 + with: + node-version: 24 + - name: Read approval protection without creating an environment + env: + GH_TOKEN: ${{ github.token }} + run: gh api "repos/$GITHUB_REPOSITORY/environments/evals" > "$RUNNER_TEMP/evals-environment.json" + - name: Require approval protection and select changed skills + id: select + env: + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: | + node --input-type=module <<'JS' + import { execFileSync, spawnSync } from 'node:child_process'; + import { createHash } from 'node:crypto'; + import { appendFileSync, existsSync, readFileSync } from 'node:fs'; + import { resolve } from 'node:path'; + const environment = JSON.parse(readFileSync(resolve(process.env.RUNNER_TEMP, 'evals-environment.json'), 'utf8')); + if (!environment.protection_rules?.some(rule => rule.type === 'required_reviewers' && rule.reviewers?.length > 0)) { + throw new Error('Configure the evals environment with required maintainer reviewers before enabling paid PR evals.'); + } + const git = (...args) => execFileSync('git', args, { encoding: 'utf8' }).trim(); + const head = process.env.HEAD_SHA; + if (!/^[a-f0-9]{40}$/.test(head ?? '') || git('rev-parse', 'HEAD^2') !== head) { + throw new Error('Expected the PR merge commit and its exact head parent.'); + } + const base = git('rev-parse', 'HEAD^1'); + const changed = execFileSync('git', ['diff', '--no-renames', '--name-only', '-z', base, 'HEAD', '--', 'plugins'], { encoding: 'utf8' }); + const keys = [...new Set(changed.split('\0').flatMap(file => { + const match = /^plugins\/([^/]+)\/skills\/([^/]+)\//.exec(file); + return match ? [`${match[1]}/${match[2]}`] : []; + }))].sort(); + const include = []; + for (const key of keys) { + if (!/^[a-z0-9][a-z0-9._-]*\/[a-z0-9][a-z0-9._-]*$/.test(key)) { + throw new Error('Unsupported plugin or skill path in PR changes.'); + } + const [plugin, skill] = key.split('/'); + const directory = `plugins/${plugin}/skills/${skill}`; + const evalPath = `evals/${plugin}/skills/${skill}/eval.yaml`; + const present = ref => { + const result = spawnSync('git', ['cat-file', '-e', `${ref}:${directory}/SKILL.md`], { stdio: 'ignore' }); + if (result.error) throw result.error; + if (result.status !== 0 && result.status !== 128) throw new Error('Could not inspect the skill source tree.'); + return result.status === 0; + }; + if (!present(base) || !present(head) || !existsSync(evalPath)) { + console.log(`::warning::No before/after pair for ${key}: both skill versions and ${evalPath} are required. No eval was run for this skill.`); + continue; + } + include.push({ plugin, skill, directory, evalPath, base, head, id: `skill-${createHash('sha256').update(key).digest('hex').slice(0, 12)}` }); + } + appendFileSync(process.env.GITHUB_OUTPUT, `matrix=${JSON.stringify({ include })}\ncount=${include.length}\n`); + JS + + evals: + needs: select + if: needs.select.outputs.count != '0' + name: ${{ matrix.plugin }}/${{ matrix.skill }} + runs-on: ubuntu-latest + timeout-minutes: 60 + environment: evals + permissions: + contents: read + copilot-requests: write + strategy: + fail-fast: false + matrix: ${{ fromJSON(needs.select.outputs.matrix) }} + env: + VALLY_TELEMETRY_OPTOUT: '1' + SKILL_DIR: ${{ matrix.directory }} + steps: + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 + with: + fetch-depth: 2 + persist-credentials: false + - uses: actions/setup-node@249970729cb0ef3589644e2896645e5dc5ba9c38 # v6 + with: + node-version: 24 + - name: Select the target skill from current main + id: before_source + env: + SOURCE_SHA: ${{ matrix.base }} + run: | + set -euo pipefail + git --literal-pathspecs restore --source="$SOURCE_SHA" --staged --worktree -- "$SKILL_DIR" + git --literal-pathspecs diff --exit-code "$SOURCE_SHA" -- "$SKILL_DIR" + if [[ -n "$(git diff --name-only HEAD | awk -v dir="$SKILL_DIR/" 'index($0,dir)!=1')$(git ls-files --others --exclude-standard)" ]]; then + echo "::error::Measurement files changed outside the selected skill." + exit 1 + fi + echo "Before: $SKILL_DIR from $SOURCE_SHA; measurement revision $GITHUB_SHA" >> "$GITHUB_STEP_SUMMARY" + - name: Run before evals + uses: github/evals-action@6e4c077e92194c7e75663d8d782426841047e887 # v0.2.0 + with: + command: run + eval-paths: ${{ matrix.evalPath }} + vally-version: '0.19.0' + copilot-token: ${{ github.token }} + runs: 1 + workers: 1 + max-retries: 0 + upload-transcripts: false + report-id: ${{ matrix.id }}-before + - name: Select the target skill from the PR head + id: after_source + if: ${{ !cancelled() && steps.before_source.outcome == 'success' }} + env: + SOURCE_SHA: ${{ matrix.head }} + run: | + set -euo pipefail + git --literal-pathspecs restore --source="$SOURCE_SHA" --staged --worktree -- "$SKILL_DIR" + git --literal-pathspecs diff --exit-code "$SOURCE_SHA" -- "$SKILL_DIR" + if [[ -n "$(git diff --name-only HEAD | awk -v dir="$SKILL_DIR/" 'index($0,dir)!=1')$(git ls-files --others --exclude-standard)" ]]; then + echo "::error::Measurement files changed outside the selected skill." + exit 1 + fi + echo "After: $SKILL_DIR from $SOURCE_SHA; measurement revision $GITHUB_SHA" >> "$GITHUB_STEP_SUMMARY" + - name: Run after evals even if the before verdict failed + if: ${{ !cancelled() && steps.after_source.outcome == 'success' }} + uses: github/evals-action@6e4c077e92194c7e75663d8d782426841047e887 # v0.2.0 + with: + command: run + eval-paths: ${{ matrix.evalPath }} + vally-version: '0.19.0' + copilot-token: ${{ github.token }} + runs: 1 + workers: 1 + max-retries: 0 + upload-transcripts: false + report-id: ${{ matrix.id }}-after diff --git a/README.md b/README.md index 3ca8f26..4a002ab 100644 --- a/README.md +++ b/README.md @@ -197,7 +197,9 @@ request. **Paid evals:** `npm run eval` runs real Copilot sessions and costs AI credits. [Evals](.github/workflows/evals.yml) runs only when manually started; pull -requests never trigger paid evals. +requests that change a plugin skill use the separate +[before/after workflow](.github/workflows/skill-evals.yml), with maintainer approval +required in the protected `evals` environment. Fork PRs do not run paid evals. See [Checks and evals](docs/checks-and-evals.md) for CI details, local test commands, and Bash and PowerShell eval setup. diff --git a/docs/checks-and-evals.md b/docs/checks-and-evals.md index a4d6988..0c76df2 100644 --- a/docs/checks-and-evals.md +++ b/docs/checks-and-evals.md @@ -124,5 +124,31 @@ npm run eval:compare -- --baseline evals/_shared/fixtures/pr-description/baselin [Evals](../.github/workflows/evals.yml) runs the smoke suite only when you explicitly start it with **Run workflow** in GitHub Actions -(`workflow_dispatch`). Pull requests never start paid evals. Review the -selected branch and its eval code before dispatching a run. +(`workflow_dispatch`). Review the selected branch and its eval code before +dispatching a run. + +[Skill evals](../.github/workflows/skill-evals.yml) compares each changed existing +plugin skill with its version on current `main`. It uses the same PR merge +revision's evals, graders, fixtures and shared marketplace context for both runs; +only the selected skill comes from the exact base-parent or PR-head commit. +Both Action invocations run sequentially in one job, reusing the Action's verified +Vally installation. They use one trial per case and no trial retries. A failed +before verdict does not prevent the after run; failures remain visible, without +a new regression-only gate or statistical claim. + +A maintainer must first configure the `evals` environment with required +reviewers and review the PR's workflow, eval code and billing effects before +approval. The read-only selection job refuses missing or unprotected +environments, rather than automatically creating an unprotected environment. +Same-repository PRs are trusted code with model-billing permission, not an +OS/network sandbox; fork PRs are excluded and `pull_request_target` is not used. +The organization's Copilot billing policy must also permit workflow-token usage. +The workflow changes none of these repository or organization settings. + +New or deleted skills and skills without their corresponding +`evals//skills//eval.yaml` cannot form a before/after pair. They are +reported as not run, not counted as passing, and do not make evals mandatory for +every catalog skill. Each Action invocation retains its structured report; +raw transcript uploads and PR-comment permissions are disabled. The Action +manages its own runtime dependencies, so parity with the locally pinned Copilot +CLI is not assumed.