fix(ci): re-run hygiene check on every bump-version push retry (#678) - #713
Conversation
Extracts the bump-version push retry loop from .github/workflows/ci.yml into scripts/push_version_bump.sh verbatim (inline workflow shell cannot be tested) and adds a shell test suite against a real git repo and a real bare remote. The retry-path and hygiene-failure cases fail: on a retry the loop re-bumps onto whatever landed on main concurrently and pushes that new tree without re-running the hygiene check, so a concurrent violation reaches main unchecked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A rejected push means main advanced. The loop resets to the fresh tip and re-bumps, producing a new commit on a new tree that the pre-push hygiene check never saw - and pushed it straight to main. A concurrent merge carrying a hygiene violation or version drift therefore reached main unchecked, contrary to REPOSITORY_HYGIENE.md §8. Re-run the static hygiene check inside the loop, after the re-bump and before the push, and abort on failure rather than retrying past the violation (which would defeat the gate). Aborting leaves main un-bumped; the next push to main recovers it. The non-retry path is unchanged: one check, before the first attempt. Wires the workflow up to scripts/push_version_bump.sh and runs its tests in the release-scripts job. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe release workflow now delegates version-bump retries to ChangesVersion bump retry hygiene
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change updates the version-bump retry path and adds targeted test coverage; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions
participant Script as push_version_bump.sh
participant Remote as Git remote
participant Bump as bump_version.py
participant Hygiene as check_repo_hygiene.py
CI->>Script: Run with target branch
Script->>Remote: Push version-bump commit
Remote-->>Script: Accept or reject
Script->>Remote: Fetch and reset after rejection
Script->>Bump: Re-bump from latest remote tip
Script->>Hygiene: Check re-bumped tree
Hygiene-->>Script: Pass or fail
Script->>Remote: Push validated retry commit
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| - name: Static hygiene and version check before push | ||
| run: python scripts/check_repo_hygiene.py --mode static | ||
|
|
||
| # The retry loop lives in a script so it can be tested (see | ||
| # scripts/test_push_version_bump.sh, run by the `release-scripts` job). | ||
| # Each retry re-bumps onto the new tip and re-runs the hygiene check | ||
| # above against that new tree before pushing it. | ||
| - name: Push changes | ||
| if: success() | ||
| run: | | ||
| set -eu | ||
| BRANCH="${{ github.ref_name }}" | ||
| # The bump commit must fast-forward main. When another commit lands on | ||
| # main during this run (concurrent merges), the first push is rejected | ||
| # as non-fast-forward. Re-base the bump onto the new tip and retry. | ||
| # Because two concurrent runs can compute the same next version, we | ||
| # reset to the fresh tip and re-run the bump so the version is derived | ||
| # from the true current version instead of replaying a stale bump. | ||
| for attempt in 1 2 3 4 5; do | ||
| if git push origin HEAD:"$BRANCH"; then | ||
| echo "Pushed version bump on attempt $attempt" | ||
| exit 0 | ||
| fi | ||
| echo "Push rejected on attempt $attempt (main advanced); re-bumping from latest tip..." | ||
| git fetch origin "$BRANCH" | ||
| git reset --hard "origin/$BRANCH" | ||
| python scripts/bump_version.py --update-all | ||
| done | ||
| echo "::error::Failed to push version bump after 5 attempts (main kept advancing)." | ||
| exit 1 | ||
| run: ./scripts/push_version_bump.sh "${{ github.ref_name }}" |
There was a problem hiding this comment.
🔍 Sibling versioning.yml push path still has no retry or re-check
.github/workflows/versioning.yml:44 performs a plain git push origin HEAD:${{ github.ref_name }} with a single preceding hygiene check and no retry loop. The extraction here does not change that workflow, so the two version-bump paths now differ in retry semantics. Worth confirming that is deliberate (versioning.yml presumably runs manually/scheduled where a concurrent-merge race is less likely) rather than an oversight of this refactor.
(Refers to lines 704-715)
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Pull request overview
This PR fixes a race in the bump-version CI job where a rejected git push triggers a re-bump on a new tree, but (previously) did not re-run the static hygiene gate before pushing the newly created commit to main. The retry loop is extracted into a script so it can be covered by a shell-level test suite that reproduces real non-fast-forward rejections with a real bare remote.
Changes:
- Extracted the bump-version push retry loop into
scripts/push_version_bump.sh, adding a hygiene re-check on every retry before pushing. - Added
scripts/test_push_version_bump.shto exercise happy path, retry path, hygiene-failure-on-retry, and exhaustion against real local git repos/remotes (stubbing only bump/hygiene commands). - Updated
.github/workflows/ci.ymlto call the new push script and to run the new test suite in the release-scripts job; added a dev diary entry describing the defect and fix.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/push_version_bump.sh | New push retry helper that re-runs static hygiene after each re-bump before attempting the next push. |
| scripts/test_push_version_bump.sh | New integration-style shell test suite using real git repos/remotes to validate retry + hygiene behavior. |
| .github/workflows/ci.yml | Uses the new script in bump-version and runs the new tests in release-scripts. |
| History/dev-diary/2026/2026-08-14-issue-678-bump-version-hygiene-on-retry.md | Documents the race, rationale for extracting to a script, and test evidence. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| echo "Push rejected on attempt $attempt (main advanced); re-bumping from latest tip..." | ||
| git fetch origin "$BRANCH" | ||
| git reset --hard "origin/$BRANCH" | ||
| # shellcheck disable=SC2086 # intentional word splitting: command + args | ||
| $BUMP_CMD |
| "::error::Failed to push version bump after 5 attempts (main kept advancing)." \ | ||
| "it emits the workflow error annotation" | ||
| assert_contains "$(cat "$SB/out")" "Push rejected on attempt 5" "all five attempts were made" | ||
| assert_eq "5" "$(call_count "$SB" bump)" "it re-bumped once per rejection" |
Review findings — all addressed@copilot — confirmed and fixed, both comments. The loop re-bumped after the fifth rejection even though no push follows it, so it built a commit nothing would ever send. Not free either: the production Fixed in the latest commit — the loop breaks before the re-bump once the final attempt is reached. And you correctly predicted the test change: the exhaustion case now asserts 4 re-bumps rather than 5, with a matching assertion that hygiene also ran only 4 times (once per push that actually happens). I replaced the @devin-ai-integration — checked; the difference is real but benign, and deliberate. You are right that
The failure modes differ rather than one being weaker: a concurrent-merge race makes So I have left it alone — bringing it under the shared script would mean giving a manual workflow automatic retry semantics it does not need, which is a scope expansion rather than a fix. Worth a separate issue if the two paths should converge; I did not file one. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
scripts/push_version_bump.sh:58
- The final failure annotation hard-codes
5even thoughATTEMPTSexists; using the variable keeps the message consistent if the attempt count ever changes.
echo "::error::Failed to push version bump after 5 attempts (main kept advancing)."
exit 1
scripts/push_version_bump.sh:28
ATTEMPTSis defined but the loop and final error message still hard-code5, so changing the attempt count in one place would silently desync behavior/message. Consider usingATTEMPTSas the single source of truth for the loop bounds and final error output.
This issue also appears on line 57 of the same file.
ATTEMPTS=5
for attempt in 1 2 3 4 5; do
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
scripts/push_version_bump.sh:57
- The final failure message hard-codes "5 attempts" even though the script already has an
ATTEMPTSvariable; this can become misleading if the retry count is adjusted.
echo "::error::Failed to push version bump after 5 attempts (main kept advancing)."
scripts/push_version_bump.sh:53
- The hygiene-failure annotation says "(attempt $attempt)", but the hygiene check happens between push attempts after a rejection; wording it as "after rejection on attempt" avoids confusion when debugging CI logs.
echo "::error::Static hygiene check failed on the re-bumped tree (attempt $attempt); refusing to push."
scripts/push_version_bump.sh:29
ATTEMPTSis defined but the retry loop is hard-coded to1 2 3 4 5, so changingATTEMPTSlater will silently desync the loop bounds from the break condition.
This issue also appears on line 57 of the same file.
ATTEMPTS=5
for attempt in 1 2 3 4 5; do
if git push origin HEAD:"$BRANCH"; then
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (2)
scripts/push_version_bump.sh:57
- The final workflow annotation hard-codes both the attempt count and
main. Since the script already definesATTEMPTSand resolves$BRANCH, using them keeps the message accurate if either changes.
echo "::error::Failed to push version bump after 5 attempts (main kept advancing)."
scripts/push_version_bump.sh:39
- The status message hard-codes
maineven though the script accepts an arbitrary<branch>. Using the resolved$BRANCHavoids misleading output if the script is reused for another branch.
This issue also appears on line 57 of the same file.
echo "Push rejected on attempt $attempt (main advanced); re-bumping from latest tip..."
Summary
Closes #678.
The
bump-versionjob writes directly tomain, so it runs the static hygienegate immediately before pushing — its own comment says so, citing
REPOSITORY_HYGIENE.md §8. But the gate ran once, as a separate step, while the
push step retried up to five times, and each retry builds a new commit on a
new tree:
That tree — the concurrent merge's content plus a fresh bump — went out
unchecked. A concurrent merge carrying a hygiene violation, or a bump that
drifted against the new tree, reached
mainand was caught only by the nextrun, after it had already landed on the branch this gate protects.
It takes a genuine race to hit, which is exactly why it would have gone
unnoticed until it mattered.
Changes
scripts/push_version_bump.sh(new) — the retry loop, extracted verbatimfrom the workflow, plus a hygiene re-check on the re-bumped tree before each
subsequent push. A violation aborts with a
::error::annotation ratherthan retrying past it: retrying would defeat the gate, and aborting leaves
mainun-bumped, which the next push tomainrecovers. The two shelled-outcommands are overridable (
BUMP_CMD,HYGIENE_CMD) with the productiondefaults baked in.
scripts/test_push_version_bump.sh(new) — 19 assertions across fourcases, in the idiom of
scripts/test_publish_spaces.sh..github/workflows/ci.yml—Push changesnow calls the script;release-scriptsruns the new test suite.The non-retry path is unchanged: still exactly one check, in the workflow step,
before the first attempt. The happy-path test asserts the loop makes zero
hygiene calls when no retry occurs.
Why this left the YAML
Inline workflow shell cannot be tested — it only ever executes in production, on
pushes to
main, on the rare retry path. That is the hazardscripts/test_publish_spaces.shalready exists to close for the releasescripts, and the issue explicitly asked for a test in that spirit.
The test uses a real git repository and a real bare remote, with genuine
non-fast-forward rejections produced by a second clone committing to the remote
mid-flight.
git pushis never stubbed — assertions are made against where theremote tip actually moved. Only
BUMP_CMDandHYGIENE_CMDare stubbed, asrecording fakes logging the HEAD each one saw, because the production versions
mutate the working repo and shell out to Cargo.
Test evidence
Risk class: R3 — release controls (root
testing.md§5). This machinerywrites directly to
main, so negative and failure-path coverage is required,not optional.
Acceptance criteria → tests:
the retry re-bumps and then re-checks hygiene(asserts call order),hygiene inspected the exact commit that was pushed(asserts the recorded HEAD equalsthe SHA that landed on the remote)
the script fails when hygiene fails on a retry,nothing was pushed - the remote tip is exactly where it was,it aborts rather than retrying past the violationno re-bump and no re-check when there is no retry(asserts zero hygiene invocations)the script fails after five rejections,it emits the workflow error annotationRed evidence:
6bab170(test-only, ancestor of the fix). Against afaithful extraction of the buggy loop, 7 of 19 assertions failed. The decisive
one:
The old loop pushed the unchecked re-bumped tree to the remote. Observed, not
argued.
Green evidence:
e60b8c9— 19/19 passing.Unit/component: n/a — no Rust changed.
Integration/contract:
./scripts/test_push_version_bump.sh— 19/19, realgit + real bare remote.
End-to-end: the
bump-versionjob itself only runs on pushes tomain;its retry path is now covered in PR CI by the above, which is the point of the
change.
Security/migration/concurrency/performance/other: the failure path asserts
the absence of a write (remote tip byte-identical), which is the property
that matters for a release control.
Coverage: four cases — happy path, retry, hygiene-failure-on-retry,
exhaustion.
Platforms:
release-scriptsruns onblacksmith-2vcpu-ubuntu-2404-arm. Authored and run locally on Windows (Gitfor Windows bash) as well; the only difference observed was git's CRLF warning
in the temp repos, which does not affect assertions.
Not applicable, with reason:
cargo fmt/clippy/cargo test— noRust files touched. No
Docs/change — this is CI-internal machinery with nouser-visible surface.
Rollback/recovery: revert the commit; the workflow returns to the inline
loop. No external state changes. If the new gate ever fires spuriously, the
branch is simply left un-bumped and the next push to
mainre-bumps it.Residual risk: the first attempt still relies on the separate workflow
step for its hygiene proof, so the script alone is not self-sufficient if it
is ever called from somewhere else.
python scripts/check_repo_hygiene.py --mode staticpasses on this tree.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests