From 17783cf0d52e8bb3ac867c30d26a0d15c35afd12 Mon Sep 17 00:00:00 2001 From: Zbigniew Sobiecki Date: Wed, 29 Apr 2026 20:44:01 +0200 Subject: [PATCH] fix(triggers): surface handler self-skip reason in webhook decisionReason (#1235) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR zbigniewsobiecki/ucho#155 had a CI failure that should have triggered respond-to-ci. The webhook arrived; cascade-router logged "No trigger matched for event" and bailed. The trigger DID match — something inside the handler self-skipped, but the persisted webhook log flattened the reason into the same generic string used when no matcher matches at all. Debugging that required trawling cascade-router process logs. Three coordinated fixes: A) Diagnostic upgrade. Extend TriggerResult with `skipReason: { handler, message }`. Handlers can return a structured skip instead of bare null; the registry returns the structured skip up the stack (terminating dispatch, not iterating further). webhook-processor.ts surfaces `Trigger ${handler} skipped: ${message}` in decisionReason. Bare `return null` keeps the legacy "try-next-handler" semantic for backward compatibility. Converted every return-null site in check-suite-failure.ts and pr-conflict-detected.ts to structured skips with specific messages (PR # included, attempt counts spelled out, base branches named). B) Stop swallowing persona-resolution failures in `GitHubAdapter.resolvePersonaCached`. The bare `catch {}` made every downstream gate that depends on personaIdentities silently skip without any signal. Now logs ERROR + Sentry-captures under tag `persona_identity_resolution_failed`. C) Widen the "PR author must be implementer persona" gate in both check-suite-failure.ts and pr-conflict-detected.ts to use the existing `isCascadeBot()` helper, matching either the implementer OR reviewer persona. The narrow check would have silently dropped any future PR authored by the reviewer persona; pr-comment-mention.ts and review-requested.ts already had the broader semantic. For PR #155 specifically: aaight IS the implementer for ucho, so the gate-widening (C) doesn't change the outcome — but with (A) shipped, any re-run will report the actual reason in the dashboard webhook log. Co-authored-by: Claude Opus 4.7 (1M context) --- src/router/adapters/github.ts | 17 ++- src/router/webhook-processor.ts | 19 +++ src/triggers/github/check-suite-failure.ts | 66 ++++++--- src/triggers/github/pr-conflict-detected.ts | 47 +++++-- src/types/index.ts | 18 +++ tests/unit/router/webhook-processor.test.ts | 26 +++- .../unit/triggers/check-suite-failure.test.ts | 93 ++++++++++--- .../triggers/pr-conflict-detected.test.ts | 21 ++- tests/unit/triggers/registry.test.ts | 126 ++++++++++++++++++ 9 files changed, 380 insertions(+), 53 deletions(-) diff --git a/src/router/adapters/github.ts b/src/router/adapters/github.ts index 303b9ef8..c127318d 100644 --- a/src/router/adapters/github.ts +++ b/src/router/adapters/github.ts @@ -143,8 +143,21 @@ export class GitHubRouterAdapter implements RouterPlatformAdapter { if (this.cachedPersonas !== undefined) return this.cachedPersonas; try { this.cachedPersonas = await resolvePersonaIdentities(projectId); - } catch { - // Resolution may fail — leave cachedPersonas undefined so callers can decide + } catch (err) { + // Surface the failure: every downstream gate that depends on + // personaIdentities will silently skip without these logs. Closes + // the 2026-04-29 incident class where ucho/PR#155's check_suite + // failure trigger could have been suppressed by an invisible + // token-resolution error. + logger.error('Failed to resolve cascade persona identities', { + projectId, + error: String(err), + }); + captureException(err, { + tags: { source: 'persona_identity_resolution_failed' }, + extra: { projectId }, + }); + // leave cachedPersonas undefined so callers can decide } return this.cachedPersonas; } diff --git a/src/router/webhook-processor.ts b/src/router/webhook-processor.ts index 3992f604..125b4b94 100644 --- a/src/router/webhook-processor.ts +++ b/src/router/webhook-processor.ts @@ -134,6 +134,25 @@ export async function processRouterWebhook( }; } + // Structured skip — a matched handler ran but bailed (e.g. precondition + // unmet, dedup claim lost, PR not from cascade persona). Surface the + // handler-specific reason in webhook log decisionReason so operators can + // triage from the dashboard without trawling cascade-router process logs. + if (result.skipReason && result.agentType === null) { + logger.info(`${adapter.type} trigger self-skipped`, { + handler: result.skipReason.handler, + message: result.skipReason.message, + eventType: event.eventType, + workItemId: event.workItemId, + projectId: project.id, + }); + return { + shouldProcess: true, + projectId: project.id, + decisionReason: `Trigger ${result.skipReason.handler} skipped: ${result.skipReason.message}`, + }; + } + logger.info(`${adapter.type} trigger matched`, { agentType: result.agentType || '(no agent)', workItemId: event.workItemId, diff --git a/src/triggers/github/check-suite-failure.ts b/src/triggers/github/check-suite-failure.ts index 5d073e82..4d502ffe 100644 --- a/src/triggers/github/check-suite-failure.ts +++ b/src/triggers/github/check-suite-failure.ts @@ -1,4 +1,5 @@ import { githubClient } from '../../github/client.js'; +import { isCascadeBot } from '../../github/personas.js'; import type { TriggerContext, TriggerHandler, TriggerResult } from '../../types/index.js'; import { logger } from '../../utils/logging.js'; import { parseRepoFullName } from '../../utils/repo.js'; @@ -6,6 +7,19 @@ import { checkTriggerEnabled } from '../shared/trigger-check.js'; import { type GitHubCheckSuitePayload, isGitHubCheckSuitePayload } from './types.js'; import { parsePrNumberFromRef, resolveWorkItemDisplayData, resolveWorkItemId } from './utils.js'; +/** + * Build a structured skip result so the router's webhook log decisionReason + * surfaces the real reason this handler bailed (instead of the generic + * "No trigger matched for event"). See `TriggerResult.skipReason`. + */ +function skip(handlerName: string, message: string): TriggerResult { + return { + agentType: null, + agentInput: {}, + skipReason: { handler: handlerName, message }, + }; +} + /** * Resolve a PR number from a check_suite payload. * Tries pull_requests[], then refs/pull/{N}/head parsing, then a GitHub API lookup by branch. @@ -75,7 +89,7 @@ export class CheckSuiteFailureTrigger implements TriggerHandler { this.name, )) ) { - return null; + return skip(this.name, 'respond-to-ci trigger is disabled for this project'); } const payload = ctx.payload as GitHubCheckSuitePayload; @@ -89,24 +103,33 @@ export class CheckSuiteFailureTrigger implements TriggerHandler { payload.check_suite.head_branch, this.name, ); - if (prNumber === null) return null; + if (prNumber === null) { + return skip(this.name, 'Could not resolve PR number from check_suite payload'); + } const headSha = payload.check_suite.head_sha; // Fetch PR details const prDetails = await githubClient.getPR(owner, repo, prNumber); - // Gate on PR author being the implementer persona + // Gate on PR author being a cascade persona (implementer OR reviewer). + // Loop-prevention: only auto-fix CI on PRs authored by the bot personas; + // human-authored PRs are owned by the human. if (!ctx.personaIdentities) { logger.info('No persona identities available, skipping', { handler: this.name, prNumber }); - return null; + return skip( + this.name, + 'Cascade persona identities could not be resolved (token / GitHub API issue)', + ); } - const implLogin = ctx.personaIdentities.implementer; - if (prDetails.user.login !== implLogin && prDetails.user.login !== `${implLogin}[bot]`) { - logger.info('PR not authored by implementer persona, skipping check failure trigger', { + if (!isCascadeBot(prDetails.user.login, ctx.personaIdentities)) { + logger.info('PR not authored by a cascade persona, skipping check failure trigger', { prNumber, prAuthor: prDetails.user.login, }); - return null; + return skip( + this.name, + `PR #${prNumber} not authored by a cascade persona (author: ${prDetails.user.login})`, + ); } // Only trigger for PRs targeting the project's base branch @@ -116,7 +139,10 @@ export class CheckSuiteFailureTrigger implements TriggerHandler { baseRef: prDetails.baseRef, projectBaseBranch: ctx.project.baseBranch, }); - return null; + return skip( + this.name, + `PR #${prNumber} targets ${prDetails.baseRef}, not project base branch ${ctx.project.baseBranch}`, + ); } // Resolve work item from DB @@ -129,14 +155,18 @@ export class CheckSuiteFailureTrigger implements TriggerHandler { // Verify ALL checks have completed (not still running) const allComplete = checkStatus.checkRuns.every((cr) => cr.status === 'completed'); if (!allComplete) { + const incomplete = checkStatus.checkRuns + .filter((cr) => cr.status !== 'completed') + .map((cr) => cr.name); logger.info('Not all checks complete yet, waiting', { prNumber, totalChecks: checkStatus.totalCount, - incompleteChecks: checkStatus.checkRuns - .filter((cr) => cr.status !== 'completed') - .map((cr) => cr.name), + incompleteChecks: incomplete, }); - return null; + return skip( + this.name, + `Not all checks complete yet (${incomplete.length}/${checkStatus.totalCount} still running): ${incomplete.join(', ')}`, + ); } // Verify at least one check failed @@ -152,7 +182,10 @@ export class CheckSuiteFailureTrigger implements TriggerHandler { prNumber, totalChecks: checkStatus.totalCount, }); - return null; + return skip( + this.name, + `All ${checkStatus.totalCount} checks passed for PR #${prNumber} — no action needed`, + ); } // Check attempt limit to prevent infinite loops @@ -168,7 +201,10 @@ export class CheckSuiteFailureTrigger implements TriggerHandler { prNumber, '⚠️ Unable to automatically fix failing checks after 3 attempts. Manual intervention required.', ); - return null; + return skip( + this.name, + `Max auto-fix attempts (${MAX_ATTEMPTS}) reached for PR #${prNumber} — manual intervention required`, + ); } // Increment attempt counter diff --git a/src/triggers/github/pr-conflict-detected.ts b/src/triggers/github/pr-conflict-detected.ts index 0fbcd387..1f9aa01b 100644 --- a/src/triggers/github/pr-conflict-detected.ts +++ b/src/triggers/github/pr-conflict-detected.ts @@ -1,4 +1,5 @@ import { githubClient } from '../../github/client.js'; +import { isCascadeBot } from '../../github/personas.js'; import type { TriggerContext, TriggerHandler, TriggerResult } from '../../types/index.js'; import { logger } from '../../utils/logging.js'; import { parseRepoFullName } from '../../utils/repo.js'; @@ -6,6 +7,14 @@ import { checkTriggerEnabled } from '../shared/trigger-check.js'; import { type GitHubPullRequestPayload, isGitHubPullRequestPayload } from './types.js'; import { resolveWorkItemId } from './utils.js'; +function skip(handlerName: string, message: string): TriggerResult { + return { + agentType: null, + agentInput: {}, + skipReason: { handler: handlerName, message }, + }; +} + // Track conflict resolution attempts per PR to prevent infinite loops const conflictAttempts = new Map(); const MAX_ATTEMPTS = 2; @@ -44,7 +53,7 @@ export class PRConflictDetectedTrigger implements TriggerHandler { this.name, )) ) { - return null; + return skip(this.name, 'resolve-conflicts trigger is disabled for this project'); } const payload = ctx.payload as GitHubPullRequestPayload; @@ -52,22 +61,29 @@ export class PRConflictDetectedTrigger implements TriggerHandler { const repoFullName = payload.repository.full_name; const { owner, repo } = parseRepoFullName(repoFullName); - // Gate on PR author being the implementer persona + // Gate on PR author being a cascade persona (implementer OR reviewer). + // Loop-prevention: only auto-resolve conflicts on PRs authored by bot + // personas; human-authored PRs are owned by the human. if (!ctx.personaIdentities) { logger.info('No persona identities available, skipping', { handler: this.name, prNumber, }); - return null; + return skip( + this.name, + 'Cascade persona identities could not be resolved (token / GitHub API issue)', + ); } - const implLogin = ctx.personaIdentities.implementer; const prAuthorLogin = payload.pull_request.user.login; - if (prAuthorLogin !== implLogin && prAuthorLogin !== `${implLogin}[bot]`) { - logger.info('PR not authored by implementer persona, skipping conflict detection trigger', { + if (!isCascadeBot(prAuthorLogin, ctx.personaIdentities)) { + logger.info('PR not authored by a cascade persona, skipping conflict detection trigger', { prNumber, prAuthor: prAuthorLogin, }); - return null; + return skip( + this.name, + `PR #${prNumber} not authored by a cascade persona (author: ${prAuthorLogin})`, + ); } // Only trigger for PRs targeting the project's base branch @@ -77,7 +93,10 @@ export class PRConflictDetectedTrigger implements TriggerHandler { baseRef: payload.pull_request.base.ref, projectBaseBranch: ctx.project.baseBranch, }); - return null; + return skip( + this.name, + `PR #${prNumber} targets ${payload.pull_request.base.ref}, not project base branch ${ctx.project.baseBranch}`, + ); } // Fetch PR details, retrying if mergeable is null (GitHub computes it asynchronously) @@ -101,13 +120,16 @@ export class PRConflictDetectedTrigger implements TriggerHandler { logger.info('mergeable still null after retries, skipping conflict detection trigger', { prNumber, }); - return null; + return skip( + this.name, + `mergeable still null after ${MERGEABLE_RETRY_COUNT} retries for PR #${prNumber} — cannot determine mergeability`, + ); } // Only fire if PR is unmergeable (has conflicts) if (prDetails.mergeable !== false) { logger.debug('PR is mergeable, no conflict detected', { prNumber }); - return null; + return skip(this.name, `PR #${prNumber} is mergeable — no conflict detected`); } // Check attempt limit to prevent infinite loops @@ -123,7 +145,10 @@ export class PRConflictDetectedTrigger implements TriggerHandler { prNumber, '⚠️ Unable to automatically resolve merge conflicts after 2 attempts. Manual intervention required.', ); - return null; + return skip( + this.name, + `Max conflict resolution attempts (${MAX_ATTEMPTS}) reached for PR #${prNumber} — manual intervention required`, + ); } // Increment attempt counter diff --git a/src/types/index.ts b/src/types/index.ts index 6a22fcc7..bdc05af1 100644 --- a/src/types/index.ts +++ b/src/types/index.ts @@ -124,6 +124,24 @@ export interface TriggerResult { * Typical key: `${projectId}:${workItemId}`. */ coalesceKey?: string; + /** + * Structured self-skip signal — set when a matched handler decided NOT to + * dispatch an agent (e.g. all-checks-not-complete, attempt limit hit, PR + * not authored by a cascade persona). `agentType` MUST be `null` when + * `skipReason` is set. + * + * The router promotes `skipReason.message` into the persisted webhook log's + * `decisionReason` so operators can distinguish "no matcher matched" from + * "matcher claimed the event but bailed" without trawling cascade-router + * process logs. Bare `return null` from `handle()` keeps the legacy + * "try-next-handler" semantic; structured skips terminate dispatch. + */ + skipReason?: { + /** Name of the handler that produced the skip (e.g. `'check-suite-failure'`). */ + handler: string; + /** Human-readable explanation. Used verbatim in webhook log decisionReason. */ + message: string; + }; } export interface TriggerHandler { diff --git a/tests/unit/router/webhook-processor.test.ts b/tests/unit/router/webhook-processor.test.ts index 6f4b57de..199a485c 100644 --- a/tests/unit/router/webhook-processor.test.ts +++ b/tests/unit/router/webhook-processor.test.ts @@ -196,7 +196,7 @@ describe('processRouterWebhook', () => { expect(addJob).not.toHaveBeenCalled(); }); - it('does not queue when dispatch returns null', async () => { + it('does not queue when dispatch returns null (no matcher matched)', async () => { const adapter = makeMockAdapter({ dispatchWithCredentials: vi.fn().mockResolvedValue(null), }); @@ -206,6 +206,30 @@ describe('processRouterWebhook', () => { expect(addJob).not.toHaveBeenCalled(); }); + // Diagnostic upgrade: a structured skip from a matched handler must be + // surfaced in decisionReason — distinct from "no matcher matched". Closes + // the 2026-04-29 ucho/PR#155 incident where check_suite-failure self-skipped + // for an unknown reason and webhooklogs only said "No trigger matched for + // event", forcing log-trawling instead of dashboard-level diagnosis. + it('surfaces handler-specific skipReason in decisionReason when a matched handler self-skipped', async () => { + const adapter = makeMockAdapter({ + dispatchWithCredentials: vi.fn().mockResolvedValue({ + agentType: null, + agentInput: {}, + skipReason: { + handler: 'check-suite-failure', + message: 'Not all checks complete yet, waiting', + }, + }), + }); + const result = await processRouterWebhook(adapter, {}, mockTriggerRegistry); + expect(result.shouldProcess).toBe(true); + expect(result.decisionReason).toBe( + 'Trigger check-suite-failure skipped: Not all checks complete yet, waiting', + ); + expect(addJob).not.toHaveBeenCalled(); + }); + it('queues job when dispatch returns a trigger result', async () => { const triggerResult = { agentType: 'implementation', agentInput: { cardId: 'card1' } }; vi.mocked(addJob).mockResolvedValue('job-1'); diff --git a/tests/unit/triggers/check-suite-failure.test.ts b/tests/unit/triggers/check-suite-failure.test.ts index 782e0150..c686c141 100644 --- a/tests/unit/triggers/check-suite-failure.test.ts +++ b/tests/unit/triggers/check-suite-failure.test.ts @@ -27,6 +27,28 @@ vi.mock('../../../src/db/repositories/prWorkItemsRepository.js', () => ({ import { lookupWorkItemForPR } from '../../../src/db/repositories/prWorkItemsRepository.js'; import { checkTriggerEnabled } from '../../../src/triggers/shared/trigger-check.js'; +/** + * Assert that a TriggerResult is a structured self-skip — i.e. `agentType` is + * null and `skipReason` is populated. Closes the 2026-04-29 prod incident + * where every self-skip looked like "No trigger matched for event" in the + * webhook log. + */ +function expectSkip( + result: Awaited>, + messageMatcher?: string | RegExp, +): void { + expect(result?.agentType).toBeNull(); + expect(result?.skipReason).toBeDefined(); + expect(result?.skipReason?.handler).toBe('check-suite-failure'); + if (messageMatcher !== undefined) { + if (typeof messageMatcher === 'string') { + expect(result?.skipReason?.message).toContain(messageMatcher); + } else { + expect(result?.skipReason?.message).toMatch(messageMatcher); + } + } +} + describe('CheckSuiteFailureTrigger', () => { const trigger = new CheckSuiteFailureTrigger(); @@ -170,7 +192,7 @@ describe('CheckSuiteFailureTrigger', () => { }); describe('handle', () => { - it('should return null when trigger is disabled', async () => { + it('returns a structured skip when trigger is disabled', async () => { vi.mocked(checkTriggerEnabled).mockResolvedValueOnce(false); const ctx: TriggerContext = { @@ -181,7 +203,7 @@ describe('CheckSuiteFailureTrigger', () => { }; const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result, 'respond-to-ci trigger is disabled for this project'); expect(checkTriggerEnabled).toHaveBeenCalledWith( 'test', 'respond-to-ci', @@ -239,7 +261,7 @@ describe('CheckSuiteFailureTrigger', () => { }); }); - it('returns null when PR targets non-base branch', async () => { + it('returns a structured skip when PR targets non-base branch', async () => { vi.mocked(githubClient.getPR).mockResolvedValue({ number: 42, title: 'Test PR', @@ -262,11 +284,11 @@ describe('CheckSuiteFailureTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result, /targets develop, not project base branch main/); expect(githubClient.getCheckSuiteStatus).not.toHaveBeenCalled(); }); - it('returns null when PR not authored by implementer persona', async () => { + it('returns a structured skip when PR not authored by any cascade persona', async () => { vi.mocked(githubClient.getPR).mockResolvedValue({ number: 42, title: 'Test PR', @@ -289,10 +311,47 @@ describe('CheckSuiteFailureTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result, /not authored by a cascade persona.*author: some-human/); + }); + + // Fix 3: gate widening — both implementer AND reviewer personas should + // match. PR #155 incident: aaight is the implementer for ucho; respond-to-ci + // should also fire for PRs authored by the reviewer persona. + it('fires when PR is authored by the REVIEWER persona (not just the implementer)', async () => { + vi.mocked(githubClient.getPR).mockResolvedValue({ + number: 42, + title: 'Test PR', + body: 'https://trello.com/c/abc123/card-name', + state: 'open', + htmlUrl: 'https://github.com/owner/repo/pull/42', + headRef: 'feature/test', + headSha: 'sha123', + baseRef: 'main', + merged: false, + user: { login: mockPersonaIdentities.reviewer }, + }); + vi.mocked(githubClient.getCheckSuiteStatus).mockResolvedValue({ + allPassing: false, + totalCount: 2, + checkRuns: [ + { name: 'lint', status: 'completed', conclusion: 'success' }, + { name: 'test', status: 'completed', conclusion: 'failure' }, + ], + }); + + const ctx: TriggerContext = { + project: mockProject, + source: 'github', + payload: makeFailurePayload(), + personaIdentities: mockPersonaIdentities, + }; + + const result = await trigger.handle(ctx); + + expect(result?.agentType).toBe('respond-to-ci'); }); - it('returns null when no personaIdentities available', async () => { + it('returns a structured skip when no personaIdentities available', async () => { vi.mocked(githubClient.getPR).mockResolvedValue({ number: 42, title: 'Test PR', @@ -314,7 +373,7 @@ describe('CheckSuiteFailureTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result, /persona identities could not be resolved/); }); it('fires without work item when DB has no link', async () => { @@ -351,7 +410,7 @@ describe('CheckSuiteFailureTrigger', () => { expect(result?.agentInput.workItemId).toBeUndefined(); }); - it('returns null when not all checks are complete', async () => { + it('returns a structured skip when not all checks are complete', async () => { vi.mocked(githubClient.getPR).mockResolvedValue({ number: 42, title: 'Test PR', @@ -382,10 +441,10 @@ describe('CheckSuiteFailureTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result, /Not all checks complete yet.*test/); }); - it('returns null when all checks actually passed (no failures)', async () => { + it('returns a structured skip when all checks actually passed (no failures)', async () => { vi.mocked(githubClient.getPR).mockResolvedValue({ number: 42, title: 'Test PR', @@ -416,10 +475,10 @@ describe('CheckSuiteFailureTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result, /All 2 checks passed/); }); - it('posts warning and returns null after MAX_ATTEMPTS (3)', async () => { + it('posts warning and returns a structured skip after MAX_ATTEMPTS (3)', async () => { vi.mocked(githubClient.getPR).mockResolvedValue({ number: 42, title: 'Test PR', @@ -453,7 +512,7 @@ describe('CheckSuiteFailureTrigger', () => { // 4th attempt should be blocked const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result, /Max auto-fix attempts \(3\) reached for PR #42/); expect(githubClient.createPRComment).toHaveBeenCalledWith( 'owner', 'repo', @@ -599,10 +658,10 @@ describe('CheckSuiteFailureTrigger', () => { const result = await trigger.handle(ctx); expect(githubClient.getOpenPRByBranch).toHaveBeenCalledWith('owner', 'repo', 'main'); - expect(result).toBeNull(); + expectSkip(result, /Could not resolve PR number/); }); - it('returns null when pull_requests is empty and head_branch is absent', async () => { + it('returns a structured skip when pull_requests is empty and head_branch is absent', async () => { const ctx: TriggerContext = { project: mockProject, source: 'github', @@ -624,7 +683,7 @@ describe('CheckSuiteFailureTrigger', () => { const result = await trigger.handle(ctx); expect(githubClient.getOpenPRByBranch).not.toHaveBeenCalled(); - expect(result).toBeNull(); + expectSkip(result, /Could not resolve PR number/); }); it('resetFixAttempts clears attempts for a PR', async () => { diff --git a/tests/unit/triggers/pr-conflict-detected.test.ts b/tests/unit/triggers/pr-conflict-detected.test.ts index 25be084e..f186223b 100644 --- a/tests/unit/triggers/pr-conflict-detected.test.ts +++ b/tests/unit/triggers/pr-conflict-detected.test.ts @@ -110,7 +110,8 @@ describe('PRConflictDetectedTrigger', () => { }; const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expect(result?.agentType).toBeNull(); + expect(result?.skipReason?.handler).toBe('pr-conflict-detected'); expect(checkTriggerEnabled).toHaveBeenCalledWith( 'test', 'resolve-conflicts', @@ -190,7 +191,8 @@ describe('PRConflictDetectedTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expect(result?.agentType).toBeNull(); + expect(result?.skipReason?.handler).toBe('pr-conflict-detected'); }); it('returns null when PR not authored by implementer persona', async () => { @@ -222,7 +224,8 @@ describe('PRConflictDetectedTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expect(result?.agentType).toBeNull(); + expect(result?.skipReason?.handler).toBe('pr-conflict-detected'); }); it('returns null when no personaIdentities available', async () => { @@ -248,7 +251,8 @@ describe('PRConflictDetectedTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expect(result?.agentType).toBeNull(); + expect(result?.skipReason?.handler).toBe('pr-conflict-detected'); }); it('fires without work item when DB has no link', async () => { @@ -310,7 +314,8 @@ describe('PRConflictDetectedTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expect(result?.agentType).toBeNull(); + expect(result?.skipReason?.handler).toBe('pr-conflict-detected'); }); it('returns null when mergeable is null after retries', async () => { @@ -337,7 +342,8 @@ describe('PRConflictDetectedTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + 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); }); @@ -415,7 +421,8 @@ describe('PRConflictDetectedTrigger', () => { // 3rd attempt should be blocked const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expect(result?.agentType).toBeNull(); + expect(result?.skipReason?.handler).toBe('pr-conflict-detected'); expect(githubClient.createPRComment).toHaveBeenCalledWith( 'owner', 'repo', diff --git a/tests/unit/triggers/registry.test.ts b/tests/unit/triggers/registry.test.ts index f8cae82f..286157f1 100644 --- a/tests/unit/triggers/registry.test.ts +++ b/tests/unit/triggers/registry.test.ts @@ -157,4 +157,130 @@ describe('TriggerRegistry', () => { expect(handler1.handle).toHaveBeenCalled(); expect(handler2.handle).toHaveBeenCalled(); }); + + // ============================================================================ + // Structured skipReason — diagnostic upgrade for "matcher matched + handler self-skipped" + // + // Before: every `return null` is indistinguishable from "no matcher matched" + // in the webhook log decisionReason. Closes the prod incident on + // 2026-04-29 where ucho/PR#155's check_suite failure didn't fire respond-to-ci + // and the only signal in webhooklogs was "No trigger matched for event". + // ============================================================================ + + describe('structured skipReason', () => { + it('returns the handler result when handle() returns a TriggerResult with skipReason and agentType=null', async () => { + const registry = createTriggerRegistry(); + + const handler: TriggerHandler = { + name: 'check-suite-failure', + description: 'Skips with structured reason', + matches: () => true, + handle: vi.fn().mockResolvedValue({ + agentType: null, + agentInput: {}, + skipReason: { + handler: 'check-suite-failure', + message: 'PR not authored by a cascade persona, skipping check failure trigger', + }, + }), + }; + + registry.register(handler); + + const ctx: TriggerContext = { + project: mockProject, + source: 'github', + payload: {}, + }; + + const result = await registry.dispatch(ctx); + + expect(result).not.toBeNull(); + expect(result?.agentType).toBeNull(); + expect(result?.skipReason).toEqual({ + handler: 'check-suite-failure', + message: 'PR not authored by a cascade persona, skipping check failure trigger', + }); + }); + + it('does NOT continue to the next handler when first matched handler returns a structured skip', async () => { + // A structured skip signals "I claim this event but bail" — distinct + // from the legacy `return null` path which means "I don't claim it". + const registry = createTriggerRegistry(); + + const skipper: TriggerHandler = { + name: 'first', + description: 'Skips', + matches: () => true, + handle: vi.fn().mockResolvedValue({ + agentType: null, + agentInput: {}, + skipReason: { handler: 'first', message: 'precondition unmet' }, + }), + }; + + const wouldRun: TriggerHandler = { + name: 'second', + description: 'Would have matched', + matches: () => true, + handle: vi.fn().mockResolvedValue({ + agentType: 'planning', + agentInput: {}, + }), + }; + + registry.register(skipper); + registry.register(wouldRun); + + const ctx: TriggerContext = { + project: mockProject, + source: 'github', + payload: {}, + }; + + const result = await registry.dispatch(ctx); + + expect(result?.skipReason?.handler).toBe('first'); + expect(skipper.handle).toHaveBeenCalled(); + expect(wouldRun.handle).not.toHaveBeenCalled(); + }); + + it('preserves legacy bare-null = continue behavior (unchanged)', async () => { + // Backward-compat: handlers can still return plain null to mean + // "not my event, try next handler". Only structured skips terminate. + const registry = createTriggerRegistry(); + + const bareNull: TriggerHandler = { + name: 'bare-null', + description: 'Returns null', + matches: () => true, + handle: vi.fn().mockResolvedValue(null), + }; + + const next: TriggerHandler = { + name: 'next', + description: 'Real match', + matches: () => true, + handle: vi.fn().mockResolvedValue({ + agentType: 'review', + agentInput: {}, + }), + }; + + registry.register(bareNull); + registry.register(next); + + const ctx: TriggerContext = { + project: mockProject, + source: 'github', + payload: {}, + }; + + const result = await registry.dispatch(ctx); + + expect(result?.agentType).toBe('review'); + expect(bareNull.handle).toHaveBeenCalled(); + expect(next.handle).toHaveBeenCalled(); + }); + }); });