From e356d3a1c8c7b404ed241174b9b1ccb608896b1b Mon Sep 17 00:00:00 2001 From: Zbigniew Sobiecki Date: Sat, 2 May 2026 17:49:32 +0000 Subject: [PATCH] fix(triggers): pr-conflict-detected fires on opened + reopened, not only synchronize MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ucho/PR #226 was created by aaight at 2026-05-02T16:51:32 already CONFLICTING against `dev`. resolve-conflicts never fired. The PR sat open with merge conflicts and no auto-resolution; the user had to manually intervene. Trigger config IS enabled for ucho: resolve-conflicts scm:pr-conflict-detected yes Root cause: `pr-conflict-detected.matches()` at `src/triggers/github/pr-conflict-detected.ts:32-42` hard-coded `payload.action !== 'synchronize'`, rejecting `opened` events. Since PR #226 only fired a `pull_request.opened` event (no commits pushed since), the matcher returned `false` and `handle()` never ran. The handler body — gates on cascade persona + base branch, retries on `mergeable === null`, dispatches when `mergeable === false` — never got the chance. The existing test at lines 80-88 explicitly codified the bug (`does not match non-synchronize action` with `action: 'opened'`). Fix: widen the matcher to accept `opened`, `reopened`, and `synchronize` — the three actions GitHub emits when the PR head is the candidate state we should check for mergeability. `closed`, `edited`, `labeled`, `assigned`, `ready_for_review` correctly stay rejected. Why the handler body needs no changes: the `mergeable === null` retry loop already handles GitHub's async mergeability computation (most prominent on `opened`); cascade-persona + base-branch gates and the `gateAttemptLimit` remain untouched. The cost of the widening is one extra getPR API call per opened/reopened cascade-impl PR — small and bounded by the persona gate. Tests: - Replaced inverted `does not match non-synchronize action` (was the bug pin) with `matches opened action (PR #226 regression pin)` asserting true. - Added `matches reopened action`. - Replaced single `does not match closed action` with an `it.each` over `closed`, `edited`, `labeled`, `unlabeled`, `assigned`, `ready_for_review` to confirm we narrowed the rejection set, not widened carelessly. Out of scope (follow-up): periodic mergeability re-check for PRs whose base branch advanced and created a new conflict without any per-PR event. Real concern for long-lived PRs, but the simpler `opened` fix solves PR #226's case. Defer. Verification: - `npx vitest run --project unit-triggers tests/unit/triggers/pr-conflict-detected.test.ts` → 22/22. - `npx vitest run --project unit-triggers --project unit-api` → 2503/2503. - `npm run typecheck` + `npm run lint` clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/triggers/github/pr-conflict-detected.ts | 23 ++++++++++++-- .../triggers/pr-conflict-detected.test.ts | 30 ++++++++++++++++--- 2 files changed, 47 insertions(+), 6 deletions(-) diff --git a/src/triggers/github/pr-conflict-detected.ts b/src/triggers/github/pr-conflict-detected.ts index 75e289c5..64a593c8 100644 --- a/src/triggers/github/pr-conflict-detected.ts +++ b/src/triggers/github/pr-conflict-detected.ts @@ -35,8 +35,27 @@ export class PRConflictDetectedTrigger implements TriggerHandler { const payload = ctx.payload; - // Only trigger on synchronize events (when PR head is pushed/updated) - if (payload.action !== 'synchronize') return false; + // Trigger on `opened`, `reopened`, and `synchronize` — the three + // actions that produce a candidate head SHA whose mergeability we + // should check: + // - opened: brand-new PR. Bit us on ucho/PR #226 (2026-05-02) — + // the impl bot opened the PR already CONFLICTING against `dev`, + // and because the matcher previously accepted only `synchronize`, + // `resolve-conflicts` never fired until someone pushed a commit. + // - reopened: closed PR brought back; mergeability may have flipped + // against the now-advanced base. + // - synchronize: new commit pushed to existing PR (the original + // intent of this trigger). + // `closed`, `edited`, `labeled`, etc. correctly stay rejected. + // The handler's `mergeable === null` retry loop covers GitHub's async + // mergeability computation that's most prominent on `opened`. + if ( + payload.action !== 'opened' && + payload.action !== 'reopened' && + payload.action !== 'synchronize' + ) { + return false; + } return true; } diff --git a/tests/unit/triggers/pr-conflict-detected.test.ts b/tests/unit/triggers/pr-conflict-detected.test.ts index f186223b..55fc10be 100644 --- a/tests/unit/triggers/pr-conflict-detected.test.ts +++ b/tests/unit/triggers/pr-conflict-detected.test.ts @@ -77,21 +77,43 @@ describe('PRConflictDetectedTrigger', () => { expect(trigger.matches(ctx)).toBe(false); }); - it('does not match non-synchronize action', () => { + // PR #226 (2026-05-02) regression pin: the impl bot opened the PR + // already conflicting against `dev`, but the matcher previously + // accepted only `synchronize`, so resolve-conflicts never fired + // until someone pushed a commit. Both `opened` and `reopened` must + // match so the handler's mergeability retry + dispatch path runs. + it('matches opened action (PR #226 regression pin)', () => { const ctx: TriggerContext = { project: mockProject, source: 'github', payload: makeSynchronizePayload({ action: 'opened' }), }; - expect(trigger.matches(ctx)).toBe(false); + expect(trigger.matches(ctx)).toBe(true); + }); + + it('matches reopened action', () => { + const ctx: TriggerContext = { + project: mockProject, + source: 'github', + payload: makeSynchronizePayload({ action: 'reopened' }), + }; + + expect(trigger.matches(ctx)).toBe(true); }); - it('does not match closed action', () => { + it.each([ + ['closed'], + ['edited'], + ['labeled'], + ['unlabeled'], + ['assigned'], + ['ready_for_review'], + ])('does not match %s action', (action) => { const ctx: TriggerContext = { project: mockProject, source: 'github', - payload: makeSynchronizePayload({ action: 'closed' }), + payload: makeSynchronizePayload({ action }), }; expect(trigger.matches(ctx)).toBe(false);