Repository navigation
🐛 crux-sync: tag runs tag the matching synced commit, never move main - #14226
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of No conflicting open PRs or same-file overlaps were found. |
|
@clubanderson is auto-promoted for contributor-lane merging on this PR: the last 5 merged PRs by this author in this repository have no revert or sentinel-alert evidence. CI, reviewer verdict, diff fences, soak, holds and branch protection still apply. |
|
@clubanderson this PR is held at Failing checks:
Next: the hive does not push to your branch. Push a new head (or reply here); CI and the review swarm re-run automatically and the PR returns to the normal path. |
There was a problem hiding this comment.
One real defect: the "find the already-synced commit" search can only ever see one commit, so the release path fails in exactly the case it was written for.
.github/workflows/crux-sync.yml:106— thehivecommons/cruxcheckout intocrux-publishisfetch-depth: 1, and nothing insrc/scripts/crux-publish.shdeepens it (its only git calls arerev-parseagainst the hive checkout,:53-54)..github/workflows/crux-sync.yml:149-150—for c in $(git rev-list -n 200 HEAD)therefore iterates exactly one SHA (HEAD ofmain), and the match degenerates to "is the tagged hive commit's tree identical to currentmain".- The comment at
:144-146names the motivating case: the tag points at a hive commit older than what the push trigger has already synced. In that casemainHEAD carries the newer tree, the single comparison fails, and the step exits 1 at:152-154with "no commit on main matches … sync it first" — but syncing cannot help, since the matching commit is already onmain, just behind HEAD. Severity: high; consequence: every release tag cut on a commit that isn't the latestv5sync fails and cannot be recovered via the documented remedy.
Fix is one line: fetch-depth: 0 on the crux-publish checkout (or ≥200 to match the rev-list bound). The crux repo is small (one synced copy of src/crux), so a full clone is cheap.
Everything else reads correctly to me: the Push step (:213-224) only moves main when changed=true, the ls-remote --exit-code --tags guard (:227-230) prevents a silent re-tag, $TARGET is a local commit so git tag -a … "$TARGET" resolves, and git diff --cached --quiet "$c" -- . ':!.hive-sync' correctly excludes the timestamped .hive-sync from the tree comparison. src/docs/crux-publish.md:80-88 describes the intended behaviour accurately once the depth is fixed.
The red test/coverage checks: this diff touches no Go, so they are not caused by this change; I have not triaged them.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
cc6844b
ℹ️ Sentinel notice — informational; author is trusted (owner), merge is not blockedHive flagged this PR (author @clubanderson, head
Hive did not add the |
|
hive/awaiting-ci: 🐝 Hive Agent: — hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
…tting A crux/vX.Y.Z tag can point at a hive commit older than what the push trigger has already synced; committing its tree on top of main would roll the published repo back. Find the commit on main whose tree equals the publish tree (ignoring .hive-sync) and tag that; error out if none matches or the tag already exists. Verified the match loop locally against two synced commits. Refs #14138 Signed-off-by: Andrew Anderson <andy@clubanderson.com>
cc6844b to
c4a3a2e
Compare
There was a problem hiding this comment.
Correctness (read at head c4a3a2e)
1. The match loop can only ever see main's tip — the shallow checkout defeats the fix. .github/workflows/crux-sync.yml:106 checks out hivecommons/crux with fetch-depth: 1, but the new release path at :144-146 walks git rev-list -n 200 HEAD. In a depth-1 clone that yields exactly one commit (verified: git clone --depth 1 hivecommons/crux && git rev-list -n 200 HEAD | wc -l → 1). So a crux/vX.Y.Z tag matches only if it points at the hive commit of the latest sync; the exact case this PR exists for (tag behind the newest push-triggered sync) hits :147-150 and fails with "no commit on main matches … sync it first" — and re-syncing cannot help, since main is already ahead. The PR body's own example (hive 4bee931f95f7 → crux 4478555, two commits behind today's tip 544a36a) would fail in CI; it only worked locally because that clone had history. Fail-safe, but the feature doesn't work.
Fix — give the crux checkout history (repo is ~11 commits):
persist-credentials: true
fetch-depth: 0
(or, if you'd rather keep the push path shallow, git fetch -q --deepen=200 origin main just before the for c in $(git rev-list …) loop at :144.)
2. (low) In release mode git reset -q --hard HEAD at :151 runs before "Upload publish tree" (:181-188), so the crux-publish-tree artifact for a tag run now contains main's tip tree, not the tagged hive commit's tree. Harmless for the happy path, but the artifact is the documented manual-recovery input, and on the "no match" failure above it would be the wrong tree to push by hand. Consider moving the reset after the upload, or skipping it (the Push step never commits in release mode anyway).
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Conclusion: the tag-run design is right (never move main, tag the matching synced commit), but as wired the match loop can only ever see one commit, so the exact case this PR exists for — a crux/vX.Y.Z tag pointing at a hive commit older than the latest push-triggered sync — fails with "no commit on main matches … sync it first", and syncing again cannot fix it (it would produce the newest tree, not the tagged one).
Mechanism
.github/workflows/crux-sync.yml:106— thecrux-publishcheckout isfetch-depth: 1. This PR does not change it, and nothing in the job unshallows (grep -n 'unshallow\|git fetch'on the head file finds nothing)..github/workflows/crux-sync.yml:149—for c in $(git rev-list -n 200 HEAD)runs in that checkout. On a depth-1 clonerev-liststops at the shallow boundary and returns onlyHEAD. (Checked: a freshgit clone --depth 1of a 3-commit repo →git rev-list -n 200 HEAD | wc -l=1; full clone =3.)- So
git diff --cached --quiet "$c" -- . ':!.hive-sync'is evaluated againstmain's tip only. It matches only when the tag points at the most recently synced hive commit; otherwisetargetstays empty and the step exits 1 at line 152.
Consequence: tagging anything but the very latest synced src/crux commit is a hard failure in CI. The local verification in the body (4bee931f95f7 → 4478555, 6350270f14dc → bb649b1) walks real history, which is why it worked there and won't in the runner. Not a regression against today's rollback behaviour — the run fails instead of rewinding main — but the feature as documented in src/docs/crux-publish.md ("finds the commit already on main whose tree matches") does not hold.
Fix is one line either way: fetch-depth: 0 on the hivecommons/crux checkout at line 106 (the repo is small), or a git fetch --deepen=200 origin main immediately before the loop at line 149 so the 200-commit window actually exists.
Everything else I checked holds: Resolve release tag is correctly moved ahead of Commit so $TAG is defined when the branch at line 143 runs; target is emitted on both paths and Push keys on CHANGED/TARGET consistently; the ls-remote --exit-code --tags guard correctly refuses to clobber an existing tag; .hive-sync exclusion is the right pathspec given it carries the ref/timestamp. Token handling and permissions: are unchanged.
Severity: high (release path broken for the normal case). Recommending the author push the depth fix; nothing else blocks this from my read.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
merged by Hive trusted-author auto-merge: author @clubanderson holds role owner and GitHub permission admin on repo; CI green at c4a3a2e. |
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
Follow-up to #14199/#14221 after the first syncs ran green (hivecommons/crux
ab8097b,bb649b1viaTOPUP_PUSH_TOKEN).A
crux/vX.Y.Ztag run checked out hive at the tag, rebuilt the publish tree and committed it on top ofhivecommons/cruxmain. If the tag points at a hive commit older than the latest push-triggered sync (normal: tag after a few moresrc/cruxmerges), that commit rollsmainback to the older tree. Tag runs now never movemain: they walkmainfor the commit whose tree matches the tagged hive commit (ignoring.hive-sync) and pushvX.Y.Zthere; no match → clear error telling you to sync first; tag already on crux → error rather than clobber.Verified the match loop locally: publish tree of hive
4bee931f95f7→ crux4478555(the newest commit with that code), hive6350270f14dc→ cruxbb649b1. Docs updated insrc/docs/crux-publish.md; notes thatv0.1.0was tagged directly on crux.Refs #14138