diff --git a/src/services/notify-discord.ts b/src/services/notify-discord.ts index 0ca521dde3..08c0230ce2 100644 --- a/src/services/notify-discord.ts +++ b/src/services/notify-discord.ts @@ -149,8 +149,10 @@ export async function notifyActionToDiscord( } } -/** Slack incoming-webhook URL validation — only `https://hooks.slack.com/services/…`. */ -function isValidSlackWebhook(url: string): boolean { +/** Slack incoming-webhook URL validation — only `https://hooks.slack.com/services/…`. Exported so other + * Slack senders (e.g. the recap digest's {@link deliverRecapToSlack}, review-recap.ts) reuse the SAME + * validation instead of re-typing the host/path allowlist. */ +export function isValidSlackWebhook(url: string): boolean { try { const parsed = new URL(url); return parsed.protocol === "https:" && parsed.hostname.toLowerCase() === "hooks.slack.com" && parsed.pathname.startsWith("/services/"); @@ -159,7 +161,8 @@ function isValidSlackWebhook(url: string): boolean { } } -function escapeSlackMrkdwnText(value: string): string { +/** Exported alongside {@link isValidSlackWebhook} so any Slack Block Kit sender escapes mrkdwn the same way. */ +export function escapeSlackMrkdwnText(value: string): string { return value.replace(/&/g, "&").replace(//g, ">"); } diff --git a/src/services/review-recap.ts b/src/services/review-recap.ts index a2d368155c..4fef950e98 100644 --- a/src/services/review-recap.ts +++ b/src/services/review-recap.ts @@ -11,9 +11,16 @@ // (generateAndSendReviewRecap, reusing resolveDiscordWebhook from notify-discord.ts — no second webhook // resolution mechanism). The scheduled cron trigger (mirroring the weekly-value-report cron wiring in // src/index.ts) is a clear, scoped follow-up — see the PR description. +// +// #2246 adds deliverRecapToSlack, the Slack sibling of sendReviewRecapToDiscord: same recap, same +// best-effort/never-throws contract, delivered to SLACK_WEBHOOK_URL as a Block Kit mrkdwn section instead of +// a Discord embed. It reuses isValidSlackWebhook + escapeSlackMrkdwnText from notify-discord.ts — the SAME +// validation/escaping notifyActionToSlack's per-event notifier uses — so there is only one Slack webhook +// allowlist and one mrkdwn escaper in the codebase. Fanning both channels out together (#2252) is a +// follow-up; this PR only adds the standalone Slack delivery function. import { listPullRequests, recordAuditEvent } from "../db/repositories"; import { computeGateEval } from "../review/parity"; -import { resolveDiscordWebhook } from "./notify-discord"; +import { escapeSlackMrkdwnText, isValidSlackWebhook, resolveDiscordWebhook } from "./notify-discord"; import type { ReviewRecap } from "../types"; import { errorMessage, nowIso } from "../utils/json"; import { PUBLIC_LOCAL_PATH_SCRUB_PATTERN } from "../signals/redaction"; @@ -193,6 +200,65 @@ export async function sendReviewRecapToDiscord(env: Env, recap: ReviewRecap): Pr } } +/** Post the recap to `SLACK_WEBHOOK_URL` as a Block Kit mrkdwn section, reusing {@link isValidSlackWebhook} + + * {@link escapeSlackMrkdwnText} from notify-discord.ts — the SAME validation/escaping notifyActionToSlack's + * per-event notifier uses (#2246, sibling of {@link sendReviewRecapToDiscord}). Best-effort: a delivery + * failure is recorded to the audit ledger but never thrown, mirroring notifyActionToSlack's fail-safe + * contract. */ +export async function deliverRecapToSlack(env: Env, recap: ReviewRecap): Promise<{ sent: boolean; reason?: string }> { + const webhookUrl = (env as unknown as Record).SLACK_WEBHOOK_URL; + if (typeof webhookUrl !== "string" || !isValidSlackWebhook(webhookUrl)) { + const reason = typeof webhookUrl === "string" ? "invalid_webhook" : "missing_webhook"; + await recordAuditEvent(env, { + eventType: "review_recap_notification.slack", + actor: "gittensory", + targetKey: `review-recap:${recap.repoFullName}:${recap.windowDays}`, + outcome: "denied", + detail: reason, + metadata: { repoFullName: recap.repoFullName, windowDays: recap.windowDays }, + }); + return { sent: false, reason }; + } + const lines = [ + `*${escapeSlackMrkdwnText(recap.repoFullName)} · review recap (${recap.windowDays}d)*`, + escapeSlackMrkdwnText(formatRecapDescription(recap)), + ]; + const body = { + text: `${recap.repoFullName} review recap (${recap.windowDays}d)`, + blocks: [{ type: "section", text: { type: "mrkdwn", text: lines.join("\n") } }], + }; + try { + const response = await fetch(webhookUrl, { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + signal: AbortSignal.timeout(10_000), + }); + if (!response.ok) throw new Error(`slack_webhook_http_${response.status}`); + await recordAuditEvent(env, { + eventType: "review_recap_notification.slack", + actor: "gittensory", + targetKey: `review-recap:${recap.repoFullName}:${recap.windowDays}`, + outcome: "completed", + detail: "sent", + metadata: { repoFullName: recap.repoFullName, windowDays: recap.windowDays }, + }); + return { sent: true }; + } catch (error) { + const detail = errorMessage(error).slice(0, 160); + console.warn(JSON.stringify({ event: "review_recap_slack_failed", repo: recap.repoFullName, message: detail })); + await recordAuditEvent(env, { + eventType: "review_recap_notification.slack", + actor: "gittensory", + targetKey: `review-recap:${recap.repoFullName}:${recap.windowDays}`, + outcome: "error", + detail, + metadata: { repoFullName: recap.repoFullName, windowDays: recap.windowDays }, + }); + return { sent: false, reason: detail }; + } +} + /** Build the recap for one repo and deliver it to Discord in one call — the manual-trigger entry point * (`/v1/internal/jobs/generate-review-recap/run`). Always returns the recap even when delivery is denied * (e.g. no webhook configured), so the caller can inspect the computed numbers either way. */ diff --git a/test/unit/review-recap.test.ts b/test/unit/review-recap.test.ts index aa21fbef4a..d9a482511c 100644 --- a/test/unit/review-recap.test.ts +++ b/test/unit/review-recap.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, it, vi } from "vitest"; -import { buildReviewRecap, generateAndSendReviewRecap, loadReviewRecap, sendReviewRecapToDiscord } from "../../src/services/review-recap"; +import { buildReviewRecap, deliverRecapToSlack, generateAndSendReviewRecap, loadReviewRecap, sendReviewRecapToDiscord } from "../../src/services/review-recap"; import { createTestEnv } from "../helpers/d1"; const NOW = "2026-07-06T00:00:00Z"; @@ -292,6 +292,107 @@ describe("sendReviewRecapToDiscord (#1963, reuses resolveDiscordWebhook)", () => }); }); +const SLACK_HOOK = "https://hooks.slack.com/services/T0/B0/xyz"; + +function envWithSlackWebhook(): Env { + return Object.assign(createTestEnv(), { SLACK_WEBHOOK_URL: SLACK_HOOK }) as Env; +} + +async function slackAuditRows(env: Env): Promise> { + const rows = await env.DB.prepare("select outcome, detail from audit_events where event_type = 'review_recap_notification.slack' order by created_at").all<{ outcome: string; detail: string }>(); + return rows.results ?? []; +} + +describe("deliverRecapToSlack (#2246, reuses isValidSlackWebhook/escapeSlackMrkdwnText)", () => { + const recap = buildReviewRecap({ + repoFullName: "JSONbored/gittensory", + generatedAt: NOW, + windowDays: 7, + pullRequests: [], + gateMergePrecision: 0.95, + gateDecided: 20, + }); + + it("posts a Block Kit mrkdwn section and records a completed audit event when a webhook IS configured (isValidSlackWebhook true side)", async () => { + const calls: Array<{ url: string; body: string }> = []; + vi.stubGlobal("fetch", async (url: RequestInfo | URL, init?: RequestInit) => { + calls.push({ url: String(url), body: String(init?.body ?? "") }); + return new Response(null, { status: 200 }); + }); + const env = envWithSlackWebhook(); + const result = await deliverRecapToSlack(env, recap); + expect(result.sent).toBe(true); + expect(calls).toHaveLength(1); + expect(calls[0]?.url).toBe(SLACK_HOOK); + const body = JSON.parse(calls[0]?.body ?? "{}"); + expect(body.blocks[0].text.text).toContain("JSONbored/gittensory"); + const rows = await slackAuditRows(env); + expect(rows.some((r) => r.outcome === "completed")).toBe(true); + }); + + it("escapes &, <, and > in both the repo name and the summary text (mrkdwn escaping)", async () => { + const calls: Array<{ body: string }> = []; + vi.stubGlobal("fetch", async (_url: RequestInfo | URL, init?: RequestInit) => { + calls.push({ body: String(init?.body ?? "") }); + return new Response(null, { status: 200 }); + }); + const unsafeRecap = buildReviewRecap({ + repoFullName: "acme/widgets & co", + generatedAt: NOW, + windowDays: 7, + pullRequests: [], + gateMergePrecision: null, + gateDecided: 0, + }); + const env = envWithSlackWebhook(); + await deliverRecapToSlack(env, unsafeRecap); + const text = JSON.parse(calls[0]?.body ?? "{}").blocks[0].text.text as string; + expect(text).not.toContain(""); + expect(text).toContain("<b>"); + expect(text).toContain("&"); + }); + + it("denies delivery with missing_webhook and records it when SLACK_WEBHOOK_URL is unset (typeof webhookUrl !== string side)", async () => { + const env = createTestEnv(); + const result = await deliverRecapToSlack(env, recap); + expect(result.sent).toBe(false); + expect(result.reason).toBe("missing_webhook"); + const rows = await slackAuditRows(env); + expect(rows.some((r) => r.outcome === "denied" && r.detail === "missing_webhook")).toBe(true); + }); + + it("denies delivery with invalid_webhook when SLACK_WEBHOOK_URL is set but fails isValidSlackWebhook (isValidSlackWebhook false side)", async () => { + const env = Object.assign(createTestEnv(), { SLACK_WEBHOOK_URL: "https://evil.example/services/x" }) as Env; + const result = await deliverRecapToSlack(env, recap); + expect(result.sent).toBe(false); + expect(result.reason).toBe("invalid_webhook"); + const rows = await slackAuditRows(env); + expect(rows.some((r) => r.outcome === "denied" && r.detail === "invalid_webhook")).toBe(true); + }); + + it("degrades to a recorded error result when the webhook POST throws (fail-safe path, never throws)", async () => { + vi.stubGlobal("fetch", async () => { + throw new Error("network down"); + }); + const env = envWithSlackWebhook(); + const result = await deliverRecapToSlack(env, recap); + expect(result.sent).toBe(false); + expect(result.reason).toBe("network down"); + const rows = await slackAuditRows(env); + expect(rows.some((r) => r.outcome === "error")).toBe(true); + }); + + it("treats a non-2xx webhook response as a failure (mirrors notifyActionToSlack's http-status guard)", async () => { + vi.stubGlobal("fetch", async () => new Response(null, { status: 403 })); + const env = envWithSlackWebhook(); + const result = await deliverRecapToSlack(env, recap); + expect(result.sent).toBe(false); + expect(result.reason).toBe("slack_webhook_http_403"); + const rows = await slackAuditRows(env); + expect(rows.some((r) => r.outcome === "error" && r.detail === "slack_webhook_http_403")).toBe(true); + }); +}); + describe("generateAndSendReviewRecap (#1963, manual-trigger entry point)", () => { it("builds the recap and returns both the recap and the delivery result together", async () => { vi.stubGlobal("fetch", async () => new Response(null, { status: 204 }));