Skip to content

Commit d29b809

Browse files
authored
fix(bot): keep conflict-resolver escalation loop-safe when git commit fails (#2866)
The "Verify resolution, push, or escalate (loop-safe)" step in bot-resolve-conflicts.yml runs under `set -euo pipefail` and finished a resolved merge with a bare `git commit --no-edit`. If that commit exits non-zero (a pre-commit hook rejects it), or `git push` later fails, `set -e` aborts the step before the escalation block runs — so no `conflict-attempted` marker is posted and the freshen sweep re-dispatches the resolver every cycle (runaway loop), since the skip-check only matches that marker. Arm an EXIT trap that posts the marker and `conflict:needs-human` label on ANY non-zero exit (a failed commit, a failed push, or any command added to the step later), guarded by a `pushed` flag so the clean-push success path stays a no-op. This satisfies the acceptance criterion that "any failure on the resolve path leaves a conflict-attempted marker". Add a regression spec (BotResolveConflictsLoopSafeSpec) pinning the invariant: the exit trap is armed before the merge commit, and the marker + label are posted from the escalation path. Closes #2849 Signed-off-by: Peter Amiri <peter@alurium.com>
1 parent 772d05b commit d29b809

2 files changed

Lines changed: 131 additions & 16 deletions

File tree

.github/workflows/bot-resolve-conflicts.yml

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -142,29 +142,40 @@ jobs:
142142
BASE_SHA: ${{ steps.merge.outputs.base }}
143143
run: |
144144
set -euo pipefail
145-
# Success = a clean, committed merge to push. Failure = conflict markers
146-
# still present, OR the command's safety gate aborted the merge with no
147-
# new commit. On failure the PR stays DIRTY, so we MUST post the
148-
# conflict-attempted marker and escalate — otherwise the freshen sweep
149-
# re-dispatches this resolver every cycle (runaway loop), since the
150-
# skip-check only matches that marker.
145+
# Success = a clean, committed merge pushed to the PR branch. ANY other
146+
# outcome must escalate and post the conflict-attempted marker, or the PR
147+
# stays DIRTY with no marker and the freshen sweep re-dispatches this
148+
# resolver every cycle (runaway loop) — the skip-check only matches that
149+
# marker. A bare merge-commit (or push) that exits non-zero under
150+
# "set -e" would abort this step BEFORE a linear escalation block could
151+
# run, so we arm an EXIT trap that posts the marker on ANY non-zero exit
152+
# — including failures of commands added to this step later. See issue
153+
# #2849 (a #2847 follow-up).
154+
pushed=0
155+
escalate() {
156+
# Runs from the EXIT trap. The success path sets pushed=1 first, so a
157+
# clean push is a no-op here; every other exit lands the marker + label.
158+
if [ "$pushed" = "1" ]; then return 0; fi
159+
echo "::error::resolve path produced no pushed merge; escalating to human"
160+
gh label create conflict:needs-human --repo "$REPO" --color B60205 \
161+
--description "Merge conflict needs manual resolution" 2>/dev/null || true
162+
gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label conflict:needs-human || true
163+
gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$(printf '%s\n' \
164+
"⚠️ **Automated content-conflict resolution did not complete** — leaving this for a human." \
165+
"" \
166+
"<!-- wheels-bot:conflict-attempted:$PR_NUMBER -->")" || true
167+
}
168+
trap escalate EXIT
169+
151170
if git diff --name-only --diff-filter=U | grep -q .; then
152171
git merge --abort 2>/dev/null || true # unresolved markers -> not resolved
153172
elif [ -f .git/MERGE_HEAD ]; then
154173
git commit --no-edit # resolved but uncommitted -> finish merge
155174
fi
156175
if [ "$(git rev-parse HEAD)" != "$BASE_SHA" ]; then
157176
git push origin HEAD # resolved cleanly; PR checks re-validate
177+
pushed=1
158178
exit 0
159179
fi
160-
# No new commit -> resolution did not complete. Escalate to a human and
161-
# post the marker so freshen does not re-dispatch this resolver.
162-
gh label create conflict:needs-human --repo "$REPO" --color B60205 \
163-
--description "Merge conflict needs manual resolution" 2>/dev/null || true
164-
gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label conflict:needs-human
165-
gh pr comment "$PR_NUMBER" --repo "$REPO" --body "$(printf '%s\n' \
166-
"⚠️ **Automated content-conflict resolution did not complete** — leaving this for a human." \
167-
"" \
168-
"<!-- wheels-bot:conflict-attempted:$PR_NUMBER -->")"
169-
echo "::error::resolve path produced no committed merge; escalated to human"
180+
# No new commit -> resolution did not complete; the EXIT trap escalates.
170181
exit 1
Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
component extends="wheels.WheelsTest" {
2+
3+
// Regression for issue ##2849 (a follow-up to ##2847).
4+
//
5+
// The "Verify resolution, push, or escalate (loop-safe)" step in
6+
// .github/workflows/bot-resolve-conflicts.yml runs under `set -euo pipefail`.
7+
// It finishes a resolved-but-uncommitted merge with `git commit --no-edit`,
8+
// then — if no new commit landed — escalates by applying the
9+
// `conflict:needs-human` label and posting a comment carrying the
10+
// `wheels-bot:conflict-attempted` marker. The freshen sweep's skip-check
11+
// keys off that marker, so the marker is what stops the resolver from being
12+
// re-dispatched on every cycle.
13+
//
14+
// The bug: a *bare* `git commit --no-edit` is fatal under `set -e`. If the
15+
// commit itself exits non-zero (e.g. a pre-commit hook rejects it), the step
16+
// aborts at that line, BEFORE the escalation block runs. No marker is posted,
17+
// the PR stays DIRTY, and the freshen sweep re-dispatches this resolver every
18+
// cycle (runaway loop). The author's original comment anticipated the
19+
// "merge aborted -> no new commit" path (which correctly falls through to
20+
// escalation) but not the "the commit command itself failed" path.
21+
//
22+
// The fix arms an exit-time trap that posts the marker on ANY non-zero exit
23+
// of the step (a hook-rejected commit, a failed push, or any command added
24+
// later) — satisfying the acceptance criterion that "any failure on the
25+
// resolve path leaves a conflict-attempted marker". This spec pins that
26+
// invariant with a static check of the workflow, since the step's behaviour
27+
// (gh / git side effects against a real checkout) cannot be exercised in a
28+
// unit test.
29+
30+
function run() {
31+
32+
describe("bot-resolve-conflicts.yml loop-safe escalation (issue ##2849)", () => {
33+
34+
// expandPath("/wheels") resolves to vendor/wheels via the configured
35+
// Lucee mapping; the repo root is two levels above.
36+
var repoRoot = expandPath("/wheels/../..");
37+
var workflow = repoRoot & "/.github/workflows/bot-resolve-conflicts.yml";
38+
39+
// Scope assertions to the finalize step (the last step in the file)
40+
// so we test the loop-safe escalation path specifically, not the
41+
// separate code-conflict escalation step that shares the same marker.
42+
var stepAnchor = "Verify resolution, push, or escalate (loop-safe)";
43+
44+
it("protects the merge commit so a failed commit cannot bypass escalation", () => {
45+
expect(fileExists(workflow)).toBeTrue("Missing file: " & workflow);
46+
var src = fileRead(workflow);
47+
48+
var anchorPos = find(stepAnchor, src);
49+
expect(anchorPos > 0).toBeTrue("Could not find the '" & stepAnchor & "' step in " & workflow);
50+
var block = mid(src, anchorPos, len(src) - anchorPos + 1);
51+
52+
// The step should still finish a resolved merge with a commit.
53+
var commitPos = reFindNoCase("git[[:space:]]+commit[[:space:]]+--no-edit", block);
54+
expect(commitPos > 0).toBeTrue(
55+
"The finalize step should still finish a resolved-but-uncommitted merge with "
56+
& "`git commit --no-edit`. See issue ##2849."
57+
);
58+
59+
// Protection takes one of the two shapes blessed by the acceptance
60+
// criteria: an exit-time trap armed BEFORE the commit, or the commit
61+
// itself guarded so its failure falls through to escalation.
62+
var trapPos = reFindNoCase("trap[[:space:]]+[^\n]*EXIT", block);
63+
var trapArmedBeforeCommit = trapPos > 0 && trapPos < commitPos;
64+
var commitIsGuarded =
65+
reFindNoCase("if[[:space:]]+![^\n]*git[[:space:]]+commit[[:space:]]+--no-edit", block) > 0
66+
|| reFindNoCase("git[[:space:]]+commit[[:space:]]+--no-edit[^\n]*\|\|", block) > 0;
67+
68+
expect(trapArmedBeforeCommit || commitIsGuarded).toBeTrue(
69+
"issue ##2849: under `set -euo pipefail` a bare `git commit --no-edit` that exits "
70+
& "non-zero (e.g. a pre-commit hook rejects it) aborts the step BEFORE the "
71+
& "escalation block runs, so no `conflict-attempted` marker is posted and the "
72+
& "freshen sweep re-dispatches the resolver forever. The finalize step MUST "
73+
& "either arm an exit-time trap before the commit (`trap ... EXIT`) or guard the "
74+
& "commit itself (`if ! git commit ...` / `git commit ... ||`) so the failure "
75+
& "falls through to the escalation path."
76+
);
77+
});
78+
79+
it("always posts the conflict-attempted marker and needs-human label on the escalation path", () => {
80+
var src = fileRead(workflow);
81+
82+
var anchorPos = find(stepAnchor, src);
83+
expect(anchorPos > 0).toBeTrue("Could not find the '" & stepAnchor & "' step in " & workflow);
84+
var block = mid(src, anchorPos, len(src) - anchorPos + 1);
85+
86+
expect(reFindNoCase("wheels-bot:conflict-attempted:", block) > 0).toBeTrue(
87+
"The finalize step must post the `wheels-bot:conflict-attempted` marker when "
88+
& "resolution does not complete — the freshen skip-check keys off it to stop "
89+
& "re-dispatching the resolver. See issue ##2849."
90+
);
91+
expect(reFindNoCase("conflict:needs-human", block) > 0).toBeTrue(
92+
"The finalize step must apply the `conflict:needs-human` label on escalation. "
93+
& "See issue ##2849."
94+
);
95+
expect(reFindNoCase("gh[[:space:]]+pr[[:space:]]+comment", block) > 0).toBeTrue(
96+
"The finalize step must publish the marker via `gh pr comment`. See issue ##2849."
97+
);
98+
});
99+
100+
});
101+
102+
}
103+
104+
}

0 commit comments

Comments
 (0)