Skip to content

refactor(triggers): consolidate handler boilerplate via shared skip + gates #1237

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zbigniewsobiecki merged 4 commits into from
Apr 29, 2026
Merged

refactor(triggers): consolidate handler boilerplate via shared skip + gates #1237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5284544
Merge pull request #1232 from mongrel-intelligence/dev
zbigniewsobiecki Apr 29, 2026
24e1e4d
Merge pull request #1234 from mongrel-intelligence/dev
zbigniewsobiecki Apr 29, 2026
e13f090
Merge pull request #1236 from mongrel-intelligence/dev
zbigniewsobiecki Apr 29, 2026
6ba5d79
refactor(triggers): consolidate handler boilerplate via shared skip +…
zbigniewsobiecki Apr 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 30 additions & 68 deletions src/triggers/github/check-suite-failure.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -80,17 +73,13 @@ export class CheckSuiteFailureTrigger implements TriggerHandler {
}

async handle(ctx: TriggerContext): Promise<TriggerResult | null> {
// 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;
Copy link
Copy Markdown
Collaborator

@nhopeatall nhopeatall Apr 29, 2026

Choose a reason for hiding this comment

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

[NITPICK] The variable name enabled is confusing here — when truthy, it means the trigger is not enabled (the value is a skip result). Other gate call sites in this PR use clearer names like gateChainSkip, baseSkip, limitSkip. Consider disabledSkip or triggerSkip for readability.


const payload = ctx.payload as GitHubCheckSuitePayload;
const { owner, repo } = parseRepoFullName(payload.repository.full_name);
Expand All @@ -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);
Expand Down Expand Up @@ -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
Expand Down
37 changes: 22 additions & 15 deletions src/triggers/github/check-suite-success.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand All @@ -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;
}
Expand All @@ -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', {
Expand All @@ -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);
Expand Down Expand Up @@ -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,
Expand All @@ -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.
Expand Down
56 changes: 36 additions & 20 deletions src/triggers/github/pr-comment-mention.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -35,26 +36,30 @@ export class PRCommentMentionTrigger implements TriggerHandler {
}

async handle(ctx: TriggerContext): Promise<TriggerResult | null> {
// 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;
Expand Down Expand Up @@ -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
Expand Down
Loading
Loading