fix(bump-callers): commit each repo's files atomically via the Git Data API (BE-3902)#46
Conversation
…-3882) The dispatcher embedded the new short-SHA in the branch name (ci/bump-<tag>-<short>), so every bump minted a unique branch and thus a brand-new PR in each caller repo, leaving the prior bump PRs open. The stack made it unclear which pin was current. Make the branch stable per (repo, TAG) — ci/bump-<tag> — and update it in place: each run rebuilds the branch from the caller's current default-branch tip (a clean single-commit "bump to @short" diff), then, if a bump PR is already open for that branch, refreshes its title/body to the new SHA instead of opening another; a fresh PR is opened only when none is open (first bump, or the prior one merged/closed since the last run). Result: at most one open bump PR per (repo, workflow) at any time. Applies to both the cursor-review and agents-md fleets via the shared script. Adds test coverage for the update-in-place path (open PR is edited, not re-opened) and the create path (no edit when no PR is open).
…or (BE-3882) Harden the stable-branch bump PR reuse from the review panel: - Existing-PR lookup: use `.[0].number // empty` (a bare `.[0].number` prints the literal `null` on an empty list, so the first bump for a repo — and every bump after the prior PR merged/closed — ran `gh pr edit null` and failed the caller). Also exclude cross-repository PRs (`isCrossRepository == false`): `--head` matches by branch name across forks, so with the now-predictable branch name an attacker could pre-open a fork PR the bot would stamp instead of bumping the real caller. - Create path: `return 1` on a genuine `gh pr create` failure. The update-in-place path already handles an open PR, so reaching create means none exists and a failure is a real miss — record it in FAILED instead of reporting success. - Serialize each fleet with a `concurrency:` group (cancel-in-progress: false) so overlapping runs can't race the shared stable branch; the newest pending run wins so the latest SHA is committed. - Tests: the gh stub now faithfully models `gh pr list --json --jq` by running the real jq over the JSON gh would return, so the no-open-PR case reproduces production instead of masking it; add a fork-PR decoy regression case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…(BE-3896) Group CALLERS entries by repo before the bump loop and reset the stable branch ONCE per repo, then commit each of that repo's files onto it with successive PUTs (each carrying that file's own blob SHA). The old per-entry loop reset the branch before every file, so a second same-repo entry's reset discarded the first entry's commit and the PR shipped only the last file while every entry reported success — a silent partial bump. Only affects repos that appear more than once in a caller list; single-file repos are unchanged. The test stub now models the one bump branch's committed file set (a ref reset truncates it, a PUT appends), and a new same-repo two-file case asserts BOTH files land on the branch — it fails against the pre-fix script.
…bumps (BE-3896) Address cursor-review panel findings on the multi-file-per-repo bumper: - Distinguish a genuine 404 (per-file skip) from any other fetch error (auth/rate-limit/5xx/network); a transient error now fails the whole repo instead of silently shipping a PR that omits the un-fetched file. - Resolve the default-branch tip (MAIN_SHA) up front and pin every Pass-1 blob fetch to that immutable SHA, closing a TOCTOU race with a moving default branch. - De-duplicate staged files by path so a repo listed twice for the same file commits it once (a second PUT with a now-stale blob sha would 409 the repo). - Skip an already-pinned file by comparing rewritten-vs-original content instead of grepping for NEW_SHA anywhere, which also repairs a half-bumped file. - Anchor the 40-hex SHA rewrite to the github-workflows / workflows_ref pin contexts so a full-SHA pin of another action (actions/checkout@<sha>) is not clobbered. - Collect a caller entry's label only once its file is confirmed staged, so a skipped entry's label never lands on the real bump PR. - Exact-match label de-dup (GitHub label names may contain the `|` sentinel). Adds functional tests for each: 404-skip vs transient-fail, same-file de-dup, non-github-workflows pin preservation, half-bump repair, and already-pinned skip. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mattmillerai
added
agent-coded
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 4 finding(s).
| Severity | Count |
|---|---|
| 🔴 Critical | 1 |
| 🟢 Low | 2 |
| ⚪ Nit | 1 |
Panel: 6/8 reviewers contributed findings.
Reviewers that did not contribute: kimi-k2.5:adversarial (empty), kimi-k2.5:edge-case (empty)
…3902) The Git Data Create-a-tree call was passing $MAIN_SHA — the tip *commit* SHA — as base_tree, but that API requires a *tree* SHA. A commit SHA either 422s the call (every bump fails) or yields a tree carrying ONLY the bumped files, so merging the PR would delete every other path in the caller repo. Resolve the tip commit's tree via GET git/commits and pass that as base_tree; $MAIN_SHA stays correct as the new commit's parent. Also send the blob create body on stdin (--input -) instead of --field on argv, matching the adjacent tree/commit calls and keeping content off cmdline/ARG_MAX. Test stub now models GET git/commits and asserts base_tree is the resolved tree SHA, not the commit SHA, so the offline suite catches this class of bug. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…it-data-commit # Conflicts: # .github/bump-callers/bump-callers.sh # .github/bump-callers/tests/test_bump_callers.sh
|
Self-review verdict — HEAD Reviewed the full diff fresh: the atomic-commit implementation (blob → tree → commit → single ref move) genuinely delivers the all-or-nothing guarantee — every Git Data API call before the ref move is inert/unreferenced on failure, so a mid-sequence error leaves the branch untouched. Merge conflict against Babysitter is done; your turn. |
…it-data-commit # Conflicts: # .github/bump-callers/bump-callers.sh
|
Merge-conflict resolution — HEAD
Root cause: BE-1814's wire-bot feature was built on top of the old per-file Contents-API loop ( Verified post-merge:
|
1 participant
ELI-5
When we bump a caller repo, it might pin our reusable workflow from more than one file. The current code commits those files one at a time (a separate API call per file). If the 3rd call fails after the 1st and 2nd already landed, the branch is left with a partial bump — and if an open PR points at that branch, someone could merge a half-done bump. This PR makes all of a repo's files land in one commit or none: we build the whole commit off to the side (blobs → tree → commit) and only then move the branch to it in a single step. A failure anywhere leaves the branch untouched.
What changed
bump_repo()Pass 2 no longer loopsPUT repos/<repo>/contents/<file>per file. Instead, for each staged file it:POST git/blobs— create a blob for the new content.POST git/trees— build one tree withbase_tree=MAIN_SHA(the resolved default-branch tip) carrying all staged blobs, so every other path in the repo is preserved and only the bumped files appear.POST git/commits— create one commit with that tree, parented onMAIN_SHA(clean "bump to @short" diff vs the default branch).POST/PATCH git/refs/heads/<branch>— point the stable bump branch at the finished commit (create if new, else force). The ref moves in one step from its old state to the complete commit; it never transiently sits at an empty/partial tree.Because the commit is fully built before the ref moves, a failure at any step leaves only dangling (GC-able) blobs/tree/commit and the branch untouched — all-or-nothing. This closes the residual BE-3896 partial-bump window: a mid-sequence failure that previously left earlier files committed on the branch (and, if an open PR pointed at it, a partial-yet-mergeable bump).
Test changes
tests/test_bump_callers.sh— theghstub now models the Git Data API calls instead of the per-file Contents PUT:POST git/blobs→ captures each blob's decoded content (put.$n.txt/put.last.txt;count= number of blobs) — same content assertions as before.POST git/trees→ records the tree's path list asbranch_files(the atomic branch's final file set).POST git/commits→ returns a commit sha.POST/PATCH git/refs→ points the branch (no-op for file-set modeling).The BE-3896 monorepo assertion is preserved: both files still land on the one branch (now via one tree carrying both blobs). All 69 assertions pass;
shellcheck -xis clean.Verification
shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/tests/test_bump_callers.sh→ clean.bash .github/bump-callers/tests/test_bump_callers.sh→ 69 passed, 0 failed.Judgment calls
100644. Git tree entries need an explicit file mode; the bumped files are.github/workflows/*.ymlcallers, which are never executable, so100644is correct. (The old Contents PUT preserved mode implicitly; preserving it here would require an extra read of each existing tree entry's mode for no real-world benefit.)bump_repo()Pass 2's per-file PUT loop) only exists on PR fix(bump-callers): commit all of a repo's files onto one bump branch (BE-3896) #45's branch, not onmain.