Skip to content

Add a Vally-based evaluation harness for repository-owned Copilot customizations. - #39002

Merged
AndriySvyryd merged 3 commits into
mainfrom
Vally
Sep 18, 2026
Merged

AndriySvyryd merged 3 commits into
mainfrom
Vally

Conversation

@AndriySvyryd

Copy link
Copy Markdown
Member
  • Discovers and validates instructions, skills, agents, prompts, and agentic workflows by convention.
  • Runs paired treatment and unskilled-control evaluations with token grading and Vally comparison.
  • Selectively evaluates changed components and publishes progressive PR results.
  • Restricts secret-backed runs to trusted PR authors; later commits require manual reruns.

Replace repository MCP configuration and duplicated skills with plugins.
Update skill guidance and add matching repository-grounded evaluations.
Add Dependabot coverage for Vally dependencies.

Copilot AI left a comment

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.

🟡 Changes recommended

Secret-bearing PR-head execution and control-isolation defects must be addressed before approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds a Vally-based harness to validate and compare repository-owned Copilot customizations.

Changes:

  • Adds discovery, validation, preparation, evaluation, and comparison tooling.
  • Adds CI workflows and evaluations for repository skills and instructions.
  • Replaces local MCP configuration and duplicated testing guidance with plugins.
File summaries
File Description
eng/harness-evaluation/test/harness.test.mjs Tests harness behavior.
eng/harness-evaluation/src/harness.mjs Implements harness logic.
eng/harness-evaluation/src/cli.mjs Adds harness CLI.
eng/harness-evaluation/README.md Documents the harness.
eng/harness-evaluation/package.json Defines Node dependencies and scripts.
eng/harness-evaluation/package-lock.json Locks evaluation dependencies.
eng/harness-evaluation/instructions/copilot-instructions/eval.yaml Evaluates repository instructions.
eng/harness-evaluation/skills/change-tracking/eval.yaml Evaluates change-tracking guidance.
eng/harness-evaluation/skills/cosmos-provider/eval.yaml Evaluates Cosmos guidance.
eng/harness-evaluation/skills/make-custom-agent/eval.yaml Evaluates agent authoring.
eng/harness-evaluation/skills/make-github-actions-workflow/eval.yaml Evaluates workflow authoring.
eng/harness-evaluation/skills/make-instructions/eval.yaml Evaluates instruction authoring.
eng/harness-evaluation/skills/make-skill/eval.yaml Evaluates skill authoring.
eng/harness-evaluation/skills/migrations/eval.yaml Evaluates migrations guidance.
eng/harness-evaluation/skills/model-building/eval.yaml Evaluates model-building guidance.
eng/harness-evaluation/skills/query-pipeline/eval.yaml Evaluates query guidance.
eng/harness-evaluation/skills/run-apichief/eval.yaml Evaluates ApiChief guidance.
eng/harness-evaluation/skills/scaffolding/eval.yaml Evaluates scaffolding guidance.
eng/harness-evaluation/skills/servicing-pr/eval.yaml Evaluates servicing guidance.
eng/harness-evaluation/skills/sqlite-adonet/eval.yaml Evaluates SQLite guidance.
eng/harness-evaluation/skills/tooling/eval.yaml Evaluates tooling guidance.
eng/harness-evaluation/skills/triage/eval.yaml Evaluates issue triage.
eng/harness-evaluation/skills/update-pipeline/eval.yaml Evaluates update-pipeline guidance.
.github/workflows/agent-harness-evaluation.yml Runs behavioral evaluations.
.github/workflows/agent-harness-coverage.yml Validates harness coverage.
.github/dependabot.yml Adds npm update coverage.
.github/copilot/settings.json Enables Copilot plugins.
.github/copilot-instructions.md Centralizes testing guidance.
.claude/settings.json Enables corresponding Claude plugins.
.vscode/mcp.json Removes duplicated MCP configuration.
.agents/skills/testing/SKILL.md Removes duplicated testing skill.
.agents/skills/sqlite-adonet/SKILL.md Clarifies command lifecycle.
.agents/skills/servicing-pr/SKILL.md Clarifies quirk recommendations.
.agents/skills/run-apichief/SKILL.md Updates repository references.
.agents/skills/migrations/SKILL.md Expands migration guidance.
.agents/skills/make-skill/SKILL.md Adds paired-evaluation workflow.
.agents/skills/make-skill/references/testing-patterns.md Replaces Waza with Vally guidance.
.agents/skills/make-instructions/SKILL.md Adds instruction evaluation guidance.
.agents/skills/make-custom-agent/SKILL.md Updates related-skill references.
.agents/skills/change-tracking/SKILL.md Refines change-tracking guidance.
Review details

