Run smoke evaluations for Vally updates - #1028
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 03c6d29f-0826-4c56-b97f-92c3169d204e
There was a problem hiding this comment.
Pull request overview
Updates the evaluation and triage infrastructure to treat eng/evaluation-tools/** (Vally dependency/tooling manifests) as evaluation-infrastructure changes, running a two-plugin smoke evaluation that installs the PR’s evaluation-tools manifests while keeping executable workflow/adapter/validator code pinned to the trusted workflow commit. Also refines PR-triage messaging to distinguish “no evaluation required” from “evaluation passed”.
Changes:
- Adds an
evaluation_tools_refsignal fromevaluation.ymlto the reusableevaluation-run.ymlso evaluation-tools PRs can installeng/evaluation-toolspackage manifests from the PR commit while keeping trusted tooling pinned. - Improves PR triage messaging by reading the
evaluation-statusdescription (not just state) to differentiate skipped/no-op evaluations from completed ones. - Documents the smoke-evaluation trust boundary and behavior in both the Vally adapter guide and legacy skill-validator docs, plus the triage workflow design doc.
Show a summary per file
| File | Description |
|---|---|
| eng/vally-adapter/InvestigatingResults.md | Documents the evaluation-tools smoke path and trust boundary for Vally harness runs. |
| eng/skill-validator/src/docs/InvestigatingResults.md | Adds the same evaluation-tools smoke note for historical/legacy results documentation. |
| eng/evaluation/test_token_failover.py | Adds regression tests asserting the new evaluation_tools_ref wiring and triage “skipped vs passed” messaging behavior. |
| docs/design/pr-triage-workflows.md | Documents triage behavior and evaluation-tools smoke evaluation semantics. |
| .github/workflows/evaluation.yml | Treats eng/evaluation-tools/** as infra; emits evaluation_tools_ref when applicable and forwards it to the reusable workflow. |
| .github/workflows/evaluation-run.yml | Adds evaluation_tools_ref input, checks out PR manifests for eng/evaluation-tools, and installs manifests from the correct source. |
| .github/scripts/pr-triage-act.sh | Reads evaluation status state+description and adjusts maintainer ping text accordingly. |
Review details
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 7/7 changed files
- Comments generated: 0
- Review effort level: Lite
| cp "$EVALUATION_TOOLS_SOURCE/package.json" \ | ||
| "$EVALUATION_TOOLS_SOURCE/package-lock.json" \ | ||
| "$RUNNER_TEMP/evaluation-tools/" | ||
| npm ci \ |
There was a problem hiding this comment.
Could we avoid installing PR-controlled manifests in a job that later runs the installed Copilot/Vally binaries with a real Copilot PAT? Previously these dependencies came from the trusted workflow commit; this change makes them PR-controlled, so a modified dependency could receive or exfiltrate the selected token when executed later.
Could we keep PAT-backed execution on trusted dependencies, or run this PR dependency smoke test without reusable secrets?
Suggested by Copilot.
|
👋 @AbhitejJohn — this PR has 1 unresolved review thread(s). When you're ready, please address the feedback and push an update; the triage bot will pick up the next state automatically. (Add the |
|
Thanks for catching this. We are closing this change because it would let a PR-selected Vally binary run with a reusable Copilot PAT. Although the path is restricted to maintainer-authorized same-repository evaluations, this expands the executable surface that can directly access the token. We want to keep the evaluation infrastructure secure, so we will not add this automatic PR path. Vally updates can still be validated by pushing a same-repository branch and manually dispatching the (Copilot, closing on Abhitej's behalf.) |
Summary
eng/evaluation-tools/**changes as evaluation infrastructure and select two random eligible plugins.Validation
actionlintpassed.dotnet-dataanddotnet-mauiclaude-sonnet-4.6andgpt-5.6-lunaFollow-up
Vally 0.13.0 declares npm
>=11.11.1, while the current runner provides npm10.9.8. The live run completed despite theEBADENGINEwarning; npm toolchain alignment is intentionally separate from this automation and messaging change.