fix(autocoder): pin target SHA in merge-to-integration.sh, don't trust mutable HEAD - #133
Open
laird wants to merge 1 commit into
Open
fix(autocoder): pin target SHA in merge-to-integration.sh, don't trust mutable HEAD#133laird wants to merge 1 commit into
laird wants to merge 1 commit into
Conversation
… of re-reading HEAD
A concurrent /fix invocation reusing the same worktree directory for a
different issue can run `git checkout -b feature/issue-N origin/<integration>`
while an in-flight merge-to-integration.sh for another issue is still mid-run
(its test-cmd can take minutes). Both the push (`HEAD:$INTEGRATION_BRANCH`)
and the later tree-verification (`HEAD^{tree}`) re-read the worktree's
mutable HEAD at the moment they run, so a hijack between the pre-integration
merge and those steps makes both silently push/verify nothing while the
script still reports "merged and verified".
Capture the intended commit's SHA once, immediately after each merge that
produces it, and use that SHA (not HEAD) for the push, the retry loop, the
GitHub API fallback, and the tree comparison. A git object hash is valid
regardless of what HEAD points to later, so it can't be fooled by the hijack
the way re-reading HEAD was. Also added a check right after the initial
checkout that fails loudly if the worktree isn't actually on $FEATURE,
instead of silently proceeding on whatever branch happens to be checked out.
Added tests/test_merge_to_integration_worktree_hijack.sh, which reproduces
the exact hijack (via --test-cmd, which runs in the script's own cwd — the
same real repo the checkout below repoints) against a real throwaway git
repo. Confirmed this fails against the pre-fix script (reports "merged and
verified" while main only has the pre-existing content) and passes against
the fix (main actually contains the feature commit).
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
Reported against athena2 as ey-org/athena2#1736 (P0, corroborated by a second, independent observer on that thread).
merge-to-integration.shpushes withgit push origin "HEAD:${INTEGRATION_BRANCH}"and later verifies withgit rev-parse "HEAD^{tree}"— both re-read the worktree's mutableHEADat the moment they run, not a value pinned when the script actually had the right commit checked out.When a second
/fixinvocation reuses the same worktree directory for a different issue (git checkout -b feature/issue-N origin/<integration>) while the first script's--test-cmdis still running (which can take minutes), it silently repoints that worktree's.git/HEAD. The first script's later push and tree-check then operate on the hijacked branch instead of the commit it actually built — and in the reported incident the hijacked branch happened to already equalorigin/main, so the push reportedEverything up-to-dateand the tree comparison matched trivially. The script printed✅ ... merged and verifiedwhile nothing had landed.This is the swarm's only proof a merge landed (branch protection has
enforce_admins: false), so a verification step that can pass on a no-op run makes every "merged and verified" log line unreliable.Fix
Capture the commit we intend to land (
TARGET_SHA=$(git rev-parse HEAD)) immediately after each merge that produces it (the initial pre-integration sync, and again after each push-retry re-sync), and use$TARGET_SHA— notHEAD— for:git push origin "${TARGET_SHA}:${INTEGRATION_BRANCH}")git rev-parse "${TARGET_SHA}^{tree}")A captured SHA is a git object reference, valid regardless of what HEAD is repointed to afterward, so it can't be fooled by a worktree hijack the way re-reading
HEADwas.Also added a check right after the initial
git checkout "$FEATURE"that fails loudly if the worktree isn't actually on$FEATURE, instead of silently proceeding on whatever happens to be checked out — this catches the same hazard if it occurs before the script's first merge.Out of scope (flagged in the issue as a "consider"): a lock around worktree reuse in the dispatcher, so a new
/fixinvocation can'tgit checkoutin a worktree that still has a livemerge-to-integration.sh/merge-poll.shprocess. That's a separate, larger change to the worker-loop dispatcher and doesn't block this fix — this fix makes the verification trustworthy even if that hazard isn't eliminated.Testing
Added
tests/test_merge_to_integration_worktree_hijack.sh: builds a real throwaway git repo, runs the script with--test-cmd 'git checkout -b feature/issue-8888 origin/main'(the hijack —--test-cmdrunsbash -cin the script's own cwd, the same real repo the checkout repoints), and asserts the feature commit actually reachesorigin/main.origin/mainonly contains the pre-existing content (reproduces the exact reported incident).origin/maincontains the feature commit.bash tests/run-shell-suite.sh→ 27/27 passed (including the new test).bash -n plugins/autocoder/scripts/*.sh && python3 -m py_compile plugins/autocoder/scripts/*.py→ OK.Closes the athena2-side defect once this merges; will comment back on ey-org/athena2#1736 with this PR link.