From d6c773854138edc6bd54e8383fe9256e72064415 Mon Sep 17 00:00:00 2001 From: Thibault Le Ouay Ducasse Date: Fri, 16 Oct 2026 21:29:20 +0200 Subject: [PATCH] slack: address review on channel context and follow-ups - record created cards in a Redis list (atomic append) - keep user-written titles out of the thread memory note - drop DM ids from permalinks, page all members of a private channel - no draft when the model says no page fits - don't offer relink from a group DM - docs: Started at is conditional, subscribe joins get no welcome card Co-Authored-By: Claude Opus 5.5 --- .../src/libs/test/doubles/upstash.mock.ts | 44 +++++++++++++++++++ .../routes/slack/confirmation-store.test.ts | 10 ++++- .../src/routes/slack/confirmation-store.ts | 39 ++++++++++------ .../server/src/routes/slack/incident-slack.ts | 17 ++++++- .../src/routes/slack/interactions.test.ts | 21 ++++++++- .../src/routes/slack/report-draft.test.ts | 11 +++++ apps/server/src/routes/slack/report-draft.ts | 2 + apps/server/src/routes/slack/slack-links.ts | 8 ++-- .../src/routes/slack/thread-memory.test.ts | 3 +- apps/server/src/routes/slack/thread-memory.ts | 3 +- .../docs/guides/how-to-setup-slack-agent.mdx | 2 +- .../docs/reference/incident-management.mdx | 2 +- 12 files changed, 136 insertions(+), 26 deletions(-) diff --git a/apps/server/src/libs/test/doubles/upstash.mock.ts b/apps/server/src/libs/test/doubles/upstash.mock.ts index 6522349de..832213313 100644 --- a/apps/server/src/libs/test/doubles/upstash.mock.ts +++ b/apps/server/src/libs/test/doubles/upstash.mock.ts @@ -28,8 +28,52 @@ const client = { return Promise.resolve(next); }, expire: () => Promise.resolve(1), + // Lists live in the same Map as a JSON array of their items. + rpush: (key: string, ...values: string[]) => { + const list = [...readList(key), ...values]; + store.set(key, JSON.stringify(list)); + return Promise.resolve(list.length); + }, + ltrim: (key: string, start: number, stop: number) => { + store.set(key, JSON.stringify(slice(readList(key), start, stop))); + return Promise.resolve("OK"); + }, + lrange: (key: string, start: number, stop: number) => + Promise.resolve(slice(readList(key), start, stop)), + // Commands run as they are queued, so `exec` only collects their results. + multi() { + const results: Promise[] = []; + const tx = { + rpush: (key: string, ...values: string[]) => { + results.push(client.rpush(key, ...values)); + return tx; + }, + ltrim: (key: string, start: number, stop: number) => { + results.push(client.ltrim(key, start, stop)); + return tx; + }, + expire: (..._args: unknown[]) => { + results.push(client.expire()); + return tx; + }, + exec: () => Promise.all(results), + }; + return tx; + }, }; +function readList(key: string): string[] { + const raw = store.get(key); + return raw ? (JSON.parse(raw) as string[]) : []; +} + +/** Redis LRANGE/LTRIM bounds: inclusive, negative counts from the end. */ +function slice(list: string[], start: number, stop: number): string[] { + const from = start < 0 ? Math.max(list.length + start, 0) : start; + const to = stop < 0 ? list.length + stop : stop; + return list.slice(from, to + 1); +} + export const redis = client; export const Redis = { diff --git a/apps/server/src/routes/slack/confirmation-store.test.ts b/apps/server/src/routes/slack/confirmation-store.test.ts index a670f1e5d..40a9e84c6 100644 --- a/apps/server/src/routes/slack/confirmation-store.test.ts +++ b/apps/server/src/routes/slack/confirmation-store.test.ts @@ -278,7 +278,13 @@ describe("created records", () => { }); test("ignores a corrupted entry", async () => { - redisStore.set("slack:created:9.3", JSON.stringify([{ kind: "nope" }])); - expect(await listCreated("9.3")).toEqual([]); + redisStore.set( + "slack:created-list:9.3", + JSON.stringify([ + JSON.stringify({ kind: "nope" }), + JSON.stringify({ kind: "incident", id: 7 }), + ]), + ); + expect(await listCreated("9.3")).toEqual([{ kind: "incident", id: 7 }]); }); }); diff --git a/apps/server/src/routes/slack/confirmation-store.ts b/apps/server/src/routes/slack/confirmation-store.ts index 633c6ef8d..b7b1cac09 100644 --- a/apps/server/src/routes/slack/confirmation-store.ts +++ b/apps/server/src/routes/slack/confirmation-store.ts @@ -94,6 +94,11 @@ function appendCreated( return [...rest, record].slice(-MAX_CREATED); } +/** Collapses repeats of the same record, keeping its latest position. */ +function dedupeCreated(list: CreatedRecord[]): CreatedRecord[] { + return list.reduce(appendCreated, []); +} + /** * A storage backstop, not a deadline. Approving is the user's call and a draft * stays clickable for as long as they need — the card is a Slack message, so @@ -103,7 +108,8 @@ function appendCreated( const TTL_SECONDS = 30 * 24 * 60 * 60; const ACTION_PREFIX = "slack:action:"; const THREAD_PREFIX = "slack:thread:"; -const CREATED_PREFIX = "slack:created:"; +// A Redis list, so concurrent approvals append without overwriting each other. +const CREATED_PREFIX = "slack:created-list:"; function parse(raw: unknown): PendingAction | undefined { const data = typeof raw === "string" ? JSON.parse(raw) : raw; @@ -194,21 +200,28 @@ export function createRedisCarrierStore(): CarrierStore { }, async recordCreated(threadTs, record) { - const list = await this.listCreated(threadTs); - await redis.set( - `${CREATED_PREFIX}${threadTs}`, - JSON.stringify(appendCreated(list, record)), - { ex: TTL_SECONDS }, - ); + const key = `${CREATED_PREFIX}${threadTs}`; + await redis + .multi() + .rpush(key, JSON.stringify(record)) + .ltrim(key, -MAX_CREATED, -1) + .expire(key, TTL_SECONDS) + .exec(); }, async listCreated(threadTs) { - const raw = await redis.get(`${CREATED_PREFIX}${threadTs}`); - if (!raw) return []; - const parsed = z - .array(createdRecordSchema) - .safeParse(typeof raw === "string" ? JSON.parse(raw) : raw); - return parsed.success ? parsed.data : []; + const raw = await redis.lrange( + `${CREATED_PREFIX}${threadTs}`, + 0, + -1, + ); + const records = raw.flatMap((item) => { + const parsed = createdRecordSchema.safeParse( + typeof item === "string" ? JSON.parse(item) : item, + ); + return parsed.success ? [parsed.data] : []; + }); + return dedupeCreated(records); }, }; } diff --git a/apps/server/src/routes/slack/incident-slack.ts b/apps/server/src/routes/slack/incident-slack.ts index 5f3f06e11..9d09a148b 100644 --- a/apps/server/src/routes/slack/incident-slack.ts +++ b/apps/server/src/routes/slack/incident-slack.ts @@ -173,6 +173,20 @@ export function offersRelink(sourceId: string, incidentChannelId: string) { return /^[CG]/.test(sourceId) && sourceId !== incidentChannelId; } +/** `G…` is also a group DM's prefix: ask Slack. Unknown counts as no. */ +export async function isSlackChannel(slack: WebClient, channelId: string) { + const info = await slack.conversations + .info({ channel: channelId }) + .catch(() => undefined); + const channel = info?.channel; + return Boolean( + channel && + (channel.is_channel || channel.is_group) && + !channel.is_mpim && + !channel.is_im, + ); +} + export function relinkNoticeBlocks( incidentId: number, incidentChannelId: string, @@ -235,7 +249,8 @@ export async function onIncidentDeclared( if ( source?.channelId && result.status === "bound" && - offersRelink(source.channelId, result.channelId) + offersRelink(source.channelId, result.channelId) && + (await isSlackChannel(source.slack, source.channelId)) ) { relinkPosted = await source.slack.chat .postMessage({ diff --git a/apps/server/src/routes/slack/interactions.test.ts b/apps/server/src/routes/slack/interactions.test.ts index 8312e7a4f..e09deb58a 100644 --- a/apps/server/src/routes/slack/interactions.test.ts +++ b/apps/server/src/routes/slack/interactions.test.ts @@ -45,7 +45,11 @@ import { import { busyChannelHistory } from "@/libs/test/slack-incident-fixtures"; import type { SlackEnv } from "./config"; -import { offersRelink, relinkNoticeBlocks } from "./incident-slack"; +import { + isSlackChannel, + offersRelink, + relinkNoticeBlocks, +} from "./incident-slack"; import { handleSlackInteraction } from "./interactions"; import { postReportFollowUp } from "./report-follow-up"; import { verifySlackSignature } from "./verify"; @@ -2164,6 +2168,21 @@ describe("link this channel instead", () => { expect(offersRelink("C2", "C2")).toBe(false); }); + test("a group DM is not a channel, even with a G id", async () => { + const slack = new WebClient("xoxb-test"); + const as = (channel: unknown) => { + slackTestState.conversationsInfoImpl = () => + Promise.resolve({ ok: true, channel }); + }; + as({ is_group: true }); + expect(await isSlackChannel(slack, "G1")).toBe(true); + as({ is_group: true, is_mpim: true }); + expect(await isSlackChannel(slack, "G1")).toBe(false); + slackTestState.conversationsInfoImpl = () => + Promise.reject(new Error("missing_scope")); + expect(await isSlackChannel(slack, "G1")).toBe(false); + }); + test("moves the incident, pins its card and archives the old channel", async () => { await relink(); expect(await boundChannel()).toBe(sourceChannel); diff --git a/apps/server/src/routes/slack/report-draft.test.ts b/apps/server/src/routes/slack/report-draft.test.ts index 96cd2877b..e3dfd9606 100644 --- a/apps/server/src/routes/slack/report-draft.test.ts +++ b/apps/server/src/routes/slack/report-draft.test.ts @@ -64,6 +64,17 @@ describe("validateDraft", () => { expect(draft?.pageId).toBe(2); }); + test("no page fits, no draft", () => { + expect( + validateDraft( + { ...modelDraft, pageId: null }, + candidates, + incidentStartedAt, + now, + ), + ).toBeUndefined(); + }); + test("keeps a start only when it is earlier, past and recent", () => { const at = (startedAt: string | null) => validateDraft( diff --git a/apps/server/src/routes/slack/report-draft.ts b/apps/server/src/routes/slack/report-draft.ts index 11773e496..aa2c492cb 100644 --- a/apps/server/src/routes/slack/report-draft.ts +++ b/apps/server/src/routes/slack/report-draft.ts @@ -126,6 +126,8 @@ export function validateDraft( incidentStartedAt: Date, now = new Date(), ): ReportDraft | undefined { + // The model saw every page: its "no page fits" must not become a preselection. + if (draft.pageId === null) return undefined; const failingPage = candidates.components.find((c) => c.failing)?.pageId; const pageId = candidates.pages.find((p) => p.id === draft.pageId)?.id ?? diff --git a/apps/server/src/routes/slack/slack-links.ts b/apps/server/src/routes/slack/slack-links.ts index 37004be27..363a373cd 100644 --- a/apps/server/src/routes/slack/slack-links.ts +++ b/apps/server/src/routes/slack/slack-links.ts @@ -8,10 +8,9 @@ import { formatSlackTime, slackTsToDate } from "./thread-messages"; const logger = getLogger("api-server"); const PERMALINK = - /https?:\/\/[\w-]+\.slack\.com\/archives\/([CDG][A-Z0-9]+)\/p(\d{16})(\?[^\s>|]*)?/g; + /https?:\/\/[\w-]+\.slack\.com\/archives\/([CG][A-Z0-9]+)\/p(\d{16})(\?[^\s>|]*)?/g; const MAX_LINKS = 5; const MAX_TEXT_CHARS = 2_000; -const MAX_MEMBER_PAGES = 10; export type Permalink = { url: string; @@ -76,7 +75,7 @@ async function isMember( userId: string, ): Promise { let cursor: string | undefined; - for (let page = 0; page < MAX_MEMBER_PAGES; page++) { + do { const res = await slack.conversations.members({ channel, limit: 1000, @@ -84,8 +83,7 @@ async function isMember( }); if (res.members?.includes(userId)) return true; cursor = res.response_metadata?.next_cursor || undefined; - if (!cursor) return false; - } + } while (cursor); return false; } diff --git a/apps/server/src/routes/slack/thread-memory.test.ts b/apps/server/src/routes/slack/thread-memory.test.ts index 58504ec2e..855d9c7bb 100644 --- a/apps/server/src/routes/slack/thread-memory.test.ts +++ b/apps/server/src/routes/slack/thread-memory.test.ts @@ -47,7 +47,8 @@ describe("createdNote", () => { { kind: "status_report", id: 12, label: "API down" }, { kind: "incident", id: 7 }, ]); - expect(note).toContain('status report #12 "API down"'); + expect(note).toContain("status report #12"); + expect(note).not.toContain("API down"); expect(note).toContain("managed incident #7"); expect(createdNote([])).toBeUndefined(); }); diff --git a/apps/server/src/routes/slack/thread-memory.ts b/apps/server/src/routes/slack/thread-memory.ts index 49f7bf551..74bbe225d 100644 --- a/apps/server/src/routes/slack/thread-memory.ts +++ b/apps/server/src/routes/slack/thread-memory.ts @@ -45,7 +45,8 @@ const NOUNS: Record = { export function createdNote(records: CreatedRecord[]): string | undefined { if (!records.length) return undefined; const items = records.map( - (r) => `${NOUNS[r.kind]} #${r.id}${r.label ? ` "${r.label}"` : ""}`, + // Titles are user-written: keep them out of the system prompt. + (r) => `${NOUNS[r.kind]} #${r.id}`, ); return `Earlier in this thread, approved cards created or updated: ${items.join(", ")}. These exist now: use their ids only in subsequent tool calls, never show the ids in replies, and don't tell the user they don't exist.`; } diff --git a/apps/web/src/content/pages/docs/guides/how-to-setup-slack-agent.mdx b/apps/web/src/content/pages/docs/guides/how-to-setup-slack-agent.mdx index 69d12f7b2..22f431a53 100644 --- a/apps/web/src/content/pages/docs/guides/how-to-setup-slack-agent.mdx +++ b/apps/web/src/content/pages/docs/guides/how-to-setup-slack-agent.mdx @@ -54,7 +54,7 @@ When someone invites the bot to a channel, that person gets a private **What do - **Catch up on this channel**: the agent reads the recent messages, posts a summary in a thread, and drafts a status report if something is going on. - **Publish status update**: opens the status report form. -The welcome card does not read the channel until someone clicks **Catch up**. +The welcome card does not read the channel until someone clicks **Catch up**. When the bot joins through `/openstatus subscribe`, there is no inviter, so no card is shown. ### 4. Verify the installation diff --git a/apps/web/src/content/pages/docs/reference/incident-management.mdx b/apps/web/src/content/pages/docs/reference/incident-management.mdx index e9faa1d80..7e98862e5 100644 --- a/apps/web/src/content/pages/docs/reference/incident-management.mdx +++ b/apps/web/src/content/pages/docs/reference/incident-management.mdx @@ -81,7 +81,7 @@ Requires the [Slack agent](/docs/guides/how-to-setup-slack-agent), which is avai Declaring an incident, from Slack or the dashboard, opens a channel named `inc-YYYY-MM-DD-` (UTC date). The declarer is invited, the topic shows severity, status and a link to the incident, and the incident card is pinned. - **Link this channel instead.** When you declare from an existing channel, that channel gets a notice with this button. Clicking it moves the incident there: the card is pinned in it, the `inc-` channel is told and archived, and the move is on the timeline. -- **Create a status report?** The incident channel gets a card with a suggested first update, drafted from the incident and the channel you declared from. It suggests the page, components and **Started at**, the time users were first affected; the form also pre-fills the impact. **Create status report** opens the form filled in. Nothing is published until you submit it. **Not needed** dismisses the card. +- **Create a status report?** The incident channel gets a card with a suggested first update, drafted from the incident and the channel you declared from. It suggests the page, components and **Started at** (the time users were first affected) when the draft has enough evidence; the form also pre-fills the impact. **Create status report** opens the form filled in. Nothing is published until you submit it. **Not needed** dismisses the card. - Use the pinned card's buttons. **Update status** changes the status or severity, or posts an update to the channel and timeline. **Update status page** publishes a status report linked to the incident, or adds an update to the one already linked. You can notify subscribers from the same form. The card redraws with the new status after the change. - **Started at** on a new status report sets when the report begins, defaulting to the incident's start. Picking an earlier time also moves the incident's start back. It can't be in the future. -- 2.54.0