Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/services/notify-discord.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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/");
Expand All @@ -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, "&amp;").replace(/</g, "&lt;").replace(/>/g, "&gt;");
}

Expand Down
68 changes: 67 additions & 1 deletion src/services/review-recap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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<string, unknown>).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. */
Expand Down
103 changes: 102 additions & 1 deletion test/unit/review-recap.test.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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<Array<{ outcome: string; detail: string }>> {
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 <b> & 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("<b>");
expect(text).toContain("&lt;b&gt;");
expect(text).toContain("&amp;");
});

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 }));
Expand Down