Merge pull request #105 from odefun/feat/slack-router-refactor-366959

refactor: harden Slack message router hot path

Author: LIU9293

packages/ims/slack/commands.ts

@@ -58,6 +58,35 @@ type AgentProvider = "opencode" | "claudecode" | "codex" | "kimi" | "kiro" | "ki
 type StatusMessageFormat = "aggressive" | "medium" | "minimum";
 type GitStrategy = "default" | "worktree";
 
+type SlackActionBody = {
+  actions?: Array<{
+    value?: string;
+    selected_option?: {
+      value?: string;
+    };
+  }>;
+  channel?: {
+    id?: string;
+  };
+  user?: {
+    id?: string;
+  };
+  trigger_id?: string;
+  view?: {
+    private_metadata?: string;
+    id?: string;
+    hash?: string;
+    state?: {
+      values?: Record<string, Record<string, { value?: string; selected_option?: { value?: string } }>>;
+    };
+  };
+  message?: {
+    thread_ts?: string;
+    ts?: string;
+    text?: string;
+  };
+};
+
 const AGENT_PROVIDERS: AgentProvider[] = ["opencode", "claudecode", "codex", "kimi", "kiro", "kilo", "qwen", "goose"];
 
 const AGENT_PROVIDER_LABELS: Record<AgentProvider, string> = {
@@ -109,6 +138,38 @@ function toSelectableProvider(provider: "opencode" | "claudecode" | "codex" | "k
   return parseAgentProvider(provider);
 }
 
+function getActionChannelId(body: SlackActionBody): string | undefined {
+  return body.actions?.[0]?.value ?? body.channel?.id;
+}
+
+function getActionUserId(body: SlackActionBody): string | undefined {
+  return body.user?.id;
+}
+
+function getActionTriggerId(body: SlackActionBody): string | undefined {
+  return body.trigger_id;
+}
+
+function getActionSelectedOptionValue(body: SlackActionBody): string | undefined {
+  return body.actions?.[0]?.selected_option?.value;
+}
+
+function getActionViewMetadata(body: SlackActionBody): string | undefined {
+  return body.view?.private_metadata;
+}
+
+function getActionThreadTs(body: SlackActionBody): string | undefined {
+  return body.message?.thread_ts || body.message?.ts;
+}
+
+function getActionMessageTs(body: SlackActionBody): string | undefined {
+  return body.message?.ts;
+}
+
+function getActionMessageText(body: SlackActionBody): string {
+  return body.message?.text || "Question";
+}
+
 function buildSettingsModal(params: {
   channelId: string;
   enabledProviders: AgentProvider[];
@@ -417,9 +478,11 @@ export function setupInteractiveHandlers(): void {
     slackApp.action(SETTINGS_LAUNCH_ACTION, async ({ ack, body, client }) => {
     await ack();
 
-    const channelId = (body as any).actions?.[0]?.value
-      ?? (body as any).channel?.id;
-    if (!channelId) return;
+    const actionBody = body as SlackActionBody;
+
+    const channelId = getActionChannelId(actionBody);
+    const triggerId = getActionTriggerId(actionBody);
+    if (!channelId || !triggerId) return;
 
     try {
       await startOpenCodeServer();
@@ -448,18 +511,20 @@ export function setupInteractiveHandlers(): void {
     });
 
     await client.views.open({
-      trigger_id: (body as any).trigger_id,
+      trigger_id: triggerId,
       view,
     });
     });
 
     slackApp.action(GITHUB_LAUNCH_ACTION, async ({ ack, body, client }) => {
     await ack();
 
-    const channelId = (body as any).actions?.[0]?.value
-      ?? (body as any).channel?.id;
-    const userId = (body as any).user?.id;
-    if (!channelId || !userId) return;
+    const actionBody = body as SlackActionBody;
+
+    const channelId = getActionChannelId(actionBody);
+    const userId = getActionUserId(actionBody);
+    const triggerId = getActionTriggerId(actionBody);
+    if (!channelId || !userId || !triggerId) return;
 
     const info = getGitHubInfoForUser(userId);
     const view = buildGitHubTokenModal({
@@ -471,16 +536,19 @@ export function setupInteractiveHandlers(): void {
     });
 
     await client.views.open({
-      trigger_id: (body as any).trigger_id,
+      trigger_id: triggerId,
       view,
     });
     });
 
     slackApp.action(GENERAL_SETTINGS_LAUNCH_ACTION, async ({ ack, body, client }) => {
     await ack();
 
-    const channelId = (body as any).actions?.[0]?.value
-      ?? (body as any).channel?.id
+    const actionBody = body as SlackActionBody;
+    const triggerId = getActionTriggerId(actionBody);
+    if (!triggerId) return;
+
+    const channelId = getActionChannelId(actionBody)
       ?? "";
     const generalSettings = getUserGeneralSettings();
     const view = buildGeneralSettingsModal({
@@ -490,18 +558,20 @@ export function setupInteractiveHandlers(): void {
     });
 
     await client.views.open({
-      trigger_id: (body as any).trigger_id,
+      trigger_id: triggerId,
       view,
     });
     });
 
     slackApp.action(GENERAL_SYNC_WORKSPACE_ACTION, async ({ ack, body, client }) => {
       await ack();
 
-      const channelId = (body as any).actions?.[0]?.value
-        ?? (body as any).view?.private_metadata
-        ?? (body as any).channel?.id;
-      const userId = (body as any).user?.id;
+      const actionBody = body as SlackActionBody;
+
+      const channelId = getActionChannelId(actionBody)
+        ?? getActionViewMetadata(actionBody)
+        ?? actionBody.channel?.id;
+      const userId = getActionUserId(actionBody);
       if (!channelId || !userId) return;
 
       const workspaces = getWorkspaces();
@@ -532,11 +602,14 @@ export function setupInteractiveHandlers(): void {
     slackApp.action(PROVIDER_ACTION, async ({ ack, body, client }) => {
     await ack();
 
-    const view = (body as any).view;
-    if (!view) return;
+    const actionBody = body as SlackActionBody;
+
+    const view = actionBody.view;
+    if (!view || !view.id || !view.hash) return;
 
     const channelId = view.private_metadata;
-    const selectedOption = (body as any).actions?.[0]?.selected_option?.value;
+    if (!channelId) return;
+    const selectedOption = getActionSelectedOptionValue(actionBody);
     const selectedProvider = parseAgentProvider(selectedOption);
     if (selectedProvider === "opencode") {
       try {
@@ -656,7 +729,8 @@ export function setupInteractiveHandlers(): void {
       setChannelBaseBranch(channelId, baseBranch);
       setChannelSystemMessage(channelId, channelSystemMessage);
     } catch (err) {
-      const userId = (body as any).user?.id;
+      const actionBody = body as SlackActionBody;
+      const userId = getActionUserId(actionBody);
       if (userId) {
         await client.chat.postEphemeral({
           channel: channelId,
@@ -667,7 +741,8 @@ export function setupInteractiveHandlers(): void {
       return;
     }
 
-    const userId = (body as any).user?.id;
+    const actionBody = body as SlackActionBody;
+    const userId = getActionUserId(actionBody);
     if (userId) {
       await client.chat.postEphemeral({
         channel: channelId,
@@ -686,9 +761,11 @@ export function setupInteractiveHandlers(): void {
 
     await ack();
 
-    const userId = (body as any).user?.id;
-    const channelId = (body as any).view?.private_metadata
-      ?? (body as any).channel?.id;
+    const actionBody = body as SlackActionBody;
+
+    const userId = getActionUserId(actionBody);
+    const channelId = getActionViewMetadata(actionBody)
+      ?? actionBody.channel?.id;
     if (!userId || !channelId) return;
 
     try {
@@ -734,8 +811,9 @@ export function setupInteractiveHandlers(): void {
         gitStrategy,
       });
     } catch (err) {
-      const userId = (body as any).user?.id;
-      const channelId = view.private_metadata || (body as any).channel?.id;
+      const actionBody = body as SlackActionBody;
+      const userId = getActionUserId(actionBody);
+      const channelId = view.private_metadata || actionBody.channel?.id;
       if (userId && channelId) {
         await client.chat.postEphemeral({
           channel: channelId,
@@ -746,8 +824,9 @@ export function setupInteractiveHandlers(): void {
       return;
     }
 
-    const userId = (body as any).user?.id;
-    const channelId = view.private_metadata || (body as any).channel?.id;
+    const actionBody = body as SlackActionBody;
+    const userId = getActionUserId(actionBody);
+    const channelId = view.private_metadata || actionBody.channel?.id;
     if (userId && channelId) {
       await client.chat.postEphemeral({
         channel: channelId,
@@ -761,18 +840,20 @@ export function setupInteractiveHandlers(): void {
     slackApp.action(/^user_choice_\d+$/, async ({ ack, body, client }) => {
     await ack();
 
-    const action = (body as any).actions?.[0];
+    const actionBody = body as SlackActionBody;
+
+    const action = actionBody.actions?.[0];
     const value = action?.value;
-    const channel = (body as any).channel?.id;
-    const threadTs = (body as any).message?.thread_ts || (body as any).message?.ts;
-    const userId = (body as any).user?.id;
-    const messageTs = (body as any).message?.ts;
+    const channel = actionBody.channel?.id;
+    const threadTs = getActionThreadTs(actionBody);
+    const userId = getActionUserId(actionBody);
+    const messageTs = getActionMessageTs(actionBody);
 
     if (!value || !channel || !threadTs) return;
 
     // Update the original message to remove buttons (keep question text only)
     if (messageTs) {
-      const originalText = (body as any).message?.text || "Question";
+      const originalText = getActionMessageText(actionBody);
       await client.chat.update({
         channel,
         ts: messageTs,

packages/ims/slack/message-router.test.ts

@@ -0,0 +1,126 @@
+import { describe, expect, it, mock } from "bun:test";
+import { registerSlackMessageRouter } from "./message-router";
+
+function createDeps(overrides: Partial<Parameters<typeof registerSlackMessageRouter>[0]> = {}) {
+  const handleIncomingMessage = mock(async () => {});
+
+  return {
+    app: {
+      message: (handler: unknown) => handler,
+    },
+    isAuthorizedChannel: () => true,
+    resolveWorkspaceAuth: () => undefined,
+    syncWorkspaceForChannel: async () => false,
+    getChannelWorkspaceName: () => "workspace",
+    setChannelWorkspaceName: () => {},
+    setChannelWorkspaceAuth: () => {},
+    isThreadActive: () => false,
+    markThreadActive: () => {},
+    isGeneralSettingsCommand: () => false,
+    postGeneralSettingsLauncher: async () => {},
+    describeSettingsIssues: () => [],
+    getChannelAgentProvider: () => "opencode" as const,
+    handleStopCommand: async () => false,
+    handleIncomingMessage,
+    ...overrides,
+  };
+}
+
+describe("registerSlackMessageRouter", () => {
+  it("caches bot identity and avoids auth.test on every message", async () => {
+    let registeredHandler: ((args: any) => Promise<void>) | undefined;
+    const authTest = mock(async () => ({ user_id: "U_BOT", team_id: "T1", enterprise_id: "E1" }));
+    const handleIncomingMessage = mock(async () => {});
+    const deps = createDeps({
+      app: {
+        message: (handler: (args: any) => Promise<void>) => {
+          registeredHandler = handler;
+        },
+      },
+      handleIncomingMessage,
+    });
+
+    registerSlackMessageRouter(deps);
+    expect(registeredHandler).toBeDefined();
+
+    const say = mock(async () => {});
+
+    await registeredHandler!({
+      message: {
+        channel: "C1",
+        user: "U1",
+        text: "<@U_BOT> hello",
+        ts: "1710000000.000001",
+      },
+      client: {
+        auth: {
+          test: authTest,
+        },
+      },
+      context: { teamId: "T1", enterpriseId: "E1" },
+      body: { team_id: "T1", enterprise_id: "E1" },
+      say,
+    });
+
+    await registeredHandler!({
+      message: {
+        channel: "C1",
+        user: "U2",
+        text: "<@U_BOT> again",
+        ts: "1710000000.000002",
+      },
+      client: {
+        auth: {
+          test: authTest,
+        },
+      },
+      context: { teamId: "T1", enterpriseId: "E1" },
+      body: { team_id: "T1", enterprise_id: "E1" },
+      say,
+    });
+
+    expect(authTest).toHaveBeenCalledTimes(1);
+    expect(handleIncomingMessage).toHaveBeenCalledTimes(2);
+  });
+
+  it("handles message processing errors with a thread reply", async () => {
+    let registeredHandler: ((args: any) => Promise<void>) | undefined;
+    const deps = createDeps({
+      app: {
+        message: (handler: (args: any) => Promise<void>) => {
+          registeredHandler = handler;
+        },
+      },
+      handleIncomingMessage: async () => {
+        throw new Error("boom");
+      },
+    });
+
+    registerSlackMessageRouter(deps);
+    expect(registeredHandler).toBeDefined();
+
+    const say = mock(async () => {});
+
+    await registeredHandler!({
+      message: {
+        channel: "C1",
+        user: "U1",
+        text: "<@U_BOT> fail",
+        ts: "1710000000.000003",
+      },
+      client: {
+        auth: {
+          test: async () => ({ user_id: "U_BOT", team_id: "T1" }),
+        },
+      },
+      context: { teamId: "T1" },
+      body: { team_id: "T1" },
+      say,
+    });
+
+    expect(say).toHaveBeenCalledWith({
+      text: "I hit an internal error while handling that message. Please try again.",
+      thread_ts: "1710000000.000003",
+    });
+  });
+});