From f0523afd65425854d47206e47cb991f499ae948f Mon Sep 17 00:00:00 2001 From: David Cramer Date: Sat, 9 May 2026 16:18:52 -0700 Subject: [PATCH] ref(chat): Enforce service Slack boundary Move Slack-backed conversation image and backfill details out of domain services so services depend on small injected ports instead of Slack infrastructure. Add an architecture guard and update the contract docs to keep the boundary enforceable. Co-Authored-By: GPT-5 Codex --- AGENTS.md | 1 + packages/junior/.dependency-cruiser.mjs | 12 ++ packages/junior/src/chat/app/services.ts | 5 +- .../junior/src/chat/runtime/reply-executor.ts | 4 +- .../src/chat/runtime/turn-preparation.ts | 138 +++++++++++++++++- .../src/chat/services/conversation-memory.ts | 125 +--------------- .../src/chat/services/vision-context.ts | 43 ++++-- .../slack/bot-image-hydration.test.ts | 32 ++-- specs/chat-architecture-spec.md | 4 +- 9 files changed, 194 insertions(+), 170 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ff61c576..bc7f358c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -79,6 +79,7 @@ Co-Authored-By: (agent model name) - Do not add mutable runtime behavior globals or test-only singleton mutation APIs (`set*ForTests`, `reset*ForTests`, observer globals for core behavior). - Prefer small consumer-owned service interfaces over broad deps bags or service locators. - Do not leak third-party SDK types across chat subsystem boundaries when a small local interface will do; keep vendor SDKs inside infrastructure modules. +- Service modules must depend on small injected ports for Slack behavior, not import Slack infrastructure directly. - `runtime/` orchestrates turns and turn-scoped formatting, `services/` do domain work (reply policy, delivery planning, channel intent, attachment validation), `state/` persists by concern, `ingress/` only normalizes/routes. - **Feature-based colocation**: group files by domain feature, not by technical role. Within a module, create subdirectories for each feature domain (e.g., `tools/slack/`, `tools/web/`, `tools/sandbox/`, `tools/skill/`). Shared contracts and cross-cutting utilities live at the module root. Only extract to a shared location when 2+ features need the same code. - Do not use barrel `index.ts` re-exports inside feature subdirectories — import directly from the source file. A module-root `index.ts` is acceptable as a composition root that wires features together. diff --git a/packages/junior/.dependency-cruiser.mjs b/packages/junior/.dependency-cruiser.mjs index fd379ce1..88201419 100644 --- a/packages/junior/.dependency-cruiser.mjs +++ b/packages/junior/.dependency-cruiser.mjs @@ -24,6 +24,18 @@ export default { path: "^src/chat/runtime/", }, }, + { + name: "no-chat-services-to-slack", + comment: + "Service modules must depend on small injected ports, not Slack infrastructure.", + severity: "error", + from: { + path: "^src/chat/services/", + }, + to: { + path: "^src/chat/slack/", + }, + }, { name: "no-chat-state-to-runtime", comment: "State modules must not depend on runtime orchestration.", diff --git a/packages/junior/src/chat/app/services.ts b/packages/junior/src/chat/app/services.ts index 0eb152be..9a7440fe 100644 --- a/packages/junior/src/chat/app/services.ts +++ b/packages/junior/src/chat/app/services.ts @@ -45,9 +45,8 @@ export function createJuniorRuntimeServices( completeText: overrides.visionContext?.completeText ?? completeText, listThreadReplies: overrides.visionContext?.listThreadReplies ?? listThreadReplies, - downloadPrivateSlackFile: - overrides.visionContext?.downloadPrivateSlackFile ?? - downloadPrivateSlackFile, + downloadFile: + overrides.visionContext?.downloadFile ?? downloadPrivateSlackFile, }); return { diff --git a/packages/junior/src/chat/runtime/reply-executor.ts b/packages/junior/src/chat/runtime/reply-executor.ts index ee6cac82..5606a5e3 100644 --- a/packages/junior/src/chat/runtime/reply-executor.ts +++ b/packages/junior/src/chat/runtime/reply-executor.ts @@ -35,7 +35,7 @@ import { import { completeAuthPauseTurn } from "@/chat/runtime/auth-pause-state"; import type { PreparedTurnState } from "@/chat/runtime/turn-preparation"; import { - generateThreadTitle, + type ConversationMemoryService, markConversationMessage, normalizeConversationText, upsertConversationMessage, @@ -68,7 +68,7 @@ import { export interface ReplyExecutorServices { generateAssistantReply: typeof generateAssistantReplyImpl; - generateThreadTitle: typeof generateThreadTitle; + generateThreadTitle: ConversationMemoryService["generateThreadTitle"]; lookupSlackUser: typeof lookupSlackUser; scheduleTurnTimeoutResume: ( request: TurnTimeoutResumeRequest, diff --git a/packages/junior/src/chat/runtime/turn-preparation.ts b/packages/junior/src/chat/runtime/turn-preparation.ts index b2659632..3ef0e6c0 100644 --- a/packages/junior/src/chat/runtime/turn-preparation.ts +++ b/packages/junior/src/chat/runtime/turn-preparation.ts @@ -13,22 +13,22 @@ import { type ThreadArtifactsState, } from "@/chat/state/artifacts"; import { - compactConversationIfNeeded, buildConversationContext, isHumanConversationMessage, normalizeConversationText, - seedConversationBackfill, + updateConversationStats, upsertConversationMessage, } from "@/chat/services/conversation-memory"; import { countPotentialImageAttachments, hasPotentialImageAttachment, - hydrateConversationVisionContext, isVisionEnabled, } from "@/chat/services/vision-context"; import { getChannelConfigurationService } from "@/chat/runtime/thread-state"; import type { ChannelConfigurationService } from "@/chat/configuration/types"; +const BACKFILL_MESSAGE_LIMIT = 80; + export interface PreparedTurnState { artifacts: ThreadArtifactsState; configuration?: Record; @@ -42,8 +42,25 @@ export interface PreparedTurnState { } export interface PrepareTurnStateDeps { - compactConversationIfNeeded: typeof compactConversationIfNeeded; - hydrateConversationVisionContext: typeof hydrateConversationVisionContext; + compactConversationIfNeeded: ( + conversation: ThreadConversationState, + context: { + threadId?: string; + channelId?: string; + requesterId?: string; + runId?: string; + }, + ) => Promise; + hydrateConversationVisionContext: ( + conversation: ThreadConversationState, + context: { + threadId?: string; + channelId?: string; + requesterId?: string; + runId?: string; + threadTs?: string; + }, + ) => Promise; } function hasPendingImageHydration( @@ -55,6 +72,117 @@ function hasPendingImageHydration( ); } +function createConversationMessageFromSdkMessage( + entry: Message, +): ConversationMessage | null { + const rawText = normalizeConversationText(entry.text); + if (!rawText) { + return null; + } + + return { + id: entry.id, + role: entry.author.isMe ? "assistant" : "user", + text: rawText, + createdAtMs: entry.metadata.dateSent.getTime(), + author: { + userId: entry.author.userId, + userName: entry.author.userName, + fullName: entry.author.fullName, + isBot: + typeof entry.author.isBot === "boolean" + ? entry.author.isBot + : undefined, + }, + meta: { + slackTs: getSlackMessageTs(entry), + }, + }; +} + +async function seedConversationBackfill( + thread: Thread, + conversation: ThreadConversationState, + currentTurn: { + messageId: string; + messageCreatedAtMs: number; + }, +): Promise { + if (conversation.backfill.completedAtMs) { + return; + } + if (conversation.messages.length > 0 || conversation.compactions.length > 0) { + conversation.backfill = { + completedAtMs: Date.now(), + source: "recent_messages", + }; + updateConversationStats(conversation); + return; + } + + const seeded: ConversationMessage[] = []; + let source: "recent_messages" | "thread_fetch" = "recent_messages"; + + try { + const fetchedNewestFirst: Message[] = []; + for await (const entry of thread.messages) { + fetchedNewestFirst.push(entry); + if (fetchedNewestFirst.length >= BACKFILL_MESSAGE_LIMIT) { + break; + } + } + fetchedNewestFirst.reverse(); + for (const entry of fetchedNewestFirst) { + const message = createConversationMessageFromSdkMessage(entry); + if (message) { + seeded.push(message); + } + } + if (seeded.length > 0) { + source = "thread_fetch"; + } + } catch {} + + if (seeded.length === 0) { + try { + await thread.refresh(); + } catch {} + + const fromRecent = thread.recentMessages.slice(-BACKFILL_MESSAGE_LIMIT); + for (const entry of fromRecent) { + const message = createConversationMessageFromSdkMessage(entry); + if (message) { + seeded.push(message); + } + } + source = "recent_messages"; + } + + for (const message of seeded) { + if ( + message.id !== currentTurn.messageId && + message.createdAtMs > currentTurn.messageCreatedAtMs + ) { + continue; + } + if ( + message.id !== currentTurn.messageId && + message.createdAtMs === currentTurn.messageCreatedAtMs && + message.id > currentTurn.messageId + ) { + continue; + } + upsertConversationMessage(conversation, message); + } + + conversation.backfill = { + completedAtMs: Date.now(), + source, + }; + updateConversationStats(conversation); +} + +/** Build the turn-state preparer from injected conversation services. */ export function createPrepareTurnState(deps: PrepareTurnStateDeps) { return async function prepareTurnState(args: { explicitMention: boolean; diff --git a/packages/junior/src/chat/services/conversation-memory.ts b/packages/junior/src/chat/services/conversation-memory.ts index 60f6b78e..2c33d53f 100644 --- a/packages/junior/src/chat/services/conversation-memory.ts +++ b/packages/junior/src/chat/services/conversation-memory.ts @@ -1,7 +1,5 @@ -import type { Message, Thread } from "chat"; import { botConfig } from "@/chat/config"; -import { completeText } from "@/chat/pi/client"; -import { getSlackMessageTs } from "@/chat/slack/message"; +import type { completeText } from "@/chat/pi/client"; import type { ConversationCompaction, ConversationMessage, @@ -17,7 +15,6 @@ const CONTEXT_MIN_LIVE_MESSAGES = 12; const CONTEXT_COMPACTION_BATCH_SIZE = 24; const CONTEXT_MAX_COMPACTIONS = 16; const CONTEXT_MAX_MESSAGE_CHARS = 3200; -const BACKFILL_MESSAGE_LIMIT = 80; export interface ConversationMemoryDeps { completeText: typeof completeText; @@ -417,6 +414,7 @@ async function compactConversationIfNeededWithDeps( } } +/** Build the service that owns durable conversation memory compaction and titles. */ export function createConversationMemoryService( deps: ConversationMemoryDeps, ): ConversationMemoryService { @@ -428,125 +426,6 @@ export function createConversationMemoryService( }; } -const defaultConversationMemoryService = createConversationMemoryService({ - completeText, -}); - -export const compactConversationIfNeeded = - defaultConversationMemoryService.compactConversationIfNeeded; -export const generateThreadTitle = - defaultConversationMemoryService.generateThreadTitle; - -function createConversationMessageFromSdkMessage( - entry: Message, -): ConversationMessage | null { - const rawText = normalizeConversationText(entry.text); - if (!rawText) { - return null; - } - - return { - id: entry.id, - role: entry.author.isMe ? "assistant" : "user", - text: rawText, - createdAtMs: entry.metadata.dateSent.getTime(), - author: { - userId: entry.author.userId, - userName: entry.author.userName, - fullName: entry.author.fullName, - isBot: - typeof entry.author.isBot === "boolean" - ? entry.author.isBot - : undefined, - }, - meta: { - slackTs: getSlackMessageTs(entry), - }, - }; -} - -export async function seedConversationBackfill( - thread: Thread, - conversation: ThreadConversationState, - currentTurn: { - messageId: string; - messageCreatedAtMs: number; - }, -): Promise { - if (conversation.backfill.completedAtMs) { - return; - } - if (conversation.messages.length > 0 || conversation.compactions.length > 0) { - conversation.backfill = { - completedAtMs: Date.now(), - source: "recent_messages", - }; - updateConversationStats(conversation); - return; - } - - const seeded: ConversationMessage[] = []; - let source: "recent_messages" | "thread_fetch" = "recent_messages"; - - try { - const fetchedNewestFirst: Message[] = []; - for await (const entry of thread.messages) { - fetchedNewestFirst.push(entry); - if (fetchedNewestFirst.length >= BACKFILL_MESSAGE_LIMIT) { - break; - } - } - fetchedNewestFirst.reverse(); - for (const entry of fetchedNewestFirst) { - const message = createConversationMessageFromSdkMessage(entry); - if (message) { - seeded.push(message); - } - } - if (seeded.length > 0) { - source = "thread_fetch"; - } - } catch {} - - if (seeded.length === 0) { - try { - await thread.refresh(); - } catch {} - - const fromRecent = thread.recentMessages.slice(-BACKFILL_MESSAGE_LIMIT); - for (const entry of fromRecent) { - const message = createConversationMessageFromSdkMessage(entry); - if (message) { - seeded.push(message); - } - } - source = "recent_messages"; - } - - for (const message of seeded) { - if ( - message.id !== currentTurn.messageId && - message.createdAtMs > currentTurn.messageCreatedAtMs - ) { - continue; - } - if ( - message.id !== currentTurn.messageId && - message.createdAtMs === currentTurn.messageCreatedAtMs && - message.id > currentTurn.messageId - ) { - continue; - } - upsertConversationMessage(conversation, message); - } - - conversation.backfill = { - completedAtMs: Date.now(), - source, - }; - updateConversationStats(conversation); -} - export function isHumanConversationMessage( message: ConversationMessage, ): boolean { diff --git a/packages/junior/src/chat/services/vision-context.ts b/packages/junior/src/chat/services/vision-context.ts index d2f05869..59b9faa6 100644 --- a/packages/junior/src/chat/services/vision-context.ts +++ b/packages/junior/src/chat/services/vision-context.ts @@ -1,11 +1,9 @@ import type { Attachment } from "chat"; import { botConfig } from "@/chat/config"; -import { completeText } from "@/chat/pi/client"; +import type { completeText } from "@/chat/pi/client"; import type { ThreadConversationState } from "@/chat/state/conversation"; import { toOptionalString } from "@/chat/coerce"; import { logInfo, logWarn } from "@/chat/logging"; -import { listThreadReplies } from "@/chat/slack/channel"; -import { downloadPrivateSlackFile } from "@/chat/slack/client"; import { getConversationMessageSlackTs, isHumanConversationMessage, @@ -19,6 +17,22 @@ export interface UserInputAttachment { promptText?: string; } +interface VisionThreadFile { + id?: string; + mimetype?: string; + name?: string; + size?: number; + url_private?: string; + url_private_download?: string; +} + +interface VisionThreadReply { + ts?: string; + subtype?: string; + bot_id?: string; + files?: VisionThreadFile[]; +} + const MAX_USER_ATTACHMENTS = 3; const MAX_USER_ATTACHMENT_BYTES = 5 * 1024 * 1024; const MAX_MESSAGE_IMAGE_ATTACHMENTS = 3; @@ -26,8 +40,14 @@ const MAX_VISION_SUMMARY_CHARS = 500; export interface VisionContextDeps { completeText: typeof completeText; - downloadPrivateSlackFile: typeof downloadPrivateSlackFile; - listThreadReplies: typeof listThreadReplies; + downloadFile: (url: string) => Promise; + listThreadReplies: (input: { + channelId: string; + threadTs: string; + limit?: number; + maxPages?: number; + targetMessageTs?: string[]; + }) => Promise; } export interface VisionContextService { @@ -457,7 +477,7 @@ async function hydrateConversationVisionContextWithDeps( return; } - let replies: Awaited>; + let replies: VisionThreadReply[]; try { replies = await deps.listThreadReplies({ channelId: context.channelId, @@ -579,7 +599,7 @@ async function hydrateConversationVisionContextWithDeps( let imageData: Buffer; try { - imageData = await deps.downloadPrivateSlackFile(downloadUrl); + imageData = await deps.downloadFile(downloadUrl); } catch (error) { logWarn( "conversation_image_download_failed", @@ -695,12 +715,3 @@ export function createVisionContextService( ), }; } - -const defaultVisionContextService = createVisionContextService({ - completeText, - downloadPrivateSlackFile, - listThreadReplies, -}); - -export const hydrateConversationVisionContext = - defaultVisionContextService.hydrateConversationVisionContext; diff --git a/packages/junior/tests/integration/slack/bot-image-hydration.test.ts b/packages/junior/tests/integration/slack/bot-image-hydration.test.ts index 811826e1..86414285 100644 --- a/packages/junior/tests/integration/slack/bot-image-hydration.test.ts +++ b/packages/junior/tests/integration/slack/bot-image-hydration.test.ts @@ -322,9 +322,7 @@ describe("bot image hydration", () => { ], }, ]); - const downloadPrivateSlackFileMock = vi.fn(async () => - Buffer.from("downloaded-image"), - ); + const downloadFileMock = vi.fn(async () => Buffer.from("downloaded-image")); const completeTextMock = vi.fn(async () => ({ text: "Recovered screenshot context", message: {} as never, @@ -335,7 +333,7 @@ describe("bot image hydration", () => { services: { visionContext: { listThreadReplies: listThreadRepliesMock, - downloadPrivateSlackFile: downloadPrivateSlackFileMock, + downloadFile: downloadFileMock, completeText: completeTextMock, }, replyExecutor: { @@ -370,7 +368,7 @@ describe("bot image hydration", () => { ); expect(listThreadRepliesMock).toHaveBeenCalledTimes(1); - expect(downloadPrivateSlackFileMock).toHaveBeenCalledTimes(1); + expect(downloadFileMock).toHaveBeenCalledTimes(1); expect(completeTextMock).toHaveBeenCalledTimes(1); const persistedState = secondThread.getState() as { conversation: { @@ -418,9 +416,7 @@ describe("bot image hydration", () => { ], }, ]); - const downloadPrivateSlackFileMock = vi.fn(async () => - Buffer.from("downloaded-image"), - ); + const downloadFileMock = vi.fn(async () => Buffer.from("downloaded-image")); const completeTextMock = vi.fn(async () => ({ text: "Passive screenshot summary", message: {} as never, @@ -446,7 +442,7 @@ describe("bot image hydration", () => { }, visionContext: { listThreadReplies: listThreadRepliesMock, - downloadPrivateSlackFile: downloadPrivateSlackFileMock, + downloadFile: downloadFileMock, completeText: completeTextMock, }, replyExecutor: { @@ -529,7 +525,7 @@ describe("bot image hydration", () => { ); expect(listThreadRepliesMock).toHaveBeenCalledTimes(1); - expect(downloadPrivateSlackFileMock).toHaveBeenCalledTimes(1); + expect(downloadFileMock).toHaveBeenCalledTimes(1); expect(completeTextMock).toHaveBeenCalledTimes(1); expect(generateAssistantReply).toHaveBeenCalledTimes(1); @@ -575,9 +571,7 @@ describe("bot image hydration", () => { ], }, ]); - const downloadPrivateSlackFileMock = vi.fn(async () => - Buffer.from("downloaded-image"), - ); + const downloadFileMock = vi.fn(async () => Buffer.from("downloaded-image")); const completeTextMock = vi.fn(async () => ({ text: "Current screenshot summary", message: {} as never, @@ -601,7 +595,7 @@ describe("bot image hydration", () => { services: { visionContext: { listThreadReplies: listThreadRepliesMock, - downloadPrivateSlackFile: downloadPrivateSlackFileMock, + downloadFile: downloadFileMock, completeText: completeTextMock, }, replyExecutor: { @@ -662,7 +656,7 @@ describe("bot image hydration", () => { }), ); - expect(downloadPrivateSlackFileMock).toHaveBeenCalledTimes(1); + expect(downloadFileMock).toHaveBeenCalledTimes(1); expect(completeTextMock).toHaveBeenCalledTimes(1); expect(attachmentFetch).not.toHaveBeenCalled(); expect(generateAssistantReply).toHaveBeenCalledTimes(1); @@ -686,9 +680,7 @@ describe("bot image hydration", () => { ], }, ]); - const downloadPrivateSlackFileMock = vi.fn(async () => - Buffer.from("downloaded-image"), - ); + const downloadFileMock = vi.fn(async () => Buffer.from("downloaded-image")); let completeTextCallCount = 0; const completeTextMock = vi.fn(async () => { completeTextCallCount += 1; @@ -734,7 +726,7 @@ describe("bot image hydration", () => { services: { visionContext: { listThreadReplies: listThreadRepliesMock, - downloadPrivateSlackFile: downloadPrivateSlackFileMock, + downloadFile: downloadFileMock, completeText: completeTextMock, }, replyExecutor: { @@ -801,7 +793,7 @@ describe("bot image hydration", () => { }), ); - expect(downloadPrivateSlackFileMock).toHaveBeenCalledTimes(2); + expect(downloadFileMock).toHaveBeenCalledTimes(2); expect(completeTextMock).toHaveBeenCalledTimes(3); expect(firstAttachmentFetch).toHaveBeenCalledTimes(1); expect(secondAttachmentFetch).not.toHaveBeenCalled(); diff --git a/specs/chat-architecture-spec.md b/specs/chat-architecture-spec.md index c460ffd6..bc7b1146 100644 --- a/specs/chat-architecture-spec.md +++ b/specs/chat-architecture-spec.md @@ -3,7 +3,7 @@ ## Metadata - Created: 2026-03-21 -- Last Edited: 2026-03-22 +- Last Edited: 2026-05-09 ## Changelog @@ -14,6 +14,7 @@ - 2026-03-22: Moved canonical ingress routing and chat bindings under `chat/ingress/*` and deleted the old `chat-background-patch.ts` module. - 2026-03-22: Removed test-only plugin registry mutation APIs and made plugin discovery reload from the current root signature instead of import-time state. - 2026-03-22: Replaced prototype-patch ingress with the explicit `JuniorChat` subclass, removed the OAuth resume post-message observer global, split queue transport from queue retry policy, and replaced the ambient user-token-store singleton with construction at the call site. +- 2026-05-09: Added the enforced service-to-Slack boundary: domain services depend on injected Slack-backed ports instead of Slack infrastructure modules. ## Status @@ -64,6 +65,7 @@ Define the normative architecture contract for `packages/junior/src/chat` so new - Non-`app/` modules must not import from `app/`. - `runtime/` may depend on `services/`, `state/`, `queue/`, `turn/`, `slack/`, and pure helpers. - `services/` must not depend on `runtime/`. +- `services/` must not import Slack infrastructure; use small injected ports owned by the service when a domain service needs Slack-backed data or files. - `state/` must not depend on `runtime/` or service modules. - `ingress/` may route into queue/runtime entrypoints, but must not own business logic that belongs in `runtime/` or `services/`.