diff --git a/src/triggers/github/pr-comment-mention.ts b/src/triggers/github/pr-comment-mention.ts index 14642a43..cd1ee3c6 100644 --- a/src/triggers/github/pr-comment-mention.ts +++ b/src/triggers/github/pr-comment-mention.ts @@ -140,6 +140,7 @@ export class PRCommentMentionTrigger implements TriggerHandler { triggerCommentPath: commentPath, triggerCommentUrl: commentUrl, commentAuthor, + workItemId, }, prNumber, prUrl, diff --git a/src/triggers/github/pr-review-submitted.ts b/src/triggers/github/pr-review-submitted.ts index cf8e5fe8..4f2c2e37 100644 --- a/src/triggers/github/pr-review-submitted.ts +++ b/src/triggers/github/pr-review-submitted.ts @@ -79,6 +79,7 @@ export class PRReviewSubmittedTrigger implements TriggerHandler { triggerCommentBody: reviewPayload.review.body || `Review: ${reviewPayload.review.state}`, triggerCommentPath: '', // Reviews don't have a specific file path triggerCommentUrl: reviewPayload.review.html_url, + workItemId, }, prNumber, prUrl: reviewPayload.pull_request.html_url, diff --git a/src/triggers/shared/agent-execution.ts b/src/triggers/shared/agent-execution.ts index fa8cb100..fc935f50 100644 --- a/src/triggers/shared/agent-execution.ts +++ b/src/triggers/shared/agent-execution.ts @@ -497,11 +497,21 @@ export async function runAgentExecutionPipeline( // post-execution linkPRToWorkItem write. const workItemId = await resolveWorkItemId(result.workItemId, project.id, result.prNumber); - // If we recovered a workItemId the trigger didn't have, patch agentInput so - // the corrected value flows into runAgent and into the agent_runs row that - // tryCreateRun (src/agents/shared/runTracking.ts) writes. + // Patch agentInput.workItemId whenever it diverges from the resolved value. + // Two cases this catches: + // 1. Re-resolution recovered a workItemId the trigger didn't have at + // webhook-arrival time (the original motivation — see PROpenedTrigger). + // 2. The trigger set workItemId at the top level of its TriggerResult but + // forgot to include it inside `agentInput` (live incident: respond-to- + // review and respond-to-pr-comment, 2026-04-29 — 0/103 and 0/9 runs + // had a non-null work_item_id, hiding them from the dashboard's + // work-item page). The static guard at + // tests/unit/triggers/trigger-work-item-id-consistency.test.ts + // catches this at write-time; this runtime patch is the safety net. + // tryCreateRun (src/agents/shared/runTracking.ts) reads workItemId from + // agentInput when persisting agent_runs.work_item_id. const agentInput = - workItemId && workItemId !== result.workItemId + workItemId && (result.agentInput.workItemId as string | undefined) !== workItemId ? { ...result.agentInput, workItemId } : result.agentInput; diff --git a/tests/unit/triggers/github-pr-comment-mention.test.ts b/tests/unit/triggers/github-pr-comment-mention.test.ts index 2ea38bb7..457f675f 100644 --- a/tests/unit/triggers/github-pr-comment-mention.test.ts +++ b/tests/unit/triggers/github-pr-comment-mention.test.ts @@ -225,6 +225,16 @@ describe('PRCommentMentionTrigger', () => { expect(result?.agentInput.triggerCommentBody).toContain(`@${IMPLEMENTER_USERNAME}`); }); + it('propagates workItemId into agentInput so tryCreateRun persists it on agent_runs', async () => { + // Regression: see same test in pr-review-submitted.test.ts. tryCreateRun + // reads input.workItemId from agentInput, so omitting it leaves + // agent_runs.work_item_id NULL and hides the run from the work-item page. + const result = await trigger.handle(buildCtx()); + + expect(result?.workItemId).toBe(CARD_SHORT_ID); + expect(result?.agentInput.workItemId).toBe(CARD_SHORT_ID); + }); + it('returns null when no @mention of implementer', async () => { const result = await trigger.handle( buildCtx({ diff --git a/tests/unit/triggers/pr-review-submitted.test.ts b/tests/unit/triggers/pr-review-submitted.test.ts index 8f1817cb..dcc49bd3 100644 --- a/tests/unit/triggers/pr-review-submitted.test.ts +++ b/tests/unit/triggers/pr-review-submitted.test.ts @@ -169,6 +169,7 @@ describe('PRReviewSubmittedTrigger', () => { triggerCommentPath: '', triggerCommentUrl: 'https://github.com/owner/repo/pull/42#pullrequestreview-100', triggerEvent: 'scm:pr-review-submitted', + workItemId: 'abc123', }, prNumber: 42, prUrl: 'https://github.com/owner/repo/pull/42', @@ -306,6 +307,7 @@ describe('PRReviewSubmittedTrigger', () => { triggerCommentPath: '', triggerCommentUrl: 'https://github.com/owner/repo/pull/42#pullrequestreview-200', triggerEvent: 'scm:pr-review-submitted', + workItemId: 'abc123', }, prNumber: 42, prUrl: 'https://github.com/owner/repo/pull/42', @@ -314,6 +316,26 @@ describe('PRReviewSubmittedTrigger', () => { }); }); + it('propagates workItemId into agentInput so tryCreateRun persists it on agent_runs', async () => { + // Regression: respond-to-review runs were stored with NULL work_item_id + // because workItemId was set at the top-level only, not on agentInput. + // runTracking reads input.workItemId from agentInput; the dashboard's + // work-item page filters by agent_runs.work_item_id. Live incident: + // 4 respond-to-review runs for ucho/MNG-400 (PR #136) on 2026-04-28 + // were invisible on the work-item page despite firing successfully. + const ctx: TriggerContext = { + project: mockProject, + source: 'github', + payload: makeReviewPayload(), + personaIdentities: mockPersonaIdentities, + }; + + const result = await trigger.handle(ctx); + + expect(result?.workItemId).toBe('abc123'); + expect(result?.agentInput.workItemId).toBe('abc123'); + }); + it('uses Review: commented fallback when commented review has null body', async () => { const ctx: TriggerContext = { project: mockProject, diff --git a/tests/unit/triggers/trigger-work-item-id-consistency.test.ts b/tests/unit/triggers/trigger-work-item-id-consistency.test.ts new file mode 100644 index 00000000..17818571 --- /dev/null +++ b/tests/unit/triggers/trigger-work-item-id-consistency.test.ts @@ -0,0 +1,160 @@ +/** + * Static-analysis guard against the agentInput.workItemId-omission bug class. + * + * Every TriggerHandler that returns a TriggerResult dispatching a real agent + * (`agentType: ''` — not `null`) and sets a top-level `workItemId` + * MUST also include `workItemId` inside the same return's `agentInput` object. + * + * Why: tryCreateRun (src/agents/shared/runTracking.ts) reads `input.workItemId` + * from agentInput when persisting the run row to `agent_runs.work_item_id`. + * If the trigger sets workItemId at the top level only, the DB column stays + * NULL, and the dashboard's work-item page (which filters by that column) + * silently hides the run. The runtime patch in runAgentExecutionPipeline + * only repairs the field when the re-resolved value DIFFERS from the top-level + * value, so a trigger that already populates the top-level field bypasses + * the safety net entirely. + * + * Live incident: src/triggers/github/pr-review-submitted.ts and + * src/triggers/github/pr-comment-mention.ts both shipped this omission. + * Confirmed via `cascade runs list --project ucho` on 2026-04-29: 0/103 + * respond-to-review runs and 0/9 respond-to-pr-comment runs had a non-null + * work_item_id, while every other agent-type populated it correctly. Four + * respond-to-review runs for ucho/MNG-400 (PR #136) on 2026-04-28 were + * invisible on the work-item page despite firing successfully. + * + * This test parses every trigger-handler file under `src/triggers/` via the + * TypeScript AST and asserts the invariant. It does NOT execute the handlers; + * it inspects every `return { agentType: ..., agentInput: { ... }, workItemId, ... }` + * literal and fires loudly with a precise file:line reference when violated. + */ +import { readdirSync, readFileSync, statSync } from 'node:fs'; +import { join } from 'node:path'; +import ts from 'typescript'; +import { describe, expect, it } from 'vitest'; + +const TRIGGERS_ROOT = join(__dirname, '..', '..', '..', 'src', 'triggers'); + +interface Violation { + file: string; + line: number; + agentType: string; +} + +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 findProperty( + obj: ts.ObjectLiteralExpression, + name: string, +): ts.PropertyAssignment | ts.ShorthandPropertyAssignment | undefined { + for (const prop of obj.properties) { + const propName = prop.name && ts.isIdentifier(prop.name) ? prop.name.text : undefined; + if (propName !== name) continue; + if (ts.isPropertyAssignment(prop) || ts.isShorthandPropertyAssignment(prop)) return prop; + } + return undefined; +} + +function getAgentTypeLiteral(obj: ts.ObjectLiteralExpression): string | null { + const prop = findProperty(obj, 'agentType'); + if (!prop || !ts.isPropertyAssignment(prop)) return null; + const init = prop.initializer; + if (ts.isStringLiteral(init) || ts.isNoSubstitutionTemplateLiteral(init)) { + return init.text; + } + // Identifier (e.g. `agentType` shorthand from a variable) — can't statically + // confirm a literal value but it's still a real dispatch, so flag for audit. + if (ts.isIdentifier(init)) return init.text; + return null; +} + +/** + * Inspect a single object literal that is the argument of a `return` statement. + * Returns the agentType label if this return is a TriggerResult that violates + * the invariant; returns null otherwise. + */ +function inspectReturnObject(obj: ts.ObjectLiteralExpression): string | null { + const agentType = getAgentTypeLiteral(obj); + // Skip lifecycle-only returns (`agentType: null`) — they don't run an + // agent so workItemId is never persisted to agent_runs anyway. + if (!agentType) return null; + + const outerWorkItemId = findProperty(obj, 'workItemId'); + const agentInputProp = findProperty(obj, 'agentInput'); + if (!outerWorkItemId || !agentInputProp) return null; + + // agentInput must be an object literal we can inspect statically. Spreads + // or variable references are out of scope for this guard. + if ( + !ts.isPropertyAssignment(agentInputProp) || + !ts.isObjectLiteralExpression(agentInputProp.initializer) + ) { + return null; + } + + const innerWorkItemId = findProperty(agentInputProp.initializer, 'workItemId'); + return innerWorkItemId ? null : agentType; +} + +function scanFile(file: string): Violation[] { + const src = readFileSync(file, 'utf-8'); + const sf = ts.createSourceFile(file, src, ts.ScriptTarget.Latest, true); + const violations: Violation[] = []; + + function visit(node: ts.Node): void { + if ( + ts.isReturnStatement(node) && + node.expression && + ts.isObjectLiteralExpression(node.expression) + ) { + const violatingAgentType = inspectReturnObject(node.expression); + if (violatingAgentType) { + const { line } = sf.getLineAndCharacterOfPosition(node.getStart(sf)); + violations.push({ file, line: line + 1, agentType: violatingAgentType }); + } + } + ts.forEachChild(node, visit); + } + + visit(sf); + return violations; +} + +describe('trigger workItemId consistency (static guard)', () => { + const allFiles = listHandlerFiles(TRIGGERS_ROOT); + + it('finds at least one trigger handler to scan (sanity)', () => { + expect(allFiles.length).toBeGreaterThan(10); + }); + + const allViolations = allFiles.flatMap(scanFile); + + it('every TriggerResult with a top-level workItemId also carries it inside agentInput', () => { + const formatted = allViolations + .map( + (v) => + ` ${v.file.replace(`${TRIGGERS_ROOT}/`, 'src/triggers/')}:${v.line} agentType=${v.agentType}`, + ) + .join('\n'); + expect( + allViolations, + allViolations.length === 0 + ? '' + : `Found ${allViolations.length} TriggerResult return(s) that set workItemId at the top level ` + + `but omit it from agentInput:\n${formatted}\n\n` + + `tryCreateRun (src/agents/shared/runTracking.ts) reads workItemId from agentInput; ` + + `omitting it leaves agent_runs.work_item_id NULL and hides the run from the work-item ` + + `page. Add \`workItemId\` (shorthand is fine) to the agentInput object in each listed return.`, + ).toEqual([]); + }); +});