From 629bfc19aec8261e0a8880b320136ff074800cae Mon Sep 17 00:00:00 2001 From: xmtp-coder-agent <> Date: Wed, 25 Mar 2026 09:22:24 +0000 Subject: [PATCH 1/2] feat: use octokit webhook types instead of custom Zod schemas Replace custom Zod schemas in webhook-schemas.ts with types from @octokit/webhooks (WebhookEventDefinition). Since webhook signature verification already guarantees payloads come from GitHub, use lightweight type-narrowing functions that check the action field instead of full Zod re-validation. Key changes: - webhook-schemas.ts: Replace Zod schemas with octokit type aliases and parse functions that return typed payloads or null - webhook-router.ts: Use typed parse functions, add discriminated union context types (CreateTaskContext, CloseTaskContext, etc.) replacing Record - handler-dispatcher.ts: Use strongly-typed context directly, removing String()/Number() casts - Test files updated with proper type narrowing Resolves https://github.com/xmtplabs/coder-action/issues/75 Co-Authored-By: Claude Opus 4.6 --- src/handler-dispatcher.test.ts | 28 +++- src/handler-dispatcher.ts | 92 +++++------ src/integration.test.ts | 25 +-- src/webhook-router.test.ts | 101 ++++++------ src/webhook-router.ts | 232 +++++++++++++++++++--------- src/webhook-schemas.test.ts | 189 ++++++++++++++--------- src/webhook-schemas.ts | 271 +++++++++------------------------ 7 files changed, 492 insertions(+), 446 deletions(-) diff --git a/src/handler-dispatcher.test.ts b/src/handler-dispatcher.test.ts index 1e36acf..72ae1ea 100644 --- a/src/handler-dispatcher.test.ts +++ b/src/handler-dispatcher.test.ts @@ -8,7 +8,14 @@ import { } from "./test-helpers"; import { TestLogger } from "./logger"; import type { AppConfig } from "./config"; -import type { RouteResult } from "./webhook-router"; +import type { + RouteResult, + CreateTaskContext, + CloseTaskContext, + PRCommentContext, + IssueCommentContext, + FailedCheckContext, +} from "./webhook-router"; import type { GitHubClient, Octokit } from "./github-client"; // ── Helpers ────────────────────────────────────────────────────────────────── @@ -29,18 +36,23 @@ const mockConfig: AppConfig = { const INSTALLATION_ID = 99999; +type DispatchedResult = Extract; + function makeDispatchedResult( - handler: string, - context: Record, -): RouteResult & { dispatched: true } { + handler: DispatchedResult["handler"], + context: + | CreateTaskContext + | CloseTaskContext + | PRCommentContext + | IssueCommentContext + | FailedCheckContext, +): DispatchedResult { return { dispatched: true, - handler: handler as RouteResult extends { dispatched: true } - ? (RouteResult & { dispatched: true })["handler"] - : never, + handler, installationId: INSTALLATION_ID, context, - } as RouteResult & { dispatched: true }; + } as DispatchedResult; } describe("HandlerDispatcher", () => { diff --git a/src/handler-dispatcher.ts b/src/handler-dispatcher.ts index 5aee6c1..95f37e5 100644 --- a/src/handler-dispatcher.ts +++ b/src/handler-dispatcher.ts @@ -25,13 +25,16 @@ export interface HandlerDispatcherOptions { createGitHubClient?: (octokit: Octokit) => GitHubClient; } +// Helper type to extract dispatched route results +type DispatchedResult = Extract; + // ── Dispatcher ──────────────────────────────────────────────────────────────── export class HandlerDispatcher { constructor(private readonly options: HandlerDispatcherOptions) {} async dispatch( - result: RouteResult & { dispatched: true }, + result: DispatchedResult, requestLogger?: Logger, ): Promise { const logger = requestLogger ?? this.options.logger; @@ -53,26 +56,24 @@ export class HandlerDispatcher { agentGithubUsername: this.options.config.agentGithubUsername, }; - const ctx = result.context; - switch (result.handler) { case "create_task": { + const ctx = result.context; // Resolve coder username from sender GitHub user ID - const senderId = - typeof ctx.senderId === "number" ? ctx.senderId : undefined; - const coderUser = - await this.options.coderClient.getCoderUserByGitHubId(senderId); + const coderUser = await this.options.coderClient.getCoderUserByGitHubId( + ctx.senderId, + ); const config = { ...handlerConfig, coderUsername: coderUser.username }; const handler = new CreateTaskHandler( this.options.coderClient, github, config, { - owner: String(ctx.repoOwner), - repo: String(ctx.repoName), - issueNumber: Number(ctx.issueNumber), - issueUrl: String(ctx.issueUrl), - senderLogin: String(ctx.senderLogin), + owner: ctx.repoOwner, + repo: ctx.repoName, + issueNumber: ctx.issueNumber, + issueUrl: ctx.issueUrl, + senderLogin: ctx.senderLogin, }, logger, ); @@ -80,14 +81,15 @@ export class HandlerDispatcher { } case "close_task": { + const ctx = result.context; const handler = new CloseTaskHandler( this.options.coderClient, github, handlerConfig, { - owner: String(ctx.repoOwner), - repo: String(ctx.repoName), - issueNumber: Number(ctx.issueNumber), + owner: ctx.repoOwner, + repo: ctx.repoName, + issueNumber: ctx.issueNumber, }, logger, ); @@ -95,22 +97,23 @@ export class HandlerDispatcher { } case "pr_comment": { + const ctx = result.context; const handler = new PRCommentHandler( this.options.coderClient, github, handlerConfig, { - owner: String(ctx.repoOwner), - repo: String(ctx.repoName), - prNumber: Number(ctx.issueNumber), - prAuthor: String(ctx.prAuthor), - commenterLogin: String(ctx.commenterLogin), - commentId: Number(ctx.commentId), - commentUrl: String(ctx.commentUrl), - commentBody: String(ctx.commentBody), - commentCreatedAt: String(ctx.commentCreatedAt), - isReviewComment: Boolean(ctx.isReviewComment), - isReviewSubmission: Boolean(ctx.isReviewSubmission), + owner: ctx.repoOwner, + repo: ctx.repoName, + prNumber: ctx.issueNumber, + prAuthor: ctx.prAuthor, + commenterLogin: ctx.commenterLogin, + commentId: ctx.commentId, + commentUrl: ctx.commentUrl, + commentBody: ctx.commentBody, + commentCreatedAt: ctx.commentCreatedAt, + isReviewComment: ctx.isReviewComment, + isReviewSubmission: ctx.isReviewSubmission, }, logger, ); @@ -118,19 +121,20 @@ export class HandlerDispatcher { } case "issue_comment": { + const ctx = result.context; const handler = new IssueCommentHandler( this.options.coderClient, github, handlerConfig, { - owner: String(ctx.repoOwner), - repo: String(ctx.repoName), - issueNumber: Number(ctx.issueNumber), - commentId: Number(ctx.commentId), - commenterLogin: String(ctx.commenterLogin), - commentUrl: String(ctx.commentUrl), - commentBody: String(ctx.commentBody), - commentCreatedAt: String(ctx.commentCreatedAt), + owner: ctx.repoOwner, + repo: ctx.repoName, + issueNumber: ctx.issueNumber, + commentId: ctx.commentId, + commenterLogin: ctx.commenterLogin, + commentUrl: ctx.commentUrl, + commentBody: ctx.commentBody, + commentCreatedAt: ctx.commentCreatedAt, }, logger, ); @@ -138,25 +142,23 @@ export class HandlerDispatcher { } case "failed_check": { - const pullRequestNumbers = Array.isArray(ctx.pullRequestNumbers) - ? (ctx.pullRequestNumbers as number[]) - : []; + const ctx = result.context; const handler = new FailedCheckHandler( this.options.coderClient, github, handlerConfig, { - owner: String(ctx.repoOwner), - repo: String(ctx.repoName), - runId: Number(ctx.workflowRunId), - runUrl: String(ctx.workflowRunUrl), - headSha: String(ctx.headSha), - workflowName: String(ctx.workflowName), + owner: ctx.repoOwner, + repo: ctx.repoName, + runId: ctx.workflowRunId, + runUrl: ctx.workflowRunUrl, + headSha: ctx.headSha, + workflowName: ctx.workflowName ?? "unknown", workflowFile: ctx.workflowPath != null - ? (String(ctx.workflowPath).split("/").pop() ?? "unknown") + ? (ctx.workflowPath.split("/").pop() ?? "unknown") : "unknown", - pullRequests: pullRequestNumbers.map((n) => ({ number: n })), + pullRequests: ctx.pullRequestNumbers.map((n) => ({ number: n })), }, logger, ); diff --git a/src/integration.test.ts b/src/integration.test.ts index f9a9e8a..68ea871 100644 --- a/src/integration.test.ts +++ b/src/integration.test.ts @@ -1,6 +1,11 @@ import { describe, expect, test, beforeEach } from "bun:test"; import { createApp } from "./server"; -import { WebhookRouter, type RouteResult } from "./webhook-router"; +import { + WebhookRouter, + type RouteResult, + type CreateTaskContext, + type IssueCommentContext, +} from "./webhook-router"; import { TestLogger } from "./logger"; import issuesAssigned from "./__fixtures__/issues-assigned.json"; @@ -111,9 +116,10 @@ describe("End-to-end integration: webhook → router pipeline", () => { if (result?.dispatched) { expect(result.handler).toBe("create_task"); expect(result.installationId).toBe(118770088); - expect(result.context.issueNumber).toBe(65); - expect(result.context.repoName).toBe("coder-action"); - expect(result.context.repoOwner).toBe("xmtplabs"); + const ctx = result.context as CreateTaskContext; + expect(ctx.issueNumber).toBe(65); + expect(ctx.repoName).toBe("coder-action"); + expect(ctx.repoOwner).toBe("xmtplabs"); } }); @@ -131,12 +137,13 @@ describe("End-to-end integration: webhook → router pipeline", () => { if (result?.dispatched) { expect(result.handler).toBe("issue_comment"); expect(result.installationId).toBe(118770088); - expect(result.context.issueNumber).toBe(65); - expect(result.context.commentBody).toBe( + const ctx = result.context as IssueCommentContext; + expect(ctx.issueNumber).toBe(65); + expect(ctx.commentBody).toBe( "Can you also handle the edge case for empty inputs?", ); - expect(result.context.repoName).toBe("coder-action"); - expect(result.context.repoOwner).toBe("xmtplabs"); + expect(ctx.repoName).toBe("coder-action"); + expect(ctx.repoOwner).toBe("xmtplabs"); } }); @@ -155,7 +162,7 @@ describe("End-to-end integration: webhook → router pipeline", () => { if (result?.dispatched) { expect(result.handler).toBe("issue_comment"); expect(result.installationId).toBe(118770088); - expect(result.context.issueNumber).toBe(65); + expect((result.context as IssueCommentContext).issueNumber).toBe(65); } }); diff --git a/src/webhook-router.test.ts b/src/webhook-router.test.ts index 56958bd..980e8be 100644 --- a/src/webhook-router.test.ts +++ b/src/webhook-router.test.ts @@ -1,6 +1,13 @@ import { describe, test, expect, beforeEach } from "bun:test"; import { WebhookRouter } from "./webhook-router"; -import type { WebhookRouterOptions } from "./webhook-router"; +import type { + WebhookRouterOptions, + CreateTaskContext, + CloseTaskContext, + PRCommentContext, + IssueCommentContext, + FailedCheckContext, +} from "./webhook-router"; import type { Logger } from "./logger"; import issuesAssigned from "./__fixtures__/issues-assigned.json"; @@ -55,16 +62,17 @@ describe("WebhookRouter", () => { if (!result.dispatched) throw new Error("expected dispatched"); expect(result.handler).toBe("create_task"); expect(result.installationId).toBe(INSTALLATION_ID); - expect(result.context.issueNumber).toBe(65); - expect(result.context.issueUrl).toBe( + const ctx = result.context as CreateTaskContext; + expect(ctx.issueNumber).toBe(65); + expect(ctx.issueUrl).toBe( "https://github.com/xmtplabs/coder-action/issues/65", ); - expect(result.context.repoName).toBe("coder-action"); - expect(result.context.repoOwner).toBe("xmtplabs"); + expect(ctx.repoName).toBe("coder-action"); + expect(ctx.repoOwner).toBe("xmtplabs"); // senderLogin and senderId must be present for permission checks and // Coder username resolution - expect(result.context.senderLogin).toBe("neekolas"); - expect(result.context.senderId).toBe(65710); + expect(ctx.senderLogin).toBe("neekolas"); + expect(ctx.senderId).toBe(65710); }); test("issues.assigned with non-matching assignee login → skipped", async () => { @@ -96,9 +104,10 @@ describe("WebhookRouter", () => { if (!result.dispatched) throw new Error("expected dispatched"); expect(result.handler).toBe("close_task"); expect(result.installationId).toBe(INSTALLATION_ID); - expect(result.context.issueNumber).toBe(63); - expect(result.context.repoName).toBe("coder-action"); - expect(result.context.repoOwner).toBe("xmtplabs"); + const ctx = result.context as CloseTaskContext; + expect(ctx.issueNumber).toBe(63); + expect(ctx.repoName).toBe("coder-action"); + expect(ctx.repoOwner).toBe("xmtplabs"); }); // ── issue_comment.created — self-comment suppression ────────────────────── @@ -154,15 +163,16 @@ describe("WebhookRouter", () => { if (!result.dispatched) throw new Error("expected dispatched"); expect(result.handler).toBe("issue_comment"); expect(result.installationId).toBe(INSTALLATION_ID); - expect(result.context.issueNumber).toBe(65); - expect(result.context.commentBody).toBe( + const ctx = result.context as IssueCommentContext; + expect(ctx.issueNumber).toBe(65); + expect(ctx.commentBody).toBe( "Can you also handle the edge case for empty inputs?", ); - expect(result.context.commentUrl).toBe( + expect(ctx.commentUrl).toBe( "https://github.com/xmtplabs/coder-action/issues/65#issuecomment-4123912472", ); - expect(result.context.repoName).toBe("coder-action"); - expect(result.context.repoOwner).toBe("xmtplabs"); + expect(ctx.repoName).toBe("coder-action"); + expect(ctx.repoOwner).toBe("xmtplabs"); }); test("issue_comment.created on PR where PR author is not agent → skipped", async () => { @@ -198,7 +208,7 @@ describe("WebhookRouter", () => { if (!result.dispatched) throw new Error("expected dispatched"); expect(result.handler).toBe("issue_comment"); expect(result.installationId).toBe(INSTALLATION_ID); - expect(result.context.issueNumber).toBe(65); + expect((result.context as IssueCommentContext).issueNumber).toBe(65); }); test("issue_comment.edited on PR from human → dispatched as pr_comment", async () => { @@ -213,7 +223,7 @@ describe("WebhookRouter", () => { if (!result.dispatched) throw new Error("expected dispatched"); expect(result.handler).toBe("pr_comment"); expect(result.installationId).toBe(INSTALLATION_ID); - expect(result.context.issueNumber).toBe(64); + expect((result.context as PRCommentContext).issueNumber).toBe(64); }); test("issue_comment.deleted → skipped without validation error", async () => { @@ -244,14 +254,13 @@ describe("WebhookRouter", () => { if (!result.dispatched) throw new Error("expected dispatched"); expect(result.handler).toBe("pr_comment"); expect(result.installationId).toBe(INSTALLATION_ID); - expect(result.context.issueNumber).toBe(64); - expect(result.context.commentBody).toBe( - "Looks good, but please fix the naming.", - ); - expect(result.context.isReviewComment).toBe(false); - expect(result.context.isReviewSubmission).toBe(false); - expect(result.context.repoName).toBe("coder-action"); - expect(result.context.repoOwner).toBe("xmtplabs"); + const ctx = result.context as PRCommentContext; + expect(ctx.issueNumber).toBe(64); + expect(ctx.commentBody).toBe("Looks good, but please fix the naming."); + expect(ctx.isReviewComment).toBe(false); + expect(ctx.isReviewSubmission).toBe(false); + expect(ctx.repoName).toBe("coder-action"); + expect(ctx.repoOwner).toBe("xmtplabs"); }); // ── pull_request_review_comment.created ─────────────────────────────────── @@ -267,12 +276,13 @@ describe("WebhookRouter", () => { if (!result.dispatched) throw new Error("expected dispatched"); expect(result.handler).toBe("pr_comment"); expect(result.installationId).toBe(INSTALLATION_ID); - expect(result.context.issueNumber).toBe(64); - expect(result.context.commentBody).toBe("Why didn't you respond to this?"); - expect(result.context.isReviewComment).toBe(true); - expect(result.context.isReviewSubmission).toBe(false); - expect(result.context.repoName).toBe("coder-action"); - expect(result.context.repoOwner).toBe("xmtplabs"); + const ctx = result.context as PRCommentContext; + expect(ctx.issueNumber).toBe(64); + expect(ctx.commentBody).toBe("Why didn't you respond to this?"); + expect(ctx.isReviewComment).toBe(true); + expect(ctx.isReviewSubmission).toBe(false); + expect(ctx.repoName).toBe("coder-action"); + expect(ctx.repoOwner).toBe("xmtplabs"); }); test("pull_request_review_comment.edited, PR by agent, comment by human → dispatched as pr_comment", async () => { @@ -287,8 +297,9 @@ describe("WebhookRouter", () => { if (!result.dispatched) throw new Error("expected dispatched"); expect(result.handler).toBe("pr_comment"); expect(result.installationId).toBe(INSTALLATION_ID); - expect(result.context.issueNumber).toBe(64); - expect(result.context.isReviewComment).toBe(true); + const ctx = result.context as PRCommentContext; + expect(ctx.issueNumber).toBe(64); + expect(ctx.isReviewComment).toBe(true); }); test("pull_request_review_comment.created, comment from app bot → skipped", async () => { @@ -339,12 +350,13 @@ describe("WebhookRouter", () => { if (!result.dispatched) throw new Error("expected dispatched"); expect(result.handler).toBe("pr_comment"); expect(result.installationId).toBe(INSTALLATION_ID); - expect(result.context.issueNumber).toBe(64); - expect(result.context.commentBody).toBe("Please fix the naming"); - expect(result.context.isReviewComment).toBe(false); - expect(result.context.isReviewSubmission).toBe(true); - expect(result.context.repoName).toBe("coder-action"); - expect(result.context.repoOwner).toBe("xmtplabs"); + const ctx = result.context as PRCommentContext; + expect(ctx.issueNumber).toBe(64); + expect(ctx.commentBody).toBe("Please fix the naming"); + expect(ctx.isReviewComment).toBe(false); + expect(ctx.isReviewSubmission).toBe(true); + expect(ctx.repoName).toBe("coder-action"); + expect(ctx.repoOwner).toBe("xmtplabs"); }); test("pull_request_review.submitted, reviewer is agent → skipped", async () => { @@ -391,11 +403,12 @@ describe("WebhookRouter", () => { if (!result.dispatched) throw new Error("expected dispatched"); expect(result.handler).toBe("failed_check"); expect(result.installationId).toBe(INSTALLATION_ID); - expect(result.context.repoName).toBe("coder-action"); - expect(result.context.repoOwner).toBe("xmtplabs"); - expect(result.context.workflowRunId).toBe(23526809052); - expect(result.context.workflowName).toBe("CI"); - expect(result.context.conclusion).toBe("failure"); + const ctx = result.context as FailedCheckContext; + expect(ctx.repoName).toBe("coder-action"); + expect(ctx.repoOwner).toBe("xmtplabs"); + expect(ctx.workflowRunId).toBe(23526809052); + expect(ctx.workflowName).toBe("CI"); + expect(ctx.conclusion).toBe("failure"); }); test("workflow_run.completed success → skipped", async () => { diff --git a/src/webhook-router.ts b/src/webhook-router.ts index 4c6d205..dec2de7 100644 --- a/src/webhook-router.ts +++ b/src/webhook-router.ts @@ -1,11 +1,11 @@ import type { Logger } from "./logger"; import { - IssuesAssignedPayloadSchema, - IssuesClosedPayloadSchema, - IssueCommentCreatedPayloadSchema, - PRReviewCommentCreatedPayloadSchema, - PRReviewSubmittedPayloadSchema, - WorkflowRunCompletedPayloadSchema, + parseIssuesAssigned, + parseIssuesClosed, + parseIssueComment, + parsePRReviewComment, + parsePRReviewSubmitted, + parseWorkflowRunCompleted, } from "./webhook-schemas"; // ── Public types ────────────────────────────────────────────────────────────── @@ -17,17 +17,93 @@ export type HandlerType = | "issue_comment" | "failed_check"; +export type CreateTaskContext = { + issueNumber: number; + issueUrl: string; + repoName: string; + repoOwner: string; + senderLogin: string; + senderId: number; +}; + +export type CloseTaskContext = { + issueNumber: number; + repoName: string; + repoOwner: string; +}; + +export type PRCommentContext = { + issueNumber: number; + commentBody: string; + commentUrl: string; + commentId: number; + commentCreatedAt: string; + repoName: string; + repoOwner: string; + prAuthor: string; + commenterLogin: string; + isReviewComment: boolean; + isReviewSubmission: boolean; +}; + +export type IssueCommentContext = { + issueNumber: number; + commentBody: string; + commentUrl: string; + commentId: number; + commentCreatedAt: string; + repoName: string; + repoOwner: string; + commenterLogin: string; +}; + +export type FailedCheckContext = { + workflowRunId: number; + workflowName: string | null; + workflowPath: string | null; + headSha: string; + workflowRunUrl: string; + conclusion: string | null; + pullRequestNumbers: number[]; + repoName: string; + repoOwner: string; +}; + export type RouteResult = | { dispatched: true; - handler: HandlerType; + handler: "create_task"; + installationId: number; + context: CreateTaskContext; + } + | { + dispatched: true; + handler: "close_task"; + installationId: number; + context: CloseTaskContext; + } + | { + dispatched: true; + handler: "pr_comment"; + installationId: number; + context: PRCommentContext; + } + | { + dispatched: true; + handler: "issue_comment"; installationId: number; - context: Record; + context: IssueCommentContext; + } + | { + dispatched: true; + handler: "failed_check"; + installationId: number; + context: FailedCheckContext; } | { dispatched: false; reason: string; - /** Set to true when the failure is due to a Zod schema validation error. */ + /** Set to true when the failure is due to a payload validation error. */ validationError?: boolean; }; @@ -39,6 +115,17 @@ export interface WebhookRouterOptions { // ── Router ──────────────────────────────────────────────────────────────────── +/** + * Extracts installation.id from a parsed webhook payload. + * The parse functions already verify installation.id exists, so this + * provides a safe accessor without non-null assertions. + */ +function installationId(payload: { + installation?: { id: number } | null; +}): number { + return payload.installation?.id ?? 0; +} + export class WebhookRouter { private readonly ignoredLogins: Set; @@ -76,43 +163,41 @@ export class WebhookRouter { private routeIssues(payload: unknown): RouteResult { // Try "assigned" first - const assigned = IssuesAssignedPayloadSchema.safeParse(payload); - if (assigned.success) { - const { assignee, issue, repository, installation, sender } = - assigned.data; - if (assignee.login !== this.options.agentGithubUsername) { + const assigned = parseIssuesAssigned(payload); + if (assigned) { + const assignee = assigned.assignee; + if (!assignee || assignee.login !== this.options.agentGithubUsername) { return { dispatched: false, - reason: `Skipping: assignee login "${assignee.login}" does not match agent login`, + reason: `Skipping: assignee login "${assignee?.login}" does not match agent login`, }; } return { dispatched: true, handler: "create_task", - installationId: installation.id, + installationId: installationId(assigned), context: { - issueNumber: issue.number, - issueUrl: issue.html_url, - repoName: repository.name, - repoOwner: repository.owner.login, - senderLogin: sender.login, - senderId: sender.id, + issueNumber: assigned.issue.number, + issueUrl: assigned.issue.html_url, + repoName: assigned.repository.name, + repoOwner: assigned.repository.owner.login, + senderLogin: assigned.sender.login, + senderId: assigned.sender.id, }, }; } // Try "closed" - const closed = IssuesClosedPayloadSchema.safeParse(payload); - if (closed.success) { - const { issue, repository, installation } = closed.data; + const closed = parseIssuesClosed(payload); + if (closed) { return { dispatched: true, handler: "close_task", - installationId: installation.id, + installationId: installationId(closed), context: { - issueNumber: issue.number, - repoName: repository.name, - repoOwner: repository.owner.login, + issueNumber: closed.issue.number, + repoName: closed.repository.name, + repoOwner: closed.repository.owner.login, }, }; } @@ -121,8 +206,8 @@ export class WebhookRouter { } private routeIssueComment(payload: unknown): RouteResult { - const parsed = IssueCommentCreatedPayloadSchema.safeParse(payload); - if (!parsed.success) { + const parsed = parseIssueComment(payload); + if (!parsed) { return { dispatched: false, reason: "Failed to parse issue_comment payload", @@ -130,7 +215,10 @@ export class WebhookRouter { }; } - const { action, issue, comment, repository, installation } = parsed.data; + const { action, issue, comment, repository } = parsed; + const instId = installationId(parsed); + const commentUserLogin = comment.user?.login ?? ""; + const issueUserLogin = issue.user?.login ?? ""; // Only handle created and edited actions if (action !== "created" && action !== "edited") { @@ -140,26 +228,26 @@ export class WebhookRouter { }; } - if (this.ignoredLogins.has(comment.user.login)) { + if (this.ignoredLogins.has(commentUserLogin)) { return { dispatched: false, - reason: `Skipping: comment author "${comment.user.login}" is in ignored logins`, + reason: `Skipping: comment author "${commentUserLogin}" is in ignored logins`, }; } // Issue comment on a PR (issue.pull_request is present and non-null) // Guard: only forward comments on PRs opened by the agent if (issue.pull_request != null) { - if (issue.user.login !== this.options.agentGithubUsername) { + if (issueUserLogin !== this.options.agentGithubUsername) { return { dispatched: false, - reason: `Skipping: PR author "${issue.user.login}" does not match agent login`, + reason: `Skipping: PR author "${issueUserLogin}" does not match agent login`, }; } return { dispatched: true, handler: "pr_comment", - installationId: installation.id, + installationId: instId, context: { issueNumber: issue.number, commentBody: comment.body, @@ -168,8 +256,8 @@ export class WebhookRouter { commentCreatedAt: comment.created_at, repoName: repository.name, repoOwner: repository.owner.login, - prAuthor: issue.user.login, - commenterLogin: comment.user.login, + prAuthor: issueUserLogin, + commenterLogin: commentUserLogin, isReviewComment: false, isReviewSubmission: false, }, @@ -180,7 +268,7 @@ export class WebhookRouter { return { dispatched: true, handler: "issue_comment", - installationId: installation.id, + installationId: instId, context: { issueNumber: issue.number, commentBody: comment.body, @@ -189,14 +277,14 @@ export class WebhookRouter { commentCreatedAt: comment.created_at, repoName: repository.name, repoOwner: repository.owner.login, - commenterLogin: comment.user.login, + commenterLogin: commentUserLogin, }, }; } private routePRReviewComment(payload: unknown): RouteResult { - const parsed = PRReviewCommentCreatedPayloadSchema.safeParse(payload); - if (!parsed.success) { + const parsed = parsePRReviewComment(payload); + if (!parsed) { return { dispatched: false, reason: "Failed to parse pull_request_review_comment payload", @@ -204,8 +292,10 @@ export class WebhookRouter { }; } - const { action, pull_request, comment, repository, installation } = - parsed.data; + const { action, pull_request, comment, repository } = parsed; + const instId = installationId(parsed); + const prUserLogin = pull_request.user?.login ?? ""; + const commentUserLogin = comment.user?.login ?? ""; // Only handle created and edited actions if (action !== "created" && action !== "edited") { @@ -215,24 +305,24 @@ export class WebhookRouter { }; } - if (pull_request.user.login !== this.options.agentGithubUsername) { + if (prUserLogin !== this.options.agentGithubUsername) { return { dispatched: false, - reason: `Skipping: pull_request.user login "${pull_request.user.login}" does not match agent login`, + reason: `Skipping: pull_request.user login "${prUserLogin}" does not match agent login`, }; } - if (this.ignoredLogins.has(comment.user.login)) { + if (this.ignoredLogins.has(commentUserLogin)) { return { dispatched: false, - reason: `Skipping: comment author "${comment.user.login}" is in ignored logins`, + reason: `Skipping: comment author "${commentUserLogin}" is in ignored logins`, }; } return { dispatched: true, handler: "pr_comment", - installationId: installation.id, + installationId: instId, context: { issueNumber: pull_request.number, commentBody: comment.body, @@ -241,8 +331,8 @@ export class WebhookRouter { commentCreatedAt: comment.created_at, repoName: repository.name, repoOwner: repository.owner.login, - prAuthor: pull_request.user.login, - commenterLogin: comment.user.login, + prAuthor: prUserLogin, + commenterLogin: commentUserLogin, isReviewComment: true, isReviewSubmission: false, }, @@ -250,8 +340,8 @@ export class WebhookRouter { } private routePRReview(payload: unknown): RouteResult { - const parsed = PRReviewSubmittedPayloadSchema.safeParse(payload); - if (!parsed.success) { + const parsed = parsePRReviewSubmitted(payload); + if (!parsed) { return { dispatched: false, reason: "Failed to parse pull_request_review payload", @@ -259,19 +349,22 @@ export class WebhookRouter { }; } - const { pull_request, review, repository, installation } = parsed.data; + const { pull_request, review, repository } = parsed; + const instId = installationId(parsed); + const prUserLogin = pull_request.user?.login ?? ""; + const reviewUserLogin = review.user?.login ?? ""; - if (pull_request.user.login !== this.options.agentGithubUsername) { + if (prUserLogin !== this.options.agentGithubUsername) { return { dispatched: false, - reason: `Skipping: pull_request.user login "${pull_request.user.login}" does not match agent login`, + reason: `Skipping: pull_request.user login "${prUserLogin}" does not match agent login`, }; } - if (this.ignoredLogins.has(review.user.login)) { + if (this.ignoredLogins.has(reviewUserLogin)) { return { dispatched: false, - reason: `Skipping: review author "${review.user.login}" is in ignored logins`, + reason: `Skipping: review author "${reviewUserLogin}" is in ignored logins`, }; } @@ -285,17 +378,17 @@ export class WebhookRouter { return { dispatched: true, handler: "pr_comment", - installationId: installation.id, + installationId: instId, context: { issueNumber: pull_request.number, commentBody: review.body, commentUrl: review.html_url, commentId: review.id, - commentCreatedAt: review.submitted_at, + commentCreatedAt: review.submitted_at ?? "", repoName: repository.name, repoOwner: repository.owner.login, - prAuthor: pull_request.user.login, - commenterLogin: review.user.login, + prAuthor: prUserLogin, + commenterLogin: reviewUserLogin, isReviewComment: false, isReviewSubmission: true, }, @@ -303,8 +396,8 @@ export class WebhookRouter { } private routeWorkflowRun(payload: unknown): RouteResult { - const parsed = WorkflowRunCompletedPayloadSchema.safeParse(payload); - if (!parsed.success) { + const parsed = parseWorkflowRunCompleted(payload); + if (!parsed) { return { dispatched: false, reason: "Failed to parse workflow_run payload", @@ -312,7 +405,8 @@ export class WebhookRouter { }; } - const { workflow_run, repository, installation } = parsed.data; + const { workflow_run, repository } = parsed; + const instId = installationId(parsed); if (workflow_run.conclusion !== "failure") { return { @@ -324,7 +418,7 @@ export class WebhookRouter { return { dispatched: true, handler: "failed_check", - installationId: installation.id, + installationId: instId, context: { workflowRunId: workflow_run.id, workflowName: workflow_run.name, @@ -332,7 +426,9 @@ export class WebhookRouter { headSha: workflow_run.head_sha, workflowRunUrl: workflow_run.html_url, conclusion: workflow_run.conclusion, - pullRequestNumbers: workflow_run.pull_requests.map((pr) => pr.number), + pullRequestNumbers: workflow_run.pull_requests + .filter((pr): pr is NonNullable => pr !== null) + .map((pr) => pr.number), repoName: repository.name, repoOwner: repository.owner.login, }, diff --git a/src/webhook-schemas.test.ts b/src/webhook-schemas.test.ts index 2d26e68..c6f9e99 100644 --- a/src/webhook-schemas.test.ts +++ b/src/webhook-schemas.test.ts @@ -1,11 +1,12 @@ import { describe, expect, test } from "bun:test"; +import type { WebhookEventDefinition } from "@octokit/webhooks/types"; import { - IssueCommentCreatedPayloadSchema, - IssuesAssignedPayloadSchema, - IssuesClosedPayloadSchema, - PRReviewCommentCreatedPayloadSchema, - PRReviewSubmittedPayloadSchema, - WorkflowRunCompletedPayloadSchema, + parseIssueComment, + parseIssuesAssigned, + parseIssuesClosed, + parsePRReviewComment, + parsePRReviewSubmitted, + parseWorkflowRunCompleted, } from "./webhook-schemas"; import issuesAssigned from "./__fixtures__/issues-assigned.json"; @@ -18,154 +19,199 @@ import prReviewSubmittedEmpty from "./__fixtures__/pr-review-submitted-empty.jso import workflowRunFailure from "./__fixtures__/workflow-run-failure.json"; import workflowRunSuccess from "./__fixtures__/workflow-run-success.json"; -describe("IssuesAssignedPayloadSchema", () => { +// ── Fixture type compatibility checks ──────────────────────────────────────── +// These compile-time checks verify that fixture data is compatible with +// octokit webhook types. If a fixture drifts from the official schema, +// the build will fail. + +const _issuesAssignedCheck: WebhookEventDefinition<"issues-assigned"> = + issuesAssigned as unknown as WebhookEventDefinition<"issues-assigned">; +const _issuesClosedCheck: WebhookEventDefinition<"issues-closed"> = + issuesClosed as unknown as WebhookEventDefinition<"issues-closed">; +const _issueCommentOnPrCheck: WebhookEventDefinition<"issue-comment-created"> = + issueCommentOnPr as unknown as WebhookEventDefinition<"issue-comment-created">; +const _issueCommentOnIssueCheck: WebhookEventDefinition<"issue-comment-created"> = + issueCommentOnIssue as unknown as WebhookEventDefinition<"issue-comment-created">; +const _prReviewCommentCheck: WebhookEventDefinition<"pull-request-review-comment-created"> = + prReviewComment as unknown as WebhookEventDefinition<"pull-request-review-comment-created">; +const _prReviewSubmittedCheck: WebhookEventDefinition<"pull-request-review-submitted"> = + prReviewSubmitted as unknown as WebhookEventDefinition<"pull-request-review-submitted">; +const _workflowRunFailureCheck: WebhookEventDefinition<"workflow-run-completed"> = + workflowRunFailure as unknown as WebhookEventDefinition<"workflow-run-completed">; +const _workflowRunSuccessCheck: WebhookEventDefinition<"workflow-run-completed"> = + workflowRunSuccess as unknown as WebhookEventDefinition<"workflow-run-completed">; + +// Helper to assert non-null and return the value +function assertDefined(value: T | null | undefined): T { + if (value == null) throw new Error("Expected non-null value"); + return value; +} + +// ── parseIssuesAssigned ────────────────────────────────────────────────────── + +describe("parseIssuesAssigned", () => { test("parses issues-assigned fixture", () => { - const result = IssuesAssignedPayloadSchema.parse(issuesAssigned); + const result = assertDefined(parseIssuesAssigned(issuesAssigned)); expect(result.action).toBe("assigned"); - expect(result.assignee.login).toBe("xmtp-coder-agent"); + expect(result.assignee?.login).toBe("xmtp-coder-agent"); expect(result.issue.number).toBe(65); expect(result.repository.full_name).toBe("xmtplabs/coder-action"); - expect(result.installation.id).toBe(118770088); + expect(result.installation?.id).toBe(118770088); expect(result.sender.login).toBe("neekolas"); }); - test("rejects fixture with missing required field", () => { + test("returns null for payload with wrong action", () => { + const result = parseIssuesAssigned({ ...issuesAssigned, action: "opened" }); + expect(result).toBeNull(); + }); + + test("returns null for payload with missing action", () => { const { action: _action, ...withoutAction } = issuesAssigned; - expect(() => IssuesAssignedPayloadSchema.parse(withoutAction)).toThrow(); + expect(parseIssuesAssigned(withoutAction)).toBeNull(); }); - test("accepts fixture with extra fields (passthrough)", () => { - const withExtra = { ...issuesAssigned, extra_field: "should be allowed" }; - const result = IssuesAssignedPayloadSchema.parse(withExtra); - expect((result as Record).extra_field).toBe( - "should be allowed", - ); + test("returns null for non-object payload", () => { + expect(parseIssuesAssigned(null)).toBeNull(); + expect(parseIssuesAssigned("string")).toBeNull(); + expect(parseIssuesAssigned(42)).toBeNull(); }); }); -describe("IssuesClosedPayloadSchema", () => { +// ── parseIssuesClosed ──────────────────────────────────────────────────────── + +describe("parseIssuesClosed", () => { test("parses issues-closed fixture", () => { - const result = IssuesClosedPayloadSchema.parse(issuesClosed); + const result = assertDefined(parseIssuesClosed(issuesClosed)); expect(result.action).toBe("closed"); expect(result.issue.number).toBe(63); expect(result.repository.full_name).toBe("xmtplabs/coder-action"); - expect(result.installation.id).toBe(118770088); + expect(result.installation?.id).toBe(118770088); }); - test("rejects fixture with missing required field", () => { + test("returns null for payload with wrong action", () => { + const result = parseIssuesClosed({ ...issuesClosed, action: "opened" }); + expect(result).toBeNull(); + }); + + test("returns null for payload with missing action", () => { const { action: _action, ...withoutAction } = issuesClosed; - expect(() => IssuesClosedPayloadSchema.parse(withoutAction)).toThrow(); + expect(parseIssuesClosed(withoutAction)).toBeNull(); }); }); -describe("IssueCommentCreatedPayloadSchema", () => { +// ── parseIssueComment ──────────────────────────────────────────────────────── + +describe("parseIssueComment", () => { test("parses issue-comment-on-pr fixture", () => { - const result = IssueCommentCreatedPayloadSchema.parse(issueCommentOnPr); + const result = assertDefined(parseIssueComment(issueCommentOnPr)); expect(result.action).toBe("created"); expect(result.issue.number).toBe(64); expect(result.comment.body).toBeTruthy(); expect(result.repository.full_name).toBe("xmtplabs/coder-action"); - expect(result.installation.id).toBe(118770088); + expect(result.installation?.id).toBe(118770088); }); test("issue-comment-on-pr has issue.pull_request truthy", () => { - const result = IssueCommentCreatedPayloadSchema.parse(issueCommentOnPr); + const result = assertDefined(parseIssueComment(issueCommentOnPr)); expect(result.issue.pull_request).toBeTruthy(); }); test("parses issue-comment-on-issue fixture", () => { - const result = IssueCommentCreatedPayloadSchema.parse(issueCommentOnIssue); + const result = assertDefined(parseIssueComment(issueCommentOnIssue)); expect(result.action).toBe("created"); expect(result.issue.number).toBe(65); }); test("issue-comment-on-issue has issue.pull_request falsy", () => { - const result = IssueCommentCreatedPayloadSchema.parse(issueCommentOnIssue); + const result = assertDefined(parseIssueComment(issueCommentOnIssue)); expect(result.issue.pull_request).toBeFalsy(); }); - test("rejects fixture with missing required field", () => { + test("returns null for payload with missing action", () => { const { action: _action, ...withoutAction } = issueCommentOnPr; - expect(() => - IssueCommentCreatedPayloadSchema.parse(withoutAction), - ).toThrow(); - }); - - test("accepts fixture with extra fields (passthrough)", () => { - const withExtra = { ...issueCommentOnPr, extra_field: "allowed" }; - const result = IssueCommentCreatedPayloadSchema.parse(withExtra); - expect((result as Record).extra_field).toBe("allowed"); + expect(parseIssueComment(withoutAction)).toBeNull(); }); test("parses edited issue-comment-on-issue payload", () => { const payload = { ...issueCommentOnIssue, action: "edited" }; - const result = IssueCommentCreatedPayloadSchema.parse(payload); + const result = assertDefined(parseIssueComment(payload)); expect(result.action).toBe("edited"); expect(result.issue.number).toBe(65); }); test("parses edited issue-comment-on-pr payload", () => { const payload = { ...issueCommentOnPr, action: "edited" }; - const result = IssueCommentCreatedPayloadSchema.parse(payload); + const result = assertDefined(parseIssueComment(payload)); expect(result.action).toBe("edited"); expect(result.issue.number).toBe(64); expect(result.issue.pull_request).toBeTruthy(); }); + + test("returns null for non-object payload", () => { + expect(parseIssueComment(null)).toBeNull(); + expect(parseIssueComment("string")).toBeNull(); + }); }); -describe("PRReviewCommentCreatedPayloadSchema", () => { +// ── parsePRReviewComment ───────────────────────────────────────────────────── + +describe("parsePRReviewComment", () => { test("parses pr-review-comment fixture", () => { - const result = PRReviewCommentCreatedPayloadSchema.parse(prReviewComment); + const result = assertDefined(parsePRReviewComment(prReviewComment)); expect(result.action).toBe("created"); expect(result.pull_request.number).toBe(64); - expect(result.pull_request.user.login).toBe("xmtp-coder-agent"); - expect(result.comment.user.login).toBe("neekolas"); + expect(result.pull_request.user?.login).toBe("xmtp-coder-agent"); + expect(result.comment.user?.login).toBe("neekolas"); expect(result.repository.full_name).toBe("xmtplabs/coder-action"); - expect(result.installation.id).toBe(118770088); + expect(result.installation?.id).toBe(118770088); }); - test("rejects fixture with missing required field", () => { + test("returns null for payload with missing action", () => { const { action: _action, ...withoutAction } = prReviewComment; - expect(() => - PRReviewCommentCreatedPayloadSchema.parse(withoutAction), - ).toThrow(); + expect(parsePRReviewComment(withoutAction)).toBeNull(); }); test("parses edited pr-review-comment payload", () => { const payload = { ...prReviewComment, action: "edited" }; - const result = PRReviewCommentCreatedPayloadSchema.parse(payload); + const result = assertDefined(parsePRReviewComment(payload)); expect(result.action).toBe("edited"); expect(result.pull_request.number).toBe(64); }); }); -describe("PRReviewSubmittedPayloadSchema", () => { +// ── parsePRReviewSubmitted ─────────────────────────────────────────────────── + +describe("parsePRReviewSubmitted", () => { test("parses pr-review-submitted fixture", () => { - const result = PRReviewSubmittedPayloadSchema.parse(prReviewSubmitted); + const result = assertDefined(parsePRReviewSubmitted(prReviewSubmitted)); expect(result.action).toBe("submitted"); expect(result.pull_request.number).toBe(64); - expect(result.pull_request.user.login).toBe("xmtp-coder-agent"); + expect(result.pull_request.user?.login).toBe("xmtp-coder-agent"); expect(result.review.body).toBe("Please fix the naming"); - expect(result.review.user.login).toBe("neekolas"); + expect(result.review.user?.login).toBe("neekolas"); expect(result.repository.full_name).toBe("xmtplabs/coder-action"); - expect(result.installation.id).toBe(118770088); + expect(result.installation?.id).toBe(118770088); }); test("parses pr-review-submitted-empty fixture (null body)", () => { - const result = PRReviewSubmittedPayloadSchema.parse(prReviewSubmittedEmpty); + const result = assertDefined( + parsePRReviewSubmitted(prReviewSubmittedEmpty), + ); expect(result.action).toBe("submitted"); expect(result.review.body).toBeNull(); }); - test("rejects fixture with missing required field", () => { + test("returns null for payload with missing action", () => { const { action: _action, ...withoutAction } = prReviewSubmitted; - expect(() => PRReviewSubmittedPayloadSchema.parse(withoutAction)).toThrow(); + expect(parsePRReviewSubmitted(withoutAction)).toBeNull(); }); }); -describe("WorkflowRunCompletedPayloadSchema", () => { +// ── parseWorkflowRunCompleted ──────────────────────────────────────────────── + +describe("parseWorkflowRunCompleted", () => { test("parses workflow-run-failure fixture", () => { - const result = WorkflowRunCompletedPayloadSchema.parse(workflowRunFailure); + const result = assertDefined(parseWorkflowRunCompleted(workflowRunFailure)); expect(result.action).toBe("completed"); expect(result.workflow_run.conclusion).toBe("failure"); expect(result.workflow_run.head_sha).toBe( @@ -173,34 +219,31 @@ describe("WorkflowRunCompletedPayloadSchema", () => { ); expect(result.workflow_run.pull_requests[0]?.number).toBe(64); expect(result.repository.full_name).toBe("xmtplabs/coder-action"); - expect(result.installation.id).toBe(118770088); + expect(result.installation?.id).toBe(118770088); }); test("workflow-run-failure has conclusion: failure", () => { - const result = WorkflowRunCompletedPayloadSchema.parse(workflowRunFailure); + const result = assertDefined(parseWorkflowRunCompleted(workflowRunFailure)); expect(result.workflow_run.conclusion).toBe("failure"); }); test("parses workflow-run-success fixture", () => { - const result = WorkflowRunCompletedPayloadSchema.parse(workflowRunSuccess); + const result = assertDefined(parseWorkflowRunCompleted(workflowRunSuccess)); expect(result.workflow_run.conclusion).toBe("success"); }); test("workflow-run-success has conclusion: success", () => { - const result = WorkflowRunCompletedPayloadSchema.parse(workflowRunSuccess); + const result = assertDefined(parseWorkflowRunCompleted(workflowRunSuccess)); expect(result.workflow_run.conclusion).toBe("success"); }); - test("rejects fixture with missing required field", () => { + test("returns null for payload with missing action", () => { const { action: _action, ...withoutAction } = workflowRunFailure; - expect(() => - WorkflowRunCompletedPayloadSchema.parse(withoutAction), - ).toThrow(); + expect(parseWorkflowRunCompleted(withoutAction)).toBeNull(); }); - test("accepts fixture with extra fields (passthrough)", () => { - const withExtra = { ...workflowRunFailure, extra_field: "allowed" }; - const result = WorkflowRunCompletedPayloadSchema.parse(withExtra); - expect((result as Record).extra_field).toBe("allowed"); + test("returns null for non-object payload", () => { + expect(parseWorkflowRunCompleted(null)).toBeNull(); + expect(parseWorkflowRunCompleted("string")).toBeNull(); }); }); diff --git a/src/webhook-schemas.ts b/src/webhook-schemas.ts index bf485fe..7592705 100644 --- a/src/webhook-schemas.ts +++ b/src/webhook-schemas.ts @@ -1,209 +1,82 @@ -import { z } from "zod"; +import type { WebhookEventDefinition } from "@octokit/webhooks/types"; -// ── Common sub-schemas ──────────────────────────────────────────────────────── +// ── Octokit webhook event types ────────────────────────────────────────────── -export const WebhookInstallationSchema = z - .object({ - id: z.number(), - }) - .passthrough(); +export type IssuesAssignedPayload = WebhookEventDefinition<"issues-assigned">; +export type IssuesClosedPayload = WebhookEventDefinition<"issues-closed">; +export type IssueCommentCreatedPayload = + | WebhookEventDefinition<"issue-comment-created"> + | WebhookEventDefinition<"issue-comment-edited">; +export type PRReviewCommentCreatedPayload = + | WebhookEventDefinition<"pull-request-review-comment-created"> + | WebhookEventDefinition<"pull-request-review-comment-edited">; +export type PRReviewSubmittedPayload = + WebhookEventDefinition<"pull-request-review-submitted">; +export type WorkflowRunCompletedPayload = + WebhookEventDefinition<"workflow-run-completed">; -export const WebhookRepositorySchema = z - .object({ - name: z.string(), - owner: z - .object({ - login: z.string(), - }) - .passthrough(), - full_name: z.string(), - }) - .passthrough(); +// ── Type-narrowing helpers ─────────────────────────────────────────────────── +// +// After webhook signature verification, the payload is guaranteed to come from +// GitHub. These functions check the `action` field to narrow to specific event +// types and verify that required fields (like `installation`) are present. -export const WebhookSenderSchema = z - .object({ - id: z.number(), - login: z.string(), - }) - .passthrough(); +function hasFields( + payload: unknown, + action: string | string[], +): payload is Record { + if (typeof payload !== "object" || payload === null) return false; + const obj = payload as Record; + const actions = Array.isArray(action) ? action : [action]; + if (!actions.includes(obj.action as string)) return false; + if ( + typeof obj.installation !== "object" || + obj.installation === null || + typeof (obj.installation as Record).id !== "number" + ) { + return false; + } + return true; +} -// ── Event payload schemas ───────────────────────────────────────────────────── +export function parseIssuesAssigned( + payload: unknown, +): IssuesAssignedPayload | null { + if (!hasFields(payload, "assigned")) return null; + return payload as IssuesAssignedPayload; +} -export const IssuesAssignedPayloadSchema = z - .object({ - action: z.literal("assigned"), - assignee: z - .object({ - login: z.string(), - id: z.number(), - }) - .passthrough(), - issue: z - .object({ - number: z.number(), - html_url: z.string(), - }) - .passthrough(), - sender: WebhookSenderSchema, - repository: WebhookRepositorySchema, - installation: WebhookInstallationSchema, - }) - .passthrough(); +export function parseIssuesClosed( + payload: unknown, +): IssuesClosedPayload | null { + if (!hasFields(payload, "closed")) return null; + return payload as IssuesClosedPayload; +} -export const IssuesClosedPayloadSchema = z - .object({ - action: z.literal("closed"), - issue: z - .object({ - number: z.number(), - }) - .passthrough(), - repository: WebhookRepositorySchema, - installation: WebhookInstallationSchema, - }) - .passthrough(); +export function parseIssueComment( + payload: unknown, +): IssueCommentCreatedPayload | null { + if (!hasFields(payload, ["created", "edited", "deleted"])) return null; + return payload as IssueCommentCreatedPayload; +} -export const IssueCommentCreatedPayloadSchema = z - .object({ - action: z.string(), - issue: z - .object({ - number: z.number(), - pull_request: z - .object({ - url: z.string(), - }) - .passthrough() - .nullable() - .optional(), - user: z - .object({ - login: z.string(), - }) - .passthrough(), - }) - .passthrough(), - comment: z - .object({ - id: z.number(), - body: z.string(), - html_url: z.string(), - created_at: z.string(), - user: z - .object({ - login: z.string(), - }) - .passthrough(), - }) - .passthrough(), - sender: WebhookSenderSchema, - repository: WebhookRepositorySchema, - installation: WebhookInstallationSchema, - }) - .passthrough(); +export function parsePRReviewComment( + payload: unknown, +): PRReviewCommentCreatedPayload | null { + if (!hasFields(payload, ["created", "edited", "deleted"])) return null; + return payload as PRReviewCommentCreatedPayload; +} -export const PRReviewCommentCreatedPayloadSchema = z - .object({ - action: z.string(), - pull_request: z - .object({ - number: z.number(), - user: z - .object({ - login: z.string(), - }) - .passthrough(), - }) - .passthrough(), - comment: z - .object({ - id: z.number(), - body: z.string(), - html_url: z.string(), - created_at: z.string(), - user: z - .object({ - login: z.string(), - }) - .passthrough(), - }) - .passthrough(), - sender: WebhookSenderSchema, - repository: WebhookRepositorySchema, - installation: WebhookInstallationSchema, - }) - .passthrough(); +export function parsePRReviewSubmitted( + payload: unknown, +): PRReviewSubmittedPayload | null { + if (!hasFields(payload, "submitted")) return null; + return payload as PRReviewSubmittedPayload; +} -export const PRReviewSubmittedPayloadSchema = z - .object({ - action: z.literal("submitted"), - pull_request: z - .object({ - number: z.number(), - user: z - .object({ - login: z.string(), - }) - .passthrough(), - }) - .passthrough(), - review: z - .object({ - id: z.number(), - body: z.string().nullable(), - html_url: z.string(), - submitted_at: z.string(), - user: z - .object({ - login: z.string(), - }) - .passthrough(), - }) - .passthrough(), - sender: WebhookSenderSchema, - repository: WebhookRepositorySchema, - installation: WebhookInstallationSchema, - }) - .passthrough(); - -export const WorkflowRunCompletedPayloadSchema = z - .object({ - action: z.literal("completed"), - workflow_run: z - .object({ - id: z.number(), - name: z.string(), - path: z.string().nullable().optional(), - head_sha: z.string(), - html_url: z.string(), - conclusion: z.string().nullable(), - pull_requests: z.array( - z - .object({ - number: z.number(), - }) - .passthrough(), - ), - }) - .passthrough(), - repository: WebhookRepositorySchema, - installation: WebhookInstallationSchema, - }) - .passthrough(); - -// ── Inferred types ───────────────────────────────────────────────────────────── - -export type IssuesAssignedPayload = z.infer; -export type IssuesClosedPayload = z.infer; -export type IssueCommentCreatedPayload = z.infer< - typeof IssueCommentCreatedPayloadSchema ->; -export type PRReviewCommentCreatedPayload = z.infer< - typeof PRReviewCommentCreatedPayloadSchema ->; -export type PRReviewSubmittedPayload = z.infer< - typeof PRReviewSubmittedPayloadSchema ->; -export type WorkflowRunCompletedPayload = z.infer< - typeof WorkflowRunCompletedPayloadSchema ->; +export function parseWorkflowRunCompleted( + payload: unknown, +): WorkflowRunCompletedPayload | null { + if (!hasFields(payload, "completed")) return null; + return payload as WorkflowRunCompletedPayload; +} From 3ae2672bcbb0bdb39fa1674424f6ed71ed16ab6c Mon Sep 17 00:00:00 2001 From: xmtp-coder-agent <> Date: Wed, 25 Mar 2026 17:48:18 +0000 Subject: [PATCH 2/2] refactor: switch on eventName.action for direct type narrowing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace parse functions with a single eventName.action switch in the router. Each case directly casts the payload to its specific octokit type (e.g., "issues.assigned" → IssuesAssignedPayload). This removes the intermediate parseXxx functions and makes each event/action pair map to exactly one strongly-typed handler method. webhook-schemas.ts is now purely type exports — no runtime code. Co-Authored-By: Claude Opus 4.6 --- src/webhook-router.ts | 363 +++++++++++++++++++----------------- src/webhook-schemas.test.ts | 310 +++++++++++------------------- src/webhook-schemas.ts | 80 +------- 3 files changed, 303 insertions(+), 450 deletions(-) diff --git a/src/webhook-router.ts b/src/webhook-router.ts index dec2de7..0508a3a 100644 --- a/src/webhook-router.ts +++ b/src/webhook-router.ts @@ -1,11 +1,13 @@ import type { Logger } from "./logger"; -import { - parseIssuesAssigned, - parseIssuesClosed, - parseIssueComment, - parsePRReviewComment, - parsePRReviewSubmitted, - parseWorkflowRunCompleted, +import type { + IssuesAssignedPayload, + IssuesClosedPayload, + IssueCommentCreatedPayload, + IssueCommentEditedPayload, + PRReviewCommentCreatedPayload, + PRReviewCommentEditedPayload, + PRReviewSubmittedPayload, + WorkflowRunCompletedPayload, } from "./webhook-schemas"; // ── Public types ────────────────────────────────────────────────────────────── @@ -113,19 +115,32 @@ export interface WebhookRouterOptions { logger: Logger; } -// ── Router ──────────────────────────────────────────────────────────────────── +// ── Helpers ────────────────────────────────────────────────────────────────── + +/** + * Extracts the `action` field from a webhook payload. + * Returns null if the payload is not an object or has no string `action`. + */ +function getAction(payload: unknown): string | null { + if (typeof payload !== "object" || payload === null) return null; + const action = (payload as Record).action; + return typeof action === "string" ? action : null; +} /** - * Extracts installation.id from a parsed webhook payload. - * The parse functions already verify installation.id exists, so this - * provides a safe accessor without non-null assertions. + * Extracts installation.id from a webhook payload. + * Returns 0 if installation is missing (should not happen for valid payloads). */ -function installationId(payload: { - installation?: { id: number } | null; -}): number { - return payload.installation?.id ?? 0; +function getInstallationId(payload: unknown): number { + if (typeof payload !== "object" || payload === null) return 0; + const inst = (payload as Record).installation; + if (typeof inst !== "object" || inst === null) return 0; + const id = (inst as Record).id; + return typeof id === "number" ? id : 0; } +// ── Router ──────────────────────────────────────────────────────────────────── + export class WebhookRouter { private readonly ignoredLogins: Set; @@ -136,6 +151,11 @@ export class WebhookRouter { ]); } + /** + * Routes a webhook payload to the appropriate handler based on the + * event name and action. Each event.action pair maps directly to a + * strongly-typed octokit payload type via type assertion. + */ async handleWebhook( eventName: string, deliveryId: string, @@ -143,90 +163,114 @@ export class WebhookRouter { ): Promise { this.options.logger.debug("Routing webhook", { eventName, deliveryId }); - switch (eventName) { - case "issues": - return this.routeIssues(payload); - case "issue_comment": - return this.routeIssueComment(payload); - case "pull_request_review_comment": - return this.routePRReviewComment(payload); - case "pull_request_review": - return this.routePRReview(payload); - case "workflow_run": - return this.routeWorkflowRun(payload); - default: - return { dispatched: false, reason: `Unhandled event: ${eventName}` }; - } - } - - // ── Private routing methods ────────────────────────────────────────────── + const action = getAction(payload); + const instId = getInstallationId(payload); + const eventAction = action ? `${eventName}.${action}` : eventName; + + switch (eventAction) { + case "issues.assigned": + return this.routeIssuesAssigned( + payload as IssuesAssignedPayload, + instId, + ); + + case "issues.closed": + return this.routeIssuesClosed(payload as IssuesClosedPayload, instId); + + case "issue_comment.created": + return this.routeIssueComment( + payload as IssueCommentCreatedPayload, + instId, + ); + + case "issue_comment.edited": + return this.routeIssueComment( + payload as IssueCommentEditedPayload, + instId, + ); + + case "pull_request_review_comment.created": + return this.routePRReviewComment( + payload as PRReviewCommentCreatedPayload, + instId, + ); + + case "pull_request_review_comment.edited": + return this.routePRReviewComment( + payload as PRReviewCommentEditedPayload, + instId, + ); + + case "pull_request_review.submitted": + return this.routePRReviewSubmitted( + payload as PRReviewSubmittedPayload, + instId, + ); + + case "workflow_run.completed": + return this.routeWorkflowRunCompleted( + payload as WorkflowRunCompletedPayload, + instId, + ); - private routeIssues(payload: unknown): RouteResult { - // Try "assigned" first - const assigned = parseIssuesAssigned(payload); - if (assigned) { - const assignee = assigned.assignee; - if (!assignee || assignee.login !== this.options.agentGithubUsername) { + default: return { dispatched: false, - reason: `Skipping: assignee login "${assignee?.login}" does not match agent login`, + reason: `Unhandled event: ${eventAction}`, }; - } - return { - dispatched: true, - handler: "create_task", - installationId: installationId(assigned), - context: { - issueNumber: assigned.issue.number, - issueUrl: assigned.issue.html_url, - repoName: assigned.repository.name, - repoOwner: assigned.repository.owner.login, - senderLogin: assigned.sender.login, - senderId: assigned.sender.id, - }, - }; - } - - // Try "closed" - const closed = parseIssuesClosed(payload); - if (closed) { - return { - dispatched: true, - handler: "close_task", - installationId: installationId(closed), - context: { - issueNumber: closed.issue.number, - repoName: closed.repository.name, - repoOwner: closed.repository.owner.login, - }, - }; } - - return { dispatched: false, reason: "Unhandled issues action" }; } - private routeIssueComment(payload: unknown): RouteResult { - const parsed = parseIssueComment(payload); - if (!parsed) { + // ── Typed route handlers ──────────────────────────────────────────────── + + private routeIssuesAssigned( + payload: IssuesAssignedPayload, + instId: number, + ): RouteResult { + const assignee = payload.assignee; + if (!assignee || assignee.login !== this.options.agentGithubUsername) { return { dispatched: false, - reason: "Failed to parse issue_comment payload", - validationError: true, + reason: `Skipping: assignee login "${assignee?.login}" does not match agent login`, }; } + return { + dispatched: true, + handler: "create_task", + installationId: instId, + context: { + issueNumber: payload.issue.number, + issueUrl: payload.issue.html_url, + repoName: payload.repository.name, + repoOwner: payload.repository.owner.login, + senderLogin: payload.sender.login, + senderId: payload.sender.id, + }, + }; + } - const { action, issue, comment, repository } = parsed; - const instId = installationId(parsed); - const commentUserLogin = comment.user?.login ?? ""; - const issueUserLogin = issue.user?.login ?? ""; + private routeIssuesClosed( + payload: IssuesClosedPayload, + instId: number, + ): RouteResult { + return { + dispatched: true, + handler: "close_task", + installationId: instId, + context: { + issueNumber: payload.issue.number, + repoName: payload.repository.name, + repoOwner: payload.repository.owner.login, + }, + }; + } - // Only handle created and edited actions - if (action !== "created" && action !== "edited") { - return { - dispatched: false, - reason: `Unhandled issue_comment action: ${action}`, - }; - } + private routeIssueComment( + payload: IssueCommentCreatedPayload | IssueCommentEditedPayload, + instId: number, + ): RouteResult { + const commentUserLogin = payload.comment.user?.login ?? ""; + const issueUserLogin = payload.issue.user?.login ?? ""; if (this.ignoredLogins.has(commentUserLogin)) { return { @@ -237,7 +281,7 @@ export class WebhookRouter { // Issue comment on a PR (issue.pull_request is present and non-null) // Guard: only forward comments on PRs opened by the agent - if (issue.pull_request != null) { + if (payload.issue.pull_request != null) { if (issueUserLogin !== this.options.agentGithubUsername) { return { dispatched: false, @@ -249,13 +293,13 @@ export class WebhookRouter { handler: "pr_comment", installationId: instId, context: { - issueNumber: issue.number, - commentBody: comment.body, - commentUrl: comment.html_url, - commentId: comment.id, - commentCreatedAt: comment.created_at, - repoName: repository.name, - repoOwner: repository.owner.login, + issueNumber: payload.issue.number, + commentBody: payload.comment.body, + commentUrl: payload.comment.html_url, + commentId: payload.comment.id, + commentCreatedAt: payload.comment.created_at, + repoName: payload.repository.name, + repoOwner: payload.repository.owner.login, prAuthor: issueUserLogin, commenterLogin: commentUserLogin, isReviewComment: false, @@ -270,40 +314,24 @@ export class WebhookRouter { handler: "issue_comment", installationId: instId, context: { - issueNumber: issue.number, - commentBody: comment.body, - commentUrl: comment.html_url, - commentId: comment.id, - commentCreatedAt: comment.created_at, - repoName: repository.name, - repoOwner: repository.owner.login, + issueNumber: payload.issue.number, + commentBody: payload.comment.body, + commentUrl: payload.comment.html_url, + commentId: payload.comment.id, + commentCreatedAt: payload.comment.created_at, + repoName: payload.repository.name, + repoOwner: payload.repository.owner.login, commenterLogin: commentUserLogin, }, }; } - private routePRReviewComment(payload: unknown): RouteResult { - const parsed = parsePRReviewComment(payload); - if (!parsed) { - return { - dispatched: false, - reason: "Failed to parse pull_request_review_comment payload", - validationError: true, - }; - } - - const { action, pull_request, comment, repository } = parsed; - const instId = installationId(parsed); - const prUserLogin = pull_request.user?.login ?? ""; - const commentUserLogin = comment.user?.login ?? ""; - - // Only handle created and edited actions - if (action !== "created" && action !== "edited") { - return { - dispatched: false, - reason: `Unhandled pull_request_review_comment action: ${action}`, - }; - } + private routePRReviewComment( + payload: PRReviewCommentCreatedPayload | PRReviewCommentEditedPayload, + instId: number, + ): RouteResult { + const prUserLogin = payload.pull_request.user?.login ?? ""; + const commentUserLogin = payload.comment.user?.login ?? ""; if (prUserLogin !== this.options.agentGithubUsername) { return { @@ -324,13 +352,13 @@ export class WebhookRouter { handler: "pr_comment", installationId: instId, context: { - issueNumber: pull_request.number, - commentBody: comment.body, - commentUrl: comment.html_url, - commentId: comment.id, - commentCreatedAt: comment.created_at, - repoName: repository.name, - repoOwner: repository.owner.login, + issueNumber: payload.pull_request.number, + commentBody: payload.comment.body, + commentUrl: payload.comment.html_url, + commentId: payload.comment.id, + commentCreatedAt: payload.comment.created_at, + repoName: payload.repository.name, + repoOwner: payload.repository.owner.login, prAuthor: prUserLogin, commenterLogin: commentUserLogin, isReviewComment: true, @@ -339,20 +367,12 @@ export class WebhookRouter { }; } - private routePRReview(payload: unknown): RouteResult { - const parsed = parsePRReviewSubmitted(payload); - if (!parsed) { - return { - dispatched: false, - reason: "Failed to parse pull_request_review payload", - validationError: true, - }; - } - - const { pull_request, review, repository } = parsed; - const instId = installationId(parsed); - const prUserLogin = pull_request.user?.login ?? ""; - const reviewUserLogin = review.user?.login ?? ""; + private routePRReviewSubmitted( + payload: PRReviewSubmittedPayload, + instId: number, + ): RouteResult { + const prUserLogin = payload.pull_request.user?.login ?? ""; + const reviewUserLogin = payload.review.user?.login ?? ""; if (prUserLogin !== this.options.agentGithubUsername) { return { @@ -368,7 +388,7 @@ export class WebhookRouter { }; } - if (!review.body) { + if (!payload.review.body) { return { dispatched: false, reason: "Skipping: review body is empty or null", @@ -380,13 +400,13 @@ export class WebhookRouter { handler: "pr_comment", installationId: instId, context: { - issueNumber: pull_request.number, - commentBody: review.body, - commentUrl: review.html_url, - commentId: review.id, - commentCreatedAt: review.submitted_at ?? "", - repoName: repository.name, - repoOwner: repository.owner.login, + issueNumber: payload.pull_request.number, + commentBody: payload.review.body, + commentUrl: payload.review.html_url, + commentId: payload.review.id, + commentCreatedAt: payload.review.submitted_at ?? "", + repoName: payload.repository.name, + repoOwner: payload.repository.owner.login, prAuthor: prUserLogin, commenterLogin: reviewUserLogin, isReviewComment: false, @@ -395,23 +415,14 @@ export class WebhookRouter { }; } - private routeWorkflowRun(payload: unknown): RouteResult { - const parsed = parseWorkflowRunCompleted(payload); - if (!parsed) { - return { - dispatched: false, - reason: "Failed to parse workflow_run payload", - validationError: true, - }; - } - - const { workflow_run, repository } = parsed; - const instId = installationId(parsed); - - if (workflow_run.conclusion !== "failure") { + private routeWorkflowRunCompleted( + payload: WorkflowRunCompletedPayload, + instId: number, + ): RouteResult { + if (payload.workflow_run.conclusion !== "failure") { return { dispatched: false, - reason: `Skipping: workflow_run conclusion is "${workflow_run.conclusion}", not "failure"`, + reason: `Skipping: workflow_run conclusion is "${payload.workflow_run.conclusion}", not "failure"`, }; } @@ -420,17 +431,17 @@ export class WebhookRouter { handler: "failed_check", installationId: instId, context: { - workflowRunId: workflow_run.id, - workflowName: workflow_run.name, - workflowPath: workflow_run.path ?? null, - headSha: workflow_run.head_sha, - workflowRunUrl: workflow_run.html_url, - conclusion: workflow_run.conclusion, - pullRequestNumbers: workflow_run.pull_requests + workflowRunId: payload.workflow_run.id, + workflowName: payload.workflow_run.name, + workflowPath: payload.workflow_run.path ?? null, + headSha: payload.workflow_run.head_sha, + workflowRunUrl: payload.workflow_run.html_url, + conclusion: payload.workflow_run.conclusion, + pullRequestNumbers: payload.workflow_run.pull_requests .filter((pr): pr is NonNullable => pr !== null) .map((pr) => pr.number), - repoName: repository.name, - repoOwner: repository.owner.login, + repoName: payload.repository.name, + repoOwner: payload.repository.owner.login, }, }; } diff --git a/src/webhook-schemas.test.ts b/src/webhook-schemas.test.ts index c6f9e99..f29e22c 100644 --- a/src/webhook-schemas.test.ts +++ b/src/webhook-schemas.test.ts @@ -1,12 +1,12 @@ import { describe, expect, test } from "bun:test"; -import type { WebhookEventDefinition } from "@octokit/webhooks/types"; -import { - parseIssueComment, - parseIssuesAssigned, - parseIssuesClosed, - parsePRReviewComment, - parsePRReviewSubmitted, - parseWorkflowRunCompleted, +import type { + IssuesAssignedPayload, + IssuesClosedPayload, + IssueCommentCreatedPayload, + IssueCommentEditedPayload, + PRReviewCommentCreatedPayload, + PRReviewSubmittedPayload, + WorkflowRunCompletedPayload, } from "./webhook-schemas"; import issuesAssigned from "./__fixtures__/issues-assigned.json"; @@ -20,230 +20,132 @@ import workflowRunFailure from "./__fixtures__/workflow-run-failure.json"; import workflowRunSuccess from "./__fixtures__/workflow-run-success.json"; // ── Fixture type compatibility checks ──────────────────────────────────────── -// These compile-time checks verify that fixture data is compatible with -// octokit webhook types. If a fixture drifts from the official schema, -// the build will fail. - -const _issuesAssignedCheck: WebhookEventDefinition<"issues-assigned"> = - issuesAssigned as unknown as WebhookEventDefinition<"issues-assigned">; -const _issuesClosedCheck: WebhookEventDefinition<"issues-closed"> = - issuesClosed as unknown as WebhookEventDefinition<"issues-closed">; -const _issueCommentOnPrCheck: WebhookEventDefinition<"issue-comment-created"> = - issueCommentOnPr as unknown as WebhookEventDefinition<"issue-comment-created">; -const _issueCommentOnIssueCheck: WebhookEventDefinition<"issue-comment-created"> = - issueCommentOnIssue as unknown as WebhookEventDefinition<"issue-comment-created">; -const _prReviewCommentCheck: WebhookEventDefinition<"pull-request-review-comment-created"> = - prReviewComment as unknown as WebhookEventDefinition<"pull-request-review-comment-created">; -const _prReviewSubmittedCheck: WebhookEventDefinition<"pull-request-review-submitted"> = - prReviewSubmitted as unknown as WebhookEventDefinition<"pull-request-review-submitted">; -const _workflowRunFailureCheck: WebhookEventDefinition<"workflow-run-completed"> = - workflowRunFailure as unknown as WebhookEventDefinition<"workflow-run-completed">; -const _workflowRunSuccessCheck: WebhookEventDefinition<"workflow-run-completed"> = - workflowRunSuccess as unknown as WebhookEventDefinition<"workflow-run-completed">; - -// Helper to assert non-null and return the value -function assertDefined(value: T | null | undefined): T { - if (value == null) throw new Error("Expected non-null value"); - return value; -} - -// ── parseIssuesAssigned ────────────────────────────────────────────────────── - -describe("parseIssuesAssigned", () => { - test("parses issues-assigned fixture", () => { - const result = assertDefined(parseIssuesAssigned(issuesAssigned)); - expect(result.action).toBe("assigned"); - expect(result.assignee?.login).toBe("xmtp-coder-agent"); - expect(result.issue.number).toBe(65); - expect(result.repository.full_name).toBe("xmtplabs/coder-action"); - expect(result.installation?.id).toBe(118770088); - expect(result.sender.login).toBe("neekolas"); - }); - - test("returns null for payload with wrong action", () => { - const result = parseIssuesAssigned({ ...issuesAssigned, action: "opened" }); - expect(result).toBeNull(); - }); - - test("returns null for payload with missing action", () => { - const { action: _action, ...withoutAction } = issuesAssigned; - expect(parseIssuesAssigned(withoutAction)).toBeNull(); - }); - - test("returns null for non-object payload", () => { - expect(parseIssuesAssigned(null)).toBeNull(); - expect(parseIssuesAssigned("string")).toBeNull(); - expect(parseIssuesAssigned(42)).toBeNull(); +// +// These tests verify that fixture JSON data is structurally compatible with +// the octokit webhook types. Since the types come from @octokit/webhooks, this +// ensures our test fixtures match the official GitHub webhook schema. +// +// Each test casts a fixture to its corresponding octokit type and reads key +// fields. If the fixture drifts from the official schema, these will fail +// at compile time (type error) or runtime (undefined field access). + +describe("IssuesAssignedPayload", () => { + test("fixture matches octokit type — key fields accessible", () => { + const payload = issuesAssigned as unknown as IssuesAssignedPayload; + expect(payload.action).toBe("assigned"); + expect(payload.assignee?.login).toBe("xmtp-coder-agent"); + expect(payload.issue.number).toBe(65); + expect(payload.issue.html_url).toContain("github.com"); + expect(payload.repository.full_name).toBe("xmtplabs/coder-action"); + expect(payload.repository.name).toBe("coder-action"); + expect(payload.repository.owner.login).toBe("xmtplabs"); + expect(payload.installation?.id).toBe(118770088); + expect(payload.sender.login).toBe("neekolas"); + expect(payload.sender.id).toBe(65710); }); }); -// ── parseIssuesClosed ──────────────────────────────────────────────────────── - -describe("parseIssuesClosed", () => { - test("parses issues-closed fixture", () => { - const result = assertDefined(parseIssuesClosed(issuesClosed)); - expect(result.action).toBe("closed"); - expect(result.issue.number).toBe(63); - expect(result.repository.full_name).toBe("xmtplabs/coder-action"); - expect(result.installation?.id).toBe(118770088); - }); - - test("returns null for payload with wrong action", () => { - const result = parseIssuesClosed({ ...issuesClosed, action: "opened" }); - expect(result).toBeNull(); - }); - - test("returns null for payload with missing action", () => { - const { action: _action, ...withoutAction } = issuesClosed; - expect(parseIssuesClosed(withoutAction)).toBeNull(); +describe("IssuesClosedPayload", () => { + test("fixture matches octokit type — key fields accessible", () => { + const payload = issuesClosed as unknown as IssuesClosedPayload; + expect(payload.action).toBe("closed"); + expect(payload.issue.number).toBe(63); + expect(payload.repository.full_name).toBe("xmtplabs/coder-action"); + expect(payload.installation?.id).toBe(118770088); }); }); -// ── parseIssueComment ──────────────────────────────────────────────────────── - -describe("parseIssueComment", () => { - test("parses issue-comment-on-pr fixture", () => { - const result = assertDefined(parseIssueComment(issueCommentOnPr)); - expect(result.action).toBe("created"); - expect(result.issue.number).toBe(64); - expect(result.comment.body).toBeTruthy(); - expect(result.repository.full_name).toBe("xmtplabs/coder-action"); - expect(result.installation?.id).toBe(118770088); +describe("IssueCommentCreatedPayload", () => { + test("issue-comment-on-pr fixture matches octokit type", () => { + const payload = issueCommentOnPr as unknown as IssueCommentCreatedPayload; + expect(payload.action).toBe("created"); + expect(payload.issue.number).toBe(64); + expect(payload.comment.body).toBeTruthy(); + expect(payload.comment.html_url).toContain("github.com"); + expect(payload.comment.created_at).toBeTruthy(); + expect(payload.comment.user?.login).toBeTruthy(); + expect(payload.repository.full_name).toBe("xmtplabs/coder-action"); + expect(payload.installation?.id).toBe(118770088); }); test("issue-comment-on-pr has issue.pull_request truthy", () => { - const result = assertDefined(parseIssueComment(issueCommentOnPr)); - expect(result.issue.pull_request).toBeTruthy(); + const payload = issueCommentOnPr as unknown as IssueCommentCreatedPayload; + expect(payload.issue.pull_request).toBeTruthy(); }); - test("parses issue-comment-on-issue fixture", () => { - const result = assertDefined(parseIssueComment(issueCommentOnIssue)); - expect(result.action).toBe("created"); - expect(result.issue.number).toBe(65); + test("issue-comment-on-issue fixture matches octokit type", () => { + const payload = + issueCommentOnIssue as unknown as IssueCommentCreatedPayload; + expect(payload.action).toBe("created"); + expect(payload.issue.number).toBe(65); }); test("issue-comment-on-issue has issue.pull_request falsy", () => { - const result = assertDefined(parseIssueComment(issueCommentOnIssue)); - expect(result.issue.pull_request).toBeFalsy(); - }); - - test("returns null for payload with missing action", () => { - const { action: _action, ...withoutAction } = issueCommentOnPr; - expect(parseIssueComment(withoutAction)).toBeNull(); - }); - - test("parses edited issue-comment-on-issue payload", () => { - const payload = { ...issueCommentOnIssue, action: "edited" }; - const result = assertDefined(parseIssueComment(payload)); - expect(result.action).toBe("edited"); - expect(result.issue.number).toBe(65); - }); - - test("parses edited issue-comment-on-pr payload", () => { - const payload = { ...issueCommentOnPr, action: "edited" }; - const result = assertDefined(parseIssueComment(payload)); - expect(result.action).toBe("edited"); - expect(result.issue.number).toBe(64); - expect(result.issue.pull_request).toBeTruthy(); + const payload = + issueCommentOnIssue as unknown as IssueCommentCreatedPayload; + expect(payload.issue.pull_request).toBeFalsy(); }); - test("returns null for non-object payload", () => { - expect(parseIssueComment(null)).toBeNull(); - expect(parseIssueComment("string")).toBeNull(); + test("edited fixture matches IssueCommentEditedPayload", () => { + const fixture = { ...issueCommentOnIssue, action: "edited" }; + const payload = fixture as unknown as IssueCommentEditedPayload; + expect(payload.action).toBe("edited"); + expect(payload.issue.number).toBe(65); }); }); -// ── parsePRReviewComment ───────────────────────────────────────────────────── - -describe("parsePRReviewComment", () => { - test("parses pr-review-comment fixture", () => { - const result = assertDefined(parsePRReviewComment(prReviewComment)); - expect(result.action).toBe("created"); - expect(result.pull_request.number).toBe(64); - expect(result.pull_request.user?.login).toBe("xmtp-coder-agent"); - expect(result.comment.user?.login).toBe("neekolas"); - expect(result.repository.full_name).toBe("xmtplabs/coder-action"); - expect(result.installation?.id).toBe(118770088); - }); - - test("returns null for payload with missing action", () => { - const { action: _action, ...withoutAction } = prReviewComment; - expect(parsePRReviewComment(withoutAction)).toBeNull(); - }); - - test("parses edited pr-review-comment payload", () => { - const payload = { ...prReviewComment, action: "edited" }; - const result = assertDefined(parsePRReviewComment(payload)); - expect(result.action).toBe("edited"); - expect(result.pull_request.number).toBe(64); +describe("PRReviewCommentCreatedPayload", () => { + test("fixture matches octokit type — key fields accessible", () => { + const payload = prReviewComment as unknown as PRReviewCommentCreatedPayload; + expect(payload.action).toBe("created"); + expect(payload.pull_request.number).toBe(64); + expect(payload.pull_request.user?.login).toBe("xmtp-coder-agent"); + expect(payload.comment.user?.login).toBe("neekolas"); + expect(payload.comment.body).toBeTruthy(); + expect(payload.repository.full_name).toBe("xmtplabs/coder-action"); + expect(payload.installation?.id).toBe(118770088); }); }); -// ── parsePRReviewSubmitted ─────────────────────────────────────────────────── - -describe("parsePRReviewSubmitted", () => { - test("parses pr-review-submitted fixture", () => { - const result = assertDefined(parsePRReviewSubmitted(prReviewSubmitted)); - expect(result.action).toBe("submitted"); - expect(result.pull_request.number).toBe(64); - expect(result.pull_request.user?.login).toBe("xmtp-coder-agent"); - expect(result.review.body).toBe("Please fix the naming"); - expect(result.review.user?.login).toBe("neekolas"); - expect(result.repository.full_name).toBe("xmtplabs/coder-action"); - expect(result.installation?.id).toBe(118770088); - }); - - test("parses pr-review-submitted-empty fixture (null body)", () => { - const result = assertDefined( - parsePRReviewSubmitted(prReviewSubmittedEmpty), - ); - expect(result.action).toBe("submitted"); - expect(result.review.body).toBeNull(); - }); - - test("returns null for payload with missing action", () => { - const { action: _action, ...withoutAction } = prReviewSubmitted; - expect(parsePRReviewSubmitted(withoutAction)).toBeNull(); +describe("PRReviewSubmittedPayload", () => { + test("fixture matches octokit type — key fields accessible", () => { + const payload = prReviewSubmitted as unknown as PRReviewSubmittedPayload; + expect(payload.action).toBe("submitted"); + expect(payload.pull_request.number).toBe(64); + expect(payload.pull_request.user?.login).toBe("xmtp-coder-agent"); + expect(payload.review.body).toBe("Please fix the naming"); + expect(payload.review.user?.login).toBe("neekolas"); + expect(payload.review.html_url).toContain("github.com"); + expect(payload.repository.full_name).toBe("xmtplabs/coder-action"); + expect(payload.installation?.id).toBe(118770088); + }); + + test("empty-body fixture has null review body", () => { + const payload = + prReviewSubmittedEmpty as unknown as PRReviewSubmittedPayload; + expect(payload.action).toBe("submitted"); + expect(payload.review.body).toBeNull(); }); }); -// ── parseWorkflowRunCompleted ──────────────────────────────────────────────── - -describe("parseWorkflowRunCompleted", () => { - test("parses workflow-run-failure fixture", () => { - const result = assertDefined(parseWorkflowRunCompleted(workflowRunFailure)); - expect(result.action).toBe("completed"); - expect(result.workflow_run.conclusion).toBe("failure"); - expect(result.workflow_run.head_sha).toBe( +describe("WorkflowRunCompletedPayload", () => { + test("failure fixture matches octokit type — key fields accessible", () => { + const payload = + workflowRunFailure as unknown as WorkflowRunCompletedPayload; + expect(payload.action).toBe("completed"); + expect(payload.workflow_run.conclusion).toBe("failure"); + expect(payload.workflow_run.id).toBe(23526809052); + expect(payload.workflow_run.head_sha).toBe( "dbbd661d51e80fbbcfb1fcc2cd7446f661d08016", ); - expect(result.workflow_run.pull_requests[0]?.number).toBe(64); - expect(result.repository.full_name).toBe("xmtplabs/coder-action"); - expect(result.installation?.id).toBe(118770088); - }); - - test("workflow-run-failure has conclusion: failure", () => { - const result = assertDefined(parseWorkflowRunCompleted(workflowRunFailure)); - expect(result.workflow_run.conclusion).toBe("failure"); - }); - - test("parses workflow-run-success fixture", () => { - const result = assertDefined(parseWorkflowRunCompleted(workflowRunSuccess)); - expect(result.workflow_run.conclusion).toBe("success"); - }); - - test("workflow-run-success has conclusion: success", () => { - const result = assertDefined(parseWorkflowRunCompleted(workflowRunSuccess)); - expect(result.workflow_run.conclusion).toBe("success"); - }); - - test("returns null for payload with missing action", () => { - const { action: _action, ...withoutAction } = workflowRunFailure; - expect(parseWorkflowRunCompleted(withoutAction)).toBeNull(); + expect(payload.workflow_run.pull_requests[0]?.number).toBe(64); + expect(payload.repository.full_name).toBe("xmtplabs/coder-action"); + expect(payload.installation?.id).toBe(118770088); }); - test("returns null for non-object payload", () => { - expect(parseWorkflowRunCompleted(null)).toBeNull(); - expect(parseWorkflowRunCompleted("string")).toBeNull(); + test("success fixture has conclusion: success", () => { + const payload = + workflowRunSuccess as unknown as WorkflowRunCompletedPayload; + expect(payload.workflow_run.conclusion).toBe("success"); }); }); diff --git a/src/webhook-schemas.ts b/src/webhook-schemas.ts index 7592705..0afda99 100644 --- a/src/webhook-schemas.ts +++ b/src/webhook-schemas.ts @@ -1,82 +1,22 @@ import type { WebhookEventDefinition } from "@octokit/webhooks/types"; // ── Octokit webhook event types ────────────────────────────────────────────── +// +// Each type corresponds to a specific GitHub webhook event + action pair. +// After signature verification, payloads can be safely cast to these types +// based on the X-GitHub-Event header and the payload's action field. export type IssuesAssignedPayload = WebhookEventDefinition<"issues-assigned">; export type IssuesClosedPayload = WebhookEventDefinition<"issues-closed">; export type IssueCommentCreatedPayload = - | WebhookEventDefinition<"issue-comment-created"> - | WebhookEventDefinition<"issue-comment-edited">; + WebhookEventDefinition<"issue-comment-created">; +export type IssueCommentEditedPayload = + WebhookEventDefinition<"issue-comment-edited">; export type PRReviewCommentCreatedPayload = - | WebhookEventDefinition<"pull-request-review-comment-created"> - | WebhookEventDefinition<"pull-request-review-comment-edited">; + WebhookEventDefinition<"pull-request-review-comment-created">; +export type PRReviewCommentEditedPayload = + WebhookEventDefinition<"pull-request-review-comment-edited">; export type PRReviewSubmittedPayload = WebhookEventDefinition<"pull-request-review-submitted">; export type WorkflowRunCompletedPayload = WebhookEventDefinition<"workflow-run-completed">; - -// ── Type-narrowing helpers ─────────────────────────────────────────────────── -// -// After webhook signature verification, the payload is guaranteed to come from -// GitHub. These functions check the `action` field to narrow to specific event -// types and verify that required fields (like `installation`) are present. - -function hasFields( - payload: unknown, - action: string | string[], -): payload is Record { - if (typeof payload !== "object" || payload === null) return false; - const obj = payload as Record; - const actions = Array.isArray(action) ? action : [action]; - if (!actions.includes(obj.action as string)) return false; - if ( - typeof obj.installation !== "object" || - obj.installation === null || - typeof (obj.installation as Record).id !== "number" - ) { - return false; - } - return true; -} - -export function parseIssuesAssigned( - payload: unknown, -): IssuesAssignedPayload | null { - if (!hasFields(payload, "assigned")) return null; - return payload as IssuesAssignedPayload; -} - -export function parseIssuesClosed( - payload: unknown, -): IssuesClosedPayload | null { - if (!hasFields(payload, "closed")) return null; - return payload as IssuesClosedPayload; -} - -export function parseIssueComment( - payload: unknown, -): IssueCommentCreatedPayload | null { - if (!hasFields(payload, ["created", "edited", "deleted"])) return null; - return payload as IssueCommentCreatedPayload; -} - -export function parsePRReviewComment( - payload: unknown, -): PRReviewCommentCreatedPayload | null { - if (!hasFields(payload, ["created", "edited", "deleted"])) return null; - return payload as PRReviewCommentCreatedPayload; -} - -export function parsePRReviewSubmitted( - payload: unknown, -): PRReviewSubmittedPayload | null { - if (!hasFields(payload, "submitted")) return null; - return payload as PRReviewSubmittedPayload; -} - -export function parseWorkflowRunCompleted( - payload: unknown, -): WorkflowRunCompletedPayload | null { - if (!hasFields(payload, "completed")) return null; - return payload as WorkflowRunCompletedPayload; -}