Skip to content

fix: add fetch/rebase before push to handle concurrent workflow changes #186

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
stranske merged 1 commit into from
Dec 26, 2025
Merged

fix: add fetch/rebase before push to handle concurrent workflow changes #186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion .github/workflows/reusable-codex-run.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,24 @@ jobs:
COMMIT_SHA=$(git rev-parse HEAD)
echo "commit-sha=${COMMIT_SHA}" >> "$GITHUB_OUTPUT"

git push "https://x-access-token:${PUSH_TOKEN}@github.com/${{ github.repository }}" "HEAD:${TARGET_BRANCH}"
# Pull and rebase before push to handle concurrent changes (e.g., Autofix)
# This prevents push failures when the branch has been updated while Codex ran
REMOTE_URL="https://x-access-token:${PUSH_TOKEN}@github.com/${{ github.repository }}"
echo "::group::Sync with remote before push"
git fetch "${REMOTE_URL}" "${TARGET_BRANCH}" 2>/dev/null || true
if git rev-parse "FETCH_HEAD" >/dev/null 2>&1; then
Comment on lines +447 to +448

Copilot AI Dec 26, 2025

Copy link

Choose a reason for hiding this comment

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

The fetch operation silently ignores all errors with 2>/dev/null || true. While this prevents the workflow from failing on fetch errors, it means that if the fetch fails (e.g., due to network issues or authentication problems), the workflow will proceed without syncing with the remote and the subsequent push will still fail.

Consider logging a warning when fetch fails so that there's visibility into why the sync step didn't work, or alternatively, remove the || true and let fetch errors be visible.

Suggested change
git fetch "${REMOTE_URL}" "${TARGET_BRANCH}" 2>/dev/null || true
if git rev-parse "FETCH_HEAD" >/dev/null 2>&1; then
git fetch "${REMOTE_URL}" "${TARGET_BRANCH}" 2>/dev/null
FETCH_EXIT_CODE=$?
if [ "$FETCH_EXIT_CODE" -ne 0 ]; then
echo "::warning::git fetch failed with exit code ${FETCH_EXIT_CODE}; skipping sync with remote before push"
elif git rev-parse "FETCH_HEAD" >/dev/null 2>&1; then

Copilot uses AI. Check for mistakes.
if ! git rebase FETCH_HEAD; then
echo "::warning::Rebase failed, attempting merge strategy"
git rebase --abort 2>/dev/null || true
git pull --no-rebase "${REMOTE_URL}" "${TARGET_BRANCH}" --allow-unrelated-histories || true
Comment on lines +451 to +452

Copilot AI Dec 26, 2025

Copy link

Choose a reason for hiding this comment

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

The rebase abort command uses 2>/dev/null || true which silently suppresses errors. While this is generally acceptable for a cleanup operation, if the rebase abort fails, the repository could be left in a partially rebased state before attempting the merge.

Consider checking if the repository is in a clean state before attempting the merge fallback, or at minimum log a warning if the abort fails.

Suggested change
git rebase --abort 2>/dev/null || true
git pull --no-rebase "${REMOTE_URL}" "${TARGET_BRANCH}" --allow-unrelated-histories || true
if git rebase --abort 2>/dev/null; then
git pull --no-rebase "${REMOTE_URL}" "${TARGET_BRANCH}" --allow-unrelated-histories || true
else
echo "::warning::Rebase abort failed; skipping merge fallback to avoid inconsistent repository state"
fi

Copilot uses AI. Check for mistakes.

Copilot AI Dec 26, 2025

Copy link

Choose a reason for hiding this comment

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

The merge fallback on this line uses || true which silently suppresses all errors from the merge operation. If the merge fails (e.g., due to conflicts), the workflow will continue to the push step without having successfully integrated the remote changes, causing the push to fail with the same error this PR is trying to fix.

Consider removing the || true suffix and instead checking the exit code to handle merge conflicts explicitly. If merge conflicts occur, the workflow should either abort with a clear error message or use a more aggressive merge strategy.

Suggested change
git pull --no-rebase "${REMOTE_URL}" "${TARGET_BRANCH}" --allow-unrelated-histories || true
if ! git pull --no-rebase "${REMOTE_URL}" "${TARGET_BRANCH}" --allow-unrelated-histories; then
echo "::error::Failed to synchronize with remote branch '${TARGET_BRANCH}' due to merge conflicts or other errors. Please resolve conflicts manually and rerun."
exit 1
fi

Copilot uses AI. Check for mistakes.

Copilot AI Dec 26, 2025

Copy link

Choose a reason for hiding this comment

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

The --allow-unrelated-histories flag is used in the merge fallback, which can merge branches with completely unrelated commit histories. This is a very permissive setting that could potentially merge unrelated changes if the branch history has diverged significantly.

This flag should only be used if there's a specific reason to expect unrelated histories. For the concurrent push scenario described in the PR (where Autofix pushes to the same branch), the histories should be related. Consider removing this flag or documenting why it's necessary.

Suggested change
git pull --no-rebase "${REMOTE_URL}" "${TARGET_BRANCH}" --allow-unrelated-histories || true
git pull --no-rebase "${REMOTE_URL}" "${TARGET_BRANCH}" || true

Copilot uses AI. Check for mistakes.
fi
# Update commit SHA after rebase/merge
COMMIT_SHA=$(git rev-parse HEAD)
echo "commit-sha=${COMMIT_SHA}" >> "$GITHUB_OUTPUT"
Comment on lines +449 to +456

Copilot AI Dec 26, 2025

Copy link

Choose a reason for hiding this comment

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

The commit SHA is unconditionally updated after rebase/merge, even if the merge operation failed on line 452. Since the merge command has || true, a failed merge will leave the repository in an inconsistent state but still update the output with a potentially incorrect commit SHA.

The SHA update should only occur after verifying that the rebase or merge succeeded. Consider moving this inside a conditional block that checks if the previous operation was successful.

Suggested change
if ! git rebase FETCH_HEAD; then
echo "::warning::Rebase failed, attempting merge strategy"
git rebase --abort 2>/dev/null || true
git pull --no-rebase "${REMOTE_URL}" "${TARGET_BRANCH}" --allow-unrelated-histories || true
fi
# Update commit SHA after rebase/merge
COMMIT_SHA=$(git rev-parse HEAD)
echo "commit-sha=${COMMIT_SHA}" >> "$GITHUB_OUTPUT"
SYNC_OK=true
if ! git rebase FETCH_HEAD; then
echo "::warning::Rebase failed, attempting merge strategy"
git rebase --abort 2>/dev/null || true
if ! git pull --no-rebase "${REMOTE_URL}" "${TARGET_BRANCH}" --allow-unrelated-histories; then
echo "::warning::Merge strategy pull failed; retaining pre-sync commit SHA"
SYNC_OK=false
fi
fi
if [ "$SYNC_OK" = true ]; then
# Update commit SHA after successful rebase/merge
COMMIT_SHA=$(git rev-parse HEAD)
echo "commit-sha=${COMMIT_SHA}" >> "$GITHUB_OUTPUT"
fi

Copilot uses AI. Check for mistakes.
fi
echo "::endgroup::"

git push "${REMOTE_URL}" "HEAD:${TARGET_BRANCH}"

echo "::notice::Pushed commit ${COMMIT_SHA} with ${CHANGED_FILES} file(s) changed"

Expand Down
1 change: 0 additions & 1 deletion keepalive-metrics.ndjson
View file
Open in desktop

This file was deleted.

Loading