From 55baf8e5c3f650c2188ee108500b1ffdb5e93810 Mon Sep 17 00:00:00 2001 From: Zbigniew Sobiecki Date: Mon, 27 Apr 2026 10:37:53 +0000 Subject: [PATCH] fix(triggers): correct pr-conflict-detected gating event so resolve-conflicts actually fires MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gating call passed `'scm:conflict-resolution'` to `checkTriggerEnabled`, but every other surface (the agent YAML, the trigger-types registry the dashboard / CLI write through, and the handler's own emitted `triggerEvent` on AgentInput) uses `'scm:pr-conflict-detected'`. So `isTriggerEnabled('resolve-conflicts', 'scm:conflict-resolution')` always missed the operator's enabled DB row, fell back to the agent definition's `defaultEnabled: false`, and silently returned null on every `pull_request synchronize` webhook. Confirmed via `cascade webhooklogs` on 2026-04-27: 6+ synchronize events for ucho on PRs #81 and #85 in an 11-minute window each returned `decisionReason: "No trigger matched for event"` despite `cascade projects trigger-list ucho` showing `resolve-conflicts | scm:pr-conflict-detected | enabled=yes`. The trigger handler's `matches()` returned true; only the gating string was wrong. Conflict detection has been silently broken in production for every project that "enabled" it via the dashboard, since the same commit that standardized the emitted `triggerEvent` to `'scm:pr-conflict-detected'` everywhere else left this one gating call out of the rename. Fix: one-line string change. The existing handler test was codifying the bug (it asserted `checkTriggerEnabled` was called with `'scm:conflict-resolution'`) and is updated to pin the correct value. Audit: cross-checked all 20 `checkTriggerEnabled` / `checkTriggerEnabledWithParams` call sites in `src/triggers/` against each handler's emitted `triggerEvent`. This was the only mismatch. Guard: added `tests/unit/triggers/trigger-event-consistency.test.ts` — a static-analysis test that scans every trigger handler file under `src/triggers/`, extracts the gating event string from each `checkTriggerEnabled(...)` call and the emitted `triggerEvent: '...'` property, and asserts every gating event appears in the file's emitted set. The test fails loudly with a precise file:line reference on any future drift, listing the actual events found and pointing the contributor at either fixing the mismatch or adding an explicit `EXEMPT_FILES` entry with a reason. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/triggers/github/pr-conflict-detected.ts | 2 +- .../triggers/pr-conflict-detected.test.ts | 2 +- .../trigger-event-consistency.test.ts | 124 ++++++++++++++++++ 3 files changed, 126 insertions(+), 2 deletions(-) create mode 100644 tests/unit/triggers/trigger-event-consistency.test.ts diff --git a/src/triggers/github/pr-conflict-detected.ts b/src/triggers/github/pr-conflict-detected.ts index 4702b40b..0fbcd387 100644 --- a/src/triggers/github/pr-conflict-detected.ts +++ b/src/triggers/github/pr-conflict-detected.ts @@ -40,7 +40,7 @@ export class PRConflictDetectedTrigger implements TriggerHandler { !(await checkTriggerEnabled( ctx.project.id, 'resolve-conflicts', - 'scm:conflict-resolution', + 'scm:pr-conflict-detected', this.name, )) ) { diff --git a/tests/unit/triggers/pr-conflict-detected.test.ts b/tests/unit/triggers/pr-conflict-detected.test.ts index 684b03c4..25be084e 100644 --- a/tests/unit/triggers/pr-conflict-detected.test.ts +++ b/tests/unit/triggers/pr-conflict-detected.test.ts @@ -114,7 +114,7 @@ describe('PRConflictDetectedTrigger', () => { expect(checkTriggerEnabled).toHaveBeenCalledWith( 'test', 'resolve-conflicts', - 'scm:conflict-resolution', + 'scm:pr-conflict-detected', 'pr-conflict-detected', ); }); diff --git a/tests/unit/triggers/trigger-event-consistency.test.ts b/tests/unit/triggers/trigger-event-consistency.test.ts new file mode 100644 index 00000000..d078fb0f --- /dev/null +++ b/tests/unit/triggers/trigger-event-consistency.test.ts @@ -0,0 +1,124 @@ +/** + * Static-analysis guard against the trigger-event-string drift bug class. + * + * Every TriggerHandler that calls `checkTriggerEnabled(projectId, agentType, + * , ...)` MUST use the same `` string that it later emits as + * `triggerEvent: ''` on the `AgentInput` it returns. If they drift, + * the gating check looks up a DB row that doesn't exist (operators write + * the EMITTED event when they enable the trigger), falls back to the agent + * YAML's `defaultEnabled` (typically `false`), and the trigger silently + * never fires in production. + * + * Live incident: `src/triggers/github/pr-conflict-detected.ts` had this + * exact mismatch — gated on `'scm:conflict-resolution'` while emitting + * `triggerEvent: 'scm:pr-conflict-detected'`. Conflict detection was + * silently disabled in prod for every project that "enabled" it via the + * dashboard. Confirmed via `cascade webhooklogs` on 2026-04-27: every + * `pull_request synchronize` event for ucho returned "No trigger matched + * for event" instead of queuing `resolve-conflicts`. + * + * This test parses every trigger-handler file under `src/triggers/` and + * asserts the invariant. It does NOT run the handlers; it's a static + * grep-style check against the file source so it stays cheap and fires + * loudly with a precise file:line reference when violated. + */ +import { readdirSync, readFileSync, statSync } from 'node:fs'; +import { join } from 'node:path'; +import { describe, expect, it } from 'vitest'; + +const TRIGGERS_ROOT = join(__dirname, '..', '..', '..', 'src', 'triggers'); + +// Handlers that legitimately gate on one event without emitting it as a +// `triggerEvent`. Add an entry ONLY when there's a real reason — every +// exemption silently weakens the guard. +const EXEMPT_FILES = new Set([ + // Auto-chain dispatcher: gates on a synthetic 'internal:auto-chain' + // event but does emit it as triggerEvent at line 772 — passes the check + // without exemption. Listed here only as documentation. +]); + +interface HandlerScan { + file: string; + gatingEvents: Set; + emittedEvents: Set; +} + +function listHandlerFiles(dir: string): string[] { + const out: string[] = []; + for (const entry of readdirSync(dir)) { + const full = join(dir, entry); + if (statSync(full).isDirectory()) { + out.push(...listHandlerFiles(full)); + } else if (entry.endsWith('.ts') && !entry.endsWith('.test.ts')) { + out.push(full); + } + } + return out; +} + +function scanHandler(file: string): HandlerScan { + const src = readFileSync(file, 'utf-8'); + const gatingEvents = new Set(); + const emittedEvents = new Set(); + + // Match `checkTriggerEnabled(...)` and `checkTriggerEnabledWithParams(...)`. + // Captures the third argument (event string literal). Permits arbitrary + // whitespace/newlines and any agentType expression in slot 2. + // + // Examples matched: + // checkTriggerEnabled(ctx.project.id, 'review', 'scm:pr-opened', this.name) + // checkTriggerEnabled(ctx.project.id, agentType, 'pm:label-added', this.name) + // await checkTriggerEnabledWithParams( + // ctx.project.id, + // 'alerting', + // 'alerting:issue-alert', + // this.name, + // ); + const gatingPattern = + /checkTriggerEnabled(?:WithParams)?\s*\(\s*[^,]+,\s*[^,]+,\s*['"]([^'"]+)['"]/g; + for (const m of src.matchAll(gatingPattern)) { + gatingEvents.add(m[1]); + } + + // Match `triggerEvent: ''` in object literals (the AgentInput emission). + const emittedPattern = /triggerEvent\s*:\s*['"]([^'"]+)['"]/g; + for (const m of src.matchAll(emittedPattern)) { + emittedEvents.add(m[1]); + } + + return { file, gatingEvents, emittedEvents }; +} + +describe('trigger-event-string consistency (static guard)', () => { + const allFiles = listHandlerFiles(TRIGGERS_ROOT).filter( + (f) => !f.endsWith('/trigger-check.ts') && !EXEMPT_FILES.has(f), + ); + const scans = allFiles + .map(scanHandler) + .filter((s) => s.gatingEvents.size > 0 || s.emittedEvents.size > 0); + + it('finds at least one trigger handler to scan (sanity)', () => { + expect(scans.length).toBeGreaterThan(10); + }); + + for (const scan of scans) { + const relPath = scan.file.replace(`${TRIGGERS_ROOT}/`, 'src/triggers/'); + + it(`${relPath}: every gating event is also emitted as triggerEvent`, () => { + for (const gating of scan.gatingEvents) { + expect( + scan.emittedEvents.has(gating), + `Handler ${relPath} calls checkTriggerEnabled(..., '${gating}', ...) but never emits ` + + `triggerEvent: '${gating}'. ` + + `Emitted events in this file: [${[...scan.emittedEvents].join(', ') || '(none)'}]. ` + + `This silently disables the trigger in production: operators write the EMITTED ` + + `event when they enable it via the dashboard, the gating check looks up a DB row ` + + `that doesn't exist, falls back to the agent YAML's defaultEnabled (typically false), ` + + `and the trigger never fires. Either fix the gating event string to match the emitted ` + + `one, or — if the mismatch is intentional — add this file to EXEMPT_FILES with a ` + + `comment explaining why.`, + ).toBe(true); + } + }); + } +});