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/`.