Skip to content

fix: update-bindings.sh — converge pre-commit before commit, don't push aborted commits#94

Open
cloudsmith-iduffy wants to merge 1 commit into
masterfrom
fix-update-bindings-precommit-convergence
Open

fix: update-bindings.sh — converge pre-commit before commit, don't push aborted commits#94
cloudsmith-iduffy wants to merge 1 commit into
masterfrom
fix-update-bindings-precommit-convergence

Conversation

@cloudsmith-iduffy

Copy link
Copy Markdown
Contributor

Problem

scripts/update-bindings.sh runs git commit with the ruff pre-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 to git push regardless — leaving the remote branch pointing at the same commit as master with 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() — runs pre-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.
  • Guard git commit — if the commit still fails for any reason, log and exit 1 instead of falling through to git 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 — passes
  • shellcheck — no new findings (pre-existing SC2034/SC2155 warnings untouched)
  • Convergence loop unit-tested in isolation with a stubbed pre-commit:
    • hook that auto-fixes twice then passes → converges, exit 0
    • hook that never passes → aborts after 5 attempts with exit 1 (no fall-through to commit/push)

🤖 Generated with Claude Code

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>
Copilot AI review requested due to automatic review settings June 9, 2026 09:52

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

  • Addresses a failure mode in scripts/update-bindings.sh where pre-commit (notably ruff) can auto-fix files during git commit, aborting the commit while the script still proceeds to git 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 commit fails.

Changes:

  • Add run_precommit_to_convergence() to run pre-commit run --all-files with 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 commit fails, ensuring no push occurs without a successful commit.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants