diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ef8181c3..6be893183 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable user-visible changes to CASCADE are documented here. The format is l ## Unreleased +### Fixed + +- **`resolve-conflicts` agent no longer silently skips when GitHub's async mergeability computation hasn't resolved by the time the `pull_request` webhook is processed** (spec 020). `PRConflictDetectedTrigger` previously exhausted a 2×2s synchronous retry budget and silently discarded the event when `mergeable === null` — because GitHub never sends a follow-up webhook once mergeability resolves, the `resolve-conflicts` agent never fired. The trigger now returns `TriggerResult.deferredRecheck`, which causes the router to schedule a bare BullMQ delayed re-check job ~45s later via `scheduleCoalescedJob` (deduped per PR). The worker re-dispatches via the trigger registry to get fresh mergeability state. Multiple rapid webhooks for the same PR coalesce to a single re-check job. If mergeability is still `null` after the re-check fires, a Sentry event is captured under tag `mergeability_recheck_exhausted` and a WARN log is emitted — not a silent discard. Observed live on ucho/PR #329 (2026-05-07). See [spec 020](docs/specs/020-github-mergeability-deferred-recheck.md). + ### Added - **Sentry alerts now materialize as real PM work items in the configured `alerts` slot** (spec 019). The alerting trigger previously minted a synthetic `sentry:issue:` workItemId, which caused a Trello 400 error on the budget gate for projects with a cost custom field, silently killing every alerting run. The trigger now calls `materializeAlertWorkItem('sentry', issueId, project, hints)`, which creates (or idempotently retrieves) a Trello card / JIRA issue / Linear issue in the PM `alerts` slot and returns its native ID. Budget tracking, lifecycle transitions, and label writes all work correctly on the resulting card. A partial UNIQUE index on `(project_id, external_source, external_id)` in `work_items` ensures a second Sentry alert on the same issue produces the same PM card, not a duplicate. Configure the `alerts` slot in the PM wizard's **Status Mapping** step; validation pre-flight emits a `pm`-category error when the slot is unset and an alerting trigger is enabled. See [spec 019](docs/specs/019-sentry-alert-pm-materialization.md). diff --git a/CLAUDE.md b/CLAUDE.md index d1a4c292c..5592c11a0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -123,6 +123,8 @@ Some triggers take params (e.g. `review` + `scm:check-suite-success` accepts `{" **Post-completion review dispatch** — when an implementation agent succeeds with a PR, the execution pipeline checks CI status and fires the review agent deterministically (before the container exits). This guarantees review dispatch within seconds of implementation completion, regardless of GitHub webhook timing. Uses the same `claimReviewDispatch` dedup key as the `check-suite-success` trigger, so the two paths cannot double-enqueue. +**Deferred re-check** — any trigger handler can return `TriggerResult.deferredRecheck: { delayMs, coalesceKey }` (with `agentType: null`) to schedule a bare delayed job that re-dispatches via the trigger registry when it fires. The router uses `scheduleCoalescedJob` for dedup. The worker detects re-check jobs via `GitHubJob.mergeabilityRecheckAttempt` and Sentry-captures under tag `mergeability_recheck_exhausted` when the trigger still cannot resolve state. Workers do not re-queue — a second `deferredRecheck` return in the worker handler exits gracefully. + **Worker exit diagnostics** — when a worker container exits non-zero, the router calls `container.inspect()` *before* AutoRemove reaps it and stamps the run record's `error` field with a structured, grep-stable string: `Worker crashed with exit code N · OOMKilled= · reason=""`. The `OOMKilled=true` marker is the definitive cgroup-OOM signal (per Docker's own `State.OOMKilled`); a 137 exit *without* `OOMKilled=true` means the kill came from inside the container or from a non-cgroup signal — *not* memory. The `[WorkerManager] Resolved spawn settings` log emitted at every spawn includes both `projectWatchdogTimeoutMs` and `globalWorkerTimeoutMs` so post-mortems can confirm whether the per-project override actually won. See `src/router/active-workers.ts:formatCrashReason` for the format and `tests/unit/router/container-manager-diagnostics.test.ts` for regression pins. **Dispatch failure semantics** — spec 015 (verified live in prod via the ucho/MNG-350 incident on 2026-04-26): diff --git a/docs/plans/020-github-mergeability-deferred-recheck/1-infrastructure.md.done b/docs/plans/020-github-mergeability-deferred-recheck/1-infrastructure.md.done new file mode 100644 index 000000000..9af585573 --- /dev/null +++ b/docs/plans/020-github-mergeability-deferred-recheck/1-infrastructure.md.done @@ -0,0 +1,255 @@ +--- +id: 020 +slug: github-mergeability-deferred-recheck +plan: 1 +plan_slug: infrastructure +level: plan +parent_spec: docs/specs/020-github-mergeability-deferred-recheck.md +depends_on: [] +status: done +--- + +# 020/1: Deferred Re-check Infrastructure + +> Part 1 of 2 in the 020-github-mergeability-deferred-recheck plan. See [parent spec](../../specs/020-github-mergeability-deferred-recheck.md). + +## Summary + +Adds the generic `deferredRecheck` field to `TriggerResult` and wires a new branch in `processRouterWebhook` that schedules a bare (no embedded `triggerResult`) BullMQ delayed job when any trigger returns that field. Also adds `mergeabilityRecheckAttempt?: number` to `GitHubJob` and updates `GitHubRouterAdapter.buildJob` to populate it (and omit `triggerResult`) when the result carries `deferredRecheck`. + +This plan ships infrastructure only — no trigger returns `deferredRecheck` yet (plan 2 wires the conflict trigger). The mechanism is fully exercisable by unit tests using a mock adapter. + +**Components delivered:** +- `src/types/index.ts` — `TriggerResult.deferredRecheck` optional field +- `src/router/webhook-processor.ts` — new deferred re-check branch after the `skipReason` check +- `src/router/queue.ts` — `GitHubJob.mergeabilityRecheckAttempt` optional field +- `src/router/adapters/github.ts` — `buildJob` omits `triggerResult` and sets `mergeabilityRecheckAttempt: 1` when `deferredRecheck` is present +- `CLAUDE.md` — note documenting the `deferredRecheck` pattern (analogous to the "Post-completion review dispatch" note) + +**Deferred to plan 2:** +- `PRConflictDetectedTrigger` returning `deferredRecheck` +- Worker-side Sentry capture on re-check exhaustion +- `processGitHubWebhook` `isRecheckJob` parameter +- CHANGELOG entry + +--- + +## Spec ACs satisfied by this plan + +- Spec AC #2 (multiple webhooks coalesce) — **full** (router uses `scheduleCoalescedJob` with caller-supplied coalesceKey) +- Spec AC #6 (router returns within 10s) — **full** (deferred re-check branch is non-blocking: `scheduleCoalescedJob` is a fast Redis enqueue) +- Spec AC #7 (synchronous fast path unchanged) — **full** (no change to existing retry logic in any trigger) +- Spec AC #8 (generic `deferredRecheck` field usable by any trigger) — **full** (`TriggerResult.deferredRecheck` type + router branch exist independent of trigger identity) +- Spec AC #1 (agent fires after deferred re-check) — **partial** (router schedules the bare job; plan 2 wires the trigger) +- Spec AC #3 (WARN + Sentry on exhaustion) — **partial** (`mergeabilityRecheckAttempt` field set in the job; plan 2 adds the worker-side detection) + +--- + +## Depends On + +None. This plan is the foundation. + +--- + +## Detailed Task List (TDD) + +### 1. `TriggerResult.deferredRecheck` type addition + +**Tests first** (`tests/unit/router/deferred-recheck.test.ts` — new file): + +- `TriggerResult accepts deferredRecheck field` — unit — construct a `TriggerResult` object with `agentType: null, agentInput: {}, deferredRecheck: { delayMs: 45000, coalesceKey: 'test:key' }` — TypeScript compilation must not error. Expected red: TypeScript compile error `Property 'deferredRecheck' does not exist on type 'TriggerResult'`. + +**Implementation** (`src/types/index.ts`): +- Add to `TriggerResult`: + ``` + deferredRecheck?: { + delayMs: number; + coalesceKey: string; + }; + ``` +- JSDoc: "When set and `agentType` is `null`, the router schedules a bare delayed job (no embedded trigger result) keyed by `coalesceKey`. The worker re-dispatches via the trigger registry when the job fires, obtaining fresh state. Any trigger handler can use this field; the router branch in `processRouterWebhook` is adapter-agnostic." + +--- + +### 2. `GitHubJob.mergeabilityRecheckAttempt` field + +**Tests first** (`tests/unit/router/deferred-recheck.test.ts`): + +- `GitHubJob accepts mergeabilityRecheckAttempt field` — unit — TypeScript: assign `mergeabilityRecheckAttempt: 1` on a `GitHubJob` object. Expected red: TypeScript compile error `Property 'mergeabilityRecheckAttempt' does not exist on type 'GitHubJob'`. + +**Implementation** (`src/router/queue.ts`): +- Add to `GitHubJob`: `mergeabilityRecheckAttempt?: number;` +- JSDoc: "Set to 1 when this job is a deferred mergeability re-check (router side). The worker uses `!!job.mergeabilityRecheckAttempt` to detect re-check jobs and emit Sentry capture when the trigger still cannot determine mergeability." + +--- + +### 3. `GitHubRouterAdapter.buildJob` — deferred re-check behavior + +**Tests first** (`tests/unit/router/deferred-recheck.test.ts`): + +- `buildJob: omits triggerResult when result.deferredRecheck is set` — unit — call `adapter.buildJob(event, payload, project, resultWithDeferredRecheck, undefined)` → `job.triggerResult` is `undefined`. Expected red: `expected undefined, got [object Object]`. +- `buildJob: sets mergeabilityRecheckAttempt: 1 when result.deferredRecheck is set` — unit — same call → `job.mergeabilityRecheckAttempt === 1`. Expected red: `expected 1, got undefined`. +- `buildJob: includes triggerResult normally when deferredRecheck is absent` — unit — call with normal `TriggerResult` (no `deferredRecheck`) → `job.triggerResult` equals the result object. Expected red: n/a (regression pin — stays green; verify it passes before changing `buildJob`). +- `buildJob: does not set mergeabilityRecheckAttempt when deferredRecheck is absent` — unit — normal result → `job.mergeabilityRecheckAttempt` is `undefined`. Expected red: n/a (regression pin). + +**Implementation** (`src/router/adapters/github.ts`): + +Modify `buildJob`: +``` +buildJob(event, payload, _project, result, ackResult): CascadeJob { + const isDeferredRecheck = !!result.deferredRecheck; + const job: GitHubJob = { + type: 'github', + source: 'github', + payload, + eventType: event.eventType, + repoFullName: (event as GitHubParsedEvent).repoFullName, + receivedAt: new Date().toISOString(), + triggerResult: isDeferredRecheck ? undefined : result, + ackCommentId: ackResult?.commentId as number | undefined, + ackMessage: ackResult?.message, + ...(isDeferredRecheck ? { mergeabilityRecheckAttempt: 1 } : {}), + }; + return job; +} +``` + +--- + +### 4. `processRouterWebhook` — deferred re-check branch + +**Tests first** (`tests/unit/router/deferred-recheck.test.ts`): + +The test file must mock `scheduleCoalescedJob` from `../../../../src/router/queue.js` and the adapter. Use the existing mock shape from `tests/unit/router/` as reference. + +- `processRouterWebhook: calls scheduleCoalescedJob when trigger returns deferredRecheck` — unit — adapter's `dispatchWithCredentials` returns `{ agentType: null, agentInput: {}, deferredRecheck: { delayMs: 45000, coalesceKey: 'proj:pr-conflict-recheck:42' } }` → `scheduleCoalescedJob` called once with `(job, 'proj:pr-conflict-recheck:42', 45000)`. Expected red: `expect(scheduleCoalescedJob).toHaveBeenCalled()` → `AssertionError: expected 0 calls, got 0`. +- `processRouterWebhook: scheduled job has no triggerResult` — unit — same setup → inspect first arg to `scheduleCoalescedJob` (the job) → `job.triggerResult === undefined`. Expected red: `expected undefined, got [object Object]`. +- `processRouterWebhook: scheduled job has mergeabilityRecheckAttempt: 1` — unit — same setup → `job.mergeabilityRecheckAttempt === 1`. Expected red: `expected 1, got undefined`. +- `processRouterWebhook: returns decisionReason containing coalesceKey` — unit — same setup → return value has `decisionReason` matching `'Deferred re-check scheduled'`. Expected red: `expected string to include 'Deferred re-check scheduled', got 'No trigger matched'`. +- `processRouterWebhook: does NOT call isWorkItemLocked when deferredRecheck` — unit — verify `isWorkItemLocked` mock not called. Expected red: `isWorkItemLocked called unexpectedly`. +- `processRouterWebhook: does NOT advance to coalesceKey path when deferredRecheck` — unit — result has both `deferredRecheck` and `coalesceKey` set; only `scheduleCoalescedJob` is called once (deferred-recheck branch, not the PM-coalesce branch). Expected red: `scheduleCoalescedJob called twice`. +- `processRouterWebhook: Sentry-captures but still returns shouldProcess:true when scheduleCoalescedJob throws` — unit — mock `scheduleCoalescedJob` to throw → return value is `{ shouldProcess: true }` AND `captureException` called with tag `deferred_recheck_schedule_failure`. Expected red: `captureException not called`. +- `processRouterWebhook: deferred re-check branch skipped when agentType is non-null` — unit — result has `agentType: 'resolve-conflicts'` AND `deferredRecheck` set → normal job dispatch path runs, NOT the deferred-recheck branch. Expected red: `scheduleCoalescedJob called unexpectedly`. + +**Implementation** (`src/router/webhook-processor.ts`): + +Add after the `skipReason` block (before the `coalesceKey` block): + +```typescript +// Step 7c: Deferred re-check — trigger asked the router to retry this event +// after a delay. Schedules a bare job (no embedded triggerResult) so the worker +// re-dispatches fresh via the trigger registry. Used for GitHub async state that +// resolves after the webhook delivery window (e.g. PR mergeability). +if (result.deferredRecheck && result.agentType === null) { + const job = adapter.buildJob(event, payload, project, result, undefined); + try { + await scheduleCoalescedJob( + job, + result.deferredRecheck.coalesceKey, + result.deferredRecheck.delayMs, + ); + logger.info(`${adapter.type} deferred re-check scheduled`, { + coalesceKey: result.deferredRecheck.coalesceKey, + delayMs: result.deferredRecheck.delayMs, + projectId: project.id, + }); + } catch (err) { + captureException(err instanceof Error ? err : new Error(String(err)), { + tags: { source: 'deferred_recheck_schedule_failure' }, + extra: { coalesceKey: result.deferredRecheck.coalesceKey, projectId: project.id }, + }); + logger.error(`Failed to schedule deferred re-check for ${adapter.type} event`, { + error: String(err), + coalesceKey: result.deferredRecheck.coalesceKey, + }); + } + return { + shouldProcess: true, + projectId: project.id, + decisionReason: `Deferred re-check scheduled: ${result.deferredRecheck.coalesceKey}`, + }; +} +``` + +--- + +### 5. CLAUDE.md documentation + +**Implementation** (`CLAUDE.md`): + +In the "Agent triggers" section, after the "Post-completion review dispatch" paragraph, add: + +``` +**Deferred re-check** — any trigger handler can return `TriggerResult.deferredRecheck: { delayMs, coalesceKey }` (with `agentType: null`) to schedule a bare delayed job that re-dispatches via the trigger registry when it fires. The router uses `scheduleCoalescedJob` for dedup. The worker detects re-check jobs via `GitHubJob.mergeabilityRecheckAttempt` and Sentry-captures under tag `mergeability_recheck_exhausted` when the trigger still cannot resolve state. Workers do not re-queue — a second `deferredRecheck` return in the worker handler exits gracefully. +``` + +--- + +## Test Plan + +### Unit tests +- [ ] `tests/unit/router/deferred-recheck.test.ts` (new): 9 tests covering type fields, `buildJob` behavior, `processRouterWebhook` branch, and error handling + +### Integration tests +- [ ] None required for this plan — the mechanism is fully exercisable via unit tests with mock adapters + +### Acceptance tests +- [ ] AC #2, #6, #7, #8 verified by unit tests above + +--- + +## Manual Verification + +*n/a — all ACs auto-tested.* + +--- + +## Acceptance Criteria (per-plan, testable) + +1. `TriggerResult` accepts `deferredRecheck?: { delayMs: number; coalesceKey: string }` without TypeScript errors. +2. `GitHubJob` accepts `mergeabilityRecheckAttempt?: number` without TypeScript errors. +3. `GitHubRouterAdapter.buildJob` omits `triggerResult` and sets `mergeabilityRecheckAttempt: 1` when `result.deferredRecheck` is set; includes `triggerResult` normally otherwise. +4. `processRouterWebhook` calls `scheduleCoalescedJob` with the job, `coalesceKey`, and `delayMs` from `result.deferredRecheck` when the trigger returns that field with `agentType: null`. +5. The scheduled job has no `triggerResult` (so the worker re-dispatches fresh). +6. When `scheduleCoalescedJob` throws, `processRouterWebhook` Sentry-captures under tag `deferred_recheck_schedule_failure` and returns `shouldProcess: true` — webhook delivery is not 500'd. +7. The deferred re-check branch is skipped when `result.agentType` is non-null (structural guard). +8. All new/modified code has corresponding unit tests. +9. `npm test` passes (unit suite). +10. `npm run typecheck` passes. +11. `npm run lint` passes. +12. CLAUDE.md updated with the `deferredRecheck` pattern note. + +--- + +## Documentation Impact (this plan only) + +| File | Change | +|---|---| +| `CLAUDE.md` | Add "Deferred re-check" paragraph to "Agent triggers" section | + +--- + +## Out of Scope (this plan) + +- `PRConflictDetectedTrigger` returning `deferredRecheck` — deferred to plan 2 +- `processGitHubWebhook` `isRecheckJob` parameter — deferred to plan 2 +- Worker-side Sentry capture on re-check exhaustion — deferred to plan 2 +- `CHANGELOG.md` entry — deferred to plan 2 + +--- + +## Progress + + +- [x] AC #1 +- [x] AC #2 +- [x] AC #3 +- [x] AC #4 +- [x] AC #5 +- [x] AC #6 +- [x] AC #7 +- [x] AC #8 +- [x] AC #9 +- [x] AC #10 +- [x] AC #11 +- [x] AC #12 diff --git a/docs/plans/020-github-mergeability-deferred-recheck/2-trigger-wiring.md.done b/docs/plans/020-github-mergeability-deferred-recheck/2-trigger-wiring.md.done new file mode 100644 index 000000000..492c55655 --- /dev/null +++ b/docs/plans/020-github-mergeability-deferred-recheck/2-trigger-wiring.md.done @@ -0,0 +1,252 @@ +--- +id: 020 +slug: github-mergeability-deferred-recheck +plan: 2 +plan_slug: trigger-wiring +level: plan +parent_spec: docs/specs/020-github-mergeability-deferred-recheck.md +depends_on: [1-infrastructure.md] +status: done +--- + +# 020/2: Conflict Trigger Wiring + Worker Re-check Detection + +> Part 2 of 2 in the 020-github-mergeability-deferred-recheck plan. See [parent spec](../../specs/020-github-mergeability-deferred-recheck.md). + +## Summary + +Wires `PRConflictDetectedTrigger` to return `deferredRecheck` instead of a structured skip when `mergeable === null` exhausts the synchronous retry budget. Adds `isRecheckJob` awareness to `processGitHubWebhook` so that when a deferred re-check job fires and the trigger **still** cannot determine mergeability, a WARN log and Sentry capture are emitted rather than silently discarding the event. + +**Components delivered:** +- `src/triggers/github/pr-conflict-detected.ts` — return `deferredRecheck` on null-after-retries path +- `src/triggers/github/webhook-handler.ts` — add `isRecheckJob?: boolean` parameter; emit WARN + Sentry when re-check trigger returns `deferredRecheck` +- `src/worker-entry.ts` — pass `!!job.mergeabilityRecheckAttempt` as `isRecheckJob` +- `tests/unit/triggers/pr-conflict-detected.test.ts` — update existing null-retry test; add deferredRecheck shape tests +- `tests/unit/triggers/github-webhook-handler-recheck.test.ts` — new file for re-check exhaustion path +- `CHANGELOG.md` — entry for the fix + +**Deferred:** nothing — this plan completes spec 020. + +--- + +## Spec ACs satisfied by this plan + +- Spec AC #1 (agent fires after deferred re-check) — **partial** (trigger now returns `deferredRecheck`; plan 1 provides the router scheduling) +- Spec AC #3 (WARN + Sentry on exhaustion) — **partial** (worker reads `mergeabilityRecheckAttempt` from plan 1; this plan adds the detection + emission) +- Spec AC #4 (no agent when `mergeable=true` after re-check) — **full** (trigger's existing `mergeable !== false` skip path is unchanged and runs normally on re-check job) +- Spec AC #5 (identical `agentInput` shape when re-check fires with conflicts) — **full** (same `TriggerResult` return path runs for the re-check dispatch) + +--- + +## Depends On + +- Plan 1 (infrastructure) — provides `TriggerResult.deferredRecheck`, `GitHubJob.mergeabilityRecheckAttempt`, and the router branch that schedules the bare job. + +--- + +## Detailed Task List (TDD) + +### 1. `PRConflictDetectedTrigger` — return `deferredRecheck` on null timeout + +**Tests first** (`tests/unit/triggers/pr-conflict-detected.test.ts` — modify existing): + +- `returns deferredRecheck (not skipReason) when mergeable is null after retries` — unit — update the EXISTING test `'returns null when mergeable is null after retries'`: change assertion from `expect(result?.skipReason?.handler).toBe('pr-conflict-detected')` to: + - `expect(result?.deferredRecheck).toBeDefined()` + - `expect(result?.skipReason).toBeUndefined()` + - `expect(result?.agentType).toBeNull()` + Expected red: `expected { delayMs: ..., coalesceKey: ... }, got undefined` (before trigger is updated). + +- `deferredRecheck coalesceKey includes projectId and prNumber` — unit — same setup (mergeable: null × 3 calls) → `result.deferredRecheck?.coalesceKey` equals `'test:pr-conflict-recheck:42'` (project id `'test'`, PR `#42`). Expected red: `expected 'test:pr-conflict-recheck:42', got undefined`. + +- `deferredRecheck delayMs is 45000` — unit — same setup → `result.deferredRecheck?.delayMs === 45000`. Expected red: `expected 45000, got undefined`. + +- `getPR called 3 times total (1 initial + 2 retries) before deferredRecheck` — unit — same null-every-call setup → `expect(githubClient.getPR).toHaveBeenCalledTimes(3)`. Expected red: this was already asserted in the existing test — verify it still passes after the behavior change. If the existing assertion count changes, update it here. + +- `retries: when first call null, second call false → dispatches resolve-conflicts (unchanged)` — unit — regression pin on the existing successful-retry test. Expected red: n/a (must stay green after trigger change). + +**Implementation** (`src/triggers/github/pr-conflict-detected.ts`): + +1. Add constant: `const MERGEABILITY_RECHECK_DELAY_MS = 45_000;` + +2. Replace the terminal null check: + ```typescript + // BEFORE: + if (prDetails.mergeable === null) { + logger.info('mergeable still null after retries, skipping ...', { prNumber }); + return skip(this.name, `mergeable still null after ... PR #${prNumber}`); + } + + // AFTER: + if (prDetails.mergeable === null) { + const coalesceKey = `${ctx.project.id}:pr-conflict-recheck:${prNumber}`; + logger.info('mergeable still null after retries, scheduling deferred re-check', { + prNumber, + coalesceKey, + delayMs: MERGEABILITY_RECHECK_DELAY_MS, + }); + return { + agentType: null, + agentInput: {}, + deferredRecheck: { + delayMs: MERGEABILITY_RECHECK_DELAY_MS, + coalesceKey, + }, + }; + } + ``` + +--- + +### 2. `processGitHubWebhook` — re-check exhaustion detection + +**Tests first** (`tests/unit/triggers/github-webhook-handler-recheck.test.ts` — new file): + +Mock `processGitHubWebhook`'s dependencies: `registry.dispatch`, `captureException` from `src/sentry.js`. + +- `emits WARN and Sentry capture when isRecheckJob=true and trigger returns deferredRecheck` — unit — mock `registry.dispatch` to return `{ agentType: null, agentInput: {}, deferredRecheck: { delayMs: 45000, coalesceKey: 'x' } }` → call `processGitHubWebhook(payload, eventType, registry, undefined, undefined, undefined, true)` → `captureException` called with error containing `'mergeability_recheck_exhausted'` in message AND options `{ tags: { source: 'mergeability_recheck_exhausted' } }`. Expected red: `captureException not called`. + +- `does NOT capture Sentry when isRecheckJob=false and trigger returns deferredRecheck` — unit — same dispatch mock, `isRecheckJob = false` → `captureException` NOT called. Expected red: n/a (regression pin). + +- `does NOT capture Sentry when isRecheckJob=true but trigger returns non-deferredRecheck result` — unit — trigger returns `{ agentType: 'resolve-conflicts', agentInput: {...} }`, `isRecheckJob = true` → `captureException` NOT called under `mergeability_recheck_exhausted` tag; agent runs normally. Expected red: n/a (regression pin). + +- `does NOT capture Sentry when triggerResult is pre-resolved (isRecheckJob=true but triggerResult passed)` — unit — `triggerResult` arg is pre-populated (no re-dispatch), `isRecheckJob = true` → `captureException` NOT called (the Sentry path only fires when re-dispatch happens AND returns `deferredRecheck`). Expected red: n/a (regression pin). + +**Implementation** (`src/triggers/github/webhook-handler.ts`): + +1. Add `captureException` import from `../../sentry.js`. + +2. Update signature: + ```typescript + export async function processGitHubWebhook( + payload: unknown, + eventType: string, + registry: TriggerRegistry, + ackCommentId?: number, + ackMessage?: string, + triggerResult?: TriggerResult, + isRecheckJob?: boolean, + ): Promise + ``` + +3. After the re-dispatch call (the `else` branch where `result = await dispatchTrigger(...)`), add: + ```typescript + if (result?.deferredRecheck && isRecheckJob) { + logger.warn('Mergeability still null after deferred re-check — giving up', { eventType }); + captureException( + new Error('mergeability_recheck_exhausted: still null after deferred re-check'), + { + tags: { source: 'mergeability_recheck_exhausted' }, + extra: { eventType }, + }, + ); + return; + } + ``` + +--- + +### 3. `worker-entry.ts` — pass `isRecheckJob` to `processGitHubWebhook` + +**Tests first** (`tests/unit/triggers/github-webhook-handler-recheck.test.ts`): + +- `worker passes isRecheckJob=true when job.mergeabilityRecheckAttempt is set` — integration-ish — directly invoke the `github` case logic from `worker-entry.ts` with a job that has `mergeabilityRecheckAttempt: 1` → the mock `processGitHubWebhook` is called with 7th arg `true`. Expected red: `expected processGitHubWebhook called with isRecheckJob=true, got isRecheckJob=undefined`. + + NOTE: Testing `worker-entry.ts` directly is awkward (it's the process entry point). If extracting the `case 'github'` into a tested helper is too invasive, this test can be written as: mock `processGitHubWebhook` and confirm its call signature when called with a `GitHubJob` that has `mergeabilityRecheckAttempt: 1`. Use the same mocking pattern that existing worker-entry tests use. + +**Implementation** (`src/worker-entry.ts`): + +In the `case 'github':` block, add the 7th argument: +```typescript +await processGitHubWebhook( + jobData.payload, + jobData.eventType, + triggerRegistry, + jobData.ackCommentId, + jobData.ackMessage, + jobData.triggerResult, + !!jobData.mergeabilityRecheckAttempt, // NEW +); +``` + +--- + +### 4. CHANGELOG entry + +**Implementation** (`CHANGELOG.md`): + +Add entry under the appropriate version heading: + +``` +- Fix: `resolve-conflicts` agent no longer silently skips when GitHub's async mergeability + computation hasn't resolved by the time the `pull_request` webhook is processed. + A durable BullMQ delayed re-check fires ~45s later to re-evaluate with fresh state. + Multiple rapid webhooks for the same PR coalesce to a single re-check job. +``` + +--- + +## Test Plan + +### Unit tests +- [ ] `tests/unit/triggers/pr-conflict-detected.test.ts` (modified): update 1 existing test; add 3 new tests for `deferredRecheck` shape +- [ ] `tests/unit/triggers/github-webhook-handler-recheck.test.ts` (new): 4 tests for re-check exhaustion detection + +### Integration tests +- [ ] None required — the observable behavior (agent fires after deferred re-check) is verified by unit tests at each boundary + +### Acceptance tests +- [ ] AC #1 end-to-end: trigger returns `deferredRecheck` (plan 2) → router schedules job (plan 1) → worker re-dispatches → trigger gets `mergeable=false` → `resolve-conflicts` agent fires. Covered by the combination of plan-1 router tests and plan-2 trigger tests. + +--- + +## Manual Verification + +*n/a — all ACs auto-tested.* + +--- + +## Acceptance Criteria (per-plan, testable) + +1. `PRConflictDetectedTrigger.handle()` returns `{ agentType: null, deferredRecheck: { delayMs: 45000, coalesceKey: '${projectId}:pr-conflict-recheck:${prNumber}' } }` when `mergeable` is still `null` after the synchronous retry loop. +2. `PRConflictDetectedTrigger.handle()` does NOT set `skipReason` on the null-timeout path — the router deferred-recheck branch fires instead of the skip branch. +3. When a deferred re-check job fires and `processGitHubWebhook` re-dispatches to get `deferredRecheck` again (mergeability still null), `captureException` is called with tag `source: 'mergeability_recheck_exhausted'` and `processGitHubWebhook` returns without running an agent. +4. When a deferred re-check job fires and `processGitHubWebhook` re-dispatches to get `agentType: 'resolve-conflicts'` (mergeability resolved to `false`), the agent runs normally with the same `agentInput` shape as the synchronous path. +5. When a deferred re-check job fires and `processGitHubWebhook` re-dispatches to get `mergeable=true` (conflict resolved), no agent runs and no Sentry capture fires. +6. `worker-entry.ts` passes `isRecheckJob=true` to `processGitHubWebhook` when the job has `mergeabilityRecheckAttempt > 0`. +7. All new/modified code has corresponding unit tests. +8. `npm test` passes (unit suite). +9. `npm run typecheck` passes. +10. `npm run lint` passes. +11. `CHANGELOG.md` updated with the fix entry. + +--- + +## Documentation Impact (this plan only) + +| File | Change | +|---|---| +| `CHANGELOG.md` | Add entry for the silent-skip-forever fix | + +--- + +## Out of Scope (this plan) + +- Generic deferred re-check infrastructure (`TriggerResult.deferredRecheck`, router branch) — delivered by plan 1 +- Additional trigger handlers using `deferredRecheck` — out of scope for spec 020 + +--- + +## Progress + + +- [x] AC #1 +- [x] AC #2 +- [x] AC #3 +- [x] AC #4 +- [x] AC #5 +- [x] AC #6 +- [x] AC #7 +- [x] AC #8 +- [x] AC #9 +- [x] AC #10 +- [x] AC #11 diff --git a/docs/plans/020-github-mergeability-deferred-recheck/_coverage.md b/docs/plans/020-github-mergeability-deferred-recheck/_coverage.md new file mode 100644 index 000000000..906f59097 --- /dev/null +++ b/docs/plans/020-github-mergeability-deferred-recheck/_coverage.md @@ -0,0 +1,28 @@ +# Coverage map for spec 020-github-mergeability-deferred-recheck + +Auto-generated by /plan. Tracks which plans satisfy which spec ACs. + +## Spec ACs + +| # | Spec AC (short) | Satisfied by | Status | +|---|---|---|---| +| 1 | `resolve-conflicts` fires when GitHub mergeability resolves within ~60s | plan 1 (infrastructure) + plan 2 (trigger-wiring) | partial chain | +| 2 | Multiple rapid webhooks coalesce to single re-check job | plan 1 (infrastructure) | full | +| 3 | WARN + Sentry with tag `mergeability_recheck_exhausted` when still null after re-check | plan 1 (infrastructure) + plan 2 (trigger-wiring) | partial chain | +| 4 | No agent dispatched when `mergeable=true` after re-check | plan 2 (trigger-wiring) | full | +| 5 | `resolve-conflicts` `agentInput` shape identical to synchronous path | plan 2 (trigger-wiring) | full | +| 6 | Router HTTP handler returns within 10s | plan 1 (infrastructure) | full | +| 7 | Existing synchronous fast path (<4s) unchanged | plan 1 (infrastructure) | full | +| 8 | Generic `TriggerResult.deferredRecheck` field usable by any trigger | plan 1 (infrastructure) | full | + +## Coverage summary + +- **8 spec ACs** mapped to **2 plans** +- **6 ACs** with full coverage (ACs 2, 4, 5, 6, 7, 8) +- **2 ACs** with partial-chain coverage requiring both plans (ACs 1 and 3) + +## Plan dependency graph + +``` +1-infrastructure ──→ 2-trigger-wiring +``` diff --git a/docs/specs/020-github-mergeability-deferred-recheck.md.done b/docs/specs/020-github-mergeability-deferred-recheck.md.done new file mode 100644 index 000000000..afddf9698 --- /dev/null +++ b/docs/specs/020-github-mergeability-deferred-recheck.md.done @@ -0,0 +1,121 @@ +--- +id: 020 +slug: github-mergeability-deferred-recheck +level: spec +title: GitHub Async Mergeability — Durable Deferred Re-check +created: 2026-05-08 +status: done +--- + +# 020: GitHub Async Mergeability — Durable Deferred Re-check + +## Problem & Motivation + +GitHub computes pull request mergeability asynchronously. When a `pull_request.opened`, `reopened`, or `synchronize` webhook fires, the `mergeable` field on the PR object is frequently `null` — GitHub has started the background merge-commit computation but not yet finished. The computation typically resolves in 10–30 seconds; for freshly-opened PRs it can take longer. Critically, **GitHub does not send a follow-up webhook when mergeability resolves** — there is no `mergeable_resolved` event or a re-fire of `pull_request.opened`. + +`PRConflictDetectedTrigger` currently handles this with a synchronous retry loop: 2 retries × 2 s = 4 s total. When the budget exhausts while `mergeable` is still `null`, the trigger returns a structured skip and the router discards the event. Because no follow-up webhook ever arrives, the `resolve-conflicts` agent never fires. The PR sits in a conflicting state indefinitely, requiring manual intervention. + +This was observed live on ucho/PR #329 (2026-05-07): the implementation bot opened the PR already conflicting against the base branch, the trigger fired immediately, GitHub had not yet resolved mergeability, the 4 s budget exhausted, and `resolve-conflicts` was never dispatched. + +--- + +## Goals + +- The `resolve-conflicts` agent fires whenever a cascade-bot PR is genuinely conflicting, even when GitHub's mergeability computation takes 10–60 s. +- Multiple rapid webhooks for the same PR (e.g. `opened` quickly followed by `synchronize`) coalesce to a single deferred re-check job, not N separate jobs. +- Silent skip-forever is replaced by observable failure: if mergeability is still `null` after the deferred re-check, the event is captured in Sentry and logged at WARN — not silently discarded. +- The deferred re-check mechanism is generic enough for other trigger handlers to use in the future if they encounter similar async-state problems. + +--- + +## Non-goals + +- Fixing mergeability handling for human-authored PRs (the trigger already gates on cascade-bot persona). +- Polling GitHub continuously until mergeability resolves. One deferred re-check is sufficient; GitHub resolves within 60 s in all observed cases. +- Changing how `resolve-conflicts` runs or how the agent itself works. +- Supporting multiple recursive re-checks from the worker side (workers do not enqueue). + +--- + +## Constraints + +- Router HTTP handlers must respond within 10 s (GitHub's webhook delivery timeout). The synchronous polling budget must stay ≤ 6 s total to leave margin. +- Workers do not enqueue BullMQ jobs. The deferred re-check must be scheduled at router time, not worker time. +- The `TriggerResult` interface is shared across all trigger handlers and all adapter types. Additions must be opt-in and backward-compatible. +- The existing `scheduleCoalescedJob` infrastructure is the designated mechanism for delayed dedup jobs — no new queue helpers. + +--- + +## User stories / Requirements + +1. As an operator, when a cascade bot opens or pushes to a PR that is conflicting, I expect `resolve-conflicts` to fire — even if the conflict isn't detectable within 4 s of the webhook. +2. As an operator, when the deferred re-check fires and the PR is no longer conflicting (e.g. someone merged base in the meantime), no agent is dispatched — the normal "mergeable=true → skip" path runs. +3. As an operator, when the deferred re-check fires and mergeability is still `null` (pathological GitHub latency), I see a WARN log and a Sentry capture so I can investigate — not a silent discard. +4. As a developer adding a new trigger handler, I can return `deferredRecheck` from `TriggerResult` to defer event processing without adding custom infrastructure. + +--- + +## Research Notes + +- [GitHub REST docs — pull request mergeable](https://docs.github.com/en/rest/pulls/pulls): "The value of the `mergeable` attribute can be `true`, `false`, or `null`. If the value is `null`, then GitHub has started a background job to compute the mergeability. After giving the job time to complete, resubmit the request." +- [octokit.net issue #1710](https://github.com/octokit/octokit.net/issues/1710) and [PyGithub issue #256](https://github.com/PyGithub/PyGithub/issues/256): consistent community reports that `null` is common on first request; retry after ~5 s works in most cases. +- [Gitea issue #19755](https://github.com/go-gitea/gitea/issues/19755): "Web hook does not wait for completion of 'mergeable' check" — same root cause, same class of failure. +- Industry tools (Atlantis, automerge-action) use 5–10 s intervals × 5–10 retries for synchronous polling. This is consistent with "almost always resolved within 60 s." +- Cascade already has `scheduleCoalescedJob` (BullMQ delayed job with per-key dedup, backed by Redis). This is the canonical mechanism for deferred processing. +- The GitHub webhook worker handler already re-dispatches via the trigger registry when a GitHub job has no embedded trigger result. This is the hook for fresh re-evaluation at deferred job fire time. + +--- + +## Open Source Decisions + +| Tool | Solves | Decision | Reason | +|------|--------|----------|--------| +| BullMQ delayed jobs (already in use) | Schedule deferred re-check with dedup | Use (existing) | Already the designated delayed-job mechanism; `scheduleCoalescedJob` provides dedup semantics | + +No new OSS dependencies required. + +--- + +## Strategic decisions + +1. **Durable deferred re-check over larger synchronous budget** — chosen over increasing the in-handler poll loop. Synchronous polling at 30 s would risk exceeding GitHub's 10 s webhook delivery timeout. The deferred path has no ceiling on wait time. + +2. **Single deferred re-check, not recursive multi-attempt** — GitHub virtually never takes >60 s. Recursive re-checks would require threading attempt counts through the worker layer, which would violate the "workers do not enqueue" architectural invariant. One deferred re-check + graceful Sentry capture on second failure is sufficient. + +3. **`TriggerResult.deferredRecheck` as a generic field** — the field lives on `TriggerResult`, not in GitHub-specific types. The router's `processRouterWebhook` handles it generically so any trigger handler in any adapter can use it in the future without touching shared infrastructure again. + +4. **Keep current synchronous budget (2 × 2 s)** — fast resolves (<10 s) are still handled inline; the deferred path activates only for the slow tail. Reducing the synchronous budget would increase deferred-path frequency for no benefit. + +5. **Workers do not enqueue** — the deferred re-check job is scheduled at router time. If the re-check trigger still returns `deferredRecheck` (mergeability still null after 45 s), `processGitHubWebhook` sees `agentType: null` and exits. Sentry capture + WARN log in the trigger signals permanent failure. + +6. **`buildJob` omits `triggerResult` for deferred re-checks** — when `result.deferredRecheck` is set, `GitHubRouterAdapter.buildJob` produces a bare `GitHubJob` without an embedded `triggerResult`. This ensures the worker calls `dispatchTrigger → registry.dispatch` fresh, getting current mergeability state. + +--- + +## Acceptance Criteria (outcome-level) + +1. When a cascade-bot PR is opened or force-pushed and GitHub resolves `mergeable=false` within ~60 s, the `resolve-conflicts` agent fires — even if mergeability was `null` at webhook receipt time. +2. Multiple rapid `pull_request` webhooks for the same PR (opened + synchronize within the deferred window) coalesce to a single re-check job; the `resolve-conflicts` agent fires at most once per conflict episode. +3. When mergeability is still `null` after the deferred re-check fires, a WARN-level log is emitted and a Sentry event is captured with tag `mergeability_recheck_exhausted`; no agent is dispatched. +4. When the deferred re-check fires and `mergeable=true` (conflict resolved before the re-check), no agent is dispatched. +5. When the deferred re-check fires and `mergeable=false`, the `resolve-conflicts` agent runs identically to the synchronous path — same `agentInput` shape, same PR comment, same work-item linking. +6. The router HTTP handler returns within 10 s for all `pull_request` events, including those that trigger a deferred re-check. +7. The existing synchronous fast path (mergeable resolves within 4 s) is unchanged. +8. A new trigger handler can use `TriggerResult.deferredRecheck` to schedule a deferred bare-job re-dispatch without modifying shared router or worker infrastructure. + +--- + +## Documentation Impact (high-level) + +- `CLAUDE.md` — add a note to the "Agent triggers" section documenting `TriggerResult.deferredRecheck` semantics, analogous to the existing "Post-completion review dispatch" note. Explains the bare-job/re-dispatch pattern so future handlers know it exists. +- `CHANGELOG.md` — entry for the silent-skip-forever fix. + +--- + +## Out of Scope + +- Fixing mergeability handling for human-authored PRs. +- GitHub mergeability state for non-PR webhook types (check_suite, etc. — they don't gate on `mergeable`). +- Adding a second or third deferred re-check (one is sufficient; see strategic decision 2). +- Any change to how `resolve-conflicts` agent runs after dispatch. +- Deferred re-check mechanism for non-GitHub trigger adapters (future — the generic `TriggerResult` field makes it possible, but no other trigger currently needs it). diff --git a/src/pm/integration.ts b/src/pm/integration.ts index 8f915cbc7..947756a19 100644 --- a/src/pm/integration.ts +++ b/src/pm/integration.ts @@ -12,7 +12,7 @@ */ import type { IntegrationModule } from '../integrations/types.js'; -import type { AgentExecutionConfig } from '../triggers/shared/agent-execution.js'; +import type { AgentExecutionConfig } from '../triggers/shared/agent-execution-types.js'; import type { CascadeConfig, ProjectConfig } from '../types/index.js'; import type { ProjectPMConfig } from './lifecycle.js'; import type { PMProvider } from './types.js'; diff --git a/src/router/adapters/github.ts b/src/router/adapters/github.ts index c127318d3..9b352c999 100644 --- a/src/router/adapters/github.ts +++ b/src/router/adapters/github.ts @@ -373,6 +373,7 @@ export class GitHubRouterAdapter implements RouterPlatformAdapter { result: TriggerResult, ackResult?: AckResult, ): CascadeJob { + const isDeferredRecheck = !!result.deferredRecheck; const job: GitHubJob = { type: 'github', source: 'github', @@ -380,9 +381,10 @@ export class GitHubRouterAdapter implements RouterPlatformAdapter { eventType: event.eventType, repoFullName: (event as GitHubParsedEvent).repoFullName, receivedAt: new Date().toISOString(), - triggerResult: result, + triggerResult: isDeferredRecheck ? undefined : result, ackCommentId: ackResult?.commentId as number | undefined, ackMessage: ackResult?.message, + ...(isDeferredRecheck ? { mergeabilityRecheckAttempt: 1 } : {}), }; return job; } diff --git a/src/router/queue.ts b/src/router/queue.ts index 36c8ba752..69d9c3bb7 100644 --- a/src/router/queue.ts +++ b/src/router/queue.ts @@ -42,6 +42,13 @@ export interface GitHubJob { ackCommentId?: number; ackMessage?: string; triggerResult?: TriggerResult; + /** + * Set to 1 when this job is a deferred mergeability re-check (router side). + * The worker uses `!!job.mergeabilityRecheckAttempt` to detect re-check + * jobs and emit Sentry capture when the trigger still cannot determine + * mergeability. + */ + mergeabilityRecheckAttempt?: number; } export interface JiraJob { diff --git a/src/router/webhook-processor.ts b/src/router/webhook-processor.ts index 83c88f564..5041c70cd 100644 --- a/src/router/webhook-processor.ts +++ b/src/router/webhook-processor.ts @@ -174,6 +174,40 @@ export async function processRouterWebhook( }; } + // Step 7c: Deferred re-check — trigger asked the router to retry this event + // after a delay. Schedules a bare job (no embedded triggerResult) so the worker + // re-dispatches fresh via the trigger registry. Used for GitHub async state that + // resolves after the webhook delivery window (e.g. PR mergeability). + if (result.deferredRecheck && result.agentType === null) { + const job = adapter.buildJob(event, payload, project, result, undefined); + try { + await scheduleCoalescedJob( + job, + result.deferredRecheck.coalesceKey, + result.deferredRecheck.delayMs, + ); + logger.info(`${adapter.type} deferred re-check scheduled`, { + coalesceKey: result.deferredRecheck.coalesceKey, + delayMs: result.deferredRecheck.delayMs, + projectId: project.id, + }); + } catch (err) { + captureException(err instanceof Error ? err : new Error(String(err)), { + tags: { source: 'deferred_recheck_schedule_failure' }, + extra: { coalesceKey: result.deferredRecheck.coalesceKey, projectId: project.id }, + }); + logger.error(`Failed to schedule deferred re-check for ${adapter.type} event`, { + error: String(err), + coalesceKey: result.deferredRecheck.coalesceKey, + }); + } + return { + shouldProcess: true, + projectId: project.id, + decisionReason: `Deferred re-check scheduled: ${result.deferredRecheck.coalesceKey}`, + }; + } + logger.info(`${adapter.type} trigger matched`, { agentType: result.agentType || '(no agent)', workItemId: event.workItemId, diff --git a/src/triggers/github/integration.ts b/src/triggers/github/integration.ts index e9f6c8992..b178aef45 100644 --- a/src/triggers/github/integration.ts +++ b/src/triggers/github/integration.ts @@ -16,7 +16,7 @@ import type { PMIntegration, PMWebhookEvent } from '../../pm/integration.js'; import type { ProjectPMConfig } from '../../pm/lifecycle.js'; import type { PMProvider } from '../../pm/types.js'; import type { CascadeConfig, ProjectConfig } from '../../types/index.js'; -import type { AgentExecutionConfig } from '../shared/agent-execution.js'; +import type { AgentExecutionConfig } from '../shared/agent-execution-types.js'; import { deleteProgressCommentOnSuccess, updateInitialCommentWithError } from './ack-comments.js'; export class GitHubWebhookIntegration implements PMIntegration { diff --git a/src/triggers/github/pr-conflict-detected.ts b/src/triggers/github/pr-conflict-detected.ts index 64a593c82..745cade36 100644 --- a/src/triggers/github/pr-conflict-detected.ts +++ b/src/triggers/github/pr-conflict-detected.ts @@ -18,6 +18,7 @@ const conflictAttempts = new Map(); const MAX_ATTEMPTS = 2; const MERGEABLE_RETRY_COUNT = 2; const MERGEABLE_RETRY_DELAY_MS = 2000; +const MERGEABILITY_RECHECK_DELAY_MS = 45_000; // Export for cleanup when conflicts are resolved export function resetConflictAttempts(prNumber: number): void { @@ -103,15 +104,22 @@ export class PRConflictDetectedTrigger implements TriggerHandler { } } - // If still null after retries, skip — we can't determine mergeability + // If still null after retries, schedule a deferred re-check ~45s later if (prDetails.mergeable === null) { - logger.info('mergeable still null after retries, skipping conflict detection trigger', { + const coalesceKey = `${ctx.project.id}:pr-conflict-recheck:${prNumber}`; + logger.info('mergeable still null after retries, scheduling deferred re-check', { prNumber, + coalesceKey, + delayMs: MERGEABILITY_RECHECK_DELAY_MS, }); - return skip( - this.name, - `mergeable still null after ${MERGEABLE_RETRY_COUNT} retries for PR #${prNumber} — cannot determine mergeability`, - ); + return { + agentType: null, + agentInput: {}, + deferredRecheck: { + delayMs: MERGEABILITY_RECHECK_DELAY_MS, + coalesceKey, + }, + }; } // Only fire if PR is unmergeable (has conflicts) diff --git a/src/triggers/github/webhook-handler.ts b/src/triggers/github/webhook-handler.ts index 2492ce5d3..fd87a2b1a 100644 --- a/src/triggers/github/webhook-handler.ts +++ b/src/triggers/github/webhook-handler.ts @@ -15,6 +15,7 @@ import { isPMFocusedAgent } from '../../agents/definitions/loader.js'; import { withGitHubToken } from '../../github/client.js'; import { getPersonaToken, resolvePersonaIdentities } from '../../github/personas.js'; import { extractGitHubContext, generateAckMessage } from '../../router/ackMessageGenerator.js'; +import { captureException } from '../../sentry.js'; import type { CascadeConfig, ProjectConfig, TriggerContext } from '../../types/index.js'; import { logger, startWatchdog } from '../../utils/index.js'; import type { TriggerRegistry } from '../registry.js'; @@ -199,6 +200,7 @@ export async function processGitHubWebhook( ackCommentId?: number, ackMessage?: string, triggerResult?: TriggerResult, + isRecheckJob?: boolean, ): Promise { logger.info('Processing GitHub webhook', { eventType, hasTriggerResult: !!triggerResult }); @@ -226,6 +228,17 @@ export async function processGitHubWebhook( result = triggerResult; } else { result = await dispatchTrigger(registry, payload, project); + if (result?.deferredRecheck && isRecheckJob) { + logger.warn('Mergeability still null after deferred re-check — giving up', { eventType }); + captureException( + new Error('mergeability_recheck_exhausted: still null after deferred re-check'), + { + tags: { source: 'mergeability_recheck_exhausted' }, + extra: { eventType }, + }, + ); + return; + } } if (!result) { diff --git a/src/triggers/shared/agent-execution-types.ts b/src/triggers/shared/agent-execution-types.ts new file mode 100644 index 000000000..2ef7b1d40 --- /dev/null +++ b/src/triggers/shared/agent-execution-types.ts @@ -0,0 +1,63 @@ +import type { LifecycleHooks } from '../../agents/definitions/schema.js'; +import type { PMLifecycleManager } from '../../pm/index.js'; +import type { AgentInput, AgentResult, CascadeConfig, ProjectConfig } from '../../types/index.js'; +import type { TriggerResult } from '../types.js'; + +/** + * Configuration for source-specific behavior in the agent execution pipeline. + */ +export interface AgentExecutionConfig { + /** + * Whether to skip calling lifecycle.prepareForAgent before running the agent. + * GitHub handlers skip this step; Trello and JIRA handlers call it. + */ + skipPrepareForAgent?: boolean; + + /** + * Whether to skip calling lifecycle.handleFailure on agent failure. + * GitHub handlers only call handleSuccess for the 'implementation' agent type, + * so they skip handleFailure entirely. + */ + skipHandleFailure?: boolean; + + /** + * Whether to only call lifecycle.handleSuccess for a specific agent type. + * If set, handleSuccess is only called when agentType matches this value. + * GitHub uses this to only call handleSuccess for 'implementation'. + */ + handleSuccessOnlyForAgentType?: string; + + /** + * Optional callback invoked when the agent succeeds (after pipeline completes). + * Used by GitHub to delete the progress comment for non-implementation agents. + */ + onSuccess?: (result: TriggerResult, agentResult: AgentResult) => Promise; + + /** + * Optional callback invoked when the agent fails (after pipeline completes). + * Used by GitHub to update the PR comment with an error message. + */ + onFailure?: (result: TriggerResult, agentResult: AgentResult) => Promise; + + /** + * Log label used in log messages (e.g. 'GitHub', 'JIRA', 'Trello'). + */ + logLabel?: string; +} + +/** + * Internal context assembled by runAgentExecutionPipeline once source-specific + * configuration, lifecycle dependencies, and live work-item state are resolved. + */ +export interface AgentExecutionContext { + result: TriggerResult; + project: ProjectConfig; + config: CascadeConfig; + executionConfig: AgentExecutionConfig; + agentType: string; + logLabel: string; + lifecycle: PMLifecycleManager; + lifecycleHooks: LifecycleHooks; + workItemId: string | undefined; + agentInput: AgentInput; +} diff --git a/src/triggers/shared/agent-execution.ts b/src/triggers/shared/agent-execution.ts index 3d67d78ca..1f09a7d89 100644 --- a/src/triggers/shared/agent-execution.ts +++ b/src/triggers/shared/agent-execution.ts @@ -20,6 +20,7 @@ import { logger } from '../../utils/logging.js'; import { extractPRNumber } from '../../utils/prUrl.js'; import { parseRepoFullName } from '../../utils/repo.js'; import type { TriggerResult } from '../types.js'; +import type { AgentExecutionConfig, AgentExecutionContext } from './agent-execution-types.js'; import { handleAgentResultArtifacts } from './agent-result-handler.js'; import { isPipelineAtCapacity } from './backlog-check.js'; import { checkBudgetExceeded } from './budget.js'; @@ -31,47 +32,7 @@ import { validateIntegrations, } from './integration-validation.js'; -/** - * Configuration for source-specific behavior in the agent execution pipeline. - */ -export interface AgentExecutionConfig { - /** - * Whether to skip calling lifecycle.prepareForAgent before running the agent. - * GitHub handlers skip this step; Trello and JIRA handlers call it. - */ - skipPrepareForAgent?: boolean; - - /** - * Whether to skip calling lifecycle.handleFailure on agent failure. - * GitHub handlers only call handleSuccess for the 'implementation' agent type, - * so they skip handleFailure entirely. - */ - skipHandleFailure?: boolean; - - /** - * Whether to only call lifecycle.handleSuccess for a specific agent type. - * If set, handleSuccess is only called when agentType matches this value. - * GitHub uses this to only call handleSuccess for 'implementation'. - */ - handleSuccessOnlyForAgentType?: string; - - /** - * Optional callback invoked when the agent succeeds (after pipeline completes). - * Used by GitHub to delete the progress comment for non-implementation agents. - */ - onSuccess?: (result: TriggerResult, agentResult: AgentResult) => Promise; - - /** - * Optional callback invoked when the agent fails (after pipeline completes). - * Used by GitHub to update the PR comment with an error message. - */ - onFailure?: (result: TriggerResult, agentResult: AgentResult) => Promise; - - /** - * Log label used in log messages (e.g. 'GitHub', 'JIRA', 'Trello'). - */ - logLabel?: string; -} +export type { AgentExecutionConfig } from './agent-execution-types.js'; /** * Check the budget before running an agent. @@ -515,9 +476,26 @@ export async function runAgentExecutionPipeline( ? { ...result.agentInput, workItemId } : result.agentInput; + const executionContext: AgentExecutionContext = { + result, + project, + config, + executionConfig, + agentType, + logLabel, + lifecycle, + lifecycleHooks, + workItemId, + agentInput, + }; + let remainingBudgetUsd: number | undefined; - if (workItemId) { - const budgetResult = await checkPreRunBudget(workItemId, project, lifecycle); + if (executionContext.workItemId) { + const budgetResult = await checkPreRunBudget( + executionContext.workItemId, + executionContext.project, + executionContext.lifecycle, + ); if (budgetResult.abort) return; remainingBudgetUsd = budgetResult.remainingBudgetUsd; } @@ -525,16 +503,16 @@ export async function runAgentExecutionPipeline( // Insert a work-item-only row so PM-triggered runs show up in the dashboard // even before a PR is created. This is idempotent — if a row already exists // it is updated with the latest display fields. - if (workItemId) { + if (executionContext.workItemId) { try { - await createWorkItem(project.id, workItemId, { - workItemUrl: result.workItemUrl, - workItemTitle: result.workItemTitle, + await createWorkItem(executionContext.project.id, executionContext.workItemId, { + workItemUrl: executionContext.result.workItemUrl, + workItemTitle: executionContext.result.workItemTitle, }); } catch (err) { logger.warn('Failed to persist work-item row for PM-triggered run', { - projectId: project.id, - workItemId, + projectId: executionContext.project.id, + workItemId: executionContext.workItemId, error: String(err), }); } @@ -561,15 +539,18 @@ export async function runAgentExecutionPipeline( } } - if (workItemId && !skipPrepareForAgent) { - await lifecycle.prepareForAgent(workItemId, lifecycleHooks); + if (executionContext.workItemId && !skipPrepareForAgent) { + await executionContext.lifecycle.prepareForAgent( + executionContext.workItemId, + executionContext.lifecycleHooks, + ); } - const agentResult = await runAgent(agentType, { - ...agentInput, + const agentResult = await runAgent(executionContext.agentType, { + ...executionContext.agentInput, remainingBudgetUsd, - project, - config, + project: executionContext.project, + config: executionContext.config, }); // Link PR to work item post-execution (single code path for all backends) diff --git a/src/triggers/shared/webhook-execution.ts b/src/triggers/shared/webhook-execution.ts index 7724cc819..e8229e373 100644 --- a/src/triggers/shared/webhook-execution.ts +++ b/src/triggers/shared/webhook-execution.ts @@ -19,8 +19,8 @@ import type { PMIntegration } from '../../pm/integration.js'; import type { CascadeConfig, ProjectConfig } from '../../types/index.js'; import { injectLlmApiKeys } from '../../utils/llmEnv.js'; import type { TriggerResult } from '../types.js'; -import type { AgentExecutionConfig } from './agent-execution.js'; import { runAgentExecutionPipeline } from './agent-execution.js'; +import type { AgentExecutionConfig } from './agent-execution-types.js'; /** * Run the agent execution pipeline inside the full credential scope. diff --git a/src/types/index.ts b/src/types/index.ts index fdc894cda..b3e0ddcdf 100644 --- a/src/types/index.ts +++ b/src/types/index.ts @@ -160,6 +160,17 @@ export interface TriggerResult { /** Human-readable explanation. Used verbatim in webhook log decisionReason. */ message: string; }; + /** + * When set and `agentType` is `null`, the router schedules a bare delayed + * job (no embedded trigger result) keyed by `coalesceKey`. The worker + * re-dispatches via the trigger registry when the job fires, obtaining fresh + * state. Any trigger handler can use this field; the router branch in + * `processRouterWebhook` is adapter-agnostic. + */ + deferredRecheck?: { + delayMs: number; + coalesceKey: string; + }; } export interface TriggerHandler { diff --git a/src/worker-entry.ts b/src/worker-entry.ts index 353c41d7b..d8ffece40 100644 --- a/src/worker-entry.ts +++ b/src/worker-entry.ts @@ -76,6 +76,7 @@ export interface GitHubJobData { ackCommentId?: number; ackMessage?: string; triggerResult?: TriggerResult; + mergeabilityRecheckAttempt?: number; } export interface JiraJobData { @@ -326,6 +327,7 @@ export async function dispatchJob( jobData.ackCommentId, jobData.ackMessage, jobData.triggerResult, + !!jobData.mergeabilityRecheckAttempt, ); break; case 'jira': { diff --git a/tests/unit/router/deferred-recheck.test.ts b/tests/unit/router/deferred-recheck.test.ts new file mode 100644 index 000000000..cb68645b8 --- /dev/null +++ b/tests/unit/router/deferred-recheck.test.ts @@ -0,0 +1,313 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +// ── shared mocks ────────────────────────────────────────────────────────────── +vi.mock('../../../src/utils/logging.js', () => ({ + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, +})); +vi.mock('../../../src/router/queue.js', () => ({ + addJob: vi.fn(), + scheduleCoalescedJob: vi.fn().mockResolvedValue({ jobId: 'cj-1', superseded: false }), +})); +vi.mock('../../../src/pm/coalesce-config.js', () => ({ + getCoalesceWindowMs: vi.fn().mockReturnValue(10_000), +})); +vi.mock('../../../src/router/work-item-lock.js', () => ({ + isWorkItemLocked: vi.fn().mockResolvedValue({ locked: false }), + markWorkItemEnqueued: vi.fn(), + clearWorkItemEnqueued: vi.fn(), +})); +vi.mock('../../../src/router/agent-type-lock.js', () => ({ + checkAgentTypeConcurrency: vi.fn().mockResolvedValue({ maxConcurrency: null, blocked: false }), + markAgentTypeEnqueued: vi.fn(), + markRecentlyDispatched: vi.fn(), + clearAgentTypeEnqueued: vi.fn(), + clearRecentlyDispatched: vi.fn(), +})); +vi.mock('../../../src/router/action-dedup.js', () => ({ + isDuplicateAction: vi.fn().mockReturnValue(false), + markActionProcessed: vi.fn(), +})); +vi.mock('../../../src/router/lock-state-classifier.js', () => ({ + classifyLockState: vi.fn().mockResolvedValue('awaiting-slot'), +})); +vi.mock('../../../src/sentry.js', () => ({ + captureException: vi.fn(), +})); + +// ── github adapter mocks (for buildJob tests) ───────────────────────────────── +vi.mock('../../../src/router/config.js', () => ({ + loadProjectConfig: vi.fn(), +})); +vi.mock('../../../src/config/projects.js', () => ({ + getProjectGitHubToken: vi.fn().mockResolvedValue('ghp_mock'), +})); +vi.mock('../../../src/config/provider.js', () => ({ + findProjectByRepo: vi.fn().mockResolvedValue(null), +})); +vi.mock('../../../src/github/personas.js', () => ({ + resolvePersonaIdentities: vi.fn().mockResolvedValue({}), + isCascadeBot: vi.fn().mockReturnValue(false), +})); +vi.mock('../../../src/github/client.js', () => ({ + withGitHubToken: vi.fn().mockImplementation((_t: unknown, fn: () => unknown) => fn()), +})); +vi.mock('../../../src/pm/context.js', () => ({ + withPMProvider: vi.fn().mockImplementation((_p: unknown, fn: () => unknown) => fn()), + withPMCredentials: vi + .fn() + .mockImplementation((_id: unknown, _type: unknown, _get: unknown, fn: () => unknown) => fn()), +})); +vi.mock('../../../src/pm/registry.js', () => ({ + pmRegistry: { + getOrNull: vi.fn().mockReturnValue(null), + createProvider: vi.fn().mockReturnValue({}), + }, +})); +vi.mock('../../../src/router/acknowledgments.js', () => ({ + postGitHubAck: vi.fn(), + resolveGitHubTokenForAckByAgent: vi.fn(), +})); +vi.mock('../../../src/router/pm-ack-dispatch.js', () => ({ dispatchPMAck: vi.fn() })); +vi.mock('../../../src/agents/definitions/loader.js', () => ({ + isPMFocusedAgent: vi.fn().mockResolvedValue(false), +})); +vi.mock('../../../src/router/notifications.js', () => ({ extractPRNumber: vi.fn() })); +vi.mock('../../../src/router/pre-actions.js', () => ({ addEyesReactionToPR: vi.fn() })); +vi.mock('../../../src/router/ackMessageGenerator.js', () => ({ + extractGitHubContext: vi.fn().mockReturnValue('ctx'), + generateAckMessage: vi.fn().mockResolvedValue('ack'), +})); +vi.mock('../../../src/router/reactions.js', () => ({ + sendAcknowledgeReaction: vi.fn().mockResolvedValue(undefined), +})); +vi.mock('../../../src/utils/runLink.js', () => ({ + buildWorkItemRunsLink: vi.fn().mockReturnValue(''), + getDashboardUrl: vi.fn().mockReturnValue(''), +})); + +// ── imports ─────────────────────────────────────────────────────────────────── +import { GitHubRouterAdapter } from '../../../src/router/adapters/github.js'; +import type { RouterProjectConfig } from '../../../src/router/config.js'; +import { loadProjectConfig } from '../../../src/router/config.js'; +import type { RouterPlatformAdapter } from '../../../src/router/platform-adapter.js'; +import type { GitHubJob } from '../../../src/router/queue.js'; +import { scheduleCoalescedJob } from '../../../src/router/queue.js'; +import { processRouterWebhook } from '../../../src/router/webhook-processor.js'; +import { isWorkItemLocked } from '../../../src/router/work-item-lock.js'; +import { captureException } from '../../../src/sentry.js'; +import type { TriggerRegistry } from '../../../src/triggers/registry.js'; +import type { TriggerResult } from '../../../src/types/index.js'; + +// ── fixtures ────────────────────────────────────────────────────────────────── + +const mockProject: RouterProjectConfig = { id: 'p1', repo: 'owner/repo', pmType: 'trello' }; + +const mockTriggerRegistry = { + dispatch: vi.fn().mockResolvedValue(null), +} as unknown as TriggerRegistry; + +const deferredRecheckResult: TriggerResult = { + agentType: null, + agentInput: {}, + deferredRecheck: { delayMs: 45_000, coalesceKey: 'p1:pr-conflict-recheck:42' }, +}; + +const deferredBareJob: GitHubJob = { + type: 'github', + source: 'github', + payload: {}, + eventType: 'pull_request', + repoFullName: 'owner/repo', + receivedAt: new Date().toISOString(), + // NOTE: triggerResult intentionally absent — this is the "bare" re-check job + mergeabilityRecheckAttempt: 1, +}; + +function makeMockAdapter(overrides: Partial = {}): RouterPlatformAdapter { + return { + type: 'github', + parseWebhook: vi.fn().mockResolvedValue({ + projectIdentifier: 'owner/repo', + eventType: 'pull_request', + workItemId: '42', + isCommentEvent: false, + }), + isProcessableEvent: vi.fn().mockReturnValue(true), + isSelfAuthored: vi.fn().mockResolvedValue(false), + sendReaction: vi.fn(), + resolveProject: vi.fn().mockResolvedValue(mockProject), + dispatchWithCredentials: vi.fn().mockResolvedValue(null), + postAck: vi.fn().mockResolvedValue(undefined), + buildJob: vi.fn().mockReturnValue(deferredBareJob), + firePreActions: vi.fn(), + ...overrides, + }; +} + +// ── tests ───────────────────────────────────────────────────────────────────── + +describe('TriggerResult.deferredRecheck type field', () => { + it('accepts deferredRecheck field with correct shape', () => { + // TypeScript compile-time check (also verified by npm run typecheck). + // If the field doesn't exist on TriggerResult, this file won't compile. + const result: TriggerResult = { + agentType: null, + agentInput: {}, + deferredRecheck: { delayMs: 45_000, coalesceKey: 'proj:pr-conflict-recheck:42' }, + }; + expect(result.deferredRecheck?.delayMs).toBe(45_000); + expect(result.deferredRecheck?.coalesceKey).toBe('proj:pr-conflict-recheck:42'); + }); +}); + +describe('GitHubJob.mergeabilityRecheckAttempt type field', () => { + it('accepts mergeabilityRecheckAttempt field', () => { + // TypeScript compile-time check. If the field doesn't exist on GitHubJob, this won't compile. + const job: GitHubJob = { + type: 'github', + source: 'github', + payload: {}, + eventType: 'pull_request', + repoFullName: 'owner/repo', + receivedAt: '', + mergeabilityRecheckAttempt: 1, + }; + expect(job.mergeabilityRecheckAttempt).toBe(1); + }); +}); + +describe('GitHubRouterAdapter.buildJob — deferred re-check behavior', () => { + let adapter: GitHubRouterAdapter; + + beforeEach(() => { + adapter = new GitHubRouterAdapter(); + vi.mocked(loadProjectConfig).mockResolvedValue({ + projects: [mockProject], + fullProjects: [{ id: 'p1', repo: 'owner/repo' } as never], + }); + }); + + const event = { + projectIdentifier: 'owner/repo', + eventType: 'pull_request', + workItemId: '42', + isCommentEvent: false, + repoFullName: 'owner/repo', + } as ReturnType extends Promise + ? NonNullable + : never; + + it('omits triggerResult when result.deferredRecheck is set', () => { + const job = adapter.buildJob( + event, + {}, + mockProject, + deferredRecheckResult, + undefined, + ) as GitHubJob; + expect(job.triggerResult).toBeUndefined(); + }); + + it('sets mergeabilityRecheckAttempt: 1 when result.deferredRecheck is set', () => { + const job = adapter.buildJob( + event, + {}, + mockProject, + deferredRecheckResult, + undefined, + ) as GitHubJob; + expect(job.mergeabilityRecheckAttempt).toBe(1); + }); + + it('includes triggerResult normally when deferredRecheck is absent', () => { + const normalResult: TriggerResult = { + agentType: 'resolve-conflicts', + agentInput: { prNumber: 42 }, + }; + const job = adapter.buildJob(event, {}, mockProject, normalResult, undefined) as GitHubJob; + expect(job.triggerResult).toBe(normalResult); + }); + + it('does not set mergeabilityRecheckAttempt when deferredRecheck is absent', () => { + const normalResult: TriggerResult = { + agentType: 'resolve-conflicts', + agentInput: { prNumber: 42 }, + }; + const job = adapter.buildJob(event, {}, mockProject, normalResult, undefined) as GitHubJob; + expect(job.mergeabilityRecheckAttempt).toBeUndefined(); + }); +}); + +describe('processRouterWebhook — deferred re-check branch', () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(scheduleCoalescedJob).mockResolvedValue({ + jobId: 'cj-1', + superseded: false, + supersededJobData: undefined, + }); + vi.mocked(isWorkItemLocked).mockResolvedValue({ locked: false }); + }); + + it('calls scheduleCoalescedJob with the built job, coalesceKey, and delayMs', async () => { + const adapter = makeMockAdapter({ + dispatchWithCredentials: vi.fn().mockResolvedValue(deferredRecheckResult), + }); + await processRouterWebhook(adapter, {}, mockTriggerRegistry); + expect(scheduleCoalescedJob).toHaveBeenCalledOnce(); + expect(scheduleCoalescedJob).toHaveBeenCalledWith( + deferredBareJob, + 'p1:pr-conflict-recheck:42', + 45_000, + ); + }); + + it('returns decisionReason containing coalesceKey', async () => { + const adapter = makeMockAdapter({ + dispatchWithCredentials: vi.fn().mockResolvedValue(deferredRecheckResult), + }); + const result = await processRouterWebhook(adapter, {}, mockTriggerRegistry); + expect(result.shouldProcess).toBe(true); + expect(result.decisionReason).toContain('Deferred re-check scheduled'); + expect(result.decisionReason).toContain('p1:pr-conflict-recheck:42'); + }); + + it('does NOT call isWorkItemLocked (exits before lock check)', async () => { + const adapter = makeMockAdapter({ + dispatchWithCredentials: vi.fn().mockResolvedValue(deferredRecheckResult), + }); + await processRouterWebhook(adapter, {}, mockTriggerRegistry); + expect(isWorkItemLocked).not.toHaveBeenCalled(); + }); + + it('Sentry-captures and returns shouldProcess:true when scheduleCoalescedJob throws', async () => { + vi.mocked(scheduleCoalescedJob).mockRejectedValueOnce(new Error('Redis down')); + const adapter = makeMockAdapter({ + dispatchWithCredentials: vi.fn().mockResolvedValue(deferredRecheckResult), + }); + const result = await processRouterWebhook(adapter, {}, mockTriggerRegistry); + expect(result.shouldProcess).toBe(true); + expect(captureException).toHaveBeenCalledWith( + expect.any(Error), + expect.objectContaining({ tags: { source: 'deferred_recheck_schedule_failure' } }), + ); + }); + + it('deferred re-check branch skipped when agentType is non-null', async () => { + const resultWithAgent: TriggerResult = { + agentType: 'resolve-conflicts', + agentInput: { prNumber: 42 }, + deferredRecheck: { delayMs: 45_000, coalesceKey: 'p1:pr-conflict-recheck:42' }, + }; + const adapter = makeMockAdapter({ + dispatchWithCredentials: vi.fn().mockResolvedValue(resultWithAgent), + }); + await processRouterWebhook(adapter, {}, mockTriggerRegistry); + // Should have gone to the normal dispatch path, NOT the deferred-recheck branch + expect(scheduleCoalescedJob).not.toHaveBeenCalledWith( + expect.anything(), + 'p1:pr-conflict-recheck:42', + 45_000, + ); + }); +}); diff --git a/tests/unit/triggers/github-webhook-handler-recheck.test.ts b/tests/unit/triggers/github-webhook-handler-recheck.test.ts new file mode 100644 index 000000000..0af7f6910 --- /dev/null +++ b/tests/unit/triggers/github-webhook-handler-recheck.test.ts @@ -0,0 +1,173 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +// ── Static mocks (hoisted before imports) ───────────────────────────────────── + +vi.mock('../../../src/sentry.js', () => ({ + captureException: vi.fn(), + flush: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock('../../../src/triggers/github/integration.js', () => { + const mockIntegration = { + type: 'github', + parseWebhookPayload: vi.fn().mockReturnValue({ + eventType: 'pull_request', + projectIdentifier: 'owner/repo', + workItemId: undefined, + raw: {}, + }), + lookupProject: vi.fn().mockResolvedValue({ + project: { + id: 'project-1', + name: 'Test', + repo: 'owner/repo', + baseBranch: 'main', + watchdogTimeoutMs: 120000, + }, + config: { projects: [] }, + }), + withCredentials: vi.fn().mockImplementation((_projectId: unknown, fn: () => unknown) => fn()), + resolveExecutionConfig: vi.fn().mockReturnValue({ + skipPrepareForAgent: true, + skipHandleFailure: true, + handleSuccessOnlyForAgentType: 'implementation', + logLabel: 'GitHub agent', + }), + }; + return { GitHubWebhookIntegration: vi.fn().mockImplementation(() => mockIntegration) }; +}); + +vi.mock('../../../src/github/personas.js', () => ({ + getPersonaToken: vi.fn().mockResolvedValue('gh-token-xxx'), + resolvePersonaIdentities: vi + .fn() + .mockResolvedValue({ implementer: 'bot', reviewer: 'reviewer-bot' }), +})); + +vi.mock('../../../src/github/client.js', () => ({ + githubClient: { deletePRComment: vi.fn().mockResolvedValue(undefined) }, + withGitHubToken: vi.fn().mockImplementation((_token: unknown, fn: () => unknown) => fn()), +})); + +vi.mock('../../../src/utils/repo.js', () => ({ + parseRepoFullName: vi.fn().mockReturnValue({ owner: 'owner', repo: 'repo' }), + getWorkspaceDir: vi.fn().mockReturnValue('/tmp/workspace'), +})); + +vi.mock('../../../src/agents/definitions/loader.js', () => ({ + isPMFocusedAgent: vi.fn().mockResolvedValue(false), +})); + +vi.mock('../../../src/router/ackMessageGenerator.js', () => ({ + extractGitHubContext: vi.fn().mockReturnValue('PR context'), + generateAckMessage: vi.fn().mockResolvedValue('Starting...'), +})); + +vi.mock('../../../src/utils/safeOperation.js', () => ({ + safeOperation: vi.fn().mockImplementation((fn: () => unknown) => fn()), +})); + +vi.mock('../../../src/triggers/shared/concurrency.js', () => ({ + withAgentTypeConcurrency: vi + .fn() + .mockImplementation((_id: unknown, _type: unknown, fn: () => unknown) => fn()), +})); + +vi.mock('../../../src/triggers/shared/credential-scope.js', () => ({ + withPMScope: vi.fn().mockImplementation((_project: unknown, fn: () => unknown) => fn()), +})); + +vi.mock('../../../src/triggers/shared/pm-ack.js', () => ({ + postPMAckComment: vi.fn().mockResolvedValue('pm-comment-id'), +})); + +vi.mock('../../../src/triggers/shared/webhook-execution.js', () => ({ + runAgentWithCredentials: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock('../../../src/triggers/github/ack-comments.js', () => ({ + postAcknowledgmentComment: vi.fn().mockResolvedValue(undefined), + updateInitialCommentWithError: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock('../../../src/utils/index.js', () => ({ + logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn(), error: vi.fn() }, + startWatchdog: vi.fn(), +})); + +// ── Imports ─────────────────────────────────────────────────────────────────── + +import { captureException } from '../../../src/sentry.js'; +import { processGitHubWebhook } from '../../../src/triggers/github/webhook-handler.js'; +import type { TriggerResult } from '../../../src/types/index.js'; + +// ── Fixtures ────────────────────────────────────────────────────────────────── + +const deferredRecheckResult: TriggerResult = { + agentType: null, + agentInput: {}, + deferredRecheck: { delayMs: 45_000, coalesceKey: 'project-1:pr-conflict-recheck:42' }, +}; + +const normalResult: TriggerResult = { + agentType: 'resolve-conflicts', + agentInput: { prNumber: 42 }, +}; + +function makeRegistry(result: TriggerResult | null) { + return { dispatch: vi.fn().mockResolvedValue(result) } as never; +} + +// ── Tests ───────────────────────────────────────────────────────────────────── + +describe('processGitHubWebhook — re-check exhaustion detection', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('emits Sentry capture when isRecheckJob=true and trigger returns deferredRecheck', async () => { + const registry = makeRegistry(deferredRecheckResult); + await processGitHubWebhook({}, 'pull_request', registry, undefined, undefined, undefined, true); + expect(captureException).toHaveBeenCalledWith( + expect.objectContaining({ + message: expect.stringContaining('mergeability_recheck_exhausted'), + }), + expect.objectContaining({ tags: { source: 'mergeability_recheck_exhausted' } }), + ); + }); + + it('does NOT capture Sentry when isRecheckJob=false and trigger returns deferredRecheck', async () => { + const registry = makeRegistry(deferredRecheckResult); + await processGitHubWebhook( + {}, + 'pull_request', + registry, + undefined, + undefined, + undefined, + false, + ); + expect(captureException).not.toHaveBeenCalled(); + }); + + it('does NOT capture Sentry when isRecheckJob=true but trigger returns non-deferredRecheck result', async () => { + const registry = makeRegistry(normalResult); + await processGitHubWebhook({}, 'pull_request', registry, undefined, undefined, undefined, true); + expect(captureException).not.toHaveBeenCalled(); + }); + + it('does NOT capture Sentry when triggerResult is pre-resolved (no re-dispatch)', async () => { + const registry = makeRegistry(null); + await processGitHubWebhook( + {}, + 'pull_request', + registry, + undefined, + undefined, + normalResult, + true, + ); + expect(captureException).not.toHaveBeenCalled(); + expect(registry.dispatch).not.toHaveBeenCalled(); + }); +}); diff --git a/tests/unit/triggers/pr-conflict-detected.test.ts b/tests/unit/triggers/pr-conflict-detected.test.ts index 55fc10bed..96dbe6e9f 100644 --- a/tests/unit/triggers/pr-conflict-detected.test.ts +++ b/tests/unit/triggers/pr-conflict-detected.test.ts @@ -340,7 +340,7 @@ describe('PRConflictDetectedTrigger', () => { expect(result?.skipReason?.handler).toBe('pr-conflict-detected'); }); - it('returns null when mergeable is null after retries', async () => { + it('returns deferredRecheck (not skipReason) when mergeable is null after retries', async () => { vi.mocked(githubClient.getPR).mockResolvedValue({ number: 42, title: 'Test PR', @@ -364,12 +364,67 @@ describe('PRConflictDetectedTrigger', () => { const result = await trigger.handle(ctx); + expect(result?.deferredRecheck).toBeDefined(); + expect(result?.skipReason).toBeUndefined(); expect(result?.agentType).toBeNull(); - expect(result?.skipReason?.handler).toBe('pr-conflict-detected'); // Should have retried 2 times + initial call = 3 calls expect(githubClient.getPR).toHaveBeenCalledTimes(3); }); + it('deferredRecheck coalesceKey includes projectId and prNumber', async () => { + vi.mocked(githubClient.getPR).mockResolvedValue({ + number: 42, + title: 'Test PR', + body: 'https://trello.com/c/abc123', + state: 'open', + htmlUrl: 'https://github.com/owner/repo/pull/42', + headRef: 'feature/test', + headSha: 'sha123', + baseRef: 'main', + merged: false, + mergeable: null, + user: { login: 'cascade-impl' }, + }); + + const ctx: TriggerContext = { + project: mockProject, + source: 'github', + payload: makeSynchronizePayload(), + personaIdentities: mockPersonaIdentities, + }; + + const result = await trigger.handle(ctx); + + expect(result?.deferredRecheck?.coalesceKey).toBe('test:pr-conflict-recheck:42'); + }); + + it('deferredRecheck delayMs is 45000', async () => { + vi.mocked(githubClient.getPR).mockResolvedValue({ + number: 42, + title: 'Test PR', + body: 'https://trello.com/c/abc123', + state: 'open', + htmlUrl: 'https://github.com/owner/repo/pull/42', + headRef: 'feature/test', + headSha: 'sha123', + baseRef: 'main', + merged: false, + mergeable: null, + user: { login: 'cascade-impl' }, + }); + + const ctx: TriggerContext = { + project: mockProject, + source: 'github', + payload: makeSynchronizePayload(), + personaIdentities: mockPersonaIdentities, + }; + + const result = await trigger.handle(ctx); + + expect(result?.deferredRecheck?.delayMs).toBe(45_000); + }); + it('retries when mergeable is initially null, then succeeds when it becomes false', async () => { // First two calls return null, third returns false (conflict detected) vi.mocked(githubClient.getPR) diff --git a/tests/unit/triggers/shared/agent-execution-types.test.ts b/tests/unit/triggers/shared/agent-execution-types.test.ts new file mode 100644 index 000000000..ad7dc5146 --- /dev/null +++ b/tests/unit/triggers/shared/agent-execution-types.test.ts @@ -0,0 +1,37 @@ +import { describe, expectTypeOf, it } from 'vitest'; +import type { LifecycleHooks } from '../../../../src/agents/definitions/schema.js'; +import type { PMLifecycleManager } from '../../../../src/pm/index.js'; +import type { AgentExecutionConfig as ReExportedAgentExecutionConfig } from '../../../../src/triggers/shared/agent-execution.js'; +import type { + AgentExecutionConfig, + AgentExecutionContext, +} from '../../../../src/triggers/shared/agent-execution-types.js'; +import type { TriggerResult } from '../../../../src/triggers/types.js'; +import type { AgentInput, CascadeConfig, ProjectConfig } from '../../../../src/types/index.js'; + +describe('agent execution shared types', () => { + it('keeps AgentExecutionConfig available from the existing agent-execution module', () => { + expectTypeOf().toEqualTypeOf(); + }); + + it('captures the resolved execution context contract for later extraction helpers', () => { + expectTypeOf().toHaveProperty('result').toEqualTypeOf(); + expectTypeOf().toHaveProperty('project').toEqualTypeOf(); + expectTypeOf().toHaveProperty('config').toEqualTypeOf(); + expectTypeOf() + .toHaveProperty('executionConfig') + .toEqualTypeOf(); + expectTypeOf().toHaveProperty('agentType').toEqualTypeOf(); + expectTypeOf().toHaveProperty('logLabel').toEqualTypeOf(); + expectTypeOf() + .toHaveProperty('lifecycle') + .toEqualTypeOf(); + expectTypeOf() + .toHaveProperty('lifecycleHooks') + .toEqualTypeOf(); + expectTypeOf() + .toHaveProperty('workItemId') + .toEqualTypeOf(); + expectTypeOf().toHaveProperty('agentInput').toEqualTypeOf(); + }); +}); diff --git a/tests/unit/triggers/shared/agent-execution.test.ts b/tests/unit/triggers/shared/agent-execution.test.ts index 810e300bd..e37b72468 100644 --- a/tests/unit/triggers/shared/agent-execution.test.ts +++ b/tests/unit/triggers/shared/agent-execution.test.ts @@ -174,7 +174,10 @@ vi.mock('../../../../src/triggers/github/review-dispatch-dedup.js', () => ({ buildReviewDispatchKey: (...args: unknown[]) => mockBuildReviewDispatchKey(...args), })); -import { linkPRToWorkItem } from '../../../../src/db/repositories/prWorkItemsRepository.js'; +import { + createWorkItem, + linkPRToWorkItem, +} from '../../../../src/db/repositories/prWorkItemsRepository.js'; import { runAgentExecutionPipeline } from '../../../../src/triggers/shared/agent-execution.js'; // --------------------------------------------------------------------------- @@ -252,6 +255,173 @@ function setupSplittingDefaults(providerOverrides: Record = {}) // Tests // --------------------------------------------------------------------------- +describe('runAgentExecutionPipeline facade characterization', () => { + function setupPipelineDefaults() { + vi.clearAllMocks(); + mockCreatePMProvider.mockReturnValue({}); + mockResolveProjectPMConfig.mockReturnValue(PM_CONFIG); + mockValidateIntegrations.mockResolvedValue({ valid: true, errors: [] }); + mockCheckBudgetExceeded.mockResolvedValue(null); + mockHandleAgentResultArtifacts.mockResolvedValue(undefined); + mockShouldTriggerDebug.mockResolvedValue(null); + mockGetSessionState.mockReturnValue({}); + mockRunAgent.mockResolvedValue({ success: true, output: '', runId: 'run-1' }); + mockGithubClient.getCheckSuiteStatus.mockResolvedValue({ allPassing: false }); + } + + function lifecycleInstance() { + return MockPMLifecycleManager.mock.results[0]?.value as { + prepareForAgent: ReturnType; + handleSuccess: ReturnType; + handleFailure: ReturnType; + cleanupProcessing: ReturnType; + }; + } + + beforeEach(() => { + setupPipelineDefaults(); + }); + + it('returns before constructing lifecycle or running the agent when agentType is missing', async () => { + await runAgentExecutionPipeline( + { agentType: null, agentInput: {}, workItemId: 'card-1' }, + PROJECT, + CONFIG, + ); + + expect(mockLogger.warn).toHaveBeenCalledWith( + 'No agent type in trigger result, skipping execution pipeline', + ); + expect(mockCreatePMProvider).not.toHaveBeenCalled(); + expect(mockValidateIntegrations).not.toHaveBeenCalled(); + expect(mockRunAgent).not.toHaveBeenCalled(); + }); + + it('notifies PM and invokes onFailure when integration validation fails after PM validation is usable', async () => { + mockValidateIntegrations.mockResolvedValueOnce({ + valid: false, + errors: [{ category: 'scm', message: 'GitHub token missing' }], + }); + const onFailure = vi.fn().mockResolvedValue(undefined); + + await runAgentExecutionPipeline( + { agentType: 'implementation', agentInput: {}, workItemId: 'card-1' }, + PROJECT, + CONFIG, + { onFailure }, + ); + + const lifecycle = lifecycleInstance(); + expect(mockRunAgent).not.toHaveBeenCalled(); + expect(lifecycle.handleFailure).toHaveBeenCalledWith('card-1', 'validation error'); + expect(onFailure).toHaveBeenCalledWith( + expect.objectContaining({ agentType: 'implementation', workItemId: 'card-1' }), + { success: false, output: '', error: 'validation error' }, + ); + }); + + it('does not attempt PM failure notification when PM validation itself failed', async () => { + mockValidateIntegrations.mockResolvedValueOnce({ + valid: false, + errors: [{ category: 'pm', message: 'Trello missing' }], + }); + const onFailure = vi.fn().mockResolvedValue(undefined); + + await runAgentExecutionPipeline( + { agentType: 'implementation', agentInput: {}, workItemId: 'card-1' }, + PROJECT, + CONFIG, + { onFailure }, + ); + + const lifecycle = lifecycleInstance(); + expect(mockRunAgent).not.toHaveBeenCalled(); + expect(lifecycle.handleFailure).not.toHaveBeenCalled(); + expect(onFailure).toHaveBeenCalledWith( + expect.objectContaining({ agentType: 'implementation', workItemId: 'card-1' }), + { success: false, output: '', error: 'validation error' }, + ); + }); + + it('continues to run the agent when work-item persistence fails before execution', async () => { + vi.mocked(createWorkItem).mockRejectedValueOnce(new Error('db unavailable')); + + await runAgentExecutionPipeline( + { agentType: 'implementation', agentInput: {}, workItemId: 'card-1' }, + PROJECT, + CONFIG, + ); + + expect(mockRunAgent).toHaveBeenCalledWith( + 'implementation', + expect.objectContaining({ workItemId: 'card-1' }), + ); + expect(mockLogger.warn).toHaveBeenCalledWith( + 'Failed to persist work-item row for PM-triggered run', + expect.objectContaining({ projectId: 'project-1', workItemId: 'card-1' }), + ); + }); + + it('orders onSuccess after agent execution and successful post-run lifecycle handling', async () => { + const onSuccess = vi.fn().mockResolvedValue(undefined); + + await runAgentExecutionPipeline( + { agentType: 'implementation', agentInput: {}, workItemId: 'card-1' }, + PROJECT, + CONFIG, + { onSuccess }, + ); + + const lifecycle = lifecycleInstance(); + expect(mockRunAgent.mock.invocationCallOrder[0]).toBeLessThan( + lifecycle.handleSuccess.mock.invocationCallOrder[0], + ); + expect(lifecycle.handleSuccess.mock.invocationCallOrder[0]).toBeLessThan( + onSuccess.mock.invocationCallOrder[0], + ); + }); + + it('orders onFailure after agent execution and failed post-run lifecycle handling', async () => { + mockRunAgent.mockResolvedValueOnce({ success: false, output: '', error: 'agent failed' }); + const onFailure = vi.fn().mockResolvedValue(undefined); + + await runAgentExecutionPipeline( + { agentType: 'implementation', agentInput: {}, workItemId: 'card-1' }, + PROJECT, + CONFIG, + { onFailure }, + ); + + const lifecycle = lifecycleInstance(); + expect(mockRunAgent.mock.invocationCallOrder[0]).toBeLessThan( + lifecycle.handleFailure.mock.invocationCallOrder[0], + ); + expect(lifecycle.handleFailure.mock.invocationCallOrder[0]).toBeLessThan( + onFailure.mock.invocationCallOrder[0], + ); + }); + + it('honors lifecycle skip flags without skipping the agent run', async () => { + await runAgentExecutionPipeline( + { agentType: 'review', agentInput: {}, workItemId: 'card-1' }, + PROJECT, + CONFIG, + { + skipPrepareForAgent: true, + skipHandleFailure: true, + handleSuccessOnlyForAgentType: 'implementation', + }, + ); + + const lifecycle = lifecycleInstance(); + expect(mockRunAgent).toHaveBeenCalledWith('review', expect.anything()); + expect(lifecycle.prepareForAgent).not.toHaveBeenCalled(); + expect(lifecycle.cleanupProcessing).not.toHaveBeenCalled(); + expect(lifecycle.handleSuccess).not.toHaveBeenCalled(); + expect(lifecycle.handleFailure).not.toHaveBeenCalled(); + }); +}); + describe('propagateAutoLabelAfterSplitting (via runAgentExecutionPipeline)', () => { it('chains to backlog-manager when splitting succeeds with auto label and trigger enabled', async () => { const provider = setupSplittingDefaults(); diff --git a/tests/unit/worker-entry.test.ts b/tests/unit/worker-entry.test.ts index 034e271f8..77c65aa79 100644 --- a/tests/unit/worker-entry.test.ts +++ b/tests/unit/worker-entry.test.ts @@ -183,6 +183,34 @@ describe('dispatchJob routing', () => { 456, 'Starting implementation...', triggerResult, + false, + ); + }); + + it('passes isRecheckJob=true when job has mergeabilityRecheckAttempt set', async () => { + const mockRegistry = {}; + const jobPayload = { action: 'synchronize', pull_request: {} }; + + const jobData: GitHubJobData = { + type: 'github', + source: 'github', + payload: jobPayload, + eventType: 'pull_request', + repoFullName: 'org/repo', + receivedAt: '2024-01-01T00:00:00Z', + mergeabilityRecheckAttempt: 1, + }; + + await dispatchJob('job-recheck', jobData, mockRegistry as never); + + expect(processGitHubWebhook).toHaveBeenCalledWith( + jobPayload, + 'pull_request', + mockRegistry, + undefined, + undefined, + undefined, + true, ); });