Repository navigation
Compare changed skills through the reusable Evals Action #15
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.
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
Open
ronniegeraghty
wants to merge
29
commits into
main
Choose a base branch
from
ronniegeraghty-skill-eval-workflow
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+997
−1,139
Open
Changes from all commits
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
a7ae2c1
Compare changed skills before and after with the native evals Action
ronniegeraghty 33720df
Render native before/after skill comparisons in one PR comment
ronniegeraghty 8b9a7be
Scope comparison artifacts to the workflow run attempt
ronniegeraghty d03c07a
Keep incomplete comparisons unknown and reject unsafe workflow retries
ronniegeraghty 2c47a19
Publish PR comparison data from a trusted workflow_run workflow
ronniegeraghty 4430df5
Move paid skill comparisons behind the trusted native Vally entrypoint
ronniegeraghty 6c1c553
Fix trusted workflow contexts and isolate comparison arm state
ronniegeraghty 943b65f
Bind comparison coverage to trusted selection and complete native counts
ronniegeraghty df46f6c
Allowlist paid comparison artifact filenames
ronniegeraghty 406535f
Merge reviewed main tooling and lock trusted native bootstrap depende…
ronniegeraghty a519c9b
Recheck PR eligibility before each paid arm and handle quoted source …
ronniegeraghty 4f212e9
Scope comparison cancellation to authenticated selected heads
ronniegeraghty 01bf547
Delegate PR comparisons to reusable Evals Action
ronniegeraghty 18a2ec9
Use native Vally results UI and comparison commands
ronniegeraghty 395b970
Pin corrected comparison Action implementation
ronniegeraghty 780ed81
Pin final NUL-safe comparison Action
ronniegeraghty b4da539
Merge local-backend compatibility and address comparison review
ronniegeraghty f2c16e7
Exclude uncatalogued skills from comparisons
ronniegeraghty 0aff5c0
Update comparison Action and authenticated workflow context
ronniegeraghty 7780c5d
Consume corrected comparison execution and publication guards
ronniegeraghty a269098
Cover unsafe selection paths and update comparison evidence guards
ronniegeraghty 28d14bd
Refresh comparison pin and clarify paid-eval approval
ronniegeraghty 8250837
Preserve scoped inventory credentials and refresh Git-state guards
ronniegeraghty 3aa54e6
Reject malformed inventory fields before remote reads
ronniegeraghty 36d911d
Consume native group fallback and failure-row corrections
ronniegeraghty 5ec6fa1
Consume scoped comparison setup failure codes
ronniegeraghty 2b1c998
Consume tracked Git-filter integrity guards
ronniegeraghty 73d89b6
Identify invalid selection inputs without exposing JSON
ronniegeraghty cde7dd8
Name the actual approval and paid execution workflow
ronniegeraghty File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Oops, something went wrong.
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,130 @@ | ||
| import test from "node:test"; | ||
| import assert from "node:assert/strict"; | ||
| import { randomUUID } from "node:crypto"; | ||
| import { copyFileSync, existsSync, mkdirSync, readFileSync, readdirSync, rmSync } from "node:fs"; | ||
| import { resolve } from "node:path"; | ||
| import { fileURLToPath, pathToFileURL } from "node:url"; | ||
| import { createDefaultGraderRegistry } from "@microsoft/vally"; | ||
| import { parse } from "yaml"; | ||
|
|
||
| const root = fileURLToPath(new URL("../../", import.meta.url)); | ||
| const prDescriptionTest = existsSync(resolve(root, "plugins/pr-description")) ? test : test.skip; | ||
|
|
||
| prDescriptionTest("pr-description eval stages catalog-local skills and enforces non-collision", async () => { | ||
| const evalDir = resolve(root, "evals/pr-description/skills/pr-description"); | ||
| const spec = parse(readFileSync(resolve(evalDir, "eval.yaml"), "utf8")); | ||
| assert.equal(spec.scoring.threshold, 1); | ||
| const project = parse(readFileSync(resolve(root, ".vally.yaml"), "utf8")); | ||
| assert.equal(spec.agent_environment, "marketplace"); | ||
| const skills = project.environments.marketplace.skills; | ||
| for (const configuredSkill of skills) { | ||
| assert.ok(existsSync(resolve(evalDir, configuredSkill)), `configured cohort skill path is missing: ${configuredSkill}`); | ||
| } | ||
| const staged = new Set(skills.map((skill) => resolve(evalDir, skill))); | ||
| const catalog = JSON.parse(readFileSync(resolve(root, "marketplace.json"), "utf8")); | ||
| for (const plugin of catalog.plugins.filter((plugin) => typeof plugin.source === "string")) { | ||
| const directory = resolve(root, plugin.source, "skills"); | ||
| if (!existsSync(directory)) continue; | ||
| for (const skill of readdirSync(directory)) { | ||
| const source = resolve(directory, skill); | ||
| if (existsSync(resolve(source, "SKILL.md"))) { | ||
| assert.ok(staged.has(source), `Add catalog skill ${plugin.name}/${skill} to the native marketplace environment.`); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const grader = createDefaultGraderRegistry().get("skill-invocation"); | ||
| const positive = spec.stimuli.find((item) => item.tags?.control === "positive"); | ||
| const negative = spec.stimuli.find((item) => item.tags?.control === "negative"); | ||
| const posConfig = positive.graders.find((item) => item.type === "skill-invocation").config; | ||
| const negConfig = negative.graders.find((item) => item.type === "skill-invocation").config; | ||
| const activation = (name) => ({ type: "skill_activation", data: { name } }); | ||
|
|
||
| const exactMatch = await grader.grade({ | ||
| stimulus: positive, config: posConfig, | ||
| trajectory: { output: "ok", events: [activation("pr-description")] }, | ||
| }); | ||
| assert.equal(exactMatch.passed, true); | ||
|
|
||
| if (posConfig.disallowed?.length) { | ||
| const neighborCollision = await grader.grade({ | ||
| stimulus: positive, config: posConfig, | ||
| trajectory: { output: "ok", events: [activation("pr-description"), activation(posConfig.disallowed[0])] }, | ||
| }); | ||
| assert.equal(neighborCollision.passed, false, "activating a configured neighbor must fail"); | ||
| } | ||
|
|
||
| const negativeTriggered = await grader.grade({ | ||
| stimulus: negative, config: negConfig, | ||
| trajectory: { output: "ok", events: [activation("pr-description")] }, | ||
| }); | ||
| assert.equal(negativeTriggered.passed, false, "activating pr-description on the negative case must fail"); | ||
| }); | ||
|
|
||
| prDescriptionTest("pr-description program grader enforces ordered headings, factual changes, and unshipped backlog exclusion", async (t) => { | ||
| const gradingDir = resolve(root, ".github/scripts", `.pr-grader-test-${randomUUID()}`); | ||
| mkdirSync(gradingDir, { recursive: true }); | ||
| t.after(() => rmSync(gradingDir, { recursive: true, force: true })); | ||
| copyFileSync(resolve(root, "evals/pr-description/skills/pr-description/grader.mjs"), resolve(gradingDir, "grader.mjs")); | ||
| copyFileSync(resolve(root, "evals/_shared/graders/common.mjs"), resolve(gradingDir, "common.mjs")); | ||
| const { evaluate: evaluatePrDescription } = await import(pathToFileURL(resolve(gradingDir, "grader.mjs"))); | ||
| const { score } = await import(pathToFileURL(resolve(gradingDir, "common.mjs"))); | ||
| const validOutput = [ | ||
| "## Summary", | ||
| "Validate `--timeout` as a positive integer in `src/cli.mjs` and add unit tests. Retry backoff (#42) remains open and out of scope.", | ||
| "", | ||
| "## Changes", | ||
| "- Added positive integer validation for `--timeout` in `src/cli.mjs`.", | ||
| "- Added unit tests for `--timeout 0` and `--timeout -5` in `test/cli.test.mjs`.", | ||
| "", | ||
| "## Verification", | ||
| "- `npm test` passed (14 tests).", | ||
| "", | ||
| "## Risks / Rollback", | ||
| "- Low risk; revert the commit if needed.", | ||
| ].join("\n"); | ||
| assert.equal(score("pr-description-contract", evaluatePrDescription({ output: validOutput, events: [] })).passed, true); | ||
| for (const passingLine of [ | ||
| "- `npm test` (14 tests): passed.", | ||
| "- `npm test` \u2014 14 tests, all passing.", | ||
| "- Ran `npm test`; 14/14 tests passed.", | ||
| "- `npm test`: 14 tests passed, 0 failed, 0 errors.", | ||
| "- `npm test` passed (14 tests, no errors).", | ||
| "- `npm test`: 14 passed, # fail 0, 0 failures.", | ||
| ]) { | ||
| const output = validOutput.replace("- `npm test` passed (14 tests).", passingLine); | ||
| assert.equal(score("pr-description-contract", evaluatePrDescription({ output, events: [] })).passed, true, passingLine); | ||
| } | ||
| for (const negatedLine of [ | ||
| "- `npm test` (14 tests).", | ||
| "- `npm test` did not pass (14 tests).", | ||
| "- `npm test` (14 tests): not passing.", | ||
| "- `npm test`: 14 tests; 12 passed, 2 failures.", | ||
| "- `npm test` failed (14 tests).", | ||
| ]) { | ||
| const output = validOutput.replace("- `npm test` passed (14 tests).", negatedLine); | ||
| const result = score("pr-description-contract", evaluatePrDescription({ output, events: [] })); | ||
| assert.equal(result.passed, false, negatedLine); | ||
| assert.match(result.evidence, /Recorded the supplied npm test verification result/); | ||
| } | ||
| for (const leakedChanges of [ | ||
| "- Added unit tests for `--timeout 0` and `--timeout -5` in `test/cli.test.mjs`.\n- Implemented exponential retry backoff (#42).", | ||
| "- Rejects `--timeout` values that are not positive integers.\n- Adds exponential retry backoff (#42).", | ||
| ]) { | ||
| const output = validOutput.replace( | ||
| "- Added unit tests for `--timeout 0` and `--timeout -5` in `test/cli.test.mjs`.", leakedChanges); | ||
| const result = score("pr-description-contract", evaluatePrDescription({ output, events: [] })); | ||
| assert.equal(result.passed, false); | ||
| assert.match(result.evidence, /Excluded unshipped retry backoff/); | ||
| } | ||
| const missingRisks = validOutput.split("## Risks / Rollback")[0]; | ||
| assert.equal(score("pr-description-contract", evaluatePrDescription({ output: missingRisks, events: [] })).passed, false); | ||
| for (const prohibitedTool of ["bash", "functions.bash", "functions.powershell", "mcp:edit", "tools/write_file"]) { | ||
| const result = score("pr-description-contract", evaluatePrDescription({ | ||
| output: validOutput, | ||
| events: [{ type: "tool_call", data: { toolCallId: "1", toolName: prohibitedTool, arguments: { command: "git status" } } }], | ||
| })); | ||
| assert.equal(result.passed, false, `must reject prohibited tool ${prohibitedTool}`); | ||
| assert.match(result.evidence, /Did not run shell commands or create a pull request/); | ||
| } | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| import { execFileSync, spawnSync } from "node:child_process"; | ||
| import { createHash } from "node:crypto"; | ||
| import { appendFileSync, existsSync, readFileSync, readdirSync, realpathSync, writeFileSync } from "node:fs"; | ||
| import { resolve } from "node:path"; | ||
| import { fileURLToPath } from "node:url"; | ||
| import { githubApi } from "./check-remote-pins.mjs"; | ||
|
|
||
| // Unlike $, the end assertion also rejects a final line break. | ||
| const name = /^[a-z0-9][a-z0-9._-]*(?![\s\S])/; | ||
| const sha = /^[a-f0-9]{40}(?![\s\S])/; | ||
| const parse = (contents, subject) => { | ||
| try { return JSON.parse(contents); } catch (error) { | ||
| if (!(error instanceof SyntaxError)) throw error; | ||
| throw new Error(`Invalid ${subject} JSON.`); | ||
| } | ||
| }; | ||
| const json = (path, subject) => parse(readFileSync(path, "utf8"), subject); | ||
|
|
||
| function pinnedTree(repo, commit) { | ||
| const response = githubApi(`repos/${repo}/git/trees/${commit}?recursive=1`, (command, args, options) => | ||
| spawnSync(command, args, { ...options, timeout: 30000, maxBuffer: 8 * 1024 * 1024 })); | ||
| if (response.status !== 200) throw new Error(`Pinned skill inventory returned GitHub HTTP ${response.status}.`); | ||
| return parse(response.body, "pinned remote inventory"); | ||
| } | ||
|
|
||
| export function inventorySkills(root, readTree = pinnedTree) { | ||
| const skills = []; | ||
| for (const plugin of json(resolve(root, "marketplace.json"), "marketplace catalog").plugins) { | ||
| if (typeof plugin.name !== "string" || !name.test(plugin.name)) throw new Error("Unsupported catalog plugin name."); | ||
| let names; | ||
| if (typeof plugin.source === "string") { | ||
| if (plugin.source !== `./plugins/${plugin.name}` && plugin.source !== `plugins/${plugin.name}`) { | ||
| throw new Error("Expected a self-contained local plugin path."); | ||
| } | ||
| const directory = resolve(root, plugin.source, "skills"); | ||
| names = existsSync(directory) ? readdirSync(directory).filter((skill) => | ||
| name.test(skill) && existsSync(resolve(directory, skill, "SKILL.md"))) : []; | ||
| } else { | ||
| const source = plugin.source; | ||
| if (source?.source !== "github" || typeof source.repo !== "string" || !/^[\w.-]+\/[\w.-]+(?![\s\S])/.test(source.repo) || | ||
| typeof source.sha !== "string" || !sha.test(source.sha)) { | ||
| throw new Error("Remote skill inventory requires a pinned GitHub source."); | ||
| } | ||
| if (source.path !== undefined && (typeof source.path !== "string" || source.path.split("/").includes(".."))) { | ||
| throw new Error("Unsupported remote plugin path."); | ||
| } | ||
| const prefix = source.path ? `${source.path}/` : ""; | ||
| const tree = readTree(source.repo, source.sha); | ||
| if (tree.truncated || !Array.isArray(tree.tree)) throw new Error("Incomplete pinned remote skill inventory."); | ||
| names = tree.tree.flatMap((entry) => { | ||
| if (entry.type !== "blob" || !entry.path.startsWith(`${prefix}skills/`)) return []; | ||
| const parts = entry.path.slice(`${prefix}skills/`.length).split("/"); | ||
| return parts.length === 2 && parts[1] === "SKILL.md" && name.test(parts[0]) ? [parts[0]] : []; | ||
| }); | ||
| } | ||
| for (const skill of names) skills.push({ | ||
| key: `${plugin.name}/${skill}`, | ||
| hasEval: existsSync(resolve(root, "evals", plugin.name, "skills", skill, "eval.yaml")), | ||
| }); | ||
| } | ||
| return skills.sort((a, b) => a.key.localeCompare(b.key)); | ||
| } | ||
|
|
||
| export function selectSkillEvals(root, context, environment, readTree = pinnedTree) { | ||
| if (!environment.protection_rules?.some((rule) => rule.type === "required_reviewers" && rule.reviewers?.length)) { | ||
| throw new Error("Configure the evals environment with required maintainer reviewers before enabling paid PR evals."); | ||
| } | ||
| const git = (...args) => execFileSync("git", args, { cwd: root, encoding: "utf8" }).trim(); | ||
| if (![context.base, context.head, context.merge].every((value) => sha.test(value ?? "")) || | ||
| git("rev-parse", "HEAD") !== context.merge || git("rev-parse", "HEAD^1") !== context.base || | ||
| git("rev-parse", "HEAD^2") !== context.head) { | ||
| throw new Error("Expected the verified PR merge and its exact parents."); | ||
| } | ||
| const changed = execFileSync("git", ["diff", "--no-renames", "--name-only", "-z", context.base, "HEAD", "--", "plugins"], | ||
| { cwd: root, 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 skills = inventorySkills(root, readTree); | ||
| const catalogued = new Set(skills.map((skill) => skill.key)); | ||
| const include = []; | ||
| for (const key of keys) { | ||
| if (key.split("/").length !== 2 || !key.split("/").every((part) => name.test(part))) { | ||
| 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`], { cwd: root, 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 (catalogued.has(key) && present(context.base) && present(context.head) && existsSync(resolve(root, evalPath))) { | ||
| include.push({ key, id: `skill-${createHash("sha256").update(key).digest("hex").slice(0, 12)}`, | ||
| directory, evalPath, base: context.base, head: context.head }); | ||
| } | ||
| } | ||
| return { ...context, include, changed: keys, skills }; | ||
| } | ||
|
|
||
| if (process.argv[1] && realpathSync(process.argv[1]) === realpathSync(fileURLToPath(import.meta.url))) { | ||
| try { | ||
| const [contextFile, environmentFile, outputFile] = process.argv.slice(2); | ||
| if (!contextFile || !environmentFile || !outputFile) throw new Error("Expected context, environment and selection file paths."); | ||
| const selection = selectSkillEvals(process.cwd(), | ||
| json(contextFile, "comparison context"), json(environmentFile, "evals environment")); | ||
| for (const key of selection.changed.filter((key) => !selection.include.some((entry) => entry.key === key))) { | ||
| console.log(`::warning::No before/after pair for ${key}: a catalogued skill, both source versions and its repo eval are required. No eval was run.`); | ||
| } | ||
| writeFileSync(outputFile, `${JSON.stringify(selection)}\n`); | ||
| appendFileSync(process.env.GITHUB_OUTPUT, | ||
| `matrix=${JSON.stringify({ include: selection.include })}\ncount=${selection.include.length}\nattempt=${selection.attempt}\nselection=${JSON.stringify(selection)}\n`); | ||
| } catch (error) { | ||
| console.error(error.message); | ||
| process.exitCode = 1; | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.