refactor(agents-api): decompose Agent.ts into domain-specific modules#2470
refactor(agents-api): decompose Agent.ts into domain-specific modules#2470tim-inkeep wants to merge 22 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
🦋 Changeset detectedLatest commit: 5886d9a The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Ito Test Report ✅27 test cases ran. 27 passed. This test run validated PR #2470, which refactors the agents-api module by decomposing the monolithic Key verification areas included:
✅ Passed (27)
📋 View Recording |
|
@pullfrog, can you review this PR and tell me what you think? are there places that should be merged or redundancies I should look into? |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid decomposition overall. The monolithic Agent.ts is now a clean facade, and the file relocations into domain directories (session/, stream/, artifacts/, compression/) are consistent with no stale files left behind.
That said, there are a few structural redundancies and issues worth addressing before merging:
1. Circular dependency between generate.ts ↔ system-prompt.ts — both import from each other. Extract getRelationTools/createRelationToolName into a standalone module to break the cycle.
2. Duplicated setup logic in runGenerate — setupGenerationContext() already sets ctx.conversationId and calls updateArtifactComponents, but lines 407-414 repeat the same work.
3. ~80 lines of copy-pasted tool-approval logic between function-tools.ts and mcp-tools.ts — this includes parseEmbeddedJson, approval waiting, UI bus publishing, tracing spans, and denial handling. Should be extracted into a shared helper.
4. Stale vi.mock() paths in 2 test files that weren't fully updated during the move (details in inline comments and body notes below).
5. Two streaming directories (agents/streaming/ and run/stream/) — the agents/streaming/ directory has only ~60 lines of glue code across 2 files. Consider inlining into generation/generate.ts or adding a comment clarifying the boundary.
6. ToolSessionManager placement — it's in agents/services/ but collaborates closely with AgentSession and the other session managers in run/session/. Consider co-locating.
Not in diff (body-only notes):
ArtifactParser.typeSchema.test.tshas two stale mock paths:vi.mock('../../agents/ToolSessionManager')should bevi.mock('../../agents/services/ToolSessionManager'), andvi.mock('../AgentSession')should bevi.mock('../../session/AgentSession'). This file wasn't touched in the PR but the modules it mocks were moved.session/__tests__/AgentSession.test.tsline 54:vi.mock('../../utils/stream-registry.js')should bevi.mock('../../stream/stream-registry.js')— the import at line 8 was updated but the mock was missed.
| import { getFunctionTools } from '../tools/function-tools'; | ||
| import { getMcpTools } from '../tools/mcp-tools'; | ||
| import type { SystemPromptV1 } from '../types'; | ||
| import { getRelationTools } from './generate'; |
There was a problem hiding this comment.
Circular dependency: system-prompt.ts imports getRelationTools from ../generation/generate, while generate.ts imports buildSystemPrompt from ./system-prompt. This creates a cycle: generate.ts → system-prompt.ts → generate.ts.
Node resolves this at runtime for named exports, but it's fragile. Extract getRelationTools and createRelationToolName into a standalone module (e.g., generation/relation-tools.ts) so both generate.ts and system-prompt.ts can import from it without a cycle.
| if (streamRequestId && ctx.artifactComponents.length > 0) { | ||
| agentSessionManager.updateArtifactComponents(streamRequestId, ctx.artifactComponents); | ||
| } | ||
| const conversationId = runtimeContext?.metadata?.conversationId; | ||
|
|
||
| if (conversationId) { | ||
| ctx.conversationId = conversationId; | ||
| } |
There was a problem hiding this comment.
Duplicated setup logic: These 8 lines repeat work already performed by setupGenerationContext() (called at line 395). Specifically:
agentSessionManager.updateArtifactComponents(streamRequestId, ctx.artifactComponents)is already called insetupGenerationContextat lines 130-132ctx.conversationId = conversationIdis already set insetupGenerationContextat lines 134-137
Remove this block — it's a copy-paste leftover from the extraction that runs the same side effects twice.
| } | ||
| } | ||
|
|
||
| const { getMaxGenerationSteps } = await import('./model-config'); |
There was a problem hiding this comment.
Unnecessary dynamic import: await import('./model-config') for getMaxGenerationSteps, but this file already statically imports from ./model-config at line 6 (getSummarizerModel). Add getMaxGenerationSteps to the existing static import instead.
| 'Sub-agent reached maximum generation steps limit' | ||
| ); | ||
|
|
||
| const { tracer } = await import('../../utils/tracer'); |
There was a problem hiding this comment.
Same issue — await import('../../utils/tracer') should be a static import at the top of the file. tracer is a singleton with no circular dependency concern. The dynamic import adds overhead on every invocation.
| if (!contextConfig) { | ||
| logger.warn({ contextConfigId: ctx.config.contextConfigId }, 'Context config not found'); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Dead code: if (!contextConfig) is already checked and returns at lines 31-34. This second check at lines 44-47 is unreachable.
| if (conversationId) { | ||
| ctx.conversationId = conversationId; | ||
| } |
There was a problem hiding this comment.
Surprising side effect: buildSystemPrompt mutates ctx.conversationId — unexpected for a function named "build system prompt." This same assignment also happens in setupGenerationContext and in the duplicated block in runGenerate (lines 407-414). Consider removing the mutation from here and making the caller responsible for setting ctx.conversationId before calling buildSystemPrompt.
| { toolName, toolCallId, error: (error as Error).message }, | ||
| 'Failed to parse embedded JSON, using original args' | ||
| ); | ||
| processedArgs = args; |
There was a problem hiding this comment.
Major duplication: The entire tool-approval flow spanning ~120 lines here (approval waiting, UI bus publish, tracing spans, denial handling, parseEmbeddedJson) is nearly identical to the same block in function-tools.ts (~lines 97-243). This is a maintenance hazard — changes to approval logic would need to be mirrored in both places.
Consider extracting a shared handleToolApproval(ctx, toolName, toolCallId, args, providerMetadata) helper into tool-wrapper.ts or a new tool-approval.ts that both modules call.
| export function getRelationshipIdForTool( | ||
| ctx: AgentRunContext, | ||
| toolName: string, | ||
| toolType?: ToolType | ||
| ): string | undefined { | ||
| if (toolType === 'mcp') { | ||
| const matchingTool = ctx.config.tools?.find((tool) => { | ||
| if (tool.config?.type !== 'mcp') { | ||
| return false; | ||
| } | ||
|
|
||
| if (tool.availableTools?.some((available) => available.name === toolName)) { | ||
| return true; | ||
| } | ||
|
|
||
| if (tool.config.mcp.activeTools?.includes(toolName)) { | ||
| return true; | ||
| } | ||
|
|
||
| return tool.name === toolName; | ||
| }); | ||
|
|
||
| return matchingTool?.relationshipId; | ||
| } | ||
|
|
||
| if (toolType === 'tool') { | ||
| return ctx.functionToolRelationshipIdByName.get(toolName); | ||
| } | ||
|
|
||
| if (toolType === 'delegation') { | ||
| const relation = ctx.config.delegateRelations.find( | ||
| (relation) => | ||
| `delegate_to_${relation.config.id.toLowerCase().replace(/\s+/g, '_')}` === toolName | ||
| ); | ||
|
|
||
| return relation?.config.relationId; | ||
| } | ||
| } |
There was a problem hiding this comment.
getRelationshipIdForTool is a pure lookup against AgentRunContext config data with no dependency on streaming or tool-wrapping. It's also used by AgentMcpManager (via constructor callback). Consider moving it to agent-types.ts where ToolType and AgentRunContext already live — this better expresses that it's a context utility, not a wrapper concern.
|
@pullfrog, can you review again and tell me what you think? |
There was a problem hiding this comment.
Improved further since last review. The double-prompt concern (issue #2 from last review) is genuinely resolved — the comments at system-prompt.ts:222-223 and 312-314 accurately describe two distinct data sources (corePrompt = sub-agent instructions, prompt = overarching agent system prompt). Stale test mocks are all fixed. The streaming handler no longer mutates the SDK response.
One remaining substantive issue:
Duplicate tool loading per generation — loadToolsAndPrompts loads tools for execution (MCP, function, relation, default) in parallel with buildSystemPrompt, which internally re-fetches the same three tool sets (system-prompt.ts:244-246) solely for prompt metadata. Every generation request makes 2x MCP calls, 2x function tool DB queries, and 2x relation tool computation. The fix is to load tools once and pass the metadata into buildSystemPrompt.
Additionally, one behavioral issue in the tool-approval flow, and several nits around type safety and dead code in the streaming layer. All detailed in inline comments.
|
Acknowledged @tim-inkeep's feedback on the The remaining 5 inline comments from the latest review (duplicate tool loading, |
|
Addressed both review feedback items from @tim-inkeep:
Both threads resolved. Typecheck passes. |
|
activity timeout: no output for 300s |
…erationResponse in stream-handler
|
Error agent completed without reporting progress |
|
Leaping into action... |
|
Decomposes the monolithic Key changes:
|
There was a problem hiding this comment.
Low urgency — clean decomposition, but a few correctness issues to address before merge.
The refactoring is architecturally sound: the module graph is acyclic, the Agent.ts barrel re-exports everything consumers need, and AgentRunContext correctly carries all fields. Two issues need fixes: (1) a parameter ordering bug in system-prompt.ts's fallback tool-loading path, and (2) duplicate hasAgentArtifactComponents logic with divergent error-handling. The rest is minor type-safety improvements and a pre-existing test mock path bug worth fixing while these paths are being touched.
| const rawSchema = toolDef.inputSchema as Record<string, unknown>; | ||
| let inputSchema: ReturnType<typeof z.fromJSONSchema> | ReturnType<typeof jsonSchema>; | ||
| try { | ||
| inputSchema = override.schema | ||
| ? z.fromJSONSchema(override.schema) | ||
| : (toolDef as any).inputSchema; | ||
| inputSchema = override.schema ? z.fromJSONSchema(override.schema) : jsonSchema(rawSchema); |
There was a problem hiding this comment.
toolDef.inputSchema is typed as unknown on AiSdkToolDefinition, but cast to Record<string, unknown> without a guard. If inputSchema is undefined (which the type allows), jsonSchema(undefined as any) will throw.
Add a fallback:
const rawSchema = (toolDef.inputSchema ?? {}) as Record<string, unknown>;Also worth verifying: previously the non-override path used (toolDef as any).inputSchema raw. Now jsonSchema(rawSchema) produces an AI SDK wrapper that disables argument validation by default (unlike Zod schemas). If MCP tools previously relied on Zod validation from the raw schema, this is a behavioral change. If inputSchema is always a plain JSON Schema object here (likely), this is fine.
|
|
||
| // Mock stream helpers | ||
| vi.mock('../../domains/run/utils/stream-helpers.js', () => ({ | ||
| vi.mock('../../domains/run/stream/stream-helpers.js', () => ({ |
There was a problem hiding this comment.
Pre-existing bug carried forward: this test is at __tests__/run/routes/webhooks.test.ts, so ../../domains/run/stream/stream-helpers.js resolves to __tests__/domains/run/stream/... which doesn't exist. Should be ../../../domains/run/stream/stream-helpers.js (three ../ levels), matching the pattern in chat.test.ts line 73. Since this line is being touched anyway, worth fixing now.
| vi.mock('../../domains/run/stream/stream-helpers.js', () => ({ | |
| vi.mock('../../../domains/run/stream/stream-helpers.js', () => ({ |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(5) Total Issues | Risk: Low
This is a well-executed structural refactoring that successfully decomposes a monolithic 3400-line Agent.ts into focused, domain-specific modules. The module graph is acyclic, the facade pattern is correctly implemented, and all test mock paths have been properly updated. The issues identified are minor correctness and consistency improvements — no blocking concerns.
🟠⚠️ Major (2) 🟠⚠️
🟠 1) tool-utils.ts:32-39 Missing transfer tool type handling
Issue: getRelationshipIdForTool handles 'mcp', 'tool', and 'delegation' types but has no case for 'transfer', despite ToolType including 'transfer' (agent-types.ts:194) and relation-tools.ts:39 passing 'transfer' to wrapToolWithStreaming.
Why: Transfer tool relationship IDs will always return undefined, causing incomplete telemetry tracking and session recording for transfer tool calls. This is a behavioral gap, not just missing code.
Fix: Add a case for toolType === 'transfer' that looks up the relation in ctx.config.transferRelations using the same pattern as delegation.
Refs:
🟠 2) stream-handler.ts:67-73 Error handler loses context when error is not an Error instance
Issue: The error event handler extracts event.error?.error?.message but if that path doesn't exist, it throws new Error(errorMessage) where errorMessage is undefined, producing unhelpful "Error: undefined" messages.
Why: AI SDK errors can have different shapes. This code path may produce generic errors that are impossible to debug in production, especially for edge cases like network failures or malformed responses.
Fix: Add fallback error message extraction: errorObj?.error?.message ?? errorObj?.message ?? String(event.error) ?? 'Stream encountered an unknown error'
Refs:
Inline Comments:
- 🟠 Major:
tool-utils.ts:39Missing transfer tool type handling - 🟠 Major:
stream-handler.ts:73Error handler loses error context
🟡 Minor (3) 🟡
🟡 1) AgentMcpManager.ts:20 Import AgentConfig from agent-types.ts for consistency
Issue: AgentConfig is imported from ../Agent while AiSdkToolDefinition is imported from ../agent-types. Since AgentConfig is defined in agent-types.ts and merely re-exported by Agent.ts, this creates inconsistent import patterns within the same file.
Why: All other files in tools/ and generation/ subdirectories consistently import types from ../agent-types. Aligning this improves consistency and clarifies the canonical type source.
Fix: Change to import type { AgentConfig, AiSdkToolDefinition } from '../agent-types';
Refs:
🟡 2) default-tools.ts:125-126 Redundant null check on createLoadSkillTool
Issue: The if (loadSkillTool) check can never fail — createLoadSkillTool(ctx) always returns a Tool<...> object via the tool() factory.
Why: This is dead code that adds unnecessary complexity and may mislead future maintainers into thinking the function can return null.
Fix: Remove the inner conditional and call wrapToolWithStreaming directly.
Refs:
🟡 3) relation-tools.ts:23 Use ToolSet type for return type consistency
Issue: getRelationTools returns Record<string, any> instead of ToolSet type used by the other tool-loading functions (getMcpTools, getFunctionTools, getDefaultTools).
Why: Inconsistent return types make it harder to compose tools and lose type information at call sites.
Fix: Change return type to ToolSet from the ai package.
Refs:
Inline Comments:
- 🟡 Minor:
AgentMcpManager.ts:20Import consistency - 🟡 Minor:
default-tools.ts:135Redundant null check - 🟡 Minor:
relation-tools.ts:23Use ToolSet type
💭 Consider (1) 💭
💭 1) agent-types.ts:196 Replace any with unknown in type guard
Issue: The isValidTool type guard uses any for the input parameter, allowing callers to pass arbitrary values without compile-time checks.
Why: Using unknown would force explicit narrowing at call sites, catching potential type mismatches earlier.
Fix: Change parameter type from any to unknown.
Inline Comments:
- 💭 Consider:
agent-types.ts:196Use unknown instead of any
🕐 Pending Recommendations (3)
- 🟠
tool-loading.tsDuplicate tool loading per generation — tools loaded inloadToolsAndPromptsare re-fetched insidebuildSystemPrompt(prior review by pullfrog) - 🟠
system-prompt.tsParameter ordering bug in fallback tool-loading path (prior review by pullfrog) - 🟡
default-tools.ts+system-prompt.tsDuplicatehasAgentArtifactComponentslogic with divergent error handling (prior review by pullfrog)
💡 APPROVE WITH SUGGESTIONS
Summary: This is a solid refactoring that successfully decomposes a monolithic file into well-organized modules. The architecture is sound — clean module boundaries, proper facade pattern, correct dependency direction, and all test paths properly updated. The 2 Major issues (missing transfer tool handling, error context loss) are worth addressing, but they're localized fixes that don't require rethinking the decomposition. The Minor issues and prior pending recommendations are polish items. Ship it after addressing the Majors. 🚀
Discarded (8)
| Location | Issue | Reason Discarded |
|---|---|---|
compression.ts:11-17 |
any types in setupCompression |
Pre-existing pattern from original Agent.ts; out of scope for this refactor |
agent-types.ts:44-51 |
any in hasToolCallWithPrefix |
Pre-existing code moved without modification |
agent-types.ts:70-72 |
any in ResolvedGenerationResponse |
Pre-existing type definition |
model-config.ts:85-90 |
any return in configureModelSettings |
Pre-existing pattern; would require broader typing changes |
function-tools.ts:89 |
any in execute context |
Addressed in Consider section via isValidTool suggestion |
mcp-tools.ts:81 |
any in execute context |
Same as above |
tool-wrapper.ts:77 |
any in wrapped execute |
Same as above |
agents/streaming/ directory |
Two streaming directories | Valid architectural choice; agents/streaming handles agent-specific orchestration, run/stream provides reusable primitives |
Reviewers (6)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
4 | 2 | 0 | 0 | 2 | 0 | 0 |
pr-review-architecture |
6 | 0 | 0 | 0 | 1 | 0 | 5 |
pr-review-types |
9 | 1 | 1 | 0 | 2 | 0 | 5 |
pr-review-precision |
5 | 0 | 0 | 0 | 1 | 3 | 1 |
pr-review-tests |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-consistency |
6 | 0 | 0 | 0 | 0 | 0 | 6 |
| Total | 30 | 3 | 1 | 0 | 6 | 3 | 17 |
Note: pr-review-tests confirmed all mock paths are correctly updated — no findings needed.
| ); | ||
|
|
||
| return relation?.config.relationId; | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR Missing transfer tool type handling
Issue: getRelationshipIdForTool handles 'mcp', 'tool', and 'delegation' types but has no case for 'transfer', despite ToolType including 'transfer' and relation-tools.ts:39 passing 'transfer' to wrapToolWithStreaming.
Why: Transfer tool relationship IDs will always return undefined, causing incomplete telemetry tracking and session recording for transfer tool calls.
Fix:
| } | |
| if (toolType === 'delegation') { | |
| const relation = ctx.config.delegateRelations.find( | |
| (relation) => | |
| `delegate_to_${relation.config.id.toLowerCase().replace(/\s+/g, '_')}` === toolName | |
| ); | |
| return relation?.config.relationId; | |
| } | |
| if (toolType === 'transfer') { | |
| const relation = ctx.config.transferRelations?.find( | |
| (relation) => | |
| `transfer_to_${relation.id.toLowerCase().replace(/\s+/g, '_')}` === toolName | |
| ); | |
| return relation?.relationId; | |
| } |
Refs:
| } | ||
| const errorMessage = (event.error as { error?: { message?: string } })?.error?.message; | ||
| throw new Error(errorMessage); | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR Error handler loses context when error is not an Error instance
Issue: The error case extracts event.error?.error?.message but if that path doesn't exist, it throws new Error(undefined) which provides no debugging information.
Why: AI SDK errors can have different shapes. This code path may produce generic "Error: undefined" messages that are impossible to debug in production.
Fix:
| } | |
| case 'error': { | |
| if (event.error instanceof Error) { | |
| throw event.error; | |
| } | |
| const errorObj = event.error as { error?: { message?: string }; message?: string } | undefined; | |
| const errorMessage = errorObj?.error?.message ?? errorObj?.message ?? String(event.error); | |
| throw new Error(errorMessage || 'Stream encountered an unknown error'); | |
| } |
Refs:
| import { getLogger } from '../../../../logger'; | ||
| import { agentSessionManager } from '../../session/AgentSession'; | ||
| import { setSpanWithError, tracer } from '../../utils/tracer'; | ||
| import type { AgentConfig } from '../Agent'; |
There was a problem hiding this comment.
🟡 Minor Import AgentConfig from agent-types.ts for consistency
Issue: AgentConfig is imported from ../Agent while AiSdkToolDefinition is imported from ../agent-types. Since AgentConfig is defined in agent-types.ts and merely re-exported by Agent.ts, this creates inconsistent import patterns.
Why: All other files in tools/ and generation/ subdirectories consistently import types from ../agent-types. This improves consistency and clarifies the canonical type source.
Fix:
| import type { AgentConfig } from '../Agent'; | |
| import type { AgentConfig, AiSdkToolDefinition } from '../agent-types'; |
Refs:
| 'tool' | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Minor Redundant null check on createLoadSkillTool return value
Issue: The if (loadSkillTool) check at line 126 can never fail — createLoadSkillTool(ctx) always returns a Tool<...> object via the tool() factory (see lines 80-110). The function signature declares a return type of Tool<...>, not Tool<...> | null.
Why: This is dead code that adds unnecessary complexity. The check may have been carried over from a previous implementation or added defensively without verifying the function's contract.
Fix:
| } | |
| const hasOnDemandSkills = ctx.config.skills?.some((skill) => !skill.alwaysLoaded); | |
| if (hasOnDemandSkills) { | |
| defaultTools.load_skill = wrapToolWithStreaming( | |
| ctx, | |
| 'load_skill', | |
| createLoadSkillTool(ctx), | |
| streamRequestId, | |
| 'tool' | |
| ); | |
| } |
Refs:
| }; | ||
| }, | ||
| sessionId?: string | ||
| ): Record<string, any> { |
There was a problem hiding this comment.
🟡 Minor Use ToolSet type for consistency
Issue: getRelationTools returns Record<string, any> instead of ToolSet type used by the other tool-loading functions (getMcpTools, getFunctionTools, getDefaultTools).
Why: Inconsistent return types make it harder to compose tools and lose type information at call sites.
Fix: Change return type to ToolSet:
import type { ToolSet } from 'ai';
export function getRelationTools(
ctx: AgentRunContext,
runtimeContext?: { ... },
sessionId?: string
): ToolSet {Refs:
|
|
||
| export type ToolType = 'transfer' | 'delegation' | 'mcp' | 'tool'; | ||
|
|
||
| export function isValidTool(tool: any): tool is { |
There was a problem hiding this comment.
💭 Consider Replace any with unknown in type guard
Issue: The isValidTool type guard uses any for the input parameter. Using unknown would provide better type safety by forcing explicit narrowing.
Why: With any, callers can pass arbitrary values without compile-time checks. unknown would catch type mismatches at call sites.
Fix: Change signature to:
export function isValidTool(tool: unknown): tool is {
description: string;
inputSchema: Record<string, unknown>;
execute: (args: unknown, context?: unknown) => Promise<unknown>;
}Refs:
2 participants
Agent.ts Refactor — Technical Spec
Status: Draft
Branch:
feature/agent-refactorLast updated: 2026-03-04
Compared against:
main1. Problem Statement
Agent.tsonmainwas a 3,392-line monolith that bundled every concern of agent execution into a single class:This made the file:
thisand private methods, making unit testing require instantiating the fullAgentclassthis.config,this.streamHelper,this.conversationId, etc.) was mutated across many methods with no clear ownership boundary2. Goals
Agent.tsto a thin orchestrator with a single public surfaceAgentRunContext) that flows through modules instead of living onthisAgent— no breaking changes to callers3. Non-Goals
Agentclass's external interface (constructor signature, public methods)4. Core Refactoring Strategy
4.1 The
AgentRunContextObjectThe central design decision is the introduction of
AgentRunContext(defined inagent-types.ts).Before: All execution state was stored as private fields on the
Agentclass instance:After: All execution state is assembled into a plain
AgentRunContextobject in the constructor, then passed explicitly into every module function:The
Agentclass now holds onlyprivate ctx: AgentRunContext. All methods either delegate directly to a module function (passingthis.ctx), or expose a getter/setter that reads/writes a single field onctx.Why this matters: Module functions can now be pure(-ish) functions that take
ctxas their first argument. They don't need the class — they need the context. This makes them testable without instantiatingAgent.4.2 Module Decomposition
The 3,392-line monolith was split into a two-level directory structure. Every module has a single, named concern.
agent-types.ts(239 lines)All shared types, interfaces, and two pure utility functions that were previously inlined in
Agent.ts:AgentConfigAgent.ts(type export)AgentRunContextDelegateRelation,ExternalAgentRelationConfig,TeamAgentRelationConfigAgent.tsToolType,AiSdkContentPart,AiSdkTextPart,AiSdkImagePart,AiSdkToolDefinitionAgent.tsResolvedGenerationResponseAgent.ts(with detailed JSDoc)hasToolCallWithPrefix()Agent.ts(private method)resolveGenerationResponse()Agent.ts(exported function)validateModel()Agent.ts(private method)isValidTool()Agent.ts(private function)generation/— The Generation Pipeline (9 modules)This is the heart of the refactor. The
generate()method onmainwas a single function of ~400 lines. It is now an orchestrated pipeline where each stage is a named function in its own module.The pipeline flow in
generate.ts → runGenerate():Every stage is now independently readable. You can understand step 4 (model config) without touching step 6 (compression).
tools/— Tool Loading and Wrapping (7 modules)Previously,
sanitizeToolsForAISDK,#getRelationshipIdForTool, and#createRelationToolNamewere private class methods. They are now standalone exported functions that takectxas input, making them directly unit-testable.streaming/— Stream Handling (2 modules)The streaming logic (
for await (const event of fullStream)) was previously a large inline block insidegenerate(). It is now its own module.services/— Long-lived Managers (2 modules, relocated)These were already class-based services. They were moved into
services/to make theagents/directory flat structure cleaner.4.3 Run-domain Directory Reorganization
Beyond the
agents/subtree, several utility classes were promoted to first-class domain directories:agents-api/src/domains/run/utils/stream-helpers.tsrun/stream/stream-helpers.tsagents-api/src/domains/run/utils/stream-registry.tsrun/stream/stream-registry.tsagents-api/src/domains/run/services/IncrementalStreamParser.tsrun/stream/IncrementalStreamParser.tsagents-api/src/domains/run/services/ResponseFormatter.tsrun/stream/ResponseFormatter.tsagents-api/src/domains/run/services/AgentSession.tsrun/session/AgentSession.tsagents-api/src/domains/run/services/PendingToolApprovalManager.tsrun/session/PendingToolApprovalManager.tsagents-api/src/domains/run/services/ToolApprovalUiBus.tsrun/session/ToolApprovalUiBus.tsagents-api/src/domains/run/services/BaseCompressor.tsrun/compression/BaseCompressor.tsagents-api/src/domains/run/services/ConversationCompressor.tsrun/compression/ConversationCompressor.tsagents-api/src/domains/run/services/MidGenerationCompressor.tsrun/compression/MidGenerationCompressor.tsagents-api/src/domains/run/utils/artifact-component-schema.tsrun/artifacts/artifact-component-schema.tsagents-api/src/domains/run/utils/artifact-utils.tsrun/artifacts/artifact-utils.tsThe result is a
run/domain organized by subdomain (stream/,session/,compression/,artifacts/) rather than a flatutils/andservices/bucket.5. The Agent Class After Refactor (211 lines)
The
Agentclass is now a thin facade. Its entire job is:AgentRunContextfrom the input parametersstreamRequestIdandmcpManageras pass-throughs toctxThe re-exports at the bottom of
Agent.tspreserve backward compatibility for any importers that were consuming types or functions fromAgent.tsdirectly:6. Key Design Decisions
D1: Plain context object over class inheritance / mixins
Decision: Use
AgentRunContextas a passed plain object, not a base class or mixin.Rationale: Avoids the complexity of
super()chains and prototype coupling. Module functions that takectxare easy to test by constructing a minimal partial context. Class hierarchy would have made testing harder.D2: Mutable context fields (
streamRequestId,conversationId, etc.) remain on the objectDecision:
AgentRunContexthas mutable fields (written bysetupGenerationContextand external setters).Rationale: These fields represent per-request state that is set before
generate()is called and then read throughout the pipeline. Passing them as separate function arguments through every call frame would require refactoring every function signature — high noise, no gain. The context object is the right home for request-scoped state.D3:
Agentclass retained (not replaced with plain functions)Decision: Keep
Agentas a class, not convert to a factory function + module exports.Rationale: External callers (
executionHandler.ts,generateTaskHandler.ts, etc.) already instantiateAgentwithnew. Changing to a factory function would require updating every call site without changing behavior. The class shell is preserved; only its innards changed.D4: Re-export types from
Agent.tsfor backward compatibilityDecision: Types moved to
agent-types.tsare re-exported fromAgent.ts.Rationale: External code imports types from
'./Agent'or'../agents/Agent'. Moving the types without re-exporting would break those imports silently in TypeScript if not caught bypnpm check.D5: Parallel tool loading in
loadToolsAndPromptsDecision: All four tool sources (MCP, function, relation, default) are fetched with
Promise.all.Rationale: This was already the behavior on
main— the refactor preserved it explicitly by wiring the parallel fetch throughtool-loading.ts. Making it explicit makes the intent visible rather than buried.7. Impact Surface
Files changed on this branch (run-domain only)
Core refactor targets:
agents/Agent.ts— 3,392 → 211 lines (−94%)agents/agent-types.ts— NEW (239 lines, extracted types + utilities)New modules (generation pipeline):
agents/generation/generate.tsagents/generation/compression.tsagents/generation/conversation-history.tsagents/generation/model-config.tsagents/generation/response-formatting.tsagents/generation/schema-builder.tsagents/generation/system-prompt.tsagents/generation/tool-loading.tsagents/generation/tool-result.tsNew modules (tools):
agents/tools/function-tools.tsagents/tools/mcp-tools.tsagents/tools/relation-tools.tsagents/tools/default-tools.tsagents/tools/tool-approval.tsagents/tools/tool-utils.tsagents/tools/tool-wrapper.tsNew modules (streaming):
agents/streaming/stream-handler.tsagents/streaming/stream-parser.tsRelocated services:
agents/services/AgentMcpManager.ts(wasagents/AgentMcpManager.ts)agents/services/ToolSessionManager.ts(wasagents/ToolSessionManager.ts)Promoted run-domain directories (renamed/moved, not rewritten):
run/stream/(fromrun/utils/+run/services/)run/session/(fromrun/services/)run/compression/(fromrun/services/)run/artifacts/(fromrun/utils/)No changes to:
Agentpublic interface (constructor,generate,getFunctionTools,getRelationTools,cleanup)generateTaskHandler.tscall patternsexecutionHandler.tscall patterns8. Test Coverage
All test files were updated to reflect new import paths. The refactor preserves all existing test cases. The new module structure enables future unit tests to test individual pipeline stages (
buildSystemPrompt,configureModelSettings,setupCompression, etc.) without instantiating a fullAgent.Relevant test files updated:
agents-api/src/__tests__/run/agents/Agent.test.tsagents-api/src/__tests__/run/agents/AgentMcpManager.test.tsagents-api/src/__tests__/run/agents/functionToolApprovals.test.tsagents-api/src/__tests__/run/agents/generateTaskHandler.test.tsagents-api/src/__tests__/run/agents/relationTools.test.tsagents-api/src/__tests__/run/stream/(new location)agents-api/src/__tests__/run/session/(new location)agents-api/src/__tests__/run/compression/(new location)9. Verification
Run
pnpm checkfrom the monorepo root. All of the following must pass:pnpm typecheck— confirms import paths and type compatibility after extractionpnpm lint— confirms no new lint violations from the restructuringpnpm test— confirms no regressions in agent generation, streaming, compression, or tool handlingEvidence: File Inventory
Agent.ts line counts
Diff stats (Agent.ts only)
All files changed on this branch (git diff main...HEAD --name-only)
agents-api/src/domains/run/agents/Agent.ts
agents-api/src/domains/run/agents/agent-types.ts (NEW)
agents-api/src/domains/run/agents/generateTaskHandler.ts
agents-api/src/domains/run/agents/generation/compression.ts (NEW)
agents-api/src/domains/run/agents/generation/conversation-history.ts (NEW)
agents-api/src/domains/run/agents/generation/generate.ts (NEW)
agents-api/src/domains/run/agents/generation/model-config.ts (NEW)
agents-api/src/domains/run/agents/generation/response-formatting.ts (NEW)
agents-api/src/domains/run/agents/generation/schema-builder.ts (NEW)
agents-api/src/domains/run/agents/generation/system-prompt.ts (NEW)
agents-api/src/domains/run/agents/generation/tool-loading.ts (NEW)
agents-api/src/domains/run/agents/generation/tool-result.ts (NEW)
agents-api/src/domains/run/agents/relationTools.ts
agents-api/src/domains/run/agents/services/AgentMcpManager.ts (relocated)
agents-api/src/domains/run/agents/services/ToolSessionManager.ts (relocated)
agents-api/src/domains/run/agents/streaming/stream-handler.ts (NEW)
agents-api/src/domains/run/agents/streaming/stream-parser.ts (NEW)
agents-api/src/domains/run/agents/tools/default-tools.ts (NEW)
agents-api/src/domains/run/agents/tools/function-tools.ts (NEW)
agents-api/src/domains/run/agents/tools/mcp-tools.ts (NEW)
agents-api/src/domains/run/agents/tools/relation-tools.ts (NEW)
agents-api/src/domains/run/agents/tools/tool-approval.ts (NEW)
agents-api/src/domains/run/agents/tools/tool-utils.ts (NEW)
agents-api/src/domains/run/agents/tools/tool-wrapper.ts (NEW)
agents-api/src/domains/run/artifacts/ArtifactParser.ts
agents-api/src/domains/run/artifacts/ArtifactService.ts
agents-api/src/domains/run/artifacts/artifact-component-schema.ts (relocated)
agents-api/src/domains/run/artifacts/artifact-utils.ts (relocated)
agents-api/src/domains/run/compression/BaseCompressor.ts (relocated)
agents-api/src/domains/run/compression/ConversationCompressor.ts (relocated)
agents-api/src/domains/run/compression/MidGenerationCompressor.ts (relocated)
agents-api/src/domains/run/session/AgentSession.ts (relocated)
agents-api/src/domains/run/session/PendingToolApprovalManager.ts (relocated)
agents-api/src/domains/run/session/ToolApprovalUiBus.ts (relocated)
agents-api/src/domains/run/stream/IncrementalStreamParser.ts (relocated)
agents-api/src/domains/run/stream/ResponseFormatter.ts (relocated)
agents-api/src/domains/run/stream/stream-helpers.ts (relocated)
agents-api/src/domains/run/stream/stream-registry.ts (relocated)
(+ test files and route files reflecting updated import paths)
AgentRunContext fields (agent-types.ts:222-238)
config, executionContext, mcpManager, contextResolver,
credentialStoreRegistry, credentialStuffer, systemPromptBuilder,
streamHelper, streamRequestId, conversationId, delegationId,
isDelegatedAgent, artifactComponents, currentCompressor,
functionToolRelationshipIdByName