-
Notifications
You must be signed in to change notification settings - Fork 2k
Improve evaluate-pr-tests workflow: slash_command + workflow_dispatch, security hardening #34678
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
Changes from 9 commits
0cc2384
2dfcf71
d79ff20
8234b33
e1dee57
f4c4791
c089534
e6004b7
00af259
17c0f27
f4f02df
27e14d6
69c0a16
f3dc6a9
7bcc980
04bf387
082ed7a
e7fdf22
b40ccb5
d142b43
cfee2bc
e76545e
bf19b8c
0faafa9
f8ab3c5
6154bfd
3b49c7a
72356a7
338d7c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,9 +1,8 @@ | ||||||||||||||
| --- | ||||||||||||||
| description: Evaluates test quality, coverage, and appropriateness on PRs that add or modify tests | ||||||||||||||
| on: | ||||||||||||||
| pull_request: | ||||||||||||||
| types: [opened, synchronize, reopened, ready_for_review] | ||||||||||||||
| forks: ["*"] | ||||||||||||||
| pull_request_target: | ||||||||||||||
| types: [opened, synchronize, reopened] | ||||||||||||||
| paths: | ||||||||||||||
| - 'src/**/tests/**' | ||||||||||||||
| - 'src/**/test/**' | ||||||||||||||
|
|
@@ -15,9 +14,14 @@ on: | |||||||||||||
| description: 'PR number to evaluate' | ||||||||||||||
| required: true | ||||||||||||||
| type: number | ||||||||||||||
| suppress_comment: | ||||||||||||||
| description: 'Dry-run — evaluate but do not post a comment on the PR' | ||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Future-proofing: I suggest renaming to suppress_output in case the output changes (to a PR review for example).
Suggested change
|
||||||||||||||
| required: false | ||||||||||||||
| type: boolean | ||||||||||||||
| default: false | ||||||||||||||
|
|
||||||||||||||
| if: >- | ||||||||||||||
| (github.event_name == 'pull_request' && github.event.pull_request.draft == false) || | ||||||||||||||
| (github.event_name == 'pull_request_target' && github.event.pull_request.draft == false) || | ||||||||||||||
| github.event_name == 'workflow_dispatch' || | ||||||||||||||
| (github.event_name == 'issue_comment' && | ||||||||||||||
| github.event.issue.pull_request && | ||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I always guard against forks as well, preventing the workflow from running on forks except for the Simple case that needs adapting to your scenario: |
||||||||||||||
|
|
@@ -57,7 +61,7 @@ timeout-minutes: 15 | |||||||||||||
|
|
||||||||||||||
| steps: | ||||||||||||||
| - name: Gate — skip if no test source files in diff | ||||||||||||||
| if: github.event_name == 'pull_request' | ||||||||||||||
| if: github.event_name == 'pull_request_target' | ||||||||||||||
| env: | ||||||||||||||
| GH_TOKEN: ${{ github.token }} | ||||||||||||||
| PR_NUMBER: ${{ github.event.pull_request.number }} | ||||||||||||||
|
|
@@ -73,9 +77,10 @@ steps: | |||||||||||||
| echo "✅ Found test files to evaluate:" | ||||||||||||||
| echo "$TEST_FILES" | head -20 | ||||||||||||||
|
|
||||||||||||||
| # Only needed for workflow_dispatch — for pull_request and issue_comment, | ||||||||||||||
| # Only needed for workflow_dispatch — for pull_request_target and issue_comment, | ||||||||||||||
| # the gh-aw platform's checkout_pr_branch.cjs handles PR checkout automatically. | ||||||||||||||
| # workflow_dispatch skips the platform checkout entirely, so we must do it here. | ||||||||||||||
| # The script gates on PR author having write access before checkout. | ||||||||||||||
| - name: Checkout PR and restore agent infrastructure | ||||||||||||||
| if: github.event_name == 'workflow_dispatch' | ||||||||||||||
| env: | ||||||||||||||
|
|
@@ -110,11 +115,27 @@ If the file is **missing**, the fork PR branch is likely not rebased on the late | |||||||||||||
|
|
||||||||||||||
| ❌ **Cannot evaluate**: this PR's branch does not include the evaluate-pr-tests skill (`.github/skills/evaluate-pr-tests/SKILL.md` is missing). | ||||||||||||||
|
|
||||||||||||||
| **Fix**: rebase your fork on the latest `main` branch, or use the **workflow_dispatch** trigger (Actions tab → "Evaluate PR Tests" → "Run workflow" → enter PR number) which handles this automatically. | ||||||||||||||
| **Fix**: rebase your fork on the latest `main` branch and push again. The evaluation will trigger automatically once the skill file is available. | ||||||||||||||
| ``` | ||||||||||||||
|
|
||||||||||||||
| Then stop — do not proceed with the evaluation. | ||||||||||||||
|
|
||||||||||||||
| ## Dry-run mode | ||||||||||||||
|
|
||||||||||||||
| When triggered via `workflow_dispatch` with `suppress_comment` = `${{ inputs.suppress_comment }}`: | ||||||||||||||
| - If **true**, perform the full evaluation but **do not** post a comment on the PR. Write the evaluation to the workflow log only. This is useful for testing the skill without spamming the PR. | ||||||||||||||
| - If **false** (default), post the comment as normal. | ||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These expressions get replaced on their way to the model so this would end up embedding the true or false value into the opening statement. I think you need something more like this (but I recommend validating my understanding here). Note I also reflected my suggested input rename from above.
Suggested change
|
||||||||||||||
|
|
||||||||||||||
| ## When no action is needed | ||||||||||||||
|
|
||||||||||||||
| If there is nothing to evaluate (PR has no test files, PR is a docs-only change, etc.), you **must** call the `noop` tool with a message explaining why: | ||||||||||||||
|
|
||||||||||||||
| ```json | ||||||||||||||
| {"noop": {"message": "No action needed: [brief explanation, e.g. 'PR contains no test files']"}} | ||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You might want to configure to not generate a no-op run report issue (within the frontmatter). https://github.github.com/gh-aw/patterns/monitoring/#no-op-run-reports |
||||||||||||||
| ``` | ||||||||||||||
|
|
||||||||||||||
| Do not post a comment and do not silently exit — always use `noop` so the workflow run shows a clear reason. | ||||||||||||||
|
|
||||||||||||||
| ## Running the skill | ||||||||||||||
|
|
||||||||||||||
| 1. Use `gh pr view <number>` to fetch PR metadata (title, body, labels, base branch). If `gh` CLI is unavailable, use the GitHub MCP tools instead. | ||||||||||||||
|
|
@@ -124,7 +145,9 @@ Then stop — do not proceed with the evaluation. | |||||||||||||
|
|
||||||||||||||
| ## Posting Results | ||||||||||||||
|
|
||||||||||||||
| Call `add_comment` with `item_number` set to the PR number. Wrap the report in a collapsible `<details>` block: | ||||||||||||||
| If dry-run mode is active (`suppress_comment` is true), log the evaluation report to stdout and stop — do **not** call `add_comment`. | ||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||
|
|
||||||||||||||
| Otherwise, call `add_comment` with `item_number` set to the PR number. Wrap the report in a collapsible `<details>` block: | ||||||||||||||
|
|
||||||||||||||
| ```markdown | ||||||||||||||
| ## 🧪 PR Test Evaluation | ||||||||||||||
|
|
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think this will still lead to the 'Approve and run workflows' button showing up for PRs from untrusted forks. We need to solidify the guidance we give for when to hit that button. I really wish that button navigated into a list of workflows needing approval for the PR with boxes to select which to approve.