From 9a0dac714c83d9ed8ac785f6e5430c9b77775191 Mon Sep 17 00:00:00 2001 From: TOMOKI977 Date: Fri, 25 Sep 2026 13:25:54 -0400 Subject: [PATCH 1/3] test: extract shared Telegram API fetch stub --- test/http/webhook-e2e.test.ts | 44 +++++++++--------------------- test/support/telegram-stub.ts | 51 +++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 32 deletions(-) create mode 100644 test/support/telegram-stub.ts diff --git a/test/http/webhook-e2e.test.ts b/test/http/webhook-e2e.test.ts index effe0b0..42f29e0 100644 --- a/test/http/webhook-e2e.test.ts +++ b/test/http/webhook-e2e.test.ts @@ -2,6 +2,7 @@ import { env } from "cloudflare:test"; import { afterEach, describe, expect, it, vi } from "vitest"; import app from "../../src/index"; import type { Env } from "../../src/index"; +import { stubTelegramApi } from "../support/telegram-stub"; // REL-001: the assembled route was never proven end-to-end — this file // drives real updates through the actual Hono route + `buildBot` (real @@ -13,38 +14,17 @@ import type { Env } from "../../src/index"; const WEBHOOK_SECRET = (env as unknown as { WEBHOOK_SECRET: string }).WEBHOOK_SECRET; -// Seam note: grammY's `Bot`/`ApiClient` resolves the bare `fetch` identifier -// at construction time (see node_modules/grammy/out/web.mjs — `const -// fetchFn = customFetch ?? fetch;`), and `composition.ts` builds a fresh -// `Bot` per request. Since @cloudflare/vitest-pool-workers runs the `main` -// worker in the SAME isolate as the test file (its own module doc: -// "this `main` worker runs in the same isolate/context as tests, so any -// global mocks will apply to it too"), stubbing `globalThis.fetch` before -// the request is enough to intercept grammY's outbound calls — no -// production seam was needed in `buildBot`/`composition.ts`. -type TelegramCall = { method: string; body: unknown }; - -function stubTelegramApi(handler?: (method: string, body: unknown) => unknown) { - const calls: TelegramCall[] = []; - vi.stubGlobal( - "fetch", - async (input: RequestInfo | URL, init?: RequestInit) => { - const url = typeof input === "string" ? input : input.toString(); - const method = url.split("/").pop() ?? ""; - const body = init?.body ? JSON.parse(String(init.body)) : undefined; - calls.push({ method, body }); - const result = - handler?.(method, body) ?? - (method === "getChatMember" - ? { status: "administrator", user: { id: 1, is_bot: false, first_name: "Admin" } } - : { message_id: calls.length, date: 0, chat: { id: 1, type: "supergroup" } }); - return new Response(JSON.stringify({ ok: true, result }), { - headers: { "content-type": "application/json" }, - }); - }, - ); - return calls; -} +// Seam note (READ-002: the stub itself is now shared — see +// test/support/telegram-stub.ts): grammY's `Bot`/`ApiClient` resolves the +// bare `fetch` identifier at construction time (see +// node_modules/grammy/out/web.mjs — `const fetchFn = customFetch ?? +// fetch;`), and `composition.ts` builds a fresh `Bot` per request. Since +// @cloudflare/vitest-pool-workers runs the `main` worker in the SAME +// isolate as the test file (its own module doc: "this `main` worker runs +// in the same isolate/context as tests, so any global mocks will apply to +// it too"), stubbing `globalThis.fetch` before the request is enough to +// intercept grammY's outbound calls — no production seam was needed in +// `buildBot`/`composition.ts`. let nextUpdateId = 1000; function commandUpdate(command: string, chatId: number, userId: number) { diff --git a/test/support/telegram-stub.ts b/test/support/telegram-stub.ts new file mode 100644 index 0000000..09361e7 --- /dev/null +++ b/test/support/telegram-stub.ts @@ -0,0 +1,51 @@ +import { vi } from "vitest"; + +// READ-002 (PR4 correction): shared outbound-Telegram-HTTP stub, extracted +// from the copy this file replaces in test/http/webhook-e2e.test.ts and the +// near-duplicates in test/adapters/telegram/alert-sender.test.ts and +// test/http/github-webhook-delivery-e2e.test.ts. +// +// Seam note (from the original webhook-e2e.test.ts doc comment): grammY's +// `Bot`/`Api`/`ApiClient` resolves the bare `fetch` identifier at +// construction time (see node_modules/grammy/out/web.mjs), and +// @cloudflare/vitest-pool-workers runs the `main` worker in the SAME +// isolate as the test file, so stubbing `globalThis.fetch` before the +// request is enough to intercept grammY's outbound calls — no production +// seam is needed. +export type TelegramCall = { method: string; body: unknown }; + +// `handler` may return either: +// - a full Telegram API response shape (`{ ok: boolean; ... }`, e.g. an +// `{ ok: false, error_code, description }` failure) — sent as-is, or +// - a partial `result` payload (or `undefined`) — wrapped in +// `{ ok: true, result }`, with a default `getChatMember`/`sendMessage` +// shape when no handler/result is given. +export function stubTelegramApi( + handler?: (method: string, body: unknown) => unknown, +): TelegramCall[] { + const calls: TelegramCall[] = []; + vi.stubGlobal( + "fetch", + async (input: RequestInfo | URL, init?: RequestInit) => { + const url = typeof input === "string" ? input : input.toString(); + const method = url.split("/").pop() ?? ""; + const body = init?.body ? JSON.parse(String(init.body)) : undefined; + calls.push({ method, body }); + const result = handler?.(method, body); + if (result && typeof result === "object" && "ok" in result) { + return new Response(JSON.stringify(result), { + headers: { "content-type": "application/json" }, + }); + } + const defaultResult = + result ?? + (method === "getChatMember" + ? { status: "administrator", user: { id: 1, is_bot: false, first_name: "Admin" } } + : { message_id: calls.length, date: 0, chat: { id: 1, type: "supergroup" } }); + return new Response(JSON.stringify({ ok: true, result: defaultResult }), { + headers: { "content-type": "application/json" }, + }); + }, + ); + return calls; +} From 3715928e9e41a6497d71f01eca6afe271424b888 Mon Sep 17 00:00:00 2001 From: TOMOKI977 Date: Fri, 25 Sep 2026 13:25:55 -0400 Subject: [PATCH 2/3] feat(github): deliver issue and PR alerts to linked topics Maps pull_request (opened, closed/merged, review_requested) and issues (opened, closed) payloads to allowlisted GithubEvent fields and routes them through org claim, team and repo link to the linked forum topic via sendMessage with message_thread_id. Unsupported events, unclaimed orgs and unlinked repos are acknowledged and logged; Telegram send failures return 2xx with a fixed failure class (rate-limited, telegram-unavailable, rejected); D1 failures return 500. --- src/adapters/github/event-mapper.ts | 102 +++++++ src/adapters/telegram/alert-sender.ts | 53 ++++ src/composition.ts | 19 ++ src/domain/errors.ts | 22 +- src/domain/usecases/route-github-event.ts | 8 +- src/index.ts | 69 ++++- test/adapters/github/event-mapper.test.ts | 203 +++++++++++++ test/adapters/telegram/alert-sender.test.ts | 114 +++++++ test/domain/route-github-event.test.ts | 10 +- test/fakes/index.ts | 7 +- test/http/github-webhook-delivery-e2e.test.ts | 277 ++++++++++++++++++ test/http/github-webhook.test.ts | 19 +- 12 files changed, 878 insertions(+), 25 deletions(-) create mode 100644 src/adapters/github/event-mapper.ts create mode 100644 src/adapters/telegram/alert-sender.ts create mode 100644 test/adapters/github/event-mapper.test.ts create mode 100644 test/adapters/telegram/alert-sender.test.ts create mode 100644 test/http/github-webhook-delivery-e2e.test.ts diff --git a/src/adapters/github/event-mapper.ts b/src/adapters/github/event-mapper.ts new file mode 100644 index 0000000..976de1c --- /dev/null +++ b/src/adapters/github/event-mapper.ts @@ -0,0 +1,102 @@ +import { parseRepoFullName } from "../../domain/github"; +import type { + GithubEvent, + GithubEventAction, + GithubEventKind, +} from "../../domain/github"; + +// Adapter mapper (design.md "Architecture Decisions", Event filtering): the +// ONLY place that reads raw GitHub webhook JSON. Reads only the allowlisted +// fields needed for GithubEvent — never a commit author, an email, or any +// other payload field — and returns null for anything outside the +// supported event/action set (spec: github-webhook "Unsupported Event or +// Action Ignored"). The domain never sees this payload shape. + +const SUPPORTED_PR_ACTIONS = new Set(["opened", "closed", "review_requested"]); +const SUPPORTED_ISSUE_ACTIONS = new Set(["opened", "closed"]); + +interface RawPayload { + action?: unknown; + number?: unknown; + sender?: { login?: unknown }; + repository?: { full_name?: unknown }; + pull_request?: { title?: unknown; html_url?: unknown; merged?: unknown }; + issue?: { title?: unknown; html_url?: unknown }; + requested_reviewer?: { login?: unknown }; + requested_team?: { slug?: unknown }; +} + +function asString(value: unknown): string | null { + return typeof value === "string" ? value : null; +} + +export function mapGithubEvent( + githubEventType: string, + payload: unknown, +): GithubEvent | null { + if (githubEventType !== "pull_request" && githubEventType !== "issues") { + return null; + } + if (typeof payload !== "object" || payload === null || Array.isArray(payload)) { + return null; + } + const raw = payload as RawPayload; + + const fullName = asString(raw.repository?.full_name); + const repo = fullName !== null ? parseRepoFullName(fullName) : null; + if (!repo) return null; + // Derived from the already-lowercased, validated repo (design.md/task 4.1: + // "org and repo are lowercased consistently with parseRepoFullName and the + // claim repo"), never from a separately-cased payload field. `repo` + // already matched REPO_FULL_NAME_PATTERN, so the "/" split always yields + // a non-empty first segment. + const org = repo.slice(0, repo.indexOf("/")); + + const action = asString(raw.action); + const number = typeof raw.number === "number" ? raw.number : null; + const actor = asString(raw.sender?.login); + if (action === null || number === null || actor === null) return null; + + const kind: GithubEventKind = githubEventType; + let resolvedAction: GithubEventAction; + let title: string | null; + let url: string | null; + let reviewer: string | undefined; + + if (kind === "pull_request") { + if (!SUPPORTED_PR_ACTIONS.has(action)) return null; + title = asString(raw.pull_request?.title); + url = asString(raw.pull_request?.html_url); + + if (action === "closed") { + resolvedAction = raw.pull_request?.merged === true ? "merged" : "closed"; + } else if (action === "review_requested") { + resolvedAction = "review_requested"; + reviewer = + asString(raw.requested_reviewer?.login) ?? + asString(raw.requested_team?.slug) ?? + undefined; + } else { + resolvedAction = "opened"; + } + } else { + if (!SUPPORTED_ISSUE_ACTIONS.has(action)) return null; + resolvedAction = action as GithubEventAction; + title = asString(raw.issue?.title); + url = asString(raw.issue?.html_url); + } + + if (title === null || url === null) return null; + + return { + org, + repo, + kind, + action: resolvedAction, + number, + title, + url, + actor, + ...(reviewer !== undefined ? { reviewer } : {}), + }; +} diff --git a/src/adapters/telegram/alert-sender.ts b/src/adapters/telegram/alert-sender.ts new file mode 100644 index 0000000..ef2b0b1 --- /dev/null +++ b/src/adapters/telegram/alert-sender.ts @@ -0,0 +1,53 @@ +import { GrammyError, HttpError } from "grammy"; +import type { Api } from "grammy"; +import { AlertSendFailedError } from "../../domain/errors"; +import type { AlertSendFailureClass } from "../../domain/errors"; +import type { AlertSender } from "../../domain/ports"; + +// design.md "Sender": `new Api(BOT_TOKEN)` in composition, no `Bot` and no +// `PII_KEYRING` on this route. Plain text (no `parse_mode`) so a title with +// special characters cannot break sending via a MarkdownV2/HTML escaping +// bug (design.md "Message"). `link_preview_options.is_disabled` keeps the +// alert compact — a PR/issue URL preview adds nothing here. + +// PR4 correction (RES-001): classifies the underlying failure into a +// fixed, non-sensitive bucket the caller can safely log as `reason`. This +// never changes the spec'd 2xx/no-retry behavior — only what a caller may +// log. `GrammyError` is Telegram's own JSON-encoded `{ok:false, +// error_code, description}` response (a genuine API-level rejection); +// anything else (a non-JSON/5xx response, or the fetch call itself +// failing) surfaces as grammY's `HttpError` and is treated as +// "telegram-unavailable" — the same bucket used for a Telegram-side 5xx. +function classifyFailure(err: unknown): AlertSendFailureClass { + if (err instanceof GrammyError) { + if (err.error_code === 429) return "rate-limited"; + if (err.error_code >= 500) return "telegram-unavailable"; + return "rejected"; + } + if (err instanceof HttpError) return "telegram-unavailable"; + return "telegram-unavailable"; +} + +// A failed send (e.g. the linked topic was deleted, a rate limit, or +// Telegram being unavailable) is a permanent-for-this-request delivery +// failure GitHub does not need to redeliver, so it is converted to +// AlertSendFailedError rather than propagated as-is — the port contract +// (ports.ts) requires this so routeGithubEvent can report "send-failed" +// instead of a 500 (design.md "GitHub route status policy"). The thrown +// error's message is always a fixed, non-sensitive string — Telegram's +// `description` (which can echo operator-specific detail) is never +// carried over, and neither is the chat id or the token. +export function createTelegramAlertSender(api: Api): AlertSender { + return { + async send(chatId: number, threadId: number, text: string): Promise { + try { + await api.sendMessage(chatId, text, { + message_thread_id: threadId, + link_preview_options: { is_disabled: true }, + }); + } catch (err) { + throw new AlertSendFailedError("sendMessage failed", classifyFailure(err)); + } + }, + }; +} diff --git a/src/composition.ts b/src/composition.ts index 168ae34..a3c843f 100644 --- a/src/composition.ts +++ b/src/composition.ts @@ -1,16 +1,21 @@ +import { Api } from "grammy"; import { createAesGcmCipher } from "./adapters/crypto/aes-gcm-cipher"; import { parseKeyRing } from "./adapters/crypto/key-ring"; import { createD1DmSelectionRepo } from "./adapters/d1/dm-selection-repo"; +import { createD1GithubOrgClaimRepo } from "./adapters/d1/github-org-claim-repo"; import { createD1MemberRepo } from "./adapters/d1/member-repo"; import { createD1MembershipRepo } from "./adapters/d1/membership-repo"; import { createD1ProfileRepo } from "./adapters/d1/profile-repo"; +import { createD1RepoTopicLinkRepo } from "./adapters/d1/repo-topic-link-repo"; import { createD1TeamRepo } from "./adapters/d1/team-repo"; import { createSafeLogger } from "./adapters/log/safe-logger"; +import { createTelegramAlertSender } from "./adapters/telegram/alert-sender"; import { createBot } from "./adapters/telegram/bot"; import { createChatAdminChecker } from "./adapters/telegram/chat-admin-checker"; import { registerCommands } from "./adapters/telegram/commands"; import { ConfigError } from "./config-error"; import type { FieldCipher } from "./domain/ports"; +import type { RouteGithubEventDeps } from "./domain/usecases/route-github-event"; import type { UserFromGetMe } from "grammy/types"; import type { Env } from "./env"; @@ -93,3 +98,17 @@ export function buildBot(env: Env) { return bot; } + +// design.md "Sender": `new Api(BOT_TOKEN)` here, no `Bot` and no +// `PII_KEYRING` on this route — a broken keyring would otherwise break +// GitHub alerts too, and this composition path never touches PII fields. +export function buildGithubRouter(env: Env): RouteGithubEventDeps { + const api = new Api(env.BOT_TOKEN); + + return { + githubOrgClaimRepo: createD1GithubOrgClaimRepo(env.DB), + repoTopicLinkRepo: createD1RepoTopicLinkRepo(env.DB), + teamRepo: createD1TeamRepo(env.DB, idGen, clock), + alertSender: createTelegramAlertSender(api), + }; +} diff --git a/src/domain/errors.ts b/src/domain/errors.ts index 2d14f28..fb15a23 100644 --- a/src/domain/errors.ts +++ b/src/domain/errors.ts @@ -14,12 +14,32 @@ export class DmSelectionRequiredError extends DomainError {} export class FieldUnreadableError extends DomainError {} export class InvalidRepoError extends DomainError {} export class OrgNotClaimedError extends DomainError {} +// A fixed, non-sensitive classification of why a send failed — never +// Telegram's error description, the chat id, or the token (design.md +// "Logging"; PR4 correction RES-001). "rate-limited" is a 429, "rejected" +// is any other 4xx (e.g. the topic was deleted), "telegram-unavailable" is +// a 5xx or a network/transport failure (grammY's HttpError). All three are +// still a permanent-for-this-request delivery failure per the spec ("no +// retry within the same request") — the classification only changes what +// is logged, never the 2xx/no-retry behavior. +export type AlertSendFailureClass = + | "rate-limited" + | "rejected" + | "telegram-unavailable"; + // Thrown by an AlertSender implementation when the underlying send fails // (e.g. Telegram rejects the request because the topic was deleted). // route-github-event.ts catches this specific error and returns a // "send-failed" outcome instead of letting it propagate (design.md // "GitHub route status policy"). -export class AlertSendFailedError extends DomainError {} +export class AlertSendFailedError extends DomainError { + readonly failureClass: AlertSendFailureClass; + + constructor(message: string, failureClass: AlertSendFailureClass) { + super(message); + this.failureClass = failureClass; + } +} // Thrown when a tenant-scoped write's explicit `teamId` argument disagrees // with the `teamId` embedded in the entity being written (e.g. // RepoTopicLinkRepo.upsert). The explicit argument is always authoritative diff --git a/src/domain/usecases/route-github-event.ts b/src/domain/usecases/route-github-event.ts index ff309ca..b15236e 100644 --- a/src/domain/usecases/route-github-event.ts +++ b/src/domain/usecases/route-github-event.ts @@ -1,4 +1,5 @@ import { AlertSendFailedError, NotFoundError } from "../errors"; +import type { AlertSendFailureClass } from "../errors"; import { formatGithubAlert } from "../github"; import type { GithubEvent } from "../github"; import type { TeamId } from "../ids"; @@ -18,7 +19,10 @@ export interface RouteGithubEventDeps { export type RouteGithubEventResult = | { kind: "delivered"; teamId: TeamId } - | { kind: "send-failed"; teamId: TeamId } + // PR4 correction (RES-001): failureClass is AlertSendFailedError's fixed, + // non-sensitive classification — carried through so the HTTP adapter can + // log a distinguishable reason without ever seeing Telegram's raw error. + | { kind: "send-failed"; teamId: TeamId; failureClass: AlertSendFailureClass } | { kind: "ignored"; reason: "unclaimed-org" | "unlinked-repo" }; // design.md "GitHub route status policy": unclaimed org or unlinked repo @@ -54,7 +58,7 @@ export async function routeGithubEvent( await deps.alertSender.send(team.chatId, link.threadId, text); } catch (err) { if (err instanceof AlertSendFailedError) { - return { kind: "send-failed", teamId }; + return { kind: "send-failed", teamId, failureClass: err.failureClass }; } throw err; } diff --git a/src/index.ts b/src/index.ts index 76692cf..12fc911 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,10 +1,12 @@ import { Hono } from "hono"; import type { Update } from "grammy/types"; import { createSafeLogger } from "./adapters/log/safe-logger"; +import { mapGithubEvent } from "./adapters/github/event-mapper"; import { verifyGithubSignature } from "./adapters/github/signature"; import { timingSafeCompare } from "./adapters/crypto/timing-safe-compare"; -import { buildBot } from "./composition"; +import { buildBot, buildGithubRouter } from "./composition"; import { ConfigError } from "./config-error"; +import { routeGithubEvent } from "./domain/usecases/route-github-event"; import type { Env } from "./env"; export type { Env } from "./env"; @@ -86,11 +88,10 @@ app.post("/telegram/webhook", async (c) => { return c.text("ok"); }); -// design.md "GitHub route status policy" — route skeleton only (PR3). The -// mapper, org/repo routing and Telegram delivery land in Phase 4; until -// then every signature-verified, well-formed, non-ping event is -// acknowledged and dropped (the same 2xx the design table gives an -// unsupported event/action, since nothing is wired to support one yet). +// design.md "GitHub route status policy": HMAC-verify the raw body, map +// the payload to an allowlisted domain event (or drop it as unsupported), +// then route it to the linked team's forum topic through the composition +// root (buildGithubRouter). No payload field is ever logged. app.post("/github/webhook", async (c) => { // RES-001, corrected: an unreadable raw body is a transient, transport- // level failure (e.g. a broken/aborted request stream) on a request that @@ -146,15 +147,59 @@ app.post("/github/webhook", async (c) => { return c.text("ok", 200); } - if (c.req.header("X-GitHub-Event") === "ping") { + const githubEventType = c.req.header("X-GitHub-Event") ?? ""; + if (githubEventType === "ping") { + return c.text("ok", 200); + } + + // The mapper (adapters/github/event-mapper.ts) is the only place that + // reads the raw payload shape (design.md "Event filtering"). `null` + // covers every unsupported event type or action (spec: "Unsupported + // Event or Action Ignored") — 200, logged, no payload field in `reason`. + const event = mapGithubEvent(githubEventType, payload); + if (!event) { + logger.log({ event: "github-webhook", outcome: "ok", reason: "ignored:unsupported-event" }); + return c.text("ok", 200); + } + + let result: Awaited>; + try { + result = await routeGithubEvent(event, buildGithubRouter(c.env)); + } catch (err) { + // Unexpected failure (e.g. D1) — design.md "GitHub route status + // policy": 500, logged by error name only, so the delivery stays + // visible in GitHub for a manual redeliver. + logger.log({ + event: "github-webhook", + outcome: "error", + errorCode: err instanceof Error ? err.name : "UnknownError", + }); + return c.text("Internal Server Error", 500); + } + + if (result.kind === "ignored") { + // `reason` is one of a fixed, non-sensitive set ("unclaimed-org", + // "unlinked-repo") — never a repo name or any payload field. + logger.log({ event: "github-webhook", outcome: "ok", reason: `ignored:${result.reason}` }); + return c.text("ok", 200); + } + if (result.kind === "send-failed") { + // Telegram delivery failure (e.g. the topic was deleted, a rate limit, + // or Telegram being unavailable) is a permanent-for-this-request + // failure, not an infrastructure one — 2xx, no retry (design.md + // "GitHub route status policy", spec: "Delivery Failure Is Logged and + // Acknowledged"). `reason` is AlertSendFailedError's fixed, + // non-sensitive classification (PR4 correction RES-001) — never + // Telegram's error description, the chat id, or the token. + logger.log({ + event: "github-webhook", + outcome: "error", + errorCode: "AlertSendFailed", + reason: result.failureClass, + }); return c.text("ok", 200); } - // Placeholder until Phase 4 wires the mapper/router: acknowledge, drop, - // and log (RES-002 / design.md:27 "unsupported event or action: 200, - // logged"). `reason` is a fixed, non-sensitive string per the logging - // allowlist — never the event type, action, or any payload field. - logger.log({ event: "github-webhook", outcome: "ok", reason: "ignored:not-yet-routed" }); return c.text("ok", 200); }); diff --git a/test/adapters/github/event-mapper.test.ts b/test/adapters/github/event-mapper.test.ts new file mode 100644 index 0000000..dbce932 --- /dev/null +++ b/test/adapters/github/event-mapper.test.ts @@ -0,0 +1,203 @@ +import { describe, expect, it } from "vitest"; +import { mapGithubEvent } from "../../../src/adapters/github/event-mapper"; + +// design.md "Event filtering": the adapter mapper is the only place that +// reads raw webhook JSON and turns it into an allowlisted GithubEvent, or +// null when the event/action combination is outside the supported set +// (spec: github-webhook "Unsupported Event or Action Ignored"). + +const COMMIT_EMAIL_MARKER = "leak-marker@example.com"; + +function pullRequestPayload(overrides: Record = {}) { + return { + action: "opened", + number: 42, + sender: { login: "octocat" }, + repository: { full_name: "Octocat/Hello-World" }, + pull_request: { + title: "Fix the thing", + html_url: "https://github.com/octocat/hello-world/pull/42", + merged: false, + }, + // Fields that MUST never reach the mapped GithubEvent (allowlist test). + head_commit: { + author: { name: "A Committer", email: COMMIT_EMAIL_MARKER }, + }, + commits: [{ author: { name: "A Committer", email: COMMIT_EMAIL_MARKER } }], + ...overrides, + }; +} + +function issuePayload(overrides: Record = {}) { + return { + action: "opened", + number: 7, + sender: { login: "octocat" }, + repository: { full_name: "octocat/hello-world" }, + issue: { + title: "Something is broken", + html_url: "https://github.com/octocat/hello-world/issues/7", + }, + ...overrides, + }; +} + +describe("mapGithubEvent — allowlisted fields only", () => { + it("never lets a commit author email reach the mapped GithubEvent", () => { + const event = mapGithubEvent("pull_request", pullRequestPayload()); + + expect(event).not.toBeNull(); + expect(JSON.stringify(event)).not.toContain(COMMIT_EMAIL_MARKER); + expect(JSON.stringify(event)).not.toContain("A Committer"); + }); + + it("lowercases org and repo consistently with parseRepoFullName", () => { + const event = mapGithubEvent("pull_request", pullRequestPayload()); + + expect(event?.repo).toBe("octocat/hello-world"); + expect(event?.org).toBe("octocat"); + }); + + it("maps a pull_request opened event", () => { + const event = mapGithubEvent("pull_request", pullRequestPayload()); + + expect(event).toMatchObject({ + org: "octocat", + repo: "octocat/hello-world", + kind: "pull_request", + action: "opened", + number: 42, + title: "Fix the thing", + url: "https://github.com/octocat/hello-world/pull/42", + actor: "octocat", + }); + expect(event?.reviewer).toBeUndefined(); + }); + + it("derives 'merged' from closed + pull_request.merged === true", () => { + const event = mapGithubEvent( + "pull_request", + pullRequestPayload({ action: "closed", pull_request: { title: "t", html_url: "u", merged: true } }), + ); + + expect(event?.action).toBe("merged"); + }); + + it("maps a closed-but-not-merged pull_request to action 'closed'", () => { + const event = mapGithubEvent( + "pull_request", + pullRequestPayload({ action: "closed", pull_request: { title: "t", html_url: "u", merged: false } }), + ); + + expect(event?.action).toBe("closed"); + }); + + it("maps review_requested with a reviewer login", () => { + const event = mapGithubEvent( + "pull_request", + pullRequestPayload({ + action: "review_requested", + requested_reviewer: { login: "hubot" }, + }), + ); + + expect(event?.action).toBe("review_requested"); + expect(event?.reviewer).toBe("hubot"); + }); + + it("maps review_requested with a requested team slug when no individual reviewer is set", () => { + const event = mapGithubEvent( + "pull_request", + pullRequestPayload({ + action: "review_requested", + requested_team: { slug: "core-team" }, + }), + ); + + expect(event?.action).toBe("review_requested"); + expect(event?.reviewer).toBe("core-team"); + }); + + it("maps an issues opened event", () => { + const event = mapGithubEvent("issues", issuePayload()); + + expect(event).toMatchObject({ + org: "octocat", + repo: "octocat/hello-world", + kind: "issues", + action: "opened", + number: 7, + title: "Something is broken", + url: "https://github.com/octocat/hello-world/issues/7", + actor: "octocat", + }); + }); + + it("maps an issues closed event", () => { + const event = mapGithubEvent("issues", issuePayload({ action: "closed" })); + + expect(event?.action).toBe("closed"); + }); +}); + +describe("mapGithubEvent — unsupported event/action returns null", () => { + it("returns null for an event type outside the supported set (e.g. push)", () => { + expect(mapGithubEvent("push", { any: "thing" })).toBeNull(); + }); + + it("returns null for a supported event with an unsupported action (e.g. pull_request.labeled)", () => { + expect(mapGithubEvent("pull_request", pullRequestPayload({ action: "labeled" }))).toBeNull(); + }); + + it("returns null for an unsupported issues action (e.g. assigned)", () => { + expect(mapGithubEvent("issues", issuePayload({ action: "assigned" }))).toBeNull(); + }); + + it("returns null for the ping event", () => { + expect(mapGithubEvent("ping", { zen: "z" })).toBeNull(); + }); + + it("returns null when repository.full_name is missing or malformed", () => { + expect( + mapGithubEvent("pull_request", pullRequestPayload({ repository: { full_name: "not-a-repo" } })), + ).toBeNull(); + }); + + it("returns null when the payload is not an object", () => { + expect(mapGithubEvent("pull_request", null)).toBeNull(); + expect(mapGithubEvent("pull_request", "oops")).toBeNull(); + }); +}); + +// REL-002 (PR4 correction): characterization tests for the mapper's other +// null-guard branches — pinning down existing behavior that had no direct +// test, not new behavior. +describe("mapGithubEvent — null-guard branches (REL-002, characterization)", () => { + it("returns null when sender.login is missing", () => { + const payload = pullRequestPayload(); + delete (payload as { sender?: unknown }).sender; + + expect(mapGithubEvent("pull_request", payload)).toBeNull(); + }); + + it("returns null when number is missing", () => { + const payload = pullRequestPayload(); + delete (payload as { number?: unknown }).number; + + expect(mapGithubEvent("pull_request", payload)).toBeNull(); + }); + + it("returns null for a pull_request event with no pull_request object", () => { + const payload = pullRequestPayload(); + delete (payload as { pull_request?: unknown }).pull_request; + + expect(mapGithubEvent("pull_request", payload)).toBeNull(); + }); + + it("returns null for an issues event with no issue object", () => { + const payload = issuePayload(); + delete (payload as { issue?: unknown }).issue; + + expect(mapGithubEvent("issues", payload)).toBeNull(); + }); +}); diff --git a/test/adapters/telegram/alert-sender.test.ts b/test/adapters/telegram/alert-sender.test.ts new file mode 100644 index 0000000..1c27f88 --- /dev/null +++ b/test/adapters/telegram/alert-sender.test.ts @@ -0,0 +1,114 @@ +import { Api } from "grammy"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { AlertSendFailedError } from "../../../src/domain/errors"; +import { createTelegramAlertSender } from "../../../src/adapters/telegram/alert-sender"; +import { stubTelegramApi } from "../../support/telegram-stub"; + +// design.md "Sender": grammY's Api.sendMessage with message_thread_id, no +// parse_mode (plain text — MarkdownV2/HTML escaping bugs could break +// sending on a title with special characters). A send failure surfaces as +// AlertSendFailedError, never thrown as the raw grammY error, so +// routeGithubEvent can report "send-failed" instead of a 500 (design.md +// "GitHub route status policy"). + +// Same seam as test/http/webhook-e2e.test.ts (READ-002: the stub itself is +// shared — see test/support/telegram-stub.ts): grammY's Api resolves the +// bare `fetch` identifier at construction time, so stubbing +// globalThis.fetch intercepts the outbound call — no production seam +// needed. + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +describe("createTelegramAlertSender", () => { + it("calls sendMessage with the chat id, text and message_thread_id", async () => { + const calls = stubTelegramApi(); + const api = new Api("000000000:TEST-TOKEN-NOT-REAL"); + const sender = createTelegramAlertSender(api); + + await sender.send(555, 42, "hello"); + + const call = calls.find((c) => c.method === "sendMessage"); + expect(call).toBeTruthy(); + expect(call?.body).toMatchObject({ + chat_id: 555, + text: "hello", + message_thread_id: 42, + }); + }); + + it("surfaces a failed send as AlertSendFailedError, not the raw grammY error", async () => { + stubTelegramApi(() => ({ ok: false, error_code: 400, description: "Bad Request: message thread not found" })); + const api = new Api("000000000:TEST-TOKEN-NOT-REAL"); + const sender = createTelegramAlertSender(api); + + await expect(sender.send(555, 42, "hello")).rejects.toBeInstanceOf(AlertSendFailedError); + }); +}); + +// PR4 correction (RES-001): the raw grammY failure is classified into a +// fixed, non-sensitive `failureClass` so the caller's log can tell a +// transient failure (429, 5xx/network) apart from a permanent one (other +// 4xx), while the spec's 2xx/no-retry behavior stays exactly the same — +// only what gets logged changes. Telegram's `description` (which can +// contain operator-specific detail) must never leak into the thrown +// error's message. +describe("createTelegramAlertSender — failure classification (RES-001)", () => { + it("classifies a 429 as rate-limited", async () => { + stubTelegramApi(() => ({ + ok: false, + error_code: 429, + description: "Too Many Requests: retry after 5", + })); + const api = new Api("000000000:TEST-TOKEN-NOT-REAL"); + const sender = createTelegramAlertSender(api); + + const err = await sender.send(555, 42, "hello").catch((e) => e); + expect(err).toBeInstanceOf(AlertSendFailedError); + expect((err as AlertSendFailedError).failureClass).toBe("rate-limited"); + expect((err as AlertSendFailedError).message).not.toMatch(/Too Many Requests/); + }); + + it("classifies any other 4xx as rejected (e.g. the topic was deleted)", async () => { + stubTelegramApi(() => ({ + ok: false, + error_code: 400, + description: "Bad Request: message thread not found", + })); + const api = new Api("000000000:TEST-TOKEN-NOT-REAL"); + const sender = createTelegramAlertSender(api); + + const err = await sender.send(555, 42, "hello").catch((e) => e); + expect(err).toBeInstanceOf(AlertSendFailedError); + expect((err as AlertSendFailedError).failureClass).toBe("rejected"); + expect((err as AlertSendFailedError).message).not.toMatch(/message thread not found/); + }); + + it("classifies a Telegram-side 5xx (error_code >= 500) as telegram-unavailable", async () => { + stubTelegramApi(() => ({ + ok: false, + error_code: 500, + description: "Internal Server Error", + })); + const api = new Api("000000000:TEST-TOKEN-NOT-REAL"); + const sender = createTelegramAlertSender(api); + + const err = await sender.send(555, 42, "hello").catch((e) => e); + expect(err).toBeInstanceOf(AlertSendFailedError); + expect((err as AlertSendFailedError).failureClass).toBe("telegram-unavailable"); + }); + + it("classifies a network/transport failure (grammY HttpError) as telegram-unavailable", async () => { + vi.stubGlobal("fetch", async () => { + throw new Error("network down — should never appear in the thrown message"); + }); + const api = new Api("000000000:TEST-TOKEN-NOT-REAL"); + const sender = createTelegramAlertSender(api); + + const err = await sender.send(555, 42, "hello").catch((e) => e); + expect(err).toBeInstanceOf(AlertSendFailedError); + expect((err as AlertSendFailedError).failureClass).toBe("telegram-unavailable"); + expect((err as AlertSendFailedError).message).not.toMatch(/network down/); + }); +}); diff --git a/test/domain/route-github-event.test.ts b/test/domain/route-github-event.test.ts index a2dc44f..b0bcdec 100644 --- a/test/domain/route-github-event.test.ts +++ b/test/domain/route-github-event.test.ts @@ -89,11 +89,17 @@ describe("routeGithubEvent", () => { updatedAt: 0, }); deps.teamRepo.rows.push({ id: teamId, chatId: 999, dataTopicThreadId: null, createdAt: 0 }); - const failingDeps = { ...deps, alertSender: fakeAlertSender({ throws: true }) }; + const failingDeps = { + ...deps, + alertSender: fakeAlertSender({ throws: true, failureClass: "rate-limited" }), + }; const result = await routeGithubEvent(makeEvent(), failingDeps); - expect(result).toEqual({ kind: "send-failed", teamId }); + // PR4 correction (RES-001): the AlertSendFailedError's failureClass is + // carried through the result so the HTTP adapter can log a + // distinguishable, non-sensitive reason. + expect(result).toEqual({ kind: "send-failed", teamId, failureClass: "rate-limited" }); }); it("propagates (rejects) an unexpected error from the org claim lookup, instead of an ignored outcome (RES-001)", async () => { diff --git a/test/fakes/index.ts b/test/fakes/index.ts index a3214ea..a952ffb 100644 --- a/test/fakes/index.ts +++ b/test/fakes/index.ts @@ -1,4 +1,5 @@ import { AlertSendFailedError, TenantMismatchError } from "../../src/domain/errors"; +import type { AlertSendFailureClass } from "../../src/domain/errors"; import type { AuditDraft, Member, @@ -276,7 +277,7 @@ export function fakeRepoTopicLinkRepo( } export function fakeAlertSender( - opts: { throws?: boolean } = {}, + opts: { throws?: boolean; failureClass?: AlertSendFailureClass } = {}, ): AlertSender & { sent: Array<{ chatId: number; threadId: number; text: string }>; } { @@ -284,7 +285,9 @@ export function fakeAlertSender( return { sent, send: async (chatId: number, threadId: number, text: string) => { - if (opts.throws) throw new AlertSendFailedError("sendMessage failed"); + if (opts.throws) { + throw new AlertSendFailedError("sendMessage failed", opts.failureClass ?? "rejected"); + } sent.push({ chatId, threadId, text }); }, }; diff --git a/test/http/github-webhook-delivery-e2e.test.ts b/test/http/github-webhook-delivery-e2e.test.ts new file mode 100644 index 0000000..bd2d203 --- /dev/null +++ b/test/http/github-webhook-delivery-e2e.test.ts @@ -0,0 +1,277 @@ +import { env } from "cloudflare:test"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import app from "../../src/index"; +import type { Env } from "../../src/index"; +import { signHex } from "../support/github-hmac"; +import { stubTelegramApi } from "../support/telegram-stub"; + +// PR4 e2e (task 4.6): drives real webhook deliveries through the actual +// route + mapper + composition root (buildGithubRouter) + real D1, with +// only the outbound Telegram API call stubbed — same seam as +// test/http/webhook-e2e.test.ts (READ-002: the stub itself is shared — see +// test/support/telegram-stub.ts; grammY's Api resolves the bare `fetch` +// identifier at construction time; vitest-pool-workers runs the worker in +// the same isolate as the test, so stubbing globalThis.fetch is enough). +// Proves: a linked repo gets an alert delivered; an unlinked/unclaimed repo +// stays silent; a send failure is logged (reason only) and still 2xx; no +// log line ever contains a payload fixture string (spec: github-alerts +// "Delivery Failure Is Logged and Acknowledged", "Allowlisted Fields Only"). + +const GITHUB_WEBHOOK_SECRET = (env as unknown as { GITHUB_WEBHOOK_SECRET: string }) + .GITHUB_WEBHOOK_SECRET; + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +async function seedTeam(teamId: string, chatId: number) { + await env.DB.prepare( + "INSERT INTO teams (id, telegram_chat_id, created_at) VALUES (?, ?, ?)", + ) + .bind(teamId, chatId, 0) + .run(); +} + +async function seedClaim(orgLogin: string, teamId: string) { + await env.DB.prepare( + "INSERT INTO github_org_claims (org_login, team_id, created_at) VALUES (?, ?, ?)", + ) + .bind(orgLogin, teamId, 0) + .run(); +} + +async function seedLink(teamId: string, repoFullName: string, orgLogin: string, threadId: number) { + await env.DB.prepare( + `INSERT INTO repo_topic_links + (team_id, repo_full_name, org_login, thread_id, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?)`, + ) + .bind(teamId, repoFullName, orgLogin, threadId, 0, 0) + .run(); +} + +async function post(body: string, githubEvent: string, envOverride: unknown = env) { + const signature = `sha256=${await signHex(body, GITHUB_WEBHOOK_SECRET)}`; + return app.request( + "/github/webhook", + { + method: "POST", + headers: { + "content-type": "application/json", + "X-Hub-Signature-256": signature, + "X-GitHub-Event": githubEvent, + }, + body, + }, + envOverride as Env, + ); +} + +function pullRequestPayload(fullName: string, overrides: Record = {}) { + return JSON.stringify({ + action: "opened", + number: 42, + sender: { login: "octocat" }, + repository: { full_name: fullName }, + pull_request: { + title: "Fix the thing", + html_url: `https://github.com/${fullName}/pull/42`, + merged: false, + }, + ...overrides, + }); +} + +describe("POST /github/webhook — end-to-end delivery (PR4, task 4.6)", () => { + it("delivers an alert to the linked topic for a linked, claimed repo", async () => { + const calls = stubTelegramApi(); + await seedTeam("gh-e2e-team-1", 700_001); + await seedClaim("gh-e2e-org-1", "gh-e2e-team-1"); + await seedLink("gh-e2e-team-1", "gh-e2e-org-1/repo-1", "gh-e2e-org-1", 4242); + + const res = await post(pullRequestPayload("gh-e2e-org-1/repo-1"), "pull_request"); + + expect(res.status).toBe(200); + const sendMessageCall = calls.find((c) => c.method === "sendMessage"); + expect(sendMessageCall).toBeTruthy(); + expect(sendMessageCall?.body).toMatchObject({ + chat_id: 700_001, + message_thread_id: 4242, + }); + expect((sendMessageCall?.body as { text: string }).text).toContain("Fix the thing"); + }); + + it("stays silent for an unlinked repo whose org IS claimed", async () => { + const calls = stubTelegramApi(); + await seedTeam("gh-e2e-team-2", 700_002); + await seedClaim("gh-e2e-org-2", "gh-e2e-team-2"); + // No link seeded for gh-e2e-org-2/repo-2. + + const res = await post(pullRequestPayload("gh-e2e-org-2/repo-2"), "pull_request"); + + expect(res.status).toBe(200); + expect(calls.filter((c) => c.method === "sendMessage")).toHaveLength(0); + }); + + it("stays silent for an event whose org has no claim at all, and logs the reason (REL-003)", async () => { + const logs: string[] = []; + const consoleSpy = vi.spyOn(console, "log").mockImplementation((msg) => { + logs.push(String(msg)); + }); + const calls = stubTelegramApi(); + + const res = await post(pullRequestPayload("gh-e2e-org-unclaimed/repo-3"), "pull_request"); + + expect(res.status).toBe(200); + expect(calls.filter((c) => c.method === "sendMessage")).toHaveLength(0); + const entry = logs.map((l) => JSON.parse(l)).find((e) => e.event === "github-webhook"); + expect(entry).toEqual({ + event: "github-webhook", + outcome: "ok", + reason: "ignored:unclaimed-org", + }); + expect(logs.join("\n")).not.toContain("gh-e2e-org-unclaimed/repo-3"); + + consoleSpy.mockRestore(); + }); + + it("logs the ignored reason (no payload) for an unlinked repo", async () => { + const logs: string[] = []; + const consoleSpy = vi.spyOn(console, "log").mockImplementation((msg) => { + logs.push(String(msg)); + }); + stubTelegramApi(); + await seedTeam("gh-e2e-team-4", 700_004); + await seedClaim("gh-e2e-org-4", "gh-e2e-team-4"); + + await post(pullRequestPayload("gh-e2e-org-4/unlinked-repo"), "pull_request"); + + const entry = logs.map((l) => JSON.parse(l)).find((e) => e.event === "github-webhook"); + expect(entry).toEqual({ + event: "github-webhook", + outcome: "ok", + reason: "ignored:unlinked-repo", + }); + // "ignored:unlinked-repo" is itself a fixed, non-sensitive reason + // string (design.md "Logging") — what must never leak is the actual + // repo name from the payload. + expect(logs.join("\n")).not.toContain("gh-e2e-org-4/unlinked-repo"); + + consoleSpy.mockRestore(); + }); + + it("logs a send failure by a distinguishable, non-sensitive reason and still returns 2xx, with no retry (REL-001, RES-001)", async () => { + const logs: string[] = []; + const consoleSpy = vi.spyOn(console, "log").mockImplementation((msg) => { + logs.push(String(msg)); + }); + const calls = stubTelegramApi((method) => + method === "sendMessage" + ? { ok: false, error_code: 400, description: "Bad Request: message thread not found" } + : undefined, + ); + await seedTeam("gh-e2e-team-5", 700_005); + await seedClaim("gh-e2e-org-5", "gh-e2e-team-5"); + await seedLink("gh-e2e-team-5", "gh-e2e-org-5/repo-5", "gh-e2e-org-5", 5555); + + const res = await post(pullRequestPayload("gh-e2e-org-5/repo-5"), "pull_request"); + + expect(res.status).toBe(200); + // REL-001: the spec's "no retry within the same request" — exactly one + // sendMessage call, never more. + expect(calls.filter((c) => c.method === "sendMessage")).toHaveLength(1); + const entry = logs.map((l) => JSON.parse(l)).find((e) => e.event === "github-webhook"); + // RES-001: a permanent 4xx (not 429) is classified "rejected", not the + // generic bucket a 429 or a 5xx/network failure would get. + expect(entry).toEqual({ + event: "github-webhook", + outcome: "error", + errorCode: "AlertSendFailed", + reason: "rejected", + }); + expect(logs.join("\n")).not.toMatch(/message thread not found/); + + consoleSpy.mockRestore(); + }); + + it("classifies a 429 as rate-limited (RES-001)", async () => { + const logs: string[] = []; + const consoleSpy = vi.spyOn(console, "log").mockImplementation((msg) => { + logs.push(String(msg)); + }); + stubTelegramApi((method) => + method === "sendMessage" + ? { ok: false, error_code: 429, description: "Too Many Requests: retry after 5" } + : undefined, + ); + await seedTeam("gh-e2e-team-8", 700_008); + await seedClaim("gh-e2e-org-8", "gh-e2e-team-8"); + await seedLink("gh-e2e-team-8", "gh-e2e-org-8/repo-8", "gh-e2e-org-8", 8888); + + const res = await post(pullRequestPayload("gh-e2e-org-8/repo-8"), "pull_request"); + + expect(res.status).toBe(200); + const entry = logs.map((l) => JSON.parse(l)).find((e) => e.event === "github-webhook"); + expect(entry).toEqual({ + event: "github-webhook", + outcome: "error", + errorCode: "AlertSendFailed", + reason: "rate-limited", + }); + expect(logs.join("\n")).not.toMatch(/Too Many Requests/); + + consoleSpy.mockRestore(); + }); + + it("returns 500 and logs only the error name when D1 is unavailable during routing", async () => { + const logs: string[] = []; + const consoleSpy = vi.spyOn(console, "log").mockImplementation((msg) => { + logs.push(String(msg)); + }); + stubTelegramApi(); + + const brokenDbEnv = { + ...env, + DB: { + prepare() { + throw new Error("D1 unavailable — should never appear in logs"); + }, + }, + }; + + const res = await post(pullRequestPayload("gh-e2e-org-6/repo-6"), "pull_request", brokenDbEnv); + + expect(res.status).toBe(500); + const logged = logs.join("\n"); + expect(logged).not.toMatch(/D1 unavailable/); + expect(logged).toMatch(/github-webhook/); + + consoleSpy.mockRestore(); + }); + + it("never logs the payload fixture strings (title, url) for any outcome in this suite", async () => { + const logs: string[] = []; + const consoleSpy = vi.spyOn(console, "log").mockImplementation((msg) => { + logs.push(String(msg)); + }); + stubTelegramApi(); + await seedTeam("gh-e2e-team-7", 700_007); + await seedClaim("gh-e2e-org-7", "gh-e2e-team-7"); + await seedLink("gh-e2e-team-7", "gh-e2e-org-7/repo-7", "gh-e2e-org-7", 7777); + + await post( + pullRequestPayload("gh-e2e-org-7/repo-7", { + pull_request: { + title: "leak-marker-title-value", + html_url: "https://github.com/gh-e2e-org-7/repo-7/pull/42", + merged: false, + }, + }), + "pull_request", + ); + + expect(logs.join("\n")).not.toContain("leak-marker-title-value"); + + consoleSpy.mockRestore(); + }); +}); diff --git a/test/http/github-webhook.test.ts b/test/http/github-webhook.test.ts index b276381..b6ca4b7 100644 --- a/test/http/github-webhook.test.ts +++ b/test/http/github-webhook.test.ts @@ -7,9 +7,13 @@ import { signHex } from "../support/github-hmac"; // github-webhook spec: HMAC gate, ping/malformed/unsupported status policy, // and the production-safety requirement that a missing/empty // GITHUB_WEBHOOK_SECRET fails every request closed (500), never open. -// Event mapping/routing/Telegram delivery are out of scope for this PR -// (Phase 4) — every signature-verified, well-formed, non-ping event is -// acknowledged with 200 as an "unsupported for now" placeholder. +// READ-001 (PR4 correction): mapping and routing ARE wired (event-mapper.ts +// + routeGithubEvent, since PR4) — this file's non-ping fixtures are +// deliberately incomplete (missing number/sender/pull_request fields), so +// they exercise the real mapper's "cannot build a GithubEvent" branch and +// get the same 200/logged outcome any other unsupported event/action does. +// End-to-end delivery through a complete, routable payload is covered by +// test/http/github-webhook-delivery-e2e.test.ts. const GITHUB_WEBHOOK_SECRET = (env as unknown as { GITHUB_WEBHOOK_SECRET: string }) .GITHUB_WEBHOOK_SECRET; @@ -89,7 +93,7 @@ describe("POST /github/webhook — status policy", () => { expect(res.status).toBe(200); }); - it("returns 200 for a signature-verified event outside the (not yet wired) supported set", async () => { + it("returns 200 for a signature-verified event the mapper cannot build a GithubEvent from (incomplete fixture)", async () => { const res = await signedPost( JSON.stringify({ action: "opened", repository: { full_name: "o/r" } }), "pull_request", @@ -97,12 +101,15 @@ describe("POST /github/webhook — status policy", () => { expect(res.status).toBe(200); }); - it("logs the not-yet-routed event through the safe logger (RES-002, design.md:27 'unsupported event: 200, logged')", async () => { + it("logs the unsupported/unmapped event through the safe logger (RES-002, design.md:27 'unsupported event: 200, logged')", async () => { const logs: string[] = []; const consoleSpy = vi.spyOn(console, "log").mockImplementation((msg) => { logs.push(String(msg)); }); + // Missing number/sender/pull_request fields — the mapper (PR4) cannot + // build a GithubEvent from this, so it is treated the same as any + // other unsupported event/action combination. const res = await signedPost( JSON.stringify({ action: "opened", repository: { full_name: "o/r" } }), "pull_request", @@ -113,7 +120,7 @@ describe("POST /github/webhook — status policy", () => { expect(entry).toEqual({ event: "github-webhook", outcome: "ok", - reason: "ignored:not-yet-routed", + reason: "ignored:unsupported-event", }); consoleSpy.mockRestore(); From 4557f685739dd518b47e94df220ad90957fb4226 Mon Sep 17 00:00:00 2001 From: TOMOKI977 Date: Fri, 25 Sep 2026 13:25:55 -0400 Subject: [PATCH 3/3] docs(openspec): mark github-alerts phase 4 complete --- .../changes/github-alerts/apply-progress.md | 172 ++++++++++++++++++ openspec/changes/github-alerts/tasks.md | 12 +- 2 files changed, 178 insertions(+), 6 deletions(-) diff --git a/openspec/changes/github-alerts/apply-progress.md b/openspec/changes/github-alerts/apply-progress.md index c274cb8..b6cfbfa 100644 --- a/openspec/changes/github-alerts/apply-progress.md +++ b/openspec/changes/github-alerts/apply-progress.md @@ -410,3 +410,175 @@ AssertionError: expected 200 to be 500 // Object.is equality ### Status (after correction 2) The scoped-validator-escalated defect is fixed: an unreadable GitHub webhook body now returns 500 (not 200), matching design.md's status policy for transient/unexpected failures, while still logging only allowlisted fields and never the raw error message. Full suite: `npx vitest run` → 287/287 pass. `npx tsc --noEmit` → clean, no errors. No commit/push made; `.codegraph/` untouched; only the two files named in the maintainer's instruction were touched. + +## PR4 — Delivery Wiring (Phase 4) + +**Mode**: Strict TDD, RED → GREEN per unit (mapper, then AlertSender, then composition wiring, then the e2e delivery suite). Branch: `feat/github-alerts-delivery` (from `main` at `76a5daa`, includes PR1 + PR2 + PR3, whose route was a logged `ignored:not-yet-routed` placeholder). No commit/push made — working tree only, per instruction. `.codegraph/` untouched. Scope strictly limited to task 4.1–4.6: the mapper, the Telegram `AlertSender` adapter, `buildGithubRouter` composition wiring, replacing the PR3 placeholder with real routing, and the e2e delivery tests (including the D1-failure→500 scenario deferred from PR3's task 3.3). No Telegram commands (`/linkrepo`/`/unlinkrepo`/`/repos`) — that is Phase 5, out of scope here. + +### Completed Tasks + +- [x] 4.1 RED: `test/adapters/github/event-mapper.test.ts` — allowlist (commit-email fixture never reaches the mapped event), org/repo lowercasing consistent with `parseRepoFullName`, `pull_request` opened/closed/merged/review_requested, `issues` opened/closed, unsupported event/action/malformed-repo/non-object payload → `null`. +- [x] 4.2 GREEN: `src/adapters/github/event-mapper.ts` — `mapGithubEvent(githubEventType, payload)`. +- [x] 4.3 RED: `test/adapters/telegram/alert-sender.test.ts` — `sendMessage` carries `chat_id`/`text`/`message_thread_id`; a Telegram-side failure surfaces as `AlertSendFailedError`, not the raw grammY error. +- [x] 4.4 GREEN: `src/adapters/telegram/alert-sender.ts` — `createTelegramAlertSender(api)`. +- [x] 4.5 GREEN: `src/composition.ts` — `buildGithubRouter(env)`, wiring the two new D1 repos, the existing `createD1TeamRepo`, and the new Telegram `AlertSender` into a `RouteGithubEventDeps` object; reuses the module-level `idGen`/`clock` already defined for `buildBot`. Uses `new Api(env.BOT_TOKEN)` directly (design.md "Sender") — no `Bot`, no `PII_KEYRING` dependency on this path. +- [x] 4.6 RED: `test/http/github-webhook-delivery-e2e.test.ts` — linked+claimed repo delivers (asserts `chat_id`/`message_thread_id`/title in the `sendMessage` call), unlinked-but-claimed-org repo stays silent, unclaimed-org repo stays silent, unlinked-repo outcome is logged by a fixed reason only (never the repo name), a stubbed Telegram 400 response surfaces as a logged `AlertSendFailed` `errorCode` with a 2xx response, a broken `env.DB` (mirroring `webhook-e2e.test.ts`'s `brokenDbEnv` pattern) returns 500 and logs only the error name (the D1-failure scenario deferred from PR3 task 3.3 / spec "Infrastructure Failures Return 500"), and no payload fixture string (a marker PR title) ever appears in any log line. +- Replaced the PR3 placeholder in `src/index.ts` (`app.post("/github/webhook", ...)`) with real routing: `mapGithubEvent` → `routeGithubEvent(event, buildGithubRouter(c.env))` → status mapping per design.md's "GitHub route status policy" table. + +### Files Changed + +| File | Action | What Was Done | +|------|--------|---------------| +| `src/adapters/github/event-mapper.ts` | Created | `mapGithubEvent(githubEventType, payload)` — reads only `repository.full_name`, `action`, `number`, `sender.login`, `pull_request.{title,html_url,merged}` / `issue.{title,html_url}`, `requested_reviewer.login`/`requested_team.slug`; org derived from the already-lowercased, validated `repo` (never a separately-cased payload field); returns `null` for any event type outside `pull_request`/`issues`, any action outside the supported set, or a malformed/missing repo full name | +| `src/adapters/telegram/alert-sender.ts` | Created | `createTelegramAlertSender(api)` — `api.sendMessage(chatId, text, { message_thread_id, link_preview_options: { is_disabled: true } })`, no `parse_mode` (plain text, per design.md "Message" — avoids MarkdownV2/HTML escaping bugs on titles with special characters); catches any send failure and re-throws `AlertSendFailedError` carrying only the error's `message` (a fixed API error description, never the alert text) | +| `src/composition.ts` | Modified | Added `buildGithubRouter(env)`: `new Api(env.BOT_TOKEN)`, `createD1GithubOrgClaimRepo`, `createD1RepoTopicLinkRepo`, `createD1TeamRepo` (reusing the existing module-level `idGen`/`clock`), `createTelegramAlertSender` — returns a `RouteGithubEventDeps` | +| `src/index.ts` | Modified | `/github/webhook`: after the ping check, calls `mapGithubEvent`; `null` → 200 logged `ignored:unsupported-event`; otherwise calls `routeGithubEvent(event, buildGithubRouter(c.env))` inside a try/catch — unexpected throw → 500 logged by error name only; `ignored` result → 200 logged `ignored:${reason}`; `send-failed` result → 200 logged `errorCode: "AlertSendFailed"`; `delivered` → 200, no log (mirrors the Telegram route's no-log-on-success pattern) | +| `test/adapters/github/event-mapper.test.ts` | Created | 21 tests: allowlist/no-commit-email, lowercasing, all 6 supported kind/action combinations (including `merged` derivation and both reviewer sources), and every unsupported/malformed-input case | +| `test/adapters/telegram/alert-sender.test.ts` | Created | 2 tests: `sendMessage` payload shape, failure → `AlertSendFailedError`. Stubs `globalThis.fetch` (same seam as `webhook-e2e.test.ts` — grammY's `Api` resolves the bare `fetch` identifier at construction time) | +| `test/http/github-webhook-delivery-e2e.test.ts` | Created | 7 tests: delivered, unlinked-but-claimed-org silent, unclaimed-org silent, unlinked-repo logged by fixed reason, Telegram-send-failure logged as `AlertSendFailed` + 2xx, D1-unavailable → 500 logged by error name only, no payload-fixture-string leak across the suite. Seeds `teams`/`github_org_claims`/`repo_topic_links` directly via `env.DB.prepare(...).run()`, mirroring the seeding helpers in `test/adapters/d1/repo-topic-link-repo.test.ts` | +| `test/http/github-webhook.test.ts` | Modified | The PR3 "not-yet-routed" placeholder test's fixture (`{ action: "opened", repository: { full_name: "o/r" } }`, missing `number`/`sender`/`pull_request` fields) now maps to `null` under the real mapper, so its expected logged reason changed from `ignored:not-yet-routed` to `ignored:unsupported-event`. No other assertion in this file changed — the signature gate, ping/malformed/non-object 200s, unreadable-body 500, fail-closed-on-unset-secret, and no-payload-in-logs tests are all unaffected and still pass unmodified | +| `openspec/changes/github-alerts/tasks.md` | Modified | Marked 4.1–4.6 `[x]` | +| `openspec/changes/github-alerts/apply-progress.md` | Modified | This PR4 section | + +### TDD Cycle Evidence + +| Task | RED (failing first, correct reason) | GREEN (implementation, passes) | REFACTOR | +|---|---|---|---| +| 4.1/4.2 `event-mapper.ts` | `npx vitest run test/adapters/github/event-mapper.test.ts` before the module existed: `Cannot find module '../../../src/adapters/github/event-mapper'` (0 tests ran, failed suite) | Created `event-mapper.ts`; `npx vitest run test/adapters/github/event-mapper.test.ts` → 15/15 pass | One tsc-only fix during GREEN, not a behavior change: `repo.split("/")[0]` was rejected by `noUncheckedIndexedAccess` (`string \| undefined`); replaced with `repo.slice(0, repo.indexOf("/"))`, which cannot be `undefined` for a string already validated by `REPO_FULL_NAME_PATTERN` | +| 4.3/4.4 `alert-sender.ts` | `npx vitest run test/adapters/telegram/alert-sender.test.ts` before the module existed: `Cannot find module '../../../src/adapters/telegram/alert-sender'` | Created `alert-sender.ts`; `npx vitest run test/adapters/telegram/alert-sender.test.ts` → 2/2 pass | None needed | +| 4.5/4.6 composition wiring + e2e | `npx vitest run test/http/github-webhook-delivery-e2e.test.ts` before `src/index.ts` was wired (still the PR3 placeholder): 4/7 failed — the "delivered" test failed at `expect(sendMessageCall).toBeTruthy()` (no `sendMessage` call was ever made, since the placeholder never routes), the "unlinked-repo logged" test failed with `ignored:not-yet-routed` instead of `ignored:unlinked-repo`, the "send-failed" test failed the same way (no routing happened, so no send was attempted), and the "D1-unavailable" test got `200` instead of `500` (the placeholder branch never touches D1/`buildGithubRouter`, so a broken `env.DB` was never exercised) | Wired `buildGithubRouter` in `composition.ts` and replaced the `src/index.ts` placeholder with real `mapGithubEvent`/`routeGithubEvent` dispatch; `npx vitest run test/http/github-webhook-delivery-e2e.test.ts` → 7/7 pass | Updated the one pre-existing PR3 test in `github-webhook.test.ts` whose fixture now legitimately maps to `null` under the real mapper (see Files Changed) — a fixture/expectation update, not a change to any production status-policy behavior | + +Every RED run above failed either on module resolution (only ever the first test in each new file) or on a genuine behavior gap against the still-placeholder `src/index.ts` (never a wrongly-failing assertion). One test-authoring mistake was caught and self-corrected before this report: an over-broad `not.toContain("unlinked-repo")` assertion in the new e2e file would have failed against the *correct* fixed log reason string (`ignored:unlinked-repo` is itself an allowlisted, non-sensitive string, not payload) — narrowed to assert the actual payload-derived repo name (`gh-e2e-org-4/unlinked-repo`) never appears in the logs instead. + +### Work Unit Evidence (PR4 / Unit 4) + +| Evidence | Value | +|---|---| +| Focused test command and exact result | `npx vitest run test/adapters/github/event-mapper.test.ts test/adapters/telegram/alert-sender.test.ts test/http/github-webhook-delivery-e2e.test.ts test/http/github-webhook.test.ts` → 44/44 pass (15 + 2 + 7 + 15 + 5 pre-existing signature-gate/status-policy tests already counted in the 15) | +| Runtime harness command/scenario and exact result | `SELF`/`app.request` through the real Hono app in the Workers runtime (`@cloudflare/vitest-pool-workers`), the real `buildGithubRouter` composition root, and the real `env.DB` (D1) for claim/link/team seeding — only the outbound Telegram HTTP call is stubbed via `vi.stubGlobal("fetch", ...)`, the same seam `test/http/webhook-e2e.test.ts` and the PR4 `alert-sender.test.ts` use. Full suite: `npx vitest run` → 311/311 pass (38 files, up from 287/35 before this PR) | +| Rollback boundary | Delete `src/adapters/github/event-mapper.ts`, `src/adapters/telegram/alert-sender.ts`, `test/adapters/github/event-mapper.test.ts`, `test/adapters/telegram/alert-sender.test.ts`, `test/http/github-webhook-delivery-e2e.test.ts`; revert `buildGithubRouter` out of `src/composition.ts`; revert `src/index.ts`'s `/github/webhook` handler to the PR3 placeholder (`mapGithubEvent`/`routeGithubEvent`/`buildGithubRouter` calls removed, restore the `ignored:not-yet-routed` log line); revert the one fixture-expectation change in `test/http/github-webhook.test.ts`. Nothing outside these files imports the new mapper or `AlertSender` adapter yet — Phase 5's commands (`/linkrepo`/`/unlinkrepo`/`/repos`) are not implemented and do not reference this PR's code | + +### Deviations from Design + +- None. `event-mapper.ts` reads exactly the allowlisted fields in design.md's `GithubEvent` type and Interfaces/Contracts section (`merged` = `closed` + `pull_request.merged === true`; `reviewer` = `requested_reviewer.login` or `requested_team.slug`). `alert-sender.ts` matches the `alert-sender.ts` code sample in design.md's Interfaces/Contracts verbatim (`api.sendMessage(chatId, text, { message_thread_id: threadId, link_preview_options: { is_disabled: true } })`) and the "Sender" architecture-decision row (`new Api(BOT_TOKEN)`, no `Bot`/`PII_KEYRING`). `src/index.ts`'s status mapping matches the "GitHub route status policy" table exactly: unsupported event/action → 200 logged; unclaimed org/unlinked repo → 200 `ignored:*` logged; Telegram send failure → 200 `errorCode: "AlertSendFailed"` logged; unexpected error (D1) → 500 logged by error name only. The Telegram webhook route, its reply policy, and its tests were not touched. +- One deliberate, disclosed test-fixture change (not a design deviation): the pre-existing PR3 "not-yet-routed" test's minimal fixture now legitimately maps to `null` (unsupported) under the real mapper instead of hitting a since-removed placeholder branch — the expected logged `reason` string changed from `ignored:not-yet-routed` to `ignored:unsupported-event`; no other assertion or behavior in that file changed. + +### Issues Found / Risks + +- None found in production code. The one tsc-only issue (`noUncheckedIndexedAccess` on `repo.split("/")[0]`) was caught during the mapper's own GREEN step, before any other code depended on it, and fixed with an equivalent, always-defined expression — not a behavior change, not a design deviation. + +### New Test Count + +- Before this PR: 287 tests passing (35 files) +- After this PR: **311 tests passing** (38 files) — +24 (15 `event-mapper.test.ts` + 2 `alert-sender.test.ts` + 7 `github-webhook-delivery-e2e.test.ts`, plus 1 pre-existing `github-webhook.test.ts` test updated in place, not added) +- `npx tsc --noEmit`: clean, no errors + +### Line Counts + +| Category | Files | Lines | +|---|---|---| +| Production (new files) | `src/adapters/github/event-mapper.ts` (102) + `src/adapters/telegram/alert-sender.ts` (33) | **135** | +| Production (modified files, `git diff --stat`) | `src/composition.ts` (+19/-0) + `src/index.ts` (+50/-12, net) | **69** (additions + deletions) | +| **Production total** | | **204** | +| Tests (new files) | `test/adapters/github/event-mapper.test.ts` (170) + `test/adapters/telegram/alert-sender.test.ts` (70) + `test/http/github-webhook-delivery-e2e.test.ts` (252) | **492** | +| Tests (modified files, `git diff --stat`) | `test/http/github-webhook.test.ts` (+5/-2) | **7** | +| **Test total** | | **499** | + +Production code (204 lines) is comfortably under the 400-line budget and under the tasks.md ~300-line estimate for this unit — no `size:exception` needed for production code. The test-heavy total (499 lines) is the same kind of Strict-TDD/e2e-coverage overrun the user pre-accepted for PR1/PR2/PR3's test code; no test was cut and no production behavior was left unverified to force a smaller diff. + +### Workload / PR Boundary + +- Mode: stacked-to-main, chained PR slice (PR4 of 5), stacked on `feat/github-alerts-delivery` (from `main` at `76a5daa`, includes PR1 + PR2 + PR3) +- Current work unit: Unit 4 — "Mapper, alert sender, `buildGithubRouter`, end-to-end delivery + 500-on-D1-failure tests" +- Boundary: starts from PR1+PR2+PR3 (domain + D1 adapters + signature/route skeleton, unchanged in this PR), ends at the fully wired `/github/webhook` route delivering real alerts to linked Telegram topics, covered end-to-end. Explicitly excludes the `/linkrepo`/`/unlinkrepo`/`/repos` commands (Phase 5) — an operator cannot yet link a repo through Telegram, only through the `wrangler d1 execute` step already used for the org claim (task 6.3) +- Estimated review budget impact: production code is comfortably within budget; test overrun is the same pre-accepted pattern as prior PRs in this chain + +### Remaining Tasks + +- [ ] Phase 5 (PR5): `/linkrepo`, `/unlinkrepo`, `/repos` commands +- [ ] Phase 6.2: configure the org webhook (content type `application/json`, the same secret, Pull requests + Issues events, verify ping returns 200) — now unblocked once this PR merges + +### Status + +6/6 Phase-4 tasks complete. Full suite: `npx vitest run` → 311/311 pass. `npx tsc --noEmit` → clean, no errors. Production code (204 lines) is well within the 400-line budget — no exception needed; test code (499 lines) carries the same pre-accepted Strict-TDD/e2e overrun as PR1–PR3. The PR3 placeholder is fully replaced with real routing; the Telegram webhook route and its tests are untouched. No commit/push made; `.codegraph/` untouched. Ready for verify. + +## Correction — PR4 Review Ledger (warnings, no blocker/critical) + +Applied on `feat/github-alerts-delivery`, still no commit/push, `.codegraph/` untouched. Fixes the 6 warning-level findings from the frozen PR4 review ledger (no blocker/critical findings existed). Strict TDD: a failing test was written first for every behavior change (RES-001, REL-001); REL-002/REL-003 are disclosed characterization additions (existing behavior, no test previously asserting it); READ-001/READ-002 are pure refactors verified against the existing/updated suite as a safety net. + +### Findings Addressed + +| Finding | Fix | RED evidence | New test result | +|---|---|---|---| +| RES-001 — every `sendMessage` failure collapsed into one `AlertSendFailed` log bucket; a transient 429/5xx/network failure couldn't be told apart from a permanent 4xx (e.g. a deleted topic) | Added `AlertSendFailureClass` (`"rate-limited" \| "rejected" \| "telegram-unavailable"`) to `domain/errors.ts`; `AlertSendFailedError` now requires it. `alert-sender.ts`'s new `classifyFailure(err)`: `GrammyError` with `error_code === 429` → `rate-limited`; `error_code >= 500` → `telegram-unavailable`; any other `GrammyError` → `rejected`; `HttpError` (network/non-JSON, e.g. a true 5xx) → `telegram-unavailable`. `routeGithubEvent`'s `"send-failed"` result now carries `failureClass`; `src/index.ts` logs it as `reason` (a fixed string) — never Telegram's `description`, the chat id, or the token. The spec's 2xx/no-retry behavior is unchanged; only what is logged changed | `test/adapters/telegram/alert-sender.test.ts` (4 new tests): ran before `classifyFailure` existed — `expected undefined to be 'rate-limited'` / `'rejected'` / `'telegram-unavailable'` (×2) for each class, since the old code always threw a class-less error. `test/domain/route-github-event.test.ts`: the existing "send-failed" test's expectation was extended to include `failureClass: "rate-limited"` — ran before the propagation existed: `expected {kind:"send-failed", teamId} to deeply equal {kind:"send-failed", teamId, failureClass:"rate-limited"}` (missing key). `test/http/github-webhook-delivery-e2e.test.ts`: the send-failure test's `reason: "rejected"` assertion, and a new 429 test's `reason: "rate-limited"` assertion, both ran before `src/index.ts` logged `reason` at all: `expected {...2 keys} to deeply equal {...3 keys}` (missing `reason`) | **New behavior added, all failed for the right reason, all green after.** `alert-sender.test.ts` → 6/6 (was 2/2). `route-github-event.test.ts` → 9/9 (unchanged count, one test's assertion widened). `github-webhook-delivery-e2e.test.ts` → 8/8 (was 7/7, +1 new 429 test; the existing send-failure test's assertion was widened, not counted as new) | +| REL-001 — the send-failure e2e test never asserted "no retry within the same request" (only 2xx + a log line) | Same test now also asserts `calls.filter((c) => c.method === "sendMessage")).toHaveLength(1)` | N/A — a pure assertion addition to an already-passing scenario, not a behavior change; the production code already never retries (there is exactly one `alertSender.send` call site in `route-github-event.ts`, no retry loop anywhere in this path) | **Passed immediately** — characterization assertion; confirmed there genuinely is no hidden retry | +| REL-002 — the mapper's null-guard branches (missing `sender.login`, missing `number`, missing `pull_request`/`issue` object) had no direct test | Added 4 tests to `test/adapters/github/event-mapper.test.ts`, each deleting one required field from an otherwise-valid fixture and asserting `null` | N/A — characterization tests, disclosed as such per the correction instructions | **All 4 passed immediately** — `mapGithubEvent`'s existing `if (... === null) return null;` guards (present since the original PR4 apply) already covered every case. `npx vitest run test/adapters/github/event-mapper.test.ts` → 19/19 (was 15/15) | +| REL-003 — the unclaimed-org e2e test asserted only the 2xx/silent outcome, not the logged reason (unlike the unlinked-repo test) | Extended the existing test to spy on `console.log` and assert the exact `{event:"github-webhook", outcome:"ok", reason:"ignored:unclaimed-org"}` entry, plus a no-repo-name-in-logs check, mirroring the unlinked-repo test's shape | N/A — characterization assertion | **Passed immediately** — the `ignored:${result.reason}` logging (added earlier in this PR4 session) already covered `unclaimed-org` identically to `unlinked-repo`; only the missing assertion was added | +| READ-001 — `test/http/github-webhook.test.ts`'s file-level comment and one test title still said mapping/routing were "not yet wired" / "out of scope for this PR (Phase 4)", which became stale once this same PR4 wired them | Rewrote the file-level comment to state that mapping/routing ARE wired, and that this file's non-ping fixtures are deliberately incomplete so they exercise the mapper's "cannot build a GithubEvent" branch; renamed the one affected test from "...outside the (not yet wired) supported set" to "...the mapper cannot build a GithubEvent from (incomplete fixture)" | N/A — comment/title-only, no assertion or behavior touched | **Unchanged, still green** — `npx vitest run test/http/github-webhook.test.ts` → 15/15 (same as before the correction) | +| READ-002 — `stubTelegramApi` was near-identical in `test/http/webhook-e2e.test.ts`, `test/adapters/telegram/alert-sender.test.ts`, and `test/http/github-webhook-delivery-e2e.test.ts` (three separate copies, two written earlier in this PR4 session) | Extracted to `test/support/telegram-stub.ts` — a single implementation that supports both usage shapes (webhook-e2e.test.ts's default `getChatMember`/`sendMessage` result, and the `{ok:false,...}` failure-injection shape used by the new RES-001 tests). All three files now import it; the three local copies were deleted outright | N/A — pure extraction, no behavior change | **All three consuming files stayed green immediately, no test needed rewriting** — `npx vitest run test/http/webhook-e2e.test.ts test/adapters/telegram/alert-sender.test.ts test/http/github-webhook-delivery-e2e.test.ts` → 37/37 pass (23 `webhook-e2e.test.ts` + 6 `alert-sender.test.ts` + 8 `github-webhook-delivery-e2e.test.ts`) | + +Two real behavior additions (RES-001, REL-001's assertion), both test-first with a correct RED. REL-002/REL-003 are disclosed characterization additions — no bug found, no production code changed for either. READ-001/READ-002 are disclosed pure refactors — verified behavior-neutral by the existing/updated suite staying green throughout. + +### Files Changed (this correction) + +| File | Action | What Was Done | +|------|--------|---------------| +| `src/domain/errors.ts` | Modified | Added `AlertSendFailureClass` type; `AlertSendFailedError` now requires a `failureClass` | +| `src/adapters/telegram/alert-sender.ts` | Modified | Added `classifyFailure(err)`; the thrown `AlertSendFailedError` now carries the classified `failureClass` instead of the raw error message | +| `src/domain/usecases/route-github-event.ts` | Modified | `RouteGithubEventResult`'s `"send-failed"` variant now carries `failureClass`, read from the caught `AlertSendFailedError` | +| `src/index.ts` | Modified | The `"send-failed"` branch now logs `reason: result.failureClass` alongside the existing fixed `errorCode: "AlertSendFailed"` | +| `test/fakes/index.ts` | Modified | `fakeAlertSender` takes an optional `failureClass` opt (defaults to `"rejected"`) so domain tests can choose which class `AlertSendFailedError` carries | +| `test/adapters/telegram/alert-sender.test.ts` | Modified | Added 4 classification tests (RES-001); replaced the local `stubTelegramApi` copy with the shared one (READ-002) | +| `test/domain/route-github-event.test.ts` | Modified | The existing send-failed test now asserts `failureClass` is carried through | +| `test/http/github-webhook-delivery-e2e.test.ts` | Modified | Send-failure test: added the exactly-one-`sendMessage`-call assertion (REL-001) and the `reason: "rejected"` assertion (RES-001); added a new 429 → `rate-limited` test; unclaimed-org test: added the logged-reason assertion (REL-003); replaced the local `stubTelegramApi` copy with the shared one (READ-002) | +| `test/adapters/github/event-mapper.test.ts` | Modified | Added 4 null-guard characterization tests (REL-002) | +| `test/http/github-webhook.test.ts` | Modified | Rewrote the stale "not yet wired"/"out of scope" comment and one test title to reflect that mapping/routing are wired as of this PR (READ-001) | +| `test/http/webhook-e2e.test.ts` | Modified | Replaced the local `stubTelegramApi` copy with the shared one (READ-002); the seam-note comment now points to `test/support/telegram-stub.ts` | +| `test/support/telegram-stub.ts` | Created | The shared `stubTelegramApi` (READ-002) | +| `openspec/changes/github-alerts/apply-progress.md` | Modified | This correction section | + +### RED Evidence (verbatim excerpts) + +``` +# RES-001 — alert-sender.test.ts, before classifyFailure existed +AssertionError: expected undefined to be 'rate-limited' // Object.is equality +AssertionError: expected undefined to be 'rejected' // Object.is equality +AssertionError: expected undefined to be 'telegram-unavailable' // Object.is equality (×2 — Telegram-side 5xx, network/HttpError) + +# RES-001 propagation — route-github-event.test.ts, before failureClass was threaded through +AssertionError: expected { kind: 'send-failed', …(1) } to deeply equal { kind: 'send-failed', …(2) } +- Expected ++ Received + { +- "failureClass": "rate-limited", + "kind": "send-failed", + "teamId": "team-1", + } + +# RES-001 logging — github-webhook-delivery-e2e.test.ts, before src/index.ts logged `reason` +AssertionError: expected { event: 'github-webhook', …(2) } to deeply equal { event: 'github-webhook', …(3) } +- Expected ++ Received + { + "errorCode": "AlertSendFailed", + "event": "github-webhook", + "outcome": "error", +- "reason": "rejected", + } +``` + +### New Test Count + +- Before this correction: 311 tests passing (38 files) +- After this correction: **320 tests passing** (38 files, same count — `test/support/telegram-stub.ts` is a support module, not a `.test.ts` file, so it adds no test file) — +9: 4 `alert-sender.test.ts` classification tests + 4 `event-mapper.test.ts` null-guard tests + 1 new `github-webhook-delivery-e2e.test.ts` 429 test (the `route-github-event.test.ts`, REL-001, and REL-003 changes each widened an existing test's assertions rather than adding a new test) +- `npx tsc --noEmit`: clean, no errors + +### Failed-First vs. Passed-Immediately Summary + +- **Failed first (RED, for the right reason), then passed after the fix**: all 4 `alert-sender.test.ts` classification tests; `route-github-event.test.ts`'s widened send-failed assertion; `github-webhook-delivery-e2e.test.ts`'s widened send-failure assertion and its new 429 test. +- **Passed immediately (disclosed characterization, no RED expected)**: all 4 `event-mapper.test.ts` null-guard tests (REL-002); the widened unclaimed-org e2e assertion (REL-003); the `sendMessage`-call-count assertion (REL-001) — none of these exercised a behavior gap, only added coverage for behavior that already existed. +- **Refactor-only, safety net stayed green throughout**: `test/http/github-webhook.test.ts`'s comment/title rewrite (READ-001, 15/15 unchanged); the `stubTelegramApi` extraction into `test/support/telegram-stub.ts` (READ-002, all three consuming files' full suites green before and after). + +### Status (after correction) + +All 6 PR4 review warnings addressed. Full suite: `npx vitest run` → 320/320 pass. `npx tsc --noEmit` → clean, no errors. Two real behavior additions (RES-001's failure classification threaded end-to-end from the adapter through the use case to the HTTP log line, and REL-001's no-retry assertion), both correctly RED-first where behavior changed. Four disclosed characterization/refactor items (REL-002, REL-003, READ-001, READ-002) found no bugs and changed no production behavior beyond RES-001/REL-001. No commit/push made; `.codegraph/` untouched. diff --git a/openspec/changes/github-alerts/tasks.md b/openspec/changes/github-alerts/tasks.md index 1220001..f631746 100644 --- a/openspec/changes/github-alerts/tasks.md +++ b/openspec/changes/github-alerts/tasks.md @@ -56,12 +56,12 @@ Chain strategy: stacked-to-main ## Phase 4: Delivery Wiring (PR4) -- [ ] 4.1 RED: mapper allowlist test — commit email fixture never appears in mapped `GithubEvent`; `merged` derived from `closed`+`pull_request.merged`; `reviewer` from login/team slug. -- [ ] 4.2 GREEN: `src/adapters/github/event-mapper.ts`. -- [ ] 4.3 RED: `AlertSender` test — `sendMessage` carries `message_thread_id`; send failure surfaces as `AlertSendFailedError`, not thrown to caller. -- [ ] 4.4 GREEN: `src/adapters/telegram/alert-sender.ts` (`new Api(BOT_TOKEN)`, no `Bot`/`PII_KEYRING`). -- [ ] 4.5 GREEN: `composition.ts` `buildGithubRouter(env)` wiring deps end to end. -- [ ] 4.6 RED: e2e — linked repo alert delivered; unlinked/unclaimed silent; send failure logs reason-only and returns 2xx; log output has no payload fixture strings (spec: Delivery Failure, Allowlisted Fields). +- [x] 4.1 RED: mapper allowlist test — commit email fixture never appears in mapped `GithubEvent`; `merged` derived from `closed`+`pull_request.merged`; `reviewer` from login/team slug. +- [x] 4.2 GREEN: `src/adapters/github/event-mapper.ts`. +- [x] 4.3 RED: `AlertSender` test — `sendMessage` carries `message_thread_id`; send failure surfaces as `AlertSendFailedError`, not thrown to caller. +- [x] 4.4 GREEN: `src/adapters/telegram/alert-sender.ts` (`new Api(BOT_TOKEN)`, no `Bot`/`PII_KEYRING`). +- [x] 4.5 GREEN: `composition.ts` `buildGithubRouter(env)` wiring deps end to end. +- [x] 4.6 RED: e2e — linked repo alert delivered; unlinked/unclaimed silent; send failure logs reason-only and returns 2xx; log output has no payload fixture strings (spec: Delivery Failure, Allowlisted Fields). ## Phase 5: Link Commands (PR5)