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(); + }); +});