Require explicit /evaluate for all PRs, add message to that effect - #363
Require explicit /evaluate for all PRs, add message to that effect#363danmoseley wants to merge 3 commits into
Conversation
Remove the pull_request trigger from evaluation.yml so PRs no longer
auto-evaluate on every push. All PRs (fork and upstream) now require a
maintainer to post /evaluate to trigger evaluation.
Changes:
- evaluation.yml: schedule-only (remove pull_request trigger, remove
evaluation-status check job, simplify discover to schedule path only)
- evaluation-fork-pr.yml: rename to handle all PRs, not just forks
- Remove fork-only gate check
- Post pending commit status for all PRs via pull_request_target
- Post one-time /evaluate reminder comment when PR is opened with
skill/test changes
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates the evaluation GitHub Actions workflows to stop auto-evaluating on PR pushes and instead require a maintainer-triggered /evaluate, while keeping scheduled evaluations for the canonical repo.
Changes:
- Removed the
pull_requesttrigger fromevaluation.yml, leaving scheduled-only evaluation runs. - Shifted PR status posting +
/evaluategating toevaluation-fork-pr.ymlfor all PRs (not just forks). - Added an “open-time” reminder comment for PRs that change skills/tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| .github/workflows/evaluation.yml | Makes evaluation scheduled-only; simplifies discovery and reusable workflow inputs. |
| .github/workflows/evaluation-fork-pr.yml | Expands PR handling to all PRs: posts evaluation-status, gates evaluation behind /evaluate, and comments a one-time reminder on open for skill/test changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Shifts evaluation to an explicit, maintainer-triggered /evaluate flow for pull requests, while keeping scheduled evaluations for the canonical repo to reduce automatic token usage on PR pushes.
Changes:
- Removed
pull_requesttriggering fromevaluation.yml, leaving it schedule-only with simplified discovery and fixed parallelization settings. - Expanded
evaluation-fork-pr.ymlto handle all PRs viapull_request_target+issue_comment(/evaluate) gating. - Added a one-time reminder comment on PR open when skill/test changes are detected.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| .github/workflows/evaluation.yml | Removes PR execution path; keeps scheduled evaluation in dotnet/skills and simplifies discovery/matrix inputs. |
| .github/workflows/evaluation-fork-pr.yml | Generalizes the PR evaluation gating/status workflow and adds reminder comment behavior. |
Comments suppressed due to low confidence (1)
.github/workflows/evaluation-fork-pr.yml:54
- This step uses
actions/checkout@v6(mutable tag) and also omitspersist-credentials: false, while most workflows in this repo pin actions to a commit SHA and disable persisted git credentials. Please pinactions/checkoutto a specific commit SHA (with a version comment) and consider settingpersist-credentials: falsefor consistency and to reduce token exposure risk.
- name: Checkout base branch
uses: actions/checkout@v6
with:
ref: ${{ github.event.pull_request.base.sha }}
fetch-depth: 0
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Remove pull-requests:write from schedule-only evaluation.yml (unused) - Use stable concurrency group key for schedule runs - Add explicit pull_request_target types (opened/synchronize/reopened) - Pin checkout action to SHA with persist-credentials: false - Expand detection to include infra paths (evaluation workflows, skill-validator) - Update PR description to accurately describe pending vs success behavior Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates the evaluation automation so PRs no longer auto-run evaluation on every push; instead maintainers must explicitly trigger it via /evaluate, while scheduled evaluations continue to run periodically on the canonical repo.
Changes:
- Remove
pull_requesttrigger and PR-specific logic/check job fromevaluation.yml, making it schedule-only. - Expand
evaluation-fork-pr.ymlto handle status posting and/evaluate-triggered runs for all PRs (fork and upstream). - Add an “opened PR” reminder comment when evaluatable changes are detected.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| .github/workflows/evaluation.yml | Makes evaluation workflow schedule-only and simplifies discovery/evaluation inputs accordingly. |
| .github/workflows/evaluation-fork-pr.yml | Uses pull_request_target to post evaluation-status for all PRs and gates actual evaluation behind /evaluate, with a one-time reminder comment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When /evaluate is triggered on a PR with infrastructure changes (evaluation workflows or eng/skill-validator/), evaluate all plugins instead of only changed skills. This aligns the /evaluate discover behavior with the initial status detection that marks infra PRs as needing evaluation. Also update the reminder comment text to mention infrastructure changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ViktorHofer
left a comment
There was a problem hiding this comment.
Pull_request_target or should only be used in forks. Otherwise we can never make changes to workflows or the skill-validator source code and respecting that in the run.
We also need to keep the evaluation-status job as that one is used in the rule settings and se as required for a PR.
I can help addressing the above points on Monday.
|
I don't fully understand that so happy for you to fix on Monday as offered! |
|
Superseded by bd965cb |
Require an explicit
/evaluatecommand for all PRs instead of auto-evaluating on every push. This significantly reduces token consumption by only running evaluation when a maintainer explicitly requests it. Note, on opening a PR, it will post a message educating/reminding about/evaluate.Changes
evaluation.yml — now schedule-only:
pull_requesttrigger entirelyevaluation-statuscheck job (no longer needed for PRs)evaluation-fork-pr.yml — now handles all PRs (not just forks):
pull_request_targetposts status for all PRs/evaluateworks on upstream PRs tooHow it works
pull_request_targetruns discovery from the base branch and posts anevaluation-statuscommit status:/evaluate/evaluateto trigger evaluationImportant note — CI will still be red if evaluation didn't run on the most recent commit. That does not change, so
/evaluatemust be requested on the final commit (at least). It will go green if evaluate ran and completed.