fix: update-bindings.sh — converge pre-commit before commit, don't push aborted commits#94
Open
cloudsmith-iduffy wants to merge 1 commit into
Open
Conversation
update-bindings.sh ran `git commit` with the ruff pre-commit hook active. When ruff auto-fixed files it modified the tree and aborted the commit, but the script pushed the branch anyway — leaving the remote branch pointing at the same commit as master with the regenerated changes uncommitted. - Add run_precommit_to_convergence(): run pre-commit, re-staging and retrying until it passes (max 5 attempts) so auto-fixes are captured before commit. - Guard `git commit`: if it fails (e.g. a hook aborts it), log and exit instead of falling through to `git push`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
- Addresses a failure mode in
scripts/update-bindings.shwherepre-commit(notablyruff) can auto-fix files duringgit commit, aborting the commit while the script still proceeds togit push, resulting in an “empty” remote branch. - Introduces an explicit “run pre-commit until stable” step before committing and adds a hard guard to prevent pushing when
git commitfails.
Changes:
- Add
run_precommit_to_convergence()to runpre-commit run --all-fileswith bounded retries, re-staging changes between runs. - Call the convergence step before committing, so auto-fixes happen before
git commit. - Abort the script if
git commitfails, ensuring no push occurs without a successful commit.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Problem
scripts/update-bindings.shrunsgit commitwith theruffpre-commit hook active. When ruff auto-fixes files (e.g. the W605 invalid-escape-sequence warnings that always appear in freshly-generated Python bindings), it modifies the working tree and aborts the commit. The script doesn't check the commit's exit status, so it proceeds togit pushregardless — leaving the remote branch pointing at the same commit asmasterwith all the regenerated changes sitting uncommitted in the working tree.The result is an "empty" update branch that requires a manual rescue (re-stage, re-run pre-commit, fix, commit, push) before a PR can be opened. This was hit again on the v2.0.28 update (#93).
Fix
run_precommit_to_convergence()— runspre-commit run --all-files, re-staging and retrying (up to 5 attempts) until the hooks pass with no further modifications. This captures ruff's auto-fixes before the commit, so the commit hook no longer aborts.git commit— if the commit still fails for any reason, log andexit 1instead of falling through togit push. The branch is never pushed without its commit.No behavioural change on the happy path (clean tree → hooks pass first try → commit → push).
Verification
bash -n scripts/update-bindings.sh— passesshellcheck— no new findings (pre-existing SC2034/SC2155 warnings untouched)pre-commit:🤖 Generated with Claude Code