Add eval-review skill/eval quality checker (eng/vally-graders)

[AbhitejJohn]

What this is

A standalone, repo-local eval-review skill/eval quality checker under eng/vally-graders/. It reviews the quality of skills and their evals before merge and reports findings as [SEVERITY] dimension (rule) — file:line / PROOF / WHY / FIX.

Graders:

Grader	Kind	Checks
eval-deleading	static / free	Fixture comments or eval prompts that leak the answer
assertion-hardening	complex-static / free	Mutating scenarios (edit/fix/migrate provided files) with no result gate (run_command_and_assert / file_contains / exit_success) — a wrong edit can still pass
meta-commentary	static / free	Unsupported provenance asides in shipped skill text ("learned from telemetry", "grounded in N PRs")
negative-scenario	static / free	A DO NOT USE FOR exclusion with no eval scenario asserting expect_activation: false
eval-review	llm / opt-in, paid	Real judge-model call for the judgment dimensions (fixture honesty, design balance, domain-correctness & safety)

Enforcement path: the four static graders run via the review.mjs runner, gated on PRs as free, static-only steps in the existing check job of skill-check.yml (no new workflow), scoped to changed units only. A run-all path is available via review.mjs --all (CI toggle EVAL_REVIEW_ALL=1).

Vally compatibility (documented, not a regression): the graders are also exported as Vally-compatible Grader objects via registerGraders(registry), so vally eval/grade --grader-plugin ./eng/vally-graders can load them. But Vally does not currently enforce these graders in this repo's flow: vally lint --grader-plugin does not execute custom static graders against skills (its lint registry only holds built-ins; the plugin registry is used only to validate grader types in an eval spec), and vally experiment run — this repo's actual eval path — has no --grader-plugin option. This is why enforcement is standalone via review.mjs. See eng/vally-graders/README.md.

The paid LLM grader is excluded from the free PR gate — it runs only behind an explicit review.mjs --llm opt-in.

Proven vs. inferred (honest scope)

• PROVEN: the static graders and the LLM grader's graceful-degradation behavior.
  • npm test → 20/20 pass (includes 5 LLM tests; also proves the checker works with the optional @github/copilot-sdk/zod deps omitted, via a hand-rolled schema fallback).
  • Full run across 97 skill+eval units produced stable, reproducible findings with quoted file:line proof.
• INFERRED (not proven): the LLM grader's live model output. The local environment lacks the @github/copilot runtime binary the SDK spawns, so a live judge reply could not be produced here. Critically, the grader correctly returned an inconclusive NOTE rather than a fabricated finding — which is the designed, safe behavior. CI never runs the LLM path.

Demo results

node eng/vally-graders/review.mjs --llm --unit migrate-mstest-v3-to-v4 — real code path. Representative static findings (quoted proof):

[MAJOR] assertion-hardening (B1) — tests/dotnet-test-migration/migrate-mstest-v3-to-v4/eval.yaml:243
  PROOF: "- name: \"Handle MSTest.Sdk and MTP changes in v4\""
  WHY:   Scenario sets up project files and asks the agent to change them, but only has weak
         assertions (output_matches) and/or a prose rubric. A wrong or half-done edit can still pass.
  FIX:   Add a hard gate: run_command_and_assert (dotnet build / dotnet test) and/or
         file_contains / file_not_contains asserting the edited file's shape.

[MAJOR] negative-scenario (D4) — plugins/dotnet-test-migration/skills/migrate-mstest-v3-to-v4/SKILL.md:15
  PROOF: "DO NOT USE FOR: migrating from MSTest v1/v2 to v3 (use migrate-mstest-v1v2-to-v3"
  WHY:   The exclusion hands off to a sibling skill but no eval scenario asserts non-activation
         (expect_activation: false). The routing boundary is unenforced.
  FIX:   Add a scenario using expect_activation: false that prompts for the excluded use case.

And the LLM grader degraded gracefully (no false finding):

NOTE: eval-review (llm) inconclusive: Copilot runtime failed to start.
      (token source: GH_TOKEN; a plain gh CLI token may lack Copilot scope) …
      Ensure @github/copilot is installed.

Aggregate (full review.mjs --all, 97 units)

0 BLOCKER · 73 MAJOR · 153 MINOR · 0 INFO → verdict: Fix-then-ship.

Per-dimension MAJOR breakdown:

• assertion-hardening: 27 MAJOR (all in dotnet-test-migration), 130 MINOR
• negative-scenario: 46 MAJOR, 22 MINOR
• eval-deleading: 1 MINOR
• meta-commentary: 0

These findings are pre-existing, not introduced by this PR

The 73 MAJOR are pre-existing eval-quality issues surfaced by the new checker, not defects this PR adds. The gate is deliberately set up so it won't retroactively fail existing skills:

• Scope = changed units only. Untouched skills are never scanned by the PR gate.
• Dimension ratchet. Only assertion-hardening and meta-commentary are enforced (blocking); the noisier eval-deleading and negative-scenario run report-only (warnings) during rollout. So of the 73 MAJOR, only the 27 assertion-hardening MAJOR (all in dotnet-test-migration) are blocking — and only if a PR actually touches those units, which is the intended behavior (fix-forward). The remaining 46 negative-scenario MAJOR surface as non-blocking warnings.

Enforcement can be tightened later by dropping the --report-only flags as each dimension's backlog is burned down.

Notes

• Plain Node ESM (.mjs), matching eng/ conventions; js-yaml is the only required runtime dep. @github/copilot-sdk + zod are optional deps used only by --llm.
• Intentionally not added to the is_infra change-globs in evaluation.yml (those trigger full model evals; the graders aren't in Vally's eval path, so it would burn eval budget for zero signal). Grader changes are covered by npm test in the check job.
• skill-check.yml validated with actionlint v1.7.7 (exit 0).