feat: Implement agent tooling features and hooks for enhanced functionality #173
Conversation
…enhanced functionality
…enhanced functionality
📝 WalkthroughWalkthroughAdds a complete Agent subsystem and E2E demo: agent metadata/decorators, LLM adapter framework, execution loop (blocking + streaming), registry, flows/hooks, agent-scoped runtime, error types, transport/tool integration, demo agents, docs, extensive tests, and supporting configs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Scope
participant Flow as "CallAgentFlow"
participant Registry as "AgentRegistry"
participant Instance as "AgentInstance"
participant LLM as "LLMAdapter"
participant ToolExec as "ToolExecutor"
Client->>Scope: invoke_<agent> tool request
Scope->>Flow: run 'agents:call-agent'
Flow->>Registry: locate agent by tool name/id
Registry-->>Flow: AgentInstance
Flow->>Instance: create context & execute(input)
Instance->>LLM: completion(prompt, tools)
LLM-->>Instance: response (content or tool_call)
alt response contains tool_call
Instance->>ToolExec: execute(toolName, args)
ToolExec-->>Instance: tool result
Instance->>LLM: completion(prompt + tool result)
LLM-->>Instance: response
end
Instance-->>Flow: final validated result
Flow-->>Scope: MCP-formatted response
Scope-->>Client: return tool-call response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Comment |
|
| Status | Scanner |  Critical |
 High |
 Medium |
 Low |
