diff --git a/apps/server/src/routes/slack/confirmation-store.test.ts b/apps/server/src/routes/slack/confirmation-store.test.ts index 15fd94a1..d8bb07c1 100644 --- a/apps/server/src/routes/slack/confirmation-store.test.ts +++ b/apps/server/src/routes/slack/confirmation-store.test.ts @@ -3,6 +3,7 @@ import { beforeEach, describe, test } from "@std/testing/bdd"; import { consume, + draftKey, findByThread, get, replace, @@ -50,7 +51,7 @@ describe("confirmation-store", () => { const id = await store(input); const actionKey = `slack:action:${id}`; - const threadKey = `slack:thread:${input.threadTs}`; + const threadKey = `slack:thread:${input.threadTs}:create_status_report`; expect(redisStore.has(actionKey)).toBe(true); expect(redisStore.has(threadKey)).toBe(true); @@ -77,7 +78,9 @@ describe("confirmation-store", () => { expect(result?.payload.toolName).toBe("create_status_report"); expect(redisStore.has(`slack:action:${id}`)).toBe(true); - expect(redisStore.has(`slack:thread:${input.threadTs}`)).toBe(true); + expect( + redisStore.has(`slack:thread:${input.threadTs}:create_status_report`), + ).toBe(true); }); test("returns undefined for unknown id", async () => { @@ -104,7 +107,9 @@ describe("confirmation-store", () => { expect(result?.payload.toolName).toBe("create_status_report"); expect(redisStore.has(`slack:action:${id}`)).toBe(false); - expect(redisStore.has(`slack:thread:${input.threadTs}`)).toBe(false); + expect( + redisStore.has(`slack:thread:${input.threadTs}:create_status_report`), + ).toBe(false); }); test("returns undefined for unknown id", async () => { @@ -125,22 +130,72 @@ describe("confirmation-store", () => { const input = makePendingInput(); const id = await store(input); - const result = await findByThread(input.threadTs); + const result = await findByThread(input.threadTs, input.payload); expect(result).toBeDefined(); expect(result?.id).toBe(id); }); test("returns undefined for unknown thread", async () => { - const result = await findByThread("unknown.thread"); + const result = await findByThread( + "unknown.thread", + makePendingInput().payload, + ); expect(result).toBeUndefined(); }); test("cleans up orphaned thread index", async () => { - redisStore.set("slack:thread:orphan.ts", "missing-id"); - - const result = await findByThread("orphan.ts"); + redisStore.set( + "slack:thread:orphan.ts:create_status_report", + "missing-id", + ); + + const result = await findByThread( + "orphan.ts", + makePendingInput().payload, + ); expect(result).toBeUndefined(); - expect(redisStore.has("slack:thread:orphan.ts")).toBe(false); + expect( + redisStore.has("slack:thread:orphan.ts:create_status_report"), + ).toBe(false); + }); + + test("keeps separate drafts in one thread apart", async () => { + const input = makePendingInput(); + const rename = { + toolName: "update_status_report", + input: { statusReportId: 7, title: "Elevated API error rate" }, + }; + const update = { + toolName: "add_status_report_update", + input: { statusReportId: 7, status: "investigating", message: "500s" }, + }; + const renameId = await store({ ...input, payload: rename }); + const updateId = await store({ ...input, payload: update }); + + expect((await findByThread(input.threadTs, rename))?.id).toBe(renameId); + expect((await findByThread(input.threadTs, update))?.id).toBe(updateId); + + // Approving one card leaves the other clickable. + await consume(renameId); + expect(await findByThread(input.threadTs, rename)).toBeUndefined(); + expect((await findByThread(input.threadTs, update))?.id).toBe(updateId); + }); + + test("narrows the draft to the report it acts on", () => { + expect( + draftKey({ + toolName: "add_status_report_update", + input: { statusReportId: 1 }, + }), + ).not.toBe( + draftKey({ + toolName: "add_status_report_update", + input: { statusReportId: 2 }, + }), + ); + expect(draftKey({ toolName: "create_maintenance", input: {} })).toBe( + "create_maintenance", + ); }); }); diff --git a/apps/server/src/routes/slack/confirmation-store.ts b/apps/server/src/routes/slack/confirmation-store.ts index 9dbdd7a8..6c212fe0 100644 --- a/apps/server/src/routes/slack/confirmation-store.ts +++ b/apps/server/src/routes/slack/confirmation-store.ts @@ -11,6 +11,27 @@ const pendingPayloadSchema = z.object({ export type PendingPayload = z.infer; +/** + * What a card stands for within its thread: the tool, narrowed to the report + * it acts on when there is one. A later draft with the same key revises that + * card; a different key gets a card of its own — so renaming a report and + * posting an update to it are two cards, not one replacing the other. + */ +export function draftKey(payload: PendingPayload): string { + const input = payload.input; + const reportId = + typeof input === "object" && input !== null + ? (input as { statusReportId?: unknown }).statusReportId + : undefined; + return typeof reportId === "number" + ? `${payload.toolName}:${reportId}` + : payload.toolName; +} + +function threadKey(threadTs: string, payload: PendingPayload): string { + return `${threadTs}:${draftKey(payload)}`; +} + const pendingActionSchema = z.object({ id: z.string(), workspaceId: z.number(), @@ -43,7 +64,11 @@ export interface CarrierStore { get(id: string): Promise; /** Atomic getdel — defends against double-click double-execution. */ consume(id: string): Promise; - findByThread(threadTs: string): Promise; + /** The thread's pending action standing for the same draft, if any. */ + findByThread( + threadTs: string, + payload: PendingPayload, + ): Promise; replace(id: string, payload: PendingPayload): Promise; } @@ -77,9 +102,13 @@ export function createRedisCarrierStore(): CarrierStore { redis.set(`${ACTION_PREFIX}${id}`, JSON.stringify(pending), { ex: TTL_SECONDS, }), - redis.set(`${THREAD_PREFIX}${action.threadTs}`, id, { - ex: TTL_SECONDS, - }), + redis.set( + `${THREAD_PREFIX}${threadKey(action.threadTs, action.payload)}`, + id, + { + ex: TTL_SECONDS, + }, + ), ]); return id; @@ -99,18 +128,21 @@ export function createRedisCarrierStore(): CarrierStore { if (!action) return undefined; // Thread mapping cleanup is best-effort and not part of atomicity. - await redis.del(`${THREAD_PREFIX}${action.threadTs}`); + await redis.del( + `${THREAD_PREFIX}${threadKey(action.threadTs, action.payload)}`, + ); return action; }, - async findByThread(threadTs) { - const actionId = await redis.get(`${THREAD_PREFIX}${threadTs}`); + async findByThread(threadTs, payload) { + const key = `${THREAD_PREFIX}${threadKey(threadTs, payload)}`; + const actionId = await redis.get(key); if (!actionId) return undefined; const raw = await redis.get(`${ACTION_PREFIX}${actionId}`); if (!raw) { - await redis.del(`${THREAD_PREFIX}${threadTs}`); + await redis.del(key); return undefined; } @@ -131,7 +163,10 @@ export function createRedisCarrierStore(): CarrierStore { redis.set(`${ACTION_PREFIX}${id}`, JSON.stringify(existing), { ex: TTL_SECONDS, }), - redis.expire(`${THREAD_PREFIX}${existing.threadTs}`, TTL_SECONDS), + redis.expire( + `${THREAD_PREFIX}${threadKey(existing.threadTs, existing.payload)}`, + TTL_SECONDS, + ), ]); }, }; @@ -150,7 +185,7 @@ export function createMemoryCarrierStore(): CarrierStore { const id = nanoid(); const pending: PendingAction = { ...action, id, createdAt: Date.now() }; actions.set(id, pending); - threads.set(action.threadTs, id); + threads.set(threadKey(action.threadTs, action.payload), id); return id; }, @@ -162,16 +197,17 @@ export function createMemoryCarrierStore(): CarrierStore { const action = actions.get(id); if (!action) return undefined; actions.delete(id); - threads.delete(action.threadTs); + threads.delete(threadKey(action.threadTs, action.payload)); return action; }, - async findByThread(threadTs) { - const id = threads.get(threadTs); + async findByThread(threadTs, payload) { + const key = threadKey(threadTs, payload); + const id = threads.get(key); if (!id) return undefined; const action = actions.get(id); if (!action) { - threads.delete(threadTs); + threads.delete(key); return undefined; } return action; @@ -202,6 +238,8 @@ export const consume = (id: string): Promise => defaultStore.consume(id); export const findByThread = ( threadTs: string, -): Promise => defaultStore.findByThread(threadTs); + payload: PendingPayload, +): Promise => + defaultStore.findByThread(threadTs, payload); export const replace = (id: string, payload: PendingPayload): Promise => defaultStore.replace(id, payload); diff --git a/apps/server/src/routes/slack/handler.test.ts b/apps/server/src/routes/slack/handler.test.ts index 252c3dda..42d8c26a 100644 --- a/apps/server/src/routes/slack/handler.test.ts +++ b/apps/server/src/routes/slack/handler.test.ts @@ -1587,3 +1587,71 @@ describe("the channel the user is viewing", () => { expect(seen[0].tools).toBeUndefined(); }); }); + +describe("confirmation cards", () => { + const app = createTestApp(); + + beforeEach(resetSlackTestState); + + test("posts a card for every draft in the turn", async () => { + const draft = (toolName: string, input: Record) => ({ + toolName, + result: { needsConfirmation: true, toolName, input, displayInput: input }, + }); + slackTestState.runAgentOverride = () => + Promise.resolve({ + text: "Two cards for you.", + toolResults: [ + draft("update_status_report", { + statusReportId: 7, + title: "Elevated API error rate", + }), + draft("add_status_report_update", { + statusReportId: 7, + status: "investigating", + message: "Some requests return 500s.", + }), + ], + finishReason: "stop", + stepCount: 2, + hitStepLimit: false, + aborted: false, + }); + + await signAndPost(app, { + type: "event_callback", + team_id: "T_KNOWN", + event_id: `evt_two_cards_${Date.now()}`, + event: { + type: "app_mention", + text: "<@UBOT> rename it and post an update", + user: "U1", + channel: "C1", + ts: `${Date.now()}.30`, + }, + }); + await new Promise((r) => setTimeout(r, 100)); + + const actionIds = slackTestState.calls + .filter((m) => m.method === "update" && Array.isArray(m.args.blocks)) + .flatMap((m) => + ( + m.args.blocks as { + type: string; + elements?: { action_id: string }[]; + }[] + ) + .filter((b) => b.type === "actions") + .flatMap((b) => b.elements?.map((e) => e.action_id) ?? []), + ); + // Rename: approve + cancel. Update: approve, approve & notify, cancel. + expect(actionIds.filter((id) => id.startsWith("cancel_"))).toHaveLength(2); + expect( + actionIds.filter((id) => id.startsWith("approve_flag_")), + ).toHaveLength(1); + // The second card is a message of its own, not an overwrite of the first. + expect( + slackTestState.calls.filter((m) => m.method === "postMessage").length, + ).toBeGreaterThanOrEqual(2); + }); +}); diff --git a/apps/server/src/routes/slack/handler.ts b/apps/server/src/routes/slack/handler.ts index f04045da..57947890 100644 --- a/apps/server/src/routes/slack/handler.ts +++ b/apps/server/src/routes/slack/handler.ts @@ -22,7 +22,7 @@ import { recallContext, rememberContext, } from "./channel-context"; -import { findByThread, replace, store } from "./confirmation-store"; +import { draftKey, findByThread, replace, store } from "./confirmation-store"; import type { PendingPayload } from "./confirmation-store"; import { publishHomeView } from "./home"; import { @@ -30,7 +30,11 @@ import { getPageDashboardLink, getStatusReportLink, } from "./page-urls"; -import { getRegistryTool, isSlackToolDraft } from "./registry-runner"; +import { + getRegistryTool, + isSlackToolDraft, + type SlackToolDraft, +} from "./registry-runner"; import { abortTurn, endTurn, startTurn } from "./running-turns"; import { buildThreadTitle, @@ -560,32 +564,41 @@ async function processEvent(body: SlackEvent) { hitStepLimit: result.hitStepLimit, }); - // One pending action per thread (see findByThread/replace below). - // If the model emits multiple destructive drafts in a single step we - // only honour the first; the carrier's thread index can't represent - // a queue, and forcing the user to confirm twice in a row is worse - // UX than asking them to re-issue the second request. - const confirmationResult = result.toolResults.find((tr) => - isSlackToolDraft(tr.result), - ); - - if (confirmationResult) { - logger.info("slack confirmation requested", { - teamId, - channel: event.channel, - threadTs, - toolName: confirmationResult.toolName, + // One card per draft. A turn can draft several changes (rename a report + // *and* post an update to it); dropping all but the first would leave the + // user with nothing to click for the rest. When the model drafts the same + // thing twice, its last draft is the one it meant. + const drafts = new Map(); + for (const tr of result.toolResults) { + if (!isSlackToolDraft(tr.result)) continue; + const key = draftKey({ + toolName: tr.result.toolName, + input: tr.result.input, }); - await handleConfirmation( - slack, - reply, - event.channel, - threadTs, - event.user ?? "", - resolved.workspace.id, - teamId, - confirmationResult, - ); + drafts.delete(key); + drafts.set(key, tr.result); + } + const firstDraft = drafts.values().next().value; + + if (firstDraft) { + for (const draft of drafts.values()) { + logger.info("slack confirmation requested", { + teamId, + channel: event.channel, + threadTs, + toolName: draft.toolName, + }); + await handleConfirmation( + slack, + reply, + event.channel, + threadTs, + event.user ?? "", + resolved.workspace.id, + teamId, + draft, + ); + } } else { // No draft means no card. Distinguish a legitimate text answer from the // model drafting a change in prose and asking for permission instead of @@ -620,10 +633,7 @@ async function processEvent(body: SlackEvent) { teamId, workspaceId: resolved.workspace.id, isAgentThread, - draft: - confirmationResult && isSlackToolDraft(confirmationResult.result) - ? confirmationResult.result - : undefined, + draft: firstDraft, userText: event.text, }); } catch (err) { @@ -1132,7 +1142,13 @@ async function acknowledgeInChannel( } const ts = thinkingTs; + const postNew = postInThread(slack, channel, threadTs); + let placeholderUsed = false; + // The first message takes over "Thinking..."; any after it (a second + // confirmation card) is a message of its own, or it would overwrite the first. const send: Reply["send"] = async ({ text, blocks }) => { + if (placeholderUsed) return postNew({ text, blocks }); + placeholderUsed = true; await slack.chat.update({ channel, ts, text, blocks }); return ts; }; @@ -1155,10 +1171,8 @@ async function handleConfirmation( userId: string, workspaceId: number, teamId: string, - confirmationResult: { toolName: string; result: unknown }, + draft: SlackToolDraft, ) { - if (!isSlackToolDraft(confirmationResult.result)) return; - const draft = confirmationResult.result; const tool = getRegistryTool(draft.toolName); if (!tool) { logger.error("slack: registry tool not found", { @@ -1180,7 +1194,7 @@ async function handleConfirmation( // file suppressing duplicate event_ids, plus Slack's own per-thread // event throttling. Cross-process dedup is *not* covered; see note in // processedEvents. - const existing = await findByThread(threadTs); + const existing = await findByThread(threadTs, payload); if (existing) { await replace(existing.id, payload); diff --git a/apps/server/src/routes/slack/system-prompt.test.ts b/apps/server/src/routes/slack/system-prompt.test.ts index 74f520cd..246836a5 100644 --- a/apps/server/src/routes/slack/system-prompt.test.ts +++ b/apps/server/src/routes/slack/system-prompt.test.ts @@ -34,6 +34,8 @@ describe("buildSystemPrompt", () => { expect(prompt).toContain("shall I go ahead?"); // notify is a button, so the model must not spend a turn asking. expect(prompt).toContain("NEVER ask whether to notify subscribers"); + // The card lands after the answer; "card's up 👆" points at nothing. + expect(prompt).toContain("Cards are posted BELOW your message"); }); test("guides the model on componentImpacts", () => { diff --git a/apps/server/src/routes/slack/system-prompt.ts b/apps/server/src/routes/slack/system-prompt.ts index 80085649..3fd272e0 100644 --- a/apps/server/src/routes/slack/system-prompt.ts +++ b/apps/server/src/routes/slack/system-prompt.ts @@ -24,6 +24,7 @@ Calling a write tool (create_status_report, add_status_report_update, update_sta - NEVER write the draft out as message text (a "**Title:** … **Message:** …" block) instead of calling the tool. - NEVER end your turn with "shall I go ahead?", "want me to publish this?", or any other request for permission to call a write tool. The buttons already ask that question; a prose question leaves the user with nothing to click. - NEVER ask whether to notify subscribers. That choice is a button on the card, not yours. +- Cards are posted BELOW your message, one per write call, after you finish writing. Refer to them as below ("card below", 👇), never above or "up". - Only ask a question in text when you genuinely cannot build the call: an ambiguous status page, an unclear component impact, a missing date. Ask that, get the answer, then call the tool. IMPORTANT: You have NO knowledge of this workspace's data. NEVER guess or make up IDs (page, component, report, maintenance, monitor, notification, response log, audit log IDs). You MUST call the appropriate tool first to get real data.