From 6ba5d79255eec8fed52900c320042558adf5c338 Mon Sep 17 00:00:00 2001 From: Zbigniew Sobiecki Date: Wed, 29 Apr 2026 20:57:09 +0000 Subject: [PATCH] refactor(triggers): consolidate handler boilerplate via shared skip + gates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The trigger/webhook layer had 14+ bug fixes in the past 3 months clustering into 5 recurring classes (skip opacity, dedup collisions, ALS scope leakage, racing writers, gate divergence). The two GitHub handlers most affected by the latest two — check-suite-failure and pr-conflict-detected — had literal copies of the same skip(handler, message) helper. ~30 other return-null self-skip sites still produced the generic "No trigger matched for event" decisionReason despite the structured skipReason plumbing landed last week. This refactor consolidates the common shape into shared modules and pins the invariant with a static-grep guard. New shared modules: - src/triggers/shared/skip.ts — single canonical skip() builder. - src/triggers/shared/gates.ts — composable pure-function gates returning TriggerResult | null: - gateTriggerEnabled — async; wraps checkTriggerEnabled - gateBaseBranch — sync; PR base ref vs project.baseBranch - gateCascadePersona — sync; wraps isCascadeBot() - gateAttemptLimit — sync; per-PR retry counter check - requirePersonaIdentities — type-narrowing variant returning a { ok: true, value } | { ok: false, skip } discriminated union so callers no longer need ctx.personaIdentities! non-null assertions - tests/helpers/triggerAssertions.ts — expectSkip(result, handler, msg) + expectSkipFor(handler) factory for terse per-handler assertions. Static guard: tests/unit/triggers/handler-shape.test.ts walks every handler under src/triggers/ and asserts no file defines a local function skip( AND every GitHub handler that uses return skip(...) imports from ../shared/skip.js. Same pattern as trigger-event-consistency.test.ts and pm-router-adapter-pm-scope.test.ts. Handler-side changes: - 9 GitHub handlers compose gates from shared/gates.ts. Sync gate chains use ?? for short-circuit composition. - ~30 bare return null self-skip sites converted to structured skips with case-specific messages (PR # included, attempt counts, base branches). - pr-comment-mention pre-extracts prNumberHint so the persona-skip carries PR context. - Two integration tests updated to seed both agent + trigger configs so the gate exercises the actual loop-prevention persona check. Net diff: -297 / +823. Most additions are the new shared modules and their dedicated tests; handler files are flatter (typical handler lost ~30 lines of boilerplate). Co-Authored-By: Claude Opus 4.7 (1M context) --- src/triggers/github/check-suite-failure.ts | 98 +++------- src/triggers/github/check-suite-success.ts | 37 ++-- src/triggers/github/pr-comment-mention.ts | 56 ++++-- src/triggers/github/pr-conflict-detected.ts | 99 +++------- src/triggers/github/pr-merged.ts | 15 +- src/triggers/github/pr-opened.ts | 13 +- src/triggers/github/pr-ready-to-merge.ts | 43 +++- src/triggers/github/pr-review-submitted.ts | 40 ++-- src/triggers/github/review-requested.ts | 50 +++-- src/triggers/shared/gates.ts | 185 ++++++++++++++++++ src/triggers/shared/skip.ts | 21 ++ tests/helpers/triggerAssertions.ts | 56 ++++++ tests/integration/github-personas.test.ts | 23 ++- .../unit/triggers/check-suite-failure.test.ts | 24 +-- .../unit/triggers/check-suite-success.test.ts | 23 ++- .../github-pr-comment-mention.test.ts | 15 +- tests/unit/triggers/handler-shape.test.ts | 91 +++++++++ tests/unit/triggers/pr-merged.test.ts | 12 +- tests/unit/triggers/pr-opened.test.ts | 15 +- tests/unit/triggers/pr-ready-to-merge.test.ts | 20 +- .../unit/triggers/pr-review-submitted.test.ts | 11 +- tests/unit/triggers/review-requested.test.ts | 17 +- tests/unit/triggers/shared/gates.test.ts | 135 +++++++++++++ tests/unit/triggers/shared/skip.test.ts | 21 ++ 24 files changed, 823 insertions(+), 297 deletions(-) create mode 100644 src/triggers/shared/gates.ts create mode 100644 src/triggers/shared/skip.ts create mode 100644 tests/helpers/triggerAssertions.ts create mode 100644 tests/unit/triggers/handler-shape.test.ts create mode 100644 tests/unit/triggers/shared/gates.test.ts create mode 100644 tests/unit/triggers/shared/skip.test.ts diff --git a/src/triggers/github/check-suite-failure.ts b/src/triggers/github/check-suite-failure.ts index 4d502ffe..68c3407d 100644 --- a/src/triggers/github/check-suite-failure.ts +++ b/src/triggers/github/check-suite-failure.ts @@ -1,25 +1,18 @@ 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'; -import { checkTriggerEnabled } from '../shared/trigger-check.js'; +import { + gateAttemptLimit, + gateBaseBranch, + gateCascadePersona, + gateTriggerEnabled, + requirePersonaIdentities, +} from '../shared/gates.js'; +import { skip } from '../shared/skip.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. @@ -80,17 +73,13 @@ export class CheckSuiteFailureTrigger implements TriggerHandler { } async handle(ctx: TriggerContext): Promise { - // Check trigger config via new DB-driven system - if ( - !(await checkTriggerEnabled( - ctx.project.id, - 'respond-to-ci', - 'scm:check-suite-failure', - this.name, - )) - ) { - return skip(this.name, 'respond-to-ci trigger is disabled for this project'); - } + const enabled = await gateTriggerEnabled( + ctx.project.id, + 'respond-to-ci', + 'scm:check-suite-failure', + this.name, + ); + if (enabled) return enabled; const payload = ctx.payload as GitHubCheckSuitePayload; const { owner, repo } = parseRepoFullName(payload.repository.full_name); @@ -111,39 +100,16 @@ export class CheckSuiteFailureTrigger implements TriggerHandler { // Fetch PR details const prDetails = await githubClient.getPR(owner, repo, prNumber); - // 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 skip( - this.name, - 'Cascade persona identities could not be resolved (token / GitHub API issue)', - ); - } - 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 skip( - this.name, - `PR #${prNumber} not authored by a cascade persona (author: ${prDetails.user.login})`, - ); - } + // Sync gate chain — author must be a cascade persona (implementer OR + // reviewer; loop-prevention) AND the PR must target the project's base + // branch. Both gates short-circuit on the first failure. + const personasResult = requirePersonaIdentities(ctx.personaIdentities, prNumber, this.name); + if (!personasResult.ok) return personasResult.skip; - // Only trigger for PRs targeting the project's base branch - if (prDetails.baseRef !== ctx.project.baseBranch) { - logger.info('PR targets non-base branch, skipping check failure trigger', { - prNumber, - baseRef: prDetails.baseRef, - projectBaseBranch: ctx.project.baseBranch, - }); - return skip( - this.name, - `PR #${prNumber} targets ${prDetails.baseRef}, not project base branch ${ctx.project.baseBranch}`, - ); - } + const gateChainSkip = + gateCascadePersona(prDetails.user.login, prNumber, personasResult.value, this.name) ?? + gateBaseBranch(prDetails.baseRef, prNumber, ctx.project, this.name); + if (gateChainSkip) return gateChainSkip; // Resolve work item from DB const workItemId = await resolveWorkItemId(ctx.project.id, prNumber); @@ -188,23 +154,19 @@ export class CheckSuiteFailureTrigger implements TriggerHandler { ); } - // Check attempt limit to prevent infinite loops + // Check attempt limit to prevent infinite loops. Side effect (PR + // comment) is handler-local because the warning text is contractual, + // not part of the gate's pure logic. const attempts = fixAttempts.get(prNumber) || 0; - if (attempts >= MAX_ATTEMPTS) { - logger.warn('Max auto-fix attempts reached for PR', { - prNumber, - attempts, - }); + const limitSkip = gateAttemptLimit(attempts, MAX_ATTEMPTS, prNumber, this.name); + if (limitSkip) { await githubClient.createPRComment( owner, repo, prNumber, '⚠️ Unable to automatically fix failing checks after 3 attempts. Manual intervention required.', ); - return skip( - this.name, - `Max auto-fix attempts (${MAX_ATTEMPTS}) reached for PR #${prNumber} — manual intervention required`, - ); + return limitSkip; } // Increment attempt counter diff --git a/src/triggers/github/check-suite-success.ts b/src/triggers/github/check-suite-success.ts index b35c734c..b35cf731 100644 --- a/src/triggers/github/check-suite-success.ts +++ b/src/triggers/github/check-suite-success.ts @@ -2,6 +2,8 @@ import { type CheckSuiteStatus, githubClient } from '../../github/client.js'; import type { TriggerContext, TriggerHandler, TriggerResult } from '../../types/index.js'; import { logger } from '../../utils/logging.js'; import { parseRepoFullName } from '../../utils/repo.js'; +import { gateBaseBranch } from '../shared/gates.js'; +import { skip } from '../shared/skip.js'; import { checkTriggerEnabledWithParams } from '../shared/trigger-check.js'; import { buildReviewDispatchKey, @@ -103,7 +105,7 @@ export class CheckSuiteSuccessTrigger implements TriggerHandler { this.name, ); if (!triggerConfig.enabled) { - return null; + return skip(this.name, 'review trigger is disabled for this project'); } const payload = ctx.payload as GitHubCheckSuitePayload; @@ -119,7 +121,7 @@ export class CheckSuiteSuccessTrigger implements TriggerHandler { logger.info('Could not parse PR number from head_branch ref, skipping', { handler: this.name, }); - return null; + return skip(this.name, 'Could not parse PR number from check_suite head_branch'); } prNumber = parsed; } @@ -136,7 +138,10 @@ export class CheckSuiteSuccessTrigger implements TriggerHandler { this.name, ); if (!authorResult) { - return null; + return skip( + this.name, + 'Cascade persona identities could not be resolved (token / GitHub API issue)', + ); } if (!authorResult.shouldTrigger) { logger.info('PR author does not match configured authorMode, skipping', { @@ -146,18 +151,14 @@ export class CheckSuiteSuccessTrigger implements TriggerHandler { isCascadePR: authorResult.isCascadePR, authorMode: authorResult.authorMode, }); - return null; + return skip( + this.name, + `PR #${prNumber} author ${prDetails.user.login} does not match configured authorMode '${authorResult.authorMode}' (isCascadePR=${authorResult.isCascadePR})`, + ); } - // Only trigger for PRs targeting the project's base branch - if (prDetails.baseRef !== ctx.project.baseBranch) { - logger.info('PR targets non-base branch, skipping review trigger', { - prNumber, - baseRef: prDetails.baseRef, - projectBaseBranch: ctx.project.baseBranch, - }); - return null; - } + const baseSkip = gateBaseBranch(prDetails.baseRef, prNumber, ctx.project, this.name); + if (baseSkip) return baseSkip; // Resolve work item from DB const workItemId = await resolveWorkItemId(ctx.project.id, prNumber); @@ -186,7 +187,10 @@ export class CheckSuiteSuccessTrigger implements TriggerHandler { reviewerUsername, headSha, }); - return null; + return skip( + this.name, + `PR #${prNumber} already reviewed at HEAD ${headSha} by ${reviewerUsername} — no re-review needed`, + ); } logger.info('New commits since last review, re-triggering review', { prNumber, @@ -199,7 +203,10 @@ export class CheckSuiteSuccessTrigger implements TriggerHandler { // check_suite deliveries and other review-producing triggers. const dedupKey = buildReviewDispatchKey(owner, repo, prNumber, headSha); if (!claimReviewDispatch(dedupKey, this.name, { prNumber, headSha })) { - return null; + return skip( + this.name, + `Review dispatch for PR #${prNumber}@${headSha} already claimed by another path (dedup)`, + ); } // The trigger decision is made — the review agent should run. diff --git a/src/triggers/github/pr-comment-mention.ts b/src/triggers/github/pr-comment-mention.ts index cd1ee3c6..126c9eb5 100644 --- a/src/triggers/github/pr-comment-mention.ts +++ b/src/triggers/github/pr-comment-mention.ts @@ -3,7 +3,8 @@ 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'; -import { checkTriggerEnabled } from '../shared/trigger-check.js'; +import { gateTriggerEnabled, requirePersonaIdentities } from '../shared/gates.js'; +import { skip } from '../shared/skip.js'; import { isGitHubIssueCommentPayload, isGitHubPRReviewCommentPayload } from './types.js'; import { resolveWorkItemDisplayData, resolveWorkItemId } from './utils.js'; @@ -35,26 +36,30 @@ export class PRCommentMentionTrigger implements TriggerHandler { } async handle(ctx: TriggerContext): Promise { - // Check trigger config via new DB-driven system - if ( - !(await checkTriggerEnabled( - ctx.project.id, - 'respond-to-pr-comment', - 'scm:pr-comment-mention', - this.name, - )) - ) { - return null; + const enabled = await gateTriggerEnabled( + ctx.project.id, + 'respond-to-pr-comment', + 'scm:pr-comment-mention', + this.name, + ); + if (enabled) return enabled; + + // Pre-extract prNumber from whichever payload type matches so subsequent + // skip-reasons carry PR context (operator-friendly diagnostics). + let prNumberHint: number | undefined; + if (isGitHubIssueCommentPayload(ctx.payload)) { + prNumberHint = ctx.payload.issue.number; + } else if (isGitHubPRReviewCommentPayload(ctx.payload)) { + prNumberHint = ctx.payload.pull_request.number; } // Require persona identities for @mention detection - if (!ctx.personaIdentities) { - logger.warn('No persona identities available, skipping @mention trigger'); - return null; - } + const personasResult = requirePersonaIdentities(ctx.personaIdentities, prNumberHint, this.name); + if (!personasResult.ok) return personasResult.skip; + const personas = personasResult.value; // The implementer persona is who humans @mention (it writes code and responds) - const mentionTarget = ctx.personaIdentities.implementer; + const mentionTarget = personas.implementer; // Extract comment body from whichever payload type matched let commentBody: string; @@ -100,20 +105,31 @@ export class PRCommentMentionTrigger implements TriggerHandler { prTitle = payload.pull_request.title; repoFullName = payload.repository.full_name; } else { - return null; + // Defensive — matches() ensured one of the two payloads, but the + // type narrowing exists for completeness. + return skip( + this.name, + 'Comment payload was neither issue_comment nor pull_request_review_comment', + ); } // Check for @mention of the implementer persona (case-insensitive) const mentionPattern = new RegExp(`@${mentionTarget}\\b`, 'i'); if (!mentionPattern.test(commentBody)) { logger.debug('No @mention in comment, skipping', { prNumber, mentionTarget }); - return null; + return skip( + this.name, + `Comment on PR #${prNumber} does not @mention ${mentionTarget} — not a respond-to-pr-comment trigger`, + ); } // Skip @mentions from any known bot persona - if (isCascadeBot(commentAuthor, ctx.personaIdentities)) { + if (isCascadeBot(commentAuthor, personas)) { logger.info('Skipping @mention from cascade bot', { prNumber, commentAuthor }); - return null; + return skip( + this.name, + `@mention on PR #${prNumber} is from cascade bot ${commentAuthor} (loop prevention)`, + ); } // Resolve work item from DB diff --git a/src/triggers/github/pr-conflict-detected.ts b/src/triggers/github/pr-conflict-detected.ts index 1f9aa01b..75e289c5 100644 --- a/src/triggers/github/pr-conflict-detected.ts +++ b/src/triggers/github/pr-conflict-detected.ts @@ -1,20 +1,18 @@ 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'; -import { checkTriggerEnabled } from '../shared/trigger-check.js'; +import { + gateAttemptLimit, + gateBaseBranch, + gateCascadePersona, + gateTriggerEnabled, + requirePersonaIdentities, +} from '../shared/gates.js'; +import { skip } from '../shared/skip.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,60 +42,31 @@ export class PRConflictDetectedTrigger implements TriggerHandler { } async handle(ctx: TriggerContext): Promise { - // Check trigger config via DB-driven system - if ( - !(await checkTriggerEnabled( - ctx.project.id, - 'resolve-conflicts', - 'scm:pr-conflict-detected', - this.name, - )) - ) { - return skip(this.name, 'resolve-conflicts trigger is disabled for this project'); - } + const enabled = await gateTriggerEnabled( + ctx.project.id, + 'resolve-conflicts', + 'scm:pr-conflict-detected', + this.name, + ); + if (enabled) return enabled; const payload = ctx.payload as GitHubPullRequestPayload; const prNumber = payload.pull_request.number; const repoFullName = payload.repository.full_name; const { owner, repo } = parseRepoFullName(repoFullName); - // 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 skip( - this.name, - 'Cascade persona identities could not be resolved (token / GitHub API issue)', - ); - } - const prAuthorLogin = payload.pull_request.user.login; - if (!isCascadeBot(prAuthorLogin, ctx.personaIdentities)) { - logger.info('PR not authored by a cascade persona, skipping conflict detection trigger', { - prNumber, - prAuthor: prAuthorLogin, - }); - return skip( - this.name, - `PR #${prNumber} not authored by a cascade persona (author: ${prAuthorLogin})`, - ); - } + // Sync gate chain — author must be a cascade persona AND the PR must + // target the project's base branch. Loop-prevention: only auto-resolve + // conflicts on PRs authored by bot personas; human PRs are owned by + // the human. + const personasResult = requirePersonaIdentities(ctx.personaIdentities, prNumber, this.name); + if (!personasResult.ok) return personasResult.skip; - // Only trigger for PRs targeting the project's base branch - if (payload.pull_request.base.ref !== ctx.project.baseBranch) { - logger.info('PR targets non-base branch, skipping conflict detection trigger', { - prNumber, - baseRef: payload.pull_request.base.ref, - projectBaseBranch: ctx.project.baseBranch, - }); - return skip( - this.name, - `PR #${prNumber} targets ${payload.pull_request.base.ref}, not project base branch ${ctx.project.baseBranch}`, - ); - } + const prAuthorLogin = payload.pull_request.user.login; + const gateChainSkip = + gateCascadePersona(prAuthorLogin, prNumber, personasResult.value, this.name) ?? + gateBaseBranch(payload.pull_request.base.ref, prNumber, ctx.project, this.name); + if (gateChainSkip) return gateChainSkip; // Fetch PR details, retrying if mergeable is null (GitHub computes it asynchronously) let prDetails = await githubClient.getPR(owner, repo, prNumber); @@ -132,23 +101,19 @@ export class PRConflictDetectedTrigger implements TriggerHandler { return skip(this.name, `PR #${prNumber} is mergeable — no conflict detected`); } - // Check attempt limit to prevent infinite loops + // Check attempt limit to prevent infinite loops. Side effect (PR + // comment) is handler-local because the warning text differs from + // other handlers and is part of the contract. const attempts = conflictAttempts.get(prNumber) || 0; - if (attempts >= MAX_ATTEMPTS) { - logger.warn('Max conflict resolution attempts reached for PR', { - prNumber, - attempts, - }); + const limitSkip = gateAttemptLimit(attempts, MAX_ATTEMPTS, prNumber, this.name); + if (limitSkip) { await githubClient.createPRComment( owner, repo, prNumber, '⚠️ Unable to automatically resolve merge conflicts after 2 attempts. Manual intervention required.', ); - return skip( - this.name, - `Max conflict resolution attempts (${MAX_ATTEMPTS}) reached for PR #${prNumber} — manual intervention required`, - ); + return limitSkip; } // Increment attempt counter diff --git a/src/triggers/github/pr-merged.ts b/src/triggers/github/pr-merged.ts index 9d86b0d7..9266d150 100644 --- a/src/triggers/github/pr-merged.ts +++ b/src/triggers/github/pr-merged.ts @@ -7,6 +7,7 @@ import { logger } from '../../utils/logging.js'; import { parseRepoFullName } from '../../utils/repo.js'; import { isPipelineAtCapacity } from '../shared/backlog-check.js'; import { isLifecycleTriggerEnabled } from '../shared/lifecycle-check.js'; +import { skip } from '../shared/skip.js'; import { checkTriggerEnabled } from '../shared/trigger-check.js'; import { type GitHubPullRequestPayload, isGitHubPullRequestPayload } from './types.js'; import { resolveWorkItemId } from './utils.js'; @@ -25,7 +26,7 @@ export class PRMergedTrigger implements TriggerHandler { async handle(ctx: TriggerContext): Promise { // Check lifecycle trigger config (stored in project_integrations.triggers) if (!(await isLifecycleTriggerEnabled(ctx.project.id, 'prMerged', this.name))) { - return null; + return skip(this.name, 'prMerged lifecycle trigger is disabled for this project'); } const payload = ctx.payload as GitHubPullRequestPayload; @@ -37,14 +38,17 @@ export class PRMergedTrigger implements TriggerHandler { if (!prDetails.merged) { logger.info('PR closed but not merged, skipping', { prNumber }); - return null; + return skip( + this.name, + `PR #${prNumber} closed but not merged — no MERGED-status move needed`, + ); } // Resolve work item from DB const workItemId = await resolveWorkItemId(ctx.project.id, prNumber); if (!workItemId) { logger.info('No work item linked to PR, skipping pr-merged', { prNumber }); - return null; + return skip(this.name, `No work item linked to PR #${prNumber} — nothing to move to MERGED`); } // Fire-and-forget: invalidate the registry entry AND `docker rmi` the @@ -66,7 +70,10 @@ export class PRMergedTrigger implements TriggerHandler { logger.warn('No merged status configured for project', { projectId: ctx.project.id, }); - return null; + return skip( + this.name, + `Project ${ctx.project.id} has no 'merged' status configured — cannot move PR #${prNumber}'s work item`, + ); } const provider = getPMProvider(); diff --git a/src/triggers/github/pr-opened.ts b/src/triggers/github/pr-opened.ts index edb90908..60c9086c 100644 --- a/src/triggers/github/pr-opened.ts +++ b/src/triggers/github/pr-opened.ts @@ -1,5 +1,6 @@ import type { TriggerContext, TriggerHandler, TriggerResult } from '../../types/index.js'; import { logger } from '../../utils/logging.js'; +import { skip } from '../shared/skip.js'; import { checkTriggerEnabledWithParams } from '../shared/trigger-check.js'; import { isGitHubPullRequestPayload } from './types.js'; import { evaluateAuthorMode, resolveWorkItemId } from './utils.js'; @@ -34,7 +35,7 @@ export class PROpenedTrigger implements TriggerHandler { this.name, ); if (!triggerConfig.enabled) { - return null; + return skip(this.name, 'review trigger is disabled for this project'); } const payload = ctx.payload as { @@ -60,7 +61,10 @@ export class PROpenedTrigger implements TriggerHandler { this.name, ); if (!authorResult) { - return null; + return skip( + this.name, + 'Cascade persona identities could not be resolved (token / GitHub API issue)', + ); } if (!authorResult.shouldTrigger) { logger.info('PR author does not match configured authorMode, skipping', { @@ -70,7 +74,10 @@ export class PROpenedTrigger implements TriggerHandler { isCascadePR: authorResult.isCascadePR, authorMode: authorResult.authorMode, }); - return null; + return skip( + this.name, + `PR #${prNumber} author ${prAuthor} does not match configured authorMode '${authorResult.authorMode}' (isCascadePR=${authorResult.isCascadePR})`, + ); } // Resolve work item from DB diff --git a/src/triggers/github/pr-ready-to-merge.ts b/src/triggers/github/pr-ready-to-merge.ts index 8fe74741..8c617be9 100644 --- a/src/triggers/github/pr-ready-to-merge.ts +++ b/src/triggers/github/pr-ready-to-merge.ts @@ -7,6 +7,7 @@ import type { TriggerContext, TriggerHandler, TriggerResult } from '../../types/ import { logger } from '../../utils/logging.js'; import { parseRepoFullName } from '../../utils/repo.js'; import { isLifecycleTriggerEnabled } from '../shared/lifecycle-check.js'; +import { skip } from '../shared/skip.js'; import { type GitHubCheckSuitePayload, type GitHubPullRequestReviewPayload, @@ -15,6 +16,8 @@ import { } from './types.js'; import { resolveWorkItemId } from './utils.js'; +const HANDLER_NAME = 'pr-ready-to-merge'; + /** Merge PR automatically and move to MERGED; fall back to DONE on merge failure. */ async function handleAutoMerge( owner: string, @@ -36,7 +39,10 @@ async function handleAutoMerge( workItemId, '⚠️ Auto-merge requested (auto label present), but no MERGED or DONE status configured. Manual action required.', ); - return null; + return skip( + HANDLER_NAME, + `PR #${prNumber} auto-merge requested but neither MERGED nor DONE status is configured — manual action required`, + ); } await provider.moveWorkItem(workItemId, doneStatus); await provider.addComment( @@ -65,7 +71,10 @@ async function handleAutoMerge( workItemId, `⚠️ Auto-merge of PR #${prNumber} failed: ${String(err)}. No DONE status configured — manual action required.`, ); - return null; + return skip( + HANDLER_NAME, + `PR #${prNumber} auto-merge failed AND no DONE status fallback configured — manual action required`, + ); } await provider.moveWorkItem(workItemId, doneStatus); await provider.addComment( @@ -113,7 +122,7 @@ export class PRReadyToMergeTrigger implements TriggerHandler { async handle(ctx: TriggerContext): Promise { // Check lifecycle trigger config (stored in project_integrations.triggers) if (!(await isLifecycleTriggerEnabled(ctx.project.id, 'prReadyToMerge', this.name))) { - return null; + return skip(this.name, 'prReadyToMerge lifecycle trigger is disabled for this project'); } let prNumber: number; @@ -133,7 +142,8 @@ export class PRReadyToMergeTrigger implements TriggerHandler { headSha = payload.pull_request.head.sha; repoFullName = payload.repository.full_name; } else { - return null; + // Defensive — matches() ensured one of the two payloads. + return skip(this.name, 'Payload was neither check_suite nor pull_request_review'); } const { owner, repo } = parseRepoFullName(repoFullName); @@ -142,18 +152,27 @@ export class PRReadyToMergeTrigger implements TriggerHandler { const workItemId = await resolveWorkItemId(ctx.project.id, prNumber); if (!workItemId) { logger.info('No work item linked to PR, skipping pr-ready-to-merge', { prNumber }); - return null; + return skip( + this.name, + `No work item linked to PR #${prNumber} — nothing to move to DONE/MERGED`, + ); } // Check 1: All checks must pass const checkStatus = await githubClient.getCheckSuiteStatus(owner, repo, headSha); if (!checkStatus.allPassing) { + const failing = checkStatus.checkRuns + .filter((c) => c.conclusion !== 'success') + .map((c) => c.name); logger.debug('Not all checks passing', { prNumber, totalChecks: checkStatus.totalCount, - failing: checkStatus.checkRuns.filter((c) => c.conclusion !== 'success').map((c) => c.name), + failing, }); - return null; + return skip( + this.name, + `PR #${prNumber} has failing/in-progress checks (${failing.join(', ') || 'unknown'}) — not ready to merge`, + ); } // Check 2: Must have approved review and no outstanding change requests @@ -178,7 +197,10 @@ export class PRReadyToMergeTrigger implements TriggerHandler { hasApproval, hasChangeRequests, }); - return null; + return skip( + this.name, + `PR #${prNumber} not ready to merge: approved=${hasApproval}, changesRequested=${hasChangeRequests}`, + ); } // All conditions met — check for auto label to determine MERGED vs DONE path @@ -203,7 +225,10 @@ export class PRReadyToMergeTrigger implements TriggerHandler { const doneStatus = pmConfig.statuses.done; if (!doneStatus) { logger.warn('No done status configured for project', { projectId: ctx.project.id }); - return null; + return skip( + this.name, + `Project ${ctx.project.id} has no 'done' status configured — cannot move PR #${prNumber}'s work item`, + ); } // Idempotency: skip if work item is already in the DONE status diff --git a/src/triggers/github/pr-review-submitted.ts b/src/triggers/github/pr-review-submitted.ts index 4f2c2e37..fe56eb3d 100644 --- a/src/triggers/github/pr-review-submitted.ts +++ b/src/triggers/github/pr-review-submitted.ts @@ -1,7 +1,8 @@ import { getPersonaForLogin } from '../../github/personas.js'; import type { TriggerContext, TriggerHandler, TriggerResult } from '../../types/index.js'; import { logger } from '../../utils/logging.js'; -import { checkTriggerEnabled } from '../shared/trigger-check.js'; +import { gateTriggerEnabled, requirePersonaIdentities } from '../shared/gates.js'; +import { skip } from '../shared/skip.js'; import { type GitHubPullRequestReviewPayload, isGitHubPullRequestReviewPayload } from './types.js'; import { resolveWorkItemDisplayData, resolveWorkItemId } from './utils.js'; @@ -23,17 +24,13 @@ export class PRReviewSubmittedTrigger implements TriggerHandler { } async handle(ctx: TriggerContext): Promise { - // Check trigger config via new DB-driven system - if ( - !(await checkTriggerEnabled( - ctx.project.id, - 'respond-to-review', - 'scm:pr-review-submitted', - this.name, - )) - ) { - return null; - } + const enabled = await gateTriggerEnabled( + ctx.project.id, + 'respond-to-review', + 'scm:pr-review-submitted', + this.name, + ); + if (enabled) return enabled; // Type assertion since we validated in matches() const reviewPayload = ctx.payload as GitHubPullRequestReviewPayload; @@ -41,20 +38,23 @@ export class PRReviewSubmittedTrigger implements TriggerHandler { const prNumber = reviewPayload.pull_request.number; const reviewAuthor = reviewPayload.review.user.login; - // Only respond to reviews from the reviewer persona - if (!ctx.personaIdentities) { - logger.warn('No persona identities available, skipping review trigger', { prNumber }); - return null; - } + // Only respond to reviews from the reviewer persona (NOT general + // cascade-bot — implementer reviews shouldn't trigger respond-to-review) + const personasResult = requirePersonaIdentities(ctx.personaIdentities, prNumber, this.name); + if (!personasResult.ok) return personasResult.skip; + const personas = personasResult.value; - const persona = getPersonaForLogin(reviewAuthor, ctx.personaIdentities); + const persona = getPersonaForLogin(reviewAuthor, personas); if (persona !== 'reviewer') { logger.info('Skipping review not from reviewer persona', { prNumber, reviewAuthor, - expectedReviewer: ctx.personaIdentities.reviewer, + expectedReviewer: personas.reviewer, }); - return null; + return skip( + this.name, + `Review on PR #${prNumber} authored by ${reviewAuthor}, not the reviewer persona — not auto-responding`, + ); } // Resolve work item from DB diff --git a/src/triggers/github/review-requested.ts b/src/triggers/github/review-requested.ts index 2cd66f08..01d27a9a 100644 --- a/src/triggers/github/review-requested.ts +++ b/src/triggers/github/review-requested.ts @@ -1,7 +1,8 @@ import { isCascadeBot } from '../../github/personas.js'; import type { TriggerContext, TriggerHandler, TriggerResult } from '../../types/index.js'; import { logger } from '../../utils/logging.js'; -import { checkTriggerEnabled } from '../shared/trigger-check.js'; +import { gateTriggerEnabled, requirePersonaIdentities } from '../shared/gates.js'; +import { skip } from '../shared/skip.js'; import { buildReviewDispatchKey, claimReviewDispatch, @@ -39,10 +40,13 @@ export class ReviewRequestedTrigger implements TriggerHandler { } async handle(ctx: TriggerContext): Promise { - // Check trigger config via new DB-driven system - if (!(await checkTriggerEnabled(ctx.project.id, 'review', 'scm:review-requested', this.name))) { - return null; - } + const enabled = await gateTriggerEnabled( + ctx.project.id, + 'review', + 'scm:review-requested', + this.name, + ); + if (enabled) return enabled; const payload = ctx.payload as GitHubPullRequestPayload; const prNumber = payload.pull_request.number; @@ -50,39 +54,44 @@ export class ReviewRequestedTrigger implements TriggerHandler { const repoFullName = payload.repository.full_name; const [owner, repo] = repoFullName.split('/', 2); - // Require persona identities for bot detection - if (!ctx.personaIdentities) { - logger.warn('No persona identities available, skipping review-requested trigger', { - prNumber, - }); - return null; - } + const personasResult = requirePersonaIdentities(ctx.personaIdentities, prNumber, this.name); + if (!personasResult.ok) return personasResult.skip; + const personas = personasResult.value; // Skip review requests FROM CASCADE personas (self-loop prevention) const senderLogin = payload.sender.login; - if (isCascadeBot(senderLogin, ctx.personaIdentities)) { + if (isCascadeBot(senderLogin, personas)) { logger.info('Skipping review request from CASCADE persona (loop prevention)', { prNumber, sender: senderLogin, requestedReviewer: payload.requested_reviewer?.login, }); - return null; + return skip( + this.name, + `Review request on PR #${prNumber} sent BY cascade persona ${senderLogin} (loop prevention)`, + ); } // Check if the requested reviewer is a CASCADE persona const requestedReviewer = payload.requested_reviewer?.login; if (!requestedReviewer) { logger.debug('No requested reviewer in payload, skipping', { prNumber }); - return null; + return skip( + this.name, + `Review request on PR #${prNumber} has no requested_reviewer in payload`, + ); } - if (!isCascadeBot(requestedReviewer, ctx.personaIdentities)) { + if (!isCascadeBot(requestedReviewer, personas)) { logger.debug('Requested reviewer is not a CASCADE persona, skipping', { prNumber, requestedReviewer, - personas: ctx.personaIdentities, + personas, }); - return null; + return skip( + this.name, + `Review request on PR #${prNumber} is for ${requestedReviewer}, not a cascade persona — not auto-triggering`, + ); } // Resolve work item from DB @@ -91,7 +100,10 @@ export class ReviewRequestedTrigger implements TriggerHandler { // Human-initiated review requests override any prior automated dispatch claim. releaseReviewDispatch(reviewDispatchKey); if (!claimReviewDispatch(reviewDispatchKey, this.name, { prNumber, headSha })) { - return null; + return skip( + this.name, + `Review dispatch for PR #${prNumber}@${headSha} already claimed by another path (dedup)`, + ); } logger.info('Review requested from CASCADE persona, triggering review agent', { diff --git a/src/triggers/shared/gates.ts b/src/triggers/shared/gates.ts new file mode 100644 index 00000000..2fd9880f --- /dev/null +++ b/src/triggers/shared/gates.ts @@ -0,0 +1,185 @@ +import type { PersonaIdentities } from '../../github/personas.js'; +import { isCascadeBot } from '../../github/personas.js'; +import type { ProjectConfig, TriggerResult } from '../../types/index.js'; +import { logger } from '../../utils/logging.js'; +import { skip } from './skip.js'; +import { checkTriggerEnabled } from './trigger-check.js'; + +/** + * Composable self-skip gates for trigger handlers. + * + * Each gate is a pure function that returns: + * - `null` — gate passed; handler continues + * - `TriggerResult` (built via `skip()`) — gate failed; handler should + * propagate the structured skip up + * + * Compose synchronous gates with the nullish-coalescing operator for the + * common "fail on first failed gate" pattern: + * + * ```ts + * const skipped = + * gateCascadePersona(login, prNumber, personas, this.name) ?? + * gateBaseBranch(prDetails.baseRef, prNumber, ctx.project, this.name); + * if (skipped) return skipped; + * ``` + * + * Async gates (e.g. database / GitHub API checks) return Promises and must + * be awaited individually. Sequence them with explicit early returns rather + * than parallel evaluation when a downstream check would be wasted on a + * skip. + */ + +/** + * Async gate: is the trigger enabled in this project's `agent_trigger_configs`? + * + * Wraps `checkTriggerEnabled` and converts a `false` result into a + * structured skip. Eliminates the repeated `if (!await checkTriggerEnabled(...)) return null;` + * pattern across every handler. + */ +export async function gateTriggerEnabled( + projectId: string, + agentType: string, + triggerEvent: string, + handlerName: string, +): Promise { + const enabled = await checkTriggerEnabled(projectId, agentType, triggerEvent, handlerName); + if (!enabled) { + return skip(handlerName, `${agentType} trigger is disabled for this project`); + } + return null; +} + +/** + * Sync gate: does the PR target the project's configured base branch? + * + * Used by `check-suite-failure`, `check-suite-success`, `pr-conflict-detected`. + * One helper, one log shape, one skip message. + */ +export function gateBaseBranch( + prBaseRef: string, + prNumber: number, + project: ProjectConfig, + handlerName: string, +): TriggerResult | null { + if (prBaseRef === project.baseBranch) return null; + logger.info(`PR targets non-base branch, skipping ${handlerName}`, { + prNumber, + baseRef: prBaseRef, + projectBaseBranch: project.baseBranch, + }); + return skip( + handlerName, + `PR #${prNumber} targets ${prBaseRef}, not project base branch ${project.baseBranch}`, + ); +} + +/** + * Sync gate: was the PR authored by a cascade persona (implementer OR reviewer)? + * + * Replaces three previous variants: + * - manual `=== implementer || === implementer[bot]` (now removed; was the + * too-narrow check that hid the ucho/PR#155 incident before being widened) + * - calling `isCascadeBot(...)` directly with bespoke skip messages + * + * Loop-prevention: respond-to-ci, resolve-conflicts, and similar handlers + * should only auto-fix on PRs the cascade bot personas authored — human PRs + * are owned by the human. + * + * Returns `null` on any failure-to-resolve case (no `personaIdentities` + * available) — those should be a SEPARATE skip via `gatePersonaIdentities`, + * with a more specific Sentry-tagged failure surface. This gate ASSUMES + * `personaIdentities` is defined; pair it with `gatePersonaIdentities` when + * the handler hasn't already early-returned on missing identities. + */ +export function gateCascadePersona( + prAuthorLogin: string, + prNumber: number, + personaIdentities: PersonaIdentities, + handlerName: string, +): TriggerResult | null { + if (isCascadeBot(prAuthorLogin, personaIdentities)) return null; + logger.info(`PR not authored by a cascade persona, skipping ${handlerName}`, { + prNumber, + prAuthor: prAuthorLogin, + }); + return skip( + handlerName, + `PR #${prNumber} not authored by a cascade persona (author: ${prAuthorLogin})`, + ); +} + +/** + * Sync gate: is the handler's per-PR attempt counter under the limit? + * + * Used by `check-suite-failure` (MAX_ATTEMPTS = 3) and + * `pr-conflict-detected` (MAX_ATTEMPTS = 2). Both maintained an in-memory + * `Map` and used the same counter-check shape. + * + * Returns the skip when the limit is hit; the handler is responsible for + * the side effect of posting a warning comment to the PR (kept handler-side + * because the comment text differs and is part of the handler's contract). + */ +export function gateAttemptLimit( + attempts: number, + maxAttempts: number, + prNumber: number, + handlerName: string, +): TriggerResult | null { + if (attempts < maxAttempts) return null; + logger.warn(`Max attempts reached for PR — skipping ${handlerName}`, { + prNumber, + attempts, + maxAttempts, + }); + return skip( + handlerName, + `Max auto-fix attempts (${maxAttempts}) reached for PR #${prNumber} — manual intervention required`, + ); +} + +/** + * Type-narrowing variant of a "require this value or skip" gate. + * + * - `{ ok: true, value: T }` → handler proceeds with the narrowed value + * - `{ ok: false, skip: TriggerResult }` → handler returns the structured skip + * + * Differs from the other `gateXxx` helpers because callers need the narrowed + * value AFTER the gate passes (TypeScript can't infer narrowing across a + * helper boundary that returns `TriggerResult | null`). + */ +export type RequireGateResult = { ok: true; value: T } | { ok: false; skip: TriggerResult }; + +/** + * Require persona identities to be available in the trigger context. + * + * Defends every gate that reads `personaIdentities` against the silent + * `undefined` case (where token resolution failed and the bare-catch in + * `resolvePersonaCached` left the field unset). `resolvePersonaCached` now + * Sentry-captures the failure (#1235); this gate's skip message refers to + * that earlier signal. + * + * **Replaces** the prior `gatePersonaIdentities` (which returned plain + * `TriggerResult | null` and forced callers to use `ctx.personaIdentities!` + * non-null assertions afterwards). Idiomatic usage: + * + * ```ts + * const personasResult = requirePersonaIdentities(ctx.personaIdentities, prNumber, this.name); + * if (!personasResult.ok) return personasResult.skip; + * const personas = personasResult.value; // narrowed PersonaIdentities + * ``` + */ +export function requirePersonaIdentities( + personaIdentities: PersonaIdentities | undefined, + prNumber: number | undefined, + handlerName: string, +): RequireGateResult { + if (personaIdentities) return { ok: true, value: personaIdentities }; + logger.info(`No persona identities available, skipping ${handlerName}`, { prNumber }); + return { + ok: false, + skip: skip( + handlerName, + 'Cascade persona identities could not be resolved (token / GitHub API issue)', + ), + }; +} diff --git a/src/triggers/shared/skip.ts b/src/triggers/shared/skip.ts new file mode 100644 index 00000000..34f2442e --- /dev/null +++ b/src/triggers/shared/skip.ts @@ -0,0 +1,21 @@ +import type { TriggerResult } from '../../types/index.js'; + +/** + * Build a structured self-skip result so the router's webhook log + * decisionReason surfaces the real reason a matched handler bailed (instead + * of the generic `'No trigger matched for event'` placeholder). The shape is + * defined by `TriggerResult.skipReason`; the webhook-processor at + * `src/router/webhook-processor.ts` promotes `skipReason.message` into the + * persisted webhook log when `agentType === null`. + * + * Single canonical source — replaces the duplicate `skip()` helpers that + * lived inside individual trigger files. Adding a new self-skip site means + * `import { skip } from '../shared/skip.js'` and one call. + */ +export function skip(handler: string, message: string): TriggerResult { + return { + agentType: null, + agentInput: {}, + skipReason: { handler, message }, + }; +} diff --git a/tests/helpers/triggerAssertions.ts b/tests/helpers/triggerAssertions.ts new file mode 100644 index 00000000..700efcf6 --- /dev/null +++ b/tests/helpers/triggerAssertions.ts @@ -0,0 +1,56 @@ +import { expect } from 'vitest'; +import type { TriggerResult } from '../../src/types/index.js'; + +/** + * Assert that a TriggerResult is a structured self-skip — i.e. `agentType` + * is null and `skipReason` is populated. + * + * After the R1 refactor, every handler's self-skip path returns a + * structured skip via `skip()` instead of bare `null`. This helper keeps + * the per-handler assertions terse: + * + * ```ts + * expectSkip(result, 'check-suite-failure', /not authored by a cascade persona/); + * ``` + * + * - `expectedHandler` (optional): the trigger's `name` field; pinning it + * guards against the wrong handler claiming the skip. + * - `messageMatcher` (optional): substring or RegExp the skip message + * must match. Pin the specific case under test. + */ +export function expectSkip( + result: TriggerResult | null, + expectedHandler?: string, + messageMatcher?: string | RegExp, +): void { + expect(result, 'expected a structured skip TriggerResult, not null').not.toBeNull(); + expect(result?.agentType).toBeNull(); + expect(result?.skipReason).toBeDefined(); + if (expectedHandler !== undefined) { + expect(result?.skipReason?.handler).toBe(expectedHandler); + } + if (messageMatcher !== undefined) { + if (typeof messageMatcher === 'string') { + expect(result?.skipReason?.message).toContain(messageMatcher); + } else { + expect(result?.skipReason?.message).toMatch(messageMatcher); + } + } +} + +/** + * Bound variant: returns a partial-applied `expectSkip` where the handler + * name is pinned at the top of a test file. Lets per-handler test files + * keep terse assertion sites without redefining a one-line wrapper: + * + * ```ts + * const expectSkip = expectSkipFor('check-suite-failure'); + * expectSkip(result); // just shape + * expectSkip(result, /not authored by a cascade/); // shape + message + * ``` + */ +export function expectSkipFor( + handler: string, +): (result: TriggerResult | null, messageMatcher?: string | RegExp) => void { + return (result, messageMatcher) => expectSkip(result, handler, messageMatcher); +} diff --git a/tests/integration/github-personas.test.ts b/tests/integration/github-personas.test.ts index da39f07d..2eb12b5f 100644 --- a/tests/integration/github-personas.test.ts +++ b/tests/integration/github-personas.test.ts @@ -20,6 +20,7 @@ import { import { PRReviewSubmittedTrigger } from '../../src/triggers/github/pr-review-submitted.js'; import { ReviewRequestedTrigger } from '../../src/triggers/github/review-requested.js'; import type { TriggerContext } from '../../src/types/index.js'; +import { expectSkip } from '../helpers/triggerAssertions.js'; import { assertFound } from './helpers/assert.js'; import { truncateAll } from './helpers/db.js'; import { @@ -272,6 +273,15 @@ describe('GitHub Dual-Persona System (integration)', () => { config: {}, triggers: { prReviewSubmitted: true }, }); + // Agent + trigger configs must both be seeded so the + // gateTriggerEnabled gate passes and we can exercise the + // loop-prevention persona check. + await seedAgentConfig({ agentType: 'respond-to-review' }); + await seedTriggerConfig({ + agentType: 'respond-to-review', + triggerEvent: 'scm:pr-review-submitted', + enabled: true, + }); const project = await findProjectByRepoFromDb('owner/repo'); expect(project).toBeDefined(); @@ -290,11 +300,11 @@ describe('GitHub Dual-Persona System (integration)', () => { personaIdentities: TEST_PERSONAS, }; - // matches() returns true (persona checks are in handle()), but handle() returns null - // because the implementer is not the reviewer persona + // matches() returns true (persona checks are in handle()), but handle() + // returns a structured skip because the implementer is not the reviewer persona expect(trigger.matches(ctxFromImpl)).toBe(true); const result = await trigger.handle(ctxFromImpl); - expect(result).toBeNull(); + expectSkip(result, 'pr-review-submitted', /not the reviewer persona/); }); it('skips approved reviews (only changes_requested triggers respond-to-review)', async () => { @@ -354,7 +364,7 @@ describe('GitHub Dual-Persona System (integration)', () => { // scm:review-requested has defaultEnabled: false in definition expect(trigger.matches(ctx)).toBe(true); const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result, 'review-requested', /trigger is disabled/); }); it('triggers review when enabled via DB and persona is requested', async () => { @@ -394,6 +404,7 @@ describe('GitHub Dual-Persona System (integration)', () => { provider: 'github', config: {}, }); + await seedAgentConfig({ agentType: 'review' }); await seedTriggerConfig({ agentType: 'review', triggerEvent: 'scm:review-requested', @@ -413,10 +424,10 @@ describe('GitHub Dual-Persona System (integration)', () => { }; // external-reviewer is not a known persona — matches() passes (persona check is in handle()) - // but handle() returns null because the requested reviewer isn't a CASCADE bot + // but handle() returns a structured skip because the requested reviewer isn't a CASCADE bot expect(trigger.matches(ctx)).toBe(true); const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result, 'review-requested', /not a cascade persona/); }); }); }); diff --git a/tests/unit/triggers/check-suite-failure.test.ts b/tests/unit/triggers/check-suite-failure.test.ts index c686c141..457ee183 100644 --- a/tests/unit/triggers/check-suite-failure.test.ts +++ b/tests/unit/triggers/check-suite-failure.test.ts @@ -27,27 +27,9 @@ 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); - } - } -} +import { expectSkipFor } from '../../helpers/triggerAssertions.js'; + +const expectSkip = expectSkipFor('check-suite-failure'); describe('CheckSuiteFailureTrigger', () => { const trigger = new CheckSuiteFailureTrigger(); diff --git a/tests/unit/triggers/check-suite-success.test.ts b/tests/unit/triggers/check-suite-success.test.ts index 54e6cfdf..ff6ceb7e 100644 --- a/tests/unit/triggers/check-suite-success.test.ts +++ b/tests/unit/triggers/check-suite-success.test.ts @@ -4,6 +4,9 @@ import { mockGitHubClientModule, mockTriggerCheckModule, } from '../../helpers/sharedMocks.js'; +import { expectSkipFor } from '../../helpers/triggerAssertions.js'; + +const expectSkip = expectSkipFor('check-suite-success'); vi.mock('../../../src/triggers/config-resolver.js', () => mockConfigResolverModule); @@ -262,7 +265,7 @@ describe('CheckSuiteSuccessTrigger', () => { const checkSuiteResult = await trigger.handle(checkSuiteContext); - expect(checkSuiteResult).toBeNull(); + expectSkip(checkSuiteResult); }); it('returns null when PR targets non-base branch', async () => { @@ -288,7 +291,7 @@ describe('CheckSuiteSuccessTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); expect(githubClient.getPRReviews).not.toHaveBeenCalled(); expect(githubClient.getCheckSuiteStatus).not.toHaveBeenCalled(); }); @@ -316,7 +319,7 @@ describe('CheckSuiteSuccessTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); expect(githubClient.getCheckSuiteStatus).not.toHaveBeenCalled(); }); @@ -342,7 +345,7 @@ describe('CheckSuiteSuccessTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); expect(githubClient.getCheckSuiteStatus).not.toHaveBeenCalled(); }); @@ -379,7 +382,7 @@ describe('CheckSuiteSuccessTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); expect(githubClient.getCheckSuiteStatus).not.toHaveBeenCalled(); }); @@ -462,7 +465,7 @@ describe('CheckSuiteSuccessTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); expect(githubClient.getCheckSuiteStatus).not.toHaveBeenCalled(); }); @@ -618,7 +621,7 @@ describe('CheckSuiteSuccessTrigger', () => { // Second call with same PR+SHA should be deduped const result2 = await trigger.handle(ctx); - expect(result2).toBeNull(); + expectSkip(result2, /already claimed by another path \(dedup\)/); }); it('onBlocked callback clears the dedup entry', async () => { @@ -809,7 +812,7 @@ describe('CheckSuiteSuccessTrigger', () => { }; const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); expect(checkTriggerEnabledWithParams).toHaveBeenCalledWith( 'test', 'review', @@ -877,7 +880,7 @@ describe('CheckSuiteSuccessTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); }); it('triggers when PR authored by reviewer persona and authorMode=own', async () => { @@ -939,7 +942,7 @@ describe('CheckSuiteSuccessTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); }); it('triggers for both authors when authorMode=all', async () => { diff --git a/tests/unit/triggers/github-pr-comment-mention.test.ts b/tests/unit/triggers/github-pr-comment-mention.test.ts index 457f675f..a3560212 100644 --- a/tests/unit/triggers/github-pr-comment-mention.test.ts +++ b/tests/unit/triggers/github-pr-comment-mention.test.ts @@ -1,4 +1,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { expectSkipFor } from '../../helpers/triggerAssertions.js'; + +const expectSkip = expectSkipFor('pr-comment-mention'); // Hoist mocks before imports const { mockGetPR, mockIsCascadeBot } = vi.hoisted(() => ({ @@ -203,7 +206,7 @@ describe('PRCommentMentionTrigger', () => { vi.mocked(checkTriggerEnabled).mockResolvedValueOnce(false); const result = await trigger.handle(buildCtx()); - expect(result).toBeNull(); + expectSkip(result); expect(checkTriggerEnabled).toHaveBeenCalledWith( 'test-project', 'respond-to-pr-comment', @@ -242,13 +245,13 @@ describe('PRCommentMentionTrigger', () => { }), ); - expect(result).toBeNull(); + expectSkip(result); }); it('returns null when persona identities are missing', async () => { const result = await trigger.handle(buildCtx({ noPersonaIdentities: true })); - expect(result).toBeNull(); + expectSkip(result); }); it('returns null when comment is from a bot persona', async () => { @@ -256,7 +259,7 @@ describe('PRCommentMentionTrigger', () => { const result = await trigger.handle(buildCtx()); - expect(result).toBeNull(); + expectSkip(result); }); it('fires with workItemId undefined when DB has no link', async () => { @@ -343,7 +346,7 @@ describe('PRCommentMentionTrigger', () => { }), ); - expect(result).toBeNull(); + expectSkip(result); }); it('returns null when comment author is a bot persona (inline)', async () => { @@ -351,7 +354,7 @@ describe('PRCommentMentionTrigger', () => { const result = await trigger.handle(buildCtx({ payload: buildReviewCommentPayload() })); - expect(result).toBeNull(); + expectSkip(result); }); it('includes headSha from the review-comment payload (for post-checkout HEAD verification)', async () => { diff --git a/tests/unit/triggers/handler-shape.test.ts b/tests/unit/triggers/handler-shape.test.ts new file mode 100644 index 00000000..e8fab0c5 --- /dev/null +++ b/tests/unit/triggers/handler-shape.test.ts @@ -0,0 +1,91 @@ +import { readdirSync, readFileSync, statSync } from 'node:fs'; +import { join } from 'node:path'; +import { describe, expect, it } from 'vitest'; + +function listTsFiles(dir: string): string[] { + const out: string[] = []; + for (const name of readdirSync(dir)) { + const full = join(dir, name); + if (statSync(full).isDirectory()) { + out.push(...listTsFiles(full)); + } else if (name.endsWith('.ts') && !name.endsWith('.test.ts') && !name.endsWith('.d.ts')) { + out.push(full); + } + } + return out; +} + +/** + * Static-grep guard against re-introducing the duplicate `skip()` helpers + * that lived inside `check-suite-failure.ts` and `pr-conflict-detected.ts` + * (both literal copy-pastes of the same body) before the R1 refactor + * consolidated them into `src/triggers/shared/skip.ts`. + * + * Future regressions where someone adds a local `function skip(...)` to a + * trigger handler — instead of importing from the shared module — fail CI + * with a precise file path. Same pattern as `trigger-event-consistency.test.ts` + * and `pm-router-adapter-pm-scope.test.ts`. + */ +describe('trigger handler shape — no local skip() helpers', () => { + const repoRoot = process.cwd(); + const allTriggerFiles = listTsFiles(join(repoRoot, 'src/triggers')); + const handlerFiles = allTriggerFiles.filter( + (f) => + !f.endsWith('/registry.ts') && + !f.endsWith('/index.ts') && + !f.endsWith('/types.ts') && + !f.endsWith('/builtins.ts') && + !f.endsWith('/config-resolver.ts') && + !f.includes('/shared/'), + ); + const githubHandlerFiles = allTriggerFiles.filter( + (f) => + f.includes('/github/') && + !f.endsWith('/utils.ts') && + !f.endsWith('/types.ts') && + !f.endsWith('/index.ts') && + !f.endsWith('/register.ts') && + !f.endsWith('/check-polling.ts'), + ); + + it('every trigger handler imports skip() from shared/skip.js, never defines it locally', () => { + const offenders: Array<{ file: string; matchedLine: string }> = []; + + for (const file of handlerFiles) { + const contents = readFileSync(file, 'utf8'); + // Match a top-level skip-builder declaration. Allowed shapes that + // the shared module owns are NOT in handler files (they live under + // src/triggers/shared/skip.ts which is excluded above). + const localDefinitionPattern = /^(?:export\s+)?function\s+skip\s*\(/m; + const match = contents.match(localDefinitionPattern); + if (match) { + offenders.push({ file, matchedLine: match[0].trim() }); + } + } + + expect( + offenders, + `Trigger handlers must NOT define a local 'function skip(' — import { skip } from '../shared/skip.js' instead. ` + + `Offending files:\n${offenders.map((o) => ` - ${o.file}: ${o.matchedLine}`).join('\n')}`, + ).toEqual([]); + }); + + it('every GitHub trigger handler that returns a self-skip imports the shared skip helper', () => { + // Looser check: any GitHub handler that has at least one `return skip(` + // call must import `skip` from the shared module. This catches the case + // where someone copy-pastes a `skip()` call but forgets to wire it to + // the shared import (would otherwise produce a TS reference error). + const offenders: string[] = []; + for (const file of githubHandlerFiles) { + const contents = readFileSync(file, 'utf8'); + const usesSkip = /\breturn\s+skip\s*\(/.test(contents); + const importsSkip = /from ['"]\.\.\/shared\/skip\.js['"]/.test(contents); + if (usesSkip && !importsSkip) offenders.push(file); + } + + expect( + offenders, + `Files using 'return skip(' but not importing from '../shared/skip.js':\n${offenders.map((f) => ` - ${f}`).join('\n')}`, + ).toEqual([]); + }); +}); diff --git a/tests/unit/triggers/pr-merged.test.ts b/tests/unit/triggers/pr-merged.test.ts index 2bf61481..a4615ca7 100644 --- a/tests/unit/triggers/pr-merged.test.ts +++ b/tests/unit/triggers/pr-merged.test.ts @@ -1,4 +1,8 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { expectSkipFor } from '../../helpers/triggerAssertions.js'; + +const expectSkip = expectSkipFor('pr-merged'); + import { mockAcknowledgmentsModule, mockConfigProvider, @@ -167,7 +171,7 @@ describe('PRMergedTrigger', () => { }; const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); expect(isLifecycleTriggerEnabled).toHaveBeenCalledWith('test', 'prMerged', 'pr-merged'); }); @@ -257,7 +261,7 @@ describe('PRMergedTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); expect(mockProvider.moveWorkItem).not.toHaveBeenCalled(); }); @@ -292,7 +296,7 @@ describe('PRMergedTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); expect(mockProvider.moveWorkItem).not.toHaveBeenCalled(); }); @@ -661,7 +665,7 @@ describe('PRMergedTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); expect(mockProvider.moveWorkItem).not.toHaveBeenCalled(); }); diff --git a/tests/unit/triggers/pr-opened.test.ts b/tests/unit/triggers/pr-opened.test.ts index 35f67b7d..dcaa0e06 100644 --- a/tests/unit/triggers/pr-opened.test.ts +++ b/tests/unit/triggers/pr-opened.test.ts @@ -1,5 +1,8 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { mockConfigResolverModule, mockTriggerCheckModule } from '../../helpers/sharedMocks.js'; +import { expectSkipFor } from '../../helpers/triggerAssertions.js'; + +const expectSkip = expectSkipFor('pr-opened'); vi.mock('../../../src/triggers/config-resolver.js', () => mockConfigResolverModule); @@ -244,7 +247,7 @@ describe('PROpenedTrigger', () => { }, }; - expect(await trigger.handle(ctx)).toBeNull(); + expectSkip(await trigger.handle(ctx)); expect(checkTriggerEnabledWithParams).toHaveBeenCalledWith( 'test', 'review', @@ -282,7 +285,7 @@ describe('PROpenedTrigger', () => { }, }; - expect(await trigger.handle(ctx)).toBeNull(); + expectSkip(await trigger.handle(ctx)); }); it('fires for implementer PR when authorMode=own', async () => { @@ -348,7 +351,7 @@ describe('PROpenedTrigger', () => { }, }; - expect(await trigger.handle(ctx)).toBeNull(); + expectSkip(await trigger.handle(ctx)); }); it('returns null for implementer [bot] variant when authorMode=external', async () => { @@ -380,7 +383,7 @@ describe('PROpenedTrigger', () => { }, }; - expect(await trigger.handle(ctx)).toBeNull(); + expectSkip(await trigger.handle(ctx)); }); it('fires for external PR when authorMode=external', async () => { @@ -447,7 +450,7 @@ describe('PROpenedTrigger', () => { }; const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); }); it('fires for reviewer persona PR when authorMode=own', async () => { @@ -564,7 +567,7 @@ describe('PROpenedTrigger', () => { }, }; - expect(await trigger.handle(ctx)).toBeNull(); + expectSkip(await trigger.handle(ctx)); }); it('fires with undefined workItemId when DB has no link', async () => { diff --git a/tests/unit/triggers/pr-ready-to-merge.test.ts b/tests/unit/triggers/pr-ready-to-merge.test.ts index 1656e192..22eed418 100644 --- a/tests/unit/triggers/pr-ready-to-merge.test.ts +++ b/tests/unit/triggers/pr-ready-to-merge.test.ts @@ -1,4 +1,8 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { expectSkipFor } from '../../helpers/triggerAssertions.js'; + +const expectSkip = expectSkipFor('pr-ready-to-merge'); + import { mockAcknowledgmentsModule, mockConfigProvider, @@ -213,7 +217,7 @@ describe('PRReadyToMergeTrigger', () => { }; const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); expect(isLifecycleTriggerEnabled).toHaveBeenCalledWith( 'test', 'prReadyToMerge', @@ -350,7 +354,7 @@ describe('PRReadyToMergeTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); expect(mockProvider.moveWorkItem).not.toHaveBeenCalled(); }); @@ -382,7 +386,7 @@ describe('PRReadyToMergeTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); expect(mockProvider.moveWorkItem).not.toHaveBeenCalled(); }); @@ -421,7 +425,7 @@ describe('PRReadyToMergeTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); }); it('returns null when there are outstanding change requests', async () => { @@ -467,7 +471,7 @@ describe('PRReadyToMergeTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); }); it('skips move and comment when card is already in DONE list', async () => { @@ -575,7 +579,7 @@ describe('PRReadyToMergeTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); expect(mockProvider.moveWorkItem).not.toHaveBeenCalled(); }); }); @@ -760,7 +764,7 @@ describe('PRReadyToMergeTrigger', () => { 'abc123', expect.stringContaining('No DONE status configured'), ); - expect(result).toBeNull(); + expectSkip(result); }); it('falls back to DONE when auto label present but no MERGED status configured', async () => { @@ -844,7 +848,7 @@ describe('PRReadyToMergeTrigger', () => { 'abc123', expect.stringContaining('no MERGED or DONE status configured'), ); - expect(result).toBeNull(); + expectSkip(result); }); }); }); diff --git a/tests/unit/triggers/pr-review-submitted.test.ts b/tests/unit/triggers/pr-review-submitted.test.ts index dcc49bd3..d60081ed 100644 --- a/tests/unit/triggers/pr-review-submitted.test.ts +++ b/tests/unit/triggers/pr-review-submitted.test.ts @@ -1,5 +1,8 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { mockConfigResolverModule, mockTriggerCheckModule } from '../../helpers/sharedMocks.js'; +import { expectSkipFor } from '../../helpers/triggerAssertions.js'; + +const expectSkip = expectSkipFor('pr-review-submitted'); vi.mock('../../../src/triggers/config-resolver.js', () => mockConfigResolverModule); @@ -138,7 +141,7 @@ describe('PRReviewSubmittedTrigger', () => { }; const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); expect(checkTriggerEnabled).toHaveBeenCalledWith( 'test', 'respond-to-review', @@ -196,7 +199,7 @@ describe('PRReviewSubmittedTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); }); it('returns null for review from human user', async () => { @@ -217,7 +220,7 @@ describe('PRReviewSubmittedTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); }); it('returns null when no persona identities available', async () => { @@ -229,7 +232,7 @@ describe('PRReviewSubmittedTrigger', () => { const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); }); it('fires without work item when PR has no work item reference', async () => { diff --git a/tests/unit/triggers/review-requested.test.ts b/tests/unit/triggers/review-requested.test.ts index 6998a891..67b7e7ff 100644 --- a/tests/unit/triggers/review-requested.test.ts +++ b/tests/unit/triggers/review-requested.test.ts @@ -1,5 +1,8 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { mockConfigResolverModule, mockTriggerCheckModule } from '../../helpers/sharedMocks.js'; +import { expectSkipFor } from '../../helpers/triggerAssertions.js'; + +const expectSkip = expectSkipFor('review-requested'); vi.mock('../../../src/triggers/config-resolver.js', () => mockConfigResolverModule); @@ -102,7 +105,7 @@ describe('ReviewRequestedTrigger', () => { // no personaIdentities }; const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); }); it('returns null when sender is the implementer persona (loop prevention)', async () => { @@ -116,7 +119,7 @@ describe('ReviewRequestedTrigger', () => { personaIdentities: mockPersonaIdentities, }; const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); }); it('returns null when sender is the reviewer persona (loop prevention)', async () => { @@ -130,7 +133,7 @@ describe('ReviewRequestedTrigger', () => { personaIdentities: mockPersonaIdentities, }; const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); }); it('returns null when a persona requests review from itself (loop prevention)', async () => { @@ -144,7 +147,7 @@ describe('ReviewRequestedTrigger', () => { personaIdentities: mockPersonaIdentities, }; const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); }); it('returns null when requested reviewer is not a CASCADE persona', async () => { @@ -155,7 +158,7 @@ describe('ReviewRequestedTrigger', () => { personaIdentities: mockPersonaIdentities, }; const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); }); it('returns null when no requested reviewer in payload', async () => { @@ -169,7 +172,7 @@ describe('ReviewRequestedTrigger', () => { personaIdentities: mockPersonaIdentities, }; const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); }); it('fires without work item when PR has no work item reference', async () => { @@ -254,7 +257,7 @@ describe('ReviewRequestedTrigger', () => { personaIdentities: mockPersonaIdentities, }; const result = await trigger.handle(ctx); - expect(result).toBeNull(); + expectSkip(result); expect(checkTriggerEnabled).toHaveBeenCalledWith( 'test', 'review', diff --git a/tests/unit/triggers/shared/gates.test.ts b/tests/unit/triggers/shared/gates.test.ts new file mode 100644 index 00000000..669d1a4a --- /dev/null +++ b/tests/unit/triggers/shared/gates.test.ts @@ -0,0 +1,135 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { mockTriggerCheckModule } from '../../../helpers/sharedMocks.js'; + +vi.mock('../../../../src/triggers/shared/trigger-check.js', () => mockTriggerCheckModule); + +import type { PersonaIdentities } from '../../../../src/github/personas.js'; +import { + gateAttemptLimit, + gateBaseBranch, + gateCascadePersona, + gateTriggerEnabled, + requirePersonaIdentities, +} from '../../../../src/triggers/shared/gates.js'; +import { checkTriggerEnabled } from '../../../../src/triggers/shared/trigger-check.js'; +import type { ProjectConfig } from '../../../../src/types/index.js'; +import { createMockProject } from '../../../helpers/factories.js'; + +const mockProject: ProjectConfig = createMockProject(); + +const mockPersonas: PersonaIdentities = { + implementer: 'cascade-impl', + reviewer: 'cascade-rev', +}; + +describe('gateTriggerEnabled', () => { + beforeEach(() => { + vi.mocked(checkTriggerEnabled).mockReset(); + }); + + it('returns null when checkTriggerEnabled resolves true', async () => { + vi.mocked(checkTriggerEnabled).mockResolvedValue(true); + const result = await gateTriggerEnabled( + 'proj', + 'respond-to-ci', + 'scm:check-suite-failure', + 'check-suite-failure', + ); + expect(result).toBeNull(); + }); + + it('returns a structured skip when checkTriggerEnabled resolves false', async () => { + vi.mocked(checkTriggerEnabled).mockResolvedValue(false); + const result = await gateTriggerEnabled( + 'proj', + 'respond-to-ci', + 'scm:check-suite-failure', + 'check-suite-failure', + ); + expect(result?.agentType).toBeNull(); + expect(result?.skipReason).toEqual({ + handler: 'check-suite-failure', + message: 'respond-to-ci trigger is disabled for this project', + }); + }); +}); + +describe('gateBaseBranch', () => { + it('returns null when prBaseRef matches project.baseBranch', () => { + expect(gateBaseBranch('main', 42, mockProject, 'check-suite-failure')).toBeNull(); + }); + + it('returns a structured skip when baseRef does not match', () => { + const result = gateBaseBranch('develop', 42, mockProject, 'check-suite-failure'); + expect(result?.skipReason?.handler).toBe('check-suite-failure'); + expect(result?.skipReason?.message).toContain( + 'PR #42 targets develop, not project base branch main', + ); + }); +}); + +describe('gateCascadePersona', () => { + it('returns null when prAuthor matches the implementer persona', () => { + expect(gateCascadePersona('cascade-impl', 42, mockPersonas, 'check-suite-failure')).toBeNull(); + }); + + it('returns null when prAuthor matches the reviewer persona', () => { + // User-confirmed widening from the 2026-04-29 incident: respond-to-ci + // must fire for either persona. + expect(gateCascadePersona('cascade-rev', 42, mockPersonas, 'check-suite-failure')).toBeNull(); + }); + + it('returns null when prAuthor is the [bot]-suffix variant of either persona', () => { + expect(gateCascadePersona('cascade-impl[bot]', 42, mockPersonas, 'h')).toBeNull(); + expect(gateCascadePersona('cascade-rev[bot]', 42, mockPersonas, 'h')).toBeNull(); + }); + + it('returns a structured skip when prAuthor is a non-cascade human', () => { + const result = gateCascadePersona('some-human', 42, mockPersonas, 'check-suite-failure'); + expect(result?.skipReason?.handler).toBe('check-suite-failure'); + expect(result?.skipReason?.message).toMatch( + /PR #42 not authored by a cascade persona.*some-human/, + ); + }); +}); + +describe('gateAttemptLimit', () => { + it('returns null when attempts is below the limit', () => { + expect(gateAttemptLimit(0, 3, 42, 'h')).toBeNull(); + expect(gateAttemptLimit(2, 3, 42, 'h')).toBeNull(); + }); + + it('returns a structured skip when attempts >= the limit', () => { + const result = gateAttemptLimit(3, 3, 42, 'check-suite-failure'); + expect(result?.skipReason?.handler).toBe('check-suite-failure'); + expect(result?.skipReason?.message).toMatch(/Max auto-fix attempts \(3\) reached for PR #42/); + }); +}); + +describe('requirePersonaIdentities', () => { + it('returns ok=true with the narrowed value when personaIdentities is defined', () => { + const result = requirePersonaIdentities(mockPersonas, 42, 'h'); + expect(result.ok).toBe(true); + if (result.ok) { + // TypeScript narrows result.value to PersonaIdentities here. + expect(result.value).toBe(mockPersonas); + } + }); + + it('returns ok=false with a structured skip when personaIdentities is undefined', () => { + const result = requirePersonaIdentities(undefined, 42, 'check-suite-failure'); + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.skip.skipReason?.handler).toBe('check-suite-failure'); + expect(result.skip.skipReason?.message).toMatch(/persona identities could not be resolved/); + } + }); + + it('returns ok=false even when prNumber is undefined', () => { + const result = requirePersonaIdentities(undefined, undefined, 'pr-comment-mention'); + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.skip.skipReason?.handler).toBe('pr-comment-mention'); + } + }); +}); diff --git a/tests/unit/triggers/shared/skip.test.ts b/tests/unit/triggers/shared/skip.test.ts new file mode 100644 index 00000000..2c4bb1c7 --- /dev/null +++ b/tests/unit/triggers/shared/skip.test.ts @@ -0,0 +1,21 @@ +import { describe, expect, it } from 'vitest'; +import { skip } from '../../../../src/triggers/shared/skip.js'; + +describe('skip()', () => { + it('builds a TriggerResult with structured skipReason', () => { + const result = skip('check-suite-failure', 'PR not authored by a cascade persona'); + expect(result).toEqual({ + agentType: null, + agentInput: {}, + skipReason: { + handler: 'check-suite-failure', + message: 'PR not authored by a cascade persona', + }, + }); + }); + + it('always sets agentType to null (so webhook-processor takes the skip branch)', () => { + const result = skip('pr-conflict-detected', 'reason'); + expect(result.agentType).toBeNull(); + }); +});