diff --git a/apps/server/src/libs/test/doubles/slack-agent.mock.ts b/apps/server/src/libs/test/doubles/slack-agent.mock.ts index ef4990b40..c6d5c2fb9 100644 --- a/apps/server/src/libs/test/doubles/slack-agent.mock.ts +++ b/apps/server/src/libs/test/doubles/slack-agent.mock.ts @@ -4,5 +4,11 @@ import { slackTestState } from "./slack-test-state.ts"; export const runAgent = () => { if (slackTestState.runAgentOverride) return slackTestState.runAgentOverride(); - return Promise.resolve({ text: "Here is my response", toolResults: [] }); + return Promise.resolve({ + text: "Here is my response", + toolResults: [], + finishReason: "stop", + stepCount: 1, + hitStepLimit: false, + }); }; diff --git a/apps/server/src/routes/slack/agent.ts b/apps/server/src/routes/slack/agent.ts index 36771a745..00678affd 100644 --- a/apps/server/src/routes/slack/agent.ts +++ b/apps/server/src/routes/slack/agent.ts @@ -22,9 +22,17 @@ interface SlackThreadMessage { text?: string; } +// A mention needs list_status_pages + list_page_components before it can draft, +// and the model is told to check monitor status first — 5 left no headroom. +const MAX_STEPS = 10; + interface AgentResult { text: string; toolResults: Array<{ toolName: string; result: unknown }>; + finishReason: string; + stepCount: number; + /** True when the model used every allowed step — a draft may have been cut off. */ + hitStepLimit: boolean; } function convertThreadToMessages( @@ -75,6 +83,9 @@ export async function runAgent( return { text: "I couldn't read your message. Please try again.", toolResults: [], + finishReason: "unknown", + stepCount: 0, + hitStepLimit: false, }; } @@ -83,7 +94,7 @@ export async function runAgent( system: buildSystemPrompt(workspace.name ?? "Unknown"), messages, tools, - stopWhen: stepCountIs(5), + stopWhen: stepCountIs(MAX_STEPS), }); const toolResults: AgentResult["toolResults"] = []; @@ -93,5 +104,11 @@ export async function runAgent( } } - return { text: result.text, toolResults }; + return { + text: result.text, + toolResults, + finishReason: result.finishReason, + stepCount: result.steps.length, + hitStepLimit: result.steps.length >= MAX_STEPS, + }; } diff --git a/apps/server/src/routes/slack/handler.test.ts b/apps/server/src/routes/slack/handler.test.ts index 5c6d46359..fe3489c06 100644 --- a/apps/server/src/routes/slack/handler.test.ts +++ b/apps/server/src/routes/slack/handler.test.ts @@ -12,7 +12,7 @@ import { } from "@/libs/test/slack-config"; import type { SlackEnv } from "./config"; -import { handleSlackEvent } from "./handler"; +import { handleSlackEvent, looksLikeUncardedDraft } from "./handler"; import { verifySlackSignature } from "./verify"; function createTestApp() { @@ -562,3 +562,49 @@ describe("handleSlackEvent", () => { // No unhandled rejection — the .catch() in the error handler swallows it }); }); + +describe("looksLikeUncardedDraft", () => { + test("flags a prose draft that ends by asking permission", () => { + expect( + looksLikeUncardedDraft( + `There's one status page ("Inter.link"). Here's the draft incident report I'll create: + +**Title:** Traffic Forwarding Issue – POP FRA1-DE +**Status:** Investigating +**Message:** We are currently investigating a traffic forwarding issue. + +Shall I go ahead and publish this, or would you like to adjust anything?`, + ), + ).toBe(true); + }); + + test("flags the other permission phrasings", () => { + const draft = "*Title:* API outage\n*Message:* We're on it.\n"; + expect(looksLikeUncardedDraft(`${draft}Want me to create this?`)).toBe( + true, + ); + expect( + looksLikeUncardedDraft(`${draft}Would you like me to post it now?`), + ).toBe(true); + }); + + test("ignores a plain answer that happens to end in a question", () => { + expect( + looksLikeUncardedDraft( + "You have no active incidents. Want me to create one?", + ), + ).toBe(false); + }); + + test("ignores a listing with field labels but no permission question", () => { + expect( + looksLikeUncardedDraft( + "*Title:* API outage\n*Status:* investigating\n*Message:* We're on it.", + ), + ).toBe(false); + }); + + test("ignores empty text", () => { + expect(looksLikeUncardedDraft("")).toBe(false); + }); +}); diff --git a/apps/server/src/routes/slack/handler.ts b/apps/server/src/routes/slack/handler.ts index 8d6a54da9..4420e5841 100644 --- a/apps/server/src/routes/slack/handler.ts +++ b/apps/server/src/routes/slack/handler.ts @@ -83,6 +83,25 @@ function isSlackPlatformError(err: unknown, errorCode: string): boolean { return parsed.success && parsed.data.data.error === errorCode; } +// Asking permission to run a write tool ("shall I go ahead and publish this?"). +const PERMISSION_QUESTION = + /\b(shall i|should i|do you want me to|would you like me to|want me to|ready for me to|can i)\b[^?]*\b(go ahead|proceed|publish|create|post|send|schedule|submit|add|resolve|update)\b/i; + +// A change written out as message text rather than passed to a tool. +const PROSE_DRAFT_FIELDS = + /\*{0,2}(title|status|message|impact|components?|from|to)\*{0,2}\s*:/gi; + +/** + * Heuristic for the "model drafted in prose instead of calling the tool" failure + * — logging only, so false positives are cheap. Both signals are required: a + * plain answer can end in a question, and a list of reports can look tabular. + */ +export function looksLikeUncardedDraft(text: string): boolean { + if (!text) return false; + if (!PERMISSION_QUESTION.test(text)) return false; + return (text.match(PROSE_DRAFT_FIELDS) ?? []).length >= 2; +} + export async function handleSlackEvent(c: Context) { const body = c.get("slackBody") as SlackEvent; @@ -281,6 +300,9 @@ async function processEvent(body: SlackEvent) { channel: event.channel, threadTs, toolCalls: result.toolResults.map((tr) => tr.toolName), + finishReason: result.finishReason, + stepCount: result.stepCount, + hitStepLimit: result.hitStepLimit, }); // One pending action per thread (see findByThread/replace below). @@ -310,6 +332,20 @@ async function processEvent(body: SlackEvent) { confirmationResult, ); } 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 + // calling the tool — the latter strands the user with nothing to click. + if (looksLikeUncardedDraft(result.text)) { + logger.warn("slack draft proposed without a card", { + teamId, + channel: event.channel, + threadTs, + finishReason: result.finishReason, + stepCount: result.stepCount, + hitStepLimit: result.hitStepLimit, + readToolCalls: result.toolResults.map((tr) => tr.toolName), + }); + } await slack.chat.update({ channel: event.channel, ts: thinkingTs, diff --git a/apps/server/src/routes/slack/registry-runner.test.ts b/apps/server/src/routes/slack/registry-runner.test.ts index a5dad44e3..a0523800f 100644 --- a/apps/server/src/routes/slack/registry-runner.test.ts +++ b/apps/server/src/routes/slack/registry-runner.test.ts @@ -9,6 +9,7 @@ import { buildSlackTools, buildTool, deriveDraftSchema, + draftToolDescription, executeRegistryAction, getRegistryTool, isSlackToolDraft, @@ -148,6 +149,34 @@ describe("buildSlackTools", () => { expect(deriveDraftSchema(t)).toBe(t.inputSchema); }); + test("destructive tools are described as drafting, not executing", () => { + for (const name of Object.keys(agentTools)) { + const t = agentTools[name as keyof typeof agentTools] as AnyAgentTool; + if (!t.destructive) continue; + const description = tools[name].description ?? ""; + expect(description, name).toContain("DOES NOT EXECUTE"); + expect(description, name).toContain("Approve"); + // The registry's own wording must survive — it carries the id-resolution + // rules the model needs to build a valid call. + expect(description, name).toContain(t.description); + } + }); + + test("read tools keep the registry description verbatim", () => { + expect(tools.list_status_pages.description).toBe( + agentTools.list_status_pages.description, + ); + }); + + test("draftToolDescription names extraFlags so the model won't ask about them", () => { + const withFlag = draftToolDescription(agentTools.create_status_report); + expect(withFlag).toContain("`notify`"); + expect(withFlag).toContain("never ask about it"); + // update_status_report has no notify path, so no flag sentence. + const withoutFlag = draftToolDescription(agentTools.update_status_report); + expect(withoutFlag).not.toContain("never ask about it"); + }); + test("read tools expose the full registry schema", () => { const t = tools.list_status_pages; // list_status_pages takes an empty object; the schema is unchanged. diff --git a/apps/server/src/routes/slack/registry-runner.ts b/apps/server/src/routes/slack/registry-runner.ts index cf5c64aec..8b591b169 100644 --- a/apps/server/src/routes/slack/registry-runner.ts +++ b/apps/server/src/routes/slack/registry-runner.ts @@ -66,7 +66,7 @@ export function buildTool(t: AnyAgentTool, ctx: ServiceContext): Tool { // re-validates after applyFlags on button click. const draftSchema = deriveDraftSchema(t); return tool({ - description: t.description, + description: draftToolDescription(t), inputSchema: draftSchema, execute: async (input: unknown) => { const parsed = draftSchema.parse(input); @@ -84,6 +84,22 @@ export function buildTool(t: AnyAgentTool, ctx: ServiceContext): Tool { }); } +/** + * Registry descriptions document execute-time semantics ("PUBLIC … irreversible + * side effects"), which don't hold here — destructive calls only produce a draft + * for the Block Kit card. Left unqualified, the model reads the warning and asks + * for permission in prose instead of calling, and the user never gets a card. + */ +export function draftToolDescription(t: AnyAgentTool): string { + const flags = t.approval?.extraFlags ?? []; + const flagNote = flags.length + ? ` The card also carries the user's ${flags.map((f) => `\`${f.id}\``).join(", ")} choice as a button, so never ask about it.` + : ""; + return `${t.description} + +IN SLACK THIS CALL DOES NOT EXECUTE. It renders an approval card with Approve/Cancel buttons; nothing is created, published, or notified until the user clicks Approve.${flagNote} Call this tool as soon as you have the required ids — do NOT describe the draft in a message and ask for permission first, because that leaves the user with no card to click.`; +} + /** * Return a schema that omits every extraFlag field declared on the tool's * approval metadata. Used only for the Slack LLM-facing schema; the diff --git a/apps/server/src/routes/slack/system-prompt.test.ts b/apps/server/src/routes/slack/system-prompt.test.ts index 42835ee74..6d431cd8d 100644 --- a/apps/server/src/routes/slack/system-prompt.test.ts +++ b/apps/server/src/routes/slack/system-prompt.test.ts @@ -25,6 +25,17 @@ describe("buildSystemPrompt", () => { expect(prompt).toContain('workspace "Acme Corp"'); }); + test("tells the model a write call drafts a card instead of executing", () => { + const prompt = buildSystemPrompt("Acme Corp"); + expect(prompt).toContain("does NOT execute"); + expect(prompt).toContain("Approve/Cancel"); + // The two ways the model strands the user without a card. + expect(prompt).toContain("NEVER write the draft out as message text"); + 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"); + }); + test("guides the model on componentImpacts", () => { const prompt = buildSystemPrompt("Acme Corp"); expect(prompt).toContain("componentImpacts"); diff --git a/apps/server/src/routes/slack/system-prompt.ts b/apps/server/src/routes/slack/system-prompt.ts index 9d61da5ba..8f32655bb 100644 --- a/apps/server/src/routes/slack/system-prompt.ts +++ b/apps/server/src/routes/slack/system-prompt.ts @@ -12,6 +12,14 @@ export function buildSystemPrompt(workspaceName: string): string { The current date and time is: ${now} (UTC). You help teams create and manage status reports and maintenance windows through Slack. +HOW APPROVAL WORKS HERE — read this before any write tool: +Calling a write tool (create_status_report, add_status_report_update, update_status_report, resolve_status_report, create_maintenance) does NOT execute it. It renders an approval card in Slack with Approve/Cancel buttons, and nothing is created, published, or notified until the user clicks Approve. The card IS how you ask. +- Call the tool as soon as you have the ids it needs. That is the ONLY way the user gets a card. +- 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. +- 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 IDs, component IDs, report IDs). You MUST call the appropriate tool first to get real data. - Questions about pages or components -> call list_status_pages FIRST - Questions about reports -> call list_status_reports FIRST @@ -54,7 +62,7 @@ Guidelines: - When tagged in a thread, synthesize the full thread into a status report draft. - Status progression: investigating -> identified -> monitoring -> resolved - Be concise. Use Slack mrkdwn formatting (*bold*, _italic_). -- For any mutation, always call the tool so the user sees a confirmation. +- Every mutation goes through a tool call — see "HOW APPROVAL WORKS HERE" above. A drafted change you did not call a tool for is a change the user cannot approve. Maintenance scheduling: - Parse natural language dates into ISO 8601 format. Convert relative dates like "next Friday from 2-3 PM" into proper ISO 8601 timestamps.