diff --git a/.v0/plans/add-is-external-channel.md b/.v0/plans/add-is-external-channel.md new file mode 100644 index 00000000..f1fd95dd --- /dev/null +++ b/.v0/plans/add-is-external-channel.md @@ -0,0 +1,76 @@ +# Add `isExternalChannel` support to chat SDK + +## Background + +Slack sends `is_ext_shared_channel: boolean` on event callback payloads. When the bot is in an external/shared channel (Slack Connect), it may be leaking internal context (repo summaries, etc.) to outsiders. We need to surface this at the `Thread` level so consumers can gate behavior. + +## API Design Decision: `isExternalChannel` (boolean) vs `visibility` (enum) + +**Recommendation: `isExternalChannel: boolean`** + +Reasons: +- **Matches the source data**: Slack (the only platform with this concept today) sends a simple `is_ext_shared_channel` boolean. No need to over-abstract. +- **Follows the `isDM` pattern**: The codebase already uses `isDM: boolean` on Thread/Postable with the exact same architecture (adapter method -> Chat.createThread -> ThreadImpl property). Adding `isExternalChannel` is perfectly consistent. +- **Discord has no equivalent**: Discord doesn't support cross-server channel sharing at all. +- **Teams**: Teams has "shared channels" (`membershipType: "shared"`) but that's a different Teams-specific concept. It could map to `isExternalChannel = true` in the future. +- **Google Chat**: GChat has external spaces but it's not exposed in webhook payloads -- would need a separate API call. Can be added later. +- **GitHub/Linear**: No concept of external channels. +- A `visibility` enum (`"private" | "external" | "public"`) conflates two orthogonal concerns -- DM/private vs external. A channel can be both public within a workspace AND externally shared. Keeping them separate (`isDM` + `isExternalChannel`) is cleaner. + +## Implementation Plan + +### 1. Core types (`packages/chat/src/types.ts`) + +- Add `isExternalChannel?(threadId: string): boolean` to the `Adapter` interface (optional, like `isDM?`) +- Add `readonly isExternalChannel: boolean` to the `Postable` interface (alongside `isDM`) +- Add `isExternalChannel?: boolean` to `ThreadInfo` interface +- Add `isExternalChannel?: boolean` to `ChannelInfo` interface + +### 2. Thread implementation (`packages/chat/src/thread.ts`) + +- Add `isExternalChannel?: boolean` to `SerializedThread` +- Add `isExternalChannel?: boolean` to both `ThreadImplConfigWithAdapter` and `ThreadImplConfigLazy` +- Add `readonly isExternalChannel: boolean` property to `ThreadImpl` class +- Set it in constructor: `this.isExternalChannel = config.isExternalChannel ?? false` +- Include it in `toJSON()` and `fromJSON()` serialization + +### 3. Channel implementation (`packages/chat/src/channel.ts`) + +- Add `isExternalChannel` to `ChannelImpl` (same pattern as `isDM`) + +### 4. Chat class (`packages/chat/src/chat.ts`) + +- In `createThread()`, call `adapter.isExternalChannel?.(threadId) ?? false` and pass to ThreadImpl constructor (same pattern as `isDM`) + +### 5. Slack adapter (`packages/adapter-slack/src/index.ts`) + +- Add `is_ext_shared_channel?: boolean` to the `SlackWebhookPayload` interface +- Cache `is_ext_shared_channel` per channel ID in a `Set` from incoming payloads +- Also read `is_ext_shared` from `conversations.info` API responses in `fetchThread` and `fetchChannelInfo` +- Implement `isExternalChannel(threadId)` that checks the cache + +### 6. Other adapters (Discord, Teams, GChat, GitHub, Linear) + +- `isExternalChannel` is optional on the `Adapter` interface, so these don't need explicit stubs +- The `Chat.createThread()` fallback (`adapter.isExternalChannel?.(threadId) ?? false`) handles the default + +### 7. Mock adapter (`packages/chat/src/mock-adapter.ts`) + +- Add `isExternalChannel` mock returning `false` (same pattern as `isDM`) + +### 8. Tests + +- Serialization round-trip tests for `isExternalChannel` on Thread +- Channel inheritance tests (`thread.channel.isExternalChannel`) +- Backward compatibility test (missing `isExternalChannel` in JSON defaults to `false`) + +## Key Files Modified + +1. `packages/chat/src/types.ts` - Core interfaces +2. `packages/chat/src/thread.ts` - ThreadImpl +3. `packages/chat/src/channel.ts` - ChannelImpl +4. `packages/chat/src/chat.ts` - Thread creation +5. `packages/chat/src/mock-adapter.ts` - Test mock +6. `packages/adapter-slack/src/index.ts` - Slack implementation (main one) +7. `packages/chat/src/serialization.test.ts` - Serialization tests +8. `packages/chat/src/channel.test.ts` - Channel tests diff --git a/packages/adapter-slack/src/index.ts b/packages/adapter-slack/src/index.ts index 2a1ce9fb..78331df7 100644 --- a/packages/adapter-slack/src/index.ts +++ b/packages/adapter-slack/src/index.ts @@ -201,6 +201,8 @@ interface SlackWebhookPayload { | SlackAppHomeOpenedEvent; event_id?: string; event_time?: number; + /** Whether this event occurred in an externally shared channel (Slack Connect) */ + is_ext_shared_channel?: boolean; team_id?: string; type: string; } @@ -300,6 +302,12 @@ export class SlackAdapter implements Adapter { private readonly formatConverter = new SlackFormatConverter(); private static USER_CACHE_TTL_MS = 60 * 60 * 1000; // 1 hour + /** + * Cache of channel IDs known to be external/shared (Slack Connect). + * Populated from `is_ext_shared_channel` in incoming webhook payloads. + */ + private readonly _externalChannels = new Set(); + // Multi-workspace support private readonly clientId: string | undefined; private readonly clientSecret: string | undefined; @@ -308,6 +316,7 @@ export class SlackAdapter implements Adapter { private readonly requestContext = new AsyncLocalStorage<{ token: string; botUserId?: string; + isExtSharedChannel?: boolean; }>(); /** Bot user ID (e.g., U_BOT_123) used for mention detection */ @@ -742,6 +751,19 @@ export class SlackAdapter implements Adapter { if (payload.type === "event_callback" && payload.event) { const event = payload.event; + // Track external/shared channel status from payload-level flag + if (payload.is_ext_shared_channel) { + let channelId: string | undefined; + if ("channel" in event) { + channelId = (event as SlackEvent).channel; + } else if ("item" in event) { + channelId = (event as SlackReactionEvent).item.channel; + } + if (channelId) { + this._externalChannels.add(channelId); + } + } + if (event.type === "message" || event.type === "app_mention") { const slackEvent = event as SlackEvent; if (!(slackEvent.team || slackEvent.team_id) && payload.team_id) { @@ -2373,7 +2395,17 @@ export class SlackAdapter implements Adapter { const result = await this.client.conversations.info( this.withToken({ channel }) ); - const channelInfo = result.channel as { name?: string } | undefined; + const channelInfo = result.channel as + | { + name?: string; + is_ext_shared?: boolean; + } + | undefined; + + // Update external channel cache from API response + if (channelInfo?.is_ext_shared) { + this._externalChannels.add(channel); + } this.logger.debug("Slack API: conversations.info response", { channelName: channelInfo?.name, @@ -2384,6 +2416,7 @@ export class SlackAdapter implements Adapter { id: threadId, channelId: channel, channelName: channelInfo?.name, + isExternalChannel: channelInfo?.is_ext_shared ?? false, metadata: { threadTs, channel: result.channel, @@ -2439,6 +2472,16 @@ export class SlackAdapter implements Adapter { return channel.startsWith("D"); } + /** + * Check if a thread is in an external/shared channel (Slack Connect). + * Uses the `is_ext_shared_channel` flag from incoming webhook payloads, + * cached per channel ID. + */ + isExternalChannel(threadId: string): boolean { + const { channel } = this.decodeThreadId(threadId); + return this._externalChannels.has(channel); + } + decodeThreadId(threadId: string): SlackThreadId { const parts = threadId.split(":"); if (parts.length < 2 || parts.length > 3 || parts[0] !== "slack") { @@ -2750,15 +2793,22 @@ export class SlackAdapter implements Adapter { name?: string; is_im?: boolean; is_mpim?: boolean; + is_ext_shared?: boolean; num_members?: number; purpose?: { value?: string }; topic?: { value?: string }; }; + // Update external channel cache from API response + if (info?.is_ext_shared) { + this._externalChannels.add(channel); + } + return { id: channelId, name: info?.name ? `#${info.name}` : undefined, isDM: Boolean(info?.is_im || info?.is_mpim), + isExternalChannel: info?.is_ext_shared ?? false, memberCount: info?.num_members, metadata: { purpose: info?.purpose?.value, diff --git a/packages/chat/src/channel.test.ts b/packages/chat/src/channel.test.ts index d3ef8989..e13c44c5 100644 --- a/packages/chat/src/channel.test.ts +++ b/packages/chat/src/channel.test.ts @@ -565,6 +565,35 @@ describe("thread.channel", () => { expect(thread.channel.isDM).toBe(true); }); + + it("should inherit isExternalChannel from thread", () => { + const mockAdapter = createMockAdapter(); + const mockState = createMockState(); + + const thread = new ThreadImpl({ + id: "slack:C123:1234.5678", + adapter: mockAdapter, + channelId: "C123", + stateAdapter: mockState, + isExternalChannel: true, + }); + + expect(thread.channel.isExternalChannel).toBe(true); + }); + + it("should default isExternalChannel to false", () => { + const mockAdapter = createMockAdapter(); + const mockState = createMockState(); + + const thread = new ThreadImpl({ + id: "slack:C123:1234.5678", + adapter: mockAdapter, + channelId: "C123", + stateAdapter: mockState, + }); + + expect(thread.channel.isExternalChannel).toBe(false); + }); }); describe("thread.messages (newest first)", () => { diff --git a/packages/chat/src/channel.ts b/packages/chat/src/channel.ts index c5833346..16ac7562 100644 --- a/packages/chat/src/channel.ts +++ b/packages/chat/src/channel.ts @@ -36,6 +36,7 @@ export interface SerializedChannel { adapterName: string; id: string; isDM: boolean; + isExternalChannel?: boolean; } /** @@ -45,6 +46,7 @@ interface ChannelImplConfigWithAdapter { adapter: Adapter; id: string; isDM?: boolean; + isExternalChannel?: boolean; stateAdapter: StateAdapter; } @@ -55,6 +57,7 @@ interface ChannelImplConfigLazy { adapterName: string; id: string; isDM?: boolean; + isExternalChannel?: boolean; } type ChannelImplConfig = ChannelImplConfigWithAdapter | ChannelImplConfigLazy; @@ -79,6 +82,7 @@ export class ChannelImpl> { readonly id: string; readonly isDM: boolean; + readonly isExternalChannel: boolean; private _adapter?: Adapter; private readonly _adapterName?: string; @@ -88,6 +92,7 @@ export class ChannelImpl> constructor(config: ChannelImplConfig) { this.id = config.id; this.isDM = config.isDM ?? false; + this.isExternalChannel = config.isExternalChannel ?? false; if (isLazyConfig(config)) { this._adapterName = config.adapterName; @@ -333,6 +338,7 @@ export class ChannelImpl> id: this.id, adapterName: this.adapter.name, isDM: this.isDM, + ...(this.isExternalChannel ? { isExternalChannel: true } : {}), }; } @@ -344,6 +350,7 @@ export class ChannelImpl> id: json.id, adapterName: json.adapterName, isDM: json.isDM, + isExternalChannel: json.isExternalChannel, }); if (adapter) { channel._adapter = adapter; diff --git a/packages/chat/src/chat.ts b/packages/chat/src/chat.ts index d0a32347..356ebb2f 100644 --- a/packages/chat/src/chat.ts +++ b/packages/chat/src/chat.ts @@ -1576,6 +1576,9 @@ export class Chat< // Check if this is a DM const isDM = adapter.isDM?.(threadId) ?? false; + // Check if this is an external/shared channel (e.g., Slack Connect) + const isExternalChannel = adapter.isExternalChannel?.(threadId) ?? false; + return new ThreadImpl({ id: threadId, adapter, @@ -1584,6 +1587,7 @@ export class Chat< initialMessage, isSubscribedContext, isDM, + isExternalChannel, currentMessage: initialMessage, streamingUpdateIntervalMs: this._streamingUpdateIntervalMs, }); diff --git a/packages/chat/src/mock-adapter.ts b/packages/chat/src/mock-adapter.ts index 6d8f7578..a32e601a 100644 --- a/packages/chat/src/mock-adapter.ts +++ b/packages/chat/src/mock-adapter.ts @@ -68,6 +68,7 @@ export function createMockAdapter(name = "slack"): Adapter { isDM: vi .fn() .mockImplementation((threadId: string) => threadId.includes(":D")), + isExternalChannel: vi.fn().mockReturnValue(false), openModal: vi.fn().mockResolvedValue({ viewId: "V123" }), channelIdFromThreadId: vi .fn() diff --git a/packages/chat/src/serialization.test.ts b/packages/chat/src/serialization.test.ts index 9598844f..2020fcec 100644 --- a/packages/chat/src/serialization.test.ts +++ b/packages/chat/src/serialization.test.ts @@ -54,6 +54,41 @@ describe("Serialization", () => { expect(json.isDM).toBe(true); }); + it("should serialize external channel thread correctly", () => { + const mockAdapter = createMockAdapter("slack"); + const mockState = createMockState(); + + const thread = new ThreadImpl({ + id: "slack:C123:1234.5678", + adapter: mockAdapter, + channelId: "C123", + stateAdapter: mockState, + isExternalChannel: true, + }); + + const json = thread.toJSON(); + + expect(json._type).toBe("chat:Thread"); + expect(json.isExternalChannel).toBe(true); + }); + + it("should omit isExternalChannel when false", () => { + const mockAdapter = createMockAdapter("slack"); + const mockState = createMockState(); + + const thread = new ThreadImpl({ + id: "slack:C123:1234.5678", + adapter: mockAdapter, + channelId: "C123", + stateAdapter: mockState, + isExternalChannel: false, + }); + + const json = thread.toJSON(); + + expect(json.isExternalChannel).toBeUndefined(); + }); + it("should produce JSON-serializable output", () => { const mockAdapter = createMockAdapter("teams"); const mockState = createMockState(); @@ -163,6 +198,37 @@ describe("Serialization", () => { expect(restored.adapter.name).toBe(original.adapter.name); }); + it("should round-trip isExternalChannel correctly", () => { + const mockAdapter = createMockAdapter("slack"); + + const original = new ThreadImpl({ + id: "slack:C123:1234.5678", + adapter: mockAdapter, + channelId: "C123", + stateAdapter: mockState, + isExternalChannel: true, + }); + + const json = original.toJSON(); + const restored = ThreadImpl.fromJSON(json); + + expect(restored.isExternalChannel).toBe(true); + }); + + it("should default isExternalChannel to false when missing from JSON", () => { + const json: SerializedThread = { + _type: "chat:Thread", + id: "slack:C123:1234.5678", + channelId: "C123", + isDM: false, + adapterName: "slack", + }; + + const thread = ThreadImpl.fromJSON(json); + + expect(thread.isExternalChannel).toBe(false); + }); + it("should serialize currentMessage", () => { const mockAdapter = createMockAdapter("slack"); const currentMessage = createTestMessage("msg-1", "Hello", { diff --git a/packages/chat/src/thread.ts b/packages/chat/src/thread.ts index c490ed29..54064f9a 100644 --- a/packages/chat/src/thread.ts +++ b/packages/chat/src/thread.ts @@ -38,6 +38,7 @@ export interface SerializedThread { currentMessage?: SerializedMessage; id: string; isDM: boolean; + isExternalChannel?: boolean; } /** @@ -50,6 +51,7 @@ interface ThreadImplConfigWithAdapter { id: string; initialMessage?: Message; isDM?: boolean; + isExternalChannel?: boolean; isSubscribedContext?: boolean; stateAdapter: StateAdapter; streamingUpdateIntervalMs?: number; @@ -66,6 +68,7 @@ interface ThreadImplConfigLazy { id: string; initialMessage?: Message; isDM?: boolean; + isExternalChannel?: boolean; isSubscribedContext?: boolean; streamingUpdateIntervalMs?: number; } @@ -96,6 +99,7 @@ export class ThreadImpl> readonly id: string; readonly channelId: string; readonly isDM: boolean; + readonly isExternalChannel: boolean; /** Direct adapter instance (if provided) */ private _adapter?: Adapter; @@ -116,6 +120,7 @@ export class ThreadImpl> this.id = config.id; this.channelId = config.channelId; this.isDM = config.isDM ?? false; + this.isExternalChannel = config.isExternalChannel ?? false; this._isSubscribedContext = config.isSubscribedContext ?? false; this._currentMessage = config.currentMessage; this._streamingUpdateIntervalMs = config.streamingUpdateIntervalMs ?? 500; @@ -227,6 +232,7 @@ export class ThreadImpl> adapter: this.adapter, stateAdapter: this._stateAdapter, isDM: this.isDM, + isExternalChannel: this.isExternalChannel, }); } return this._channel; @@ -552,6 +558,7 @@ export class ThreadImpl> channelId: this.channelId, currentMessage: this._currentMessage?.toJSON(), isDM: this.isDM, + ...(this.isExternalChannel ? { isExternalChannel: true } : {}), adapterName: this.adapter.name, }; } @@ -582,6 +589,7 @@ export class ThreadImpl> ? Message.fromJSON(json.currentMessage) : undefined, isDM: json.isDM, + isExternalChannel: json.isExternalChannel, }); if (adapter) { thread._adapter = adapter; diff --git a/packages/chat/src/types.ts b/packages/chat/src/types.ts index 4e135703..0370362a 100644 --- a/packages/chat/src/types.ts +++ b/packages/chat/src/types.ts @@ -195,6 +195,17 @@ export interface Adapter { */ isDM?(threadId: string): boolean; + /** + * Check if a thread is in an external/shared channel (e.g., Slack Connect). + * + * External channels are shared between different organizations. Bots should + * be careful about what information they expose in external channels. + * + * @param threadId - The thread ID to check + * @returns True if the thread is in an external channel, false otherwise + */ + isExternalChannel?(threadId: string): boolean; + /** * List threads in a channel. */ @@ -506,6 +517,8 @@ export interface Postable< readonly id: string; /** Whether this is a direct message conversation */ readonly isDM: boolean; + /** Whether this is an external/shared channel (e.g., Slack Connect) */ + readonly isExternalChannel: boolean; /** * Get a platform-specific mention string for a user. @@ -600,6 +613,8 @@ export interface ThreadSummary { export interface ChannelInfo { id: string; isDM?: boolean; + /** Whether this is an external/shared channel (e.g., Slack Connect) */ + isExternalChannel?: boolean; memberCount?: number; metadata: Record; name?: string; @@ -797,6 +812,8 @@ export interface ThreadInfo { id: string; /** Whether this is a direct message conversation */ isDM?: boolean; + /** Whether this is an external/shared channel (e.g., Slack Connect) */ + isExternalChannel?: boolean; /** Platform-specific metadata */ metadata: Record; }