Conversation
|
@charlesroddie Approve please |
charlesroddie
requested review from
Copilot
and removed request for
charlesroddie
There was a problem hiding this comment.
Pull request overview
This PR fixes several issues in GitHub Actions workflows related to permissions, git operations, and artifact uploads.
Changes:
- Added explicit base branch target for release PRs to ensure they target the default branch
- Removed obsolete CSharpMath.Xaml.Tests.NuGet artifact upload step from nightly builds
- Changed Label workflow from
pull_requesttopull_request_targettrigger to enable write permissions for automatic labeling
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| .github/workflows/Release.yml | Added base branch parameter for PR creation and fixed comment capitalization |
| .github/workflows/Nightly.yml | Removed obsolete CSharpMath.Xaml.Tests.NuGet artifact upload step |
| .github/workflows/Label.yml | Changed trigger to pull_request_target, added explicit permissions, updated checkout configuration, removed git fetch commands, and improved labeling logic |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| run: | | ||
| git fetch origin ${{ github.base_ref }} | ||
| $changes = git diff --numstat --shortstat origin/${{ github.base_ref }}...HEAD -- '**/PublicApi.Shipped.txt' | ||
| $changes = git diff --numstat --shortstat origin/${{ github.base_ref }}...HEAD -- '**/PublicApi.Shipped.txt' # Note that ** must expand to at least one folder here, this doesn't check root |
There was a problem hiding this comment.
The git diff command references origin/${{ github.base_ref }} but there's no git fetch operation to ensure the base branch is available locally. While fetch-depth: 0 fetches all history, when checking out a specific PR head SHA with pull_request_target, the base branch may not be available as a remote tracking branch. This could cause the git diff to fail with an error like "unknown revision or path not in the working tree". Consider adding git fetch origin ${{ github.base_ref }} before this line, or use git fetch origin ${{ github.event.pull_request.base.sha }} to explicitly fetch the base commit.
| Write-Output "This is a ship-publicapi PR, labeling as Type/Housekeeping" | ||
| } else { | ||
| # For regular PRs, check for API changes | ||
| $changes = git diff origin/${{ github.base_ref }}...HEAD -- '**/PublicApi.Unshipped.txt' |
There was a problem hiding this comment.
The git diff command references origin/${{ github.base_ref }} but there's no git fetch operation to ensure the base branch is available locally. While fetch-depth: 0 fetches all history, when checking out a specific PR head SHA with pull_request_target, the base branch may not be available as a remote tracking branch. This could cause the git diff to fail. Consider adding git fetch origin ${{ github.base_ref }} before this line, or use git fetch origin ${{ github.event.pull_request.base.sha }} to explicitly fetch the base commit.
| on: pull_request_target # Run on the merge target branch instead of the merge commit to grant pull-requests:write permission | ||
| jobs: | ||
| Label: | ||
| runs-on: windows-latest | ||
| permissions: | ||
| pull-requests: write # Allow adding a label to this pull request | ||
| steps: | ||
| - uses: actions/checkout@v6 | ||
| with: | ||
| submodules: recursive | ||
| fetch-depth: 0 # Fetch all commit history and tags, instead of the default fetch of only one commit | ||
| ref: ${{ github.event.pull_request.head.sha }} # Checkout the PR branch |
There was a problem hiding this comment.
Using pull_request_target with ref: ${{ github.event.pull_request.head.sha }} can introduce security risks if the checked-out code is executed in the workflow. This workflow executes PowerShell scripts that process the PR's code (git diff output). While the current implementation appears safe since it only reads and analyzes the diff output, be aware that pull_request_target runs with write permissions in the context of the base repository, not the fork. Any future modifications that execute untrusted code from the PR should be carefully reviewed for security implications.
| name: Label breaking changes for semantic versioning | ||
|
|
||
| on: [pull_request] | ||
| on: pull_request_target # Run on the merge target branch instead of the merge commit to grant pull-requests:write permission |
There was a problem hiding this comment.
The comment states that the workflow runs "on the merge target branch instead of the merge commit", but the checkout configuration uses ref: ${{ github.event.pull_request.head.sha }} which checks out the PR branch (head), not the target branch (base). The comment may be misleading. The pull_request_target event does run in the context of the base repository (for permissions), but the code being checked out is from the PR head.
| on: pull_request_target # Run on the merge target branch instead of the merge commit to grant pull-requests:write permission | |
| on: pull_request_target # Use pull_request_target to run with base repository permissions (while checking out the PR head) so we can add labels |
1 participant
No description provided.