[build] simplify commit-changes workflow#17503
Open
titusfortner wants to merge 5 commits into
Open
Conversation
Contributor
Review Summary by QodoSimplify commit-changes workflow with unconditional push logic
WalkthroughsDescription• Simplify commit-changes workflow logic and error handling • Remove unnecessary conditional checks for artifact download • Always push changes when requested, regardless of conditions • Streamline git operations by removing redundant error exits Diagramflowchart LR
A["Download artifact"] --> B["Configure git user"]
B --> C["Check if patch exists"]
C --> D["Apply patch and commit"]
D --> E["Determine push target"]
E --> F["Push to branch or origin"]
F --> G["Output committed status"]
File Changes1. .github/workflows/commit-changes.yml
|
Contributor
Code Review by Qodo
1. PUSH_BRANCH not validated
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR simplifies the reusable commit-changes GitHub Actions workflow by reducing conditional logic around applying a downloaded patch, committing it, and pushing updates (especially when push-branch is provided).
Changes:
- Simplifies the “apply patch + commit” flow to a single
-s changes.patchcheck and removes explicit error messaging. - Changes push behavior to always force-push when
push-branchis set (and only push the current branch when a commit was created). - Removes the explicit guard that exited early when the artifact download step didn’t succeed.
Comments suppressed due to low confidence (1)
.github/workflows/commit-changes.yml:66
PUSH_BRANCHtriggers an unconditionalgit push --forceeven when no patch was downloaded/applied and no commit was created. If the artifact download fails (it’scontinue-on-error) orchanges.patchis empty/missing, this can still force-update the target branch to the currently checked-out ref, potentially overwriting an existing remote branch/history. Consider only force-pushing when a commit was created, or use--force-with-leaseplus a safety check that the checked-out ref/branch matches the intended push target.
if [ -n "$PUSH_BRANCH" ]; then
git push --force origin HEAD:"$PUSH_BRANCH"
elif [ "$committed" = true ]; then
Comment on lines
59
to
63
| if [ -s changes.patch ]; then | ||
| git apply --index changes.patch | ||
| git commit -m "$COMMIT_MESSAGE" | ||
| committed=true | ||
| fi |
Contributor
|
Persistent review updated to latest commit d08883b |
Comment on lines
41
to
45
| - name: Download patch | ||
| id: download | ||
| uses: actions/download-artifact@v8 | ||
| with: | ||
| name: ${{ inputs.artifact-name }} | ||
| continue-on-error: true |
Contributor
|
Persistent review updated to latest commit 8c353bf |
Contributor
|
Persistent review updated to latest commit e2be623 |
| DOWNLOAD_OUTCOME: ${{ steps.download.outcome }} | ||
| COMMIT_MESSAGE: ${{ inputs.commit-message }} | ||
| PUSH_BRANCH: ${{ inputs.push-branch }} | ||
| PUSH_BRANCH: ${{ inputs.push-branch || inputs.ref || github.ref_name }} |
Contributor
There was a problem hiding this comment.
1. push_branch not validated 📘 Rule violation ⛨ Security
PUSH_BRANCH is derived from external workflow inputs (push-branch and now also ref) and used to construct a force-pushed git push destination refspec, but it is not validated/sanitized as a safe branch name. Malformed or non-branch values (e.g., containing : or being a tag/PR ref/commit SHA) can cause the push to fail or target an unintended ref namespace/ref, which is especially risky with force-push behavior.
Agent Prompt
## Issue description
`PUSH_BRANCH` is built from external inputs and used as the destination ref in a force-push `git push` refspec, but it is not validated/sanitized to ensure it is a safe, expected branch name. The current fallback to `inputs.ref` (a generic “Git ref to checkout”) can propagate non-branch values (tag/PR ref/commit SHA) or refspec characters (e.g., `:`) into the push destination, risking failures or unintended pushes.
## Issue Context
PR Compliance ID 10 expects validation of external/config-derived inputs early to avoid unchecked assumptions. This workflow force-pushes using `PUSH_BRANCH` as the destination, so the value must be branch-like and should not be coupled to a generic checkout ref unless it is strictly validated (e.g., only accept `refs/heads/*` or plain branch names) and rejected otherwise with a deterministic, actionable error.
## Fix Focus Areas
- .github/workflows/commit-changes.yml[59-59]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Current code is doing a lot of work to manage a conditional that does not matter.
Just always push when requested.
This change is needed for a renovate fix I'm working on.
🤖 AI assistance
🔄 Types of changes