Files not reviewed (1)

  • eng/harness-evaluation/package-lock.json: Generated file
  • Files reviewed: 39/40 changed files
  • Comments generated: 6
  • Review effort level: Balanced

Comment thread .github/workflows/agent-harness-evaluation.yml Outdated
Comment thread .github/workflows/agent-harness-evaluation.yml Outdated
Comment thread eng/harness-evaluation/package.json Outdated
Comment thread eng/harness-evaluation/skills/update-pipeline/eval.yaml Outdated
Comment thread eng/harness-evaluation/src/harness.mjs Outdated
Comment thread eng/harness-evaluation/skills/make-instructions/eval.yaml Outdated

Copilot AI left a comment

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.

🟡 Changes recommended

Manual authorization is not commit-bound, and control isolation and result cleanup contain correctness gaps.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

  • eng/harness-evaluation/package-lock.json: Generated file
  • Files reviewed: 39/40 changed files
  • Comments generated: 4
  • Review effort level: Balanced

Comment thread .github/workflows/agent-harness-evaluation.yml Outdated
Comment thread .github/workflows/agent-harness-evaluation.yml
Comment thread eng/harness-evaluation/src/harness.mjs Outdated
Comment thread .github/copilot-instructions.md Outdated

Copilot AI left a comment

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.

🟡 Changes recommended

Privileged execution of PR-controlled code and control-arm contamination can compromise security and invalidate evaluation results.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

  • eng/harness-evaluation/package-lock.json: Generated file
  • Files reviewed: 40/41 changed files
  • Comments generated: 7
  • Review effort level: Balanced

Comment thread .github/workflows/agent-harness-evaluation.yml Outdated
Comment thread eng/harness-evaluation/src/harness.mjs
Comment thread eng/harness-evaluation/src/harness.mjs Outdated
Comment thread .github/workflows/agent-harness-evaluation.yml Outdated
Comment thread eng/harness-evaluation/src/harness.mjs Outdated
Comment thread eng/harness-evaluation/src/harness.mjs Outdated
Comment thread .github/copilot-instructions.md Outdated

Copilot AI left a comment

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.

🟡 Changes recommended

Component selection, control isolation, path deletion safety, and rerun notification defects remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

  • eng/harness-evaluation/package-lock.json: Generated file

Suppressed comments (5)

Previously missed (5) — in code that hasn't changed since the last review.

.github/workflows/agent-harness-evaluation.yml:234

  • The production selector has the same resource-only gap as the local selector: changes below a skill's references, scripts, or assets directories do not match its SKILL.md source path, so the workflow skips the affected skill evaluation. Match skill components using the parent skill directory.
    .github/workflows/agent-harness-evaluation.yml:346
  • This unconditional return leaves a PR without rerun instructions when a synchronize event introduces its first harness-affecting change: there are no prior evaluation comments, so the stale notice above is not created, and synchronize runs are intentionally not authorized to evaluate. Return here only when a stale notice already covers the existing evaluations; otherwise continue to create the manual-required prompt.
    eng/harness-evaluation/src/harness.mjs:370
  • Relative repository paths are resolved only from the copied eval directory. Consequently, an authored target reference such as src: .agents/skills/example/SKILL.md with an unrelated destination is retained in the control, leaking the evaluated skill into the supposedly unskilled arm. Check both eval-relative and repository-relative interpretations before retaining a path.
    eng/harness-evaluation/src/harness.mjs:502
  • Skill resources are part of the evaluated component, but this matches changes only against SKILL.md. A resource-only edit such as .agents/skills/make-skill/references/testing-patterns.md therefore selects no evaluation. Match skill changes against the skill directory while retaining file matching for other component kinds.
    .agents/skills/sqlite-adonet/SKILL.md:17
  • This is phrased as current implementation behavior, but SqliteConnection.Close() currently iterates _commands and calls command.Dispose() (src/Microsoft.Data.Sqlite.Core/SqliteConnection.cs:372-380). That contradiction will mislead the evaluation away from the ownership-boundary bug it is meant to diagnose. State the expected ADO.NET lifetime rule normatively rather than claiming the current code already follows it.
  • Files reviewed: 40/41 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread eng/harness-evaluation/src/cli.mjs Outdated
@AndriySvyryd
AndriySvyryd marked this pull request as ready for review September 17, 2026 06:31
@AndriySvyryd
AndriySvyryd requested review from a team and wtgodbe as code owners September 17, 2026 06:31
Copilot AI review requested due to automatic review settings September 17, 2026 06:31

Copilot AI left a comment

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.

🟡 Changes recommended

