refactor(triggers): consolidate handler boilerplate via shared skip + gates#1237
refactor(triggers): consolidate handler boilerplate via shared skip + gates#1237zbigniewsobiecki merged 4 commits intodevfrom
Conversation
Merge dev → main: Fix 1 (BullMQ coalesce unique jobIds) + Fix 2 (snapshot rmi on PR merge)
Merge dev → main: backlog-manager scope safety (#1233)
Merge dev → main: respond-to-ci diagnostics + persona-scope hardening (#1235)
… gates
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) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
nhopeatall
left a comment
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean, well-executed refactoring that consolidates duplicated trigger handler boilerplate into shared modules. Code is correct, gates are well-documented, static guard tests prevent regression, and the integration test fixes are genuine improvements (they were previously testing the wrong skip path).
Should Fix (non-blocking)
Log level downgrade for missing persona identities
requirePersonaIdentities in gates.ts:177 uses logger.info, but 3 of the 5 original call sites used logger.warn (pr-comment-mention, pr-review-submitted, review-requested). Missing persona identities signals a token resolution failure — while the Sentry capture happens upstream in resolvePersonaCached (#1235), warn is arguably the right log level at the gate since it represents an unexpected state (not a routine skip like base-branch mismatch). Consider logger.warn for consistency with the severity of the condition.
Nitpick
Misleading variable name at gateTriggerEnabled call sites
Every handler uses const enabled = await gateTriggerEnabled(...); if (enabled) return enabled; — when enabled is truthy, the trigger is not enabled (it is a skip result). Compare with the clearer names used at other gate call sites: gateChainSkip, baseSkip, limitSkip. A name like disabledSkip or triggerSkip would read more naturally. Not worth blocking given the convention is documented and consistent across all 6 call sites.
🕵️ claude-code · claude-opus-4-6 · run details
| handlerName: string, | ||
| ): RequireGateResult<PersonaIdentities> { | ||
| if (personaIdentities) return { ok: true, value: personaIdentities }; | ||
| logger.info(`No persona identities available, skipping ${handlerName}`, { prNumber }); |
There was a problem hiding this comment.
[SHOULD_FIX] This uses logger.info but three of the five original call sites (pr-comment-mention, pr-review-submitted, review-requested) used logger.warn. Missing persona identities is an unexpected state (token resolution failure), not a routine skip — warn would better match the severity. The other two call sites (check-suite-failure, pr-conflict-detected) already used info, so this is a judgment call, but the warn-level sites had the right instinct.
| 'scm:check-suite-failure', | ||
| this.name, | ||
| ); | ||
| if (enabled) return enabled; |
There was a problem hiding this comment.
[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.
Merge dev → main: refactor(triggers) consolidate handler boilerplate (#1237)
2 participants
Summary
Past 3 months produced 14+ bug fixes in the trigger/webhook/dispatch surface, clustering into 5 recurring classes (skip opacity, dedup collisions, ALS scope leakage, racing writers, gate divergence). Two GitHub handlers had literal copies of the same
skip(handler, message)helper; ~30 otherreturn nullself-skip sites still produced the generic'No trigger matched for event'decisionReason despite the structuredskipReasonplumbing that landed in #1235.R1 of the refactor plan from
/home/zbigniew/.claude/plans/elegant-dazzling-rabbit.md. Pure consolidation — no cross-layer contract change.What's new
Shared modules:
src/triggers/shared/skip.ts— single canonicalskip()builder (replaces the duplicated literal helpers).src/triggers/shared/gates.ts— composable pure-function gates:gateTriggerEnabled,gateBaseBranch,gateCascadePersona,gateAttemptLimit. PlusrequirePersonaIdentities— a type-narrowing variant returning{ ok: true, value } | { ok: false, skip }so callers no longer needctx.personaIdentities!non-null assertions.tests/helpers/triggerAssertions.ts—expectSkip(result, handler, msg)+expectSkipFor(handler)factory.Static guard —
tests/unit/triggers/handler-shape.test.tswalks every handler undersrc/triggers/and asserts no file defines a localfunction skip(, and every GitHub handler usingreturn skip(...)imports from../shared/skip.js. Same pattern astrigger-event-consistency.test.ts+pm-router-adapter-pm-scope.test.ts.Handler refactors — 9 GitHub handlers (
check-suite-failure,check-suite-success,pr-comment-mention,pr-conflict-detected,pr-merged,pr-opened,pr-ready-to-merge,pr-review-submitted,review-requested) now compose gates from the shared module. Sync gate chains use??for short-circuit composition. ~30 barereturn nullself-skips converted to structured skips with case-specific messages (PR # included, attempt counts spelled out, base branches named).Bug-class impact
gateCascadePersona(which usesisCascadeBot).R2 (tagged-union dispatch decision) and R3 (GitHub-specific TriggerContext) remain pending.
Test plan
npm run lintclean (13 pre-existing warnings).npm run typecheckclean.shared/skip.test.ts+shared/gates.test.ts, 2 inhandler-shape.test.ts.github-personas.test.tsupdated to seed both agent + trigger configs (the test was previously asserting null returned from the disabled-trigger path, not the loop-prevention persona path; the new assertions exercise the actual claimed behavior).🤖 Generated with Claude Code