diff --git a/.gitignore b/.gitignore index c75aa7ea65..8f3a45a4df 100644 --- a/.gitignore +++ b/.gitignore @@ -43,3 +43,4 @@ coverage.* # Personal, local-only Claude Code instructions and running-state notes /CLAUDE.local.md /HANDOFF.md +.env diff --git a/sdk/typescript/examples/slack-bot/DESIGN.md b/sdk/typescript/examples/slack-bot/DESIGN.md index b1dc95d4db..252983152c 100644 --- a/sdk/typescript/examples/slack-bot/DESIGN.md +++ b/sdk/typescript/examples/slack-bot/DESIGN.md @@ -103,13 +103,15 @@ The full plan above is the target shape; v1 ships only the baseline slice answer... streaming and steer-while-running are stretch goals, not requirements") — **revised to the `agent_view` model above**: -- **DM.** Raw `app.event("app_home_opened", ...)`, filtered to - `tab === "messages"`: greets a channel once (dedup'd in-memory by - channel id). Raw `app.message(...)`, filtered to `channel_type === "im"`: +- **DM.** Raw `app.message(...)`, filtered to `channel_type === "im"`: derives the canonical root as `message.thread_ts ?? message.ts`, runs the prompt, and streams the reply into that thread. A top-level message starts a fresh mecatl session; a reply continues the session for its existing - Slack thread. + Slack thread. (An earlier revision also sent a one-time greeting on + `app_home_opened` — dropped as unnecessary noise; the `app_home_opened` + bot-event subscription stays in `slack-app-manifest.json` regardless, + since Slack's own manifest validator requires it for an `agent_view` app + even with no handler.) - **Channel.** Raw `app.event("app_mention", ...)`: starts (or continues) a session keyed by `channel:thread_ts` (a top-level mention's own `ts` becomes the thread root — Slack only sets `thread_ts` on replies). Raw @@ -151,8 +153,6 @@ fallback against a real channel before trusting this fully. Not yet implemented — additive later, not a rewrite: -- `suspended` status + Block Kit approve/deny UI for manual permission - review. - Per-run token/spend budget — the TypeScript SDK (M1) doesn't yet expose a per-call token limit; see the `TODO` in `src/bridge.ts`. `SLACK_RATE_LIMIT_MAX` (below) is a request-count mitigation, not a spend budget. @@ -215,6 +215,64 @@ an identity provider at all (guests never go through one). Replaced with: `mecated` + the bot together for a no-toolchain-needed demo. See README.md. +## Manual permission approval (#1397) + +The manual-approval experiment this doc's earlier sections flagged as +possibly-never-shipping did ship — `src/approvals.ts`, replacing +`bridge.ts`'s hardcoded `onPermissionAsk: () => "allow_once"` entirely +(not toggleable back; #1397 asked for the auto-approve to go away, not to +become optional). + +**Delivered as a DM, not in-thread Block Kit buttons** — a deliberate +deviation from the "The plan" section's original sketch above. That sketch +assumed an in-thread ephemeral message (`chat.postEphemeral`) would work +for both halves of the requirement: never actionable by the whole channel, +and clearable when the run ends. It satisfies the first (ephemeral is +already user-scoped) but not the second — Slack has no way to update an +ephemeral message except via the `response_url` handed to an actual click, +so there's no way to proactively clear it when a run ends/cancels with +nobody having clicked anything. A regular DM message is equally +user-scoped (only the bot and that one person are in it) but is a real +message, so `chat.update` works on it from any code path, at any time — +which is what "clear the pending Slack UI when the run terminates or is +canceled" actually requires. + +**Resolution goes through the SDK's `onPermissionAsk` responder Promise, +never `run.resolveAsk` called from Slack code directly.** The SDK already +invokes one `onPermissionAsk(ask, signal)` per ask, in the background, +independent of whatever the caller does with the run's own event stream +(`RunImpl#startPermissionResponder` in the SDK). `PermissionApprovalGateway` +returns a `Promise` from that call and resolves it from the Slack button +click; the SDK does the rest, including safely ignoring a resolution that +arrives after the ask's own `AbortSignal` already fired. This meant no code +here ever needs to hold a `Run` reference, and multiple concurrent asks on +one run are handled for free (each gets its own responder invocation and +its own signal) — no manual `permission.ask`/`permission.retract` watching +needed in `bridge.ts`'s `for await` loop, which would otherwise stall on a +second ask while blocked awaiting Slack for the first. + +**Correlation is one flat `Map`** — `askId` is a +server-minted, session-scoped id, so it's already globally unique; no +per-run or per-thread indexing needed on top. A stale click, a duplicate +click, and a post-terminal click are all rejected the same way: the map +entry is deleted synchronously, before any `await`, the first time +anything consumes it (a click or the ask's own abort), so anything arriving +after that finds nothing pending. + +**"Allow always" is included** — the SDK's `resolveAsk(askId, "allow_always")` +threads straight through to the server's real `Policy.Learn` semantics, so +the completion criteria's "include it only if the SDK can preserve +existing authority semantics" bar is met without extra plumbing. + +**Fails closed.** If DMing the approver throws (missing `im:write` scope, +transient Slack API error), the ask resolves as `deny` rather than hanging +the run indefinitely or silently allowing. + +**Out of scope, deliberately:** a timeout on an unanswered ask (not in the +issue's completion criteria — it just waits, same as any other blocked +consumer), and plan-approval asks (`PresentPlan` goes through the SDK's +separate `onPlanApproval` hook, which this bot has never configured). + ## Costs, stated honestly - Building on Slack's Agent Sessions API is a bet on a genuinely @@ -234,9 +292,10 @@ an identity provider at all (guests never go through one). Replaced with: section — the docs alone got the DM path wrong once already. Don't advertise channel support as working until it's actually been tested live. -- The manual-approval experiment may simply not ship — unchanged from the - original plan. Native streaming and stop-button wiring did ship, but - streaming's channel behavior is unverified live — see the note above. +- Native streaming and stop-button wiring shipped, but streaming's channel + behavior is unverified live — see the note above. The manual-approval + flow (#1397, see the section above) is likewise built and offline-tested + but not yet confirmed against a real workspace. ## See also diff --git a/sdk/typescript/examples/slack-bot/README.md b/sdk/typescript/examples/slack-bot/README.md index f97dc483b7..9e7931832a 100644 --- a/sdk/typescript/examples/slack-bot/README.md +++ b/sdk/typescript/examples/slack-bot/README.md @@ -51,7 +51,14 @@ each setting instead of pasting the manifest): `assistant:write` if not already present) — `groups:history` covers private channels. Also add `users:read` **and** `users:read.email` together — Slack requires both to return the `email` field from - `users.info`, which the access-control check below depends on. + `users.info`, which the access-control check below depends on. Add + `im:write` too — the manual-approval flow (see "Permission approvals" + below) opens a DM with `conversations.open` to deliver each ask. + + If you already installed this app before #1397 (manual permission + approval) shipped, add `im:write` to the manifest and **reinstall the + app to your workspace** — Slack doesn't retroactively grant a new scope + to an existing install. 5. **Features → Event Subscriptions → Subscribe to bot events**: add `app_home_opened`, `message.im`, `app_mention`, `message.channels`, `message.groups`, `agent_session_stopped` (Slack's native stop button — @@ -131,17 +138,18 @@ command-execution access — see below. ## Security -Every permission ask mecatl raises is auto-approved -(`onPermissionAsk: () => "allow_once"` in `src/bridge.ts`) — there is no -human-in-the-loop step before the model runs a shell command or edits a -file on whatever host `mecated` runs on. That's a deliberate v1 choice -(#882: "every demo run executes under an auto-approved posture so it never -blocks on a human"), but it means **whoever can reach the bot can run -commands unsupervised** — and "reach the bot" is broader than "the person -who set it up": anyone who can DM it, or who shares a channel it's invited -into, qualifies. +Every permission ask mecatl raises is DM'd to the Slack user who started +that run — see "Permission approvals" below — so a tool call the model +wants to make (shell command, file edit, …) waits for that person's +explicit Allow/Deny before it runs. This closes the original v1 gap (#882: +"every demo run executes under an auto-approved posture") where **whoever +could reach the bot could run commands unsupervised** — and "reach the +bot" is broader than "the person who set it up": anyone who can DM it, or +who shares a channel it's invited into, qualifies. Access control (below) +still matters independently: it decides *who can start a run at all*, not +just who approves what it wants to do once started. -Three independent mitigations: +Three independent mitigations, on top of the approval flow itself: - **Access control** (`src/access.ts`) — every message resolves the sender's Slack identity via `users.info` before it reaches the bridge, @@ -190,10 +198,34 @@ Three independent mitigations: TypeScript SDK doesn't expose it as a per-call option yet (see the `TODO` in `src/bridge.ts`). -None of these mitigations touch the auto-approve design itself — that -trade-off stands as documented above and in `DESIGN.md`. They're -independent controls: *who* can reach the bot, versus *what* mecatl will -do once reached. +These are independent controls: *who* can reach the bot at all, versus +*what it's allowed to do once reached* (the approval flow above). + +## Permission approvals + +Every ordinary tool-permission ask mecatl raises is delivered as a DM +(never posted where a whole channel could see or click it) to the Slack +user who started that run — the run's Slack session status shows +`suspended` while it waits. The DM carries the tool name, why it's being +asked, and the requested arguments, with three buttons: + +- **Allow once** — approves just this call. +- **Allow always (this session)** — approves this call and every + matching one for the rest of the mecatl session (the server's normal + `allow_always` semantics — the SDK's `run.resolveAsk` passes this + through unchanged, so it isn't bot-specific behavior). +- **Deny** — rejects this call; the model sees the denial and can try a + different approach or explain why it couldn't proceed. + +Only the person who started the run can act on its buttons; anyone else +clicking (structurally shouldn't happen, since the card is DM'd to one +person) is ignored. If the run ends or the ask is otherwise retracted +before anyone responds, the DM updates itself to say so and the buttons +stop doing anything. There's no timeout — an unanswered ask just waits. + +Plan-approval asks (`PresentPlan`) are a separate SDK hook +(`onPlanApproval`) this bot doesn't configure, so they aren't covered by +this flow. ## 4. Verify it end to end @@ -221,8 +253,9 @@ the event even arrived before the bridge runs anything. Run with ## What v1 does and doesn't do -- Every permission ask mecatl raises is auto-approved — this bot is meant - for a trusted dev workspace, not unattended production use. +- Every permission ask mecatl raises is DM'd to the run's authorized user + for an explicit Allow once / Allow always / Deny decision — see + "Permission approvals" above. - DM and channel: one `mecated` session per Slack thread (`channel:thread_ts`). A top-level DM starts a new session; a channel thread starts when the bot is `@mention`ed. Replies in either thread @@ -240,7 +273,7 @@ the event even arrived before the bridge runs anything. Run with way the DM path was. See `DESIGN.md`. - Real token streaming (`chat.startStream`/`appendStream`/`stopStream`) and Slack's native stop button (`agent_session_stopped` → `run.cancel()`) are - both wired now. No approval UI yet. See `DESIGN.md` for why and what's next. + both wired. See `DESIGN.md` for why and what's next. - Built on raw Slack event handlers, not bolt-js's `Assistant` class — that class wraps a different, older Slack feature that never fires for this app's configuration. See `DESIGN.md` for the full story. diff --git a/sdk/typescript/examples/slack-bot/slack-app-manifest.json b/sdk/typescript/examples/slack-bot/slack-app-manifest.json index f1d7b06c5b..7f389c4fce 100644 --- a/sdk/typescript/examples/slack-bot/slack-app-manifest.json +++ b/sdk/typescript/examples/slack-bot/slack-app-manifest.json @@ -24,6 +24,7 @@ "chat:write", "assistant:write", "im:history", + "im:write", "app_mentions:read", "channels:history", "groups:history", diff --git a/sdk/typescript/examples/slack-bot/src/agentSessions.ts b/sdk/typescript/examples/slack-bot/src/agentSessions.ts index 254608964f..0db51a6cab 100644 --- a/sdk/typescript/examples/slack-bot/src/agentSessions.ts +++ b/sdk/typescript/examples/slack-bot/src/agentSessions.ts @@ -1,6 +1,7 @@ import type { App, SayFn } from "@slack/bolt"; import type { AccessResolver } from "./access.js"; +import { type PermissionApprovalGateway, registerPermissionApprovals } from "./approvals.js"; import { isStuckExternalAuthorization, type MecatlBridge } from "./bridge.js"; import type { BotConfig } from "./env.js"; import { SlidingWindowRateLimiter } from "./rateLimit.js"; @@ -9,7 +10,6 @@ const EXTERNAL_AUTH_MESSAGE = "This needs a connector to be authorized by an administrator before it can be used here."; const FAILURE_MESSAGE = "Something went wrong running that against mecatl. Check the bot's logs for details."; -const GREETING = "Tag me with a prompt and I'll run it against mecatl."; const NOT_AUTHORIZED_MESSAGE = "You're not authorized to use this bot. Ask the operator to grant you access."; const RATE_LIMITED_MESSAGE = "Rate limit exceeded — try again in a bit."; @@ -52,9 +52,10 @@ const MENTION_PREFIX = /^<@[^>]+>\s*/; * back to a single final `say()` with the run's full text, exactly as * DESIGN.md's original plan describes, rather than failing the whole prompt. * - * TODO(#883 follow-up, tracked in DESIGN.md "Not yet implemented"): - * - `suspended` status + Block Kit approve/deny UI for manual permission - * review. + * Permission asks (issue #1397, see approvals.ts) DM the requesting user a + * Block Kit approve/deny card instead of auto-approving; `runPrompt` reports + * the pending/resolved state via the same `agents.sessions.setStatus` + * (`suspended` while at least one ask is pending) used for `processing`. */ export function registerAgentSessions( app: App, @@ -62,17 +63,10 @@ export function registerAgentSessions( config: BotConfig, resolver: AccessResolver, ): void { - const greetedDm = new Set(); + const approvals = registerPermissionApprovals(app); const activeChannelThreads = new Set(); const rateLimiter = new SlidingWindowRateLimiter(config.rateLimit.max, config.rateLimit.windowMs); - app.event("app_home_opened", async ({ event, say }) => { - if (event.tab !== "messages") return; - if (greetedDm.has(event.channel)) return; - greetedDm.add(event.channel); - await say(GREETING); - }); - // Slack's native stop button on an Agent View session. @slack/types defines the // shape (AgentSessionStoppedEvent) but doesn't wire it into bolt's own event // union, hence the manual cast — same "hand-rolled, not yet wrapped by bolt-js" @@ -122,6 +116,8 @@ export function registerAgentSessions( context.teamId, say, notify, + approvals, + "channel", ); }); @@ -161,6 +157,8 @@ export function registerAgentSessions( context.teamId, say, notify, + approvals, + "dm", ); return; } @@ -190,6 +188,8 @@ export function registerAgentSessions( context.teamId, say, notify, + approvals, + "channel", ); }); } @@ -237,8 +237,9 @@ async function runPrompt( recipientTeamId: string | undefined, say: SayFn, notifyError: Notifier, + approvals: PermissionApprovalGateway, + origin: "dm" | "channel", ): Promise { - await setSessionStatus(app, channelId, statusThreadTs, "processing"); // Agent Session streaming is thread-scoped, so this uses the same canonical // root as status, fallback replies, cancellation, and the mecatl session key. const stream = new SlackTextStream( @@ -248,8 +249,25 @@ async function runPrompt( recipientUserId, recipientTeamId, ); + const onPermissionAsk = approvals.createResponder({ + authorizedUserId: recipientUserId, + originLabel: origin === "dm" ? "a DM with the bot" : `<#${channelId}>`, + setStatus: (status) => setSessionStatus(app, channelId, statusThreadTs, status), + }); try { - const outcome = await bridge.handlePrompt(threadKey, text, (delta) => stream.append(delta)); + const outcome = await bridge.handlePrompt( + threadKey, + text, + (delta) => stream.append(delta), + onPermissionAsk, + // Fired from INSIDE the bridge's per-thread queue, once this call's own + // run actually starts/settles — not eagerly here, where a second + // same-thread message could otherwise overwrite a still-pending run's + // `suspended` status with `processing` before its own turn arrives + // (panel-review, samuv). + () => setSessionStatus(app, channelId, statusThreadTs, "processing"), + () => setSessionStatus(app, channelId, statusThreadTs, "active"), + ); if (stream.started) await stream.stop(); // `!stream.started` (never streamed at all) and `stream.failed` (streamed // partially, then an append broke mid-run — #1289 review, samuv: without @@ -274,8 +292,6 @@ async function runPrompt( await notifyError( isStuckExternalAuthorization(error) ? EXTERNAL_AUTH_MESSAGE : FAILURE_MESSAGE, ); - } finally { - await setSessionStatus(app, channelId, statusThreadTs, "active"); } } diff --git a/sdk/typescript/examples/slack-bot/src/approvals.ts b/sdk/typescript/examples/slack-bot/src/approvals.ts new file mode 100644 index 0000000000..783cab2cab --- /dev/null +++ b/sdk/typescript/examples/slack-bot/src/approvals.ts @@ -0,0 +1,428 @@ +import type { App } from "@slack/bolt"; +import type { + PermissionAskEventPayload, + PermissionAskResponder, + PermissionVerdict, +} from "@stacklok-oss/mecatl-sdk"; + +const ACTION_PREFIX = "mecatl_approval:"; +const MAX_ARGS_PREVIEW = 800; + +/** The minimal shape this gateway needs from `app.client` — deliberately not the real + * `@slack/web-api` `WebClient` type, same rationale as `access.ts`'s `UserLookupClient`: that + * package is only a transitive dep via `@slack/bolt`, not declared in this package's own + * `package.json`, and a narrow interface is trivial for tests to fake without any Slack SDK + * types at all. */ +export interface ApprovalWebClient { + conversations: { + open(args: { users: string }): Promise<{ channel?: { id?: string } }>; + }; + chat: { + postMessage(args: { + channel: string; + text: string; + blocks?: unknown[]; + }): Promise<{ ts?: string; channel?: string }>; + update(args: { + channel: string; + ts: string; + text: string; + blocks?: unknown[]; + }): Promise; + }; +} + +/** The Slack-facing surface `PermissionApprovalGateway` actually needs — narrower than + * {@link ApprovalWebClient}, and the seam tests fake directly (see `test/approvals.test.ts`), + * so gateway logic never has to know about `conversations.open`/`chat.*` argument shapes. */ +export interface ApprovalMessagingClient { + /** Resolves to the id of a 1:1 DM channel with `userId`, opening one if needed. */ + openDm(userId: string): Promise; + /** Posts a new message, returning its `ts` for a later `updateMessage`. */ + postMessage(channel: string, text: string, blocks: unknown[]): Promise; + /** Replaces a previously-posted message's content in place (e.g. to remove its buttons). */ + updateMessage(channel: string, ts: string, text: string, blocks?: unknown[]): Promise; +} + +/** Adapts a real Bolt `app.client` into {@link ApprovalMessagingClient}, caching each user's DM + * channel id in memory for the process's lifetime — same in-memory-only precedent as + * `MecatlBridge`'s session cache and `agentSessions.ts`'s thread-tracking sets; a bot restart + * just reopens the DM next time, which is harmless (Slack returns the same channel either way). */ +export function slackApprovalMessaging(client: ApprovalWebClient): ApprovalMessagingClient { + const dmChannels = new Map(); + return { + async openDm(userId) { + const cached = dmChannels.get(userId); + if (cached !== undefined) return cached; + const response = await client.conversations.open({ users: userId }); + const channel = response.channel?.id; + if (channel === undefined) { + throw new Error(`conversations.open returned no channel id for ${userId}`); + } + dmChannels.set(userId, channel); + return channel; + }, + async postMessage(channel, text, blocks) { + const response = await client.chat.postMessage({ blocks, channel, text }); + const ts = response.ts; + if (ts === undefined) throw new Error("chat.postMessage did not return a message ts"); + return ts; + }, + async updateMessage(channel, ts, text, blocks) { + await client.chat.update({ channel, text, ts, ...(blocks === undefined ? {} : { blocks }) }); + }, + }; +} + +interface PendingApproval { + readonly resolve: (verdict: PermissionVerdict | undefined) => void; + readonly authorizedUserId: string; + readonly dmChannel: string; + readonly ts: string; + readonly onSettled: () => void; + readonly tool: string; + readonly originLabel: string; +} + +/** Context `agentSessions.ts` supplies per run so the gateway can address the right person and + * reflect the ask back onto that thread's Slack session status. */ +export interface ApprovalContext { + /** The Slack user id who initiated this run — the only person the ask is delivered to and + * the only person whose click is honored. */ + authorizedUserId: string; + /** A short human label for where this run came from (e.g. "your DM" or a channel mention), + * shown on the approval card so it's clear which conversation the request belongs to. */ + originLabel: string; + /** Best-effort: reflects the run's suspended/resumed state via `agents.sessions.setStatus`. + * Called at most once per transition — never twice in a row with the same status. */ + setStatus: (status: "suspended" | "processing") => Promise; +} + +function verdictFromActionId(actionId: string): PermissionVerdict | undefined { + if (!actionId.startsWith(ACTION_PREFIX)) return undefined; + const suffix = actionId.slice(ACTION_PREFIX.length); + if (suffix === "allow_once" || suffix === "allow_always" || suffix === "deny") return suffix; + return undefined; +} + +function verdictLabel(verdict: PermissionVerdict): string { + switch (verdict) { + case "allow_once": + return "Allowed once"; + case "allow_always": + return "Allowed for the rest of this session"; + case "deny": + return "Denied"; + default: + return verdict; + } +} + +/** Truncates to `max` characters INCLUDING the ellipsis (panel-review, samuv: appending "…" + * after a full `max`-length slice produced `max + 1` characters, which could push a card + * title/subtitle one character past Slack's exact 150-char limit and get the whole ask + * rejected — denying it, via the fail-closed catch below, for a reason that had nothing to do + * with the ask itself). */ +function clamp(value: string, max: number): string { + return value.length > max ? `${value.slice(0, max - 1)}…` : value; +} + +function isPlainRecord(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +/** A leaf (string/boolean/null — `valueCell` handles `number` separately, as `raw_number`) + * renders as itself; anything else (a nested object or array) renders as compact inline JSON — + * the "further JSON" case. */ +function formatArgValue(value: unknown): string { + if (value === null || typeof value !== "object") return String(value); + return JSON.stringify(value); +} + +const CARD_TITLE_MAX = 150; +const CARD_SUBTITLE_MAX = 150; +const MAX_ARGS_ROWS = 10; +const MAX_ARGS_CELL_VALUE = 300; + +function rawTextCell(text: string): unknown { + return { text, type: "raw_text" }; +} + +/** A `rich_text` table cell wrapping one styled text run — structural, not markdown, so styling + * a model-influenced string (a key or an arg value) this way can't be hijacked into forging + * different text or breaking out of the cell the way an unescaped mrkdwn string could. */ +function richTextCell(text: string, style: Record): unknown { + return { + elements: [{ elements: [{ style, text, type: "text" }], type: "rich_text_section" }], + type: "rich_text", + }; +} + +function keyCell(key: string): unknown { + return richTextCell(key, { bold: true }); +} + +/** A genuine JSON number gets `raw_number` (Slack sorts/aligns it numerically); everything else + * (including the "further JSON" nested-object/array case) renders as a code-styled text run. */ +function valueCell(value: unknown): unknown { + if (typeof value === "number") return { text: String(value), type: "raw_number" }; + return richTextCell(clamp(formatArgValue(value), MAX_ARGS_CELL_VALUE), { code: true }); +} + +/** Renders `ask.args` as a Block Kit `table` block, one row per key. Falls back to a single-cell + * raw dump for anything that isn't a flat JSON object (invalid JSON, or a top-level array/ + * primitive — nothing to tabulate). Returns `undefined` for empty/absent args. */ +function argsTableBlock(argsJson: string): unknown | undefined { + if (argsJson.length === 0) return undefined; + let parsed: unknown; + try { + parsed = JSON.parse(argsJson); + } catch { + parsed = undefined; + } + if (!isPlainRecord(parsed)) { + return { rows: [[rawTextCell(clamp(argsJson, MAX_ARGS_PREVIEW))]], type: "table" }; + } + const keys = Object.keys(parsed); + if (keys.length === 0) return undefined; + const cap = keys.length > MAX_ARGS_ROWS ? MAX_ARGS_ROWS - 1 : MAX_ARGS_ROWS; + const rows = keys.slice(0, cap).map((key) => [keyCell(key), valueCell(parsed[key])]); + const omitted = keys.length - cap; + if (omitted > 0) rows.push([rawTextCell("…"), rawTextCell(`and ${omitted} more`)]); + return { rows, type: "table" }; +} + +function approvalBlocks( + ask: PermissionAskEventPayload, + originLabel: string, + askId: string, +): unknown[] { + // Block Kit's `card` has no slot for a nested table — `body` is a plain 200-char string, not + // a block container — so the reason (secondary info) rides the small gray `subtitle`, and the + // args table (the important part) is a separate block placed ABOVE the card so it reads as + // the primary content, with the card (title + approve/deny buttons) anchoring it below. + const subtitleParts = [`Requested from ${originLabel}`]; + if (ask.reason.length > 0) subtitleParts.push(ask.reason); + const card = { + actions: [ + { + action_id: `${ACTION_PREFIX}allow_once`, + style: "primary", + text: { text: "Allow once", type: "plain_text" }, + type: "button", + value: askId, + }, + { + action_id: `${ACTION_PREFIX}allow_always`, + text: { text: "Allow always", type: "plain_text" }, + type: "button", + value: askId, + }, + { + action_id: `${ACTION_PREFIX}deny`, + style: "danger", + text: { text: "Deny", type: "plain_text" }, + type: "button", + value: askId, + }, + ], + subtitle: { text: clamp(subtitleParts.join(" · "), CARD_SUBTITLE_MAX), type: "plain_text" }, + title: { text: clamp(`Permission request: ${ask.tool}`, CARD_TITLE_MAX), type: "plain_text" }, + type: "card", + }; + const argsTable = argsTableBlock(ask.args); + return argsTable === undefined ? [card] : [argsTable, card]; +} + +/** Replaces the approval card once it's resolved (clicked or retracted) — a small `card` with + * just a headline, no actions, so the DM keeps its card look instead of degrading to plain + * text once the buttons are gone. */ +function outcomeCard(originLabel: string, headline: string): unknown { + return { + subtitle: { + text: clamp(`Requested from ${originLabel}`, CARD_SUBTITLE_MAX), + type: "plain_text", + }, + title: { text: clamp(headline, CARD_TITLE_MAX), type: "plain_text" }, + type: "card", + }; +} + +/** + * Drives Slack-side manual approval for ordinary `permission.ask` events (issue #1397): DMs the + * authorized user a Block Kit card and resolves the SDK's `onPermissionAsk` responder promise + * from their click, instead of the previous unconditional `() => "allow_once"`. + * + * DMs, not in-thread messages: the completion criteria require both that the ask never renders + * as an actionable control to a whole channel, AND that the UI clears when the run + * terminates/cancels — not only when clicked. An in-thread `chat.postEphemeral` message would + * satisfy the first but Slack gives no way to update an ephemeral message outside of a + * `response_url` from an actual click, so there's no way to satisfy the second for "nobody + * clicked, the run just ended." A regular DM message is just as user-scoped, but `chat.update` + * works on it any time, from any code path — including a run-end/cancel/retract with no click + * at all. + * + * Correlation is intentionally simple: `askId` is globally unique (it's a server-minted, + * session-scoped id), so one flat in-memory `Map` is enough — no + * separate per-run/per-thread indexing needed. A double-click, a stale click, and a + * post-terminal click are all handled by the same guard: the map entry is deleted + * synchronously (before any `await`) the first time it's consumed, whether by a click or by + * the ask's own `AbortSignal` firing, so anything arriving after that finds nothing pending. + */ +/** Same minimal shape as `access.ts`'s `MinimalLogger` — kept as its own type here so this + * module doesn't need to import from a sibling for one method signature. */ +export interface MinimalLogger { + warn(msg: string, ...meta: unknown[]): void; +} + +const noopLogger: MinimalLogger = { warn: () => {} }; + +export class PermissionApprovalGateway { + readonly #client: ApprovalMessagingClient; + readonly #logger: MinimalLogger; + readonly #pending = new Map(); + + constructor(client: ApprovalMessagingClient, logger: MinimalLogger = noopLogger) { + this.#client = client; + this.#logger = logger; + } + + /** Builds the `onPermissionAsk` responder for one run. Closes over a pending-ask counter so + * `setStatus` only fires at the 0→1 ("suspended") and 1→0 ("processing") boundaries — a + * second concurrent ask on the same run must not flip status back while the first is still + * pending. */ + createResponder(context: ApprovalContext): PermissionAskResponder { + let pendingCount = 0; + + const suspend = async (): Promise => { + pendingCount += 1; + if (pendingCount === 1) await context.setStatus("suspended"); + }; + const resume = async (): Promise => { + pendingCount = Math.max(0, pendingCount - 1); + if (pendingCount === 0) await context.setStatus("processing"); + }; + + return (ask, signal) => + new Promise((resolve) => { + void this.#start(ask, context, suspend, resume, resolve, signal); + }); + } + + /** Bolt-agnostic: the `app.action(...)` handler registered by {@link registerPermissionApprovals} + * is a thin adapter onto this. Kept separate so tests exercise it without any Bolt object. */ + async handleAction(input: { + askId: string; + actionId: string; + clickerUserId: string; + }): Promise { + const verdict = verdictFromActionId(input.actionId); + if (verdict === undefined) return; + + const pending = this.#pending.get(input.askId); + if (pending === undefined) return; // stale, duplicate, or already-terminal — nothing to do + this.#pending.delete(input.askId); + + if (input.clickerUserId !== pending.authorizedUserId) { + // Structurally shouldn't happen (the card is DM'd only to that user), but reject rather + // than silently drop: put the entry back so the rightful owner can still decide. + this.#pending.set(input.askId, pending); + return; + } + + pending.onSettled(); + pending.resolve(verdict); + const headline = `${verdictLabel(verdict)}: ${pending.tool}`; + await this.#safeUpdate(pending.dmChannel, pending.ts, `${verdictLabel(verdict)}.`, [ + outcomeCard(pending.originLabel, headline), + ]); + } + + async #start( + ask: PermissionAskEventPayload, + context: ApprovalContext, + suspend: () => Promise, + resume: () => Promise, + resolve: (verdict: PermissionVerdict | undefined) => void, + signal: AbortSignal, + ): Promise { + await suspend(); + + const retractedText = "This request is no longer pending — the run ended or was cancelled."; + const retractedCard = (): unknown[] => [ + outcomeCard(context.originLabel, `No longer pending: ${ask.tool}`), + ]; + + let settled = false; + const onSettled = (): void => { + if (settled) return; + settled = true; + void resume(); + }; + signal.addEventListener("abort", () => { + const pending = this.#pending.get(ask.askId); + if (pending === undefined) return; + this.#pending.delete(ask.askId); + pending.onSettled(); + resolve(undefined); + void this.#safeUpdate(pending.dmChannel, pending.ts, retractedText, retractedCard()); + }); + + try { + const channel = await this.#client.openDm(context.authorizedUserId); + const ts = await this.#client.postMessage( + channel, + `Permission request from ${context.originLabel}: ${ask.tool}`, + approvalBlocks(ask, context.originLabel, ask.askId), + ); + if (signal.aborted) { + // Retracted/ended while the DM was still in flight — nothing was ever added to + // #pending for the abort listener above to have found, so clean up here instead. + onSettled(); + resolve(undefined); + await this.#safeUpdate(channel, ts, retractedText, retractedCard()); + return; + } + this.#pending.set(ask.askId, { + authorizedUserId: context.authorizedUserId, + dmChannel: channel, + onSettled, + originLabel: context.originLabel, + resolve, + tool: ask.tool, + ts, + }); + } catch (error) { + // Fail closed: an ask this bot can't even show the approver never silently proceeds. + this.#logger.warn("failed to DM a permission-approval card — denying closed", error); + onSettled(); + resolve("deny"); + } + } + + async #safeUpdate(channel: string, ts: string, text: string, blocks: unknown[]): Promise { + try { + await this.#client.updateMessage(channel, ts, text, blocks); + } catch (error) { + // Best-effort UI cleanup only — the ask itself is already resolved either way. + this.#logger.warn("failed to update a permission-approval DM", error); + } + } +} + +/** Registers the one Bolt-specific action handler and returns the gateway `agentSessions.ts` + * calls `createResponder` on per run. */ +export function registerPermissionApprovals(app: App): PermissionApprovalGateway { + const gateway = new PermissionApprovalGateway(slackApprovalMessaging(app.client), app.logger); + app.action(/^mecatl_approval:/, async ({ ack, action, body }) => { + await ack(); + if (!("action_id" in action) || !("value" in action)) return; + if (typeof action.action_id !== "string" || typeof action.value !== "string") return; + await gateway.handleAction({ + actionId: action.action_id, + askId: action.value, + clickerUserId: body.user.id, + }); + }); + return gateway; +} diff --git a/sdk/typescript/examples/slack-bot/src/bridge.ts b/sdk/typescript/examples/slack-bot/src/bridge.ts index ae4376a2a9..ff4825cbab 100644 --- a/sdk/typescript/examples/slack-bot/src/bridge.ts +++ b/sdk/typescript/examples/slack-bot/src/bridge.ts @@ -1,4 +1,10 @@ -import { type Client, type Run, ServerError, type Session } from "@stacklok-oss/mecatl-sdk"; +import { + type Client, + type PermissionAskResponder, + type Run, + ServerError, + type Session, +} from "@stacklok-oss/mecatl-sdk"; import { connect, type NodeConnectOptions } from "@stacklok-oss/mecatl-sdk/node"; export interface PromptOutcome { @@ -10,6 +16,10 @@ export interface PromptOutcome { /** Invoked with each incremental text chunk as the run streams, in order. */ export type DeltaHandler = (delta: string) => void | Promise; +/** Invoked once a queued prompt actually starts (`onStart`) or once it settles, success or + * failure (`onSettle`) — the run's real lifecycle, not the caller's own queueing call. */ +export type LifecycleHook = () => void | Promise; + /** * Detects the "session has a live external authorization" failed-precondition * error (#1283): a first-use ToolHive connector needs an interactive OAuth @@ -32,12 +42,13 @@ export function isStuckExternalAuthorization(error: unknown): boolean { /** * Bridges Slack threads to mecatl sessions: one session per thread key, - * created lazily and reused for every later prompt in that thread. Every - * permission ask is auto-approved (#882: "every demo run executes under an - * auto-approved posture so it never blocks on a human"). Caller-level - * authorization and a per-user rate limit live in agentSessions.ts, above - * this bridge (panel-review, #883) — this class only knows about threads, - * not who's behind them. + * created lazily and reused for every later prompt in that thread. Permission + * asks are answered by whatever `onPermissionAsk` responder the caller + * passes to `handlePrompt` (issue #1397) — this class no longer hardcodes + * `() => "allow_once"` (#882's original dev/demo baseline). Caller-level + * authorization, the per-user rate limit, and the manual-approval Slack UI + * all live in agentSessions.ts/approvals.ts, above this bridge (panel-review, + * #883) — this class only knows about threads, not who's behind them. * * Session placement is server-owned: this client never sends a workspace path. * Configure the daemon's default with `mecated --workspace`; every new Slack @@ -72,15 +83,34 @@ export class MecatlBridge { /** * Runs one prompt for a thread, queued behind any prompt already in flight * for it. `onDelta`, if given, is invoked in order with each incremental - * text chunk the run streams before the final result. + * text chunk the run streams before the final result. `onPermissionAsk`, + * if given, answers the run's tool-permission asks; with none, an ask is + * left pending until the run ends or is cancelled (see the SDK's + * `RunImpl#startPermissionResponder`) — callers that need real approvals + * must always pass one. + * + * `onStart`/`onSettle`, if given, fire exactly when THIS call's own + * queued execution actually begins/ends — not when `handlePrompt` is + * called, which can be well before its turn if an earlier prompt on the + * same thread is still running (panel-review, samuv: reflecting Slack + * session status from the caller's own call site, before it joins this + * queue, let a second same-thread message overwrite the first run's + * `suspended` status with `processing` while the first was still waiting + * on a human). Driving status from here instead ties it to the run that + * is actually active. */ async handlePrompt( threadKey: string, text: string, onDelta?: DeltaHandler, + onPermissionAsk?: PermissionAskResponder, + onStart?: LifecycleHook, + onSettle?: LifecycleHook, ): Promise { const previous = this.#queues.get(threadKey) ?? Promise.resolve(); - const next = previous.then(() => this.#runPrompt(threadKey, text, onDelta)); + const next = previous.then(() => + this.#runPrompt(threadKey, text, onDelta, onPermissionAsk, onStart, onSettle), + ); // Swallow so an awaited failure doesn't become an unhandled rejection on the queue chain. this.#queues.set( threadKey, @@ -109,16 +139,25 @@ export class MecatlBridge { threadKey: string, text: string, onDelta: DeltaHandler | undefined, + onPermissionAsk: PermissionAskResponder | undefined, + onStart: LifecycleHook | undefined, + onSettle: LifecycleHook | undefined, ): Promise { - const session = await this.#sessionFor(threadKey); + await onStart?.(); try { + // `#sessionFor` itself is inside this try too (panel-review, samuv, + // follow-up on #1707): `onStart` has already fired by this point, so a + // session-creation failure (e.g. the daemon is unreachable) must still + // reach `finally`'s `onSettle` below — otherwise Slack status is left + // stuck at "processing" forever with no run to ever resolve it. + const session = await this.#sessionFor(threadKey); // session.run() itself — not just the run's event stream — must be inside // this try (#1289 review, samuv): the server can reject a session with a // live external authorization at RUN ADMISSION, before the SDK's run() // ever resolves (it waits for the first run-ID-bearing event), so that // rejection previously escaped this catch entirely and the stuck session // was never evicted. - const run = await session.run(text, { onPermissionAsk: () => "allow_once" }); + const run = await session.run(text, onPermissionAsk === undefined ? {} : { onPermissionAsk }); this.#runs.set(threadKey, run); for await (const event of run) { if (event.kind === "message.delta") { @@ -151,6 +190,7 @@ export class MecatlBridge { throw error; } finally { this.#runs.delete(threadKey); + await onSettle?.(); } } diff --git a/sdk/typescript/examples/slack-bot/test/agentSessions.test.ts b/sdk/typescript/examples/slack-bot/test/agentSessions.test.ts index abb42f0f2a..925d301ebb 100644 --- a/sdk/typescript/examples/slack-bot/test/agentSessions.test.ts +++ b/sdk/typescript/examples/slack-bot/test/agentSessions.test.ts @@ -26,10 +26,14 @@ interface FakeApp { startStream: ReturnType; appendStream: ReturnType; stopStream: ReturnType; + conversationsOpen: ReturnType; + postMessage: ReturnType; + chatUpdate: ReturnType; warn: ReturnType; appMention: AnyHandler; message: AnyHandler; agentSessionStopped: AnyHandler; + action: AnyHandler; } /** Builds a fake bolt `App` and registers `registerAgentSessions` against it, capturing the @@ -45,17 +49,34 @@ function setUp( const startStream = vi.fn().mockResolvedValue({ ts: "stream-ts" }); const appendStream = vi.fn().mockResolvedValue(undefined); const stopStream = vi.fn().mockResolvedValue(undefined); + // `registerPermissionApprovals` (issue #1397) wires itself onto `app.client` too — these are + // only exercised by a test that actually triggers a permission ask; none here do yet. + const conversationsOpen = vi.fn().mockResolvedValue({ channel: { id: "D1" } }); + const postMessage = vi.fn().mockResolvedValue({ ts: "approval-ts" }); + const chatUpdate = vi.fn().mockResolvedValue(undefined); const warn = vi.fn(); const handlers: { appMention?: AnyHandler; message?: AnyHandler; agentSessionStopped?: AnyHandler; + action?: AnyHandler; } = {}; const app = { + action: (_pattern: RegExp, handler: AnyHandler) => { + handlers.action = handler; + }, client: { apiCall, - chat: { appendStream, postEphemeral, startStream, stopStream }, + chat: { + appendStream, + postEphemeral, + postMessage, + startStream, + stopStream, + update: chatUpdate, + }, + conversations: { open: conversationsOpen }, }, event: (eventName: string, handler: AnyHandler) => { if (eventName === "app_mention") handlers.appMention = handler; @@ -72,18 +93,23 @@ function setUp( if ( handlers.appMention === undefined || handlers.message === undefined || - handlers.agentSessionStopped === undefined + handlers.agentSessionStopped === undefined || + handlers.action === undefined ) { throw new Error("registerAgentSessions did not register the expected handlers"); } return { + action: handlers.action, agentSessionStopped: handlers.agentSessionStopped, apiCall, app, appendStream, appMention: handlers.appMention, + chatUpdate, + conversationsOpen, message: handlers.message, postEphemeral, + postMessage, say, startStream, stopStream, @@ -200,6 +226,35 @@ describe("registerAgentSessions", () => { expect(fake.say).toHaveBeenCalledWith({ text: "hi there", thread_ts: "100.001" }); }); + it("passes a permission-approval responder to handlePrompt (issue #1397)", async () => { + const handlePrompt = vi + .fn() + .mockResolvedValue({ sessionId: "s1", stopReason: "end_turn", text: "hi there" }); + const fake = setUp(fakeBridge(handlePrompt), fakeConfig()); + + await fake.appMention({ + event: { + bot_id: undefined, + channel: "C1", + text: "<@BOT> hi", + ts: "100.001", + type: "app_mention", + user: "allowed-user", + }, + context: { teamId: "T1" }, + say: fake.say, + }); + + expect(handlePrompt).toHaveBeenCalledWith( + "C1:100.001", + "hi", + expect.any(Function), + expect.any(Function), + expect.any(Function), + expect.any(Function), + ); + }); + it("rate-limits an app_mention with an ephemeral reply", async () => { const fake = setUp( fakeBridge(vi.fn()), @@ -362,10 +417,21 @@ describe("registerAgentSessions", () => { }); it("starts separate sessions for separate top-level DM messages", async () => { - const handlePrompt = vi.fn().mockImplementation(async (_threadKey, text, onDelta) => { - await onDelta(`reply to ${text}`); - return { sessionId: `session-${text}`, stopReason: "end_turn", text: `reply to ${text}` }; - }); + const handlePrompt = vi + .fn() + .mockImplementation( + async (_threadKey, text, onDelta, _onPermissionAsk, onStart, onSettle) => { + await onStart(); + await onDelta(`reply to ${text}`); + const outcome = { + sessionId: `session-${text}`, + stopReason: "end_turn", + text: `reply to ${text}`, + }; + await onSettle(); + return outcome; + }, + ); const fake = setUp(fakeBridge(handlePrompt), fakeConfig()); for (const [text, ts] of [ @@ -387,8 +453,24 @@ describe("registerAgentSessions", () => { }); } - expect(handlePrompt).toHaveBeenNthCalledWith(1, "D1:200.001", "first", expect.any(Function)); - expect(handlePrompt).toHaveBeenNthCalledWith(2, "D1:200.002", "second", expect.any(Function)); + expect(handlePrompt).toHaveBeenNthCalledWith( + 1, + "D1:200.001", + "first", + expect.any(Function), + expect.any(Function), + expect.any(Function), + expect.any(Function), + ); + expect(handlePrompt).toHaveBeenNthCalledWith( + 2, + "D1:200.002", + "second", + expect.any(Function), + expect.any(Function), + expect.any(Function), + expect.any(Function), + ); expect(fake.startStream).toHaveBeenNthCalledWith( 1, expect.objectContaining({ channel: "D1", thread_ts: "200.001" }), @@ -431,7 +513,14 @@ describe("registerAgentSessions", () => { say: fake.say, }); - expect(handlePrompt).toHaveBeenCalledWith("D1:200.001", "follow up", expect.any(Function)); + expect(handlePrompt).toHaveBeenCalledWith( + "D1:200.001", + "follow up", + expect.any(Function), + expect.any(Function), + expect.any(Function), + expect.any(Function), + ); expect(fake.startStream).toHaveBeenCalledWith( expect.objectContaining({ channel: "D1", thread_ts: "200.001" }), ); diff --git a/sdk/typescript/examples/slack-bot/test/approvals.test.ts b/sdk/typescript/examples/slack-bot/test/approvals.test.ts new file mode 100644 index 0000000000..084e82d9a0 --- /dev/null +++ b/sdk/typescript/examples/slack-bot/test/approvals.test.ts @@ -0,0 +1,444 @@ +import { describe, expect, it, vi } from "vitest"; + +import { + type ApprovalContext, + type ApprovalMessagingClient, + PermissionApprovalGateway, +} from "../src/approvals.js"; + +function fakeAsk( + overrides: Partial<{ args: string; askId: string; reason: string; tool: string }> = {}, +) { + return { + args: "{}", + askId: "ask-1", + reason: "configured ask", + tool: "Write", + ...overrides, + }; +} + +function fakeClient(): ApprovalMessagingClient & { + posted: { channel: string; text: string; blocks: unknown[] }[]; + updated: { channel: string; ts: string; text: string; blocks: unknown[] | undefined }[]; +} { + const posted: { channel: string; text: string; blocks: unknown[] }[] = []; + const updated: { channel: string; ts: string; text: string; blocks: unknown[] | undefined }[] = + []; + let nextTs = 0; + return { + async openDm(userId) { + return `dm-${userId}`; + }, + async postMessage(channel, text, blocks) { + posted.push({ blocks, channel, text }); + nextTs += 1; + return `ts-${nextTs}`; + }, + async updateMessage(channel, ts, text, blocks) { + updated.push({ blocks, channel, text, ts }); + }, + posted, + updated, + }; +} + +function fakeContext(overrides: Partial = {}): ApprovalContext & { + statuses: Array<"suspended" | "processing">; +} { + const statuses: Array<"suspended" | "processing"> = []; + return { + authorizedUserId: "U1", + originLabel: "a DM with the bot", + setStatus: async (status) => { + statuses.push(status); + }, + statuses, + ...overrides, + }; +} + +/** Waits a microtask/timer tick so an ask's fire-and-forget setup (`openDm`/`postMessage`) has + * settled before the test asserts on it — mirrors how the real SDK responder is invoked + * fire-and-forget from `RunImpl#startAsk`. */ +async function flush(): Promise { + await new Promise((resolveTick) => setTimeout(resolveTick, 0)); +} + +describe("PermissionApprovalGateway", () => { + it("resolves allow_once from a matching click and clears the pending entry", async () => { + const client = fakeClient(); + const gateway = new PermissionApprovalGateway(client); + const context = fakeContext(); + const responder = gateway.createResponder(context); + + const controller = new AbortController(); + const verdictPromise = responder(fakeAsk(), controller.signal); + await flush(); + + expect(client.posted).toHaveLength(1); + expect(context.statuses).toEqual(["suspended"]); + + await gateway.handleAction({ + actionId: "mecatl_approval:allow_once", + askId: "ask-1", + clickerUserId: "U1", + }); + + await expect(verdictPromise).resolves.toBe("allow_once"); + expect(context.statuses).toEqual(["suspended", "processing"]); + expect(client.updated).toEqual([ + { + blocks: [ + { + subtitle: { text: "Requested from a DM with the bot", type: "plain_text" }, + title: { text: "Allowed once: Write", type: "plain_text" }, + type: "card", + }, + ], + channel: "dm-U1", + text: "Allowed once.", + ts: "ts-1", + }, + ]); + }); + + it("resolves allow_always and deny the same way", async () => { + const client = fakeClient(); + const gateway = new PermissionApprovalGateway(client); + + const allowAlways = gateway.createResponder(fakeContext()); + const allowAlwaysVerdict = allowAlways( + fakeAsk({ askId: "ask-allow-always" }), + new AbortController().signal, + ); + await flush(); + await gateway.handleAction({ + actionId: "mecatl_approval:allow_always", + askId: "ask-allow-always", + clickerUserId: "U1", + }); + await expect(allowAlwaysVerdict).resolves.toBe("allow_always"); + + const deny = gateway.createResponder(fakeContext()); + const denyVerdict = deny(fakeAsk({ askId: "ask-deny" }), new AbortController().signal); + await flush(); + await gateway.handleAction({ + actionId: "mecatl_approval:deny", + askId: "ask-deny", + clickerUserId: "U1", + }); + await expect(denyVerdict).resolves.toBe("deny"); + }); + + it("ignores a click from anyone other than the authorized user", async () => { + const client = fakeClient(); + const gateway = new PermissionApprovalGateway(client); + const responder = gateway.createResponder(fakeContext({ authorizedUserId: "U1" })); + const verdictPromise = responder(fakeAsk(), new AbortController().signal); + await flush(); + + await gateway.handleAction({ + actionId: "mecatl_approval:allow_once", + askId: "ask-1", + clickerUserId: "someone-else", + }); + + // Still pending: a follow-up click from the rightful owner still resolves it. + await gateway.handleAction({ + actionId: "mecatl_approval:deny", + askId: "ask-1", + clickerUserId: "U1", + }); + await expect(verdictPromise).resolves.toBe("deny"); + expect(client.updated).toHaveLength(1); // only the rightful click updated the DM + }); + + it("ignores a duplicate click after the ask is already resolved", async () => { + const client = fakeClient(); + const gateway = new PermissionApprovalGateway(client); + const responder = gateway.createResponder(fakeContext()); + const verdictPromise = responder(fakeAsk(), new AbortController().signal); + await flush(); + + await gateway.handleAction({ + actionId: "mecatl_approval:allow_once", + askId: "ask-1", + clickerUserId: "U1", + }); + await expect(verdictPromise).resolves.toBe("allow_once"); + + // A second click (double-tap, or a stale button) after resolution: no-op, no second update. + await gateway.handleAction({ + actionId: "mecatl_approval:deny", + askId: "ask-1", + clickerUserId: "U1", + }); + expect(client.updated).toHaveLength(1); + }); + + it("clears the pending ask and the DM when the signal aborts (run ended/retracted)", async () => { + const client = fakeClient(); + const gateway = new PermissionApprovalGateway(client); + const context = fakeContext(); + const responder = gateway.createResponder(context); + const controller = new AbortController(); + const verdictPromise = responder(fakeAsk(), controller.signal); + await flush(); + + controller.abort(); + await flush(); + + await expect(verdictPromise).resolves.toBeUndefined(); + expect(context.statuses).toEqual(["suspended", "processing"]); + expect(client.updated).toEqual([ + { + blocks: [ + { + subtitle: { text: "Requested from a DM with the bot", type: "plain_text" }, + title: { text: "No longer pending: Write", type: "plain_text" }, + type: "card", + }, + ], + channel: "dm-U1", + text: "This request is no longer pending — the run ended or was cancelled.", + ts: "ts-1", + }, + ]); + + // A click arriving after the abort has nothing pending to act on. + await gateway.handleAction({ + actionId: "mecatl_approval:allow_once", + askId: "ask-1", + clickerUserId: "U1", + }); + expect(client.updated).toHaveLength(1); + }); + + it("only suspends on the first of two concurrent asks and resumes after the last resolves", async () => { + const client = fakeClient(); + const gateway = new PermissionApprovalGateway(client); + const context = fakeContext(); + const responder = gateway.createResponder(context); + + const first = responder(fakeAsk({ askId: "ask-1" }), new AbortController().signal); + const second = responder(fakeAsk({ askId: "ask-2" }), new AbortController().signal); + await flush(); + + expect(context.statuses).toEqual(["suspended"]); + + await gateway.handleAction({ + actionId: "mecatl_approval:allow_once", + askId: "ask-1", + clickerUserId: "U1", + }); + await first; + // ask-2 is still pending — status must not flip back to processing yet. + expect(context.statuses).toEqual(["suspended"]); + + await gateway.handleAction({ + actionId: "mecatl_approval:deny", + askId: "ask-2", + clickerUserId: "U1", + }); + await second; + expect(context.statuses).toEqual(["suspended", "processing"]); + }); + + it("fails closed (denies) if it can't even show the approver the ask, and logs it", async () => { + const client: ApprovalMessagingClient = { + openDm: vi.fn().mockRejectedValue(new Error("missing im:write scope")), + postMessage: vi.fn(), + updateMessage: vi.fn(), + }; + const warn = vi.fn(); + const gateway = new PermissionApprovalGateway(client, { warn }); + const context = fakeContext(); + const responder = gateway.createResponder(context); + + await expect(responder(fakeAsk(), new AbortController().signal)).resolves.toBe("deny"); + expect(context.statuses).toEqual(["suspended", "processing"]); + expect(warn).toHaveBeenCalledWith(expect.stringContaining("DM"), expect.any(Error)); + }); + + it("ignores an action_id outside the mecatl_approval: namespace", async () => { + const client = fakeClient(); + const gateway = new PermissionApprovalGateway(client); + const responder = gateway.createResponder(fakeContext()); + const verdictPromise = responder(fakeAsk(), new AbortController().signal); + await flush(); + + await gateway.handleAction({ + actionId: "some_other_plugin:button", + askId: "ask-1", + clickerUserId: "U1", + }); + + await gateway.handleAction({ + actionId: "mecatl_approval:allow_once", + askId: "ask-1", + clickerUserId: "U1", + }); + await expect(verdictPromise).resolves.toBe("allow_once"); + }); +}); + +describe("PermissionApprovalGateway args rendering", () => { + function tableBlock(blocks: unknown[]): { rows: unknown[][] } | undefined { + return blocks.find( + (block): block is { rows: unknown[][] } => + typeof block === "object" && block !== null && "rows" in block, + ); + } + + function firstPostedBlocks(client: ReturnType): unknown[] { + const post = client.posted[0]; + if (post === undefined) throw new Error("expected a message to have been posted"); + return post.blocks; + } + + /** Mirrors `keyCell`/`richTextCell` in src/approvals.ts: a bold `rich_text` run. */ + function keyCell(text: string): unknown { + return { + elements: [ + { elements: [{ style: { bold: true }, text, type: "text" }], type: "rich_text_section" }, + ], + type: "rich_text", + }; + } + + /** Mirrors `valueCell` for the non-numeric branch: a code-styled `rich_text` run. */ + function valueCell(text: string): unknown { + return { + elements: [ + { elements: [{ style: { code: true }, text, type: "text" }], type: "rich_text_section" }, + ], + type: "rich_text", + }; + } + + it("renders a flat args object as a table, one row per key", async () => { + const client = fakeClient(); + const gateway = new PermissionApprovalGateway(client); + const responder = gateway.createResponder(fakeContext()); + void responder( + fakeAsk({ args: JSON.stringify({ content: "hello", path: "test.txt" }) }), + new AbortController().signal, + ); + await flush(); + + const table = tableBlock(firstPostedBlocks(client)); + expect(table?.rows).toEqual([ + [keyCell("content"), valueCell("hello")], + [keyCell("path"), valueCell("test.txt")], + ]); + }); + + it("renders a number as a raw_number cell", async () => { + const client = fakeClient(); + const gateway = new PermissionApprovalGateway(client); + const responder = gateway.createResponder(fakeContext()); + void responder( + fakeAsk({ args: JSON.stringify({ timeout: 30 }) }), + new AbortController().signal, + ); + await flush(); + + const table = tableBlock(firstPostedBlocks(client)); + expect(table?.rows).toEqual([[keyCell("timeout"), { text: "30", type: "raw_number" }]]); + }); + + it("renders a nested value as inline code-styled JSON instead of a leaf value", async () => { + const client = fakeClient(); + const gateway = new PermissionApprovalGateway(client); + const responder = gateway.createResponder(fakeContext()); + void responder( + fakeAsk({ args: JSON.stringify({ options: { recursive: true }, tags: ["a", "b"] }) }), + new AbortController().signal, + ); + await flush(); + + const table = tableBlock(firstPostedBlocks(client)); + expect(table?.rows).toEqual([ + [keyCell("options"), valueCell('{"recursive":true}')], + [keyCell("tags"), valueCell('["a","b"]')], + ]); + }); + + it("falls back to a single-cell raw dump for non-object args (e.g. invalid JSON)", async () => { + const client = fakeClient(); + const gateway = new PermissionApprovalGateway(client); + const responder = gateway.createResponder(fakeContext()); + void responder(fakeAsk({ args: "not valid json" }), new AbortController().signal); + await flush(); + + const table = tableBlock(firstPostedBlocks(client)); + expect(table?.rows).toEqual([[{ text: "not valid json", type: "raw_text" }]]); + }); + + it("omits the args table entirely for an empty object", async () => { + const client = fakeClient(); + const gateway = new PermissionApprovalGateway(client); + const responder = gateway.createResponder(fakeContext()); + void responder(fakeAsk({ args: "{}" }), new AbortController().signal); + await flush(); + + expect(firstPostedBlocks(client)).toHaveLength(1); // just the card, no table block + }); + + it("caps the table at 10 rows and notes how many were omitted", async () => { + const client = fakeClient(); + const gateway = new PermissionApprovalGateway(client); + const responder = gateway.createResponder(fakeContext()); + const manyArgs = Object.fromEntries( + Array.from({ length: 12 }, (_, i) => [`key${i}`, `value${i}`]), + ); + void responder(fakeAsk({ args: JSON.stringify(manyArgs) }), new AbortController().signal); + await flush(); + + const rows = tableBlock(firstPostedBlocks(client))?.rows ?? []; + expect(rows).toHaveLength(10); + expect(rows[9]).toEqual([ + { text: "…", type: "raw_text" }, + { text: "and 3 more", type: "raw_text" }, + ]); + }); + + it("puts the tool on the title and origin+reason on the subtitle, not the body", async () => { + const client = fakeClient(); + const gateway = new PermissionApprovalGateway(client); + const responder = gateway.createResponder(fakeContext({ originLabel: "a channel" })); + void responder( + fakeAsk({ args: "{}", reason: "matched a configured ask rule", tool: "Shell" }), + new AbortController().signal, + ); + await flush(); + + const [card] = firstPostedBlocks(client) as [Record]; + expect(card).toMatchObject({ + subtitle: { + text: "Requested from a channel · matched a configured ask rule", + type: "plain_text", + }, + title: { text: "Permission request: Shell", type: "plain_text" }, + type: "card", + }); + expect(card).not.toHaveProperty("body"); + }); + + it("truncates a too-long subtitle to exactly 150 characters, never 151 (panel-review, samuv)", async () => { + const client = fakeClient(); + const gateway = new PermissionApprovalGateway(client); + // originLabel alone (via "Requested from ") pushes the subtitle to 151+ raw + // characters before clamping — long enough to catch an off-by-one in the truncation itself. + const originLabel = "x".repeat(200); + const responder = gateway.createResponder(fakeContext({ originLabel })); + void responder(fakeAsk({ reason: "" }), new AbortController().signal); + await flush(); + + const [card] = firstPostedBlocks(client) as [Record]; + const subtitle = card.subtitle as { text: string; type: string }; + expect(subtitle.text).toHaveLength(150); + expect(subtitle.text.endsWith("…")).toBe(true); + }); +}); diff --git a/sdk/typescript/examples/slack-bot/test/bridge.test.ts b/sdk/typescript/examples/slack-bot/test/bridge.test.ts index 3a6b577bf3..7d80edc9ab 100644 --- a/sdk/typescript/examples/slack-bot/test/bridge.test.ts +++ b/sdk/typescript/examples/slack-bot/test/bridge.test.ts @@ -1,7 +1,7 @@ import { describe, expect, it } from "vitest"; import { MecatlBridge } from "../src/bridge.js"; -import { cannedMockReply, withMockDaemon } from "./harness.js"; +import { cannedMockReply, fixture, withMockDaemon } from "./harness.js"; describe("MecatlBridge", () => { it("answers a prompt with the mock provider's canned reply", async () => { @@ -114,4 +114,135 @@ describe("MecatlBridge", () => { } }); }); + + it("threads onPermissionAsk to session.run so a real ask resolves through it", async () => { + await withMockDaemon( + async ({ baseUrl }) => { + const bridge = new MecatlBridge({ baseUrl }); + try { + const asks: string[] = []; + const outcome = await bridge.handlePrompt( + "channel:thread-1", + "approve the scripted write", + undefined, + (ask) => { + asks.push(ask.tool); + return "allow_once"; + }, + ); + expect(asks).toEqual(["Write"]); + expect(outcome.text).toBe("approved write completed"); + expect(outcome.stopReason).toBe("end_turn"); + } finally { + await bridge.close(); + } + }, + { script: fixture("permission-ask.json") }, + ); + }); + + it("with no onPermissionAsk, a permission ask is left pending until the run is cancelled", async () => { + await withMockDaemon( + async ({ baseUrl }) => { + const bridge = new MecatlBridge({ baseUrl }); + try { + const prompt = bridge.handlePrompt("channel:thread-1", "approve the scripted write"); + // Give the run a moment to reach the ask and genuinely stall on it, + // then cancel — without a responder, nothing else will ever settle it. + await new Promise((resolveWait) => setTimeout(resolveWait, 200)); + await bridge.cancel("channel:thread-1"); + await expect(prompt).resolves.toMatchObject({ stopReason: "cancelled" }); + } finally { + await bridge.close(); + } + }, + { script: fixture("permission-ask.json") }, + ); + }); + + it("does not fire a second prompt's onStart until the first's onSettle, even while the first is stalled on an ask (panel-review, samuv)", async () => { + await withMockDaemon( + async ({ baseUrl }) => { + const bridge = new MecatlBridge({ baseUrl }); + try { + const events: string[] = []; + let releaseAsk: (verdict: "allow_once") => void = () => {}; + const askHeld = new Promise<"allow_once">((resolve) => { + releaseAsk = resolve; + }); + + const first = bridge.handlePrompt( + "channel:thread-1", + "approve the scripted write", + undefined, + () => askHeld, + () => { + events.push("first:start"); + }, + () => { + events.push("first:settle"); + }, + ); + + // Give the first run time to actually reach the ask and stall on it. + await new Promise((resolveWait) => setTimeout(resolveWait, 200)); + expect(events).toEqual(["first:start"]); + + const second = bridge.handlePrompt( + "channel:thread-1", + "hello", + undefined, + undefined, + () => { + events.push("second:start"); + }, + () => { + events.push("second:settle"); + }, + ); + + // The second call is queued behind the first — its onStart must not fire + // just because handlePrompt() was called; only once it's actually dequeued. + await new Promise((resolveWait) => setTimeout(resolveWait, 200)); + expect(events).toEqual(["first:start"]); + + releaseAsk("allow_once"); + await first; + await second; + + expect(events).toEqual(["first:start", "first:settle", "second:start", "second:settle"]); + } finally { + await bridge.close(); + } + }, + { script: fixture("permission-ask.json") }, + ); + }); + + it("still fires onSettle if session creation itself fails (panel-review, samuv follow-up)", async () => { + // Nothing listens here, so sessions.create() (and #sessionFor) rejects before a session + // or run ever exists — onStart has already fired by then, so onSettle must still fire too, + // or Slack status would be stuck at "processing" forever. + const bridge = new MecatlBridge({ baseUrl: "http://127.0.0.1:1" }); + try { + const events: string[] = []; + await expect( + bridge.handlePrompt( + "channel:thread-1", + "hello", + undefined, + undefined, + () => { + events.push("start"); + }, + () => { + events.push("settle"); + }, + ), + ).rejects.toThrow(); + expect(events).toEqual(["start", "settle"]); + } finally { + await bridge.close(); + } + }); }); diff --git a/sdk/typescript/examples/slack-bot/test/fixtures/permission-ask.json b/sdk/typescript/examples/slack-bot/test/fixtures/permission-ask.json new file mode 100644 index 0000000000..05472164fc --- /dev/null +++ b/sdk/typescript/examples/slack-bot/test/fixtures/permission-ask.json @@ -0,0 +1,14 @@ +{ + "turns": [ + { + "tool_calls": [ + { + "id": "approve-write", + "name": "Write", + "args": { "path": "approved.txt", "content": "approved\n" } + } + ] + }, + { "text": "approved write completed" } + ] +} diff --git a/sdk/typescript/examples/slack-bot/test/harness.ts b/sdk/typescript/examples/slack-bot/test/harness.ts index 77c4ae9ce3..42628733ca 100644 --- a/sdk/typescript/examples/slack-bot/test/harness.ts +++ b/sdk/typescript/examples/slack-bot/test/harness.ts @@ -10,6 +10,12 @@ export const cannedMockReply = "Mock provider: no real model is configured. Set OPENAI_API_KEY for live use."; const repositoryRoot = resolve(dirname(fileURLToPath(import.meta.url)), "../../../../.."); +const testDirectory = dirname(fileURLToPath(import.meta.url)); + +/** Resolves a `--mock-script` fixture under this package's own `test/fixtures/`. */ +export function fixture(name: string): string { + return join(testDirectory, "fixtures", name); +} interface ReadyDocument { grpc_address: string; @@ -21,8 +27,18 @@ export interface Daemon { workspace: string; } +export interface MockDaemonOptions { + /** Path to a `--mock-script` fixture (see `sdk/typescript/e2e/fixtures/*.json` for the + * format) — drives the mock provider through scripted tool calls instead of always + * returning `cannedMockReply`. Omit for the default canned-reply behavior. */ + script?: string; +} + /** Spawns a real, offline `mecated --mock` daemon for the duration of `run`. */ -export async function withMockDaemon(run: (daemon: Daemon) => Promise): Promise { +export async function withMockDaemon( + run: (daemon: Daemon) => Promise, + options: MockDaemonOptions = {}, +): Promise { const runtimeDirectory = await mkdtemp(join(tmpdir(), "mecatl-slack-bot-test-")); const readyFile = join(runtimeDirectory, "ready.json"); const workspace = join(runtimeDirectory, "workspace"); @@ -45,6 +61,7 @@ export async function withMockDaemon(run: (daemon: Daemon) => Promise): Pr "--no-scheduler", "--flight-recorder=false", ]; + if (options.script !== undefined) args.push("--mock-script", options.script); const environment = { ...process.env }; delete environment.ANTHROPIC_API_KEY;