Skill sidecar changes currently bypass evaluation selection, and the CLI can recursively delete an arbitrary output path.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

  • eng/harness-evaluation/package-lock.json: Generated file
  • Files reviewed: 41/42 changed files
  • Comments generated: 3
  • Review effort level: Balanced

Comment thread eng/harness-evaluation/src/cli.mjs
Comment thread .github/workflows/agent-harness-evaluation.yml Outdated
Comment thread eng/harness-evaluation/src/harness.mjs Outdated

Copilot AI left a comment

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.

🟡 Changes recommended

Unsafe output deletion and incorrect Vally result verification remain blocking issues.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

  • eng/harness-evaluation/package-lock.json: Generated file

Suppressed comments (3)

.github/workflows/agent-harness-evaluation.yml:315

  • When a synchronize commit removes the last harness-related change, selected is empty but prior evaluation comments still make this branch create/update a “rerun required” notice. The code then returns at line 327, so the PR simultaneously reports no affected components and asks for an impossible /eval rerun. Handle the empty selection before creating the stale notice and remove any existing rerun prompt in that path.
            if (context.payload.action === 'synchronize' && evalComments.length > 0) {
              const body = `${rerunMarker}\n### Agent harness evaluation is stale\n\n` +
                `New commits were added after the previous evaluation. A contributor with write access can comment ` +
                `\`/eval\` to run all affected components or \`/eval component-name\` to run one component.`;

.github/workflows/agent-harness-evaluation.yml:208

  • GitHub's pull-request files endpoint caps results at 3,000 files. This branch paginates but never detects that cap, so a very large PR can silently omit changed customizations and produce an incomplete evaluation matrix. Fail closed when the result reaches 3,000 entries, as the historical-commit branch already does for its lower API cap.
              files = await github.paginate(github.rest.pulls.listFiles, {
                owner: context.repo.owner,
                repo: context.repo.repo,
                pull_number: pullNumber,
                per_page: 100
              });

eng/harness-evaluation/README.md:28

  • defaults.runs is an eval-level field, not a field that each stimulus sets; all of the evals added here declare it once beside stimuli. As written, this rule says every stimulus must contain an impossible/invalid setting and misstates what the validator enforces.
Every stimulus must:

1. Set `defaults.runs: ${RUNS=5}`.
  • Files reviewed: 41/42 changed files
  • Comments generated: 10
  • Review effort level: Balanced

Comment thread .github/workflows/agent-harness-evaluation.yml
Comment thread eng/harness-evaluation/src/harness.mjs Outdated
Comment thread .claude/settings.json
Comment thread .github/copilot/settings.json
Comment thread .github/workflows/agent-harness-coverage.yml Outdated
Comment thread .github/workflows/agent-harness-evaluation.yml Outdated
Comment thread eng/harness-evaluation/README.md Outdated
Comment thread eng/harness-evaluation/skills/make-custom-agent/eval.yaml Outdated
Comment thread eng/harness-evaluation/src/harness.mjs
Comment thread eng/harness-evaluation/src/harness.mjs
Copilot AI review requested due to automatic review settings September 17, 2026 19:52

Copilot AI left a comment

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.

🟡 Changes recommended

The privileged evaluation path still has trust-boundary and output-containment defects.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

  • eng/harness-evaluation/package-lock.json: Generated file

Suppressed comments (3)

eng/harness-evaluation/skills/sqlite-adonet/eval.yaml:16

  • The stimulus is based on behavior the staged provider does not have: SqliteConnection.Close() disposes tracked commands, so it cannot preserve a reusable command across close/reopen. This eval will reward the skill for proposing behavior contrary to the actual lifecycle; replace the premise with a scenario consistent with the implementation, or stage the corresponding provider change before evaluating it.
    eng/harness-evaluation/skills/sqlite-adonet/eval.yaml:16
  • The stimulus is built around behavior the staged implementation does not have: SqliteConnection.Close() disposes tracked commands, so the requested conclusion to preserve command reuse after reopening is false. This makes the treatment/control evaluation reward incorrect guidance and can drive a proposed fix in the wrong direction; require the analysis to recognize that commands are not reusable after close/reopen, or stage an implementation that actually supports that contract.
    eng/harness-evaluation/skills/sqlite-adonet/eval.yaml:16
  • The eval asks the agent to diagnose command state surviving DbConnection.Close() and to preserve command reuse after reopening, but the staged SqliteConnection.Close() implementation disposes every tracked command before closing. This makes the evaluation reward an impossible/incorrect diagnosis rather than repository-grounded guidance; rewrite the stimulus around the actual close/dispose lifecycle or stage a source change that establishes a different contract.
  • Files reviewed: 41/44 changed files
  • Comments generated: 5
  • Review effort level: Balanced

Comment thread eng/harness-evaluation/src/cli.mjs
Comment thread eng/harness-evaluation/src/harness.mjs
Comment thread .github/workflows/agent-harness-evaluation.yml Outdated
Comment thread eng/harness-evaluation/src/cli.mjs
Comment thread eng/harness-evaluation/src/harness.mjs
Copilot AI review requested due to automatic review settings September 18, 2026 21:08

Copilot AI left a comment

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.

🟡 Changes recommended

Result lifecycle, CLI validation, and paired-evaluation checks can currently produce incorrect harness outcomes.

Get a fresh assessment by requesting another Copilot review.

Review details

Files not reviewed (1)

  • eng/harness-evaluation/package-lock.json: Generated file

Suppressed comments (3)

eng/harness-evaluation/src/cli.mjs:71

  • repoRoot is the candidate checkout, so this makes a PR-controlled harness.experiment.yaml part of the executed runner. A PR that changes both an eval and this file can therefore alter variants or other experiment behavior before merge, contradicting the trusted-runner boundary documented in README.md:35 and the workflow comment at .github/workflows/agent-harness-evaluation.yml:244. Load the experiment definition from the trusted harness checkout instead; the candidate should supply only its customization, eval spec, and declared inputs.
    eng/harness-evaluation/src/cli.mjs:71
  • This makes the trusted CLI load harness.experiment.yaml from the candidate checkout. A PR that changes an eval together with this file can alter baseline, variants, or vary so the selected treatment/control comparison is no longer the committed experiment, even though infrastructure changes are supposed to be evaluated only after merge. Load the experiment definition from the trusted runner (packageRoot) or reject mixed infrastructure/component runs.
    eng/harness-evaluation/src/harness.mjs:330
  • A non-empty set of successful trials is treated as complete here; there is no check that all configured runs/stimuli were produced. Thus a truncated experiment containing one passing trial can satisfy --require-pass (the test fixture itself has this shape), even though the acceptance contract requires the configured number of control and treatment trials. Validate the result against Vally's plan snapshot or expected trial keys before computing the verdict, as the workflow verifier already does.
  • Files reviewed: 41/44 changed files
  • Comments generated: 4
  • Review effort level: Balanced

Comment thread .github/workflows/agent-harness-evaluation.yml Outdated
Comment thread eng/harness-evaluation/skills/make-instructions/eval.yaml Outdated
Comment thread eng/harness-evaluation/skills/make-skill/eval.yaml Outdated
Comment thread eng/harness-evaluation/src/cli.mjs

Copilot AI left a comment

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.

Comment thread .github/workflows/agent-harness-evaluation.yml
Comment thread .github/workflows/agent-harness-evaluation.yml Outdated
Comment thread .agents/skills/update-pipeline/SKILL.md Outdated
Comment thread .github/workflows/pr-comment-command.yml
Copilot AI review requested due to automatic review settings September 18, 2026 21:53

Copilot AI left a comment

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.

Comment thread eng/harness-evaluation/src/harness.mjs
Comment thread .github/workflows/agent-harness-evaluation.yml Outdated
Comment thread eng/harness-evaluation/src/harness.mjs
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

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.

Comment thread .github/workflows/agent-harness-evaluation.yml Outdated
Comment thread eng/harness-evaluation/src/cli.mjs Outdated
Comment thread .github/workflows/agent-harness-evaluation.yml
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 18, 2026 22:50

Copilot AI left a comment

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.

Copilot review overview

🔵 Needs a closer look

The control configuration and affected-component selection need correction, while privileged model-backed automation warrants human validation.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (2)
Files not reviewed (1)
  • eng/harness-evaluation/package-lock.json: Generated file
Previously missed (1)

In code that hasn't changed since last review

Medium severity Shared eval inputs are excluded from affected-component selection

.github/​workflows/​agent-harness-evaluation.yml:254

Affected-component selection checks only each component's source and eval directory, but evaluations also consume shared candidate files through stimulus-level agent_environment.files. For example, the make-skill, make-instructions, and make-custom-agent evals stage .github/copilot-instructions.md and this harness README. Changing either shared input can therefore skip those affected evaluations and leave their reported result stale. Track declared eval input paths when selecting components, or conservatively fan out evaluations when shared inputs change.

Comment thread eng/harness-evaluation/harness.experiment.yaml
@AndriySvyryd
AndriySvyryd merged commit 0ea4fb3 into main Sep 18, 2026
17 checks passed
@AndriySvyryd
AndriySvyryd deleted the Vally branch September 18, 2026 23:25
@dotnet-milestone-bot dotnet-milestone-bot Bot added this to the 12.0-preview1 milestone Sep 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants