slack-bot: DM manual permission approvals instead of auto-approving

.gitignore

@@ -43,3 +43,4 @@ coverage.*
 # Personal, local-only Claude Code instructions and running-state notes
 /CLAUDE.local.md
 /HANDOFF.md
+.env

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, PendingApproval>`** — `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
 

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.

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",

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,27 +52,21 @@ 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,
   bridge: MecatlBridge,
   config: BotConfig,
   resolver: AccessResolver,
 ): void {
-  const greetedDm = new Set<string>();
+  const approvals = registerPermissionApprovals(app);
   const activeChannelThreads = new Set<string>();
   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<void> {
-  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");
   }
 }