Total (0) |
|---|---|---|---|---|---|---|
| Open Source Security | 0 | 0 | 0 | 0 | See details |
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/sdk/src/resource/__tests__/resource.registry.test.ts (1)
26-26: Avoidas anyin mock setup.The mock object has well-defined properties and could be properly typed instead of using
as any, which bypasses TypeScript's type safety.🔎 Proposed fix to properly type the mock
+import type { ProviderRegistry } from '../../providers/provider.registry'; + const createMockProviderRegistry = () => { const mockScope = createMockScope(); return { getActiveScope: jest.fn().mockReturnValue(mockScope), get: jest.fn().mockReturnValue(undefined), getRegistries: jest.fn().mockReturnValue([]), addRegistry: jest.fn(), - } as any; + } as unknown as ProviderRegistry; };As per coding guidelines, avoid
anytypes without strong justification and useunknowninstead for generic type defaults.
♻️ Duplicate comments (1)
libs/sdk/src/common/migrate/__tests__/auth-transport.migrate.test.ts (1)
10-14: Duplicate test helper - consolidate into shared utility.This is an exact duplicate of the helper in
transport.options.test.ts(lines 11-15). See the review comment on that file for consolidation recommendations.
🟠 Major comments (9)
libs/sdk/src/resource/__tests__/resource.instance.test.ts-7-10 (1)
7-10: Type annotation doesn't match the intended behavior.The function signature shows
text?: string; blob?: string(optional properties on the same object), but the comment indicates handling a "text/blob union" (discriminated union). The'text' in contentcheck is appropriate for discriminated unions but won't correctly narrow optional properties—the key exists in the type definition even when the value is undefined.If the actual runtime type is a discriminated union, update the type annotation to:
content: { uri: string; text: string } | { uri: string; blob: string }If both properties can truly be optional on the same object, change the implementation to:
return content.text;🔎 Proposed fix assuming discriminated union
// Helper to safely access content properties (handles text/blob union) -function getContentText(content: { uri: string; text?: string; blob?: string }): string | undefined { +function getContentText(content: { uri: string; text: string } | { uri: string; blob: string }): string | undefined { return 'text' in content ? content.text : undefined; }libs/sdk/src/__test-utils__/fixtures/scope.fixtures.ts-42-52 (1)
42-52: UsePartial<Scope>instead ofanyfor better type safety.The
mockScopeis typed asany, which bypasses TypeScript's type checking. UsePartial<Scope>to maintain type safety while allowing a partial mock implementation.As per coding guidelines, avoid
anytypes without strong justification.🔎 Proposed fix using Partial<Scope>
- const mockScope: any = { + const mockScope: Partial<Scope> = { id: 'test-scope', logger: mockLogger, hooks: mockHookRegistry, providers: mockProviders, registryFlows: jest.fn().mockResolvedValue(undefined), metadata: { id: 'test-scope', http: { port: 3001 }, }, };Note: You may need to adjust the type of
mockProvidersto match the expected type ofScope['providers']to satisfy TypeScript. If the providers property expects aProviderRegistry, consider typing mockProviders more specifically.libs/sdk/src/__test-utils__/fixtures/scope.fixtures.ts-29-40 (1)
29-40: Replaceanywithunknownfor stricter type safety.The
tokenparameter on line 31 usesanywithout strong justification, violating the strict TypeScript guidelines for this codebase. Useunknowninstead and add proper type guards for the Scope token check.As per coding guidelines, avoid
anytypes in strict TypeScript mode.🔎 Proposed fix using `unknown` with type guards
const mockProviders = { getHooksRegistry: jest.fn().mockReturnValue(mockHookRegistry), - get: jest.fn().mockImplementation((token: any) => { + get: jest.fn().mockImplementation((token: unknown) => { if (token === Scope || token?.name === 'Scope') { // Return self-reference for Scope return mockScope; } return undefined; }), tryGet: jest.fn().mockReturnValue(undefined), getActiveScope: jest.fn().mockReturnValue(undefined), };For the type guard on line 32, consider adding a more explicit check:
if (token === Scope || (typeof token === 'function' && token.name === 'Scope'))libs/sdk/src/common/types/options/__tests__/redis.options.test.ts-5-9 (1)
5-9: Third duplicate of test helper - consolidate immediately.This is the third duplicate of this helper function across the test suite. This file also has a slightly different type signature (
redis: RedisOptions | undefinedvsredis: unknown), which introduces inconsistency.The duplication across three files makes this an essential refactor. Please consolidate into a shared test utility as suggested in the review of
transport.options.test.ts(lines 11-15).libs/sdk/src/common/types/options/__tests__/transport.options.test.ts-11-15 (1)
11-15: Consolidate the duplicated test helper into a shared utility.This helper is duplicated across at least three test files (transport.options.test.ts, auth-transport.migrate.test.ts, and redis.options.test.ts). Additionally, the use of type assertions to bypass union type checking suggests a deeper type design issue.
Consider:
- Extract to shared test utility: Move this helper to a common test utility file (e.g.,
libs/sdk/src/common/__test-utils__/redis-helpers.ts) and import it where needed.- Improve type safety: Instead of using type assertions, consider proper type guards or discriminated unions if RedisOptions truly is a union with Vercel KV types. The helper signature
(redis: unknown, key: K): unknownloses all type information.🔎 Example of a shared test utility approach
Create
libs/sdk/src/common/__test-utils__/redis-helpers.ts:// Helper to safely access redis properties (handles union type with Vercel KV) export function getRedisProperty<K extends string>( redis: unknown, key: K ): unknown { if (!redis || typeof redis !== 'object') return undefined; return (redis as Record<string, unknown>)[key]; }Then import in each test file:
+import { getRedisProperty } from '../../../__test-utils__/redis-helpers'; + -// Helper to safely access redis properties (handles union type with Vercel KV) -function getRedisProperty<K extends string>(redis: unknown, key: K): unknown { - if (!redis || typeof redis !== 'object') return undefined; - return (redis as Record<string, unknown>)[key]; -}Committable suggestion skipped: line range outside the PR's diff.
libs/sdk/src/agent/flows/call-agent.flow.ts-234-241 (1)
234-241: Correct the misleading comment about authorization scope and implement consistent authorization handling.The comment at line 238 claims agents follow the same authorization rules as tools, but they don't. Tool authorization (call-tool.flow.ts) implements progressive authorization with incremental auth configuration and throws
AuthorizationRequiredErrorwhen needed. Agent authorization only logs a verbose message and continues, with no support for incremental auth or error-based failures.Either:
- Implement proper incremental authorization for agents to match tool behavior, including
skippedBehaviorconfiguration ('anonymous' or 'require-auth'), or- Update the comment to explicitly state that agent authorization is not enforced at this stage (e.g., "Agent authorization is deferred to runtime execution" or "Agents currently skip app-level authorization validation").
Per the SDK guidelines, use specific error classes with MCP error codes instead of silently continuing when authorization fails.
libs/sdk/src/tool/flows/tools-list.flow.ts-142-169 (1)
142-169: Unsafe type assertion for agent-to-tool conversion masks structural incompatibility.The double type assertion (
as unknown as ToolEntry) on line 164 bypasses TypeScript's type system to force a minimal object into theToolEntrytype. While the current code works because only properties (metadata,rawInputSchema) are accessed,ToolEntryis an abstract class with required methods (create(),parseInput(),parseOutput(),safeParseOutput()) that the minimal object does not implement. This pattern violates the SDK coding guidelines against type assertions and creates maintenance risk—future code changes calling these methods will fail at runtime without type-checking warnings.Instead of casting, create a proper adapter class that either:
- Extends
ToolEntryand implements the required abstract methods, or- Wraps the agent tool in a factory that produces valid
ToolEntryinstancesThis ensures type safety and prevents silent failures if the code evolves to call methods on the tool object.
libs/sdk/src/common/decorators/agent.decorator.ts-3-7 (1)
3-7: Inconsistent Zod import andanytype usage.Line 4 uses default import
import z from 'zod'while other files use named importimport { z } from 'zod'. Also,AgentContextBaseusesanyin the execute signature which violates the coding guidelines.-import z from 'zod'; +import { z } from 'zod'; // Forward reference - AgentContext will be defined in agent.interface.ts -type AgentContextBase = { execute: (...args: any[]) => any }; +type AgentContextBase = { execute: (...args: unknown[]) => unknown };libs/sdk/src/agent/agent-execution-loop.ts-194-216 (1)
194-216: Timeout timer is not cleared on successful completion.The timeout promise creates a timer that's never cleaned up when the execution completes successfully. This can cause memory leaks and unexpected behavior in long-running processes.
try { // Set up timeout + let timeoutId: NodeJS.Timeout | undefined; const timeoutPromise = new Promise<never>((_, reject) => { - setTimeout( + timeoutId = setTimeout( () => reject(new Error(`Agent execution timed out after ${this.config.timeout}ms`)), this.config.timeout, ); }); // Run the loop with timeout const result = await Promise.race([ this.executeLoop(messages, toolExecutor, { onIteration: (iter, msg) => { iterations = iter; this.config.onIteration?.(iter, msg); }, onUsage: (prompt, completion) => { totalPromptTokens += prompt; totalCompletionTokens += completion; }, }), timeoutPromise, ]); + // Clear timeout on success + if (timeoutId) clearTimeout(timeoutId); + return { content: result.content,
🟡 Minor comments (4)
docs/draft/docs/servers/agents.mdx-402-411 (1)
402-411: Override example referencessuper.executeToolbut base throws error.The documentation shows overriding
executeTooland callingsuper.executeTool(name, args), butAgentContext.executeToolthrows an error by default (as seen in agent.interface.ts lines 360-363). This example would fail unlessAgentInstanceoverrides this method. Consider clarifying that the base implementation must be provided by the concrete agent runtime.libs/sdk/src/agent/agent.registry.ts-174-184 (1)
174-184: Adoption subscription may cause memory leak if child registry is removed.The
subscribecall at line 175 registers a listener on the child registry but the unsubscribe function is not stored. If a child registry needs to be removed later, there's no way to clean up the subscription, potentially causing memory leaks and stale updates.🔎 Suggested pattern
+ /** Unsubscribe functions for adopted children */ + private unsubscribes = new Map<AgentRegistry, () => void>(); adoptFromChild(child: AgentRegistry, _childOwner: EntryOwnerRef): void { if (this.children.has(child)) return; // ... existing code ... // Keep live if child changes - child.subscribe({ immediate: false }, () => { + const unsubscribe = child.subscribe({ immediate: false }, () => { const latest = child.listAllIndexed().map((r) => this.relineage(r, prepend)); this.adopted.set(child, latest); this.reindex(); this.bump('reset'); }); + this.unsubscribes.set(child, unsubscribe);libs/sdk/src/agent/agent-execution-loop.ts-255-320 (1)
255-320: Streaming mode lacks timeout protection.The
runStreamingmethod doesn't implement the timeout behavior thatrunhas. This could allow streaming executions to run indefinitely. Consider adding timeout handling or documenting that streaming mode has no timeout.libs/sdk/src/agent/adapters/langchain.adapter.ts-424-462 (1)
424-462: Pass temperature and maxTokens from options to LangChain invoke.The
buildInvokeOptionsmethod ignorestemperatureandmaxTokensfromAgentCompletionOptions. ExtendLangChainInvokeOptionsto include these fields (using LangChain'smax_tokensconvention) and pass them from the options parameter, with fallback to adapter config defaults.
🧹 Nitpick comments (37)
libs/sdk/src/transport/__tests__/transport.registry.test.ts (1)
1040-1043: Consider defining proper return types instead of inline type assertions.The repeated inline type assertions
(session as { transportState?: { protocol: string } })suggest that the return type ofgetStoredSession()doesn't properly include thetransportStateproperty. This approach reduces readability and maintainability.Consider one of these solutions:
- Preferred: Update the return type of
getStoredSession()to properly includetransportStatein its type definition.- Alternative: Define a proper type or interface for the session object that includes
transportState, then use it consistently.🔎 Example refactor with explicit typing
+// Define a type for the session with transport state +type SessionWithTransportState = { + session: { id: string; authorizationId: string; protocol: string; createdAt: number; nodeId: string }; + authorizationId: string; + createdAt: number; + lastAccessedAt: number; + transportState?: { protocol: string; requestSeq?: number; lastEventId?: number }; +}; + - expect((session as { transportState?: { protocol: string } })?.transportState).toBeDefined(); - expect((session as { transportState?: { protocol: string } })?.transportState?.protocol).toBe( - 'streamable-http', - ); + const typedSession = session as SessionWithTransportState; + expect(typedSession.transportState).toBeDefined(); + expect(typedSession.transportState?.protocol).toBe('streamable-http');Or better yet, fix the return type of
getStoredSession()in the source code to includetransportState.libs/sdk/src/auth/jwks/__tests__/dev-key-persistence.test.ts (1)
343-343: Consider applying the same type-casting pattern for consistency.Lines 343, 353, 363, 373, 383, 393, and 403 use
(invalidKey.privateKey as Record<string, unknown>)without the intermediateunknowncast. For consistency with the updated pattern on lines 190-270, consider updating these to(invalidKey.privateKey as unknown as Record<string, unknown>).🔎 Example refactor for consistency
- delete (invalidKey.privateKey as Record<string, unknown>)['n']; + delete (invalidKey.privateKey as unknown as Record<string, unknown>)['n'];Apply the same pattern to lines 353, 363, 373, 383, 393, and 403.
Also applies to: 353-353, 363-363, 373-373, 383-383, 393-393, 403-403
libs/sdk/src/auth/session/__tests__/redis-session.store.test.ts (1)
457-459: Type compatibility fix is correct.The double cast pattern (
as unknown as Record<string, unknown>) is a valid TypeScript approach when direct casting isn't allowed. The change makes the type coercion explicit and should work correctly.💡 Optional: Consider a more type-safe test helper
For better maintainability, you could define a test-specific type or helper:
// At the top of the test file, near other helpers type StoredSessionWithMeta = StoredSession & { meta?: Record<string, unknown> }; // Then in the test: const sessionWithMeta = storedSession as StoredSessionWithMeta; sessionWithMeta.meta = { largeField: 'x'.repeat(10000), };This approach makes the intent clearer and avoids the escape hatch pattern.
libs/sdk/src/resource/__tests__/resource.instance.test.ts (1)
334-334: Consider adding explicit test coverage for edge cases.The helper function
getContentTextis only tested indirectly through these two assertions. While the current usage validates the happy path, edge cases (e.g., blob content passed to the helper, explicitly undefined text property) are not covered.Per coding guidelines requiring 95%+ test coverage, consider adding a dedicated test suite for
getContentTextor additional test cases that exercise its boundary conditions.Also applies to: 375-375
libs/sdk/src/__test-utils__/fixtures/scope.fixtures.ts (1)
54-55: Consider a factory function for clearer initialization.The two-step initialization pattern (defining
mockProvidersat lines 29-40, then mutatinggetActiveScopehere) works but relies on closure and mutation. While functional for test fixtures, consider using a factory function to make the circular dependency more explicit.Alternative approach using factory pattern
function createMockProviders(getScope: () => any) { return { getHooksRegistry: jest.fn().mockReturnValue(mockHookRegistry), get: jest.fn().mockImplementation((token: unknown) => { if (token === Scope || (typeof token === 'function' && token.name === 'Scope')) { return getScope(); } return undefined; }), tryGet: jest.fn().mockReturnValue(undefined), getActiveScope: jest.fn().mockImplementation(getScope), }; } // Then in createMockScope: const mockScope: Partial<Scope> = {} as any; const mockProviders = createMockProviders(() => mockScope); mockScope.providers = mockProviders; // ... rest of propertiesThis makes the circular dependency explicit through the callback.
libs/sdk/src/common/types/options/__tests__/transport.options.test.ts (1)
161-161: Property access pattern reduces test readability.Using
getRedisProperty(result.persistence?.redis, 'host')instead of direct property access (e.g.,result.persistence?.redis?.host) makes the test assertion less intuitive and harder to read.If the underlying type structure properly modeled the Redis/Vercel KV union with discriminated unions or proper type narrowing, direct property access would be type-safe and more readable.
libs/sdk/src/common/migrate/__tests__/auth-transport.migrate.test.ts (1)
141-141: Multiple usages of indirect property access pattern.Similar to the transport.options.test.ts file, these helper-based property accesses reduce test readability. The underlying type structure should enable direct, type-safe property access without helper functions.
Also applies to: 160-161
libs/sdk/src/common/types/options/__tests__/redis.options.test.ts (1)
32-32: Inconsistent property access patterns within tests.This file demonstrates an inconsistency: some RedisOptions properties are accessed via the
getRedisPropertyhelper (port, db, tls, password, host - 11 usages), while others use direct property access (keyPrefix at line 47, defaultTtlMs at line 52).This inconsistency suggests that:
- Either all properties should be accessed consistently via the helper (if truly needed for type safety)
- Or the helper is unnecessary and all properties should use direct access
- Or the underlying RedisOptions type needs refinement to enable uniform, type-safe direct access
The most likely scenario is that certain properties (keyPrefix, defaultTtlMs) are not part of the union concern, while connection properties (host, port, password, tls, db) may vary between standard Redis and Vercel KV implementations.
Also applies to: 37-37, 42-42, 47-47, 59-59, 64-64, 69-69, 119-120, 177-177, 182-182, 187-187
libs/sdk/src/transport/adapters/transport.local.adapter.ts (1)
85-85: Both agents and tools return the sametoolscapability, causing redundant spreading.The
getCapabilities()usage correctly follows coding guidelines. However, the spreading order reveals a design issue:this.scope.agents.getCapabilities()returns{ tools: { listChanged: true } }(agents are exposed as tools), which overwrites thetoolscapability fromthis.scope.tools.getCapabilities()with an identical value.While this works functionally, having both registries return the same capability key is redundant. Consider whether agents should omit the
toolscapability since tools are already declared separately, or refactor the spreading logic to avoid duplication.libs/sdk/src/resource/__tests__/resource.registry.test.ts (1)
246-246: Bracket notation change appears isolated.The change from
params.userIdtoparams['userId']is isolated to this one location. While bracket notation is more defensive with stricter types, this appears to be the only execute method body in the test file that actually accesses params properties, so there's no pattern to compare against.If this change was made to satisfy TypeScript strict mode, consider verifying that the params type is correctly defined as
Record<string, string>in the resource interface, which would make dot notation work correctly without bracket access.libs/sdk/src/agent/__tests__/agent.registry.test.ts (1)
213-334: Good swarm visibility coverage.The tests cover the key visibility scenarios: orchestrator-worker patterns, whitelist-based filtering, and isolated agents. Consider adding edge case tests for:
- Agent visibility when
visibleAgentscontains non-existent agent IDs- Ensuring an agent doesn't appear in its own visibility list
libs/sdk/src/agent/flows/call-agent.flow.ts (2)
20-55: Consider replacingz.any()with more specific types where possible.Per coding guidelines,
anytypes should be avoided without strong justification. While line 23's comment explains the MCP SDK variation, consider:
- Line 33:
authInfocould usez.unknown()or the actualAuthInfotype- Line 37:
rawOutputcould usez.unknown()instead ofz.any()Using
z.unknown()provides better type safety by requiring explicit type narrowing before use.Proposed changes
const stateSchema = z.object({ input: z.looseObject({ name: z.string().min(1).max(128), arguments: z.looseObject({}).optional(), }), - authInfo: z.any().optional() as z.ZodType<AuthInfo>, + authInfo: z.unknown().optional() as z.ZodType<AuthInfo | undefined>, agent: z.instanceof(AgentEntry), agentContext: z.instanceof(AgentContext), // Store the raw executed output for plugins to see - rawOutput: z.any().optional(), + rawOutput: z.unknown().optional(), output: outputSchema,
103-149: Intentional early agent lookup for hook filtering.The agent is looked up twice (here and in
findAgent) to extract the owner ID early for hook filtering. While this creates some code duplication, it's necessary for the hook ordering semantics. Consider extracting the common lookup logic into a private helper method to reduce duplication:Optional: Extract agent lookup helper
private findAgentByToolName(toolName: string): AgentEntry | undefined { const scope = this.scope as Scope; if (!scope.agents) return undefined; const agentId = toolName.startsWith('invoke_') ? toolName.slice('invoke_'.length) : toolName; return scope.agents.findById(agentId) ?? scope.agents.findByName(agentId); }libs/sdk/src/agent/agent.utils.ts (2)
253-259: Inconsistent return semantics ingetVisibleAgentIds.When
canSeeOtherAgentsisfalse, returns[]. WhencanSeeOtherAgentsistruebutvisibleAgentsis not set, returnsundefined. This inconsistency could causeundefinedto be interpreted as "see all agents" vs "see no agents".Consider returning
undefinedconsistently to mean "see all" or[]to mean "see none":🔎 Suggested clarification
export function getVisibleAgentIds(metadata: AgentMetadata): string[] | undefined { if (!canAgentSeeSwarm(metadata)) return []; - return metadata.swarm?.visibleAgents; + // undefined means "see all agents", empty array means "see no specific agents" + return metadata.swarm?.visibleAgents ?? undefined; }
229-237: Validation throws generic Error instead of specific error class.Per coding guidelines, use specific error classes with MCP error codes. Consider using
AgentConfigurationErrorfrom the new error classes.🔎 Suggested fix using specific error class
+import { AgentConfigurationError } from '../errors'; + export function validateAgentMetadata(metadata: AgentMetadata): void { if (!metadata.name || metadata.name.trim() === '') { - throw new Error('Agent metadata.name is required'); + throw new AgentConfigurationError('Agent metadata.name is required', { + errors: ['metadata.name is required'], + }); } if (!metadata.llm) { - throw new Error('Agent metadata.llm is required'); + throw new AgentConfigurationError('Agent metadata.llm is required', { + errors: ['metadata.llm is required'], + }); } }Based on learnings, specific error classes with MCP error codes are preferred over generic errors.
libs/sdk/src/agent/adapters/adapter.factory.ts (1)
229-234: Redundant null check afterget()call.
ProviderResolver.get<T>()(line 40) doesn't indicate it can returnnull/undefined, yet line 230 checksif (!adapter). Either update the interface to returnT | undefinedor remove the redundant check.🔎 Align interface with usage
If
get()can return undefined/null for missing tokens:export interface ProviderResolver { - get<T>(token: symbol | { new (...args: unknown[]): T }): T; + get<T>(token: symbol | { new (...args: unknown[]): T }): T | undefined;Or if it always throws for missing tokens, remove the null check in
createAdapter.libs/sdk/src/errors/agent.errors.ts (1)
115-127: Consider HTTP 400 for configuration errors.
AgentConfigurationErroruses HTTP 500, but configuration errors are typically client-side mistakes (invalid input). HTTP 400 (Bad Request) might be more appropriate.🔎 Consider changing to 400
export class AgentConfigurationError extends PublicMcpError { readonly agentId?: string; readonly configErrors: string[]; constructor(message: string, options?: { agentId?: string; errors?: string[] }) { - super(message, 'AGENT_CONFIGURATION_ERROR', 500); + super(message, 'AGENT_CONFIGURATION_ERROR', 400); this.agentId = options?.agentId; this.configErrors = options?.errors ?? [message]; } }apps/e2e/demo-e2e-agents/tsconfig.app.json (1)
7-7: Consider removingjsxoption if not using JSX.The
jsx: "react-jsx"setting appears unnecessary for this Node.js e2e test application unless there's actual JSX usage in the source files. Removing unused configuration keeps the setup cleaner.libs/sdk/src/tool/tool.utils.ts (2)
414-436: Consider usingundefinedinstead of empty string for missing descriptions.Line 433 defaults
descriptionto an empty string (''), but sinceAgentToolDefinition.descriptionis optional (see libs/sdk/src/common/interfaces/llm-adapter.interface.ts:88), usingundefinedwould be more semantically correct when no description exists.🔎 Proposed fix
export function buildAgentToolDefinitions(tools: ToolEntry[]): AgentToolDefinition[] { return tools.map((tool) => ({ name: tool.metadata.id ?? tool.metadata.name, - description: tool.metadata.description ?? '', + description: tool.metadata.description, parameters: tool.rawInputSchema ?? { type: 'object', properties: {} }, })); }
438-450: Consider usingundefinedinstead of empty string for missing descriptions.Line 447 has the same issue as the previous function - defaulting to empty string instead of
undefinedfor optional description field.🔎 Proposed fix
export function buildAgentToolDefinition(tool: ToolEntry): AgentToolDefinition { return { name: tool.metadata.id ?? tool.metadata.name, - description: tool.metadata.description ?? '', + description: tool.metadata.description, parameters: tool.rawInputSchema ?? { type: 'object', properties: {} }, }; }libs/sdk/src/agent/__tests__/agent-execution-loop.test.ts (1)
229-253: Consider verifying the exact default iteration count.The test verifies that the loop fails with
AgentMaxIterationsErrorwhenmaxIterationsis not specified, but doesn't assert that the default is exactly 10 iterations. Adding this assertion would make the test more explicit.🔎 Suggested assertion
const result = await loop.run('Loop forever', toolExecutor); expect(result.success).toBe(false); expect(result.error).toBeInstanceOf(AgentMaxIterationsError); + expect(result.iterations).toBe(10); // Verify default maxIterations is 10 });apps/e2e/demo-e2e-agents/src/apps/agents/calculator.agent.ts (1)
7-31: Consider extracting shared evaluation logic.The same sanitization and
Function()evaluation logic is duplicated in bothmockCalculatorAdapter(lines 14-16) andexecute()(lines 71, 82-83). While acceptable for E2E test code, extracting this to a shared helper would improve maintainability.🔎 Suggested refactor
// Helper function for safe math evaluation function evaluateMathExpression(expression: string): number | null { const sanitized = expression.replace(/[^0-9+\-*/().]/g, ''); if (!sanitized) return null; try { const result = Function('"use strict"; return (' + sanitized + ')')(); return typeof result === 'number' && !isNaN(result) ? result : null; } catch { return null; } }Then use this helper in both
mockCalculatorAdapterandexecute().Also applies to: 69-83
libs/sdk/src/common/interfaces/agent.interface.ts (2)
115-120: Avoidanytypes for backing fields.The
_rawInputand_outputDraftfields use| anywhich weakens type safety. Per coding guidelines, preferunknownfor generic type defaults.🔎 Proposed fix
// ---- INPUT storages (backing fields) - private _rawInput?: Partial<In> | any; + private _rawInput?: Partial<In> | unknown; private _input?: In; // ---- OUTPUT storages (backing fields) - private _outputDraft?: Partial<Out> | any; + private _outputDraft?: Partial<Out> | unknown; private _output?: Out;
360-374: Placeholder methods throw generic errors.The
executeToolandinvokeAgentmethods throw genericErrorobjects. Consider using specific error classes with MCP error codes for better error handling consistency, as per coding guidelines.apps/e2e/demo-e2e-agents/e2e/agents.e2e.test.ts (1)
387-421: Error handling tests should validate error types.Per retrieved learnings, tests should include error class
instanceofchecks. The current tests only verify that an error is defined, not its type or message content. This makes it harder to catch regressions in error handling.🔎 Proposed improvement example
test('should handle missing required parameters', async ({ mcp }) => { - // Call without required message parameter try { await mcp.tools.call('invoke_echo-agent', {}); - // If no error thrown, the call might have validation built in + expect.fail('Expected validation error'); } catch (error) { - // Expected - missing required parameter expect(error).toBeDefined(); + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toMatch(/required|missing|validation/i); } });libs/sdk/src/agent/agent.events.ts (1)
135-144: Consider using a logger instead ofconsole.error.The error handling in
emitcorrectly prevents listener errors from propagating, but usesconsole.errordirectly. For consistency with the SDK's logging patterns, consider accepting an optional logger or using a structured logging approach.libs/sdk/src/common/entries/agent.entry.ts (1)
82-84:rawInputSchemausesanytype.The
rawInputSchemafield is typed asany. Since this represents a JSON Schema object with dynamic structure, consider usingRecord<string, unknown>or a more specific JSON Schema type from a library for better type safety.🔎 Proposed fix
/** * Raw JSON schema for the input (used in tool definition). */ - rawInputSchema?: any; + rawInputSchema?: Record<string, unknown>;libs/sdk/src/agent/adapters/base.adapter.ts (2)
1-10: Unused import:AgentToolCallThe
AgentToolCalltype is imported but never used in this file. Consider removing it to keep imports clean.import { AgentLlmAdapter, AgentPrompt, AgentMessage, AgentCompletion, AgentCompletionChunk, - AgentToolCall, AgentToolDefinition, AgentCompletionOptions, } from '../../common/interfaces/llm-adapter.interface';
165-174: String-based error detection is fragile.The
isNonRetryableErrormethod relies on substring matching in error messages, which is brittle across different LLM providers. Consider also checking error properties likestatusCodeorcodewhen available for more reliable detection.protected isNonRetryableError(error: Error): boolean { const message = error.message.toLowerCase(); + // Check for status codes if available (more reliable) + if ('statusCode' in error) { + const statusCode = (error as { statusCode?: number }).statusCode; + if (statusCode === 401 || statusCode === 403) return true; + } return ( message.includes('invalid api key') || message.includes('authentication') || message.includes('unauthorized') || message.includes('invalid_request') || message.includes('context_length_exceeded') ); }libs/sdk/src/agent/adapters/langchain.adapter.ts (1)
329-336: Silent failure on JSON parse could hide issues.When
JSON.parse(tc.rawArgs)fails, the error is silently ignored and empty args are used. Consider logging this failure for debugging purposes, as malformed tool arguments could indicate LLM issues.if (tc.rawArgs) { try { args = JSON.parse(tc.rawArgs); } catch { - // Keep empty args if parsing fails + // Log parse failure for debugging - could indicate LLM issues + // Consider: this.logger?.warn(`Failed to parse tool args: ${tc.rawArgs}`); } }libs/sdk/src/agent/agent.instance.ts (3)
108-121: Provider resolver usesanytypes.The provider resolver's
getandtryGetmethods useanyin their casts and return types, which violates the coding guidelines. Consider usingunknownwith proper type narrowing.const adapterOptions: CreateAdapterOptions = { providerResolver: { - get: <T>(token: symbol | { new (...args: unknown[]): T }): T => { - return this.providers.get(token as unknown as never) as T; + get: <T>(token: symbol | { new (...args: unknown[]): T }): T => { + return this.providers.get(token as symbol) as T; }, - tryGet: <T>(token: symbol | { new (...args: unknown[]): T }): T | undefined => { + tryGet: <T>(token: symbol | { new (...args: unknown[]): T }): T | undefined => { try { - return this.providers.get(token as unknown as never) as T; + return this.providers.get(token as symbol) as T; } catch { return undefined; } }, }, };
189-192: Use specific error class instead of generic Error.Per coding guidelines, use specific error classes with MCP error codes instead of generic errors. Consider using an agent-specific error class.
if (!this.llmAdapter) { - throw new Error(`Agent ${this.name} has no LLM adapter configured`); + throw new AgentConfigurationError(`Agent ${this.name} has no LLM adapter configured`); }You may need to import or define
AgentConfigurationErrorin the errors module.
393-410: Basic Zod-to-JSON type conversion may miss complex types.The
zodTypeToJsonTypehelper only handles basic Zod types. Complex types likeZodUnion,ZodIntersection,ZodLiteral, or nested objects will default to'string'. Consider extending this for better schema accuracy or adding a comment about limitations.private zodTypeToJsonType(typeName?: string): string { switch (typeName) { case 'ZodString': return 'string'; case 'ZodNumber': return 'number'; case 'ZodBoolean': return 'boolean'; case 'ZodArray': return 'array'; case 'ZodObject': return 'object'; case 'ZodEnum': return 'string'; + case 'ZodLiteral': + return 'string'; // Could be refined based on literal value + case 'ZodUnion': + case 'ZodDiscriminatedUnion': + return 'object'; // Complex union types default: + // TODO: Handle more Zod types for accurate JSON schema generation return 'string'; } }libs/sdk/src/common/decorators/agent.decorator.ts (1)
119-144: Type inference fallbacks toanyare intentional but could use documentation.The
anyreturn types in__InferFromSingleSchema(line 129) andAgentOutputOf(line 144) are fallbacks for unrecognized schemas. While this provides flexibility, consider usingunknownfor safer defaults or add JSDoc explaining whyanyis acceptable here.type __InferFromSingleSchema<S> = S extends 'string' ? string : S extends 'number' ? number : S extends 'boolean' ? boolean : S extends 'date' ? Date : S extends z.ZodType | z.ZodRawShape ? __InferZod<S> - : any; + : unknown; // Fallback for unrecognized schema typeslibs/sdk/src/common/metadata/agent.metadata.ts (2)
408-417: Validation schemas could be more specific.Line 409 uses
z.instanceof(Object)which accepts any object, and line 417 usesz.any()as a fallback. While these provide flexibility, they reduce type safety at runtime.const adapterConfigSchema = z.object({ - adapter: z.union([z.instanceof(Object), z.function()]), + adapter: z.union([ + z.custom<AgentLlmAdapterLocal>((v) => + typeof v === 'object' && v !== null && 'completion' in v, + { message: 'Must be an object with completion method' } + ), + z.function(), + ]), }); const llmConfigSchema = z.union([ builtinAdapterConfigSchema, adapterConfigSchema, z.custom<symbol>((v) => typeof v === 'symbol', { message: 'Must be a symbol token' }), - z.any(), // Allow direct adapter instances + z.custom((v) => typeof v === 'object' && v !== null && 'completion' in v, + { message: 'Must be a valid adapter instance' } + ), ]);
445-468: Agent validation is currently permissive.The
annotatedFrontMcpAgentsSchemaaccepts any function (line 451) with a comment noting it will be refined. Consider adding a TODO tracking issue or implementing the decorator metadata check now.export const annotatedFrontMcpAgentsSchema = z.custom<AgentType>( (v): v is AgentType => { // Check for class-based @Agent decorator (will be added after tokens are created) if (typeof v === 'function') { - // Check for FrontMcpAgentTokens.type metadata (added by decorator) - // For now, allow any function - will be refined when tokens are created - return true; + // Check if decorated with @Agent (has type metadata) + try { + const hasAgentMetadata = Reflect.hasMetadata?.('frontmcp:agent:type', v); + return hasAgentMetadata || true; // Fallback to true for backward compat + } catch { + return true; + } }libs/sdk/src/agent/agent-execution-loop.ts (1)
366-390: Tool execution logic is duplicated across three paths.The tool execution code is repeated in
executeLoop, streaming path ofexecuteLoopStreaming, and non-streaming fallback. Consider extracting a private helper method to reduce duplication.🔎 Suggested refactor
private async executeToolCall( toolCall: AgentToolCall, toolExecutor: ToolExecutor, ): Promise<{ result: unknown; error?: Error }> { this.config.onToolCall?.(toolCall); let result: unknown; let error: Error | undefined; try { result = await toolExecutor(toolCall.name, toolCall.arguments); } catch (e) { error = e as Error; result = { error: error.message }; } this.config.onToolResult?.(toolCall, result, error); return { result, error }; } private createToolMessage(toolCall: AgentToolCall, result: unknown): AgentMessage { return { role: 'tool', content: typeof result === 'string' ? result : JSON.stringify(result), toolCallId: toolCall.id, name: toolCall.name, }; }Also applies to: 481-507, 542-568
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/mcp-from-openapi/src/__tests__/generator.spec.ts (1)
868-868: Consider reverting to dot notation for consistency.These changes convert dot notation to bracket notation for property access (e.g.,
inputSchema.properties?.oldParam→inputSchema.properties?.['oldParam']). While both syntaxes are valid, this creates inconsistency within the same test file where other similar property accesses continue using dot notation (e.g., lines 802-804, 828-830, 909-910).Dot notation is typically preferred for static, valid identifiers as it's more concise and readable. Unless there's a specific technical reason for bracket notation (such as TypeScript strict index signature requirements), consider reverting these changes to maintain consistency with the rest of the codebase.
🔎 Suggested reversion to dot notation
- const schema = inputSchema.properties?.['oldParam'] as any; + const schema = inputSchema.properties?.oldParam as any;- expect(inputSchema.properties?.['bearerAuth']).toBeUndefined(); + expect(inputSchema.properties?.bearerAuth).toBeUndefined();- const jwtSchema = inputSchema.properties?.['jwt'] as any; + const jwtSchema = inputSchema.properties?.jwt as any;- const oidcSchema = inputSchema.properties?.['oidc'] as any; + const oidcSchema = inputSchema.properties?.oidc as any;Also applies to: 956-956, 968-968, 988-988
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/e2e/demo-e2e-agents/jest.e2e.config.tsapps/e2e/demo-e2e-codecall/jest.e2e.config.tslibs/mcp-from-openapi/src/__tests__/generator.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/e2e/demo-e2e-agents/jest.e2e.config.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use type parameters with constraints instead of unconstrained generics
Do not mutate rawInput in flows - use state.set() for managing flow state instead
Files:
libs/mcp-from-openapi/src/__tests__/generator.spec.tsapps/e2e/demo-e2e-codecall/jest.e2e.config.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/mcp-from-openapi/src/__tests__/generator.spec.ts
🧠 Learnings (8)
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : MCP response types should use strict MCP protocol types (GetPromptResult, ReadResourceResult) instead of `unknown`
Applied to files:
libs/mcp-from-openapi/src/__tests__/generator.spec.ts
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/src/**/*.test.{ts,tsx} : Test coverage must be 95%+ across statements, branches, functions, and lines
Applied to files:
apps/e2e/demo-e2e-codecall/jest.e2e.config.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to **/*.test.{ts,tsx} : Achieve 95%+ test coverage across all metrics (statements, branches, functions, lines)
Applied to files:
apps/e2e/demo-e2e-codecall/jest.e2e.config.ts
📚 Learning: 2025-12-24T00:41:55.397Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:55.397Z
Learning: Applies to libs/uipack/src/{adapters,theme,build}/**/*.test.{ts,tsx,js,jsx} : Test component behavior across all platform configurations (OpenAI, Claude, etc.)
Applied to files:
apps/e2e/demo-e2e-codecall/jest.e2e.config.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to **/*.ts : Avoid non-null assertions (`!`) - use proper error handling and throw specific errors instead
Applied to files:
apps/e2e/demo-e2e-codecall/jest.e2e.config.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to **/*.test.{ts,tsx} : Test all code paths including error cases and constructor validation
Applied to files:
apps/e2e/demo-e2e-codecall/jest.e2e.config.ts
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/src/bundler/**/*.{ts,tsx} : The bundler module must re-export utilities from frontmcp/uipack/bundler and provide SSR component bundling functionality
Applied to files:
apps/e2e/demo-e2e-codecall/jest.e2e.config.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : Validate hook flows and fail fast on invalid hook configurations with specific error messages
Applied to files:
apps/e2e/demo-e2e-codecall/jest.e2e.config.ts
…refix and add SummaryAgent
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (18)
libs/sdk/src/common/entries/agent.entry.ts (3)
82-83: Avoidanytype forrawInputSchema.Per coding guidelines, use
unknowninstead ofanywhen the type is generic. Since this represents a JSON schema, consider using theJsonSchematype (imported fromzod/v4/coreas seen intool.metadata.ts) or at minimumunknown.🔎 Proposed fix
+import type { JSONSchema } from 'zod/v4/core'; + +/** JSON Schema type from Zod v4 */ +type JsonSchema = JSONSchema.JSONSchema; + /** * Raw JSON schema for the input (used in tool definition). */ - rawInputSchema?: any; + rawInputSchema?: JsonSchema;
140-141: Avoidanyin union type forparseOutputparameter.The
anytype undermines type safety. Replace withunknownto maintain type checking while allowing arbitrary input.🔎 Proposed fix
- abstract parseOutput(result: Out | Partial<Out> | any): ParsedAgentResult; + abstract parseOutput(result: Out | Partial<Out> | unknown): ParsedAgentResult;
148-149: Sameanyissue insafeParseOutputparameter.Apply the same fix for consistency.
🔎 Proposed fix
- abstract safeParseOutput(raw: Out | Partial<Out> | any): SafeTransformResult<ParsedAgentResult>; + abstract safeParseOutput(raw: Out | Partial<Out> | unknown): SafeTransformResult<ParsedAgentResult>;libs/sdk/src/agent/__tests__/adapters.test.ts (1)
273-311: Consider adding a streaming error handling test.The streaming tests cover happy paths well. Consider adding a test case for when
streamthrows an error mid-stream to verify error propagation matches the non-streamingLlmAdapterErrorwrapping behavior.libs/sdk/src/agent/adapters/providers/index.ts (1)
117-119:baseUrlonly applies to OpenAI - consider documenting or extending.The
baseUrloption is silently ignored for non-OpenAI providers. Consider either:
- Documenting this limitation in
CreateProviderAdapterOptions- Supporting
baseUrlfor other providers that accept it (e.g., Anthropic, Mistral)- Throwing an error if
baseUrlis provided for unsupported providers🔎 Option: Add JSDoc to document the limitation
export interface CreateProviderAdapterOptions extends ProviderCommonOptions { provider: LlmProvider; model: string; apiKey: string; + /** + * Custom base URL for the API endpoint. + * Currently only supported for the 'openai' provider. + */ baseUrl?: string; }libs/sdk/src/common/metadata/tool.metadata.ts (1)
7-12: Consolidate duplicate constant to single source.
AGENT_TOOL_PREFIXis exported fromlibs/sdk/src/agent/agent.utils.ts(line 189) and should be imported here instead of redefined. This eliminates the maintenance risk if the prefix value changes.Remove the local definition and import from agent.utils:
-/** - * Reserved prefix for agent tool names. - * Tools cannot use this prefix - it's reserved for agent invocations. - */ -const AGENT_TOOL_PREFIX = 'use-agent:'; import { + AGENT_TOOL_PREFIX, +} from '../../agent/agent.utils'; import { ImageContentSchema,libs/sdk/src/agent/adapters/providers/types.ts (2)
30-33: Unused generic parameter inModelForProvider<P>.The generic parameter
P extends LlmProvideris declared but not used in the type body—it always returnsstring. If provider-specific model enums are planned, consider adding a TODO comment; otherwise, this could be simplified to a plain type alias.🔎 Suggested simplification (if no future plans)
-/** - * Model type based on provider. - */ -export type ModelForProvider<P extends LlmProvider> = string; +/** + * Model type (provider-specific enums can be added later). + */ +export type ModelForProvider = string;
24-28: Model type aliases are all equivalent tostring.The aliases
OpenAIModel,AnthropicModel,GoogleModel,MistralModel, andGroqModelare allstring. If you intend to add model-specific constraints later, consider adding a brief comment explaining the intent. Otherwise, these could be consolidated.libs/sdk/src/agent/adapters/langchain.adapter.ts (2)
91-99: Module-level mutable state for cached message classes.The
messageClassesvariable is module-scoped mutable state. While this is a common caching pattern, it could cause issues in testing scenarios or if the module is hot-reloaded. Consider documenting this behavior or providing a reset mechanism for tests.
640-675: Uselc_error_codeproperty for structured error classification instead of string matching.LangChain errors include an
lc_error_codeproperty with standardized error codes (e.g.,MODEL_RATE_LIMIT,MODEL_AUTHENTICATION). Relying onmessage.includes()is fragile, locale-dependent, and brittle across API changes. Check the error object'slc_error_codeproperty first before falling back to message inspection, which aligns with LangChain's recommended error handling practices.libs/sdk/src/agent/agent.registry.ts (1)
96-106: Non-null assertions ondefsandgraphmaps.Lines 98, 104 use non-null assertions (
!). Per coding guidelines, prefer proper error handling. However, these are safe here becausebuildMapguarantees that every token inthis.tokenshas corresponding entries indefsandgraph.Consider adding a guard clause for defensive coding, or document why these are safe:
🔎 Optional defensive approach
protected buildGraph(): void { for (const token of this.tokens) { - const rec = this.defs.get(token)!; + const rec = this.defs.get(token); + if (!rec) throw new Error(`Agent definition not found for token: ${String(token)}`); const deps = agentDiscoveryDeps(rec); for (const d of deps) { // Validate against hierarchical providers; throws early if missing this.providers.get(d); - this.graph.get(token)!.add(d); + const edges = this.graph.get(token); + if (!edges) throw new Error(`Graph entry not found for token: ${String(token)}`); + edges.add(d); } } }libs/sdk/src/common/decorators/agent.decorator.ts (1)
71-87: Misleading return type forfrontMcpAgentdecorator.The return type
(handler: ...) => () => voidsuggests the returned function returnsvoid, but the implementation at lines 78-85 returnsexecute(the handler function). The actual return isagentFunctionwhich wraps and returnsexecute.This works at runtime but the type signature is confusing. Consider clarifying:
🔎 Suggested type clarification
function frontMcpAgent< T extends AgentMetadata<InSchema, OutSchema>, InSchema extends ToolInputType = T['inputSchema'] extends ToolInputType ? T['inputSchema'] : ToolInputType, OutSchema extends ToolOutputType = T['outputSchema'], ->(providedMetadata: T): (handler: FrontMcpAgentExecuteHandler<InSchema, OutSchema>) => () => void { +>(providedMetadata: T): (handler: FrontMcpAgentExecuteHandler<InSchema, OutSchema>) => FrontMcpAgentExecuteHandler<InSchema, OutSchema> & { [key: symbol]: unknown } { return (execute) => { const metadata = frontMcpAgentMetadataSchema.parse(providedMetadata); const agentFunction = function () { return execute; }; Object.assign(agentFunction, { [FrontMcpAgentTokens.type]: 'function-agent', [FrontMcpAgentTokens.metadata]: metadata, }); - return agentFunction; + return agentFunction as unknown as FrontMcpAgentExecuteHandler<InSchema, OutSchema> & { [key: symbol]: unknown }; }; }libs/sdk/src/agent/agent.utils.ts (4)
20-23:isAgentClasscould be a proper type guard.The function returns
booleanbut could be a type guard likeisAgentFunctionfor better type narrowing in consuming code.🔎 Suggested improvement
-export function isAgentClass(value: unknown): boolean { +export function isAgentClass(value: unknown): value is Function & { new (...args: unknown[]): unknown } { if (typeof value !== 'function') return false; return Reflect.getMetadata(FrontMcpAgentTokens.type, value) === true; }
272-275: Inconsistent return type ingetVisibleAgentIds.When
canAgentSeeSwarmreturnsfalse, the function returns[](empty array). Otherwise, it returnsmetadata.swarm?.visibleAgentswhich could beundefined. The declared return typestring[] | undefinedcovers both, but the behavior is inconsistent—callers might expect[]instead ofundefinedwhen visibility is configured but no specific agents are listed.🔎 Suggested consistent behavior
export function getVisibleAgentIds(metadata: AgentMetadata): string[] | undefined { if (!canAgentSeeSwarm(metadata)) return []; - return metadata.swarm?.visibleAgents; + return metadata.swarm?.visibleAgents ?? undefined; }Or, if you want to always return an array:
-export function getVisibleAgentIds(metadata: AgentMetadata): string[] | undefined { +export function getVisibleAgentIds(metadata: AgentMetadata): string[] { if (!canAgentSeeSwarm(metadata)) return []; - return metadata.swarm?.visibleAgents; + return metadata.swarm?.visibleAgents ?? []; }
197-204: Agent ID sanitization may cause collisions.The sanitization logic removes non-alphanumeric characters (except
_-) and collapses multiple underscores. IDs likemy.agentandmy_agentwould both becomemy_agent, potentially causing tool name collisions.Consider logging a warning when sanitization modifies the ID, or documenting this behavior:
🔎 Suggested warning
export function agentToolName(agentId: string): string { // Sanitize the agent ID for use in a tool name const sanitized = agentId .replace(/[^a-zA-Z0-9_-]/g, '_') .replace(/_{2,}/g, '_') .replace(/^_|_$/g, ''); + if (sanitized !== agentId) { + console.warn(`Agent ID "${agentId}" was sanitized to "${sanitized}" for tool naming`); + } return `${AGENT_TOOL_PREFIX}${sanitized}`; }
245-253:validateAgentMetadatathrows genericErrorinstead of specific error class.Per coding guidelines, use specific error classes with MCP error codes instead of generic errors. Consider using a custom validation error or the SDK's error classes.
libs/sdk/src/common/metadata/agent.metadata.ts (2)
413-418:z.any()inllmConfigSchemabypasses validation.The final union member
z.any()at line 417 effectively disables validation for thellmfield—any value will pass. While the comment says "Allow direct adapter instances," this also allows completely invalid values.Consider using a more specific validator:
🔎 Suggested tighter validation
const llmConfigSchema = z.union([ builtinAdapterConfigSchema, adapterConfigSchema, z.custom<symbol>((v) => typeof v === 'symbol', { message: 'Must be a symbol token' }), // Token<AgentLlmAdapter> - z.any(), // Allow direct adapter instances + z.custom<object>( + (v) => typeof v === 'object' && v !== null && 'completion' in v, + { message: 'Direct adapter must have a completion method' } + ), ]);
446-469: Permissive function validation inannotatedFrontMcpAgentsSchema.Lines 449-453 accept any function as a valid agent type:
if (typeof v === 'function') { // For now, allow any function - will be refined when tokens are created return true; }This allows non-agent functions to pass validation. Consider adding a runtime check for the agent type metadata once tokens are available, or document this as a known limitation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (22)
apps/demo/src/apps/weather/agents/summary.agent.tsapps/demo/src/apps/weather/index.tsapps/demo/src/main.tslibs/sdk/package.jsonlibs/sdk/src/agent/__tests__/adapters.test.tslibs/sdk/src/agent/__tests__/agent.registry.test.tslibs/sdk/src/agent/adapters/adapter.factory.tslibs/sdk/src/agent/adapters/langchain.adapter.tslibs/sdk/src/agent/adapters/providers/index.tslibs/sdk/src/agent/adapters/providers/types.tslibs/sdk/src/agent/agent.registry.tslibs/sdk/src/agent/agent.utils.tslibs/sdk/src/agent/flows/call-agent.flow.tslibs/sdk/src/agent/index.tslibs/sdk/src/common/decorators/agent.decorator.tslibs/sdk/src/common/entries/agent.entry.tslibs/sdk/src/common/metadata/agent.metadata.tslibs/sdk/src/common/metadata/app.metadata.tslibs/sdk/src/common/metadata/tool.metadata.tslibs/sdk/src/tool/flows/call-tool.flow.tslibs/sdk/src/tool/flows/tools-list.flow.tslibs/sdk/src/transport/mcp-handlers/call-tool-request.handler.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- libs/sdk/src/tool/flows/tools-list.flow.ts
- libs/sdk/src/agent/tests/agent.registry.test.ts
- libs/sdk/src/tool/flows/call-tool.flow.ts
- libs/sdk/src/agent/flows/call-agent.flow.ts
- libs/sdk/src/agent/adapters/adapter.factory.ts
🧰 Additional context used
📓 Path-based instructions (7)
libs/*/package.json
📄 CodeRabbit inference engine (CLAUDE.md)
Each library in /libs/* is independent and publishable under @frontmcp/* scope
Files:
libs/sdk/package.json
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/sdk/package.jsonlibs/sdk/src/transport/mcp-handlers/call-tool-request.handler.tslibs/sdk/src/common/metadata/tool.metadata.tslibs/sdk/src/common/metadata/app.metadata.tslibs/sdk/src/agent/adapters/langchain.adapter.tslibs/sdk/src/agent/adapters/providers/types.tslibs/sdk/src/agent/adapters/providers/index.tslibs/sdk/src/common/decorators/agent.decorator.tslibs/sdk/src/agent/agent.registry.tslibs/sdk/src/agent/__tests__/adapters.test.tslibs/sdk/src/common/entries/agent.entry.tslibs/sdk/src/agent/index.tslibs/sdk/src/agent/agent.utils.tslibs/sdk/src/common/metadata/agent.metadata.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use type parameters with constraints instead of unconstrained generics
Do not mutate rawInput in flows - use state.set() for managing flow state instead
Files:
apps/demo/src/apps/weather/agents/summary.agent.tslibs/sdk/src/transport/mcp-handlers/call-tool-request.handler.tslibs/sdk/src/common/metadata/tool.metadata.tslibs/sdk/src/common/metadata/app.metadata.tslibs/sdk/src/agent/adapters/langchain.adapter.tsapps/demo/src/apps/weather/index.tslibs/sdk/src/agent/adapters/providers/types.tslibs/sdk/src/agent/adapters/providers/index.tslibs/sdk/src/common/decorators/agent.decorator.tslibs/sdk/src/agent/agent.registry.tslibs/sdk/src/agent/__tests__/adapters.test.tslibs/sdk/src/common/entries/agent.entry.tslibs/sdk/src/agent/index.tsapps/demo/src/main.tslibs/sdk/src/agent/agent.utils.tslibs/sdk/src/common/metadata/agent.metadata.ts
apps/demo/**
⚙️ CodeRabbit configuration file
apps/demo/**: apps/demo directory contains a demo application for testing purposes. It can be used as a reference for SDK usage examples.
Files:
apps/demo/src/apps/weather/agents/summary.agent.tsapps/demo/src/apps/weather/index.tsapps/demo/src/main.ts
libs/sdk/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/sdk/src/**/*.ts: MCP response types should use strict MCP protocol types (GetPromptResult, ReadResourceResult) instead ofunknown
Use specific error classes with MCP error codes instead of generic errors
UsegetCapabilities()method for dynamic capability exposure instead of hardcoding capabilities
UsechangeScopeproperty instead ofscopein change events to avoid confusion with Scope class
Validate URIs per RFC 3986 at metadata level using proper validation
Validate hook flows and fail fast on invalid hook configurations with specific error messages
Files:
libs/sdk/src/transport/mcp-handlers/call-tool-request.handler.tslibs/sdk/src/common/metadata/tool.metadata.tslibs/sdk/src/common/metadata/app.metadata.tslibs/sdk/src/agent/adapters/langchain.adapter.tslibs/sdk/src/agent/adapters/providers/types.tslibs/sdk/src/agent/adapters/providers/index.tslibs/sdk/src/common/decorators/agent.decorator.tslibs/sdk/src/agent/agent.registry.tslibs/sdk/src/agent/__tests__/adapters.test.tslibs/sdk/src/common/entries/agent.entry.tslibs/sdk/src/agent/index.tslibs/sdk/src/agent/agent.utils.tslibs/sdk/src/common/metadata/agent.metadata.ts
libs/sdk/src/**/index.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Export public API through barrel files - export users' needed items, no legacy exports with different names
Files:
libs/sdk/src/agent/adapters/providers/index.tslibs/sdk/src/agent/index.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.{ts,tsx}: Achieve 95%+ test coverage across all metrics (statements, branches, functions, lines)
Test all code paths including error cases and constructor validation
Include error classinstanceofchecks in tests to validate error handling
Files:
libs/sdk/src/agent/__tests__/adapters.test.ts
🧠 Learnings (12)
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/src/react/hooks/**/*.{ts,tsx} : MCP bridge hooks (useMcpBridge, useCallTool, useToolInput, useToolOutput, useTheme) must be properly typed and handle loading/error states
Applied to files:
libs/sdk/src/transport/mcp-handlers/call-tool-request.handler.tslibs/sdk/src/common/metadata/tool.metadata.ts
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/src/react/hooks/**/*.{ts,tsx} : Component hooks must properly handle loading and error states when calling MCP tools
Applied to files:
libs/sdk/src/transport/mcp-handlers/call-tool-request.handler.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : Validate URIs per RFC 3986 at metadata level using proper validation
Applied to files:
libs/sdk/src/common/metadata/tool.metadata.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: FrontMCP is a TypeScript-first schema validation framework - all types should align with MCP protocol definitions
Applied to files:
libs/sdk/src/common/metadata/tool.metadata.tslibs/sdk/src/common/metadata/app.metadata.tslibs/sdk/src/common/decorators/agent.decorator.tslibs/sdk/src/common/metadata/agent.metadata.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : MCP response types should use strict MCP protocol types (GetPromptResult, ReadResourceResult) instead of `unknown`
Applied to files:
libs/sdk/src/common/metadata/app.metadata.ts
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/src/universal/**/*.{ts,tsx} : Universal app shell (UniversalApp, FrontMCPProvider) must provide platform-agnostic React context and initialization
Applied to files:
apps/demo/src/apps/weather/index.ts
📚 Learning: 2025-12-24T00:41:55.397Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:55.397Z
Learning: Applies to libs/uipack/src/{adapters,theme,build}/**/*.test.{ts,tsx,js,jsx} : Test component behavior across all platform configurations (OpenAI, Claude, etc.)
Applied to files:
libs/sdk/src/agent/__tests__/adapters.test.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to **/*.test.{ts,tsx} : Test all code paths including error cases and constructor validation
Applied to files:
libs/sdk/src/agent/__tests__/adapters.test.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/index.ts : Export public API through barrel files - export users' needed items, no legacy exports with different names
Applied to files:
libs/sdk/src/agent/index.tslibs/sdk/src/agent/agent.utils.ts
📚 Learning: 2025-12-24T00:41:55.397Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:55.397Z
Learning: Applies to libs/uipack/src/*/index.ts : Implement entry point re-exports in index.ts files for each module (adapters, build, bundler, etc.)
Applied to files:
libs/sdk/src/agent/index.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/common/records/**/*.ts : Centralize record types in common/records directory and import from there instead of module-specific files
Applied to files:
libs/sdk/src/agent/index.tslibs/sdk/src/agent/agent.utils.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : Use `getCapabilities()` method for dynamic capability exposure instead of hardcoding capabilities
Applied to files:
libs/sdk/src/agent/agent.utils.ts
🧬 Code graph analysis (6)
apps/demo/src/apps/weather/agents/summary.agent.ts (1)
libs/sdk/src/agent/index.ts (2)
Agent(87-87)AgentContext(54-54)
libs/sdk/src/transport/mcp-handlers/call-tool-request.handler.ts (1)
libs/sdk/src/agent/agent.utils.ts (1)
isAgentToolName(212-214)
libs/sdk/src/common/metadata/tool.metadata.ts (1)
libs/sdk/src/agent/agent.utils.ts (1)
AGENT_TOOL_PREFIX(189-189)
libs/sdk/src/common/metadata/app.metadata.ts (3)
libs/sdk/src/agent/index.ts (2)
AgentType(70-70)annotatedFrontMcpAgentsSchema(72-72)libs/sdk/src/common/metadata/agent.metadata.ts (2)
AgentType(387-387)annotatedFrontMcpAgentsSchema(446-469)libs/sdk/src/index.ts (1)
AgentType(57-57)
libs/sdk/src/agent/adapters/providers/index.ts (2)
libs/sdk/src/agent/adapters/providers/types.ts (2)
ProviderCommonOptions(42-45)LlmProvider(12-12)libs/sdk/src/agent/adapters/langchain.adapter.ts (2)
LangChainAdapter(277-676)LangChainChatModel(21-31)
libs/sdk/src/common/decorators/agent.decorator.ts (3)
libs/sdk/src/common/metadata/tool.metadata.ts (2)
ToolInputType(164-164)ToolOutputType(163-163)libs/sdk/src/common/metadata/agent.metadata.ts (2)
AgentMetadata(268-381)frontMcpAgentMetadataSchema(474-496)libs/sdk/src/common/tokens/agent.tokens.ts (2)
FrontMcpAgentTokens(9-31)extendedAgentMetadata(37-37)
🔇 Additional comments (25)
apps/demo/src/main.ts (1)
15-15: Port configuration updated to 3003.The port change is appropriate for the new e2e demo infrastructure and is clean—no references to the old port 3002 exist elsewhere in the codebase.
libs/sdk/package.json (1)
77-84: Fix critical version incompatibility in LangChain optional dependencies.The
@langchain/google-genaiv2.1.3 package is built against@langchain/core0.x and is incompatible with the specified@langchain/corev1.1.8. This will cause runtime errors when both are installed together. You must either:
- Upgrade
@langchain/google-genaito a v1.x release (compatible with@langchain/corev1.x), or- Downgrade
@langchain/coreto a 0.x release and adjust other LangChain packages accordingly, or- Use
npmresolutions/yarnoverrides to force a single core instanceAdditionally, per coding guidelines, add documentation in
docs/draft/docs/**explaining the optional LangChain dependencies and required setup/version compatibility.⛔ Skipped due to learnings
Learnt from: CR Repo: agentfront/frontmcp PR: 0 File: libs/ui/CLAUDE.md:0-0 Timestamp: 2025-12-24T00:41:41.819Z Learning: Applies to libs/ui/package.json : The frontmcp/ui package requires React as a peer dependency (^18.0.0 || ^19.0.0)Learnt from: CR Repo: agentfront/frontmcp PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-19T02:04:46.475Z Learning: Applies to libs/*/package.json : Each library in /libs/* is independent and publishable under frontmcp/* scopelibs/sdk/src/common/entries/agent.entry.ts (1)
49-54: LGTM on the generic type parameters and class structure.The generic type parameter design with
InSchema,OutSchema,In,Outprovides good flexibility while maintaining type safety through theAgentInputOfandAgentOutputOfhelper types.apps/demo/src/apps/weather/index.ts (1)
11-19: LGTM!Clean integration of the new
SummaryAgentinto the weather app. This serves as a good reference example for registering agents within an app's metadata.libs/sdk/src/transport/mcp-handlers/call-tool-request.handler.ts (1)
24-26: LGTM!Clean routing logic to distinguish agent invocations (
use-agent:*) from regular tool calls. The flow delegation approach keeps the handler focused on routing concerns while deferring execution details to the appropriate flow.apps/demo/src/apps/weather/agents/summary.agent.ts (2)
5-21: LGTM on the agent structure.The agent definition follows the SDK pattern correctly: declarative input schema with Zod, LLM configuration, and tool registration. The empty class body extending
AgentContextis the expected pattern for declarative agents.
12-18: The modelgpt-5is now a valid OpenAI model as of December 2025. The GPT-5 family includes variants likegpt-5.2andgpt-5.2-chat-latest, making this a current and available model option for the demo application.Likely an incorrect or invalid review comment.
libs/sdk/src/common/metadata/tool.metadata.ts (1)
256-265: LGTM on the reserved prefix validation logic.The
reservedPrefixCheckrefinement correctly prevents tools from using theuse-agent:prefix, which is reserved for agent invocations. The error message is clear and actionable.libs/sdk/src/agent/__tests__/adapters.test.ts (3)
125-134: Good error handling test withinstanceofcheck.Testing that LLM errors are wrapped in
LlmAdapterErrorensures proper error class hierarchy, which aligns with the coding guideline to include error classinstanceofchecks in tests.
209-311: Comprehensive streaming tests.Good coverage of streaming scenarios including:
- Fallback when stream not available
- Content chunk streaming
- Streaming tool calls
454-469: Good DI token error case coverage.Tests properly verify error handling when DI tokens are not found or when a resolver is missing. The regex check on line 468 ensures the error message is informative.
libs/sdk/src/common/metadata/app.metadata.ts (2)
94-100: LGTM!Good documentation explaining that agents are "exposed as a callable tool with the name
use-agent:<agent_id>". This aligns with theAGENT_TOOL_PREFIXconstant and routing logic in the call-tool handler.
117-136: LGTM on schema integration.The
agentsfield is properly integrated intofrontMcpLocalAppMetadataSchemausingannotatedFrontMcpAgentsSchemafor validation, consistent with how other entity arrays (tools, resources, prompts) are handled.libs/sdk/src/agent/adapters/providers/index.ts (3)
46-68: Excellent error message for missing provider packages.The error provides clear installation instructions and usage examples, which significantly improves developer experience when a LangChain provider package is not installed.
171-207: LGTM on streaming fallback implementation.The
streamCompletionmethod correctly handles the case where the underlying adapter doesn't support streaming by falling back to non-streaming and yielding results in sequence (tool calls → content → done).
141-160: No action needed -DeferredProviderAdapteris correctly kept private.The class is not exported and should not be. Consumers receive instances as the
AgentLlmAdapterinterface throughcreateProviderAdapterSync(), which provides proper encapsulation. The concrete class is an internal implementation detail that doesn't need to be part of the public API.Likely an incorrect or invalid review comment.
libs/sdk/src/agent/index.ts (1)
1-94: Well-structured barrel file for the agent module.The exports are logically organized into core, flow, hook, and adapter sections with helpful re-exports of common types for user convenience. The module-level JSDoc with example usage is excellent for discoverability.
libs/sdk/src/agent/adapters/langchain.adapter.ts (1)
302-306: Debug logging usesconsole.logwith environment variable check.The debug logging pattern is reasonable for development. Consider using a structured logger or ensuring
DEBUG_LANGCHAINis documented for users who need to troubleshoot adapter issues.libs/sdk/src/agent/agent.registry.ts (4)
193-206: Intentionalanytypes with documented justification.The
anytypes ingetAgentsandgetInlineAgentsreturn types are documented with comments explaining they're intentional for heterogeneous agent collections. This aligns with coding guidelines that require justification foranyusage.
321-340: Proper use ofchangeScopeproperty in event emissions.The subscription and event emission pattern correctly uses
changeScope: 'global'instead of ascopeproperty, aligning with coding guidelines to avoid confusion with the Scope class.
384-393: Dynamic capability exposure viagetCapabilities().The
getCapabilities()method dynamically determines capabilities based on whether agents exist, following the coding guideline to use dynamic capability exposure instead of hardcoding.
8-8: No issue found. The import path'../regsitry'is correct.The directory
libs/sdk/src/regsitry/exists and exports bothRegistryAbstractandRegistryBuildMapResult. The spelling is consistent across the codebase—all registry implementations import from'../regsitry'. This is not a typo.Likely an incorrect or invalid review comment.
libs/sdk/src/common/decorators/agent.decorator.ts (2)
241-263: Module augmentation with@ts-expect-errorfor decorator overloads.The
@ts-expect-errorcomments are necessary for module augmentation patterns where TypeScript can't verify the decorator overloads. This is a known limitation when extending external module types.The dual overload pattern (with/without
outputSchema) provides excellent type inference for users.
23-44: Class decorator with Zod validation and Reflect metadata distribution.The
FrontMcpAgentdecorator correctly:
- Validates metadata via
frontMcpAgentMetadataSchema.parse()- Sets type marker via
FrontMcpAgentTokens.type- Distributes known properties to individual metadata keys
- Collects extended properties for passthrough
The
target: anyat line 27 is necessary forClassDecoratorcompatibility.libs/sdk/src/common/metadata/agent.metadata.ts (1)
420-435: Correct use of Zod 4.default()in nested object schemas.The usage of
.default()on optional fields withinswarmConfigSchemaandexecutionConfigSchemaleverages Zod 4's improved default value handling, where defaults are now applied even inside optional fields. This is the correct pattern per Zod 4 documentation.
…on, and improved error handling
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
libs/sdk/src/common/metadata/agent.metadata.ts (1)
101-104: Local interfaceAgentLlmAdapterLocalusesunknowntypes - existing concern.This interface was flagged in a previous review as significantly out of sync with the canonical
AgentLlmAdapterdefinition. The canonical interface uses strict types (AgentPrompt,AgentToolDefinition[],AgentCompletionOptions, etc.) while this usesunknownthroughout.If circular dependencies prevent importing the canonical types, consider:
- Creating a shared types file both can import
- Restructuring the import graph
- At minimum, documenting why
unknownis acceptable here
🧹 Nitpick comments (8)
libs/sdk/src/agent/adapters/providers/index.ts (1)
36-41: Consider clarifying the baseUrl documentation.The comment states that
baseUrlis "silently ignored" for non-OpenAI providers, but the implementation doesn't pass it to other providers' configurations at all. Consider rewording to something like "Only applicable to OpenAI provider" or "Not used by other providers" for clarity.libs/sdk/src/agent/agent.scope.ts (1)
275-285: Consider enriching the error message with flow name.The
runFlowForOutputmethod correctly enforces non-empty output from flows. However, the error message could be more helpful by including the flow name for debugging.🔎 Proposed enhancement
async runFlowForOutput<Name extends FlowName>( name: Name, input: FlowInputOf<Name>, deps?: Map<Token, Type>, ): Promise<FlowOutputOf<Name>> { const result = await this.agentFlows.runFlow(name, input, deps); if (result) { return result; } - throw new Error(`Flow exited without output`); + throw new Error(`Flow "${name}" exited without output`); }libs/sdk/src/common/entries/agent.entry.ts (1)
37-38: Consider moving this import to the top of the file.The import of
SafeTransformResultis placed after the type definitions section (line 38), which breaks the conventional import grouping. Move it to the imports block at the top for better organization.🔎 Suggested change
Move line 38 to after line 11:
import type { JSONSchema } from 'zod/v4/core'; +import { SafeTransformResult } from './tool.entry'; /** JSON Schema type from Zod v4 */And remove the comment and import from lines 37-38.
libs/sdk/src/agent/agent.instance.ts (2)
328-329: Non-null assertion onllmAdaptercould be avoided.The
this.llmAdapter!assertion on line 329 is used after the null check on line 291, but TypeScript's flow analysis may not track this through thebuildAgentCtorArgsmethod call. Consider passing the adapter as a parameter or restructuring.🔎 Suggested approach
Pass the validated adapter as a parameter to avoid the non-null assertion:
- override create(input: AgentCallArgs, ctx: AgentCallExtra): AgentContext<InSchema, OutSchema, In, Out> { + override create(input: AgentCallArgs, ctx: AgentCallExtra): AgentContext<InSchema, OutSchema, In, Out> { if (!this.llmAdapter) { throw new AgentNotConfiguredError(this.name); } - const agentCtorArgs = this.buildAgentCtorArgs(input, ctx); + const agentCtorArgs = this.buildAgentCtorArgs(input, ctx, this.llmAdapter); // ... } - private buildAgentCtorArgs(input: AgentCallArgs, ctx: AgentCallExtra): AgentCtorArgs<In> { + private buildAgentCtorArgs( + input: AgentCallArgs, + ctx: AgentCallExtra, + llmAdapter: AgentLlmAdapter + ): AgentCtorArgs<In> { // ... return { // ... - llmAdapter: this.llmAdapter!, + llmAdapter, // ... }; }
297-315: Switch statement relies on TypeScript exhaustiveness checking.The switch statement handles all
AgentKindenum values without a default case. The comment correctly notes that TypeScript ensures exhaustiveness. This is valid ifAgentKindis a proper enum, but consider adding a runtime safeguard for defensive coding.🔎 Optional defensive pattern
case AgentKind.FACTORY: return this.record.useFactory(this.providers) as AgentContext<InSchema, OutSchema, In, Out>; + + default: + // TypeScript exhaustiveness check - this should never be reached + const _exhaustiveCheck: never = this.record; + throw new Error(`Unhandled agent kind: ${(this.record as AgentRecord).kind}`); } - // Note: TypeScript already ensures exhaustiveness through AgentKind enum. - // The switch covers all AgentKind values, so this point is unreachable. }libs/sdk/src/common/metadata/agent.metadata.ts (1)
445-449: Consider using more specific schemas instead ofz.any().The
exportsConfigSchemausesz.any()for arrays, which bypasses validation. If possible, reference the specific schema types or use a union of known types.🔎 Suggested improvement
const exportsConfigSchema = z.object({ - resources: z.union([z.array(z.any()), z.literal('*')]).optional(), - prompts: z.union([z.array(z.any()), z.literal('*')]).optional(), - providers: z.array(z.any()).optional(), + resources: z.union([z.array(annotatedFrontMcpResourcesSchema), z.literal('*')]).optional(), + prompts: z.union([z.array(annotatedFrontMcpPromptsSchema), z.literal('*')]).optional(), + providers: z.array(annotatedFrontMcpProvidersSchema).optional(), });libs/sdk/src/agent/agent.utils.ts (1)
206-211: Replaceconsole.warnwith structured logger.Using
console.warndirectly bypasses any structured logging configuration and may not be appropriate for production environments. Consider accepting a logger parameter or using a scope-provided logger.🔎 Suggested approach
Option 1: Accept an optional logger parameter:
-export function agentToolName(agentId: string): string { +export function agentToolName(agentId: string, logger?: { warn: (msg: string) => void }): string { const sanitized = agentId .replace(/[^a-zA-Z0-9_-]/g, '_') .replace(/_{2,}/g, '_') .replace(/^_|_$/g, ''); if (sanitized !== agentId) { - console.warn( + (logger ?? console).warn( `Agent ID "${agentId}" was sanitized to "${sanitized}" for tool name. ` + `Different IDs may produce the same tool name - check for potential collisions.`, ); } return `${AGENT_TOOL_PREFIX}${sanitized}`; }Option 2: Return the warning as part of a result object for caller handling.
libs/sdk/src/agent/adapters/base.adapter.ts (1)
164-173: Consider documenting this method as provider-overridable.The string-based error matching is a reasonable base implementation but may not catch all provider-specific non-retryable errors. Subclasses should override this method for more accurate provider-specific error detection.
📝 Suggested JSDoc addition
/** * Check if an error should not be retried. + * Override in subclasses for provider-specific error detection. */ protected isNonRetryableError(error: Error): boolean {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
apps/demo/src/apps/weather/agents/summary.agent.tslibs/sdk/package.jsonlibs/sdk/src/agent/adapters/base.adapter.tslibs/sdk/src/agent/adapters/langchain.adapter.tslibs/sdk/src/agent/adapters/providers/index.tslibs/sdk/src/agent/adapters/providers/types.tslibs/sdk/src/agent/agent.instance.tslibs/sdk/src/agent/agent.registry.tslibs/sdk/src/agent/agent.scope.tslibs/sdk/src/agent/agent.utils.tslibs/sdk/src/common/decorators/agent.decorator.tslibs/sdk/src/common/entries/agent.entry.tslibs/sdk/src/common/metadata/agent.metadata.tslibs/sdk/src/common/metadata/tool.metadata.tslibs/sdk/src/errors/agent.errors.tslibs/sdk/src/errors/index.tslibs/sdk/src/plugin/plugin.registry.tslibs/sdk/src/tool/flows/call-tool.flow.tslibs/sdk/src/tool/tool.registry.tslibs/sdk/src/tool/tool.utils.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- libs/sdk/src/tool/tool.utils.ts
- libs/sdk/package.json
- libs/sdk/src/tool/flows/call-tool.flow.ts
- libs/sdk/src/agent/adapters/langchain.adapter.ts
- libs/sdk/src/agent/adapters/providers/types.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use type parameters with constraints instead of unconstrained generics
Do not mutate rawInput in flows - use state.set() for managing flow state instead
Files:
libs/sdk/src/agent/adapters/providers/index.tslibs/sdk/src/common/metadata/tool.metadata.tslibs/sdk/src/common/decorators/agent.decorator.tslibs/sdk/src/common/entries/agent.entry.tslibs/sdk/src/agent/agent.scope.tslibs/sdk/src/agent/agent.instance.tslibs/sdk/src/errors/index.tslibs/sdk/src/agent/agent.registry.tslibs/sdk/src/plugin/plugin.registry.tslibs/sdk/src/common/metadata/agent.metadata.tslibs/sdk/src/errors/agent.errors.tslibs/sdk/src/tool/tool.registry.tsapps/demo/src/apps/weather/agents/summary.agent.tslibs/sdk/src/agent/agent.utils.tslibs/sdk/src/agent/adapters/base.adapter.ts
libs/sdk/src/**/index.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Export public API through barrel files - export users' needed items, no legacy exports with different names
Files:
libs/sdk/src/agent/adapters/providers/index.tslibs/sdk/src/errors/index.ts
libs/sdk/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/sdk/src/**/*.ts: MCP response types should use strict MCP protocol types (GetPromptResult, ReadResourceResult) instead ofunknown
Use specific error classes with MCP error codes instead of generic errors
UsegetCapabilities()method for dynamic capability exposure instead of hardcoding capabilities
UsechangeScopeproperty instead ofscopein change events to avoid confusion with Scope class
Validate URIs per RFC 3986 at metadata level using proper validation
Validate hook flows and fail fast on invalid hook configurations with specific error messages
Files:
libs/sdk/src/agent/adapters/providers/index.tslibs/sdk/src/common/metadata/tool.metadata.tslibs/sdk/src/common/decorators/agent.decorator.tslibs/sdk/src/common/entries/agent.entry.tslibs/sdk/src/agent/agent.scope.tslibs/sdk/src/agent/agent.instance.tslibs/sdk/src/errors/index.tslibs/sdk/src/agent/agent.registry.tslibs/sdk/src/plugin/plugin.registry.tslibs/sdk/src/common/metadata/agent.metadata.tslibs/sdk/src/errors/agent.errors.tslibs/sdk/src/tool/tool.registry.tslibs/sdk/src/agent/agent.utils.tslibs/sdk/src/agent/adapters/base.adapter.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/sdk/src/agent/adapters/providers/index.tslibs/sdk/src/common/metadata/tool.metadata.tslibs/sdk/src/common/decorators/agent.decorator.tslibs/sdk/src/common/entries/agent.entry.tslibs/sdk/src/agent/agent.scope.tslibs/sdk/src/agent/agent.instance.tslibs/sdk/src/errors/index.tslibs/sdk/src/agent/agent.registry.tslibs/sdk/src/plugin/plugin.registry.tslibs/sdk/src/common/metadata/agent.metadata.tslibs/sdk/src/errors/agent.errors.tslibs/sdk/src/tool/tool.registry.tslibs/sdk/src/agent/agent.utils.tslibs/sdk/src/agent/adapters/base.adapter.ts
apps/demo/**
⚙️ CodeRabbit configuration file
apps/demo/**: apps/demo directory contains a demo application for testing purposes. It can be used as a reference for SDK usage examples.
Files:
apps/demo/src/apps/weather/agents/summary.agent.ts
🧠 Learnings (9)
📚 Learning: 2025-12-24T00:41:55.397Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:55.397Z
Learning: Applies to libs/uipack/src/**/*.{ts,tsx} : Use `.strict()` on all Zod schemas for validation
Applied to files:
libs/sdk/src/common/metadata/tool.metadata.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: FrontMCP is a TypeScript-first schema validation framework - all types should align with MCP protocol definitions
Applied to files:
libs/sdk/src/common/metadata/tool.metadata.tslibs/sdk/src/common/decorators/agent.decorator.tslibs/sdk/src/common/metadata/agent.metadata.ts
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/src/react/hooks/**/*.{ts,tsx} : MCP bridge hooks (useMcpBridge, useCallTool, useToolInput, useToolOutput, useTheme) must be properly typed and handle loading/error states
Applied to files:
libs/sdk/src/common/metadata/tool.metadata.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : Use `changeScope` property instead of `scope` in change events to avoid confusion with Scope class
Applied to files:
libs/sdk/src/agent/agent.scope.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/index.ts : Export public API through barrel files - export users' needed items, no legacy exports with different names
Applied to files:
libs/sdk/src/errors/index.tslibs/sdk/src/agent/agent.registry.tslibs/sdk/src/agent/agent.utils.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : Use specific error classes with MCP error codes instead of generic errors
Applied to files:
libs/sdk/src/errors/index.tslibs/sdk/src/errors/agent.errors.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : MCP response types should use strict MCP protocol types (GetPromptResult, ReadResourceResult) instead of `unknown`
Applied to files:
libs/sdk/src/common/metadata/agent.metadata.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/common/records/**/*.ts : Centralize record types in common/records directory and import from there instead of module-specific files
Applied to files:
libs/sdk/src/agent/agent.utils.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : Use `getCapabilities()` method for dynamic capability exposure instead of hardcoding capabilities
Applied to files:
libs/sdk/src/agent/agent.utils.ts
🧬 Code graph analysis (5)
libs/sdk/src/agent/adapters/providers/index.ts (3)
libs/sdk/src/agent/adapters/providers/types.ts (2)
ProviderCommonOptions(21-24)LlmProvider(12-12)libs/sdk/src/agent/adapters/base.adapter.ts (1)
LlmAdapterError(201-212)libs/sdk/src/agent/adapters/langchain.adapter.ts (1)
LangChainChatModel(21-31)
libs/sdk/src/common/entries/agent.entry.ts (6)
libs/sdk/src/common/metadata/tool.metadata.ts (2)
ToolInputType(160-160)ToolOutputType(159-159)libs/sdk/src/common/decorators/agent.decorator.ts (2)
AgentInputOf(121-121)AgentOutputOf(154-158)libs/sdk/src/common/interfaces/agent.interface.ts (3)
AgentContext(89-453)input(275-277)input(279-286)libs/sdk/src/common/metadata/agent.metadata.ts (1)
AgentMetadata(268-381)libs/sdk/src/common/entries/base.entry.ts (1)
EntryOwnerRef(5-5)libs/sdk/src/common/entries/tool.entry.ts (1)
SafeTransformResult(18-18)
libs/sdk/src/agent/agent.scope.ts (13)
libs/sdk/src/scope/scope.instance.ts (1)
Scope(36-360)libs/sdk/src/common/entries/base.entry.ts (1)
EntryOwnerRef(5-5)libs/sdk/src/plugin/plugin.registry.ts (1)
PluginRegistry(17-173)libs/sdk/src/tool/tool.registry.ts (1)
ToolRegistry(23-436)libs/sdk/src/resource/resource.registry.ts (1)
ResourceRegistry(39-565)libs/sdk/src/prompt/prompt.registry.ts (1)
PromptRegistry(37-464)libs/sdk/src/agent/agent.registry.ts (1)
AgentRegistry(45-405)libs/sdk/src/hooks/hook.registry.ts (1)
HookRegistry(19-195)libs/sdk/src/flows/flow.registry.ts (1)
FlowRegistry(10-144)libs/sdk/src/common/metadata/agent.metadata.ts (1)
AgentMetadata(268-381)libs/sdk/src/tool/tool.utils.ts (1)
normalizeTool(61-82)libs/sdk/src/tool/tool.instance.ts (1)
ToolInstance(29-147)libs/sdk/src/common/interfaces/flow.interface.ts (3)
FlowType(112-112)FlowInputOf(10-10)FlowOutputOf(11-11)
libs/sdk/src/errors/agent.errors.ts (1)
libs/sdk/src/errors/index.ts (11)
AgentNotFoundError(51-51)PublicMcpError(4-4)AgentExecutionError(52-52)InternalMcpError(5-5)AgentLoopExceededError(53-53)AgentTimeoutError(54-54)AgentVisibilityError(55-55)AgentLlmError(56-56)AgentConfigurationError(57-57)AgentNotConfiguredError(58-58)AgentToolNotFoundError(59-59)
libs/sdk/src/agent/adapters/base.adapter.ts (1)
libs/sdk/src/common/interfaces/llm-adapter.interface.ts (7)
AgentLlmAdapter(232-260)AgentPrompt(104-114)AgentToolDefinition(79-99)AgentCompletionOptions(119-145)AgentCompletion(150-179)AgentCompletionChunk(184-204)AgentMessage(29-54)
🔇 Additional comments (40)
libs/sdk/src/agent/adapters/providers/index.ts (4)
1-9: LGTM!The module documentation clearly describes the purpose, and the barrel export pattern aligns with the coding guidelines for public API exports.
44-83: LGTM!The optional provider mapping and dynamic import logic are well-structured. The error handling uses specific error classes with appropriate error codes, and the error messages provide clear, actionable guidance for users.
158-166: LGTM!The synchronous wrapper using deferred initialization is a pragmatic solution for supporting provider adapters in synchronous contexts while still enabling dynamic imports.
201-237: Fallback streaming behavior is correct and matches consumer expectations.The implementation properly converts non-streaming completions into streaming chunks with the correct sequence: tool calls first (each emitted individually), content second (if present), and done last with the full completion. The chunk structure and type assertions are appropriate and align with how consumers process these chunks throughout the codebase.
libs/sdk/src/plugin/plugin.registry.ts (1)
35-35: LGTM! Type extension properly supports agent ownership.The owner type extension to include
'agent'as a kind is consistent with the broader agent architecture introduced in this PR. The assignment on line 44 ensures the owner is properly stored for hook registration (lines 156-163).Also applies to: 40-40, 44-44
apps/demo/src/apps/weather/agents/summary.agent.ts (1)
8-14: LGTM! Schemas are properly defined.The inputSchema and outputSchema use Zod v4 features correctly with proper descriptions and optional fields. The raw shape format is consistent with
ToolInputTypeandToolOutputTypedefinitions in the SDK.libs/sdk/src/agent/agent.scope.ts (5)
84-99: LGTM! Constructor properly initializes agent scope.The constructor correctly sets up the agent scope by inheriting path and routing information from the parent scope, creating a unique agent owner descriptor, and initializing the ready promise for async initialization.
106-149: LGTM! Provider and registry initialization is comprehensive.The initialization properly sets up agent-scoped providers with the AgentScopeEntry registered as both
ScopeEntryandScope. This ensures flows can access the agent scope correctly viagetActiveScope(). The inclusion ofCallToolFlow(line 148) enables standard tool execution routing through the flow system.
169-180: LGTM! Tool initialization with proper error handling.The tool initialization correctly normalizes tools from metadata, creates tool instances with agent-scoped providers and owner, and includes comprehensive error handling that logs and re-throws failures. The fail-fast behavior ensures agents don't start with misconfigured tools.
182-197: LGTM! Resource, prompt, and nested agent registries properly initialized.The initialization of resource, prompt, and nested agent registries follows consistent patterns. The support for nested agents (line 191) enables agent composition, and the informational logging provides useful initialization metrics.
293-363: LGTM! AgentScopeEntry provides clean adapter pattern.The
AgentScopeEntryclass effectively adaptsAgentScopeto theScopeEntryinterface through delegation. This enables agent scopes to be used in flows and other components expecting aScopeEntry, maintaining clean separation of concerns. The class is correctly kept internal (non-exported).libs/sdk/src/errors/index.ts (1)
49-60: LGTM! Agent error classes properly exported.The export of agent-specific error classes follows the established barrel file pattern and aligns with the coding guideline to use specific error classes with MCP error codes. The error class names are descriptive and follow consistent naming conventions.
Based on learnings: Export pattern matches expected public API conventions.
libs/sdk/src/common/metadata/tool.metadata.ts (2)
4-4: LGTM! Reserved prefix validation is clear and well-implemented.The reserved prefix validation prevents naming conflicts with agent invocations. The validation function is straightforward, and the error message clearly explains the restriction. The case-sensitive check using
startsWith()is appropriate for tool identifiers.Also applies to: 252-256
260-261: LGTM! Schema refinements correctly applied.The reserved prefix validation is correctly chained in both the
idandnamefields:
- For
id(line 260): The refinement runs before.optional(), ensuring the prefix check applies when a value is provided- For
name(line 261): The refinement chains after.min(1), validating non-empty strings don't use reserved prefixesThe implementation correctly uses Zod v4's refinement chaining.
libs/sdk/src/common/entries/agent.entry.ts (2)
53-98: Well-structured abstract class with proper MCP type usage.The
AgentEntryclass design is solid:
- Uses proper MCP protocol types (
CallToolResult,Tool)- Generic constraints with
ToolInputType/ToolOutputTypeenable type-safe schema handling- Abstract methods provide a clean contract for concrete implementations
85-88: Useunknowninstead ofanyfor rawInputSchema.Per coding guidelines,
anytypes should be avoided without strong justification. TherawInputSchemaproperty should useunknownsince it represents an arbitrary JSON schema structure.🔎 Suggested fix
/** * Raw JSON schema for the input (used in tool definition). */ - rawInputSchema?: JsonSchema; + rawInputSchema?: unknown;Note: If the
JsonSchematype fromzod/v4/coreis intended, that's already a good choice. However, ifanyis being used elsewhere for this property, replace withunknown.⛔ Skipped due to learnings
Learnt from: CR Repo: agentfront/frontmcp PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-19T02:04:46.475Z Learning: Applies to **/*.ts : Use strict TypeScript mode with no `any` types without strong justification - use `unknown` instead for generic type defaultsLearnt from: CR Repo: agentfront/frontmcp PR: 0 File: libs/uipack/CLAUDE.md:0-0 Timestamp: 2025-12-24T00:41:55.397Z Learning: Applies to libs/uipack/src/**/*.{ts,tsx} : Avoid using `any` type without justification in TypeScriptLearnt from: CR Repo: agentfront/frontmcp PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-19T02:04:46.475Z Learning: Applies to libs/sdk/src/**/*.ts : MCP response types should use strict MCP protocol types (GetPromptResult, ReadResourceResult) instead of `unknown`Learnt from: CR Repo: agentfront/frontmcp PR: 0 File: libs/uipack/CLAUDE.md:0-0 Timestamp: 2025-12-24T00:41:55.397Z Learning: Applies to libs/uipack/src/**/*.{ts,tsx} : Use `.strict()` on all Zod schemas for validationLearnt from: CR Repo: agentfront/frontmcp PR: 0 File: libs/ui/CLAUDE.md:0-0 Timestamp: 2025-12-24T00:41:41.819Z Learning: Applies to libs/ui/src/**/*.{ts,tsx} : Avoid using any type without justification; all props, return types, and generics must be properly typedlibs/sdk/src/agent/agent.registry.ts (4)
204-209: Justified use ofanyfor heterogeneous collections.The
anytype usage is explicitly documented with a comment explaining the intentional decision for heterogeneous agent collections. This is acceptable given the runtime nature of agent type variations.
332-351: Proper use ofchangeScopein events per coding guidelines.The subscription and event emission correctly use
changeScope: 'global'instead ofscopeto avoid confusion with the Scope class. This aligns with the coding guidelines.
392-404: Dynamic capabilities exposure follows best practices.The
getCapabilities()method properly returns dynamic capabilities based on registry state (hasAny()), avoiding hardcoded capability values as per coding guidelines.
8-8: No issues found. The import pathfrom '../regsitry'is correct and resolves properly to the actual directorylibs/sdk/src/regsitry/registry.base.ts, which exports bothRegistryAbstractandRegistryBuildMapResult.libs/sdk/src/common/decorators/agent.decorator.ts (4)
6-7: Forward reference type stub is appropriate for circular dependency avoidance.The
AgentContextBasetype stub provides the minimal contract needed without importing the fullAgentContextclass, which would create a circular dependency. This is a valid pattern.
115-116: Internal type aliases prefixed with__follow private convention.The double-underscore prefix (
__Shape,__AsZodObj, etc.) correctly signals these are internal implementation details not meant for external consumption.
255-277: Module augmentation with@ts-expect-erroris correctly used.The
@ts-expect-errordirectives are necessary for module augmentation where TypeScript cannot verify the overload compatibility at declaration time. The comments explain the rationale.
143-143: Fallback toanyin type inference is intentional.The
anyfallback in__InferFromSingleSchema(line 143) andAgentOutputOf(line 158) handles cases where no schema is provided. This is justified as the decorator needs to support optional schemas.libs/sdk/src/agent/agent.instance.ts (3)
220-245: Excellent hook validation with fail-fast pattern.The hook registration properly validates flow names against
VALID_AGENT_HOOK_FLOWSand throwsInvalidHookFlowErrorimmediately for invalid hooks. This follows the coding guideline to "validate hook flows and fail fast on invalid hook configurations with specific error messages."
412-445: Robust MCP CallToolResult extraction with proper error handling.The
extractToolResultmethod correctly handles all MCP result formats:
- Error results with appropriate error throwing
structuredContentpreference over text content- JSON parsing fallback for text content
This aligns with MCP protocol handling requirements.
447-450: Review comment is based on a misunderstanding of the constructor logic and should be dismissed.The constructor (lines 107-109) explicitly ensures that
this.inputSchemais always a plain object by extracting.shapefrom anyz.ZodObjectinput:this.inputSchema = (schema instanceof z.ZodObject ? schema.shape : schema ?? {}) as InSchema;Therefore, the
z.object()call inparseInput(line 448) is wrapping a plain object into az.ZodObjectfor validation purposes—it is the first wrapping, not a double-wrap. No issue exists here.Likely an incorrect or invalid review comment.
libs/sdk/src/common/metadata/agent.metadata.ts (2)
482-504: Comprehensive metadata schema with proper validation constraints.The
frontMcpAgentMetadataSchemaprovides thorough runtime validation including:
- Required field validation (
name: z.string().min(1))- Nested schema validation for swarm, execution, exports
- Type-safe satisfies constraint with
RawZodShape<AgentMetadata, ExtendFrontMcpAgentMetadata>.passthrough()for extensibility
118-126: Token usage explained for circular dependency.The comment on lines 122-124 properly documents why
Token<unknown>is used instead ofToken<AgentLlmAdapter>- to avoid circular dependencies with runtime validation handling type checking. This is an acceptable trade-off.libs/sdk/src/agent/agent.utils.ts (3)
279-286: Doc comment says "Returns empty array" but function behavior matches.The JSDoc says "Returns empty array if agent cannot see swarm or has no specific visibility list" and the implementation returns
[]in both cases. However, the function signature returnsstring[]while the code returnsmetadata.swarm?.visibleAgents ?? []which could returnundefinedifvisibleAgentsis explicitlyundefined.This is actually correct since
?? []handles bothnullandundefined, returning an empty array. The implementation matches the documentation.
252-263: Validation uses specific error classes per guidelines.The
validateAgentMetadatafunction properly usesAgentConfigurationErrorinstead of genericError, following the coding guideline to use specific error classes with MCP error codes.
90-149: Comprehensive agent normalization handles all registration patterns.The
normalizeAgentfunction correctly handles:
- Class decorators (
@Agent)- Function factories (
agent())- Value objects (
useValue)- Factory objects (
useFactory)With a clear error message for unrecognized types.
libs/sdk/src/errors/agent.errors.ts (4)
12-19: Well-designed error class with appropriate HTTP status.
AgentNotFoundErrorcorrectly uses HTTP 404 and stores theagentIdfor debugging. The error codeAGENT_NOT_FOUNDis clear and specific.
24-43: Internal error with proper stack trace preservation.
AgentExecutionErrorcorrectly extendsInternalMcpErrorand overridesgetInternalMessage()to include the original error's stack trace when available. This aids debugging without exposing internals to clients.
144-158: Helpful error message includes available tools.
AgentToolNotFoundErrorprovides excellent debugging information by including the list of available tools in the error message. This helps developers quickly identify typos or misconfiguration.
1-158: Comprehensive agent error hierarchy follows MCP error patterns.The error classes properly:
- Use specific error codes following
AGENT_*naming convention- Distinguish public (client-safe) vs internal errors
- Include relevant context properties (
agentId,timeoutMs,maxIterations, etc.)- Use appropriate HTTP status codes per semantic meaning
This aligns with the coding guideline to use specific error classes with MCP error codes.
libs/sdk/src/agent/adapters/base.adapter.ts (4)
18-55: LGTM! Well-designed configuration interface.The configuration interface is clean and well-documented with appropriate defaults for timeout and maxRetries.
85-131: LGTM! Clean abstraction with proper TypeScript usage.The abstract methods provide clear extension points for provider-specific implementations, and the use of
unknownfor provider-specific types aligns with the strict TypeScript guidelines.
201-232: LGTM! Well-structured error hierarchy.The error classes provide a clean hierarchy with appropriate metadata (provider, code, statusCode, raw). The
retryAfterfield inLlmRateLimitErroris particularly useful for implementing compliant retry logic.
74-80: Provider adapters do not enforce the timeout configuration; timeout handling is delegated to underlying HTTP clients.The
timeoutconfig option (default 60000ms) is defined and stored in the base class constructor but is not used by any adapter implementation. The LangChain adapter doesn't inherit fromBaseLlmAdapterand doesn't include timeout in its ownLangChainAdapterConfig, relying instead on LangChain's model implementation to handle timeouts. If provider-specific adapters are added, ensure timeout logic is implemented in their HTTP clients or pass the timeout to the underlying SDK configuration.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
libs/sdk/src/agent/agent.registry.ts (1)
177-218: Add dispose() method for complete subscription cleanup.The code correctly addresses the memory leak from the previous review by storing unsubscribe functions (line 195) and providing
removeChildfor cleanup (lines 204-218). However, adispose()method is still needed to clean up ALL child subscriptions when the registry itself is disposed, not just when individual children are removed.🔎 Add dispose method
+ /** + * Clean up all subscriptions when disposing the registry. + */ + dispose(): void { + for (const unsubscribe of this.childSubscriptions.values()) { + unsubscribe(); + } + this.childSubscriptions.clear(); + this.children.clear(); + this.adopted.clear(); + }
🧹 Nitpick comments (1)
libs/sdk/src/agent/adapters/providers/index.ts (1)
137-144: Consider validating temperature and maxTokens ranges.The code accepts
temperatureandmaxTokensfrom user options without validation. Different providers have different valid ranges:
- OpenAI: temperature 0-2, maxTokens varies by model
- Anthropic: temperature 0-1, maxTokens varies by model
- Google: temperature 0-1, maxOutputTokens varies by model
Invalid values may cause runtime errors from the provider APIs. Consider adding validation with provider-specific constraints or documenting the expected ranges in
ProviderCommonOptions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
libs/sdk/eslint.config.mjslibs/sdk/src/agent/adapters/base.adapter.tslibs/sdk/src/agent/adapters/providers/index.tslibs/sdk/src/agent/agent.registry.tslibs/sdk/src/tool/tool.registry.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/sdk/src/tool/tool.registry.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use type parameters with constraints instead of unconstrained generics
Do not mutate rawInput in flows - use state.set() for managing flow state instead
Files:
libs/sdk/src/agent/adapters/providers/index.tslibs/sdk/src/agent/agent.registry.tslibs/sdk/src/agent/adapters/base.adapter.ts
libs/sdk/src/**/index.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Export public API through barrel files - export users' needed items, no legacy exports with different names
Files:
libs/sdk/src/agent/adapters/providers/index.ts
libs/sdk/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/sdk/src/**/*.ts: MCP response types should use strict MCP protocol types (GetPromptResult, ReadResourceResult) instead ofunknown
Use specific error classes with MCP error codes instead of generic errors
UsegetCapabilities()method for dynamic capability exposure instead of hardcoding capabilities
UsechangeScopeproperty instead ofscopein change events to avoid confusion with Scope class
Validate URIs per RFC 3986 at metadata level using proper validation
Validate hook flows and fail fast on invalid hook configurations with specific error messages
Files:
libs/sdk/src/agent/adapters/providers/index.tslibs/sdk/src/agent/agent.registry.tslibs/sdk/src/agent/adapters/base.adapter.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/sdk/src/agent/adapters/providers/index.tslibs/sdk/src/agent/agent.registry.tslibs/sdk/src/agent/adapters/base.adapter.tslibs/sdk/eslint.config.mjs
🧠 Learnings (5)
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : MCP response types should use strict MCP protocol types (GetPromptResult, ReadResourceResult) instead of `unknown`
Applied to files:
libs/sdk/src/agent/adapters/providers/index.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to **/*.ts : Avoid non-null assertions (`!`) - use proper error handling and throw specific errors instead
Applied to files:
libs/sdk/src/agent/adapters/providers/index.ts
📚 Learning: 2025-12-24T00:41:55.397Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:55.397Z
Learning: Applies to libs/uipack/src/**/*.{ts,tsx} : Avoid using `any` type without justification in TypeScript
Applied to files:
libs/sdk/src/agent/adapters/providers/index.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/index.ts : Export public API through barrel files - export users' needed items, no legacy exports with different names
Applied to files:
libs/sdk/src/agent/agent.registry.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : Validate hook flows and fail fast on invalid hook configurations with specific error messages
Applied to files:
libs/sdk/src/agent/adapters/base.adapter.ts
🧬 Code graph analysis (2)
libs/sdk/src/agent/adapters/providers/index.ts (3)
libs/sdk/src/agent/adapters/providers/types.ts (2)
ProviderCommonOptions(21-24)LlmProvider(12-12)libs/sdk/src/agent/adapters/base.adapter.ts (1)
LlmAdapterError(219-230)libs/sdk/src/agent/adapters/langchain.adapter.ts (1)
LangChainChatModel(21-31)
libs/sdk/src/agent/adapters/base.adapter.ts (1)
libs/sdk/src/common/interfaces/llm-adapter.interface.ts (7)
AgentLlmAdapter(232-260)AgentPrompt(104-114)AgentToolDefinition(79-99)AgentCompletionOptions(119-145)AgentCompletion(150-179)AgentCompletionChunk(184-204)AgentMessage(29-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Libraries
- GitHub Check: Lint & Format Checks
🔇 Additional comments (18)
libs/sdk/src/agent/agent.registry.ts (8)
1-29: LGTM!Imports and type definitions are well-structured. The
IndexedAgentinterface clearly captures the shape of indexed agent entries with proper typing.
45-78: LGTM!Class structure is well-designed. The addition of the
childSubscriptionsmap at lines 58-59 shows proper subscription lifecycle management, addressing memory leak concerns.
84-118: LGTM!Build-time validation properly implements fail-fast behavior by validating dependencies against the provider registry and throwing specific
DependencyNotFoundErrorinstances with helpful context.
124-168: LGTM!Initialization logic properly handles multi-level adoption (from app registries and other agent registries) and correctly awaits all agent instances to be ready before completing.
228-319: LGTM!Accessor methods are well-implemented. The intentional use of
anyfor heterogeneous agent collections (lines 228, 238) is properly justified with comments, and all other methods use strict typing. MCP protocol types (Tool[]) are used appropriately.
325-349: LGTM!The reindex logic uses a safe clear-then-rebuild pattern that maintains consistency across multiple index types.
355-374: LGTM!Subscription implementation correctly uses the
changeScopeproperty (lines 363, 373) instead ofscope, following the coding guidelines to avoid confusion with the Scope class.
380-450: LGTM!Helper methods are well-implemented. The
getCapabilities()method correctly uses MCP protocol types (ServerCapabilities) and the lineage management utilities properly handle deduplication.libs/sdk/src/agent/adapters/base.adapter.ts (2)
100-209: LGTM! Solid retry logic and well-designed abstractions.The implementation demonstrates strong architectural decisions:
- Default streaming: The fallback implementation (lines 113-121) is appropriate for adapters without native streaming support.
- Retry logic: Exponential backoff with capped delays (10s max) and non-retryable error detection provides robust error handling.
- Abstract methods: Proper use of
unknowntypes for provider-specific formatting maintains type safety without over-constraining subclasses.- Option merging: Clean defaults cascade from config to per-call options.
The retry loop correctly executes
maxRetries + 1attempts (initial try + retries), and the non-retryable error detection provides sensible defaults that subclasses can override.
212-250: LGTM! Well-structured error hierarchy.The error classes provide specific, actionable error types:
- LlmAdapterError: Comprehensive base with provider, code, statusCode, and raw response context.
- LlmRateLimitError: Includes
retryAfterfor intelligent retry scheduling.- LlmContextLengthError: Specific type for a common failure mode.
Each properly sets
this.namefor error identification, and the hierarchy allows both type-based and code-based error handling.libs/sdk/src/agent/adapters/providers/index.ts (7)
55-83: Excellent error handling and user guidance.The
importOptionalProviderfunction provides clear, actionable error messages with installation instructions and usage examples. This significantly improves the developer experience when optional providers are missing.
122-125: LGTM! Non-null assertion concern from past review has been addressed.The code now includes an explicit null check for
providerInfo(lines 123-125) with a descriptive error message, eliminating the non-null assertion that was flagged in previous reviews. This follows the coding guideline to "use proper error handling and throw specific errors instead" of non-null assertions.
150-156: Default case is unreachable but acceptable as defensive coding.TypeScript's exhaustiveness checking makes this default case unreachable since all
LlmProviderunion members are handled in the switch. However, keeping this default case is acceptable as defensive coding practice and protects against future changes to theLlmProvidertype.
161-169: LGTM! Deferred initialization pattern is appropriate.The synchronous wrapper using
DeferredProviderAdapteris a sound approach for handling providers that require async initialization in synchronous contexts. The initialization is properly deferred until first use.
171-241: Excellent implementation of lazy initialization and streaming fallback.The
DeferredProviderAdapterclass demonstrates:
- Proper lazy initialization with promise caching (lines 180-193)
- Correct delegation to the underlying adapter
- Well-designed fallback from streaming to non-streaming (lines 212-239), yielding tool calls, content, and completion in sequence
The implementation correctly handles both streaming and non-streaming adapters and properly types all chunks.
94-104: Type assertions are justified by architectural design and are properly safeguarded.The
LangChainChatModelinterface is intentionally minimal to avoid importing@langchain/core. Theas unknown as LangChainChatModelpattern at lines 103 and 114 is a recognized idiom for intentional downcasting, and is safe because:
- ChatOpenAI and ChatAnthropic from LangChain implement all required methods (
invoke,stream,bindTools)- Runtime checks at lines 241, 297, and 312 verify optional methods before use
- The adapter gracefully handles models with or without streaming/tool support
The null check for
providerInfoat line 122–125 is properly handled with an explicit error throw, not a non-null assertion—no issue here.Note: The original mention of CVE-2025-68665 for
@langchain/anthropic@1.3.3could not be verified against current security databases and warrants a manual check with your security tools.
107-115: The type assertion at line 114 is intentional and justified. The code uses duck-typing withLangChainChatModelas a minimal interface to avoid unnecessary imports from@langchain/core. This pattern is applied consistently across all providers (e.g., OpenAI at line 103) and enables structural compatibility without bloating the import footprint. This approach aligns with the design goal shown in the interface comments.CVE-2025-68665 does not affect
@langchain/anthropicdirectly. The vulnerability exists in@langchain/coreandlangchainpackages. If a transitive dependency upgrade is needed, it should target those packages based on actual installed versions, not@langchain/anthropic.Likely an incorrect or invalid review comment.
libs/sdk/eslint.config.mjs (1)
12-13: LGTM! ESLint configuration aligns with dynamic import strategy.Adding
ignoredDependenciesfor the optional LangChain providers is the correct approach to prevent false positives from the dependency-checks rule. These packages are loaded dynamically viaimport()and are not required dependencies, so excluding them from static analysis is appropriate.The comment clearly explains the rationale for the exclusion.
# Conflicts: # libs/sdk/src/common/metadata/app.metadata.ts # libs/sdk/src/plugin/plugin.registry.ts # yarn.lock
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
libs/sdk/src/agent/agent.registry.ts (1)
177-199: Memory leak fix properly implemented.The subscription pattern now correctly stores unsubscribe functions in
childSubscriptionsMap, and bothremoveChildanddisposemethods properly clean up subscriptions. This addresses the concern from the previous review.Also applies to: 204-233
libs/sdk/src/common/metadata/agent.metadata.ts (1)
115-118:AgentLlmAdapterLocalstill usesunknowntypes instead of proper types.This was flagged in a previous review. The interface uses
unknownfor all parameters and return types, circumventing type safety. Per the coding guidelines, this should use strict types or resolve the circular dependency properly.
🧹 Nitpick comments (8)
libs/sdk/src/tool/flows/call-tool.flow.ts (1)
10-10: Consider removing unused import.The
ToolCtorArgsimport is not referenced anywhere in this file.🔎 Proposed fix
import { Flow, FlowBase, FlowHooksOf, FlowPlan, FlowRunOptions, ToolContext, - ToolCtorArgs, ToolEntry, isOrchestratedMode, } from '../../common';libs/sdk/src/common/interfaces/agent.interface.ts (2)
115-120: Avoidanytypes - useunknowninstead.Per coding guidelines, strict TypeScript mode requires avoiding
anytypes. The_rawInputand_outputDraftbacking fields useanywhich circumvents type safety.🔎 Suggested fix
// ---- INPUT storages (backing fields) - private _rawInput?: Partial<In> | any; + private _rawInput?: Partial<In> | unknown; private _input?: In; // ---- OUTPUT storages (backing fields) - private _outputDraft?: Partial<Out> | any; + private _outputDraft?: Partial<Out> | unknown; private _output?: Out;
360-374: Use specific MCP error classes instead of genericError.Per coding guidelines, the SDK should use specific error classes with MCP error codes. Both
executeToolandinvokeAgentthrow genericErrorinstances.🔎 Suggested fix
+import { ToolExecutionError, AgentInvocationError } from '../../errors/mcp.error'; protected async executeTool(name: string, args: Record<string, unknown>): Promise<unknown> { // This will be implemented by AgentInstance which has access to ToolRegistry - throw new Error(`Tool execution not available. Tool: ${name}`); + throw new ToolExecutionError(`Tool execution not available. Tool: ${name}`); } protected async invokeAgent(agentId: string, input: unknown): Promise<unknown> { // This will be implemented by AgentInstance which has access to AgentRegistry - throw new Error(`Agent invocation not available. Agent: ${agentId}`); + throw new AgentInvocationError(`Agent invocation not available. Agent: ${agentId}`); }libs/sdk/src/agent/agent.registry.ts (1)
281-284:findByNamereturns only the first match without disambiguation.When multiple agents have the same base name (from different sources), this returns the first one arbitrarily. Consider logging a warning or providing a method to get all matches.
🔎 Suggested improvement
findByName(name: string): AgentInstance | undefined { const rows = this.byName.get(name); + if (rows && rows.length > 1) { + this.logger?.warn?.(`Multiple agents found with name '${name}', returning first match`); + } return rows?.[0]?.instance; }libs/sdk/src/common/metadata/agent.metadata.ts (2)
453-457:exportsConfigSchemausesz.any()for array contents.This reduces type safety. Consider using the annotated schemas for resources, prompts, and providers.
🔎 Suggested fix
const exportsConfigSchema = z.object({ - resources: z.union([z.array(z.any()), z.literal('*')]).optional(), - prompts: z.union([z.array(z.any()), z.literal('*')]).optional(), - providers: z.array(z.any()).optional(), + resources: z.union([z.array(annotatedFrontMcpResourcesSchema), z.literal('*')]).optional(), + prompts: z.union([z.array(annotatedFrontMcpPromptsSchema), z.literal('*')]).optional(), + providers: z.array(annotatedFrontMcpProvidersSchema).optional(), });
468-469: Schema validation forinputSchema/outputSchemais weak.Using
z.instanceof(Object)andz.any()doesn't validate that these are actual Zod schemas. Consider a custom validator that checks for Zod schema characteristics.🔎 Suggested improvement
+// Helper to validate Zod-like schema objects +const zodSchemaValidator = z.custom( + (v) => v !== null && typeof v === 'object' && ('_def' in v || 'parse' in v), + { message: 'Must be a Zod schema' } +); // In frontMcpAgentMetadataSchema: - inputSchema: z.instanceof(Object).optional(), - outputSchema: z.any().optional(), + inputSchema: zodSchemaValidator.optional(), + outputSchema: zodSchemaValidator.optional(),libs/sdk/src/agent/adapters/providers/index.ts (1)
213-249: Consider edge case: completion with no tool calls or content.The fallback streaming implementation (lines 222-248) correctly converts non-streaming completions to chunks. However, if
result.toolCallsis empty/undefined ANDresult.contentis empty/undefined, the generator will only yield a singledonechunk (lines 244-247).This is probably fine for most use cases, but consider whether you want to explicitly handle or document this edge case where the LLM returns an empty response.
📝 Optional: Add comment documenting the empty response case
} else { // Fallback to non-streaming - yield chunks in sequence const result = await adapter.completion(prompt, tools, options); + // Note: If result has neither toolCalls nor content, only the done chunk is yielded // Yield tool calls if (result.toolCalls) {libs/sdk/src/agent/adapters/base.adapter.ts (1)
154-209: LGTM! Solid retry logic with exponential backoff.The retry implementation includes:
- Exponential backoff capped at 10 seconds (lines 169-171)
- Non-retryable error detection (lines 182-191)
- Proper error propagation (lines 164-166)
The string-based error detection in
isNonRetryableErroris somewhat fragile but acceptable as a base implementation that subclasses can override for provider-specific error handling.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/demo/src/main.tslibs/sdk/src/agent/adapters/base.adapter.tslibs/sdk/src/agent/adapters/providers/index.tslibs/sdk/src/agent/agent.registry.tslibs/sdk/src/agent/index.tslibs/sdk/src/app/instances/app.local.instance.tslibs/sdk/src/common/interfaces/agent.interface.tslibs/sdk/src/common/metadata/agent.metadata.tslibs/sdk/src/common/metadata/app.metadata.tslibs/sdk/src/common/schemas/annotated-class.schema.tslibs/sdk/src/plugin/plugin.registry.tslibs/sdk/src/tool/flows/call-tool.flow.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/sdk/src/plugin/plugin.registry.ts
- apps/demo/src/main.ts
- libs/sdk/src/app/instances/app.local.instance.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use type parameters with constraints instead of unconstrained generics
Do not mutate rawInput in flows - use state.set() for managing flow state instead
Files:
libs/sdk/src/common/interfaces/agent.interface.tslibs/sdk/src/common/metadata/app.metadata.tslibs/sdk/src/tool/flows/call-tool.flow.tslibs/sdk/src/agent/agent.registry.tslibs/sdk/src/common/metadata/agent.metadata.tslibs/sdk/src/agent/adapters/base.adapter.tslibs/sdk/src/agent/adapters/providers/index.tslibs/sdk/src/common/schemas/annotated-class.schema.tslibs/sdk/src/agent/index.ts
libs/sdk/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/sdk/src/**/*.ts: MCP response types should use strict MCP protocol types (GetPromptResult, ReadResourceResult) instead ofunknown
Use specific error classes with MCP error codes instead of generic errors
UsegetCapabilities()method for dynamic capability exposure instead of hardcoding capabilities
UsechangeScopeproperty instead ofscopein change events to avoid confusion with Scope class
Validate URIs per RFC 3986 at metadata level using proper validation
Validate hook flows and fail fast on invalid hook configurations with specific error messages
Files:
libs/sdk/src/common/interfaces/agent.interface.tslibs/sdk/src/common/metadata/app.metadata.tslibs/sdk/src/tool/flows/call-tool.flow.tslibs/sdk/src/agent/agent.registry.tslibs/sdk/src/common/metadata/agent.metadata.tslibs/sdk/src/agent/adapters/base.adapter.tslibs/sdk/src/agent/adapters/providers/index.tslibs/sdk/src/common/schemas/annotated-class.schema.tslibs/sdk/src/agent/index.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/sdk/src/common/interfaces/agent.interface.tslibs/sdk/src/common/metadata/app.metadata.tslibs/sdk/src/tool/flows/call-tool.flow.tslibs/sdk/src/agent/agent.registry.tslibs/sdk/src/common/metadata/agent.metadata.tslibs/sdk/src/agent/adapters/base.adapter.tslibs/sdk/src/agent/adapters/providers/index.tslibs/sdk/src/common/schemas/annotated-class.schema.tslibs/sdk/src/agent/index.ts
libs/sdk/src/**/index.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Export public API through barrel files - export users' needed items, no legacy exports with different names
Files:
libs/sdk/src/agent/adapters/providers/index.tslibs/sdk/src/agent/index.ts
🧠 Learnings (8)
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: FrontMCP is a TypeScript-first schema validation framework - all types should align with MCP protocol definitions
Applied to files:
libs/sdk/src/common/metadata/app.metadata.tslibs/sdk/src/common/metadata/agent.metadata.tslibs/sdk/src/common/schemas/annotated-class.schema.ts
📚 Learning: 2025-12-24T00:41:41.819Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/ui/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:41.819Z
Learning: Applies to libs/ui/src/react/hooks/**/*.{ts,tsx} : MCP bridge hooks (useMcpBridge, useCallTool, useToolInput, useToolOutput, useTheme) must be properly typed and handle loading/error states
Applied to files:
libs/sdk/src/tool/flows/call-tool.flow.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : MCP response types should use strict MCP protocol types (GetPromptResult, ReadResourceResult) instead of `unknown`
Applied to files:
libs/sdk/src/common/metadata/agent.metadata.tslibs/sdk/src/agent/adapters/providers/index.tslibs/sdk/src/common/schemas/annotated-class.schema.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : Validate hook flows and fail fast on invalid hook configurations with specific error messages
Applied to files:
libs/sdk/src/agent/adapters/base.adapter.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to **/*.ts : Avoid non-null assertions (`!`) - use proper error handling and throw specific errors instead
Applied to files:
libs/sdk/src/agent/adapters/providers/index.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/index.ts : Export public API through barrel files - export users' needed items, no legacy exports with different names
Applied to files:
libs/sdk/src/agent/index.ts
📚 Learning: 2025-12-24T00:41:55.397Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: libs/uipack/CLAUDE.md:0-0
Timestamp: 2025-12-24T00:41:55.397Z
Learning: Applies to libs/uipack/src/*/index.ts : Implement entry point re-exports in index.ts files for each module (adapters, build, bundler, etc.)
Applied to files:
libs/sdk/src/agent/index.ts
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/common/records/**/*.ts : Centralize record types in common/records directory and import from there instead of module-specific files
Applied to files:
libs/sdk/src/agent/index.ts
🧬 Code graph analysis (1)
libs/sdk/src/common/interfaces/agent.interface.ts (6)
libs/sdk/src/common/interfaces/execution-context.interface.ts (1)
ExecutionContextBaseArgs(17-21)libs/sdk/src/common/metadata/agent.metadata.ts (1)
AgentMetadata(282-395)libs/sdk/src/common/interfaces/llm-adapter.interface.ts (7)
AgentLlmAdapter(232-260)AgentToolDefinition(79-99)AgentToolCall(59-74)AgentPrompt(104-114)AgentCompletionOptions(119-145)AgentCompletion(150-179)AgentCompletionChunk(184-204)libs/sdk/src/common/interfaces/internal/registry.interface.ts (1)
ProviderRegistryInterface(84-95)libs/sdk/src/agent/agent-execution-loop.ts (2)
ToolExecutor(128-128)AgentExecutionLoop(162-592)libs/sdk/src/common/interfaces/flow.interface.ts (1)
FlowControl(19-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Libraries
- GitHub Check: Lint & Format Checks
🔇 Additional comments (20)
libs/sdk/src/tool/flows/call-tool.flow.ts (2)
140-141: LGTM - Good documentation.The comment clarifies the routing behavior for agent invocations, which improves code readability and helps maintainers understand the integration with the agent flow system.
374-379: LGTM - Well-implemented UI skip logic.The guard safely skips UI rendering for agent tool calls using optional chaining and clear logging. The
_skipUIproperty access on theany-typedctxis justified given the documented MCP SDK version compatibility constraint (lines 32-33), and the property is consistently applied across the codebase (set in agent.instance.ts, checked here in the call-tool flow).libs/sdk/src/common/interfaces/agent.interface.ts (2)
181-229: Well-structured agent execution loop with clear callback hooks.The
runAgentLoopmethod provides a clean implementation with:
- Configurable callbacks for tool calls/results and content streaming
- Proper error propagation from the execution loop
- Fallback to
executeToolwhen notoolExecutoris provided
419-422:respondmethod correctly uses FlowControl for early termination.The pattern of setting output before throwing
FlowControl.respondensures state is captured before the control flow exception.libs/sdk/src/agent/agent.registry.ts (2)
242-247: Intentionalanyusage is documented.The comments explain that
anyis intentional for heterogeneous agent collections. Whileunknownwould be safer, the justification is reasonable for this use case.Also applies to: 252-255
433-442:getCapabilitiesfollows the guideline for dynamic capability exposure.The method correctly uses
getCapabilities()pattern for dynamic capability exposure as per coding guidelines.libs/sdk/src/common/metadata/app.metadata.ts (3)
95-101: Good documentation for the newagentsfield.The JSDoc clearly explains the purpose and behavior of autonomous AI agents, including how they're exposed as tools with the
use-agent:<agent_id>naming convention.
118-135: Correct Zod 4 usage withz.looseObjectandsatisfies.The migration to
z.looseObjectaligns with Zod 4 patterns per the library documentation. Thesatisfies RawZodShape<LocalAppMetadata>constraint maintains type safety between interface and schema.
184-195: Consistent schema pattern forRemoteAppMetadata.The remote app schema follows the same
z.looseObjectpattern, maintaining consistency across the codebase.libs/sdk/src/agent/index.ts (1)
1-95: Well-structured barrel file with comprehensive documentation.The module:
- Provides clear JSDoc with usage example
- Organizes exports into logical sections (Core, Flow, Hook, Adapter, Common types)
- Re-exports public API without legacy aliases
- Follows the coding guideline for barrel files
libs/sdk/src/common/metadata/agent.metadata.ts (2)
35-66: Well-designedWithConfigpattern for configuration resolution.The
WithConfig<T>type andwithConfig()factory provide a clean API for referencing configuration paths with optional transforms. This is a good pattern for flexible configuration.
282-395: ComprehensiveAgentMetadatainterface with good documentation.The interface provides:
- Clear JSDoc for each field with defaults noted
- Logical grouping of related configuration (LLM, swarm, execution, exports)
- Type-safe schema references for input/output
- Example usage in the interface docs
libs/sdk/src/agent/adapters/providers/index.ts (4)
1-41: LGTM! Clear interface definition and documentation.The interface and documentation are well-structured. The baseUrl parameter is properly documented as supported by all providers, which aligns with the implementation in lines 144-153.
43-82: LGTM! Excellent error handling with actionable guidance.The dynamic import function provides clear, actionable error messages that include installation commands. The validation at lines 58-61 properly checks for missing provider info before attempting the import.
117-157: LGTM! Past review issues have been properly addressed.The code now:
- Properly validates
providerInfoexists without using non-null assertions (lines 121-124)- Correctly passes
baseUrlto all optional providers with provider-specific property names (lines 144-153):
- Google:
baseUrl- Mistral:
endpoint- Groq:
configuration.baseURLThe error handling is comprehensive and the type assertion at line 155 is necessary for dynamic class loading.
159-178: LGTM! Clear error handling and reasonable synchronous adapter pattern.The default case provides a comprehensive error message, and the synchronous version uses a deferred loading pattern to support dynamic imports while maintaining a synchronous API.
libs/sdk/src/agent/adapters/base.adapter.ts (4)
1-55: LGTM! Well-documented configuration interface.The interface is comprehensive with clear documentation and sensible defaults for timeout (60000ms) and maxRetries (3).
74-98: LGTM! Comprehensive validation with proper type guards.The constructor validation properly checks:
- Required string fields are non-empty (lines 76-81)
- Numeric fields have correct type, are not NaN, and meet range constraints (lines 83-91)
The validation uses
Number.isNaN()correctly and addresses the previous review feedback about adding type guards for optional numeric fields.
100-149: LGTM! Clean abstraction with sensible defaults.The default
streamCompletionimplementation (lines 113-121) provides a reasonable fallback by wrapping non-streaming completions, and the abstract methods properly delegate provider-specific formatting to subclasses.
212-250: LGTM! Well-designed error hierarchy.The error classes provide:
- Specific error types for different failure scenarios (rate limiting, context length)
- Appropriate metadata (provider, code, statusCode)
- Rate limiting hints via
retryAfterparameter
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/sdk/src/tool/tool.registry.ts (1)
421-450: Consider adding scope validation for fail-fast error detection.The method documentation warns about scope binding issues (lines 403-416), but there's no runtime validation that the tool's captured scope matches this registry's scope. If a tool created with different providers is registered here, it will silently operate in the wrong scope context, potentially causing subtle bugs.
As per coding guidelines for
libs/sdk/src/**/*.ts: "Validate... and fail fast on invalid... configurations with specific error messages."🔎 Proposed validation to prevent scope mismatches
registerToolInstance(tool: ToolEntry): void { // Validate that we have a proper ToolInstance with required properties if (!(tool instanceof ToolInstance)) { throw new Error('registerToolInstance requires a ToolInstance, not a generic ToolEntry'); } const instance = tool; if (!instance.record || !instance.record.provide) { throw new Error('ToolInstance is missing required record.provide property'); } + + // Validate scope compatibility to prevent subtle bugs + const registryScope = this.providers.getActiveScope(); + if (instance.scope !== registryScope) { + throw new Error( + `Scope mismatch: ToolInstance was created with a different scope. ` + + `Create the tool with this registry's providers to ensure correct scope binding.` + ); + } const token = instance.record.provide as Token;Based on learnings: scope binding validation helps catch configuration errors early.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
libs/sdk/src/agent/flows/call-agent.flow.tslibs/sdk/src/tool/tool.instance.tslibs/sdk/src/tool/tool.registry.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/sdk/src/agent/flows/call-agent.flow.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use strict TypeScript mode with noanytypes without strong justification - useunknowninstead for generic type defaults
Avoid non-null assertions (!) - use proper error handling and throw specific errors instead
Use type parameters with constraints instead of unconstrained generics
Do not mutate rawInput in flows - use state.set() for managing flow state instead
Files:
libs/sdk/src/tool/tool.registry.tslibs/sdk/src/tool/tool.instance.ts
libs/sdk/src/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
libs/sdk/src/**/*.ts: MCP response types should use strict MCP protocol types (GetPromptResult, ReadResourceResult) instead ofunknown
Use specific error classes with MCP error codes instead of generic errors
UsegetCapabilities()method for dynamic capability exposure instead of hardcoding capabilities
UsechangeScopeproperty instead ofscopein change events to avoid confusion with Scope class
Validate URIs per RFC 3986 at metadata level using proper validation
Validate hook flows and fail fast on invalid hook configurations with specific error messages
Files:
libs/sdk/src/tool/tool.registry.tslibs/sdk/src/tool/tool.instance.ts
libs/**
⚙️ CodeRabbit configuration file
libs/**: Contains publishable SDK libraries. Review for API correctness, breaking changes, and consistency with docs. When public APIs change, ensure there is a matching docs/draft/docs/** update (not direct edits under docs/docs/**).
Files:
libs/sdk/src/tool/tool.registry.tslibs/sdk/src/tool/tool.instance.ts
🧠 Learnings (1)
📚 Learning: 2025-12-19T02:04:46.475Z
Learnt from: CR
Repo: agentfront/frontmcp PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-19T02:04:46.475Z
Learning: Applies to libs/sdk/src/**/*.ts : Use `changeScope` property instead of `scope` in change events to avoid confusion with Scope class
Applied to files:
libs/sdk/src/tool/tool.instance.ts
🧬 Code graph analysis (2)
libs/sdk/src/tool/tool.registry.ts (2)
libs/sdk/src/tool/tool.instance.ts (1)
ToolInstance(37-158)libs/sdk/src/common/entries/base.entry.ts (1)
EntryLineage(6-6)
libs/sdk/src/tool/tool.instance.ts (2)
libs/sdk/src/provider/provider.registry.ts (1)
ProviderRegistry(27-978)libs/sdk/src/scope/scope.instance.ts (1)
Scope(36-360)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Lint & Format Checks
- GitHub Check: Build Libraries
🔇 Additional comments (1)
libs/sdk/src/tool/tool.instance.ts (1)
29-48: Excellent documentation and scope binding design.The class-level documentation clearly explains that ToolInstance captures its scope and providers at construction time, with concrete examples showing correct vs. incorrect usage patterns. The readonly field declarations enforce immutability of the captured context, preventing accidental scope changes during the tool's lifetime.
The explicit scope binding design helps prevent subtle bugs where tools might inadvertently operate in the wrong scope context.
… ignored dependencies
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.