Skip to content

fix(triggers): pr-conflict-detected fires on opened + reopened, not only synchronize#1252

Merged
zbigniewsobiecki merged 1 commit intodevfrom
fix/pr-conflict-detected-fires-on-opened
May 2, 2026
Merged

fix(triggers): pr-conflict-detected fires on opened + reopened, not only synchronize#1252
zbigniewsobiecki merged 1 commit intodevfrom
fix/pr-conflict-detected-fires-on-opened

Conversation

@zbigniewsobiecki
Copy link
Copy Markdown
Member

Live regression — auto-resolve never fired on PRs that open with conflicts

ucho/PR #226 (`aaight`, opened 2026-05-02T16:51:32, base=`dev`, mergeable=CONFLICTING, mergeStateStatus=DIRTY) never had `resolve-conflicts` dispatched. The PR sat conflicting with no auto-resolution; user had to manually intervene.

Trigger config IS enabled for ucho:

```
Agent Event Enabled Parameters
resolve-conflicts scm:pr-conflict-detected yes -
```

So configuration is fine. Bug is in the matcher.

Root cause

`src/triggers/github/pr-conflict-detected.ts:32-42` hard-codes `payload.action !== 'synchronize'`:

```ts
matches(ctx: TriggerContext): boolean {
if (ctx.source !== 'github') return false;
if (!isGitHubPullRequestPayload(ctx.payload)) return false;
const payload = ctx.payload;
// Only trigger on synchronize events (when PR head is pushed/updated)
if (payload.action !== 'synchronize') return false;
return true;
}
```

PR #226 only fired `pull_request.opened` (no commits pushed since). Matcher returned `false` → `handle()` never invoked → handler's mergeability retry + dispatch path never ran. The PR is stuck conflicting until someone manually pushes a commit, which is exactly the friction we're supposed to be removing.

The existing test at lines 80-88 explicitly codified the bug (`does not match non-synchronize action` with `action: 'opened'`). This PR flips it.

The fix

One-line widen: accept `opened`, `reopened`, AND `synchronize`. All three are GitHub's "PR head is the candidate state" signals. `closed`, `edited`, `labeled`, `assigned`, `ready_for_review` correctly stay rejected.

```ts
if (
payload.action !== 'opened' &&
payload.action !== 'reopened' &&
payload.action !== 'synchronize'
) {
return false;
}
```

Why the handler body needs no changes

The handler at lines 44+ already covers all three event types correctly:

  • `gateTriggerEnabled` — config check.
  • `requirePersonaIdentities` + `gateCascadePersona` — only fires for cascade-bot PRs (loop-prevention).
  • `gateBaseBranch` — only fires when targeting the project's base branch.
  • `mergeable === null` retry loop — handles GitHub's async mergeability computation, which is most prominent on `opened`.
  • Skips if `mergeable !== false` — PRs that turn out to be mergeable are safely ignored.
  • `gateAttemptLimit(MAX_ATTEMPTS=2)` — prevents loops.

Cost of widening: one extra getPR API call per opened/reopened cascade-impl PR. Bounded by the cascade-persona gate.

Tests

  • Replaced the inverted `does not match non-synchronize action` (the bug pin) with `matches opened action (PR Merge dev to main #226 regression pin)` asserting `true`.
  • Added `matches reopened action`.
  • Replaced the single `does not match closed action` with an `it.each` covering `closed`, `edited`, `labeled`, `unlabeled`, `assigned`, `ready_for_review` — confirms we narrowed the rejection set, not widened carelessly.
  • 22/22 pass; 2503/2503 across unit-triggers + unit-api.

Out of scope (intentionally — flag for follow-up)

  • Periodic mergeability re-check for PRs whose base branch advanced and created a new conflict without any per-PR event (long-lived PRs). Would need a periodic scan analogous to `dangling-image-cleanup.ts` lifecycle. PR Merge dev to main #226's case is solved by the simpler `opened` fix; defer.
  • Filter out drafts — separate decision; current behavior on `synchronize` doesn't filter drafts either.

Test plan

  • `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` clean
  • `npm run lint` clean
  • CI green
  • Reviewer approval (@nhopeatall)
  • After deploy: open a fresh cascade-impl PR known to conflict against `dev`. Within ~1 min, Loki shows `PR has merge conflicts — triggering resolve-conflicts agent` and a new `resolve-conflicts` run record appears in `cascade runs list`.
  • For PR Merge dev to main #226 specifically: close+reopen (or push an empty commit) to re-fire the matcher; should auto-dispatch resolve-conflicts.

🤖 Generated with Claude Code

…nly synchronize

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) <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@nhopeatall nhopeatall left a comment

Choose a reason for hiding this comment

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

LGTM - clean, minimal bug fix with solid root-cause analysis and well-structured tests. The matcher widening from synchronize-only to opened/reopened/synchronize is correct: all three are GitHub candidate-head-SHA signals, and the handler body (persona gate, base-branch gate, mergeability retry loop, attempt limit) already handles all three identically. The opened+reopened overlap with PROpenedTrigger is safe - they dispatch different agent types (resolve-conflicts vs review) and the concurrency lock is per-agent-type. Test coverage is thorough: positive pins for all three accepted actions (including the PR #226 regression pin), and it.each negative coverage across six rejected actions.

🕵️ claude-code · claude-opus-4-6 · run details

@codecov
Copy link
Copy Markdown

codecov Bot commented May 2, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@zbigniewsobiecki zbigniewsobiecki merged commit 26fa2d7 into dev May 2, 2026
9 checks passed
@zbigniewsobiecki zbigniewsobiecki deleted the fix/pr-conflict-detected-fires-on-opened branch May 2, 2026 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants