Add workflow_dispatch trigger and permissions#25
Conversation
📝 WalkthroughWalkthroughModifies the GitHub Actions workflow Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/build_dist_on_merge.yml (1)
8-10: Consider scopingpermissionsto the job levelThis is functionally equivalent since there's only one job, but job-level permissions are preferred for least-privilege as the workflow grows.
♻️ Proposed refactor
-permissions: - contents: write - jobs: build: runs-on: ubuntu-latest + permissions: + contents: write🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build_dist_on_merge.yml around lines 8 - 10, Remove the top-level permissions block and instead add a job-level permissions block so the credential scope is least-privilege as the workflow grows: delete the existing top-level "permissions: contents: write" and add a "permissions" map under the specific job (jobs.<your_job_id>.permissions) with "contents: write" only for that job; leave the workflow-level permissions unset or more restrictive to avoid granting write access globally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build_dist_on_merge.yml:
- Line 6: The workflow currently exposes workflow_dispatch with no branch
filtering (symbol: workflow_dispatch) which allows manual runs to amend &
force-push history; fix by either adding a branches filter under
workflow_dispatch restricting to non-default branches, or add a top-level
concurrency: group to serialize runs (symbol: concurrency), or gate the
amend/force-push steps with an if conditional that only runs them on
pull_request (example: if: github.event_name == 'pull_request') — apply one or a
combination and ensure the amend/force-push step(s) referenced in the diff
(lines ~39-43) are updated to use the chosen guard.
---
Nitpick comments:
In @.github/workflows/build_dist_on_merge.yml:
- Around line 8-10: Remove the top-level permissions block and instead add a
job-level permissions block so the credential scope is least-privilege as the
workflow grows: delete the existing top-level "permissions: contents: write" and
add a "permissions" map under the specific job (jobs.<your_job_id>.permissions)
with "contents: write" only for that job; leave the workflow-level permissions
unset or more restrictive to avoid granting write access globally.
|
@coderabbitai take another look please |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/build_dist_on_merge.yml (1)
6-8: Past concern regardingworkflow_dispatchon protected branches is now addressed.The
ifcondition on lines 22–23 correctly blocksworkflow_dispatchruns onmain/master, and the concurrency group on lines 13–15 serializes runs per PR/ref. Both mitigations requested in the earlier review are now in place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build_dist_on_merge.yml around lines 6 - 8, The previous concern about allowing manual runs on protected branches has been addressed: verify the workflow now uses workflow_dispatch guarded by the existing if condition that checks the ref (the workflow_dispatch trigger) and that the concurrency group (concurrency: group) is present to serialize runs per PR/ref; no code change required if those checks exist — otherwise add an if guard preventing workflow_dispatch on main/master and ensure concurrency.group is set to serialize runs.
🧹 Nitpick comments (1)
.github/workflows/build_dist_on_merge.yml (1)
74-76:git fetchbefore--force-with-leaseweakens the protection.
--force-with-leaseguards against overwriting unexpected remote changes by comparing the remote ref against the local tracking ref. Fetching on line 75 updates the tracking ref to the current remote state, so any push that landed between the original checkout and this fetch is silently accepted as the new baseline — and then overwritten by the force-push.The concurrency group with
cancel-in-progressmakes this unlikely in practice, but the fetch still structurally defeats the lease check. Consider capturing the expected SHA at checkout and using the explicit form:🛡️ Suggested fix: pin the expected SHA for force-with-lease
+ - name: Record original HEAD SHA + id: original + run: echo "sha=$(git rev-parse HEAD)" >> "$GITHUB_OUTPUT" + - name: Commit dist changes (amend) shell: bash env: TARGET_BRANCH: ${{ github.head_ref || github.ref_name }} + ORIGINAL_SHA: ${{ steps.original.outputs.sha }} run: | set -euo pipefail # ... (existing checks and amend) ... echo "Pushing amended commit to $TARGET_BRANCH" - git fetch origin "$TARGET_BRANCH" --depth=2 - git push --force-with-lease origin "HEAD:$TARGET_BRANCH" + git push --force-with-lease="refs/heads/$TARGET_BRANCH:$ORIGINAL_SHA" origin "HEAD:$TARGET_BRANCH"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build_dist_on_merge.yml around lines 74 - 76, The fetch before the force-with-lease undermines the lease check; instead capture the expected remote SHA into a variable right after checkout (e.g., read the current remote ref for TARGET_BRANCH into EXPECTED_SHA) and then call git push --force-with-lease using the explicit lease form that includes the captured EXPECTED_SHA for refs/heads/TARGET_BRANCH, removing or moving the git fetch so the tracking ref isn’t updated before the lease check; reference TARGET_BRANCH, the git fetch step, and the git push --force-with-lease invocation when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/build_dist_on_merge.yml:
- Around line 6-8: The previous concern about allowing manual runs on protected
branches has been addressed: verify the workflow now uses workflow_dispatch
guarded by the existing if condition that checks the ref (the workflow_dispatch
trigger) and that the concurrency group (concurrency: group) is present to
serialize runs per PR/ref; no code change required if those checks exist —
otherwise add an if guard preventing workflow_dispatch on main/master and ensure
concurrency.group is set to serialize runs.
---
Nitpick comments:
In @.github/workflows/build_dist_on_merge.yml:
- Around line 74-76: The fetch before the force-with-lease undermines the lease
check; instead capture the expected remote SHA into a variable right after
checkout (e.g., read the current remote ref for TARGET_BRANCH into EXPECTED_SHA)
and then call git push --force-with-lease using the explicit lease form that
includes the captured EXPECTED_SHA for refs/heads/TARGET_BRANCH, removing or
moving the git fetch so the tracking ref isn’t updated before the lease check;
reference TARGET_BRANCH, the git fetch step, and the git push --force-with-lease
invocation when making the change.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
.github/workflows/build_dist_on_merge.yml
1 participant
Purpose
Updating workflow trigger to include manual dispatch
Added explicit permissions to workflow
Types of changes
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Reviewer, please remember our guidelines:
Summary by CodeRabbit